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 <[email protected]>
> ---
> 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
------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Nbd-general mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/nbd-general
