Eric, In brief I agree with all of that. I'm tempted to redo it so it's much simpler now we all seem to agree that carrying state is a bad thing. Alex On 5 Apr 2016, at 18:48, Eric Blake <eblake@...696...> wrote: > On 04/05/2016 10:38 AM, Alex Bligh wrote: >> Amend the NBD_OPT_SELECT and NBD_OPT_GO documentation as >> follows: >> >> * 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_SELECT > > I guess if the client requests "" without earlier SELECT, and the server > has no default, the reply would be NBD_REP_ERR_UNKNOWN rather than > ERR_INVALID. > >> >> * Make it clear NBD_OPT_GO is in effect a better alternative >> for NBD_OPT_EXPORT_NAME >> >> * Make it clear the NBD_OPT_SELECT 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. >> >> * Controversial: 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. > > Makes sense to me; we could drop the 'Controversial' marker, unless > anyone can come up with a reason why we'd ever want a structure with > more than just the export name as the data passed along with SELECT/GO, > where having the header length distinct from the name length would let > us pass the extra data without needing yet another NBD_OPT_ command. > > >> @@ -676,21 +682,36 @@ 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 >> + - `NBD_REP_SERVER`: The server accepts the chosen export. >> + >> + Additionally, the server MAY reply `NBD_REP_ERR_TLS_REQD` to > > s/reply/reply with/ > >> + requests for exports that are unknown if TLS has not been > > are unknown, instead of `NBD_REP_ERR_UNKNOWN`, if > >> + negotiated. This is so that clients cannot without TLS > > s/cannot without TLS/without 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); 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_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 > > Indentation. Maybe "given in the `NBD_OPT_SELECT` or `NBD_OPT_GO` option" > >> + multiple alternate names for a single export, or a default >> + export was specified. >> + - 64 bits, size of the export in bytes (unsigned) >> + - 16 bits, transmission flags. The values of the transmission flags >> + MAY differ from what was sent earlier in response to >> + an earlier `NBD_OPT_SELECT` (if any), based on other options >> + that were negotiated in the meantime. >> + >> + 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. > > This statement is given twice. It's only needed once, but maybe with > this wording and layout: > > - 16 bits, transmission flags. > > The values of the transmission flags MAY differ from what was sent in > response to an earlier `NBD_OPT_SELECT` (if any), based on... > > May also be worth a statement under NBD_OPT_SELECT that any reply other > than NBD_REP_SERVER removes any prior selection made by an earlier > NBD_OPT_SELECT (if any) (we already state that under NBD_OPT_GO, but > repeating or moving it here makes more sense, since it is the failure of > NBD_OPT_SELECT and not the action of NBD_OPT_GO that clears a > selection). Also, we may want to make sure that a failed NBD_OPT_GO > also wipes out the current selection. > >> >> - For backwards compatibility, clients should be prepared to also >> handle `NBD_REP_ERR_UNSUP`. In this case, they should fall back to >> @@ -699,34 +720,56 @@ option reply type. >> * `NBD_OPT_GO` >> >> The client wishes to terminate the negotiation phase and progress to >> - the transmission phase. Possible replies from the server include: >> + the transmission phase. This client MAY issue this command after > > s/This client/The client/ > >> + an `NBD_OPT_SELECT`, or MAY issue it without a previous >> + `NBD_OPT_SELECT`. >> > >> - - 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. >> + Data: >> + >> + - 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) >> + then the export selected with the immediately previous valid >> + `NBD_OPT_SELECT` will be selected (if any), or if no previous >> + `NBD_OPT_SELECT` valid has been issued, the default export will be >> + selected. > > Reads awkwardly. How about: > > If no name is specified (i.e. a zero length string is provided), the > operation reuses the selection from the previous `NBD_OPT_SELECT` (if > there was one and it was successful), otherwise it requests the server's > default export. > >> + >> + `NBD_OPT_GO` can thus be used as a substitute for `NBD_OPT_EXPORT_NAME` >> + that returns errors. >> + >> + The server replies with one of the following: >> + >> + - `NBD_REP_ERR_UNKNOWN`: The chosen export does not exist on this >> + 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. >> + - 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`. > > We lost the bit about servers MUST NOT fail with NBD_REP_ERR_UNSUP on > NBD_OPT_GO unless they also fail NBD_OPT_SERVER in the same manner. > Basically, we want to guarantee that servers implement both options at > the same time, even if a client can get away with using only NBD_OPT_GO. > >> + >> + Additionally, the server MAY reply `NBD_REP_ERR_TLS_REQD` to >> + requests for exports that are unknown if TLS has not been >> + negotiated. This is so that clients cannot without TLS >> + enumerate exports. > > Copy whatever wording fixes you make above to this spot as well. > >> + >> + The format of `NBD_REP_SERVER` is identical to the reply >> + for `NBD_OPT_SELECT`, except for the fact that both client >> + and server subseequently enter the transmission phase. By using this > > s/subseequently/subsequently/ > >> + reply the server acknowledges the connection, 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`); as per the note therein these >> + transmission flags MAY differ from those returned by any >> + previous `NBD_OPT_SELECT`. 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 MUST immediately move > > s/the MUST/the client MUST/ > >> + to the transmission phase; therefore, the server MUST NOT send this >> + particular reply until all other pending option requests have >> + had their final reply. >> >> ### `WRITE_ZEROES` extension >> >> > > Overall makes sense. I wonder if we can compress things further by > stating something along the lines of: > > * `NBD_OPT_GO` > > Identical in behavior to `NBD_OPT_SELECT`, except for: > > and then list just the differences (move into transmission phase on > success, transmission flags may differ, and empty string can be special > cased to reuse an earlier selection), rather than copying everything > verbatim. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > -- Alex Bligh
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail