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
Attachment:
signature.asc
Description: OpenPGP digital signature