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

[Fabiano Fidêncio]

On Tue, Sep 21, 2010 at 8:53 PM, Gustavo F. Padovan
<padovan at profusion.mobi> wrote:
> * 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) ....
>

Just following Lennart's style (see in mount.c)

>> +                }
>> +
>> +                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);
>
>

Agree.

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

... Lennart's style ...

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

No. See when cunescape fail ...

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



-- 
Fabiano Fidêncio
ProFUSION embedded systems
http://www.profusion.mobi