On 04/14/2016 12:52 PM, Alex Bligh wrote:
> Improve the documentation as per the mailing list discussion.
> Here's what we decided (broadly).
>
> * 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).
s/is/is not/ ?
>
> * 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 connection, then other
s/he/the/
> 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
Which is now in, so you may want to edit that out. Sometimes I use 'git
notes' or insert a '---' divider in my commit messages for things I want
to remember during list review, but which aren't relevant to the final
state of the commit (but that works best when the patch is applied
through 'git am'; if you have direct push rights, you have to remember
to do manual editing or do 'git am' on your own patch).
>
> Signed-off-by: Alex Bligh <alex@...872...>
> ---
> doc/proto.md | 184 +++++++++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 141 insertions(+), 43 deletions(-)
>
> +
> +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 disconnect.
> +
> +A client MAY use a soft disconnect to terminate the session
> +whenever it wishes.
> +
> +A party that is mandated by this document to terminate the
> +session MUST initiate a hard disconnect if it is not possible
> +to use a soft disconnect. Such circumstances include: where
> +that party is the server and it cannot return an error
> +(e.g. after an `NBD_OPT_EXPORT_NAME` it cannot satisfy),
> +and where that party is the client following a failed TLS
> +negotiation.
> +
> +A party MUST NOT initiate a hard disconnect save where set out
> +in this section. Therefore, unless a client's situation falls
> +within the provisions of the previous paragraph, it MUST NOT
> +use a hard disconnect, and hence its only option to terminate
> +the session is via a soft disconnect.
Inconsistency between "set out in this section" (which includes hard
disconnect permitted by a violation of a MUST) and "falls within the
provisions of the previous paragraph" (which does not). I'm not sure
how best to word-smith it, maybe "falls within other provisions of this
section" ?
> +
> There is no requirement for the client or server to complete a
> negotiation if it does not wish to do so. Either end MAY simply
> -close the TCP connection (though see below regarding prior use
> -of NBD_OPT_ABORT). Under certain circumstances either
> -the client or the server may be required by this document to close
> -the TCP connection. In each case, this is referred to as 'terminate
> -the session'.
> -
> -If the client wishes to terminate the session in the negotiation
> -phase, and is not doing so because it is required to do so
> -by this document, it SHOULD send NBD_OPT_ABORT first if the protocol
> -permits. There are instances where this is impossible, such as after
> -an NBD_OPT_EXPORTNAME has been issued, or on an unsuccessful
> -negotiation of TLS. For instance, if the client does not find an
> -export it is looking for, it MAY simply send an NBD_OPT_ABORT
> -and close the TCP connection.
> +terminate the session. In the client's case, if it wishes to
> +do so it MUST use soft disconnect.
And of course, if the client violates this, the server MAY use hard
disconnect - except the disconnection has already happened ;) Should we
use SHOULD here? Using MUST means I'll have to patch qemu (but that's
probably a good thing).
> +
> +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, except that if a server believes a client's behaviour
> +constitutes a denial of service, it MAY initiate a hard disconnect.
> +If the server is in the process of being shut down it MAY
> +error inflight options and SHOULD error further options received
> +(other than an `NBD_OPT_ABORT`) with `NBD_REP_ERR_SHUTDOWN`.
> +
> +If the client receives `NBD_REP_ERR_SHUTDOWN` it MUST initiate
> +a soft disconnect.
And an older client that doesn't understand this particular error, but
keeps on talking anyway, forms a protocol violation where the server can
now within its rights do a hard termination :)
>
> ### Transmission
>
> @@ -225,8 +271,9 @@ the simple reply, and the experimental structured reply chunk. The
> transmission phase consists of a series of transactions, where the
> client submits requests and the server sends corresponding replies
> with either a single simple reply or a series of one or more
> -structured reply chunks per request. The phase continues until either
> -side closes the connection.
> +structured reply chunks per request. The phase continues until
> +either side terminates transmission; this can be performed cleanly
> +only by the client.
>
> Note that without client negotiation, the server MUST use only simple
> replies, and that it is impossible to tell by reading the server
> @@ -309,6 +356,41 @@ S: (*length* bytes of data if the request is of type `NBD_CMD_READ`)
> This reply type MUST NOT be used except as documented by the
> experimental `STRUCTURED_REPLY` extension; see below.
>
> +#### 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 referred 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.
> +A server MAY speed this process up by issuing error replies.
> +The error value issued in respect of these requests and
> +any subsequently received requests SHOULD be `ESHUTDOWN`.
> +
> +If the client receives an `ESHUTDOWN` error it MUST initiate
> +a soft disconnect.
> +
> +The client MAY issue a soft disconnect at any time, but
> +SHOULD wait until there are no inflight requests first.
> +
> +The client and the server MUST NOT initiate any form
> +of disconnect other than in one of the above circumstances.
> +
Looks reasonable on the transmission side.
> ## TLS support
>
> The NBD protocol supports Transport Layer Security (TLS) (see
> @@ -380,8 +462,9 @@ These modes of operations are described in detail below.
>
> If the server receives `NBD_OPT_STARTTLS` it MUST respond with
> `NBD_REP_ERR_POLICY` (if it does not support TLS for
> -policy reasons) or `NBD_REP_ERR_UNSUP` (if it does not
> -support the `NBD_OPT_STARTTLS` option at all). The server MUST NOT
> +policy reasons), `NBD_REP_ERR_UNSUP` (if it does not
> +support the `NBD_OPT_STARTTLS` option at all) or another
> +error explicitly permitted by this document. The server MUST NOT
> respond to any option request with `NBD_REP_ERR_TLS_REQD`.
>
> #### FORCEDTLS mode
> @@ -665,7 +748,7 @@ receiving the handshake flags from the server.
> set `NBD_FLAG_NO_ZEROES`. If set, the server MUST NOT send the 124
> bytes of zeroes at the end of the negotiation.
>
> -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
> connection if the client sets an unknown flag, or a flag that does
> not match something advertised by the server.
>
> @@ -701,8 +784,16 @@ of the newstyle negotiation.
>
> - `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`.
> +
> + Previous versions of this document were unclear on whether
> + the server should send a reply to `NBD_OPT_ABORT`. Therefore
> + the client SHOULD gracefully handle the server closing the
> + connection after receiving an `NBD_OPT_ABORT` without it
> + sending a reply. Similarly the server SHOULD gracefully handle
> + the client sending an `NBD_OPT_ABORT` and closing the connection
> + without waiting for a reply.
>
> - `NBD_OPT_LIST` (3)
>
> @@ -721,10 +812,8 @@ of the newstyle negotiation.
> The client wishes to initiate TLS.
>
> The server MUST either reply with `NBD_REP_ACK` after which
> - point the connection is upgraded to TLS, or reply with
> - `NBD_REP_ERR_POLICY` (or if it does not support the option
> - at all, `NBD_REP_ERR_UNSUP`, or if TLS has already been
> - negotiated, `NBD_REP_ERR_INVALID`).
> + point the connection is upgraded to TLS, or an error reply
> + explicitly permitted by this document.
>
> See the section on TLS above for further details.
>
> @@ -808,7 +897,12 @@ case that data is an error message string suitable for display to the user.
>
> * `NBD_REP_ERR_UNKNOWN` (2^31 + 6)
>
> - defined by the experimental `INFO` extension; see below.
> + Defined by the experimental `INFO` extension; see below.
Unrelated cleanup, if you want to squash this one in on an earlier
typo-fix commit ;)
> +
> +* `NBD_REP_ERR_SHUTDOWN` (2^32 + 7)
> +
> + The server is unwilling to continue negotiation as it has been
> + shut down.
s/has been/is in the process of being/ ?
> @@ -960,6 +1057,7 @@ The following error values are defined:
> * `ENOSPC` (28), No space left on device.
> * `EOVERFLOW` (75), Value too large; SHOULD NOT be sent outside of the
> experimental `STRUCTURED_REPLY` extension; see below.
> +* `ESHUTDOWN` (108), Server has been shut down.
>
> The server SHOULD return `ENOSPC` if it receives a write request
> including one or more sectors beyond the size of the device. It SHOULD
> @@ -1124,7 +1222,7 @@ command flag.
> insufficient space.
>
> If an error occurs, the server SHOULD set the appropriate error code
> - in the error field. The server MAY then close the connection.
> + in the error field. The server MAY then initiate a hard disconnect.
Wow - in light of the rest of this patch, this sentence is a huge
loophole. It states that the server can reply with ESHUTDOWN (or any
other error), then immediately choose to do a hard disconnect without
waiting for the client to react to the ESHUTDOWN with a soft disconnect.
I'm wondering if we need to tone down the wording here, or at least add
something to the effect of the server should first wait for further
client action before initiating a hard disconnect after sending an error.
Overall looking nicer than v1 (always a good sign when we make progress)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature