On Mon, Apr 08, 2024 at 11:46:39AM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 05.04.24 20:44, Eric Blake wrote: > > From: Zhu Yangyang <zhuyangyan...@huawei.com> > > > > Coroutines are not supposed to block. Instead, they should yield. > > > > The client performs TLS upgrade outside of an AIOContext, during > > synchronous handshake; this still requires g_main_loop. But the > > server responds to TLS upgrade inside a coroutine, so a nested > > g_main_loop is wrong. Since the two callbacks no longer share more > > than the setting of data.complete and data.error, it's just as easy to > > use static helpers instead of trying to share a common code path. > > > > Fixes: f95910f ("nbd: implement TLS support in the protocol negotiation") > > Signed-off-by: Zhu Yangyang <zhuyangyan...@huawei.com> > > [eblake: move callbacks to their use point] > > Signed-off-by: Eric Blake <ebl...@redhat.com> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru>
I'm debating whether it is worth trying to shove this into 9.0; -rc3 is very late, and the problem is pre-existing, so I'm leaning towards no. At which point, it's better to get this right. > > still, some notes below > > > --- > > > > v3: https://lists.gnu.org/archive/html/qemu-devel/2024-04/msg00375.html > > > > in v4, factor even the struct to the .c files, avoiding a union [Vladimir] > > > > nbd/nbd-internal.h | 10 ---------- > > nbd/client.c | 27 +++++++++++++++++++++++---- > > nbd/common.c | 11 ----------- > > nbd/server.c | 29 +++++++++++++++++++++++------ > > 4 files changed, 46 insertions(+), 31 deletions(-) > > > > +++ b/nbd/client.c > > @@ -596,13 +596,31 @@ static int nbd_request_simple_option(QIOChannel *ioc, > > int opt, bool strict, > > return 1; > > } > > > > +/* Callback to learn when QIO TLS upgrade is complete */ > > +struct NBDTLSClientHandshakeData { > > + bool complete; > > + Error *error; > > + GMainLoop *loop; > > +}; > > + > > +static void nbd_client_tls_handshake(QIOTask *task, void *opaque) > > +{ > > + struct NBDTLSClientHandshakeData *data = opaque; > > + > > + qio_task_propagate_error(task, &data->error); > > + data->complete = true; > > + if (data->loop) { > > + g_main_loop_quit(data->loop); > > + } > > +} > > + > > static QIOChannel *nbd_receive_starttls(QIOChannel *ioc, > > QCryptoTLSCreds *tlscreds, > > const char *hostname, Error > > **errp) > > { > > int ret; > > QIOChannelTLS *tioc; > > - struct NBDTLSHandshakeData data = { 0 }; > > + struct NBDTLSClientHandshakeData data = { 0 }; > > > > ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, true, errp); > > if (ret <= 0) { > > @@ -619,18 +637,19 @@ static QIOChannel *nbd_receive_starttls(QIOChannel > > *ioc, > > return NULL; > > } > > qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-client-tls"); > > - data.loop = g_main_loop_new(g_main_context_default(), FALSE); > > trace_nbd_receive_starttls_tls_handshake(); > > qio_channel_tls_handshake(tioc, > > - nbd_tls_handshake, > > + nbd_client_tls_handshake, > > &data, > > NULL, > > NULL); > > > > if (!data.complete) { > > + data.loop = g_main_loop_new(g_main_context_default(), FALSE); > > g_main_loop_run(data.loop); > > + g_main_loop_unref(data.loop); > > probably good to assert(data.complete); Seems reasonable. > > +++ b/nbd/server.c > > @@ -748,6 +748,23 @@ static int nbd_negotiate_handle_info(NBDClient > > *client, Error **errp) > > return rc; > > } > > > > +/* Callback to learn when QIO TLS upgrade is complete */ > > +struct NBDTLSServerHandshakeData { > > + bool complete; > > + Error *error; > > + Coroutine *co; > > +}; > > + > > +static void nbd_server_tls_handshake(QIOTask *task, void *opaque) > > +{ > > + struct NBDTLSServerHandshakeData *data = opaque; > > + > > + qio_task_propagate_error(task, &data->error); > > + data->complete = true; > > + if (!qemu_coroutine_entered(data->co)) { > > + aio_co_wake(data->co); > > + } > > +} > > > > /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the > > * new channel for all further (now-encrypted) communication. */ > > @@ -756,7 +773,7 @@ static QIOChannel > > *nbd_negotiate_handle_starttls(NBDClient *client, > > { > > QIOChannel *ioc; > > QIOChannelTLS *tioc; > > - struct NBDTLSHandshakeData data = { 0 }; > > + struct NBDTLSServerHandshakeData data = { 0 }; > > > > assert(client->opt == NBD_OPT_STARTTLS); > > > > @@ -777,17 +794,17 @@ static QIOChannel > > *nbd_negotiate_handle_starttls(NBDClient *client, > > preexisting: lack coroutine_fn, as well as caller nbd_negotiate_options() Indeed, so now would not hurt to add them now that a callback is no longer shared between callers in different context. But probably as a separate patch. > > > > > qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-server-tls"); > > trace_nbd_negotiate_handle_starttls_handshake(); > > - data.loop = g_main_loop_new(g_main_context_default(), FALSE); > > + data.co = qemu_coroutine_self(); > > qio_channel_tls_handshake(tioc, > > - nbd_tls_handshake, > > + nbd_server_tls_handshake, > > &data, > > NULL, > > NULL); > > > > - if (!data.complete) { > > - g_main_loop_run(data.loop); > > + while (!data.complete) { > > not "if", but "while".. Do we expect entering from somewhere except > nbd_server_tls_handshake? > > if not, probably safer construction would be > > if (!data.complete) { > qemu_coroutine_yield(); > assert(data.complete); > } > > - to avoid hiding unexpected coroutine switching bugs TLS handshake is early enough in the NBD connection that there should not be any parallel I/O yet (we can't switch to parallel outstanding transactions until after successful NBD_OPT_GO), so I like your idea of asserting that the coroutine is entered at most once. v5 coming up -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org