diff mbox series

[v3] nbd/server: do not poll within a coroutine context

Message ID 20240404014418.620851-2-eblake@redhat.com (mailing list archive)
State New, archived
Headers show
Series [v3] nbd/server: do not poll within a coroutine context | expand

Commit Message

Eric Blake April 4, 2024, 1:42 a.m. UTC 
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.

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

Comments

Vladimir Sementsov-Ogievskiy April 5, 2024, 2:10 p.m. UTC | #1 
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);
Eric Blake April 5, 2024, 2:59 p.m. UTC | #2 
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 mbox series

Patch

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