Re: [Nbd] [PATCH v3 1/2] doc: Use dedicated reply types for NBD_OPT_INFO/GO

[Alex Bligh <alex@...872...>]

Eric,

> +Both options have identical formats for requests and replies. The only
> +difference is that after a successful reply to `NBD_OPT_GO` (i.e. zero
> +or more `NBD_REP_INFO` then an `NBD_REP_EXPORT`), transmission mode is
> +entered immediately.  Therefore these commands share common
> +documentation.

So just to check, for an *unsuccessful* option, can I send a sequence
of NBD_REP_INFO followed by (e.g.) NBD_REP_ERR_TLSREQD? I think it
would be useful if that was permitted, partly to convey information to
the client about the mount in error conditions (of course it need
not send any if it doesn't want to) but mostly because it would let
the server incrementally find things out and send options, only 
errorring when it found the error, as opposed to buffering everything
up and only releasing the buffer when it discovers there are no
errors at all.

If I'm right about this, it might be worth calling out explicitly.

> +* `NBD_REP_INFO`
> +
> +    A detailed description about an aspect of an export.  The response
> +    to `NBD_OPT_INFO` and `NBD_OPT_GO` may include zero or more of
> +    these messages prior to the final `NBD_REP_EXPORT`

If I'm right above 'the final `NBD_REP_EXPORT` or error'.

> , although
> +    within the sequence of replies for a single export, the server
> +    MUST NOT send a given info type more than once.

Perhaps 'unless that info type explicitly permits it'. I can see
cases where an info type might want to list things. Given there
are no such current cases and the client will ignore things it
doesn't understand, I see no point encouraging clients to error this.

>  Clients MUST
> +    ignore info types that it does not recognize.

Para break?

>  The acceptable
> +    values for the header *length* field are determined by the info
> +    type, and includes the 2 bytes for the type designator, in the
> +    following general layout:
> +
> +    - 16 bits, info type  
> +    - *length - 2* bytes, info payload  
> +
> +    The following information types are defined:
> +
> +    * `NBD_INFO_NAME` (1)
> +
> +      Represents the server's canonical name of the export. The name
> +      MAY differ from the name presented in the client's option
> +      request,

fine

> and the info item SHOULD be omitted unless the server
> +      has multiple alternate names for a single export or a default
> +      export was specified.

interesting - why 'SHOULD be omitted'? Why not leave it to the
server? If it wants to send it because it's easier, then fair
enough I think. If (e.g.) the server has case-insensitive export
names, should it really determine whether the export name happens
to be the same in upper case and lower case before sending this?

>  The *length* MUST be at least 2, and the
> +      *info payload* is interpreted as:
> +
> +      - String: name of the export, *length - 2* bytes

In some other draft somewhere, you (I think) - else it was
me - made the good point that we should keep string format consistent.
So I'd rather this was
         - Length of string
         - String

Even though there is a duplicate length, as this theoretically
allows the elements to be expanded (weak argument as we have other
elements), is consistent with strings elsewhere (strong argument)
and makes it easier for protocol analysers (strong argument).
> 
> 
> +* `NBD_REP_EXPORT`
> +
> +    The final reply to a successful `NBD_OPT_INFO` or `NBD_OPT_GO`,
> +    after zero or more `NBD_REP_INFO`.  Describes the same information
> +    that is sent in response to the older `NBD_OPT_EXPORT_NAME`,
> +    except that there are no trailing zeroes whether or not
> +    `NBD_FLAG_C_NO_ZEROES` was negotiated.  *length* MUST be 10, and
> +    the payload is interpreted as follows:
> +
> +    - 64 bits, size of the export in bytes (unsigned)  
> +    - 16 bits, transmission flags  
> +
> ### `WRITE_ZEROES` extension

OK.

-- 
Alex Bligh