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

[Wouter Verhelst]

On Wed, Jun 15, 2016 at 11:27:21AM +0100, Alex Bligh wrote:
> Perhaps this should read "If an error occurs, the server MUST either initiate
> a hard disconnect before the entire payload has been sent or
> set the appropriate code in the error field and send the response header
> without any payload." if we want to go down this route.

Something along those lines is what I meant, yes.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
   people in the world who think they really understand all of its rules,
   and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

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

[Paolo Bonzini]

On 15/06/2016 12:27, Alex Bligh wrote:
> 
> On 15 Jun 2016, at 10:18, Paolo Bonzini  wrote:
> 
>>> So what should those servers do (like 2 of mine) which don't buffer
>>> the entire read, if they get an error having already sent some data?
>>
>> They have sent an error code of zero, and it turned out to be wrong.  So
>> the only thing they can do safely is disconnect.
> 
> Right, but that is not what Wouter's change says:
> 
> +If an error occurs, the server SHOULD set the appropriate error code
> +in the error field. The server MAY then initiate a hard disconnect.
> +If it chooses not to, it MUST NOT send any payload for this request.
> 
> I read this as either
> 
> a) the server can issue a hard disconnect without sending any reply; or
> 
> b) it must send the reply header with no payload
> 
> It also seems to permit not setting the error code (it's only a 'SHOULD'),
> not disconnecting (it's a MAY), then not sending any payload, which is a
> nonsense.

Right.

> Perhaps this should read "If an error occurs, the server MUST either initiate
> a hard disconnect before the entire payload has been sent or
> set the appropriate code in the error field and send the response header
> without any payload." if we want to go down this route.

Yes, I agree.

I do believe we want to go down this route.  I think we agree that
partial buffering may always require the server to disconnect after an
error.  Therefore I don't see any benefit at all in sending a payload
after an error message.

Paolo

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

[Alex Bligh]

On 15 Jun 2016, at 10:18, Paolo Bonzini  wrote:

>> So what should those servers do (like 2 of mine) which don't buffer
>> the entire read, if they get an error having already sent some data?
> 
> They have sent an error code of zero, and it turned out to be wrong.  So
> the only thing they can do safely is disconnect.

Right, but that is not what Wouter's change says:

+If an error occurs, the server SHOULD set the appropriate error code
+in the error field. The server MAY then initiate a hard disconnect.
+If it chooses not to, it MUST NOT send any payload for this request.

I read this as either

a) the server can issue a hard disconnect without sending any reply; or

b) it must send the reply header with no payload

It also seems to permit not setting the error code (it's only a 'SHOULD'),
not disconnecting (it's a MAY), then not sending any payload, which is a
nonsense.

Perhaps this should read "If an error occurs, the server MUST either initiate
a hard disconnect before the entire payload has been sent or
set the appropriate code in the error field and send the response header
without any payload." if we want to go down this route.


-- 
Alex Bligh

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

[Paolo Bonzini]

- Original Message -
> From: "Alex Bligh" 
> To: "Wouter Verhelst" 
> Cc: "Alex Bligh" , nbd-gene...@lists.sourceforge.net, 
> "Paolo Bonzini" ,
> qemu-devel@nongnu.org, "qemu block" 
> Sent: Wednesday, June 15, 2016 10:52:21 AM
> Subject: Re: [Nbd] [PATCH v4 04/11] nbd: Improve server handling of bogus 
> commands
> 
> 
> > On 15 Jun 2016, at 09:03, Wouter Verhelst  wrote:
> > 
> > On Wed, Jun 15, 2016 at 09:05:22AM +0200, Wouter Verhelst wrote:
> >> There are more clients than the Linux and qemu ones, but I think it's
> >> fair to say that those two are the most important ones. If they agree
> >> that a read reply which errors should come without payload, then I think
> >> we should update the standard to say that, too.
> > 
> > I've just pushed a commit that changes the spec (and the implementation)
> > so that if a server encounters a read error, it does not send a payload.
> > 
> > In other words, the current behaviour of qemu is correct, is now
> > documented to be correct, and should not be changed.
> 
> So what should those servers do (like 2 of mine) which don't buffer
> the entire read, if they get an error having already sent some data?

They have sent an error code of zero, and it turned out to be wrong.  So
the only thing they can do safely is disconnect.

Paolo

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

[Alex Bligh]

> On 15 Jun 2016, at 09:03, Wouter Verhelst  wrote:
> 
> On Wed, Jun 15, 2016 at 09:05:22AM +0200, Wouter Verhelst wrote:
>> There are more clients than the Linux and qemu ones, but I think it's
>> fair to say that those two are the most important ones. If they agree
>> that a read reply which errors should come without payload, then I think
>> we should update the standard to say that, too.
> 
> I've just pushed a commit that changes the spec (and the implementation)
> so that if a server encounters a read error, it does not send a payload.
> 
> In other words, the current behaviour of qemu is correct, is now
> documented to be correct, and should not be changed.

So what should those servers do (like 2 of mine) which don't buffer
the entire read, if they get an error having already sent some data?

--
Alex Bligh






signature.asc
Description: Message signed with OpenPGP using GPGMail

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

[Wouter Verhelst]

On Wed, Jun 15, 2016 at 09:05:22AM +0200, Wouter Verhelst wrote:
> There are more clients than the Linux and qemu ones, but I think it's
> fair to say that those two are the most important ones. If they agree
> that a read reply which errors should come without payload, then I think
> we should update the standard to say that, too.

I've just pushed a commit that changes the spec (and the implementation)
so that if a server encounters a read error, it does not send a payload.

In other words, the current behaviour of qemu is correct, is now
documented to be correct, and should not be changed.

Regards,

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
   people in the world who think they really understand all of its rules,
   and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12


signature.asc
Description: PGP signature

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

[Wouter Verhelst]

On Tue, Jun 14, 2016 at 04:02:15PM +0100, Alex Bligh wrote:
> 
> On 14 Jun 2016, at 14:32, Paolo Bonzini  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.
> 
> > 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.
> 
> Note the standard is not defined by the client implementation, but
> by the protocol document.

No, it isn't. At least not yet.

The standard document has only become formal a few months ago. It's
certainly possible that we made a mistake formalizing things, and if so,
we should fix the document rather than saying "whatever you've been
doing these years, even though it worked, is wrong".

There are more clients than the Linux and qemu ones, but I think it's
fair to say that those two are the most important ones. If they agree
that a read reply which errors should come without payload, then I think
we should update the standard to say that, too.

> > What servers do doesn't matter, if all the clients agree.
> 
> 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.

That much, at any rate, is true.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
   people in the world who think they really understand all of its rules,
   and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

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

[Wouter Verhelst]

On Mon, Jun 13, 2016 at 10:41:05PM +0100, Alex Bligh wrote:
> For amusement value, the non-threaded handler (which is not used
> any more) does not send any payload on an error:
>  https://github.com/yoe/nbd/blob/master/nbd-server.c#L1734

nbd-server used to just drop the connection on read error.

> In essence read error handling is a horrible mess in NBD, and
> I would not expect it to work in general :-(

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
   people in the world who think they really understand all of its rules,
   and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12