Re: [Nbd] NBD_CMD_DISC

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

On Sat, Apr 09, 2016 at 01:21:21PM +0100, Alex Bligh wrote:
> 
> On 9 Apr 2016, at 12:32, Wouter Verhelst <w@...112...> wrote:
> 
> > On Sat, Apr 09, 2016 at 12:14:02PM +0100, Alex Bligh wrote:
> >> If you're arguing the reply doesn't add anything, in a sense you're
> >> right, but only in the same way NBD_CMD_DISC itself doesn't add anything,
> >> as you *could* just permit straight disconnections without a command
> >> at all - they would be just as safe/unsafe as with NBD_CMD_DISC
> >> as with NBD_CMD_DISC as currently documented a legally operating
> >> client can actually disconnect intending to send NBD_CMD_DISC but
> >> with it not actually reach the server.
> > 
> > My point is, it's fairly easy to clarify that that is not legal. If you
> > *intend* to send NBD_CMD_DISC, you should ensure that you've actually done so
> > before dropping the connection!
> 
> I'm not sure how you actually do that with existing TLS libraries. I'm
> not sure we should make writing clients harder.

That would be silly. If we specify that the protocol has to end with a
message from the client, then the client MUST ensure that that message
actually reaches the server. If we say that's not possible, we could
drop that final message and make the server send the final message, at
which point then the server has to end the protocol with a message,
after which it MUST ensure that that message actually reaches the
client.

No matter how you define things to be, there has to be *some* peer which
must be able to ensure that its final message (before it closes the
connection) reaches the other side.

Therefore, logically, TLS libraries have to be able to do so. If they're
unable to, they're broken and completely useless.

> >>> Clients can send FLUSH if they want to be sure.
> >> 
> >> Well they can, but we could make life easier for them,
> > 
> > I don't see how "add another command with almost-but-not-quite the same
> > semantics" makes life easier for anyone...
> 
> Well, the idea was to deprecate NBD_CMD_DISC. But as I have now
> discovered we can probably do that without a new command.
> 
> >>> The server may *also* want to know when a client wants to disconnect,
> >>> for whatever reason.
> >> 
> >> Sure, it will know when the TCP stream is terminated.
> > 
> > So will the client?
> 
> Not sure I understand. The client knows it wants to disconnect
> because it knows it wants to disconnect - that's the assumption.

I meant, the client will also know that the connection drops when the
server wants to know.

> After the client has disconnected (currently) it typically
> exits. The problem is this can happen prior to the TLS
> side completing.
> 
> If you mean "will the client also want to know if the server
> disconnects", yes it will want to know that. If it receives
> a disconnect other than in response to NBD_CMD_DISC then
> it knows something bad has happened. What happens after
> NBD_CMD_DISC has been sent is more complicated:
> 
> * If there are inflight requests (situation not currently
>   prohibited)

It obviously should be. I think it is implied, but it doesn't hurt to
make that explicit.

>   it does not know whether these are served
>   successfully or even attempted.
> 
> * It does not know whether the NBD_CMD_DISC was actually
>   received or if a disconnect happened to occur at the same
>   time.

At that point, the client has little interest in such things anymore.

> * If no TCP disconnect occurs, it does not know how long to
>   wait for one.

It doesn't have to. Once all outstanding inflight requests have
finished, it can drop its side if it wants to.

Everything else can be handled by the standard TCP timeouts.

> So the client waiting for the server to disconnect is also
> not a great option.
> 
> >> That is the only notification it currently gets *now* in some
> >> circumstances, as the NBD_CMD_DISC gets lost (occasionally) as the
> >> client has exited before its TLS buffer gets flushed.
> > 
> > If the command gets lost, clearly the client didn't do its job
> > correctly...
> 
> If we make it hard for the client to do its job, we should not
> be surprised if that happens.

I don't think we are doing that.

[...]
> >> I think there are two possible courses of action.
> >> 
> >> 1. Introduce a disconnect with a reply (either NBD_CMD_CLOSE
> >>   or some sort of flag to indicate NBD_CMD_DISC will send
> >>   a reply so the client can wait for it and close the channel
> >>   itself).
> >> 
> >> 2. Make it legal for the client to close the communication when
> >>   there is nothing in flight, and after a flush (if the
> >>   server supports it). Make this the preferred method for
> >>   disconnection.
> > 
> > 3. Clarify that NBD_CMD_DISC is an *intent* to close the connection, but
> > that whatever is outstanding should still be handled before doing so. If
> > a client sends FLUSH and DISC in rapid succession, the server should
> > handle the FLUSH before handling the disconnect.
> > 
> > That doesn't require any semantic changes (the client needs to wait for
> > the FLUSH response anyway), and gets you the certainty you're asking
> > for.
> 
> You mean that a server should wait for all outstanding requests
> to complete before processing NBD_CMD_DISC? (and presumably
> clients should send no more requests after sending NBD_CMD_DISC).

Yes.

> That's fine, but it doesn't fix the TLS buffer issue; that's
> orthogonal issue (one which you would presumably address by
> saying 'fix the client').

Indeed.

Obviously, a client which uses write buffers should ensure that its
buffers are flushed before it drops the connection. This isn't related
to the NBD protocol, it's related to "basic programming skills".

> >> On reflection, I think (2) is far easier. It has the following
> >> advantages:
> >> 
> >> * Server implementation is trivial as a client disconnect
> >>  needs to be monitored anyway. If the server is multithreaded
> >>  it can produce a warning if there are inflight commands, else
> >>  not.
> >> 
> >> * Client implementation is trivial too - simply don't bother
> >>  sending the NBD_CMD_DISC. It should send a FLUSH first (which
> >>  it should be doing anyway probably), and it should wait for
> >>  all inflight commands to complete (which it presumably should
> >>  be doing anyway).
> > 
> > Downside: servers now need to distinguish between "connection drop with
> > outstanding requests" and "connection drop with *no* outstanding
> > requests", and perhaps do different clean up in the latter case but not
> > the former, because the former is an unclean disconnect and the latter
> > isn't.
> 
> In practice it currently needs to do that anyway. Firstly because
> in order to process the NBD_CMD_DISC (per your suggestion above) it
> needs to wait until all inflight requests are processed, so it must
> already have something that determines whether or not there are
> inflight requests. Secondly because as demonstrated there are
> already clients out in the wild which disconnect when there are no
> outstanding requests without reliably sending a NBD_CMD_DISC message.

They're buggy. And qemu-2.6 (with TLS support) hadn't been released yet
when last I checked.

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