Re: [Qemu-devel] [Nbd] [PATCHv2] Amend NBD_OPT_SELECT (now NBD_OPT_INFO) and NBD_OPT_GO documentation

[Alex Bligh]

Eric (sic - sorry - long day).

On 5 Apr 2016, at 22:26, Eric Blake <address@hidden> wrote:

> Ah, you were faster than my reply mail; looks like everything I pointed
> out now gets to be a followup patch :)

I agree with all but one of the the changes you put in. As Wouter is the
fastest gun in the west ( :-) ), do you want to do a patch?

I'm also conscious that NBD_REP_SERVER is now used for 2 different
reply formats (actually it always was). Perhaps we should change this
one to NBD_REP_SERVER_EXT (or similar).

My disagreement is this bit:

> Another thing I don't like: By default, NBD_REP_SERVER puts the 'length
> of name' field first; I'm not sure if there's a strong reason to change
> that.

Not strongly felt, but having sat looking at Wireshark for a while,
I prefer things are naturally aligned, but:

> Maybe this layout would be slightly nicer:
> 
> - 32 bits, length of name
> - 64 bits, size of export
> - 16 bits, transmission flags
> - up to 124 bytes, variable based on other negotiated options (default
> of 0 bytes)
> - length of name bytes: name
> 
> where the option reply must be at least (rather than exactly) '*length
> of name* + 14' bytes (and at most length of name + 138), and where the
> tail end *length of name* bytes form the export name (because I _do_
> like your idea of putting variable-length names last), and any other
> intermediate bytes cover the variable information based on other
> negotiated options, such that those other negotiated options can also
> consume some of the 124 reserved zeroes at the end of NBD_EXPORT_NAME's
> reply.  Yeah, it's a bit weird that 'length of name' is separated by a
> variable amount of data before 'name', but it then lets us make the
> 'NBD_OPT_EXPORT_NAME' reply be a strict subset of the 'NBD_REP_SERVER'
> layout, which makes for nicer code sharing on that front (but it's still
> not the same as the original NBD_REP_SERVER sticking all extra
> information after the variable-length name field).


Please no! (or at least not like this)

I think in general we should desperately try and delete the
arbitrary obsession with 124 bytes. And (again, very long day
in which I managed to mis-spell your name) I really don't
see what this is buying us.

What does the 'variable, based on other negotiated options'
contain, and how does the client manage to find the name amongst
it?

If the proposal is that options can somehow contribute to the
'final negotiation', that's fine in principle, but let's keep
it out of this proposal, and perhaps let's have it as
<optionid, length, data> tuples so a protocol analyzer can
parse it. If you want to put in some indicator these might
be present then fine, but I'm guesing transmission flags
could take that as a bit. But if we introduce such tuples,
there's no need to limit them to 124 bytes.

--
Alex Bligh

signature.asc
Description: Message signed with OpenPGP using GPGMail