Re: [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.

Reviewed-by: Laszlo Ersek <lersek(a)redhat.com&gt;