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

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

Eric,

Thanks for breaking it up.

On 19 Apr 2016, at 16:39, 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.
> 
> Signed-off-by: Eric Blake <eblake@...696...>
> ---
> doc/proto.md | 93 +++++++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 54 insertions(+), 39 deletions(-)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index e484055..46f42ae 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -992,8 +992,7 @@ unrecognized flags.
> These values are used in the "type" field of a structured reply.
> Some chunk types can additionally be categorized by role, such as
> *error chunks* or *content chunks*.  Each type determines how to
> -interpret the "length" bytes of payload.  If the client receives
> -an unknown or unexpected type, it MUST initiate a hard disconnect.

You've moved this normative behaviour down the bottom which is
fine I guess.

By embedding string lengths inside the data
you've made the existing types inherently expandable, which is good.
But you need to make this clearer. I would add "The client MUST skip any data
within the *length* field of the reply header which is unused
by the specified reply types below; this area is reserved for
expansion." (or similar)

> +interpret the "length" bytes of payload.


> 
> - `NBD_REPLY_TYPE_NONE` (0)
> 
> @@ -1004,42 +1003,7 @@ an unknown or unexpected type, it MUST initiate a hard disconnect.
>   chunks were sent, then this type implies that the overall client
>   request is successful.  Valid as a reply to any request.
> 
> -- `NBD_REPLY_TYPE_ERROR` (1)
> -
> -  This chunk type is in the error chunk category.  *length* MUST
> -  be at least 4.  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)  
> -  *length - 4* bytes: optional string suitable for
> -     direct display to a human being  
> -
> -- `NBD_REPLY_TYPE_ERROR_OFFSET` (2)
> -
> -  This chunk type is in the error chunk category.  *length* MUST
> -  be at least 12.  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` and `NBD_CMD_TRIM`.
> -
> -  The payload is structured as:
> -
> -  32 bits: error (MUST be non-zero)  
> -  64 bits: offset (unsigned)  
> -  *length - 12* bytes: optional string suitable for
> -     direct display to a human being  
> -
> -- `NBD_REPLY_TYPE_OFFSET_DATA` (3)
> +- `NBD_REPLY_TYPE_OFFSET_DATA` (1)
> 
>   This chunk type is in the content chunk category.  *length* MUST
>   be at least 9.  It represents the contents of *length - 8* bytes
> @@ -1055,7 +1019,7 @@ an unknown or unexpected type, it MUST initiate a hard disconnect.
>   64 bits: offset (unsigned)  
>   *length - 8* bytes: data  
> 
> -- `NBD_REPLY_TYPE_OFFSET_HOLE` (4)
> +- `NBD_REPLY_TYPE_OFFSET_HOLE` (2)
> 
>   This chunk type is in the content chunk category.  *length* MUST
>   be exactly 12.  It represents that the contents of *hole size*
> @@ -1072,6 +1036,57 @@ an unknown or unexpected type, it MUST initiate a hard disconnect.
>   64 bits: offset (unsigned)  
>   32 bits: hole size (unsigned, MUST be nonzero)  
> 
> +All error chunk types have bit 15 set, and begin with the same
> +*error*, *message length*, and optional *message* fields as
> +`NBD_REPLY_TYPE_ERROR`.  If non-zero, *message length* indicates
> +that an optional error string message appears next, suitable for
> +display to a human user.  The header *length* then covers any
> +remaining structured fields at the end.
> +
> +- `NBD_REPLY_TYPE_ERROR` (2^15 + 1)
> +
> +  This chunk type is in the error chunk category.  *length* MUST
> +  be at least 6.  This chunk represents that an error occurred,
> +  and the client MAY NOT make any assumptions about partial

that should be 'MUST NOT' (existing problem).

> +  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)  
> +  16 bits: message length (no more than header *length* - 6)  
> +  *message length* bytes: optional string suitable for
> +    direct display to a human being

Everywhere else where we have a string we have a 32 byte message
length. For the sake of consistency, given this is hardly
speed criticial, perhaps we could stick with that.

>  
> +
> +- `NBD_REPLY_TYPE_ERROR_OFFSET` (2^15 + 2)
> +
> +  This chunk type is in the error chunk category.  *length* MUST
> +  be at least 14.  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`, and `NBD_CMD_TRIM`.
> +
> +  The payload is structured as:
> +
> +  32 bits: error (MUST be non-zero)  
> +  16 bits: message length (no more than header *length* - 14)  
> +  *message length* bytes: optional string suitable for
> +     direct display to a human being  

Same point re 16/32 bit string lengths

> +  64 bits: offset (unsigned)

Perhaps put the offset before the message? I'm just thinking
about the implementor who would probably read things into
a struct if they have the chance.

That's how it was before, and you seem to have reordered (not
sure why).

Otherwise LGTM.

-- 
Alex Bligh