| Message ID | 20240404014418.620851-2-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] nbd/server: do not poll within a coroutine context | expand |
On 04.04.24 04:42, Eric Blake wrote: > From: Zhu Yangyang <zhuyangyang14@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 <zhuyangyang14@huawei.com> > [eblake: move callbacks to their use point] > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > After looking at this more, I'm less convinced that there is enough > common code here to even be worth trying to share in common.c. This > takes the essence of the v2 patch, but refactors it a bit. Maybe, do the complete split, and make separate structure definitions in client.c and server.c, and don't make shared NBDTLSHandshakeData with union? Finally, it's just a simple opaque-structure for static callback function, seems good to keep it in .c as well. > > v2: https://lists.gnu.org/archive/html/qemu-devel/2024-04/msg00019.html > > nbd/nbd-internal.h | 20 ++++++++++---------- > nbd/client.c | 21 +++++++++++++++++---- > nbd/common.c | 11 ----------- > nbd/server.c | 21 ++++++++++++++++----- > 4 files changed, 43 insertions(+), 30 deletions(-) > > diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h > index dfa02f77ee4..087c6bfc002 100644 > --- a/nbd/nbd-internal.h > +++ b/nbd/nbd-internal.h > @@ -63,6 +63,16 @@ > #define NBD_SET_TIMEOUT _IO(0xab, 9) > #define NBD_SET_FLAGS _IO(0xab, 10) > > +/* Used in NBD_OPT_STARTTLS handling */ > +struct NBDTLSHandshakeData { > + bool complete; > + Error *error; > + union { > + GMainLoop *loop; > + Coroutine *co; > + } u; > +}; > + > /* nbd_write > * Writes @size bytes to @ioc. Returns 0 on success. > */ > @@ -72,16 +82,6 @@ static inline int nbd_write(QIOChannel *ioc, const void *buffer, size_t size, > return qio_channel_write_all(ioc, buffer, size, errp) < 0 ? -EIO : 0; > } > > -struct NBDTLSHandshakeData { > - GMainLoop *loop; > - bool complete; > - Error *error; > -}; > - > - > -void nbd_tls_handshake(QIOTask *task, > - void *opaque); > - > int nbd_drop(QIOChannel *ioc, size_t size, Error **errp); > > #endif > diff --git a/nbd/client.c b/nbd/client.c > index 29ffc609a4b..c9dc5265404 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -596,6 +596,18 @@ static int nbd_request_simple_option(QIOChannel *ioc, int opt, bool strict, > return 1; > } > > +/* Callback to learn when QIO TLS upgrade is complete */ > +static void nbd_client_tls_handshake(QIOTask *task, void *opaque) > +{ > + struct NBDTLSHandshakeData *data = opaque; > + > + qio_task_propagate_error(task, &data->error); > + data->complete = true; > + if (data->u.loop) { > + g_main_loop_quit(data->u.loop); > + } > +} > + > static QIOChannel *nbd_receive_starttls(QIOChannel *ioc, > QCryptoTLSCreds *tlscreds, > const char *hostname, Error **errp) > @@ -619,18 +631,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) { > - g_main_loop_run(data.loop); > + data.u.loop = g_main_loop_new(g_main_context_default(), FALSE); > + g_main_loop_run(data.u.loop); > + g_main_loop_unref(data.u.loop); > } > - g_main_loop_unref(data.loop); > + > if (data.error) { > error_propagate(errp, data.error); > object_unref(OBJECT(tioc)); > diff --git a/nbd/common.c b/nbd/common.c > index 3247c1d618a..589a748cfe6 100644 > --- a/nbd/common.c > +++ b/nbd/common.c > @@ -47,17 +47,6 @@ int nbd_drop(QIOChannel *ioc, size_t size, Error **errp) > } > > > -void nbd_tls_handshake(QIOTask *task, > - void *opaque) > -{ > - struct NBDTLSHandshakeData *data = opaque; > - > - qio_task_propagate_error(task, &data->error); > - data->complete = true; > - g_main_loop_quit(data->loop); > -} > - > - > const char *nbd_opt_lookup(uint32_t opt) > { > switch (opt) { > diff --git a/nbd/server.c b/nbd/server.c > index c3484cc1ebc..d16726a6326 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -748,6 +748,17 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp) > return rc; > } > > +/* Callback to learn when QIO TLS upgrade is complete */ > +static void nbd_server_tls_handshake(QIOTask *task, void *opaque) > +{ > + struct NBDTLSHandshakeData *data = opaque; > + > + qio_task_propagate_error(task, &data->error); > + data->complete = true; > + if (!qemu_coroutine_entered(data->u.co)) { > + aio_co_wake(data->u.co); > + } > +} > > /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the > * new channel for all further (now-encrypted) communication. */ > @@ -777,17 +788,17 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client, > > 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.u.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) { > + qemu_coroutine_yield(); > } > - g_main_loop_unref(data.loop); > + > if (data.error) { > object_unref(OBJECT(tioc)); > error_propagate(errp, data.error);
On Fri, Apr 05, 2024 at 05:10:16PM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 04.04.24 04:42, Eric Blake wrote: > > From: Zhu Yangyang <zhuyangyang14@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 <zhuyangyang14@huawei.com> > > [eblake: move callbacks to their use point] > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > > > > After looking at this more, I'm less convinced that there is enough > > common code here to even be worth trying to share in common.c. This > > takes the essence of the v2 patch, but refactors it a bit. > > Maybe, do the complete split, and make separate structure definitions in client.c and server.c, and don't make shared NBDTLSHandshakeData with union? Finally, it's just a simple opaque-structure for static callback function, seems good to keep it in .c as well. Sure, v4 coming up along those lines.
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h index dfa02f77ee4..087c6bfc002 100644 --- a/nbd/nbd-internal.h +++ b/nbd/nbd-internal.h @@ -63,6 +63,16 @@ #define NBD_SET_TIMEOUT _IO(0xab, 9) #define NBD_SET_FLAGS _IO(0xab, 10) +/* Used in NBD_OPT_STARTTLS handling */ +struct NBDTLSHandshakeData { + bool complete; + Error *error; + union { + GMainLoop *loop; + Coroutine *co; + } u; +}; + /* nbd_write * Writes @size bytes to @ioc. Returns 0 on success. */ @@ -72,16 +82,6 @@ static inline int nbd_write(QIOChannel *ioc, const void *buffer, size_t size, return qio_channel_write_all(ioc, buffer, size, errp) < 0 ? -EIO : 0; } -struct NBDTLSHandshakeData { - GMainLoop *loop; - bool complete; - Error *error; -}; - - -void nbd_tls_handshake(QIOTask *task, - void *opaque); - int nbd_drop(QIOChannel *ioc, size_t size, Error **errp); #endif diff --git a/nbd/client.c b/nbd/client.c index 29ffc609a4b..c9dc5265404 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -596,6 +596,18 @@ static int nbd_request_simple_option(QIOChannel *ioc, int opt, bool strict, return 1; } +/* Callback to learn when QIO TLS upgrade is complete */ +static void nbd_client_tls_handshake(QIOTask *task, void *opaque) +{ + struct NBDTLSHandshakeData *data = opaque; + + qio_task_propagate_error(task, &data->error); + data->complete = true; + if (data->u.loop) { + g_main_loop_quit(data->u.loop); + } +} + static QIOChannel *nbd_receive_starttls(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname, Error **errp) @@ -619,18 +631,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) { - g_main_loop_run(data.loop); + data.u.loop = g_main_loop_new(g_main_context_default(), FALSE); + g_main_loop_run(data.u.loop); + g_main_loop_unref(data.u.loop); } - g_main_loop_unref(data.loop); + if (data.error) { error_propagate(errp, data.error); object_unref(OBJECT(tioc)); diff --git a/nbd/common.c b/nbd/common.c index 3247c1d618a..589a748cfe6 100644 --- a/nbd/common.c +++ b/nbd/common.c @@ -47,17 +47,6 @@ int nbd_drop(QIOChannel *ioc, size_t size, Error **errp) } -void nbd_tls_handshake(QIOTask *task, - void *opaque) -{ - struct NBDTLSHandshakeData *data = opaque; - - qio_task_propagate_error(task, &data->error); - data->complete = true; - g_main_loop_quit(data->loop); -} - - const char *nbd_opt_lookup(uint32_t opt) { switch (opt) { diff --git a/nbd/server.c b/nbd/server.c index c3484cc1ebc..d16726a6326 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -748,6 +748,17 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp) return rc; } +/* Callback to learn when QIO TLS upgrade is complete */ +static void nbd_server_tls_handshake(QIOTask *task, void *opaque) +{ + struct NBDTLSHandshakeData *data = opaque; + + qio_task_propagate_error(task, &data->error); + data->complete = true; + if (!qemu_coroutine_entered(data->u.co)) { + aio_co_wake(data->u.co); + } +} /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the * new channel for all further (now-encrypted) communication. */ @@ -777,17 +788,17 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client, 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.u.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) { + qemu_coroutine_yield(); } - g_main_loop_unref(data.loop); + if (data.error) { object_unref(OBJECT(tioc)); error_propagate(errp, data.error);