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>
Laszlo Ersek
2022-Sep-01 09:26 UTC
[Libguestfs] [libnbd PATCH v2 03/12] api: Allow nbd_opt_list_meta_context without SR
On 09/01/22 11:21, 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". :) > > 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. >> Reviewed-by: Laszlo Ersek <lersek at redhat.com> >... I meant to request that you please update / move the comment before picking up the R-b. Thanks Laszlo
Eric Blake
2022-Sep-01 14:55 UTC
[Libguestfs] [libnbd PATCH v2 03/12] api: Allow nbd_opt_list_meta_context without SR
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". :)Yes, that's a side ask, but I would love it too. Git has a default xfuncname pattern for .c files, so it will be interesting to see if I can figure out .gitattributes that would let us attach additional patterns on to the generator/*.c files without penalizing normal .c files.> > 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.Indeed it is. I already posted a followup patch to 11/12 where I improved the comment to its final state; I will grab the appropriate parts of that comment and revise it through the series to match changes as they come in. For 2/12, that results in: diff --git c/generator/states-newstyle-opt-meta-context.c w/generator/states-newstyle-opt-meta-context.c index a6a5271a..dd5a6ded 100644 --- c/generator/states-newstyle-opt-meta-context.c +++ w/generator/states-newstyle-opt-meta-context.c @@ -23,9 +23,27 @@ STATE_MACHINE { size_t i; uint32_t len, opt; - /* If the server doesn't support SRs then we must skip this group. - * Also we skip the group if the client didn't request any metadata - * contexts, when doing SET (but an empty LIST is okay). + /* This state group is reached from: + * h->opt_mode == false (h->current_opt == 0): + * nbd_connect_*() + * -> conditionally use SET, next state OPT_GO for NBD_OPT_GO + * h->opt_mode == true (h->current_opt matches calling API): + * nbd_opt_info() + * -> conditionally use SET, next state OPT_GO for NBD_OPT_INFO + * nbd_opt_go() + * -> conditionally use SET, next state OPT_GO for NBD_OPT_GO + * nbd_opt_list_meta_context() + * -> conditionally use LIST, next state NEGOTIATING + * + * For now, we start by unconditionally clearing h->exportsize and friends, + * as well as h->meta_contexts and h->meta_valid. + * SET then manipulates h->meta_contexts, and sets h->meta_valid on success. + * If SET is conditional, we skip it if structured replies were + * not negotiated, or if there were no contexts to request. + * If OPT_GO is later successful, it populates h->exportsize and friends, + * and also sets h->meta_valid if we skipped SET here. + * LIST is conditional, skipped if structured replies were not negotiated. + * There is a callback if and only if the command is LIST. */ assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE); nbd_internal_reset_size_and_flags (h); For this patch, it changes as: diff --git c/generator/states-newstyle-opt-meta-context.c w/generator/states-newstyle-opt-meta-context.c index c45ff129..020a7adf 100644 --- c/generator/states-newstyle-opt-meta-context.c +++ w/generator/states-newstyle-opt-meta-context.c @@ -33,7 +33,7 @@ STATE_MACHINE { * nbd_opt_go() * -> conditionally use SET, next state OPT_GO for NBD_OPT_GO * nbd_opt_list_meta_context() - * -> conditionally use LIST, next state NEGOTIATING + * -> unconditionally use LIST, next state NEGOTIATING * * For now, we start by unconditionally clearing h->exportsize and friends, * as well as h->meta_contexts and h->meta_valid. @@ -42,8 +42,7 @@ STATE_MACHINE { * not negotiated, or if there were no contexts to request. * If OPT_GO is later successful, it populates h->exportsize and friends, * and also sets h->meta_valid if we skipped SET here. - * LIST is conditional, skipped if structured replies were not negotiated. - * There is a callback if and only if the command is LIST. + * There is a callback if and only if the command is unconditional. */ assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE); nbd_internal_reset_size_and_flags (h); In 4/12, it is further modified: diff --git c/generator/states-newstyle-opt-meta-context.c w/generator/states-newstyle-opt-meta-context.c index 37022594..28100199 100644 --- c/generator/states-newstyle-opt-meta-context.c +++ w/generator/states-newstyle-opt-meta-context.c @@ -35,13 +35,12 @@ STATE_MACHINE { * nbd_opt_list_meta_context() * -> unconditionally use LIST, next state NEGOTIATING * - * For now, we start by unconditionally clearing h->exportsize and friends, - * as well as h->meta_contexts and h->meta_valid. - * SET then manipulates h->meta_contexts, and sets h->meta_valid on success. - * If SET is conditional, we skip it if structured replies were - * not negotiated, or if there were no contexts to request. + * For now, we start by unconditionally clearing h->exportsize and friends. + * If SET is conditional, we skip it if h->request_meta is false, if + * structured replies were not negotiated, or if no contexts to request. * If OPT_GO is later successful, it populates h->exportsize and friends, - * and also sets h->meta_valid if we skipped SET here. + * and also sets h->meta_valid if h->request_meta but we skipped SET here. + * SET then manipulates h->meta_contexts, and sets h->meta_valid on success. * There is a callback if and only if the command is unconditional. */ assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE); And having typed that up, I'm now starting to think I should tweak patch 7/12 to move the call to nbd_internal_reset_size_and_flags() out of NEWSTYLE.OPT_META_CONTEXT.START and into NEWSTYLE.OPT_GO.START, to further solidify that it is the act of NBD_OPT_INFO/GO that wipes h->exportsize, not the act of NBD_OPT_SET/LIST_META_CONTEXT.> > +++ b/tests/opt-list-meta.c...> > > > + 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?nbd_opt_abort() is optional; it says to inform the server of our intent to do a clean disconnect during negotiation. If you skip it and just do nbd_close(), the server is more likely to see an unexpected EOF and warn that we went away unexpectedly. It is a direct counterpart to nbd_shutdown(0) informing the server that we want a clean disconnect during transmission. As for what we can do between the two - nbd_opt_abort() generally moves the handle to state CLOSED, but there you can still do things like nbd_get_size() to see what had happened while the handle was alive; while nbd_close() wipes the handle altogether.> > Reviewed-by: Laszlo Ersek <lersek at redhat.com>Should I apply your R-b with my incremental doc tweaks incorporated along the way, or are you going to want to see a v3 respin with everything in one place (especially now that I'm thinking of further tweaks in 7/12)? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2022-Sep-01 16:25 UTC
[Libguestfs] [libnbd PATCH v2 03/12] api: Allow nbd_opt_list_meta_context without SR
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 <lersek at redhat.com> > _______________________________________________ > 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 Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW