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.
