[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Amend NBD_OPT_SELECT and NBD_OPT_GO documentati
From: |
Alex Bligh |
Subject: |
Re: [Qemu-devel] [PATCH] Amend NBD_OPT_SELECT and NBD_OPT_GO documentation |
Date: |
Tue, 5 Apr 2016 19:58:19 +0100 |
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