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.