| Message ID | 1459888946-27233-1-git-send-email-alex@alex.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 05, 2016 at 09:42:26PM +0100, Alex Bligh wrote: > Amend the NBD_OPT_SELECT and NBD_OPT_GO documentation as > follows: > > * 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. > > * Allow a name to be specified on NBD_OPT_GO > > * Make clear the rules for default device selection > > * Remove the provision concerning TLS resetting device selection > > * Remove NBD_REP_ERR_INVALID as a reply to NBD_OPT_GO as there > is now no necessity for a prior NBD_OPT_INFO > > * Make it clear NBD_OPT_GO is in effect a better alternative > for NBD_OPT_EXPORT_NAME > > * Make it clear the NBD_OPT_INFO and NBD_OPT_GO are in > essence the same command, save that NBD_OPT_GO puts you > into transmission mode if successful. > > * Clarify permitted option returns outside TLS to prevent > export enumeration. > > * Remove 'length' 32 bit quantity from > NBD_OPT_SELECT (and don't copy it into NBD_OPT_GO) so it > looks exactly like NBD_OPT_EXPORT_NAME bar the reply. > This length is unnecessary as it's in the option header > anyway. > > * 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. > > Signed-off-by: Alex Bligh <alex@alex.org.uk> [...] applied, thanks.
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 <alex@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/
On 04/05/2016 03:03 PM, Wouter Verhelst wrote: > On Tue, Apr 05, 2016 at 09:42:26PM +0100, 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 <alex@alex.org.uk> > [...] > > applied, thanks. Ah, you were faster than my reply mail; looks like everything I pointed out now gets to be a followup patch :) In particular, I had a question about the ideal layout of the data in NBD_REP_SERVER; I'd like some discussion on that point before posting a cleanup patch.
Eric (sic - sorry - long day). On 5 Apr 2016, at 22:26, Eric Blake <eblake@redhat.com> wrote: > Ah, you were faster than my reply mail; looks like everything I pointed > out now gets to be a followup patch :) I agree with all but one of the the changes you put in. As Wouter is the fastest gun in the west ( :-) ), do you want to do a patch? I'm also conscious that NBD_REP_SERVER is now used for 2 different reply formats (actually it always was). Perhaps we should change this one to NBD_REP_SERVER_EXT (or similar). My disagreement is this bit: > 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. Not strongly felt, but having sat looking at Wireshark for a while, I prefer things are naturally aligned, but: > 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). Please no! (or at least not like this) I think in general we should desperately try and delete the arbitrary obsession with 124 bytes. And (again, very long day in which I managed to mis-spell your name) I really don't see what this is buying us. What does the 'variable, based on other negotiated options' contain, and how does the client manage to find the name amongst it? If the proposal is that options can somehow contribute to the 'final negotiation', that's fine in principle, but let's keep it out of this proposal, and perhaps let's have it as <optionid, length, data> tuples so a protocol analyzer can parse it. If you want to put in some indicator these might be present then fine, but I'm guesing transmission flags could take that as a bit. But if we introduce such tuples, there's no need to limit them to 124 bytes. -- Alex Bligh
On 5 Apr 2016, at 22:26, Eric Blake <eblake@redhat.com> wrote: > In particular, I had a question about the ideal layout of the data in > NBD_REP_SERVER; I'd like some discussion on that point before posting a > cleanup patch. Hmmm... Yes. In particular: > NBD_REP_SERVER (2) > > A description of an export. Data: > > • 32 bits, length of name (unsigned); MUST be no larger than the reply packet header length - 4 > • Name of the export, as expected by NBD_OPT_EXPORT_NAME (note that the length of name does NOT include a NUL terminator) > • If length of name < (reply packet header length - 4), then the rest of the data contains some implementation-specific details about the export. This is not currently implemented, but future versions of nbd-server may send along some details about the export. Therefore, unless explicitly documented otherwise by a particular client request, this field is defined to be UTF-8 encoded data suitable for direct display to a human being; with no embedded or terminating NUL characters. > The experimental INFO extension (see below) is a client request where the extra data has a definition other than a UTF-8 message. I'm not sure reordering those fields was a great idea :-/ -- Alex Bligh
diff --git a/doc/proto.md b/doc/proto.md index 35a3266..d5ea465 100644 --- a/doc/proto.md +++ b/doc/proto.md @@ -344,7 +344,7 @@ of the newstyle negotiation. A major problem of this option is that it does not support the return of error messages to the client in case of problems. To - remedy this, the experimental `SELECT` extension has been + remedy this, the experimental `INFO` extension has been introduced; see below. - `NBD_OPT_ABORT` (2) @@ -377,13 +377,13 @@ of the newstyle negotiation. implementation, but was implemented by qemu so has been moved out of the "experimental" section. -- `NBD_OPT_SELECT` (6) +- `NBD_OPT_INFO` (6) - Defined by the experimental `SELECT` extension; see below. + Defined by the experimental `INFO` extension; see below. - `NBD_OPT_GO` (7) - Defined by the experimental `SELECT` extension; see below. + Defined by the experimental `INFO` extension; see below. - `NBD_OPT_STRUCTURED_REPLY` (8) @@ -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. @@ -465,12 +465,12 @@ display to the user, with no embedded or terminating NUL characters. implementation, but was implemented by qemu so has been moved out of the "experimental" section. - The experimental `SELECT` extension makes small but compatible + The experimental `INFO` extension makes small but compatible changes to the semantics of this error message; see below. * `NBD_REP_ERR_UNKNOWN` (2^31 + 6) - defined by the experimental `SELECT` extension; see below. + defined by the experimental `INFO` extension; see below. ### Transmission phase @@ -648,27 +648,42 @@ Therefore, implementors are strongly suggested to contact the fine-tune the specifications in this section before committing to a particular implementation. -### `SELECT` extension +### `INFO` extension A major downside of the `NBD_OPT_EXPORT_NAME` option is that it does not allow for an error message to be returned by the server (or, in fact, any structured message). This is a result of a (misguided) attempt to keep backwards compatibility with non-fixed newstyle negotiation. -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 + given name for use in the transmission phase, but does not yet want + to move to the transmission phase. - - 32 bits, length of name (unsigned) - - Name of the export + `NBD_OPT_GO`: The client wishes to terminate the negotiation phase and + 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. + + Data (both commands): + + - Name of the export (as with `NBD_OPT_EXPORT_NAME`, the length + comes from the option header). + + If no name is specified (i.e. a zero length string is provided), + this specifies the default export (if any), as with + `NBD_OPT_EXPORT_NAME`. The server replies with one of the following: @@ -676,57 +691,52 @@ option reply type. server. - `NBD_REP_ERR_TLS_REQD`: The server does not wish to export this block device unless the client negotiates TLS first. - - `NBD_REP_SERVER`: The server accepts the chosen export. In this - case, the `NBD_REP_SERVER` 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); this layout is shared with the successful response to - `NBD_OPT_GO`. The option reply length MUST be *length of - name* + 14, and the option data has the following layout: - - - 32 bits, length of name (unsigned) - - Name of the export. This name MAY be different from the one - given in the `NBD_OPT_SELECT` option in case the server has - multiple alternate names for a single export. - - 64 bits, size of the export in bytes (unsigned) - - 16 bits, transmission flags + - `NBD_REP_SERVER`: The server accepts the chosen export. - - 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`. - -* `NBD_OPT_GO` - - The client wishes to terminate the negotiation phase and progress to - the transmission phase. Possible replies from the server include: - - - `NBD_REP_SERVER`: The server acknowledges, using the same format - for the reply as in `NBD_OPT_SELECT` (thus allowing the client - to receive the same transmission flags as would have been sent - by `NBD_OPT_EXPORT_NAME`). The values of the transmission flags - MAY differ from what was sent earlier in response to - `NBD_OPT_SELECT`, based on other options that were negotiated in - the meantime. 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 receiving - this reply, the client and the server have both moved to the - transmission phase; therefore, the server MUST NOT send this - particular reply until all other pending option requests have - had their final reply. - - `NBD_REP_ERR_INVALID`: No `NBD_OPT_SELECT` command was - previously issued during this negotiation (there is no default); - or, the most recent `NBD_OPT_SELECT` command resulted in an error - reply (selecting a different export clears the currently selected - export, even if the new export does not exist on the server); or, - the most recent `NBD_OPT_SELECT` command issued during this - negotiation occurred before TLS was successfully negotiated - (negotiating TLS clears the selected export). - - Servers MAY also choose to send `NBD_REP_ERR_TLS_REQD` rather than - `NBD_REP_ERR_INVALID` if no non-TLS exports are supported. - - 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) + - 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 + 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. 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 + 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 + 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. ### `WRITE_ZEROES` extension
Amend the NBD_OPT_SELECT and NBD_OPT_GO documentation as follows: * 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. * Allow a name to be specified on NBD_OPT_GO * Make clear the rules for default device selection * Remove the provision concerning TLS resetting device selection * Remove NBD_REP_ERR_INVALID as a reply to NBD_OPT_GO as there is now no necessity for a prior NBD_OPT_INFO * Make it clear NBD_OPT_GO is in effect a better alternative for NBD_OPT_EXPORT_NAME * Make it clear the NBD_OPT_INFO and NBD_OPT_GO are in essence the same command, save that NBD_OPT_GO puts you into transmission mode if successful. * Clarify permitted option returns outside TLS to prevent export enumeration. * Remove 'length' 32 bit quantity from NBD_OPT_SELECT (and don't copy it into NBD_OPT_GO) so it looks exactly like NBD_OPT_EXPORT_NAME bar the reply. This length is unnecessary as it's in the option header anyway. * 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. Signed-off-by: Alex Bligh <alex@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