Re: [Nbd] [RFC PATCH] doc: In STRUCTURED_REPLY, make error types easy to recognize

[Alex Bligh <alex@...872...>]

On 8 Apr 2016, at 17:48, Eric Blake <eblake@...696...> wrote:
> We may add future structured error replies; making it easy
> for older clients to properly treat such new reply types as
> an error gives us a bit more flexibility on introducing new
> errors to existing commands.  Of course, good design practice
> says we should try hard to prevent the server from sending
> something the client isn't expecting, but covering the
> situation from both sides never hurts.  Also, marking error
> types resembles how NBD_REP_ERR_* has a common layout.

Having the error chunks with bit 15 set is a good plan.

Not 100% sure about the other bits ...

> +    All error chunk types have bit 15 set, and begin with the same
> +    *error* and *message length* fields as `NBD_REPLY_TYPE_ERROR`
> +    (note that *message length* is optional only in
> +    `NBD_REPLY_TYPE_ERROR`; it MUST be present in all other error
> +    types).  If non-zero, *message length* indicates that an optional
> +    error string message occupies the final bytes of the chunk,
> +    suitable for display to a human user.
> +
> +    - `NBD_REPLY_TYPE_ERROR` (2^15 + 1)

Whilst you are playing with these:

NBD_REP_CHUNK_TYPE_ERROR ? (etc.)

> +
> +      This chunk type is in the error chunk category.  *length* MUST
> +      be either exactly 4 or at least 8.  As a special case, the
> +      server MAY omit the *message length* field if there is no
> +      message string.  This chunk represents that an error occurred,
> +      and the client MAY NOT make any assumptions about partial
> +      success. This type SHOULD NOT be used more than once in a
> +      structured reply.  Valid as a reply to any request.
> +
> +      The payload is structured as:
> +
> +      32 bits: error (MUST be nonzero)  
> +      32 bits: message length, optional, but MUST be the header
> +        *length* - 8 if present, and MUST be present if a message is
> +	included  
> +      *message length* bytes: optional string suitable for
> +        direct display to a human being  

This seems overly complicated which means someone will write
something that doesn't parse it correct.

Why not make the length mandatory, or absent (i.e. just work
it out from the chunk length)? The 'optional' case is
bizarre. you're optimising to save a single 32 bit word
transmission *in an error* condition. It's far more likely no
one ever tests one of the two ways of transmitting errors, than
the 'speed of reporting disk errors' makes a difference. 

Making it mandatory would equate it to NBD_REPLY_TYPE_ERROR_OFFSET
in the new form; making it absent would equate it to what
NBD_REPLY_TYPE_ERROR_OFFSET was before.

> +    - `NBD_REPLY_TYPE_ERROR_OFFSET` (2^15 + 2)
> +
> +      This chunk type is in the error chunk category.  *length* MUST
> +      be at least 16.  This reply represents that an error occurred at
> +      a given offset, which MUST lie within the original offset and
> +      length of the request; the client can use this offset to
> +      determine if request had any partial success.  This chunk type
> +      MAY appear multiple times in a structured reply, although the
> +      same offset SHOULD NOT be repeated.  Likewise, if content chunks
> +      were sent earlier in the structured reply, the server SHOULD NOT
> +      send multiple distinct offsets that lie within the bounds of a
> +      single content chunk.  Valid as a reply to `NBD_CMD_READ`,
> +      `NBD_CMD_WRITE`, `NBD_CMD_WRITE_ZEROES`, and `NBD_CMD_TRIM`.
> +
> +      The payload is structured as:
> +
> +      32 bits: error (MUST be non-zero)  
> +      32 bits: message length, MUST be *length* - 16
> +      64 bits: offset (unsigned)  
> +      *message length* bytes: optional string suitable for
> +         direct display to a human being  

Firstly, surely we can have the length of the message
next to the message!

Secondly, I'm guessing the purpose of intoducing
a new field here (message length) whose value we already
know (message length -16) is for forward compatibility, i.e.
so at a later stage we could add more structured error
stuff and clients could skip the things after the message
they didn't understand using the chunk's 'length' field.
That's great, fine in principle, but ...

> +    If the client receives an unknown or unexpected type with bit 15
> +    set, it MUST consider the current reply as errored, but MAY
> +    continue transmission unless it detects that *length* is
> +    inconsistent with *message length*.

... such a 'new' server which inserts something after the 'message'
field is going to have exactly that effect, so you are now mandating
it close the connection. I thus don't understand the purpose.

-- 
Alex Bligh