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

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(a)uter.{be,co.za} wouter(a){grep.be,fosdem.org,debian.org} I will have a Tin-Actinium-Potassium mixture, thanks.