Re: [Qemu-devel] [PATCH v2 2/6] nbd/server: refactor negotiation functions parameters

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

On 01/12/2018 04:18 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> hmm, should not it be at the end of nbd_negotiate() ? Looks OK anyway.
>> Sure, I'll squash this in, for no real change in behavior:
>>
>> diff --git i/nbd/server.c w/nbd/server.c
>> index c22d5e6ecf..3fd145592e 100644
>> --- i/nbd/server.c
>> +++ w/nbd/server.c
>> @@ -902,6 +902,7 @@ static coroutine_fn int nbd_negotiate(NBDClient
>> *client, Error **errp)
>>           }
>>       }
>>
>> +    assert(!client->optlen);
>>       trace_nbd_negotiate_success();
>>

This says that after all negotiation is complete, optlen is 0 (so in
reality, it is only checking NBD_OPT_GO and NBD_OPT_EXPORT_NAME - since
those are the only two options that can end negotiation).  But it does
not check state in between individual options during the actual
negotiation.  Also, this is the only spot in the code that can check
that optlen is 0 even for old-style connections (where we do not call
nbd_negotiate_options), so it's a nice spot to prove that when we move
into transmission phase, we aren't stranding any unread data from
handshake phase.

> 
> 
> or, even better, in nbd_negotiate_options. and you actually do it in 5/6.

Whereas that is stating that optlen is 0 after every option that does
not return early (not possible in this patch, because we need the
nbd_opt_read() helper in 5/6 that clears optlen as we go) - and
meanwhile does NOT cover the places that return early (NBD_OPT_GO and
NBD_OPT_EXPORT_NAME, which we caught here).  So we need both assertions
at the end of the series, and I'm comfortable with introducing them in
separate patches.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]