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

[Lennart Poettering]

On Thu, 30.09.10 18:10, fidencio at profusion.mobi (fidencio at profusion.mobi) wrote:

A last round of comments:

> +int umount_init(void) {
> +        LIST_HEAD(MountPoint, mp_list_head);
> +        int r;
> +        bool changed;

This isn't really an "initialization", is it? I'd prefer the name umount_all().

> +
> +        /* Preventing that we won't block umounts */
> +        if ((chdir("/")) == -1)
> +                return -1;

In systemd we use negative errno error codes, like the kernel (also see
CODING_STYLE). In this case:

if (chdir("/") < 0)
        return -errno;

> +
> +        LIST_HEAD_INIT(MountPoint, mp_list_head);
> +        if (mount_points_list_get(&mp_list_head) < 0)
> +                return -1;

Same thing here.

if ((r = mount_points_list_get(&mp_list_head)) < 0)
        return r;

(Assuming m_p_l_g() returns an errno error code, too)

> +
> +        for (changed = false; changed;)
> +                mount_points_list_umount(&mp_list_head, &changed);
> +
> +        for (changed = false; changed;)
> +                r = mount_points_list_remount_read_only(&mp_list_head, &changed);
> +
> +        mount_points_list_free(&mp_list_head);
> +
> +        return r;

I think it would make sense to return the last error we
encounter. Currently if a later mount_points_list_remount_read_only()
succeeds an earlier error is ignored.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.