Eric, >> @@ -608,6 +609,11 @@ The following request types exist: >> The values of the length and offset fields in a disconnect request >> are not defined. >> >> + A client SHOULD NOT send `NBD_CMD_DISC` if `NBD_FLAG_SEND_CLOSE` >> + is set in the transmission flags field, but SHOULD use >> + `NBD_CMD_CLOSE` instead; in such circumstances if it cannot wait >> + for inflight requests to complete it SHOULD simply disconnect. > > maybe s/in such circumstances if it cannot wait/in circumstances where > it cannot wait/ "in such circumstances" refers to if `NBD_FLAG_SEND_CLOSE` is set. I think better with a comma, i.e. "in such circumstances, if the client cannot wait for inflight requests to complete it SHOULD simply disconnect." > I'd also consider adding something like: > > If `NBD_FLAG_SEND_CLOSE` is not set, a client SHOULD ensure all network > traffic has been flushed to the socket after sending `NBD_CMD_DISC` > (especially important if using TLS), and wait for the server to > disconnect, to give the server a chance to close down cleanly; however, > the client MAY close the connection if the server is taking too long to > respond. I'm loathe to modify the behaviour of NBD_CMD_DISC in a patch for NBD_CMD_CLOSE, and personally I think it's broken beyond fixing in respect of safe closes. I'd rather encourage people to use NBD_CMD_DISC than invest false hope in NBD_CMD_DISC. > Also, do we know if any servers reply to NBD_CMD_DISC, or do they all > disconnect? qemu-nbd does not reply; but if someone does reply, or if we > think someone might write an implementation that does reply, we could add: > > The server MAY reply to this command with `NBD_REP_ACK`, or MAY > immediately close the connection. I know of none that do, and given MUST requirement to immediately close the connection, I'm not sure what it would add. In fact if a client is looking for a close, they might see the ACK and think it is a protocol violation. Therefore -1 on this one! > >> + >> * `NBD_CMD_FLUSH` (3) >> >> A flush request; a write barrier. The server MUST NOT send a >> @@ -638,6 +644,59 @@ The following request types exist: >> >> Defined by the experimental `WRITE_ZEROES` extension; see below. >> >> +* `NBD_CMD_CLOSE` (7) >> + >> + A close request. The server MUST handle all outstanding >> + requests (there should be none by the spec), and then MUST send an >> + `NBD_REP_ACK`. The server MUST NOT send an error reply. >> + The server MUST NOT send anything after sending an `NBD_REP_ACK` >> + in response to an `NBD_CMD_DISC`. > > s/DISC/CLOSE/ Thanks >> + >> + If the server receives `NBD_CMD_CLOSE` it MUST treat >> + the close as a safe close (i.e. one where all operations have >> + been peformed) at the point where the request is received > > s/peformed/performed/ > >> + (this relies on the client complying to the requirement of >> + no outstanding requests). >> + >> + A client MUST wait until there are no outstanding requests >> + before sending an `NBD_CMD_CLOSE`, and following this >> + point it MUST treat either the receipt of an `NBD_REP_ACK` >> + or a TCP level disconnect as a safe close. > > Maybe add: > > When `NBD_CMD_SEND_FLUSH` is set in the transmission flags, a client > SHOULD use `NBD_CMD_FLUSH` as the final command before `NBD_CMD_CLOSE` > > Or is that too specific? It seems unnecessary if we can just make it implied. I've added: > A server SHOULD treat the receipt of an `NBD_CMD_CLOSE` like > a `NBD_CMD_FLUSH` and MUST do so if `NBD_FLAG_SEND_FLUSH` > was set in the transmission fields. Under such circumstances > it MUST NOT send a reply until all write requests it received > prior to receipt of the `NBD_CMD_FLUSH` have reached permanent > storage. The slightly different semantic is due to the requirement > that the server MUST handle outstanding requests first. ok? >> + >> + A disconnect in any other circumstances MUST be considered an >> + unsafe close, except for NBD_CMD_DISC sent or received without > > missing `` around NBD_CMD_DISC thx > >> + outstanding requests, which the server or client MAY treat >> + as it wishes. >> + >> + A client MUST NOT send anything to the server after sending an >> + `NBD_CMD_CLOSE` command. On receipt of an `NBD_REP_ACK` in >> + response to a `NBD_CMD_CLOSE`, the client MUST close the >> + connection. >> + >> + The server MAY close the connection after sending `NBD_REP_ACK` >> + provided it has first ensured all network traffic has been >> + flushed to the socket (especially important if using TLS). >> + >> + The length and offset fields in an `NBD_CMD_CLOSE` request >> + MUST both be zero. >> + >> + A client MUST NOT send a `NBD_CMD_CLOSE` unless >> + `NBD_FLAG_SEND_CLOSE` was set in the transmission flags >> + field. >> + >> + The rationale for `NBD_CMD_CLOSE` is as follows. When a >> + client sends an `NBD_CMD_DISC`, it does not know whether it >> + has been processed or not, > > add: > > because the server is not required to reply, ok > >> hence if it sees a disconnection it >> + cannot easily distinguish this from an error. When a client >> + sends an `NBD_CMD_DISC`, it (logically) also wishes to close >> + the connection; clients often do so to avoid the situation >> + where a server never closes the connection. This close often >> + happens before the server has received or processed the >> + `NBD_CMD_DISC`, particularly when TLS is enabled. This >> + extension allows both client and server to be sure a connection >> + has been closed safely and to differentiate TCP level disconnects >> + from closes of other sorts. > > Makes sense overall. Thanks. -- Alex Bligh
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail