Re: [Nbd] [PATCH] Docs: improve description of disconnection methods

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

On Tue, Apr 12, 2016 at 08:31:33PM +0100, Alex Bligh wrote:
> Improve the documentation as per the mailing list discussion.
> Here's what we deciced (broadly).
> 
> * One side MAY drop the connection if the other end violates a
>  MUST condition.
> 
> * The server MUST drop the connection in the 'no way out' situations
>  during the negotiation phase (error on NBD_OPT_EXPORT_NAME, error
>  in negotiating text).
> 
> * The server SHOULD NOT otherwise drop the connection. It can wait
>  and error the next command. Clearly there are situations where
>  this is going to happen (e.g. server shutdown).
> 
> * If the server does need to drop the connection, it SHOULD wait
>  until there are no commands in-flight in transmission mode,
>  it possible.
> 
> * If he client is going to drop the the connection, then other
>  than in the event of a protocol violation or a 'no way out'
>  situation (e.g. TLS negotiation fails), it MUST use NBD_CMD_DISC
>  or NBD_OPT_ABORT
> 
> * We should tidy up the semantics and descriptions of NBD_CMD_DISC
>  and NBD_OPT_ABORT, viz replies or not to the latter, shutting
>  down TLS properly etc.
> 
> Other changes:
> 
> * Added a reply to NBD_OPT_ABORT. No harm if the client closes
>   the connection anyway.
> 
> * Said the offset and length fields in NBD_CMD_DISC MUST be zero.
>   Not doing so is a protocol violation and would only lead to ...
>   the connection being closed, so this is a useful tidy up.
> 
> * Introduced consistent terminology for disconnection throughout.
> 
> This patch applies on top of:
>   0001-docs-proto.md-Clarify-SHOULD-MUST-MAY-etc v7
> 
> Signed-off-by: Alex Bligh <alex@...872...>
> ---
>  doc/proto.md | 143 ++++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 107 insertions(+), 36 deletions(-)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index b88e054..db6b96d 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -122,7 +122,7 @@ C: 32 bits, flags
>  This completes the initial phase of negotiation; the client and server
>  now both know they understand the first version of the newstyle
>  handshake, with no options. The client SHOULD ignore any handshake flags
> -it does not recognize, while the server MUST close the connection if
> +it does not recognize, while the server MUST close the TCP connection if
>  it does not recognize the client's flags.  What follows is a repeating
>  group of options. In non-fixed newstyle only one option can be set
>  (`NBD_OPT_EXPORT_NAME`), and it is not optional.
> @@ -150,8 +150,8 @@ S: 16 bits, transmission flags
>  S: 124 bytes, zeroes (reserved) (unless `NBD_FLAG_C_NO_ZEROES` was
>     negotiated by the client)  
>  
> -If the server is unwilling to allow the export, it SHOULD close the
> -connection.
> +If the server is unwilling to allow the export, it MUST terminate
> +the session.
>  
>  The reason that the flags field is 16 bits large and not 32 as in the
>  oldstyle negotiation is that there are now 16 bits of transmission flags,
> @@ -201,22 +201,60 @@ request before sending the next one of the same type. The server MAY
>  send replies in the order that the requests were received, but is not
>  required to.
>  
> +#### Termination of the session during option haggling
> +
> +There are three possible mechanisms to end option haggling:
> +
> +* Transmission mode can be entered (by the client sending
> +  `NBD_OPT_EXPORT_NAME` or by the server responding to an
> +  `NBD_OPT_GO` with `NBD_REP_ACK`). This is documented
> +  elsewhere.
> +
> +* The client can send (and the server can reply to) an
> +  `NBD_OPT_ABORT`. This MUST be followed by the client
> +  shutting down TLS (if it is running), and the client
> +  dropping the connection. This is referred to as
> +  'initiating a soft disconnect'; soft disconnects can
> +  only be initiated by the client.
> +
> +* The client or the server can disconnect the TCP session
> +  without activity at the NBD protocol level. If TLS is
> +  negotiated, the party intitiating the transaction SHOULD
> +  shutdown TLS first if it is running. This is referrred
> +  to as 'initiating a hard disconnect'.
> +
> +This section concerns the second and third of these, together
> +called 'terminating the session', and under which circumstances
> +they are valid.
> +
> +If either the client or the server detects a violation of a
> +mandatory condition ('MUST' etc.) by the other party, it MAY
> +initiate a hard discconect.
> +
> +A client MAY use a soft disconnect to terminate the session
> +whenever it wishes, provided that there are no outstanding
> +replies to options.

NAK. A client MAY use a soft disconnect *at any time*, but the server
MUST NOT act upon it until there are no outstanding replies, and the
client MUST NOT send any further options after sending NBD_OPT_ABORT.

(same for CMD_DISC)

[...]
> +terminate the session. In the client's case, if it wishes to
> +do so it MUST use soft disconnect. In the server's case it
> +MUST (save where set out above) simply error inbound options until
> +the client gets the hint that it is unwelcome.

It might be good to add a "NBD_REP_ERR_NOSERVICE" error, for "server
shutting down"?

[...]
> +#### Terminating the transmission phase
> +
> +There are two methods of terminating the transmission phase:
> +
> +* The client sends `NBD_CMD_DISC` whereupon the server MUST
> +  close down the TLS session (if one is running) and then
> +  close the TCP connection. This is referred to as 'initiating
> +  a soft disconnect'. Soft disconnects can only be
> +  initiated by the client.
> +
> +* The client or the server drops the TCP session (in which
> +  case it SHOULD shut down the TLS session first). This is
> +  referred to as 'initiating a hard disconnect'.
> +
> +Together these are refrred to as 'terminating transmission'.
> +
> +Either side MAY initiate a hard disconnect if it detects
> +a violation by the other party of a mandatory condition
> +within this document.
> +
> +On a server shutdown, the server SHOULD wait for inflight
> +requests to be serviced prior to initiating a hard disconnect.

I still think it's a good idea for the server to be allowed to send an
unsolicited error reply in that case, but it might be better if indeed
that would be negotiated first.

[...]
> -Clients MUST NOT set any other flags; the server MUST drop the
> +Clients MUST NOT set any other flags; the server MUST drop the TCP

initiate a hard disconnect?

[...]
>  - `NBD_OPT_ABORT` (2)
>  
> -    The client desires to abort the negotiation and close the
> -    connection.
> +    The client desires to abort the negotiation and terminate the
> +    session. The server MUST reply with `NBD_REP_ACK`.

Maybe add a note that this hasn't always been a requirement, and that
therefore older servers may not send this reply?

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