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
