Re: [PATCH v2 3/6] spec: Add NBD_OPT_EXTENDED_HEADERS

[Wouter Verhelst]

On Mon, Nov 14, 2022 at 04:46:52PM -0600, Eric Blake wrote:
[...]
> @@ -1370,9 +1475,10 @@ of the newstyle negotiation.
>      Return a list of `NBD_REP_META_CONTEXT` replies, one per context,
>      followed by an `NBD_REP_ACK` or an error.
> 
> -    This option SHOULD NOT be requested unless structured replies have
> -    been negotiated first. If a client attempts to do so, a server
> -    MAY send `NBD_REP_ERR_INVALID`.
> +    This option SHOULD NOT be requested unless structured replies or
> +    extended headers have been negotiated first. If a client attempts
> +    to do so, a server MAY send `NBD_REP_ERR_INVALID` or
> +    `NBD_REP_ERR_EXT_HEADER_REQD`.

Is it the intent that NBD_REP_ERR_EXT_HEADER_REQD means structured
replies are not supported by this server? I think that could be
clarified here.

(this occurs twice)

[...]
> +* `NBD_OPT_EXTENDED_HEADERS` (11)
> +
> +    The client wishes to use extended headers during the transmission
> +    phase.  The client MUST NOT send any additional data with the
> +    option, and the server SHOULD reject a request that includes data
> +    with `NBD_REP_ERR_INVALID`.
> +
> +    When successful, this option takes precedence over structured
> +    replies.  A client MAY request structured replies first, although
> +    a server SHOULD support this option even if structured replies are
> +    not negotiated.
> +
> +    It is envisioned that future extensions will add other new
> +    requests that support a data payload in the request or reply.  A
> +    server that supports such extensions SHOULD NOT advertise those
> +    extensions until the client has negotiated extended headers; and a
> +    client MUST NOT make use of those extensions without first
> +    enabling support for reply payloads.
> +
> +    The server replies with the following, or with an error permitted
> +    elsewhere in this document:
> +
> +    - `NBD_REP_ACK`: Extended headers have been negotiated; the client
> +      MUST use the 32-byte extended request header, with proper use of
> +      `NBD_CMD_FLAG_PAYLOAD_LEN` for all commands sending a payload;
> +      and the server MUST use the 32-byte extended reply header.
> +    - For backwards compatibility, clients SHOULD be prepared to also
> +      handle `NBD_REP_ERR_UNSUP`; in this case, only the compact
> +      transmission headers will be used.
> +
> +    Note that a response of `NBD_REP_ERR_BLOCK_SIZE_REQD` does not
> +    make sense in response to this command, but a server MAY fail with
> +    that error for a later `NBD_OPT_GO` without a client request for
> +    `NBD_INFO_BLOCK_SIZE`, since the use of extended headers provides
> +    more incentive for a client to promise to obey block size
> +    constraints.
> +
> +    If the client requests `NBD_OPT_STARTTLS` after this option, it
> +    MUST renegotiate extended headers.
> +

Does it make sense here to also forbid use of NBD_OPT_EXPORT_NAME? I
think the sooner we get rid of that, the better ;-)

[...]
> @@ -1746,13 +1914,15 @@ 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, other than an *error chunk*, it
> -MUST initiate a hard disconnect.  A server MUST NOT send a chunk
> -larger than any advertised maximum block payload size.
> +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*, *content chunks*, or *status chunks*.  Each type determines
> +how to interpret the "length" bytes of payload.  If the client
> +receives an unknown or unexpected type, other than an *error chunk*,
> +it MAY initiate a hard disconnect on the grounds that the client is
> +uncertain whether the server handled the request as desired.  A server
> +MUST NOT send a chunk larger than any advertised maximum block payload
> +size.

Why do we make this a MAY rather than a MUST?

Also, should this section say "structured or extended reply"? We use the
same types for both.

[...]
> +* `NBD_REPLY_TYPE_BLOCK_STATUS_EXT` (6)
> +
> +  This chunk type is in the status chunk category.  *length* MUST be
> +  8 + (a positive multiple of 16).  The semantics of this chunk mirror
> +  those of `NBD_REPLY_TYPE_BLOCK_STATUS`, other than the use of a
> +  larger *extent length* field, added padding in each descriptor to
> +  ease alignment, and the addition of a *descriptor count* field that
> +  can be used for easier client processing.  This chunk type MUST NOT
> +  be used unless extended headers were negotiated with
> +  `NBD_OPT_EXTENDED_HEADERS`.
> +
> +  If the *descriptor count* field contains 0, the number of subsequent
> +  descriptors is determined solely by the *length* field of the reply
> +  header.  However, the server MAY populate the *descriptor count*
> +  field with the number of descriptors that follow; when doing this,
> +  the server MUST ensure that the header *length* is equal to
> +  *descriptor count* * 16 + 8.
> +
> +  The payload starts with:
> +
> +  32 bits, metadata context ID  
> +  32 bits, descriptor count  
> +
> +  and is followed by a list of one or more descriptors, each with this
> +  layout:
> +
> +  64 bits, length of the extent to which the status below
> +     applies (unsigned, MUST be nonzero)  
> +  32 bits, padding (MUST be zero)  
> +  32 bits, status flags  
> +
> +  Note that even when extended headers are in use, the client MUST be
> +  prepared for the server to use either the compact or extended chunk
> +  type, regardless of whether the client's hinted effect length was
> +  more or less than 32 bits; but the server MUST use exactly one of
> +  the two chunk types per negotiated metacontext ID.

Is this last paragraph really a good idea? I would think it makes more
sense to require the new format if we're already required to support it
on both sides anyway.

[...]
> -    The list of block status descriptors within the
> -    `NBD_REPLY_TYPE_BLOCK_STATUS` chunk represent consecutive portions
> -    of the file starting from specified *offset*.  If the client used

I know this was in the old text (hence me mentioning it here), but this
should probably say "export" rarher than "file". NBD does not deal
(conceptually) with files...

> -    the `NBD_CMD_FLAG_REQ_ONE` flag, each chunk contains exactly one
> -    descriptor where the *length* of the descriptor MUST NOT be
> -    greater than the *length* of the request; otherwise, a chunk MAY
> -    contain multiple descriptors, and the final descriptor MAY extend
> -    beyond the original requested size if the server can determine a
> -    larger length without additional effort.  On the other hand, the
> -    server MAY return less data than requested.  In particular, a
> -    server SHOULD NOT send more than 2^20 status descriptors in a
> -    single chunk.  However the server MUST return at least one status
> -    descriptor, and since each status descriptor has a non-zero
> -    length, a client can always make progress on a successful return.
> +    The list of block status descriptors within a given status chunk
> +    represent consecutive portions of the file starting from specified
> +    *offset*.  If the client used the `NBD_CMD_FLAG_REQ_ONE` flag,
> +    each chunk contains exactly one descriptor where the *length* of
> +    the descriptor MUST NOT be greater than the *length* of the
> +    request; otherwise, a chunk MAY contain multiple descriptors, and
> +    the final descriptor MAY extend beyond the original requested size
> +    if the server can determine a larger length without additional
> +    effort.  On the other hand, the server MAY return less data than
> +    requested.  In particular, a server SHOULD NOT send more than 2^20
> +    status descriptors in a single chunk.  However the server MUST
> +    return at least one status descriptor, and since each status
> +    descriptor has a non-zero length, a client can always make
> +    progress on a successful return.

Other than that, no comments on this one.

-- 
     w@uter.{be,co.za}
wouter@{grep.be,fosdem.org,debian.org}

I will have a Tin-Actinium-Potassium mixture, thanks.