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

[Lennart Poettering]

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.

> +
> +        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?

> +
> +        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.)

> +        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.

> +
> +        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.

Otherwise really cool. One last round of fixing and I'll merge it!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.