[systemd-devel] [PATCH 1/2] Adding unmount functions to be used in shutdown

[Gustavo Sverzut Barbieri]

On Wed, Oct 6, 2010 at 10:09 AM, Lennart Poettering
<lennart at poettering.net> wrote:
> On Wed, 06.10.10 02:05, Gustavo Sverzut Barbieri (barbieri at profusion.mobi) wrote:
>
>> +#define LOOP_CLR_FD     0x4C01
>
> Is there any particular reason you define this here? To me it appears
> that linux/loop.h is perfectly fit to be included here.

not really, fidencio did it and i just kept... probably he found that
in some exiting tool. will send a new fixed version.


>> +
>> +        if ((dir = opendir("/sys/class/block")) == NULL)
>> +                return -errno;
>
> Kay, should we use libudev for this? Right now we use libudev for all
> accesses to /sys, should we do that here, too?

I just did that because the original version was looking for /dev/*
and filtering loop devices, too much to do when we could list all
block devices and just get the loop :-)  I never used libudev, but if
you wish I can move to use it.


>> +
>> +        while ((d = readdir(dir))) {
>> +                MountPoint *lb;
>> +                char buf[PATH_MAX];
>> +                char *loop;
>> +
>> +                if (!strneq(d->d_name, "loop", 4))
>> +                        continue;
>> +
>> +                snprintf(buf, sizeof(buf), "/dev/%s", d->d_name);
>> +                if (access(buf, R_OK) != 0)
>> +                        continue;
>
> Hmm, what is this access() call good for? Calls to access() are more
> often than not an indication for racy code? Can't this check be dropped
> without ill effects? (if it cannot be dropped, then a comment would be cool.)

not really useful, it is just to avoid cases whenever we have the loop
in /sys but no counterpart in /dev, it is quite weird corner case,
yeah... so I'll remove it if you wish.


>> +        if ((fd = open(device, O_RDONLY)) < 0)
>> +                return -errno;
>
> More in the category of nitpicking, but we generally add O_CLOEXEC to
> all open() calls. (Its not strictly needed in this case, but it's handy
> to avoid false positives when grepping the sources to find open()s that
> don't specifiy O_CLOEXEC, which I do from time to time). I wonder
> whether O_NONBLOCK might be a good idea.

we're just closing it immediately below :-) and O_NONBLOCK is not
really helpful as we're doing just these tasks... as far as I checked
the kernel side is pretty simple and just check few flags in order to
reply.


>> +
>> +        ioctl(fd, LOOP_CLR_FD, 0);
>> +        r = errno;
>> +        close_nointr(fd);
>> +
>> +        if (r == ENXIO) /* not bound, so no error */
>> +                r = 0;
>> +        errno = r;
>> +        return -errno;
>> +}
>
> Might be good to print an error message here.

In the ENXIO case? or others? The ENXIO will happen if you do not have
it in use, so in my machine it will always happen 8 times.


-- 
Gustavo Sverzut Barbieri
http://profusion.mobi embedded systems
--------------------------------------
MSN: barbieri at gmail.com
Skype: gsbarbieri
Mobile: +55 (19) 9225-2202