Eric Blake
2022-Aug-31 14:39 UTC
[Libguestfs] [libnbd PATCH v2 03/12] api: Allow nbd_opt_list_meta_context without SR
Upstream NBD clarified (see NBD commit 13a4e33a8) that since
NBD_OPT_LIST_META_CONTEXT is stateless on the server side, it is
acceptable (but not mandatory) for servers to accept it without the
client having pre-negotiated structured replies. We aren't quite
stateless on the client side yet - that will be fixed in a later patch
to keep this one small and easier to test. The testsuite changes pass
when using modern nbdkit; however, it skips[*] if we talk to an older
server. But since the test also skips with our pre-patch behavior,
it's not worth separating it into a separate patch.
For more history, qemu-nbd prior to commit da24597d [v6.2] and nbdkit
prior to commit c39cba73 [v1.27.3, also backported to 1.26.6] are
servers that chose to reject NBD_OPT_LIST_META_CONTEXT without a prior
NBD_OPT_STRUCTURED_REPLY.
[*]It's easier to skip on server failure than to try and write an
nbdkit patch to add yet another --config feature probe just to depend
on new-enough nbdkit to gracefully probe in advance if the server
should succeed.
---
generator/states-newstyle-opt-meta-context.c | 1 -
lib/opt.c | 6 +----
tests/opt-list-meta.c | 28 +++++++++++++++++---
3 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/generator/states-newstyle-opt-meta-context.c
b/generator/states-newstyle-opt-meta-context.c
index a6a5271..5c65454 100644
--- a/generator/states-newstyle-opt-meta-context.c
+++ b/generator/states-newstyle-opt-meta-context.c
@@ -34,7 +34,6 @@ STATE_MACHINE {
meta_vector_reset (&h->meta_contexts);
if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) {
assert (h->opt_mode);
- assert (h->structured_replies);
assert (CALLBACK_IS_NOT_NULL (h->opt_cb.fn.context));
opt = h->opt_current;
}
diff --git a/lib/opt.c b/lib/opt.c
index e5802f4..d9114f4 100644
--- a/lib/opt.c
+++ b/lib/opt.c
@@ -1,5 +1,5 @@
/* NBD client library in userspace
- * Copyright (C) 2020-2021 Red Hat Inc.
+ * Copyright (C) 2020-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -290,10 +290,6 @@ nbd_unlocked_aio_opt_list_meta_context (struct nbd_handle
*h,
set_error (ENOTSUP, "server is not using fixed newstyle
protocol");
return -1;
}
- if (!h->structured_replies) {
- set_error (ENOTSUP, "server lacks structured replies");
- return -1;
- }
assert (CALLBACK_IS_NULL (h->opt_cb.fn.context));
h->opt_cb.fn.context = *context;
diff --git a/tests/opt-list-meta.c b/tests/opt-list-meta.c
index 9ad8e37..ccf58fc 100644
--- a/tests/opt-list-meta.c
+++ b/tests/opt-list-meta.c
@@ -17,6 +17,7 @@
*/
/* Test behavior of nbd_opt_list_meta_context. */
+/* See also unit test 240 in the various language ports. */
#include <config.h>
@@ -164,7 +165,10 @@ main (int argc, char *argv[])
nbd_opt_abort (nbd);
nbd_close (nbd);
- /* Repeat but this time without structured replies. */
+ /* Repeat but this time without structured replies. Below this point,
+ * it is not worth porting this part of the test to non-C languages
+ * because of the potential to skip the rest of the test.
+ */
nbd = nbd_create ();
if (nbd == NULL ||
nbd_set_opt_mode (nbd, true) == -1 ||
@@ -174,15 +178,31 @@ main (int argc, char *argv[])
exit (EXIT_FAILURE);
}
- /* FIXME: For now, we reject this client-side, but it is overly strict. */
+ /* Older servers don't permit this, but there is no reliable indicator
+ * of whether nbdkit is new enough, so just skip the rest of the test
+ * if the attempt fails (then read the logs to see that the skip was
+ * indeed caused by the server, and not an accidental client-side bug).
+ */
p = (struct progress) { .count = 0 };
r = nbd_opt_list_meta_context (nbd,
(nbd_context_callback) { .callback = check,
.user_data =
&p});
- if (r != -1) {
- fprintf (stderr, "not expecting command to succeed\n");
+ if (r == -1) {
+ fprintf (stderr, "Skipping test; server probably too old for listing
"
+ "without structured replies: %s\n", nbd_get_error ());
+ exit (77);
+ }
+ if (r != p.count) {
+ fprintf (stderr, "inconsistent return value %d, expected %d\n",
r, p.count);
+ exit (EXIT_FAILURE);
+ }
+ if (r < 1 || !p.seen) {
+ fprintf (stderr, "server did not reply with base:allocation\n");
exit (EXIT_FAILURE);
}
+ nbd_opt_abort (nbd);
+ nbd_close (nbd);
+
exit (EXIT_SUCCESS);
}
--
2.37.2
Laszlo Ersek
2022-Sep-01 09:21 UTC
[Libguestfs] [libnbd PATCH v2 03/12] api: Allow nbd_opt_list_meta_context without SR
On 08/31/22 16:39, Eric Blake wrote:> Upstream NBD clarified (see NBD commit 13a4e33a8) that since > NBD_OPT_LIST_META_CONTEXT is stateless on the server side, it is > acceptable (but not mandatory) for servers to accept it without the > client having pre-negotiated structured replies. We aren't quite > stateless on the client side yet - that will be fixed in a later patch > to keep this one small and easier to test. The testsuite changes pass > when using modern nbdkit; however, it skips[*] if we talk to an older > server. But since the test also skips with our pre-patch behavior, > it's not worth separating it into a separate patch. > > For more history, qemu-nbd prior to commit da24597d [v6.2] and nbdkit > prior to commit c39cba73 [v1.27.3, also backported to 1.26.6] are > servers that chose to reject NBD_OPT_LIST_META_CONTEXT without a prior > NBD_OPT_STRUCTURED_REPLY. > > [*]It's easier to skip on server failure than to try and write an > nbdkit patch to add yet another --config feature probe just to depend > on new-enough nbdkit to gracefully probe in advance if the server > should succeed. > --- > generator/states-newstyle-opt-meta-context.c | 1 - > lib/opt.c | 6 +---- > tests/opt-list-meta.c | 28 +++++++++++++++++--- > 3 files changed, 25 insertions(+), 10 deletions(-) > > diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c > index a6a5271..5c65454 100644 > --- a/generator/states-newstyle-opt-meta-context.c > +++ b/generator/states-newstyle-opt-meta-context.c > @@ -34,7 +34,6 @@ STATE_MACHINE { > meta_vector_reset (&h->meta_contexts); > if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) { > assert (h->opt_mode); > - assert (h->structured_replies); > assert (CALLBACK_IS_NOT_NULL (h->opt_cb.fn.context)); > opt = h->opt_current; > }Can we introduce some xfuncname pattern for these "state machine" C files? The STATE_MACHINE hunk header is totally useless. I'd like "NEWSTYLE.OPT_META_CONTEXT.START". :) Regarding the code -- with the removal of the assertion from the LIST branch, but preserving a similar check (albeit not an assert()) on the SET branch, the comment covering *both* branches is now out of date: /* If the server doesn't support SRs then we must skip this group.> diff --git a/lib/opt.c b/lib/opt.c > index e5802f4..d9114f4 100644 > --- a/lib/opt.c > +++ b/lib/opt.c > @@ -1,5 +1,5 @@ > /* NBD client library in userspace > - * Copyright (C) 2020-2021 Red Hat Inc. > + * Copyright (C) 2020-2022 Red Hat Inc. > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Lesser General Public > @@ -290,10 +290,6 @@ nbd_unlocked_aio_opt_list_meta_context (struct nbd_handle *h, > set_error (ENOTSUP, "server is not using fixed newstyle protocol"); > return -1; > } > - if (!h->structured_replies) { > - set_error (ENOTSUP, "server lacks structured replies"); > - return -1; > - } > > assert (CALLBACK_IS_NULL (h->opt_cb.fn.context)); > h->opt_cb.fn.context = *context; > diff --git a/tests/opt-list-meta.c b/tests/opt-list-meta.c > index 9ad8e37..ccf58fc 100644 > --- a/tests/opt-list-meta.c > +++ b/tests/opt-list-meta.c > @@ -17,6 +17,7 @@ > */ > > /* Test behavior of nbd_opt_list_meta_context. */ > +/* See also unit test 240 in the various language ports. */ > > #include <config.h> > > @@ -164,7 +165,10 @@ main (int argc, char *argv[]) > nbd_opt_abort (nbd); > nbd_close (nbd); > > - /* Repeat but this time without structured replies. */ > + /* Repeat but this time without structured replies. Below this point, > + * it is not worth porting this part of the test to non-C languages > + * because of the potential to skip the rest of the test. > + */ > nbd = nbd_create (); > if (nbd == NULL || > nbd_set_opt_mode (nbd, true) == -1 || > @@ -174,15 +178,31 @@ main (int argc, char *argv[]) > exit (EXIT_FAILURE); > } > > - /* FIXME: For now, we reject this client-side, but it is overly strict. */ > + /* Older servers don't permit this, but there is no reliable indicator > + * of whether nbdkit is new enough, so just skip the rest of the test > + * if the attempt fails (then read the logs to see that the skip was > + * indeed caused by the server, and not an accidental client-side bug). > + */ > p = (struct progress) { .count = 0 }; > r = nbd_opt_list_meta_context (nbd, > (nbd_context_callback) { .callback = check, > .user_data = &p}); > - if (r != -1) { > - fprintf (stderr, "not expecting command to succeed\n"); > + if (r == -1) { > + fprintf (stderr, "Skipping test; server probably too old for listing " > + "without structured replies: %s\n", nbd_get_error ()); > + exit (77); > + } > + if (r != p.count) { > + fprintf (stderr, "inconsistent return value %d, expected %d\n", r, p.count); > + exit (EXIT_FAILURE); > + } > + if (r < 1 || !p.seen) { > + fprintf (stderr, "server did not reply with base:allocation\n"); > exit (EXIT_FAILURE); > } > > + nbd_opt_abort (nbd); > + nbd_close (nbd); > + > exit (EXIT_SUCCESS); > } >On a tangent: why do we have nbd_opt_abort() separately from nbd_close()? What further steps would be valid between the two? Reviewed-by: Laszlo Ersek <lersek at redhat.com>
Richard W.M. Jones
2022-Sep-01 16:17 UTC
[Libguestfs] [libnbd PATCH v2 03/12] api: Allow nbd_opt_list_meta_context without SR
On Wed, Aug 31, 2022 at 09:39:19AM -0500, Eric Blake wrote:> Upstream NBD clarified (see NBD commit 13a4e33a8) that since > NBD_OPT_LIST_META_CONTEXT is stateless on the server side, it is > acceptable (but not mandatory) for servers to accept it without the > client having pre-negotiated structured replies. We aren't quite > stateless on the client side yet - that will be fixed in a later patch > to keep this one small and easier to test. The testsuite changes pass > when using modern nbdkit; however, it skips[*] if we talk to an older > server. But since the test also skips with our pre-patch behavior, > it's not worth separating it into a separate patch. > > For more history, qemu-nbd prior to commit da24597d [v6.2] and nbdkit > prior to commit c39cba73 [v1.27.3, also backported to 1.26.6] are > servers that chose to reject NBD_OPT_LIST_META_CONTEXT without a prior > NBD_OPT_STRUCTURED_REPLY. > > [*]It's easier to skip on server failure than to try and write an > nbdkit patch to add yet another --config feature probe just to depend > on new-enough nbdkit to gracefully probe in advance if the server > should succeed. > --- > generator/states-newstyle-opt-meta-context.c | 1 - > lib/opt.c | 6 +---- > tests/opt-list-meta.c | 28 +++++++++++++++++--- > 3 files changed, 25 insertions(+), 10 deletions(-) > > diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c > index a6a5271..5c65454 100644 > --- a/generator/states-newstyle-opt-meta-context.c > +++ b/generator/states-newstyle-opt-meta-context.c > @@ -34,7 +34,6 @@ STATE_MACHINE { > meta_vector_reset (&h->meta_contexts); > if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) { > assert (h->opt_mode); > - assert (h->structured_replies); > assert (CALLBACK_IS_NOT_NULL (h->opt_cb.fn.context)); > opt = h->opt_current; > } > diff --git a/lib/opt.c b/lib/opt.c > index e5802f4..d9114f4 100644 > --- a/lib/opt.c > +++ b/lib/opt.c > @@ -1,5 +1,5 @@ > /* NBD client library in userspace > - * Copyright (C) 2020-2021 Red Hat Inc. > + * Copyright (C) 2020-2022 Red Hat Inc. > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Lesser General Public > @@ -290,10 +290,6 @@ nbd_unlocked_aio_opt_list_meta_context (struct nbd_handle *h, > set_error (ENOTSUP, "server is not using fixed newstyle protocol"); > return -1; > } > - if (!h->structured_replies) { > - set_error (ENOTSUP, "server lacks structured replies"); > - return -1; > - } > > assert (CALLBACK_IS_NULL (h->opt_cb.fn.context)); > h->opt_cb.fn.context = *context; > diff --git a/tests/opt-list-meta.c b/tests/opt-list-meta.c > index 9ad8e37..ccf58fc 100644 > --- a/tests/opt-list-meta.c > +++ b/tests/opt-list-meta.c > @@ -17,6 +17,7 @@ > */ > > /* Test behavior of nbd_opt_list_meta_context. */ > +/* See also unit test 240 in the various language ports. */ > > #include <config.h> > > @@ -164,7 +165,10 @@ main (int argc, char *argv[]) > nbd_opt_abort (nbd); > nbd_close (nbd); > > - /* Repeat but this time without structured replies. */ > + /* Repeat but this time without structured replies. Below this point, > + * it is not worth porting this part of the test to non-C languages > + * because of the potential to skip the rest of the test. > + */ > nbd = nbd_create (); > if (nbd == NULL || > nbd_set_opt_mode (nbd, true) == -1 || > @@ -174,15 +178,31 @@ main (int argc, char *argv[]) > exit (EXIT_FAILURE); > } > > - /* FIXME: For now, we reject this client-side, but it is overly strict. */ > + /* Older servers don't permit this, but there is no reliable indicator > + * of whether nbdkit is new enough, so just skip the rest of the test > + * if the attempt fails (then read the logs to see that the skip was > + * indeed caused by the server, and not an accidental client-side bug). > + */In theory you could parse nbdkit --dump-config, although I agree this approach is fine too. Rich.> p = (struct progress) { .count = 0 }; > r = nbd_opt_list_meta_context (nbd, > (nbd_context_callback) { .callback = check, > .user_data = &p}); > - if (r != -1) { > - fprintf (stderr, "not expecting command to succeed\n"); > + if (r == -1) { > + fprintf (stderr, "Skipping test; server probably too old for listing " > + "without structured replies: %s\n", nbd_get_error ()); > + exit (77); > + } > + if (r != p.count) { > + fprintf (stderr, "inconsistent return value %d, expected %d\n", r, p.count); > + exit (EXIT_FAILURE); > + } > + if (r < 1 || !p.seen) { > + fprintf (stderr, "server did not reply with base:allocation\n"); > exit (EXIT_FAILURE); > } > > + nbd_opt_abort (nbd); > + nbd_close (nbd); > + > exit (EXIT_SUCCESS); > } > -- > 2.37.2 > > _______________________________________________ > Libguestfs mailing list > Libguestfs at redhat.com > https://listman.redhat.com/mailman/listinfo/libguestfs-- 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