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 <ler...@redhat.com>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
