On Thu, 21.08.14 19:07, har...@redhat.com (har...@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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel