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

[Lennart Poettering]

On Sun, 26.09.10 16:55, Fabiano Fidencio (fidencio at profusion.mobi) wrote:

> +typedef struct MountPoint {
> +        char *path;
> +        char *device;
> +        char *fstype;
> +        struct MountPoint *next;
> +} MountPoint;

Please avoid creating your own linked list implementations. Please use
the LIST_xxx macros from list.h instead!

> +typedef MountPoint * MountPointsList;

Please don't define pointer types.

> +        do {
> +                /* Trying to umount */
> +                if (umount(mp->path) == 0) {
> +                        log_info("umount: %s was umounted succesfully\n", mp->path);
> +                        goto success;
> +                }

I think it would be smart not trying to unmount /. It won't work anyway,
and so I think it makes sense to go for ro mounting for / right-away.

> +                /* Forcing to umount if busy (only for NFS mounts) */
> +                if (umount2(mp->path, MNT_FORCE) == 0) {
> +                        log_info("umount: %s was umounted succesfully\n", mp->path);
> +                        goto success;
> +                }

Why do you try first without and then with MNT_FORCE? Why not go for
MNT_FORCE right-away?

I'd prefer to downgrade the log messages to debug if possible. Log
messages from inner loops should probably not show up by default.

> +
> +                /* Trying to remount read-only */
> +                log_warning("umount: forced umount of %s failed! Trying to remount read-only.\n", mp->path);
> +                if (mount(mp->device, mp->path, mp->fstype, MS_MGC_VAL|MS_REMOUNT|MS_RDONLY, NULL) == 0)
> +                        log_warning("umount: %s busy - remounted
> read-only\n", mp->device);

AFAIR you don't have to specify the device when you remount something.

Otherwise looks fine!

Thanks,

Lennart

-- 
Lennart Poettering - Red Hat, Inc.