Re: [Qemu-devel] [PATCH for-4.0] nbd: Permit simple error to NBD_CMD_BLOCK_STATUS

[Eric Blake]

On 3/25/19 11:21 AM, Eric Blake wrote:

>>>>> -    if (!extent->length && !iter.err) {
>>>>> -        error_setg(&iter.err,
>>>>> -                   "Server did not reply with any status extents");
>>>>> +    if (!extent->length && !iter.request_ret) {
>>>>
>>>> Hmm, I don't see, what is changed. You just don't set ret and err if there 
>>>> already
>>>> set request_ret.. So, finally it may lead to other return code in 
>>>> nbd_client_co_block_status
>>>> and local_err will not be set and traced. But there is no connection close 
>>>> in this case
>>>> in current code, s->quit is not set.
>>>
>>> And that's okay. The whole point of this is that if the server replied
>>> with an error message (iter.request_ret), then it is okay that the
>>> server did not give us any extent->length, and the caller will turn
>>> iter.request_ret into the EINVAL reply (or whatever other errno value
>>> the server sent) to .bdrv_co_block_status() while keeping the connection
>>> alive.  But if the server did NOT send an error reply (iter.request_ret
>>> is 0), but also did not send us any status, then we need to turn that
>>> into an error condition (because our caller expected us to make progress
>>> on success, even though the server did not make progress).
>>
>>
>> I just say, that I don't see "the client treats the server's
>> reply as fatal and hangs up on the connection" in this place before your 
>> change.
>> Or what you mean?

Okay, maybe I'm seeing what you are asking. When I reran my testing, I
note that iter.err is left unset when iter.request_ret is set.  So my
earlier message was wrong:


> So, the full setup of things to test:
> 
> server that always fails with structured error:
>  - client before 7f86068d (qemu 3.1): connection stays alive, regardless
> of this patch
>  - client after 7f86068d: connection stays alive, regardless of this patch

The connection does not die, but this patch still matters:

- without the second half of this patch, the error given to the caller
is always EIO, regardless of what the server reported
- with the second half of this patch, the error given to the caller is
the error presented by the server. (The first half of the patch doesn't
matter).

> 
> server that always fails with simple error:
>  - client before 7f86068d: (second half of patch does not apply, so only
> the first half is needed):
>    - without first half, connection dies because "Protocol error: simple
> reply when structured reply chunk was expected"
>    - with first half, connection stays alive as it does for structured error
>  - client after 7f86068d, with various stages of this patch:
>    - without either half (or with second half only), connection dies
> because "Protocol error: simple reply when structured reply chunk was
> expected"
>    - with first half, connection dies because "server did not reply with
> any status extents"

the connection does NOT die in this case, but without the second half,
the caller gets EIO regardless of what the server passed.

>    - with both halves, connection stays alive

and here, the caller gets the server's error.

Okay, with your insistence on making me explain myself, I need to submit
a v2 of this patch, split into two pieces, and with a better commit message.

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

signature.asc
Description: OpenPGP digital signature