Libguestfs - Jun 2016 - Re: [PATCH v2] rescue: add --autosysroot option RHBZ#1183493

--autosysroot option uses suggestions to user on how to mount filesystems
and change root suggested by --suggest option in virt-rescue.

Commands are passed on kernel command line in format
guestfs_command=command;. Command ends with a semicolon and there can be
multiple commands specified. These are executed just before bash starts.
On successfull run user is presented directly with bash in chroot
environment.

Resolves RFE: RHBZ#1183493

Maros Zatko (1):
  rescue: add --autosysroot option RHBZ#1183493

 appliance/init  |   5 ++
 rescue/rescue.c | 169 +++++++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 143 insertions(+), 31 deletions(-)

-- 
2.5.5

--autosysroot option uses suggestions to user on how to mount filesystems
and change root suggested by --suggest option in virt-rescue.

Commands are passed on kernel command line in format
guestfs_command=command;. Command ends with a semicolon and there can be
multiple commands specified. These are executed just before bash starts.
On successfull run user is presented directly with bash in chroot
environment.

Resolves RFE: RHBZ#1183493
---
 appliance/init  |   5 ++
 rescue/rescue.c | 169 +++++++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 143 insertions(+), 31 deletions(-)

diff --git a/appliance/init b/appliance/init
index 3fdf4d0..0a76398 100755
--- a/appliance/init
+++ b/appliance/init
@@ -192,6 +192,11 @@ else
   echo "You have to mount the guest's partitions under /sysroot"
   echo "before you can examine them."
   echo
+  echo 'Executing commands from kernel cmdline'
+
+  grep -Eo 'guestfs_command=([^;]+);[[:space:]]*' /proc/cmdline | sed
-n 's/guestfs_command=\(.*;\)/\1/p'
+  eval $(grep -Eo 'guestfs_command=([^;]+);[[:space:]]*' /proc/cmdline
| sed -n 's/guestfs_command=\(.*;\)/\1/p')
+
   bash -i
   echo
   echo "virt-rescue: Syncing the disk now before exiting ..."
diff --git a/rescue/rescue.c b/rescue/rescue.c
index 135c9e6..77b6544 100644
--- a/rescue/rescue.c
+++ b/rescue/rescue.c
@@ -37,7 +37,7 @@
 #include "options.h"
 
 static void add_scratch_disks (int n, struct drv **drvs);
-static void do_suggestion (struct drv *drvs);
+static char ** do_suggestion (struct drv *drvs);
 
 /* Currently open libguestfs handle. */
 guestfs_h *g;
@@ -50,6 +50,9 @@ int echo_keys = 0;
 const char *libvirt_uri = NULL;
 int inspector = 0;
 
+static void use_suggestions (char **cmds);
+static void parse_opts(int argc, char * argv[]);
+
 static void __attribute__((noreturn))
 usage (int status)
 {
@@ -65,6 +68,8 @@ usage (int status)
               "Options:\n"
               "  -a|--add image       Add image\n"
               "  --append kernelopts  Append kernel options\n"
+              "  --autosysroot        Automatically use suggested mount
commands\n"
+              "                       and chroot to guest. Needs --suggest
to work\n"
               "  -c|--connect uri     Specify libvirt URI for -d
option\n"
               "  -d|--domain guest    Add disks from libvirt guest\n"
               "  --format[=raw|..]    Force disk format for -a
option\n"
@@ -87,21 +92,30 @@ usage (int status)
   exit (status);
 }
 
-int
-main (int argc, char *argv[])
-{
-  setlocale (LC_ALL, "");
-  bindtextdomain (PACKAGE, LOCALEBASEDIR);
-  textdomain (PACKAGE);
-
-  parse_config ();
+struct drv *drvs = NULL;
+struct drv *drv;
+char **cmds = NULL;
+const char *format = NULL;
+bool format_consumed = true;
+int c;
+int option_index;
+int network = 0;
+char *append = NULL;
+int memsize = 0;
+int smp = 0;
+int suggest = 0;
+int autosysroot = 0;
 
+static void
+parse_opts(int argc, char * argv[])
+{
   enum { HELP_OPTION = CHAR_MAX + 1 };
 
   static const char *options = "a:c:d:m:rvVx";
   static const struct option long_options[] = {
     { "add", 1, 0, 'a' },
     { "append", 1, 0, 0 },
+    { "autosysroot", 0, 0, 0 },
     { "connect", 1, 0, 'c' },
     { "domain", 1, 0, 'd' },
     { "format", 2, 0, 0 },
@@ -120,22 +134,10 @@ main (int argc, char *argv[])
     { "version", 0, 0, 'V' },
     { 0, 0, 0, 0 }
   };
-  struct drv *drvs = NULL;
-  struct drv *drv;
-  const char *format = NULL;
-  bool format_consumed = true;
-  int c;
-  int option_index;
-  int network = 0;
-  const char *append = NULL;
-  int memsize = 0;
-  int smp = 0;
-  int suggest = 0;
 
-  g = guestfs_create ();
-  if (g == NULL)
-    error (EXIT_FAILURE, errno, "guestfs_create");
 
+  option_index = 0;
+  optind = 0;
   for (;;) {
     c = getopt_long (argc, argv, options, long_options, &option_index);
     if (c == -1) break;
@@ -150,7 +152,9 @@ main (int argc, char *argv[])
         if (guestfs_set_selinux (g, 1) == -1)
           exit (EXIT_FAILURE);
       } else if (STREQ (long_options[option_index].name, "append")) {
-        append = optarg;
+        append = strdup (optarg);
+      } else if (STREQ (long_options[option_index].name,
"autosysroot")) {
+        autosysroot = 1;
       } else if (STREQ (long_options[option_index].name, "network"))
{
         network = 1;
       } else if (STREQ (long_options[option_index].name, "format")) {
@@ -259,10 +263,71 @@ main (int argc, char *argv[])
     }
   }
 
+}
+
+int
+main (int argc, char *argv[])
+{
+  setlocale (LC_ALL, "");
+  bindtextdomain (PACKAGE, LOCALEBASEDIR);
+  textdomain (PACKAGE);
+
+  parse_config ();
+  g = guestfs_create ();
+  if (g == NULL)
+    error (EXIT_FAILURE, errno, "guestfs_create");
+
+  parse_opts (argc, argv);
+
   /* --suggest flag */
   if (suggest) {
-    do_suggestion (drvs);
-    exit (EXIT_SUCCESS);
+    cmds = do_suggestion (drvs);
+    if (!autosysroot) {
+      exit (EXIT_SUCCESS);
+    } else {
+      /* Shut down libguestfs so we can start a new one */
+      guestfs_shutdown (g);
+      guestfs_close (g);
+
+      /* remove --suggest flag */
+      size_t newcount = argc;
+      for (int i = 0; i < argc; i++) {
+        if (strcmp (argv[i], "--suggest") == 0) {
+          newcount--;
+        }
+      }
+
+      char ** args = malloc (sizeof (char*) * (newcount));
+      for (int i = 0, j = 0; i < argc; i++) {
+        if (strcmp (argv[i], "--suggest") != 0) {
+          args[j] = strdup (argv[i]);
+          j++;
+        }
+      }
+
+      /* clear options */
+      drvs = NULL;
+      format_consumed = true;
+      c = 0;
+      option_index = 0;
+      network = 0;
+      if (append) free(append);
+      memsize = 0;
+      smp = 0;
+      suggest = 0;
+      autosysroot = 0;
+
+      /* Create new guestfs handle, this time for rescue instead of OS
detection */
+      g = guestfs_create ();
+      /* Parse options with --suggest flag removed */
+      parse_opts (newcount, args);
+      argc = newcount;
+    }
+  } else {
+    if (autosysroot) {
+      fprintf (stderr, _("%s: warning: --autosysroot used without
--suggest.\n"),
+          guestfs_int_program_name);
+    }
   }
 
   /* These are really constants, but they have to be variables for the
@@ -313,6 +378,11 @@ main (int argc, char *argv[])
     }
   }
 
+  /* Now it's time to set suggestions. */
+  if (autosysroot) {
+    use_suggestions (cmds);
+  }
+
   /* Set other features. */
   if (memsize > 0)
     if (guestfs_set_memsize (g, memsize) == -1)
@@ -367,14 +437,34 @@ compare_keys_len (const void *p1, const void *p2)
   return strlen (key1) - strlen (key2);
 }
 
+/* Use autodetected suggested filesystems */
+static void
+use_suggestions (char **cmds)
+{
+  size_t i;
+  if (cmds == NULL) return;
+  read_only = 0;
+
+  /* Craft kernel command line from suggested commands */
+  for (i = 0; cmds[i] != NULL; i++) {
+    char *old_append = append;
+    append = xasprintf ("%s guestfs_command=%s;", append == NULL?
"" : append, cmds[i]);
+    if (old_append != NULL)
+      free (old_append);
+  }
+}
+
 /* virt-rescue --suggest flag does a kind of inspection on the
  * drives and suggests mount commands that you should use.
  */
-static void
+static char **
 do_suggestion (struct drv *drvs)
 {
   CLEANUP_FREE_STRING_LIST char **roots = NULL;
   size_t i;
+  char **cmds = malloc (sizeof (char*));
+  cmds[0] = NULL;
+
 
   /* For inspection, force add_drives to add the drives read-only. */
   read_only = 1;
@@ -401,7 +491,7 @@ do_suggestion (struct drv *drvs)
 
   if (roots[0] == NULL) {
     suggest_filesystems ();
-    return;
+    return NULL;
   }
 
   printf (_("This disk contains one or more operating systems.  You can
use these mount\n"
@@ -436,10 +526,16 @@ do_suggestion (struct drv *drvs)
     qsort (mps, guestfs_int_count_strings (mps) / 2, 2 * sizeof (char *),
            compare_keys_len);
 
-    for (j = 0; mps[j] != NULL; j += 2)
-      printf ("mount %s /sysroot%s\n", mps[j+1], mps[j]);
+    /* First we save commands to mount system root. */
+    for (j = 0; mps[j] != NULL; j += 2) {
+      int cnt = guestfs_int_count_strings (cmds);
+      cmds = realloc (cmds, sizeof (char *) * (cnt + 2));
+
+      cmds[cnt] = xasprintf ("mount %s /sysroot%s", mps[j+1],
mps[j]);
+      cmds[cnt+1] = NULL;
+    }
 
-    /* If it's Linux, print the bind-mounts and a chroot command. */
+    /* If it's Linux, print the bind-mounts and a chroot command and save
them. */
     if (type && STREQ (type, "linux")) {
       printf ("mount --rbind /dev /sysroot/dev\n");
       printf ("mount --rbind /proc /sysroot/proc\n");
@@ -447,10 +543,21 @@ do_suggestion (struct drv *drvs)
       printf ("\n");
       printf ("cd /sysroot\n");
       printf ("chroot /sysroot\n");
+
+      int cnt = guestfs_int_count_strings (cmds);
+      cmds = realloc (cmds, sizeof (char *) * (cnt + 6));
+
+      cmds[cnt+0] = xasprintf ("mount --rbind /dev /sysroot/dev");
+      cmds[cnt+1] = xasprintf ("mount --rbind /proc /sysroot/proc");
+      cmds[cnt+2] = xasprintf ("mount --rbind /sys /sysroot/sys");
+      cmds[cnt+3] = xasprintf ("cd /sysroot");
+      cmds[cnt+4] = xasprintf ("chroot /sysroot");
+      cmds[cnt+5] = NULL;
     }
 
     printf ("\n");
   }
+  return cmds;
 }
 
 /* Inspection failed, so it doesn't contain any OS that we recognise.
-- 
2.5.5

In data mercoledì 1 giugno 2016 02:04:33, Maros Zatko ha
scritto:
> --autosysroot option uses suggestions to user on how to mount filesystems
> and change root suggested by --suggest option in virt-rescue.
IMHO it should be called -i, like in the other tools, as what
--autosysroot does is basically the same.
> Commands are passed on kernel command line in format
> guestfs_command=command;. Command ends with a semicolon and there can be
> multiple commands specified. These are executed just before bash starts.
Passing arbitrary commands might not be the best idea ever -- other than
potentially running into issues such as quoting and escaping, the length
of the boot command line of the Linux kernel is limited [1], and we are
already using more than 200 characters.

An alternative approach with the cmdline could be passing like
guestfs_mount=/dev:/mntpoint:opts, which is slightly more complex to
parse, although provide a more strict interface and requires no eval.

[1] https://www.kernel.org/doc/Documentation/kernel-parameters.txt
> On successfull run user is presented directly with bash in chroot
> environment.
I'd not do that -- one of the way virt-rescue is useful is work on
the guest using the tools available in the appliance, so having an
an option to mount all the filesystems while still be in the appliance
would be the optimal thing to do IMHO.

In this situation, printing a "you can chroot in the guest by
running ..." message just before running bash would help too.
> Resolves RFE: RHBZ#1183493
> ---
>  appliance/init  |   5 ++
>  rescue/rescue.c | 169
+++++++++++++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 143 insertions(+), 31 deletions(-)
> 
> diff --git a/appliance/init b/appliance/init
> index 3fdf4d0..0a76398 100755
> --- a/appliance/init
> +++ b/appliance/init
> @@ -192,6 +192,11 @@ else
>    echo "You have to mount the guest's partitions under
/sysroot"
>    echo "before you can examine them."
>    echo
> +  echo 'Executing commands from kernel cmdline'
> +
> +  grep -Eo 'guestfs_command=([^;]+);[[:space:]]*' /proc/cmdline |
sed -n 's/guestfs_command=\(.*;\)/\1/p'
> +  eval $(grep -Eo 'guestfs_command=([^;]+);[[:space:]]*'
/proc/cmdline | sed -n 's/guestfs_command=\(.*;\)/\1/p')
> +
>    bash -i
>    echo
>    echo "virt-rescue: Syncing the disk now before exiting ..."
> diff --git a/rescue/rescue.c b/rescue/rescue.c
> index 135c9e6..77b6544 100644
> --- a/rescue/rescue.c
> +++ b/rescue/rescue.c
> @@ -37,7 +37,7 @@
>  #include "options.h"
>  
>  static void add_scratch_disks (int n, struct drv **drvs);
> -static void do_suggestion (struct drv *drvs);
> +static char ** do_suggestion (struct drv *drvs);
>  
>  /* Currently open libguestfs handle. */
>  guestfs_h *g;
> @@ -50,6 +50,9 @@ int echo_keys = 0;
>  const char *libvirt_uri = NULL;
>  int inspector = 0;
>  
> +static void use_suggestions (char **cmds);
> +static void parse_opts(int argc, char * argv[]);
> +
>  static void __attribute__((noreturn))
>  usage (int status)
>  {
> @@ -65,6 +68,8 @@ usage (int status)
>                "Options:\n"
>                "  -a|--add image       Add image\n"
>                "  --append kernelopts  Append kernel options\n"
> +              "  --autosysroot        Automatically use suggested
mount commands\n"
> +              "                       and chroot to guest. Needs
--suggest to work\n"
Hm I see --suggest and --autosysroot as separate modes:

* --suggests inspects the guest, prints all the mount commands for the
guest, and exits

* --autosysroot starts the appliance, and mounts all the filesystems,
letting the user continue with the rest of the wanted work
>                "  -c|--connect uri     Specify libvirt URI for -d
option\n"
>                "  -d|--domain guest    Add disks from libvirt
guest\n"
>                "  --format[=raw|..]    Force disk format for -a
option\n"
> @@ -87,21 +92,30 @@ usage (int status)
>    exit (status);
>  }
>  
> -int
> -main (int argc, char *argv[])
> -{
> -  setlocale (LC_ALL, "");
> -  bindtextdomain (PACKAGE, LOCALEBASEDIR);
> -  textdomain (PACKAGE);
> -
> -  parse_config ();
> +struct drv *drvs = NULL;
> +struct drv *drv;
> +char **cmds = NULL;
> +const char *format = NULL;
> +bool format_consumed = true;
> +int c;
> +int option_index;
> +int network = 0;
> +char *append = NULL;
> +int memsize = 0;
> +int smp = 0;
> +int suggest = 0;
> +int autosysroot = 0;
Any reason these are turned into global variables?
> +static void
> +parse_opts(int argc, char * argv[])
> +{
Not a problematic change, but I'd do it in a separate patch.
>    enum { HELP_OPTION = CHAR_MAX + 1 };
>  
>    static const char *options = "a:c:d:m:rvVx";
>    static const struct option long_options[] = {
>      { "add", 1, 0, 'a' },
>      { "append", 1, 0, 0 },
> +    { "autosysroot", 0, 0, 0 },
>      { "connect", 1, 0, 'c' },
>      { "domain", 1, 0, 'd' },
>      { "format", 2, 0, 0 },
> @@ -120,22 +134,10 @@ main (int argc, char *argv[])
>      { "version", 0, 0, 'V' },
>      { 0, 0, 0, 0 }
>    };
> -  struct drv *drvs = NULL;
> -  struct drv *drv;
> -  const char *format = NULL;
> -  bool format_consumed = true;
> -  int c;
> -  int option_index;
> -  int network = 0;
> -  const char *append = NULL;
> -  int memsize = 0;
> -  int smp = 0;
> -  int suggest = 0;
>  
> -  g = guestfs_create ();
> -  if (g == NULL)
> -    error (EXIT_FAILURE, errno, "guestfs_create");
>  
> +  option_index = 0;
> +  optind = 0;
>    for (;;) {
>      c = getopt_long (argc, argv, options, long_options,
&option_index);
>      if (c == -1) break;
> @@ -150,7 +152,9 @@ main (int argc, char *argv[])
>          if (guestfs_set_selinux (g, 1) == -1)
>            exit (EXIT_FAILURE);
>        } else if (STREQ (long_options[option_index].name,
"append")) {
> -        append = optarg;
> +        append = strdup (optarg);
> +      } else if (STREQ (long_options[option_index].name,
"autosysroot")) {
> +        autosysroot = 1;
>        } else if (STREQ (long_options[option_index].name,
"network")) {
>          network = 1;
>        } else if (STREQ (long_options[option_index].name,
"format")) {
> @@ -259,10 +263,71 @@ main (int argc, char *argv[])
>      }
>    }
>  
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  setlocale (LC_ALL, "");
> +  bindtextdomain (PACKAGE, LOCALEBASEDIR);
> +  textdomain (PACKAGE);
> +
> +  parse_config ();
> +  g = guestfs_create ();
> +  if (g == NULL)
> +    error (EXIT_FAILURE, errno, "guestfs_create");
> +
> +  parse_opts (argc, argv);
> +
>    /* --suggest flag */
>    if (suggest) {
> -    do_suggestion (drvs);
> -    exit (EXIT_SUCCESS);
> +    cmds = do_suggestion (drvs);
> +    if (!autosysroot) {
> +      exit (EXIT_SUCCESS);
> +    } else {
> +      /* Shut down libguestfs so we can start a new one */
> +      guestfs_shutdown (g);
> +      guestfs_close (g);
> +
> +      /* remove --suggest flag */
> +      size_t newcount = argc;
> +      for (int i = 0; i < argc; i++) {
> +        if (strcmp (argv[i], "--suggest") == 0) {
strcmp -> STREQ
> +          newcount--;
> +        }
> +      }
> +
> +      char ** args = malloc (sizeof (char*) * (newcount));
> +      for (int i = 0, j = 0; i < argc; i++) {
> +        if (strcmp (argv[i], "--suggest") != 0) {
> +          args[j] = strdup (argv[i]);
> +          j++;
> +        }
> +      }
> +
> +      /* clear options */
> +      drvs = NULL;
> +      format_consumed = true;
> +      c = 0;
> +      option_index = 0;
> +      network = 0;
> +      if (append) free(append);
free() accepts (by standard) also NULL as argument, so this check is
redundant.
> +      memsize = 0;
> +      smp = 0;
> +      suggest = 0;
> +      autosysroot = 0;
> +
> +      /* Create new guestfs handle, this time for rescue instead of OS
detection */
> +      g = guestfs_create ();
> +      /* Parse options with --suggest flag removed */
> +      parse_opts (newcount, args);
> +      argc = newcount;
I'm puzzled -- why do you need to parse the arguments again? You have
all the options already, so just create a new handle, and set the
options for it.

Also, 'args' is not free'd.
> +    }
> +  } else {
> +    if (autosysroot) {
> +      fprintf (stderr, _("%s: warning: --autosysroot used without
--suggest.\n"),
> +          guestfs_int_program_name);
> +    }
>    }
>  
>    /* These are really constants, but they have to be variables for the
> @@ -313,6 +378,11 @@ main (int argc, char *argv[])
>      }
>    }
>  
> +  /* Now it's time to set suggestions. */
> +  if (autosysroot) {
> +    use_suggestions (cmds);
> +  }
> +
>    /* Set other features. */
>    if (memsize > 0)
>      if (guestfs_set_memsize (g, memsize) == -1)
> @@ -367,14 +437,34 @@ compare_keys_len (const void *p1, const void *p2)
>    return strlen (key1) - strlen (key2);
>  }
>  
> +/* Use autodetected suggested filesystems */
> +static void
> +use_suggestions (char **cmds)
> +{
> +  size_t i;
> +  if (cmds == NULL) return;
> +  read_only = 0;
> +
> +  /* Craft kernel command line from suggested commands */
> +  for (i = 0; cmds[i] != NULL; i++) {
> +    char *old_append = append;
> +    append = xasprintf ("%s guestfs_command=%s;", append ==
NULL? "" : append, cmds[i]);
> +    if (old_append != NULL)
> +      free (old_append);
> +  }
I'd loop over the commands, calculate the length of the resulting
string, allocate once the needed string, and then loop again filling
it.
> +}
> +
>  /* virt-rescue --suggest flag does a kind of inspection on the
>   * drives and suggests mount commands that you should use.
>   */
> -static void
> +static char **
>  do_suggestion (struct drv *drvs)
>  {
>    CLEANUP_FREE_STRING_LIST char **roots = NULL;
>    size_t i;
> +  char **cmds = malloc (sizeof (char*));
> +  cmds[0] = NULL;
> +
>  
>    /* For inspection, force add_drives to add the drives read-only. */
>    read_only = 1;
> @@ -401,7 +491,7 @@ do_suggestion (struct drv *drvs)
>  
>    if (roots[0] == NULL) {
>      suggest_filesystems ();
> -    return;
> +    return NULL;
'cmds' is leaked here.
>    }
>  
>    printf (_("This disk contains one or more operating systems.  You
can use these mount\n"
> @@ -436,10 +526,16 @@ do_suggestion (struct drv *drvs)
>      qsort (mps, guestfs_int_count_strings (mps) / 2, 2 * sizeof (char *),
>             compare_keys_len);
>  
> -    for (j = 0; mps[j] != NULL; j += 2)
> -      printf ("mount %s /sysroot%s\n", mps[j+1], mps[j]);
> +    /* First we save commands to mount system root. */
> +    for (j = 0; mps[j] != NULL; j += 2) {
> +      int cnt = guestfs_int_count_strings (cmds);
The number of elements (excluding the terminal NULL) can be stored in
a variable and incremented, no need to recalculate it every iteration.
> +      cmds = realloc (cmds, sizeof (char *) * (cnt + 2));
> +
> +      cmds[cnt] = xasprintf ("mount %s /sysroot%s", mps[j+1],
mps[j]);
> +      cmds[cnt+1] = NULL;
> +    }
Since the number of commands is known, loop over once to calculate the
number of elements for 'cmds'.

Thanks,
-- 
Pino Toscano