On 03/29/2016 01:39 PM, Alex Bligh wrote:
> I think we are paying too much attention to trying to keep NBD_RESPONSE
> intact. The justification for this was (I think) that it made it easier
> for existing protocol analysers. It doesn't, really, as all the data
> is going to come BEFORE the NBD_RESPONSE (unlike in NBD_CMD_READ in
> other situations).
> 
> I think we should therefore look at this the other way around. Here's
> a straw man proposal as an alternative for the reply bits. For
> a structured reply ALL we get is the chunks. The final chunk
> (possibly the only chunk) is marked specially. Each chunk looks something
> like:
> 
> offset+
> 0000    32 bit   NBD_STRUCTURED_REPLY_MAGIC
> 0004    64 bit   handle
> 000C    32 bit   Flags
> 0010    32 bit   Payload length
> 
> 
> We have a couple of flags defined:
> 
> NBD_CHUNK_IS_DATA: the chunk is data, and the payload is a 64 bit offset
> plus the data read
> 
> NBD_CHUNK_IS_HOLE: the chunk is zeroes, and the payload is a 64 bit offset
> (only)
> 
> NBD_CHUNK_IS_END: (must be the final chunk). The payload is a 64 bit offset
> plus a 32 bit error code, or zero. If no error, the offset must be set to
> the total amount read. If there is an error, the offset MAY indicate the
> position of the error. If an error occurs, no more chunks should be sent.

I'm liking it - then we aren't sending a mandatory 0 error field on read
chunks.  Although I might use '32 bit Type' rather than '32 bit Flags',
since you don't really want to allow multiple reply types at once;
rather each reply type id is documented on its specific payload layout.

Another argument in favor of this over my original proposal: my proposal
is context-free for determining reply boundaries, but still requires
context in deciphering between a reply to NBD_CMD_READ vs. a reply to
NBD_CMD_GET_LBA_STATUS (that is, there was nothing in the reply that
said what the payload represents, only how many bytes to skip).
However, with a '32 bit Type', the wireshark detector can be taught
every type of payload, and as long as every command with a structured
reply uses 1 or more distinct types, the dissector can display more
details about the payload when it recognizes the type, and still skip
the payload on extensions it does not recognize.

> 
> 4. It would be possible to allow EVERY server reply to be a structured
>    reply that simply set NBD_CHUNK_IS_END. That gives us a convenient
>    route to servers which only implement structured replies. With DF,
>    this would be little harder than implementing the current
>    protocol.

For all remaining existing commands, that is just more overhead on the
wire.  The existing non-structured replies do not send any data; they
are 16 bytes each (only NBD_CMD_READ sends more than 16 bytes in one
reply).  But your proposal inflates that to a minimum of 20 bytes (if
length is 0) or longer (if an error is set).  I'm still strongly in
favor of keeping the existing non-structured replies to commands that
don't have to return data.

I'm okay if a new command sometimes returns data and sometimes does not;
in which case, that command can always return a single structured reply
where the id of the reply says whether the payload will be length 0 or
not, but only new commands should get that treatment.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org