[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCHv2] Amend NBD_OPT_SELECT (now NBD_OPT_INFO) and N

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCHv2] Amend NBD_OPT_SELECT (now NBD_OPT_INFO) and NBD_OPT_GO documentation
Date: Tue, 5 Apr 2016 15:25:16 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.1

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

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


> +    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.


Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]