On Thu, Sep 01, 2022 at 11:21:38AM +0200, Laszlo Ersek wrote:
> 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". :)
It took me a while to work out what this means, but yes I agree.
It seems the following should work but does not for me:
.git/config add:
[diff "nbdstate"]
xfuncname = "^ [A-Z.]*:$"
.git/info/attributes:
generator/states-*.c diff=nbdstate
Rich.
> 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
--
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://listman.redhat.com/mailman/listinfo/libguestfs
