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

[Lennart Poettering]

On Mon, 27.09.10 18:34, Fabiano Fidencio (fidencio at profusion.mobi) wrote:

> +
> +static void mount_points_list_free(MountPoint **mp_list_head) {
> +        MountPoint *mp, *mp_next;
> +        LIST_FOREACH_SAFE(mount_point, mp, mp_next, *mp_list_head)
> +                mount_point_remove_and_free(mp, mp_list_head);
> +}

I'd just do 

while (*list)
      mount_point_remove_and_free(*list, *list)

Looks more readable to me.

> +                if ((k = fscanf(proc_self_mountinfo,
> +                                "%*s "       /* (1) mount id */
> +                                "%*s "       /* (2) parent id */
> +                                "%*s "       /* (3) major:minor */
> +                                "%*s "       /* (4) root */
> +                                "%ms "       /* (5) mount point */
> +                                "%*s"        /* (6) mount options */
> +                                "%*[^-]"     /* (7) optional fields */
> +                                "- "         /* (8) seperator */
> +                                "%*s "       /* (9) file system type */
> +                                "%*s"        /* (10) mount source */
> +                                "%*s"        /* (11) mount options 2 */
> +                                "%*[^\n]",   /* some rubbish at the end */
> +                                &path)) != 1) {
> +                        if (k == EOF)
> +                                break;
> +
> +                        r = -EBADMSG;
> +                        goto finish;
> +                }

Thinking about this we probably should just go on with the next line
when we fail to parse something, maybe with a warning. I figure mount.c
exits here too, but I guess we should fix that.

> +
> +                if (mount_point_is_api(path))
> +                        goto free;
> +
> +                if (!(p = cunescape(path))) {
> +                        r = -ENOMEM;
> +                        free(p);
> +                        goto finish;
> +                }
> +
> +                mp = mount_point_alloc(p);

You need to check for OOM here!

> +
> +        LIST_FOREACH_SAFE(mount_point, mp, mp_next, *mp_list_head) {
> +                if (!(strcmp(mp->path, "/")))
> +                        continue;

We have this little macro streq() which is a bit nicer to read.

> +
> +                /* Trying to umount. Forcing to umount if busy (only for NFS mounts) */
> +                if (umount2(mp->path, MNT_FORCE) == 0) {
> +                        if (!(*changed))
> +                                *changed = true;

Hmm, why do you test *changed first?

> +                        mount_point_remove_and_free(mp, mp_list_head);
> +                        log_debug("umount: \"%s\" was umounted succesfully\n", mp->path);

> +
> +                } else
> +                        log_debug("umount: forced umount of \"%s\"
> failed!\n", mp->path);

Please output error message here, and do so as log_warn(). Use %m.

> +                if (mount(NULL, mp->path, NULL, MS_MGC_VAL|MS_REMOUNT|MS_RDONLY, NULL) == 0) {
> +                        mp->read_only = true;
> +                        if (!(*changed))
> +                                *changed = true;
> +                        mount_point_remove_and_free(mp, mp_list_head);
> +                        log_debug("umount: \"%s\" busy - remounted read-only\n", mp->path);
> +                } else {
> +                        log_error("Cannot remount \"%s\" read-only\n", mp->path);
> +                        sleep(1);

Similar here.

Otherwise looks fine!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.