Re: [Libguestfs] [PATCH v2] lib: Use qemu-img info -U option to avoid locking error.
On Thu, Oct 04, 2018 at 04:52:28PM +0200, Pino Toscano wrote:
> 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 ...
I can easily narrow the regexp if that's the concern. Currently it is:
+ "grep -sq -- 'info.*-U'");
How about instead:
+ "grep -sqE -- '\\binfo\\b.*-U\\b'");
? I can't think of a way to make it narrower, given the limitations
of what we're doing, ie. grepping help output, which I agree is not
ideal.
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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2] lib: Use qemu-img info -U option to avoid locking error.
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
signature.asc
Description: This is a digitally signed message part.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
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.
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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2] lib: Use qemu-img info -U option to avoid locking error.
+1 LGTM.
Thanks Richard.
On Fri, Sep 21, 2018 at 12:53 PM Richard W.M. Jones
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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2] lib: Use qemu-img info -U option to avoid locking error.
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 ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH v2] lib: Use qemu-img info -U option to avoid locking error.
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
signature.asc
Description: This is a digitally signed message part.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
