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

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.