+/** + * 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'");

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 ...

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 ...