[Libguestfs] [libnbd PATCH v2 03/12] api: Allow nbd_opt_list_meta_context without SR

[Laszlo Ersek]

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