[Toybox] [PATCH] Fix switch_root implementation.

[Isaac Dunham]

On Tue, May 05, 2015 at 03:15:38PM -0700, Alistair Strachan wrote:
> Add the MS_MOVE of cwd to / and chroot into it. We don't need to
> chdir after the chroot because xchroot already does this for us.

It seems there was nothing updating the root filesystem at all.

I have some comments and questions about this:
* Using xchroot() breaks -h.
* Why do we need to both mount --move and chroot?
  (If I read comments elsewhere correctly, the answer is
   - mount() updates the mount table
   - chroot() updates the root
   - chdir() updates the cwd)
* It would be nice to have comments indicating all of that,
  and pointing out that we need the chdir() before mount() so no one
  looks at this and thinks they can "simplify"

> The switch_root toy was also blocking any case where NEW_ROOT/init
> did not exist, even though NEW_INIT was a required parameter and
> did not have to be '/init'. Change it to handle any NEW_INIT
> passed as either a relative or absolute path.

>  toys/other/switch_root.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/toys/other/switch_root.c b/toys/other/switch_root.c
> index 0861c70..224fdb1 100644
> --- a/toys/other/switch_root.c
> +++ b/toys/other/switch_root.c
> @@ -43,7 +43,7 @@ static int del_node(struct dirtree *node)
>  
>  void switch_root_main(void)
>  {
> -  char *newroot = *toys.optargs, **cmdline = toys.optargs+1;
> +  char *newroot = *toys.optargs, **cmdline = toys.optargs+1, *rel_cmdline;
>    struct stat st1, st2;
>    struct statfs stfs;
>    int console = console; // gcc's "may be used" warnings are broken.
> @@ -68,7 +68,10 @@ void switch_root_main(void)
>    TT.rootdev=st2.st_dev;
>  
>    // init program must exist and be an executable file
> -  if (stat("init", &st1) || !S_ISREG(st1.st_mode) || !(st1.st_mode&0100)) {
> +  rel_cmdline = *cmdline[0] == '/' ? *cmdline+1 : *cmdline;

Rob has the final say, but it took me a minute to figure out what this
line is supposed to mean.

I would find it a little bit clearer if there were parens around either
the comparison or the whole right side.

Also, while it's not standard usage, '//' on Linux is valid;
this code would make it act more typically:
  rel_cmdline = *cmdline;
  while (rel_cmdline == '/') rel_cmdline++;
or
  while (**cmdline == '/') (*cmdline)++;

> +  if (stat(rel_cmdline, &st1) || !S_ISREG(st1.st_mode) ||
> +    !(st1.st_mode&0100))
> +  {
>      error_msg("bad init");
>      goto panic;
>    }
> @@ -81,6 +84,13 @@ void switch_root_main(void)
>    // Ok, enough safety checks: wipe root partition.
>    dirtree_read("/", del_node);
>  
> +  // Move the newroot to the old root and enter it
> +  if (mount(".", "/", NULL, MS_MOVE, NULL)) {
> +    error_msg("mount(.., MS_MOVE, ..) failed");
> +    goto panic;
> +  }
> +  xchroot(".");

 1430948316.0