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

[Eric Blake <eblake@redhat.com>]

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

On 10/20/2017 02:30 PM, Vladimir Sementsov-Ogievskiy wrote:

>>>> +                } 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).
>>
>>
> 
> yes. but it looks strange) Anyway,
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 

On second thought, modifying client->structured_reply when we
successfully replied with an error is wrong.  So v6 will hoist the
modifications into the last else branch.

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