On 04/05/2016 02:42 PM, Alex Bligh wrote: > Amend the NBD_OPT_SELECT and NBD_OPT_GO documentation as > follows: >
> * Make the documentation much more concise. > > Signed-off-by: Alex Bligh <a...@alex.org.uk> > --- > doc/proto.md | 142 > ++++++++++++++++++++++++++++++++--------------------------- > 1 file changed, 76 insertions(+), 66 deletions(-) > > Changes since v1: > > * Change NBD_OPT_SELECT to be called NBD_OPT_INFO > > * Remove the 'selection' aspect of that command, so that > it now merely returns information. This is to avoid > the server storing state. > > * Reorder the fields of an NBD_OPT_INFO / NBD_OPT_GO reply > so the variable length elements are at the end. > > * Make the documentation much more concise. > > * All Erik Blake's minor modifications It's Eric, but you're not the first (and probably not the last) to use alternative spellings :) > @@ -417,7 +417,7 @@ during option haggling in the fixed newstyle negotiation. > encoded data suitable for direct display to a human being; with > no embedded or terminating NUL characters. > > - The experimental `SELECT` extension (see below) is a client > + The experimental `INFO` extension (see below) is a client > request where the extra data has a definition other than a > UTF-8 message. Not your fault, but as long as we're touching this, maybe reword it: The experimental `INFO` extension (see below) adds two client option requests where the extra data has a definition other than a UTF-8 message. > > -To remedy this, a `SELECT` extension is envisioned. This extension adds > +To remedy this, an `INFO` extension is envisioned. This extension adds > two option requests and one error reply type, and extends one existing > option reply type. > > -* `NBD_OPT_SELECT` > +Both options have identical formats for requests and replies. The > +only difference is that after a successful reply to `NBD_OPT_GO` > +(i.e. an `NBD_REP_SERVER`), transmission mode is entered immediately. > +Therefore these commands share common documentation. > > - The client wishes to select the export with the given name for use > - in the transmission phase, but does not yet want to move to the > - transmission phase. > +* `NBD_OPT_INFO` and `NBD_OPT_GO` > > - Data: > + `NBD_OPT_INFO`: The client wishes to get details about export with the s/export/an export/ > + given name for use in the transmission phase, but does not yet want > + to move to the transmission phase. Maybe a mention that "when successful, this option provides more details than `NBD_OPT_LIST`". > > - - 32 bits, length of name (unsigned) > - - Name of the export > + `NBD_OPT_GO`: The client wishes to terminate the negotiation phase and Not your fault, but we're inconsistent on "negotiation phase" vs. "handshake phase". I wouldn't worry about it here; we could do a global cleanup in a separate patch if it bugs someone enough. > + progress to the transmission phase. This client MAY issue this command > after > + an `NBD_OPT_INFO`, or MAY issue it without a previous `NBD_OPT_INFO`. > + `NBD_OPT_GO` can thus be used as a substitute for `NBD_OPT_EXPORT_NAME` > + that returns errors. The phrasing makes it sound like EXPORT_NAME returns errors. Better wording might be: `NBD_OPT_GO` can thus be used as an improved version of `NBD_OPT_EXPORT_NAME` that is capable of returning errors. > - - Servers MUST NOT send `NBD_REP_ERR_UNSUP` as a reply to this > - message if they do not also send it as a reply to the > - `NBD_OPT_SELECT` message. > + Additionally, if TLS has not been negotiated, the server MAY reply > + with `NBD_REP_ERR_TLS_REQD` (instead of `NBD_REP_ERR_UNKNOWN`) > + to requests for exports that are unknown. This is so that clients > + that have not negotiated TLS cannot enumerate exports. > + > + In the case of `NBD_REP_SERVER`, the message's data takes on a different > + interpretation than the default (so as to provide additional > + binary information normally sent in reply to `NBD_OPT_EXPORT_NAME`, > + in place of the default UTF-8 free-form string). The option reply length > + MUST be *length of name* + 14, and the option data has the following > layout: > + > + - 64 bits, size of the export in bytes (unsigned) > + - 16 bits, transmission flags. > + - 32 bits, length of name (unsigned) Not nicely aligned to a natural C struct, but that's nothing new (nor anything to worry about). At least the 'size' and 'transmission' fields are in the same relative positions as they are for the reply to NBD_OPT_EXPORT_NAME, so that part of the client code can be reused. However: Down the road, I hope to add an extension that will also let us return minimum/preferred/maximum I/O sizing for an export; I anticipate that for NBD_OPT_EXPORT_NAME, it would be carved out of the tail of 124 reserved bytes of zeroes (and if NBD_FLAG_C_NO_ZEROES is negotiated, and if my information adds 12 bytes, then NBD_FLAG_C_NO_ZEROES changes meaning to only eliminate the remaining tail of 112 zeroes after the added information). To keep the server and client code shareable between NBD_OPT_EXPORT_NAME and NBD_REP_SERVER, while still adding the extra sizing information in the reply, we'd have to inject the extra information in between 'transmission flags' and 'length of name'. If new information is always stuck at the end of the option reply, then the relative offset between 'transmission flags' and 'sizing hints' is no longer constant. Another thing I don't like: By default, NBD_REP_SERVER puts the 'length of name' field first; I'm not sure if there's a strong reason to change that. Maybe this layout would be slightly nicer: - 32 bits, length of name - 64 bits, size of export - 16 bits, transmission flags - up to 124 bytes, variable based on other negotiated options (default of 0 bytes) - length of name bytes: name where the option reply must be at least (rather than exactly) '*length of name* + 14' bytes (and at most length of name + 138), and where the tail end *length of name* bytes form the export name (because I _do_ like your idea of putting variable-length names last), and any other intermediate bytes cover the variable information based on other negotiated options, such that those other negotiated options can also consume some of the 124 reserved zeroes at the end of NBD_EXPORT_NAME's reply. Yeah, it's a bit weird that 'length of name' is separated by a variable amount of data before 'name', but it then lets us make the 'NBD_OPT_EXPORT_NAME' reply be a strict subset of the 'NBD_REP_SERVER' layout, which makes for nicer code sharing on that front (but it's still not the same as the original NBD_REP_SERVER sticking all extra information after the variable-length name field). > + - Name of the export. This name MAY be different from the one > + given in the `NBD_OPT_INFO` or `NBD_OPT_GO` option in case the server > has Long line; want to reflow this paragraph? > + multiple alternate names for a single export, or a default > + export was specified. > + > + The server MUST NOT fail an NDB_OPT_GO sent with the same parameters > + as a previous NBD_OPT_INFO which returned successfully (i.e. with > + `NBD_REP_SERVER`) unless in the intervening time the client has > + negotiated other options. Not quite the same wording as before, but I think it still nicely ensures that a server WON'T fail with NBD_REP_ERR_UNSUP on GO after succeeding on INFO. > The server MUST return the same transmission > + flags with NDB_OPT_GO as a previous NDB_OPT_INFO unless in the > + intervening time the client has negotiated other options. > + The values of the transmission flags MAY differ from what was sent > + earlier in response to an earlier `NBD_OPT_INFO` (if any), and/or > + the server may fail the request, based on other options that were Do we want s/may/MAY/ here? > + negotiated in the meantime. > + > + For backwards compatibility, clients should be prepared to also > + handle `NBD_REP_ERR_UNSUP`. In this case, they should fall back to > + using `NBD_OPT_EXPORT_NAME`. > + > + The reply to an `NBD_OPT_GO` is identical to the reply to `NBD_OPT_INFO` > + save that if the reply indicates success (i.e. is `NBD_REP_SERVER`), > + the client and the server both immediatedly enter the transmission s/immediatedly/immediately/ > + phase. The server MUST NOT send any zero padding bytes after the > + `NBD_REP_SERVER` data, whether or not the client negotiated the > + `NBD_FLAG_C_NO_ZEROES` flag. After sending this reply the server MUST > + immediately move to the transmission phase, and after receiving this > + reply, the client MUST immediately move to the transmission phase; > + therefore, the server MUST NOT send this particular reply until all > + other pending option requests have been sent by the server. s/requests/replies/ -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
