Re: [Nbd] [PATCH v4 04/11] nbd: Improve server handling of bogus commands
- To: Alex Bligh <alex@...872...>
- Cc: "nbd-general@lists.sourceforge.net" <nbd-general@lists.sourceforge.net>, "qemu-devel@...530..." <qemu-devel@...530...>, qemu block <qemu-block@...530...>
- Subject: Re: [Nbd] [PATCH v4 04/11] nbd: Improve server handling of bogus commands
- From: Paolo Bonzini <pbonzini@...696...>
- Date: Tue, 14 Jun 2016 17:11:43 +0200
- Message-id: <3f9a5b12-67eb-d2f3-f736-22ced5bbf638@...696...>
- In-reply-to: <38ABE56B-CA23-4372-A413-CDA72BDAE86A@...872...>
- References: <1463006384-7734-1-git-send-email-eblake@...696...> <1463006384-7734-5-git-send-email-eblake@...696...> <852e526a-5235-499a-741e-a695f5e74f83@...696...> <575EA656.80508@...696...> <6DD06745-C91C-4BFB-BFE5-92E5982ACB42@...872...> <11f620d2-a51d-5235-5abd-4ced314c9090@...696...> <38ABE56B-CA23-4372-A413-CDA72BDAE86A@...872...>
On 14/06/2016 17:02, Alex Bligh wrote:
>
> On 14 Jun 2016, at 14:32, Paolo Bonzini <pbonzini@...696...> 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
Reply to: