Re: [Qemu-devel] [PATCH] Amend NBD_OPT_SELECT and NBD_OPT_GO documentation

[Alex Bligh]

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 <address@hidden> 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

signature.asc
Description: Message signed with OpenPGP using GPGMail