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

[Pino Toscano]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20181004/fd017ca4/attachment.sig>