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 <ler...@redhat.com> > ... I meant to request that you please update / move the comment before picking up the R-b. Thanks Laszlo _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs