Re: [Qemu-devel] [PATCH v5 06/11] nbd: Minimal structured read for server

[Eric Blake <eblake@redhat.com>]

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

On 10/20/2017 02:03 PM, Vladimir Sementsov-Ogievskiy wrote:
> 20.10.2017 01:26, Eric Blake wrote:
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> Minimal implementation of structured read: one structured reply chunk,
>> no segmentation.
>> Minimal structured error implementation: no text message.
>> Support DF flag, but just ignore it, as there is no segmentation any
>> way.
>>

>> +
>> +            case NBD_OPT_STRUCTURED_REPLY:
>> +                if (length) {
>> +                    ret = nbd_check_zero_length(client, length,
>> option, errp);
> 
> here same thing like in previous patch, about success in
> nbd_check_zero_length

Here, successfully answering the client with an error message should NOT
stop negotiating, so THIS return is correct.

> 
>> +                } else if (client->structured_reply) {
>> +                    ret = nbd_negotiate_send_rep_err(
>> +                        client->ioc, NBD_REP_ERR_INVALID, option, errp,
>> +                        "structured reply already negotiated");
>> +                } else {
>> +                    ret = nbd_negotiate_send_rep(client->ioc,
>> NBD_REP_ACK,
>> +                                                 option, errp);
>> +                }
> 
> you've dropped "if (ret < 0) { return ret }", but the following two
> lines should not be
> executed if ret < 0.. May be it doesn't matter (we will abort connection
> anyway after switch {}) but
> it looks strange.
> 
>> +                client->structured_reply = true;
>> +                myflags |= NBD_FLAG_SEND_DF;
>> +                break;

Indeed, these two lines are harmless due to the catch-all 'ret < 0'
after the switch at the tail end of the loop (which is why I dropped the
'if' here).


-- 
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 --]