[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [PATCH 2/3] io: Support shutdown of TLS channel
Date: Fri, 27 Mar 2020 13:46:02 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]