Re: [Qemu-devel] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Paolo Bonzini]

On 14/06/2016 17:02, Alex Bligh wrote:
> 
> On 14 Jun 2016, at 14:32, Paolo Bonzini <address@hidden> wrote:
> 
>>
>> On 13/06/2016 23:41, Alex Bligh wrote:
>>> That's one of the reasons that there is a proposal to add
>>> STRUCTURED_READ to the spec (although I still haven't had time to
>>> implement that for qemu), so that we have a newer approach that allows
>>> for proper error handling without ambiguity on whether bogus bytes must
>>> be sent on a failed read.  But you'd have to convince me that ALL
>>> existing NBD server and client implementations expect to handle a read
>>> error without read payload, otherwise, I will stick with the notion that
>>> the current spec wording is correct, and that read errors CANNOT be
>>> gracefully recovered from unless BOTH sides transfer (possibly bogus)
>>> bytes along with the error message, and which is why BOTH sides of the
>>> protocol are warned that read errors usually result in a disconnection
>>> rather than clean continuation, without the addition of STRUCTURED_READ.
>>
>> I suspect that there are exactly two client implementations,
> 
> My understanding is that there are more than 2 client implementations.
> A quick google found at least one BSD client. I bet read error handling
> is a mess in all of them.

Found it, it is exactly the same as Linux and QEMU:

https://github.com/bitrig/bitrig/blob/418985278/sys/dev/nbd.c#L577

>> namely
>> Linux and QEMU's, and both do the right thing.
> 
> This depends what you mean by 'right'. Both appear to be non-compliant
> with the standard.

I mean "what makes sense".

> Note the standard is not defined by the client implementation, but
> by the protocol document.
> 
> IMHO the 'right thing' is what is in the spec. Servers can't send an
> error in any other way if they don't buffer the entire read first, as the
> read may error towards the end.
> 
> To illustrate the problem, look consider what qemu itself would do as
> a server if it can't buffer the entire read issued to it.

Return ENOMEM?

> The spec originally was not clear on how errors on reads should be
> handled, leading to any read causing a protocol drop. The spec is
> now clear. Unfortunately it is not possible to make a back compatible
> fix. Hence the real fix here is to implement structured replies,
> which is what Eric and I have been working on.

I agree that structured replies are better.  However, it looks like the
de facto status prior to structured replies is that the error is in the
spec, and this patch introduces a regression.

Paolo