Eric, Thanks for breaking it up.
On 19 Apr 2016, at 16:39, Eric Blake <[email protected]> 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 <[email protected]> > --- > 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 ------------------------------------------------------------------------------ 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 [email protected] https://lists.sourceforge.net/lists/listinfo/nbd-general
