Eric, >> +* `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_CLOSE`. > > Sorry for not catching this in v1, No problem > but NBD_REP_ACK is a handshake phase > message, but we're in transmission phase. Particularly in the case of > the userspace/kernel split-brained client, that would mean teaching the > kernel to look for NBD_REP_ACK. Not good. Oops, fixed. > Rather, we have to require that the server MUST send a normal > transmission phase reply with no error set (we could go into details > that the reply must be a simple reply if structured replies are not > negotiated; or that if structured replies are available the reply should > have NBD_REPLY_TYPE_NONE, but I don't think that adds anything over what > the structured reply extension already documents). No need for those details. > Furthermore, do we really require no error here... I think we do, mostly because the error would be misleading. The command should disconnect. You can't refuse to disconnect. If it's transmitting an error there might be an idea that the command should fail, but we've already said the server can't accept any more commands and the client can't send any. I don't think we should try and resuscitate the connection on a failed close. So that leaves the (confusing) possibility of a server errorring the close but the connection still closing. I suppose conceivably it could error the close meaning 'I tried to flush stuff to disk and it failed so you should know'? Is that worth it? I then read your stuff below and concluded you think it is, so I will fix it :-) >> + 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. > > Again, NBD_REP_ACK is the wrong thing here; this should be a > transmission reply with the error field set to 0. Fixed >> + >> + A disconnect in any other circumstances MUST be considered an >> + unsafe close, except for `NBD_CMD_DISC` sent or received without >> + 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). > > More bad references to NBD_REP_ACK. Fixed. >> + >> + The length and offset fields in an `NBD_CMD_CLOSE` request >> + MUST both be zero. > > ...what happens if they aren't? Can the server set EINVAL in the error > field, even though we said no errors up above? No, I said no errors! If the length is wrong then in practice you have a protocol violation and the server is probably going to sit there waiting to consume a packet. If the offset is wrong then, well the offset is wrong. It's mandatory on the server to send them as 0, it's not mandatory on the client to check them or even advised. What it can't do is check them and then deviate from the spec. > If we allow setting an > error, would we want to leave the connection open on error? See above discussion. > On the other hand, I guess the MUST is sufficient to state that a > compliant client will pass 0, and then we don't care what happens to a > non-compliant client. I'm trying to see if there's ever any way a > future extension could make use of a magic length/offset pair (perhaps > with a new flag), but even that would require that we decide whether the > server has to behave differently on getting an unrecognized flag. Well for some extensions at least, we'd want it to ignore things it didn't understand. EG the 'play a wild-eep noise on the console on successful close' extension. > I'm leaning towards: the server MAY report an error (such as EINVAL for > an unrecognized flag or nonzero offset or length), but still MUST reply > and send no further data; the client MUST disconnect whether the reply > received was successful or an error. > > Then, if we ever DO need an extension, I would state that the extension > requires structured replies, and we can amend the client to realize that > if a new NBD_REPLY_TYPE_FOO is sent, then the extension behavior is in > effect instead of the usual "traffic is done, client must close" semantics. OK OK you've persuaded me. > Also, in reading this, I reviewed NBD_OPT_ABORT. Should we tighten any > of the wording there? But a clean close after NBD_OPT_ABORT is less > important: until the transmission phase starts, there is no data that > needs to be flushed to persistent storage. So it doesn't matter if > client and/or server do an unclean disconnect after client sends > NBD_OPT_ABORT. I don't think this matters so much for the reason you describe. If it does, it should be in a separate patch. -- Alex Bligh
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail