Re: [Nbd] [PATCH v4 1/2] doc: Use sequence of replies 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. an `NBD_REP_SERVER`), transmission mode is entered immediately.
> -Therefore these commands share common documentation.
> +Both options have identical formats for requests and replies. The only
> +difference is that after a successful reply to `NBD_OPT_GO` (i.e. one
> +or more

this is implying that at least one INFO element is compulsory, but ...

> `NBD_REP_INFO` then an `NBD_REP_ACK`), transmission mode is
> +entered immediately.  Therefore these commands share common
> +documentation.
> 
> * `NBD_OPT_INFO` and `NBD_OPT_GO`
> 
> @@ -1032,58 +1034,109 @@ Therefore these commands share common documentation.
>     this specifies the default export (if any), as with
>     `NBD_OPT_EXPORT_NAME`.
> 
> -    The server replies with one of the following:
> +    The server replies with zero or more `NBD_REP_INFO` replies, as

... this says zero or more are permitted with NBD_REP_ACK, though it
constrains it later.

Perhaps 'zero or more NBD_REP_INFO followed by NBD_REP_ERR or
one or more NBD_REP_INFO followed by NBD_REP_ACK' or something.

> +    further constrained by the final reply, then concludes the list
> +    of information with `NBD_REP_ACK` for success, or with an
> +    appropriate error reply, as follows:
> 
> +    - `NBD_REP_ACK`: The server accepts the chosen export, and has
> +      completed providing information.  In this case, the server MUST
> +      send at least one `NBD_REP_INFO`, covering the `NBD_INFO_EXPORT`
> +      information.

s/covering.*$'/'with an `NBD_INFO_EXPORT` information type.'/ ?

>     - `NBD_REP_ERR_UNKNOWN`: The chosen export does not exist on this
> -      server.
> +      server.  In this case, the server SHOULD NOT send `NBD_REP_INFO`
> +      replies).
>     - `NBD_REP_ERR_TLS_REQD`: The server does not wish to export this
> -      block device unless the client initiates TLS first.
> -    - `NBD_REP_SERVER`: The server accepts the chosen export.
> +      block device unless the client initiates TLS first.  In this
> +      case, a FORCEDTLS server MUST NOT send `NBD_REP_INFO` replies,
> +      but a SELECTIVETLS server MAY do so if this is a TLS-only
> +      export.

Whilst this last sentence is true, I'm not sure it is necessary
because it's merely the general case under NBD_OPT_STARTTLS.
But surely it only applies if TLS has not been negotiated?

Alsp NBD_REP_ERR_TLS_REQD actually means
'I'm not telling you about this export unless you negotiate
TLS first' (i.e. the export might not exist (contrary to
'The server does not wish to export this block device'.

Is it really called a 'block device' not an export? IE is
this terminology used elsewhere? I thought the 'block device'
was what the client saw.

> +* `NBD_REP_INFO`
> +
> +    A detailed description about an aspect of an export.  The response
> +    to `NBD_OPT_INFO` and `NBD_OPT_GO` includes zero or more of these
> +    messages prior to a final error reply, or at least one before an
> +    `NBD_REP_ACK` reply indicating success.  The `NBD_INFO_EXPORT`
> +    type is mandatory prior to success,

How about 'The server MUST send and `NBD_INFO_EXPORT` at some
stage prior to sending an `NBD_REP_ACK` - to make this more RFC2119
language and also make it less contorted.

Otherwise LGTM.

-- 
Alex Bligh