Re: [Nbd] [PATCH v5 1/2] doc: Use sequence of replies for NBD_OPT_INFO/GO

[Wouter Verhelst <w@...112...>]

On Sat, Apr 16, 2016 at 03:31:04PM -0600, Eric Blake wrote:
> Since NBD_OPT_INFO and NBD_OPT_GO are experimental, we still have
> a chance to fix them up before promoting them to stable.
> 
> Attempting to reuse NBD_OPT_SERVER as the reply to NBD_OPT_INFO and
> NBD_OPT_GO has a few problems: clients must be prepared to parse
> two different styles of the reply, based on which option request
> the reply is answering (not impossible, but awkward).  Extending
> the information to provide even more details, like block sizing, is
> likewise awkward (the only way to do it within a single reply is to
> have multiple length fields that must all be consistent; but
> pre-computing the overall header length may be difficult).  And
> requiring the server to parrot back the export name is wasteful if
> the client's name is already in canonical form.
> 
> Solve this by instead making the valid response be a series of the
> new NBD_REP_INFO reply messages, followed by a concluding NBD_REP_ACK
> for success or NBD_REP_ERR_* for an appropriate error (similar to how
> NBD_OPT_LIST has a series).  On success, the series must include
> NBD_INFO_EXPORT, so that the server provides at least as much
> information as it did for NBD_OPT_EXPORT_NAME.  But prior to the
> terminal reply message, the server can now send as many additional
> optional items of information as it wants (clients must ignore the
> ones they don't recognize); this patch starts with the canonical
> name and description of the export (which is all the more
> NBD_REP_SERVER was previously being used for).  A future patch
> will then add another piece of information, for advertising server
> block sizes.
> 
> Additionally:
> 
> - The wording about repeated replies to OPT_INFO followed by OPT_GO
> was a bit repetitive; say the same thing in fewer sentences.
> 
> - Swap paragraph ordering so that NBD_REP_INFO details aren't
> split by a side-note about NBD_REP_ERR_UNSUP.
> 
> - Make it clear that NBD_OPT_LIST can end in an error, such as
> NBD_REP_ERR_SHUTDOWN
> 
> Signed-off-by: Eric Blake <eblake@...696...>
> ---
>  doc/proto.md | 185 +++++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 129 insertions(+), 56 deletions(-)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index ac290e7..402a6be 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -798,8 +798,9 @@ of the newstyle negotiation.
> 
>  - `NBD_OPT_LIST` (3)
> 
> -    Return a number of `NBD_REP_SERVER` replies, one for each export,
> -    followed by `NBD_REP_ACK`. The server SHOULD omit entries from this
> +    Return zero or more `NBD_REP_SERVER` replies, one for each export,
> +    followed by `NBD_REP_ACK` or an error (such as
> +    `NBD_REP_ERR_SHUTDOWN`). The server SHOULD omit entries from this

I know we merged this earlier, but:

We later (during the REP_INFO description) that the server MAY reply with
TLS_REQD for exports it doesn't know about if it supports TLS (to make
enumerating exports harder), but here we make that a SHOULD.

So for consistency, either the requirement for OPT_INFO should also be made a
SHOULD, or this should be made a MAY.

I'm not sure if we should do that -- I think it's fine for a server to
optionally allow enumeration of TLS-only exports, although it shouldn't
be the default -- but at any rate we should be consistent in
requirements.

>      list if TLS has not been negotiated, the server is operating in
>      SELECTIVETLS mode, and the entry concerned is a TLS-only export.
> 
> @@ -843,11 +844,12 @@ during option haggling in the fixed newstyle negotiation.
> 
>  * `NBD_REP_SERVER` (2)
> 
> -    A description of an export. Data:
> +    A description of an export name. Data:

You don't describe the name, you describe the export that is selected by the
name.

What's the idea here?

[...]

other than that, LGTM

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12