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

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

On Thu, Oct 04, 2018 at 01:34:59PM +0100, Richard W.M. Jones
wrote:
> 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 ...
Actually another reason why this isn't going to work is that we only
test qemu in the direct backend.  For the libvirt backend this would
mean we'd have to test qemu for operations like guestfs_disk_format,
but there isn't necessarily a qemu binary for us to test (libvirt
knows it, we don't necessarily know which qemu binary libvirt would
run).

I think the easiest thing here is to test the qemu-img binary.
ie. v2 patch as posted.  It's only a minor overhead.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Thursday, 4 October 2018 14:50:07 CEST Richard W.M. Jones
wrote:
> On Thu, Oct 04, 2018 at 01:34:59PM +0100, Richard W.M. Jones wrote:
> > 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 ...
> 
> Actually another reason why this isn't going to work is that we only
> test qemu in the direct backend.  For the libvirt backend this would
> mean we'd have to test qemu for operations like guestfs_disk_format,
> but there isn't necessarily a qemu binary for us to test (libvirt
> knows it, we don't necessarily know which qemu binary libvirt would
> run).
> 
> I think the easiest thing here is to test the qemu-img binary.
> ie. v2 patch as posted.  It's only a minor overhead.
I still do not think that using shell commands with such broad grep
invocations is future-proof, and not potentially breaking in the
future.

OTOH, I have no bandwidth for this debate ...

-- 
Pino Toscano