[systemd-devel] [PATCH V4] use the switch_root function in shutdown

[Lennart Poettering]

On Thu, 21.08.14 19:07, harald at redhat.com (harald at redhat.com) wrote:

>                          broadcast_signal(SIGTERM, false, true);
>  
>                          /* And switch root */
> -                        r = switch_root(switch_root_dir);
> +                        r = switch_root(switch_root_dir, "/mnt",
> true, true);


I think it would be good to add a comment that explains why MS_MOVE is
used sometimes but MS_BIND otheriwse...

>                  if (prepare_new_root() >= 0 &&
> -                    pivot_to_new_root() >= 0) {
> +                    switch_root("/run/initramfs", "/oldroot", false, false) >= 0) {
>                          arguments[0] = (char*) "/shutdown";

Similar here... 


> -int switch_root(const char *new_root) {
> +int switch_root(const char *new_root, const char *oldroot, bool detach_oldroot, bool mount_move) {
>  

(see below...)

>  
> -                if (mount(i, new_mount, NULL, MS_MOVE, NULL) < 0) {
> -                        log_error("Failed to move mount %s to %s, forcing unmount: %m", i, new_mount);
> -
> -                        if (umount2(i, MNT_FORCE) < 0)
> -                                log_warning("Failed to unmount %s: %m", i);
> +                if (mount(i, new_mount, NULL, mount_move ? MS_MOVE : MS_BIND, NULL) < 0) {

I think it would be more readable if we'd just pass the the flags to use
as "unsigned long" into the function thatn to make this into a bool. For
the caller it should then be a lot more useful to understand I think....

i.e. the "bool mount_move" function argument should better become an
"unsigned long flags", or so, that we just pass to mount()?

> +                        if (mount_move) {
> +                                log_error("Failed to move mount %s to %s, forcing unmount: %m",
> +                                          i, new_mount);
> +
> +                                if (umount2(i, MNT_FORCE) < 0)
> +                                        log_warning("Failed to unmount %s: %m", i);
> +                        } else {
> +                                log_error("Failed to bind mount %s to %s: %m",
> +                                          i, new_mount);
> +                        }


We don't break at 80ch. I mean, it's not super important and we should
not stop doing line breaks at all, but people certainly have bigger
screens than 80ch right now. So please use 140ch or so, but don't break
it so eagerly, I don't think it helps readability...

also our coding style is

        ...
        else 
                log_error(...);
        ... 

rather than:

        ...
        else {
                log_error(...);
        }
        ... 

For single-line if/else blocks...

Otherwise looks good!

Lennart

-- 
Lennart Poettering, Red Hat