[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

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

Eric,

I spent some time looking at this and am finding it very difficult
to differentiate between

a) whitespace changes
b) movement of the text within the document
c) substantive changes.

Could you break it up into a set of patches that
does each of those separately? Or at least flag the
changes somehow?

On the reorganisation, I know the document often has
an overview bit, and the detail later, but it seems
now we are splitting structured replies into
three sections:

#### Structured reply chunk message

and

*Structured replies*

and 

#### Structured Reply flags

And the thing defining structured replies comes later.

This seems a bit strange and now means you need to
hunt *more* around the document.

To be honest I wasn't fantastically happy with the old layout
where structured replies are described in detail under
NBD_CMD_READ (that's the middle of those).

I wonder if what we need is a new section called 'structured
replies' with everything in there, a much smaller intro
section up top, and a reference to it under NBD_CMD_READ.

Alex

On 18 Apr 2016, at 18:07, 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.
> 
> Furthermore, the rest of the document has a general pattern of
> high-level overview in the first half, then details about constants
> in the second half.  By that layout, the details on structured
> reply chunk flags and types belongs in the second half, alongside
> other transmission phase constants.
> 
> Signed-off-by: Eric Blake <eblake@...696...>
> ---
> 
> For the extensions-structured-reply branch
> 
> doc/proto.md | 225 +++++++++++++++++++++++++++++++----------------------------
> 1 file changed, 120 insertions(+), 105 deletions(-)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index 29ef281..322da0c 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -355,7 +355,7 @@ S: (*length* bytes of data if the request is of type `NBD_CMD_READ`)
> Some of the major downsides of the default simple reply to
> `NBD_CMD_READ` are as follows.  First, it is not possible to support
> partial reads or early errors (the command must succeed or fail as a
> -whole, and either len bytes of data must be sent or a hard disconnect
> +whole, and either *length* bytes of data must be sent or a hard disconnect
> must be initiated, even if the failure is `EINVAL` due to bad flags).
> Second, there is no way to efficiently skip over portions of a sparse
> file that are known to contain all zeroes.  Finally, it is not
> @@ -373,7 +373,7 @@ a simple reply for `NBD_CMD_READ` (even for the case of an early
> `EINVAL` due to bad flags), but MAY use either a simple reply or a
> structured reply to all other requests.  The server SHOULD prefer
> sending errors via a structured reply, as the error can then be
> - accompanied by a string payload to present to a human user.
> +accompanied by a string payload to present to a human user.
> 
> A structured reply MAY occupy multiple structured chunk messages
> (all with the same value for "handle"), and the
> @@ -404,109 +404,6 @@ The use of *length* in the reply allows context-free division of
> the overall server traffic into individual reply messages; the
> *type* field describes how to further interpret the payload.
> 
> -##### Structured reply flags
> -
> -This field of 16 bits is sent by the server as part of every
> -structured reply.
> -
> -- bit 0, `NBD_REPLY_FLAG_DONE`; the server MUST clear this bit if
> -  more structured reply chunks will be sent for the same client
> -  request, and MUST set this bit if this is the final reply.  This
> -  bit MUST always be set for the `NBD_REPLY_TYPE_NONE` chunk,
> -  although any other chunk type can also be used as the final
> -  chunk.
> -
> -The server MUST NOT set any other flags without first negotiating
> -the extension with the client, unless the client can usefully
> -react to the response without interpreting the flag (for instance
> -if the flag is some form of hint).  Clients MUST ignore
> -unrecognized flags.
> -
> -##### Structured reply types
> -
> -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.
> -
> -- `NBD_REPLY_TYPE_NONE` (0)
> -
> -  *length* MUST be 0 (and the payload field omitted).  This chunk
> -  type MUST always be used with the `NBD_REPLY_FLAG_DONE` bit set
> -  (that is, it may appear at most once in a structured reply, and
> -  is only useful as the final reply chunk).  If no earlier error
> -  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)
> -
> -  This chunk type is in the content chunk category.  *length* MUST
> -  be at least 9.  It represents the contents of *length - 8* bytes
> -  of the file, starting at *offset*.  The data MUST lie within the
> -  bounds of the original offset and length of the client's
> -  request, and MUST NOT overlap with the bounds of any earlier
> -  content chunk or error chunk in the same reply.  This chunk MAY
> -  be used more than once in a reply, unless the `NBD_CMD_FLAG_DF`
> -  flag was set.  Valid as a reply to `NBD_CMD_READ`.
> -
> -  The payload is structured as:
> -
> -  64 bits: offset (unsigned)  
> -  *length - 8* bytes: data  
> -
> -- `NBD_REPLY_TYPE_OFFSET_HOLE` (4)
> -
> -  This chunk type is in the content chunk category.  *length* MUST
> -  be exactly 12.  It represents that the contents of *hole size*
> -  bytes starting at *offset* read as all zeroes.  The hole MUST
> -  lie within the bounds of the original offset and length of the
> -  client's request, and MUST NOT overlap with the bounds of any
> -  earlier content chunk or error chunk in the same reply.  This
> -  chunk MAY be used more than once in a reply, unless the
> -  `NBD_CMD_FLAG_DF` flag was set.  Valid as a reply to
> -  `NBD_CMD_READ`.
> -
> -  The payload is structured as:
> -
> -  64 bits: offset (unsigned)  
> -  32 bits: hole size (unsigned, MUST be nonzero)  
> -
> #### Terminating the transmission phase
> 
> There are two methods of terminating the transmission phase:
> @@ -1244,6 +1141,124 @@ The following request types exist:
>     Currently one such message is known: `NBD_CMD_CACHE`, with type set to
>     5, implemented by xnbd.
> 
> +#### Structured Reply flags
> +
> +    This field of 16 bits is sent by the server as part of every
> +    structured reply.
> +
> +    - bit 0, `NBD_REPLY_FLAG_DONE`; the server MUST clear this bit if
> +      more structured reply chunks will be sent for the same client
> +      request, and MUST set this bit if this is the final reply.  This
> +      bit MUST always be set for the `NBD_REPLY_TYPE_NONE` chunk,
> +      although any other chunk type can also be used as the final
> +      chunk.
> +
> +    The server MUST NOT set any other flags without first negotiating
> +    the extension with the client, unless the client can usefully
> +    react to the response without interpreting the flag (for instance
> +    if the flag is some form of hint).  Clients MUST ignore
> +    unrecognized flags.
> +
> +#### Structured Reply types
> +
> +    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.
> +
> +    - `NBD_REPLY_TYPE_NONE` (0)
> +
> +      *length* MUST be 0 (and the payload field omitted).  This chunk
> +      type MUST always be used with the `NBD_REPLY_FLAG_DONE` bit set
> +      (that is, it may appear at most once in a structured reply, and
> +      is only useful as the final reply chunk).  If no earlier error
> +      chunks were sent, then this type implies that the overall client
> +      request is successful.  Valid as a reply to any request.
> +
> +    - `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
> +      of the file, starting at *offset*.  The data MUST lie within the
> +      bounds of the original offset and length of the client's
> +      request, and MUST NOT overlap with the bounds of any earlier
> +      content chunk or error chunk in the same reply.  This chunk MAY
> +      be used more than once in a reply, unless the `NBD_CMD_FLAG_DF`
> +      flag was set.  Valid as a reply to `NBD_CMD_READ`.
> +
> +      The payload is structured as:
> +
> +      64 bits: offset (unsigned)  
> +      *length - 8* bytes: data  
> +
> +    - `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*
> +      bytes starting at *offset* read as all zeroes.  The hole MUST
> +      lie within the bounds of the original offset and length of the
> +      client's request, and MUST NOT overlap with the bounds of any
> +      earlier content chunk or error chunk in the same reply.  This
> +      chunk MAY be used more than once in a reply, unless the
> +      `NBD_CMD_FLAG_DF` flag was set.  Valid as a reply to
> +      `NBD_CMD_READ`.
> +
> +      The payload is structured as:
> +
> +      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
> +      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  
> +
> +    - `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  
> +      64 bits: offset (unsigned)  
> +
> +    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 *message length* is
> +    too large to fit within the *length* specified by the header.  For
> +    all other messages with unknown or unexpected type or inconsistent
> +    contents, the client MUST initiate a hard disconnect.
> +
> #### Error values
> 
> The error values are used for the error field in the reply message.
> -- 
> 2.5.5
> 
> 
> ------------------------------------------------------------------------------
> Find and fix application performance issues faster with Applications Manager
> Applications Manager provides deep performance insights into multiple tiers of
> your business applications. It resolves application problems quickly and
> reduces your MTTR. Get your free trial!
> https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
> _______________________________________________
> Nbd-general mailing list
> Nbd-general@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nbd-general
> 

-- 
Alex Bligh
Reply to: