Re: [Libguestfs] [libnbd PATCH] info: Prefer NBD_OPT_INFO when possible
On Thu, Sep 07, 2023 at 11:35:27AM -0500, Eric Blake wrote: > On Tue, Sep 05, 2023 at 03:23:58PM +0100, Richard W.M. Jones wrote: > > On Tue, Sep 05, 2023 at 08:47:34AM -0500, Eric Blake wrote: > > > diff --git a/info/info-list-uris.sh b/info/info-list-uris.sh > > > index 0d6a16a0..d8ea9108 100755 > > > --- a/info/info-list-uris.sh > > > +++ b/info/info-list-uris.sh > > > @@ -22,7 +22,14 @@ set -e > > > set -x > > > > > > requires nbdkit --version > > > -requires nbdkit file --version > > > +# Avoid tickling a double-free bug in nbdkit 1.35.11 > > > +requires nbdkit -U- -r file dir=. --run 'nbdsh --opt-mode -u "$uri" -c " > > > +try: > > > + h.opt_info() > > > +except nbd.Error: > > > + pass > > > +h.opt_abort() > > > +"' > > > > > > # This test requires nbdkit >= 1.22. > > > > I guess the double-free bug is actually in any (or many versions of) > > nbdkit between 1.22 and 1.35.11, which is why the requires test is > > needed here? The message seems to imply it only affects 1.35.11. > > Hmm. There is indeed a range of affected versions, but it is not as > bad as you make out. The bug was introduced in 185e7d (v1.33.1) and > was not backported to stable-1.32; so if we assume distros only use > stable versions, then rejecting 1.34.[012] is sufficient. I've already > backported the fix to stable-1.34, but don't know if you would prefer > to cut the 1.34.3 release or let me do it. I'll do a release. > > > > It's a shame that the requires test is so long, but I couldn't see any > > way to shorten it. > > And as you pointed out in the next message, triggering the double-free > can still have the side effect of a core file. You've convinced me > that a version check is the best here. > > > > @@ -398,6 +395,13 @@ do_connect (struct nbd_handle *nbd) > > > fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); > > > exit (EXIT_FAILURE); > > >} > > > + > > > + /* If we are in opt mode, request info on the original export name. > > > + * However, ignoring failure at this time is okay, as later code > > > + * may want to try an alternate export name. > > > + */ > > > + if (nbd_aio_is_negotiating (nbd)) > > > +nbd_opt_info (nbd); > > > > I maybe don't fully understand when nbd_aio_is_negotiating would not > > be true. Aren't we always in opt mode now? > > We now unconditionally request it, but it is up to the server whether > we actually got it. An oldstyle server does not support opt mode. > And our unit tests do have coverage of connecting to an oldstyle > server. > > At any rate, with the revisions to patch 2 and 3 (nbd_shutdown now > automatically covers opt_abort without needing a new flag), and with > this patch updated to just do a version range exclusion, this series > is now in as db9f6bf6..0b587f8a Thanks, 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 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH] info: Prefer NBD_OPT_INFO when possible
On Tue, Sep 05, 2023 at 03:23:58PM +0100, Richard W.M. Jones wrote: > On Tue, Sep 05, 2023 at 08:47:34AM -0500, Eric Blake wrote: > > diff --git a/info/info-list-uris.sh b/info/info-list-uris.sh > > index 0d6a16a0..d8ea9108 100755 > > --- a/info/info-list-uris.sh > > +++ b/info/info-list-uris.sh > > @@ -22,7 +22,14 @@ set -e > > set -x > > > > requires nbdkit --version > > -requires nbdkit file --version > > +# Avoid tickling a double-free bug in nbdkit 1.35.11 > > +requires nbdkit -U- -r file dir=. --run 'nbdsh --opt-mode -u "$uri" -c " > > +try: > > + h.opt_info() > > +except nbd.Error: > > + pass > > +h.opt_abort() > > +"' > > > > # This test requires nbdkit >= 1.22. > > I guess the double-free bug is actually in any (or many versions of) > nbdkit between 1.22 and 1.35.11, which is why the requires test is > needed here? The message seems to imply it only affects 1.35.11. Hmm. There is indeed a range of affected versions, but it is not as bad as you make out. The bug was introduced in 185e7d (v1.33.1) and was not backported to stable-1.32; so if we assume distros only use stable versions, then rejecting 1.34.[012] is sufficient. I've already backported the fix to stable-1.34, but don't know if you would prefer to cut the 1.34.3 release or let me do it. > > It's a shame that the requires test is so long, but I couldn't see any > way to shorten it. And as you pointed out in the next message, triggering the double-free can still have the side effect of a core file. You've convinced me that a version check is the best here. > > @@ -398,6 +395,13 @@ do_connect (struct nbd_handle *nbd) > > fprintf (stderr, "%s: %s\n", progname, nbd_get_error ()); > > exit (EXIT_FAILURE); > >} > > + > > + /* If we are in opt mode, request info on the original export name. > > + * However, ignoring failure at this time is okay, as later code > > + * may want to try an alternate export name. > > + */ > > + if (nbd_aio_is_negotiating (nbd)) > > +nbd_opt_info (nbd); > > I maybe don't fully understand when nbd_aio_is_negotiating would not > be true. Aren't we always in opt mode now? We now unconditionally request it, but it is up to the server whether we actually got it. An oldstyle server does not support opt mode. And our unit tests do have coverage of connecting to an oldstyle server. At any rate, with the revisions to patch 2 and 3 (nbd_shutdown now automatically covers opt_abort without needing a new flag), and with this patch updated to just do a version range exclusion, this series is now in as db9f6bf6..0b587f8a -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH] info: Prefer NBD_OPT_INFO when possible
On Tue, Sep 05, 2023 at 08:47:34AM -0500, Eric Blake wrote: > --- a/info/info-list-uris.sh > +++ b/info/info-list-uris.sh > @@ -22,7 +22,14 @@ set -e > set -x > > requires nbdkit --version > -requires nbdkit file --version > +# Avoid tickling a double-free bug in nbdkit 1.35.11 > +requires nbdkit -U- -r file dir=. --run 'nbdsh --opt-mode -u "$uri" -c " > +try: > + h.opt_info() > +except nbd.Error: > + pass > +h.opt_abort() > +"' Actually I think a problem with this is that it always causes a segfault, which could leave a core dump around (does on my machine). It's unfortunate, because obviously you can't test for a double-free in nbdkit without causing a double-free. It might be a rare case when a version test is better. 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://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH] info: Prefer NBD_OPT_INFO when possible
On Tue, Sep 05, 2023 at 08:47:34AM -0500, Eric Blake wrote:
> diff --git a/info/info-list-uris.sh b/info/info-list-uris.sh
> index 0d6a16a0..d8ea9108 100755
> --- a/info/info-list-uris.sh
> +++ b/info/info-list-uris.sh
> @@ -22,7 +22,14 @@ set -e
> set -x
>
> requires nbdkit --version
> -requires nbdkit file --version
> +# Avoid tickling a double-free bug in nbdkit 1.35.11
> +requires nbdkit -U- -r file dir=. --run 'nbdsh --opt-mode -u "$uri" -c "
> +try:
> + h.opt_info()
> +except nbd.Error:
> + pass
> +h.opt_abort()
> +"'
>
> # This test requires nbdkit >= 1.22.
I guess the double-free bug is actually in any (or many versions of)
nbdkit between 1.22 and 1.35.11, which is why the requires test is
needed here? The message seems to imply it only affects 1.35.11.
It's a shame that the requires test is so long, but I couldn't see any
way to shorten it.
> minor=$( nbdkit --dump-config | grep ^version_minor | cut -d= -f2 )
> diff --git a/info/info-uri.sh b/info/info-uri.sh
> index d491e904..ba0c36d2 100755
> --- a/info/info-uri.sh
> +++ b/info/info-uri.sh
> @@ -24,7 +24,14 @@ set -e
> set -x
>
> requires nbdkit --version
> -requires nbdkit file --version
> +# Avoid tickling a double-free bug in nbdkit 1.35.11
> +requires nbdkit -U- -r file dir=. --run 'nbdsh --opt-mode -u "$uri" -c "
> +try:
> + h.opt_info()
> +except nbd.Error:
> + pass
> +h.opt_abort()
> +"'
> requires nbdkit -U - null --run 'test "$uri" != ""'
> requires jq --version
>
> diff --git a/info/main.c b/info/main.c
> index 204290a5..80d38224 100644
> --- a/info/main.c
> +++ b/info/main.c
> @@ -138,7 +138,6 @@ main (int argc, char *argv[])
>size_t output_len = 0;
>bool content_flag = false, no_content_flag = false;
>bool list_okay = true;
> - bool opt_mode = false;
>
>progname = argv[0];
>colour = isatty (STDOUT_FILENO);
> @@ -281,11 +280,9 @@ main (int argc, char *argv[])
>nbd_set_uri_allow_local_file (nbd, true); /* Allow ?tls-psk-file. */
>
>/* Set optional modes in the handle. */
> - opt_mode = !can && !map && !size_only;
> - if (opt_mode) {
> -nbd_set_opt_mode (nbd, true);
> + nbd_set_opt_mode (nbd, true);
> + if (!can && !map && !size_only)
> nbd_set_full_info (nbd, true);
> - }
>if (map)
> nbd_add_meta_context (nbd, map);
>
> @@ -398,6 +395,13 @@ do_connect (struct nbd_handle *nbd)
> fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
> exit (EXIT_FAILURE);
>}
> +
> + /* If we are in opt mode, request info on the original export name.
> + * However, ignoring failure at this time is okay, as later code
> + * may want to try an alternate export name.
> + */
> + if (nbd_aio_is_negotiating (nbd))
> +nbd_opt_info (nbd);
I maybe don't fully understand when nbd_aio_is_negotiating would not
be true. Aren't we always in opt mode now?
> }
>
> /* The URI field in output is not meaningful unless there's a
> diff --git a/info/map.c b/info/map.c
> index 594f24ff..399e0355 100644
> --- a/info/map.c
> +++ b/info/map.c
> @@ -54,6 +54,13 @@ do_map (void)
>uint64_t offset, align, max_len;
>size_t prev_entries_size;
>
> + /* Map mode requires switching over to transmission phase. */
> + if (nbd_aio_is_negotiating (nbd) &&
> + nbd_opt_go (nbd) == -1) {
> +fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
> +exit (EXIT_FAILURE);
> + }
> +
>/* Did we get the requested map? */
>if (!nbd_can_meta_context (nbd, map)) {
> fprintf (stderr,
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [libnbd PATCH] info: Prefer NBD_OPT_INFO when possible
Now that libnbd supports extended headers, it is worth observing that
the qemu implementation for NBD_CMD_BLOCK_STATUS that supports an
extended header for filtering the server's response, as advertised by
NBD_FLAG_BLOCK_STAT_PAYLOAD, is conditionally advertised. When using
NBD_OPT_GO, the flag is only advertised if the client requested more
than one meta context (as otherwise, there is no use filtering). But
for NBD_OPT_INFO, the flag is advertised if extended headers are
enabled, regardless of whether meta contexts are negotiated yet.
In order for upcoming libnbd patches to add support for probing and
using this bit reliably, we therefore need 'nbdinfo --can ...' to
favor the information learned by NBD_OPT_INFO instead of NBD_OPT_GO,
because that is lighter weight than figuring out whether an export
supports at least two meta contexts that can then be negotiated.
Do this by changing nbdinfo to prefer opt mode always, then touching
up the few places (--map, --content) that need to ensure NBD_OPT_GO.
This is made easier by the previous patches that make nbd_shutdown()
work sanely regardless of whether we are still in opt mode.
In turn, this patch flushed out a double-free bug in 'nbdkit file
dir=...' when querying opt_info on the default name, so a couple of
unit tests need to avoid false negatives on platforms where nbdkit
commit 39d62de9 is not yet backported.
Signed-off-by: Eric Blake
---
info/info-list-uris.sh | 9 -
info/info-uri.sh | 9 -
info/main.c| 14 +-
info/map.c | 7 +++
4 files changed, 32 insertions(+), 7 deletions(-)
diff --git a/info/info-list-uris.sh b/info/info-list-uris.sh
index 0d6a16a0..d8ea9108 100755
--- a/info/info-list-uris.sh
+++ b/info/info-list-uris.sh
@@ -22,7 +22,14 @@ set -e
set -x
requires nbdkit --version
-requires nbdkit file --version
+# Avoid tickling a double-free bug in nbdkit 1.35.11
+requires nbdkit -U- -r file dir=. --run 'nbdsh --opt-mode -u "$uri" -c "
+try:
+ h.opt_info()
+except nbd.Error:
+ pass
+h.opt_abort()
+"'
# This test requires nbdkit >= 1.22.
minor=$( nbdkit --dump-config | grep ^version_minor | cut -d= -f2 )
diff --git a/info/info-uri.sh b/info/info-uri.sh
index d491e904..ba0c36d2 100755
--- a/info/info-uri.sh
+++ b/info/info-uri.sh
@@ -24,7 +24,14 @@ set -e
set -x
requires nbdkit --version
-requires nbdkit file --version
+# Avoid tickling a double-free bug in nbdkit 1.35.11
+requires nbdkit -U- -r file dir=. --run 'nbdsh --opt-mode -u "$uri" -c "
+try:
+ h.opt_info()
+except nbd.Error:
+ pass
+h.opt_abort()
+"'
requires nbdkit -U - null --run 'test "$uri" != ""'
requires jq --version
diff --git a/info/main.c b/info/main.c
index 204290a5..80d38224 100644
--- a/info/main.c
+++ b/info/main.c
@@ -138,7 +138,6 @@ main (int argc, char *argv[])
size_t output_len = 0;
bool content_flag = false, no_content_flag = false;
bool list_okay = true;
- bool opt_mode = false;
progname = argv[0];
colour = isatty (STDOUT_FILENO);
@@ -281,11 +280,9 @@ main (int argc, char *argv[])
nbd_set_uri_allow_local_file (nbd, true); /* Allow ?tls-psk-file. */
/* Set optional modes in the handle. */
- opt_mode = !can && !map && !size_only;
- if (opt_mode) {
-nbd_set_opt_mode (nbd, true);
+ nbd_set_opt_mode (nbd, true);
+ if (!can && !map && !size_only)
nbd_set_full_info (nbd, true);
- }
if (map)
nbd_add_meta_context (nbd, map);
@@ -398,6 +395,13 @@ do_connect (struct nbd_handle *nbd)
fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
exit (EXIT_FAILURE);
}
+
+ /* If we are in opt mode, request info on the original export name.
+ * However, ignoring failure at this time is okay, as later code
+ * may want to try an alternate export name.
+ */
+ if (nbd_aio_is_negotiating (nbd))
+nbd_opt_info (nbd);
}
/* The URI field in output is not meaningful unless there's a
diff --git a/info/map.c b/info/map.c
index 594f24ff..399e0355 100644
--- a/info/map.c
+++ b/info/map.c
@@ -54,6 +54,13 @@ do_map (void)
uint64_t offset, align, max_len;
size_t prev_entries_size;
+ /* Map mode requires switching over to transmission phase. */
+ if (nbd_aio_is_negotiating (nbd) &&
+ nbd_opt_go (nbd) == -1) {
+fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
+exit (EXIT_FAILURE);
+ }
+
/* Did we get the requested map? */
if (!nbd_can_meta_context (nbd, map)) {
fprintf (stderr,
--
2.41.0
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
