Libguestfs - Oct 2018 - Re: [PATCH v2] lib: Use qemu-img info -U option to avoid locking error.

https://bugs.launchpad.net/qemu/+bug/1740364
---
 lib/guestfs-internal.h |  3 +++
 lib/handle.c           |  2 ++
 lib/info.c             | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 44 insertions(+)

diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index adeb9478a..c66c55e70 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -510,6 +510,9 @@ struct guestfs_h {
   /* Cached features. */
   struct cached_feature *features;
   size_t nr_features;
+
+  /* Used by lib/info.c.  -1 = not tested or error; else 0 or 1. */
+  int qemu_img_supports_U_option;
 };
 
 /**
diff --git a/lib/handle.c b/lib/handle.c
index a47aaafab..297ff6d67 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -101,6 +101,8 @@ guestfs_create_flags (unsigned flags, ...)
 
   g->memsize = DEFAULT_MEMSIZE;
 
+  g->qemu_img_supports_U_option = -1; /* not tested, see lib/info.c */
+
   /* Start with large serial numbers so they are easy to spot
    * inside the protocol.
    */
diff --git a/lib/info.c b/lib/info.c
index 2eadc1c11..74e4424b8 100644
--- a/lib/info.c
+++ b/lib/info.c
@@ -57,6 +57,7 @@ cleanup_json_t_decref (void *ptr)
 #endif
 
 static json_t *get_json_output (guestfs_h *g, const char *filename);
+static int qemu_img_supports_U_option (guestfs_h *g);
 static void set_child_rlimits (struct command *);
 
 char *
@@ -149,6 +150,11 @@ get_json_output (guestfs_h *g, const char *filename)
 
   guestfs_int_cmd_add_arg (cmd, "qemu-img");
   guestfs_int_cmd_add_arg (cmd, "info");
+  switch (qemu_img_supports_U_option (g)) {
+  case -1: return NULL;
+  case 0:  break;
+  default: guestfs_int_cmd_add_arg (cmd, "-U");
+  }
   guestfs_int_cmd_add_arg (cmd, "--output");
   guestfs_int_cmd_add_arg (cmd, "json");
   if (filename[0] == '/')
@@ -218,3 +224,36 @@ set_child_rlimits (struct command *cmd)
   guestfs_int_cmd_set_child_rlimit (cmd, RLIMIT_CPU, 10 /* seconds */);
 #endif
 }
+
+/**
+ * Test if the qemu-img info command supports the C<-U> option to
+ * disable locking.  The result is memoized in the handle.
+ *
+ * Note this option was added in qemu 2.11.  We can remove this test
+ * when we can assume everyone is using qemu >= 2.11.
+ */
+static int
+qemu_img_supports_U_option (guestfs_h *g)
+{
+  if (g->qemu_img_supports_U_option >= 0)
+    return g->qemu_img_supports_U_option;
+
+  CLEANUP_CMD_CLOSE struct command *cmd = guestfs_int_new_command (g);
+  int r;
+
+  guestfs_int_cmd_add_string_unquoted (cmd,
+                                       "qemu-img info --help | "
+                                       "grep -sq --
'info.*-U'");
+  r = guestfs_int_cmd_run (cmd);
+  if (r == -1)
+    return -1;
+  if (!WIFEXITED (r)) {
+    guestfs_int_external_command_failed (g, r,
+                                         "qemu-img info -U option
test",
+                                         NULL);
+    return -1;
+  }
+
+  g->qemu_img_supports_U_option = WEXITSTATUS (r) == 0;
+  return g->qemu_img_supports_U_option;
+}
-- 
2.19.0.rc0

On Friday, 21 September 2018 11:53:52 CEST Richard W.M. Jones
wrote:
> +/**
> + * Test if the qemu-img info command supports the C<-U> option to
> + * disable locking.  The result is memoized in the handle.
> + *
> + * Note this option was added in qemu 2.11.  We can remove this test
> + * when we can assume everyone is using qemu >= 2.11.
> + */
> +static int
> +qemu_img_supports_U_option (guestfs_h *g)
> +{
> +  if (g->qemu_img_supports_U_option >= 0)
> +    return g->qemu_img_supports_U_option;
> +
> +  CLEANUP_CMD_CLOSE struct command *cmd = guestfs_int_new_command (g);
> +  int r;
> +
> +  guestfs_int_cmd_add_string_unquoted (cmd,
> +                                       "qemu-img info --help | "
> +                                       "grep -sq --
'info.*-U'");
This may match something else in future, in case some other command of
qemu-img gets another option with "info" in it...

TBH this is something we have already, and precisely in the qemu_data
struct, which is memoized.  One downside is that using it would mean
memoizing the qemu help/schema in advance, even if the appliance is not
started; so code like `guestfish disk-format foo` will be slower.
OTOH, the upside is that there is no need to "reprobe" qemu-img for
something that we detect already from the QMP schema.

One possible idea can be to cache the qemu_data struct in the handle,
so the direct backend would not get a performance regression (since
either one of the info commands already preloaded a qemu_data, or
launch would do that).

-- 
Pino Toscano

+1 LGTM.
Thanks Richard.

On Fri, Sep 21, 2018 at 12:53 PM Richard W.M. Jones <rjones@redhat.com>
wrote:
> https://bugs.launchpad.net/qemu/+bug/1740364
> ---
>  lib/guestfs-internal.h |  3 +++
>  lib/handle.c           |  2 ++
>  lib/info.c             | 39 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 44 insertions(+)
>
> diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
> index adeb9478a..c66c55e70 100644
> --- a/lib/guestfs-internal.h
> +++ b/lib/guestfs-internal.h
> @@ -510,6 +510,9 @@ struct guestfs_h {
>    /* Cached features. */
>    struct cached_feature *features;
>    size_t nr_features;
> +
> +  /* Used by lib/info.c.  -1 = not tested or error; else 0 or 1. */
> +  int qemu_img_supports_U_option;
>  };
>
>  /**
> diff --git a/lib/handle.c b/lib/handle.c
> index a47aaafab..297ff6d67 100644
> --- a/lib/handle.c
> +++ b/lib/handle.c
> @@ -101,6 +101,8 @@ guestfs_create_flags (unsigned flags, ...)
>
>    g->memsize = DEFAULT_MEMSIZE;
>
> +  g->qemu_img_supports_U_option = -1; /* not tested, see lib/info.c */
> +
>    /* Start with large serial numbers so they are easy to spot
>     * inside the protocol.
>     */
> diff --git a/lib/info.c b/lib/info.c
> index 2eadc1c11..74e4424b8 100644
> --- a/lib/info.c
> +++ b/lib/info.c
> @@ -57,6 +57,7 @@ cleanup_json_t_decref (void *ptr)
>  #endif
>
>  static json_t *get_json_output (guestfs_h *g, const char *filename);
> +static int qemu_img_supports_U_option (guestfs_h *g);
>  static void set_child_rlimits (struct command *);
>
>  char *
> @@ -149,6 +150,11 @@ get_json_output (guestfs_h *g, const char *filename)
>
>    guestfs_int_cmd_add_arg (cmd, "qemu-img");
>    guestfs_int_cmd_add_arg (cmd, "info");
> +  switch (qemu_img_supports_U_option (g)) {
> +  case -1: return NULL;
> +  case 0:  break;
> +  default: guestfs_int_cmd_add_arg (cmd, "-U");
> +  }
>    guestfs_int_cmd_add_arg (cmd, "--output");
>    guestfs_int_cmd_add_arg (cmd, "json");
>    if (filename[0] == '/')
> @@ -218,3 +224,36 @@ set_child_rlimits (struct command *cmd)
>    guestfs_int_cmd_set_child_rlimit (cmd, RLIMIT_CPU, 10 /* seconds */);
>  #endif
>  }
> +
> +/**
> + * Test if the qemu-img info command supports the C<-U> option to
> + * disable locking.  The result is memoized in the handle.
> + *
> + * Note this option was added in qemu 2.11.  We can remove this test
> + * when we can assume everyone is using qemu >= 2.11.
> + */
> +static int
> +qemu_img_supports_U_option (guestfs_h *g)
> +{
> +  if (g->qemu_img_supports_U_option >= 0)
> +    return g->qemu_img_supports_U_option;
> +
> +  CLEANUP_CMD_CLOSE struct command *cmd = guestfs_int_new_command (g);
> +  int r;
> +
> +  guestfs_int_cmd_add_string_unquoted (cmd,
> +                                       "qemu-img info --help | "
> +                                       "grep -sq --
'info.*-U'");
> +  r = guestfs_int_cmd_run (cmd);
> +  if (r == -1)
> +    return -1;
> +  if (!WIFEXITED (r)) {
> +    guestfs_int_external_command_failed (g, r,
> +                                         "qemu-img info -U option
test",
> +                                         NULL);
> +    return -1;
> +  }
> +
> +  g->qemu_img_supports_U_option = WEXITSTATUS (r) == 0;
> +  return g->qemu_img_supports_U_option;
> +}
> --
> 2.19.0.rc0
>
>
-- 
*GAL bEN HAIM*
RHV DEVOPS

This patch needs a bit more work as Pino outlined in his reply.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Wed, Sep 26, 2018 at 06:36:47PM +0200, Pino Toscano
wrote:
> On Friday, 21 September 2018 11:53:52 CEST Richard W.M. Jones wrote:
> > +/**
> > + * Test if the qemu-img info command supports the C<-U> option
to
> > + * disable locking.  The result is memoized in the handle.
> > + *
> > + * Note this option was added in qemu 2.11.  We can remove this test
> > + * when we can assume everyone is using qemu >= 2.11.
> > + */
> > +static int
> > +qemu_img_supports_U_option (guestfs_h *g)
> > +{
> > +  if (g->qemu_img_supports_U_option >= 0)
> > +    return g->qemu_img_supports_U_option;
> > +
> > +  CLEANUP_CMD_CLOSE struct command *cmd = guestfs_int_new_command
(g);
> > +  int r;
> > +
> > +  guestfs_int_cmd_add_string_unquoted (cmd,
> > +                                       "qemu-img info --help |
"
> > +                                       "grep -sq --
'info.*-U'");
> 
> This may match something else in future, in case some other command of
> qemu-img gets another option with "info" in it...
> 
> TBH this is something we have already, and precisely in the qemu_data
> struct, which is memoized.  One downside is that using it would mean
> memoizing the qemu help/schema in advance, even if the appliance is not
> started; so code like `guestfish disk-format foo` will be slower.
> OTOH, the upside is that there is no need to "reprobe" qemu-img
for
> something that we detect already from the QMP schema.
So I had a look into this.  I guess you mean specifically the result
of ‘guestfs_int_qemu_mandatory_locking’ which queries the QMP schema.
This is a reasonable proxy for presence of the qemu-img -U option
since both features were added within a few months.

However the problem is that testing for this requires us to run qemu
before launch is called.  That has API issues because it's unclear
what code like:

  g = guestfs_create ();
  guestfs_disk_format (g, "disk.img");
  guestfs_set_hv (g, "my-weird-qemu");
  guestfs_launch (g);

should do (granted, it is a peculiar corner case and the caller
probably gets what they deserve).
> One possible idea can be to cache the qemu_data struct in the handle,
> so the direct backend would not get a performance regression (since
> either one of the info commands already preloaded a qemu_data, or
> launch would do that).
I'll see what a patch would look like ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top