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

[Gustavo F. Padovan]

* Fabiano Fidencio <fidencio at profusion.mobi> [2010-09-21 20:17:06 -0300]:

> This function will unmount all filesystem (and those which fail
> will be remounted read-only).
> Filesystems are being read from /proc/self/mountinfo.
> ---
>  src/shutdownd.c |   73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 73 insertions(+), 0 deletions(-)
> 
> diff --git a/src/shutdownd.c b/src/shutdownd.c
> index 72a2801..d47b160 100644
> --- a/src/shutdownd.c
> +++ b/src/shutdownd.c
> @@ -23,6 +23,7 @@
>  #include <sys/poll.h>
>  #include <sys/types.h>
>  #include <sys/timerfd.h>
> +#include <sys/mount.h>
>  #include <assert.h>
>  #include <string.h>
>  #include <errno.h>
> @@ -36,6 +37,78 @@
>  #include "sd-daemon.h"
>  #include "utmp-wtmp.h"
>  
> +static int umount_proc_self_mountinfo(void) {
> +        FILE *proc_self_mountinfo;
> +        int r;
> +        char *device, *path, *d, *p;
> +
> +        if (!(proc_self_mountinfo = fopen("/proc/self/mountinfo", "re")))
> +                return -errno;
> +
> +        for (;;) {
> +                int k;
> +
> +                device = path = d = p = NULL;
> +
> +                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 */
> +                                "%ms"        /* (10) mount source */
> +                                "%*s"        /* (11) mount options 2 */
> +                                "%*[^\n]",   /* some rubbish at the end */
> +                                &path,
> +                                &device)) != 2) {
> +
> +                        if (k == EOF)
> +                                break;
> +
> +                        r = -EBADMSG;
> +                        goto finish;
> +                }
> +
> +                if (!(d = cunescape(device)) ||
> +                    !(p = cunescape(path))) {
> +                        r = -ENOMEM;
> +                        goto finish;

I would prefer:
		d = cunescape(...
		p = cunescape(...
		if (!d || !p) ....

> +                }
> +
> +                if ((r = umount(p) < 0))
> +                        if ((r = umount2(p, MNT_FORCE)) < 0) {
> +                                log_warning("umount: forced umount of %s failed! Trying to remount read-only.\n", p);
> +                                if ((r = mount(d, p, NULL, MS_MGC_VAL|MS_REMOUNT|MS_RDONLY, NULL)) < 0)
> +                                        log_error("umount: Cannot remount %s read-only\n", d);

What are trying to do with 3 nested ifs? Make your code unreadable?
So lets try a different approach here:


		 if ((r = umount(p)))
			goto free;

                if ((r = umount2(p, MNT_FORCE)))
			goto free;
		else
			log_warning("umount: forced umount of %s failed! \
			Trying to remount read-only.\n", p);


                if ((r = mount(d, p, NULL, MS_MGC_VAL|MS_REMOUNT|MS_RDONLY, NULL)))
			goto free;
		else
	                log_error("umount: Cannot remount %s read-only\n", d);

free:

                free(device);
                free(path);
                free(d);
                free(p);


> +                                else
> +                                        log_warning("umount: %s busy - remounted read-only\n", d);
> +                        } else
> +                                log_warning("umount: %s was umounted succesfully\n", p);
> +                else
> +                        log_warning("umount: %s was umounted succesfully\n", p);
> +
> +                free(device);
> +                free(path);
> +                free(d);
> +                free(p);
> +        }
> +
> +        r = 0;

If you initialize r with 0 you won't need this line. Then you can remove
the finish label and just use 'break' instead of 'goto'

> +
> +finish:
> +        free(device);
> +        free(path);
> +        free(d);
> +        free(p);

I think you don't need free(d) and free(p) here, looking to the code
flow they seems to be always NULL here.

> +        fclose(proc_self_mountinfo);
> +
> +        return r;
> +}
> +
>  static int read_packet(int fd, struct shutdownd_command *_c) {
>          struct msghdr msghdr;
>          struct iovec iovec;
> -- 
> 1.7.2.3
> 
> -- 
> ProFUSION embedded systems - Code Review
> http://groups.google.com/group/profusion-review
> ALL CONTENT IS CONFIDENTIAL AND COPYRIGHTED TO PROFUSION

-- 
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi