Re: [PATCH 2/3] io: Support shutdown of TLS channel

[Eric Blake]

On 3/27/20 12:43 PM, Daniel P. Berrangé wrote:

> > > I don't think it is acceptable to do this loop here. The gnutls_bye()
> > > function triggers several I/O operations which could block. Looping
> > > like this means we busy-wait, blocking this thread for as long as I/O
> > > is blocking on the socket.
> >
> > Hmm, good point.  Should we give qio_channel_tls_shutdown a bool parameter
> > that says whether it should wait (good for use when we are being run in a
> > coroutine and can deal with the additional I/O) or just fail with -EAGAIN
> > (which the caller can ignore if it is not worried)?
>
> A bool would not be sufficient, because the caller would need to know
> which direction to wait for I/O on.
>
> Looking at the code it does a flush of outstanding data, then sends
> some bytes, and then receives some bytes. The write will probably
> work most of the time, but the receive is almost always going to
> return -EAGAIN. So I don't think that failing with EGAIN is going
> to be much of a benefit.

Here, I'm guessing you're talking about gnutls lib/record.c.  But note: 
for GNUTLS_SHUT_WR, it only does _gnutls_io_write_flush and 
gnutls_alert_send; the use of _gnutls_recv_int is reserved for 
GNUTLS_SHUT_RDWR.  When informing the other end that you are not going 
to write any more, you don't have to wait for a reply.

> > > If we must call gnutls_bye(), then it needs to be done in a way that
> > > can integrate with the main loop so it poll()'s / unblocks the current
> > > coroutine/thread.  This makes the whole thing significantly more
> > > complex to deal with, especially if the shutdown is being done in
> > > cleanup paths which ordinarily are expected to execute without
> > > blocking on I/O.  This is the big reason why i never made any attempt
> > > to use gnutls_bye().
> >
> > We _are_ using gnutls_bye(GNUTLS_SHUT_RDWR) on the close() path (which is
>
> Are you sure ?  AFAIK there is no existing usage of gnutls_bye() at all
> in QEMU.

Oh, I'm confusing that with nbdkit, which does use 
gnutls_bye(GNUTLS_SHUT_RDWR) but does not wait for a response (at least, 
not yet).

> > indeed a cleanup path where not blocking is worthwhile) even without this
> > patch, but the question is whether using gnutls_bye(GNUTLS_SHUT_WR) in the
> > normal data path, where we are already using coroutines to manage callbacks,
> > can benefit the remote endpoint, giving them a chance to see clean shutdown
> > instead of abrupt termination.
>
> I'm not convinced the clean shutdown at the TLS level does anything important
> given that we already have done a clean shutdown at the NBD level.

Okay, then for now, I'll still try and see if I can fix the 
libnbd/nbdkit hang without relying on qemu sending a clean 
gnutls_bye(GNUTLS_SHUT_WR).

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org