[PATCH v5 for-9.0? 0/2] Fix NBD TLS poll in coroutine

[PATCH v5 for-9.0? 0/2] Fix NBD TLS poll in coroutine

[Eric Blake]

v4 was here:
https://lists.gnu.org/archive/html/qemu-devel/2024-04/msg00624.html

Since then: add some asserts [Vladimir], add second patch with more
coroutine_fn annotations [Vladimir]

Eric Blake (1):
  nbd/server: Mark negotiation functions as coroutine_fn

Zhu Yangyang (1):
  nbd/server: do not poll within a coroutine context

 nbd/nbd-internal.h |  10 ----
 nbd/client.c       |  28 ++++++++--
 nbd/common.c       |  11 ----
 nbd/server.c       | 128 ++++++++++++++++++++++++++++-----------------
 4 files changed, 105 insertions(+), 72 deletions(-)

-- 
2.44.0

[PATCH v5 1/2] nbd/server: do not poll within a coroutine context

[Eric Blake]

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.  It
is also possible to add assertions that no other code is interfering
with the eventual path to qio reaching the callback, whether or not it
required a yield or main loop.

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, add assertions]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/nbd-internal.h | 10 ----------
 nbd/client.c       | 28 ++++++++++++++++++++++++----
 nbd/common.c       | 11 -----------
 nbd/server.c       | 28 +++++++++++++++++++++++-----
 4 files changed, 47 insertions(+), 30 deletions(-)

diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index dfa02f77ee4..91895106a95 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -72,16 +72,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..c89c7504673 100644
--- a/nbd/client.c
+++ 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,20 @@ 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);
+        assert(data.complete);
+        g_main_loop_unref(data.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..98ae0e16326 100644
--- a/nbd/server.c
+++ 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,18 @@ 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.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);
+        qemu_coroutine_yield();
+        assert(data.complete);
     }
-    g_main_loop_unref(data.loop);
+
     if (data.error) {
         object_unref(OBJECT(tioc));
         error_propagate(errp, data.error);
-- 
2.44.0

Re: [PATCH v5 1/2] nbd/server: do not poll within a coroutine context

[Vladimir Sementsov-Ogievskiy]

On 08.04.24 19:00, 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.  It
> is also possible to add assertions that no other code is interfering
> with the eventual path to qio reaching the callback, whether or not it
> required a yield or main loop.
> 
> 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, add assertions]
> Signed-off-by: Eric Blake<eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

[PATCH v5 2/2] nbd/server: Mark negotiation functions as coroutine_fn

[Eric Blake]

nbd_negotiate() is already marked coroutine_fn.  And given the fix in
the previous patch to have nbd_negotiate_handle_starttls not create
and wait on a g_main_loop (as that would violate coroutine
constraints), it is worth marking the rest of the related static
functions reachable only during option negotiation as also being
coroutine_fn.

Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 102 +++++++++++++++++++++++++++++----------------------
 1 file changed, 59 insertions(+), 43 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 98ae0e16326..1857fba51c1 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -195,8 +195,9 @@ static inline void set_be_option_rep(NBDOptionReply *rep, uint32_t option,

 /* Send a reply header, including length, but no payload.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_send_rep_len(NBDClient *client, uint32_t type,
-                                      uint32_t len, Error **errp)
+static coroutine_fn int
+nbd_negotiate_send_rep_len(NBDClient *client, uint32_t type,
+                           uint32_t len, Error **errp)
 {
     NBDOptionReply rep;

@@ -211,15 +212,15 @@ static int nbd_negotiate_send_rep_len(NBDClient *client, uint32_t type,

 /* Send a reply header with default 0 length.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_send_rep(NBDClient *client, uint32_t type,
-                                  Error **errp)
+static coroutine_fn int
+nbd_negotiate_send_rep(NBDClient *client, uint32_t type, Error **errp)
 {
     return nbd_negotiate_send_rep_len(client, type, 0, errp);
 }

 /* Send an error reply.
  * Return -errno on error, 0 on success. */
-static int G_GNUC_PRINTF(4, 0)
+static coroutine_fn int G_GNUC_PRINTF(4, 0)
 nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type,
                             Error **errp, const char *fmt, va_list va)
 {
@@ -259,7 +260,7 @@ nbd_sanitize_name(const char *name)

 /* Send an error reply.
  * Return -errno on error, 0 on success. */
-static int G_GNUC_PRINTF(4, 5)
+static coroutine_fn int G_GNUC_PRINTF(4, 5)
 nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type,
                            Error **errp, const char *fmt, ...)
 {
@@ -275,7 +276,7 @@ nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type,
 /* Drop remainder of the current option, and send a reply with the
  * given error type and message. Return -errno on read or write
  * failure; or 0 if connection is still live. */
-static int G_GNUC_PRINTF(4, 0)
+static coroutine_fn int G_GNUC_PRINTF(4, 0)
 nbd_opt_vdrop(NBDClient *client, uint32_t type, Error **errp,
               const char *fmt, va_list va)
 {
@@ -288,7 +289,7 @@ nbd_opt_vdrop(NBDClient *client, uint32_t type, Error **errp,
     return ret;
 }

-static int G_GNUC_PRINTF(4, 5)
+static coroutine_fn int G_GNUC_PRINTF(4, 5)
 nbd_opt_drop(NBDClient *client, uint32_t type, Error **errp,
              const char *fmt, ...)
 {
@@ -302,7 +303,7 @@ nbd_opt_drop(NBDClient *client, uint32_t type, Error **errp,
     return ret;
 }

-static int G_GNUC_PRINTF(3, 4)
+static coroutine_fn int G_GNUC_PRINTF(3, 4)
 nbd_opt_invalid(NBDClient *client, Error **errp, const char *fmt, ...)
 {
     int ret;
@@ -319,8 +320,9 @@ nbd_opt_invalid(NBDClient *client, Error **errp, const char *fmt, ...)
  * If @check_nul, require that no NUL bytes appear in buffer.
  * Return -errno on I/O error, 0 if option was completely handled by
  * sending a reply about inconsistent lengths, or 1 on success. */
-static int nbd_opt_read(NBDClient *client, void *buffer, size_t size,
-                        bool check_nul, Error **errp)
+static coroutine_fn int
+nbd_opt_read(NBDClient *client, void *buffer, size_t size,
+             bool check_nul, Error **errp)
 {
     if (size > client->optlen) {
         return nbd_opt_invalid(client, errp,
@@ -343,7 +345,8 @@ static int nbd_opt_read(NBDClient *client, void *buffer, size_t size,
 /* Drop size bytes from the unparsed payload of the current option.
  * Return -errno on I/O error, 0 if option was completely handled by
  * sending a reply about inconsistent lengths, or 1 on success. */
-static int nbd_opt_skip(NBDClient *client, size_t size, Error **errp)
+static coroutine_fn int
+nbd_opt_skip(NBDClient *client, size_t size, Error **errp)
 {
     if (size > client->optlen) {
         return nbd_opt_invalid(client, errp,
@@ -366,8 +369,9 @@ static int nbd_opt_skip(NBDClient *client, size_t size, Error **errp)
  * Return -errno on I/O error, 0 if option was completely handled by
  * sending a reply about inconsistent lengths, or 1 on success.
  */
-static int nbd_opt_read_name(NBDClient *client, char **name, uint32_t *length,
-                             Error **errp)
+static coroutine_fn int
+nbd_opt_read_name(NBDClient *client, char **name, uint32_t *length,
+                  Error **errp)
 {
     int ret;
     uint32_t len;
@@ -402,8 +406,8 @@ static int nbd_opt_read_name(NBDClient *client, char **name, uint32_t *length,

 /* Send a single NBD_REP_SERVER reply to NBD_OPT_LIST, including payload.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp,
-                                       Error **errp)
+static coroutine_fn int
+nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp, Error **errp)
 {
     ERRP_GUARD();
     size_t name_len, desc_len;
@@ -444,7 +448,8 @@ static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp,

 /* Process the NBD_OPT_LIST command, with a potential series of replies.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_handle_list(NBDClient *client, Error **errp)
+static coroutine_fn int
+nbd_negotiate_handle_list(NBDClient *client, Error **errp)
 {
     NBDExport *exp;
     assert(client->opt == NBD_OPT_LIST);
@@ -459,7 +464,8 @@ static int nbd_negotiate_handle_list(NBDClient *client, Error **errp)
     return nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
 }

-static void nbd_check_meta_export(NBDClient *client, NBDExport *exp)
+static coroutine_fn void
+nbd_check_meta_export(NBDClient *client, NBDExport *exp)
 {
     if (exp != client->contexts.exp) {
         client->contexts.count = 0;
@@ -468,8 +474,9 @@ static void nbd_check_meta_export(NBDClient *client, NBDExport *exp)

 /* Send a reply to NBD_OPT_EXPORT_NAME.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
-                                            Error **errp)
+static coroutine_fn int
+nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
+                                 Error **errp)
 {
     ERRP_GUARD();
     g_autofree char *name = NULL;
@@ -536,9 +543,9 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
 /* Send a single NBD_REP_INFO, with a buffer @buf of @length bytes.
  * The buffer does NOT include the info type prefix.
  * Return -errno on error, 0 if ready to send more. */
-static int nbd_negotiate_send_info(NBDClient *client,
-                                   uint16_t info, uint32_t length, void *buf,
-                                   Error **errp)
+static coroutine_fn int
+nbd_negotiate_send_info(NBDClient *client, uint16_t info, uint32_t length,
+                        void *buf, Error **errp)
 {
     int rc;

@@ -565,7 +572,8 @@ static int nbd_negotiate_send_info(NBDClient *client,
  * -errno  transmission error occurred or @fatal was requested, errp is set
  * 0       error message successfully sent to client, errp is not set
  */
-static int nbd_reject_length(NBDClient *client, bool fatal, Error **errp)
+static coroutine_fn int
+nbd_reject_length(NBDClient *client, bool fatal, Error **errp)
 {
     int ret;

@@ -583,7 +591,8 @@ static int nbd_reject_length(NBDClient *client, bool fatal, Error **errp)
 /* Handle NBD_OPT_INFO and NBD_OPT_GO.
  * Return -errno on error, 0 if ready for next option, and 1 to move
  * into transmission phase.  */
-static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
+static coroutine_fn int
+nbd_negotiate_handle_info(NBDClient *client, Error **errp)
 {
     int rc;
     g_autofree char *name = NULL;
@@ -755,7 +764,8 @@ struct NBDTLSServerHandshakeData {
     Coroutine *co;
 };

-static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
+static coroutine_fn void
+nbd_server_tls_handshake(QIOTask *task, void *opaque)
 {
     struct NBDTLSServerHandshakeData *data = opaque;

@@ -768,8 +778,8 @@ static void nbd_server_tls_handshake(QIOTask *task, void *opaque)

 /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the
  * new channel for all further (now-encrypted) communication. */
-static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
-                                                 Error **errp)
+static coroutine_fn QIOChannel *
+nbd_negotiate_handle_starttls(NBDClient *client, Error **errp)
 {
     QIOChannel *ioc;
     QIOChannelTLS *tioc;
@@ -821,10 +831,9 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
  *
  * For NBD_OPT_LIST_META_CONTEXT @context_id is ignored, 0 is used instead.
  */
-static int nbd_negotiate_send_meta_context(NBDClient *client,
-                                           const char *context,
-                                           uint32_t context_id,
-                                           Error **errp)
+static coroutine_fn int
+nbd_negotiate_send_meta_context(NBDClient *client, const char *context,
+                                uint32_t context_id, Error **errp)
 {
     NBDOptionReplyMetaContext opt;
     struct iovec iov[] = {
@@ -849,8 +858,9 @@ static int nbd_negotiate_send_meta_context(NBDClient *client,
  * Return true if @query matches @pattern, or if @query is empty when
  * the @client is performing _LIST_.
  */
-static bool nbd_meta_empty_or_pattern(NBDClient *client, const char *pattern,
-                                      const char *query)
+static coroutine_fn bool
+nbd_meta_empty_or_pattern(NBDClient *client, const char *pattern,
+                          const char *query)
 {
     if (!*query) {
         trace_nbd_negotiate_meta_query_parse("empty");
@@ -867,7 +877,8 @@ static bool nbd_meta_empty_or_pattern(NBDClient *client, const char *pattern,
 /*
  * Return true and adjust @str in place if it begins with @prefix.
  */
-static bool nbd_strshift(const char **str, const char *prefix)
+static coroutine_fn bool
+nbd_strshift(const char **str, const char *prefix)
 {
     size_t len = strlen(prefix);

@@ -883,8 +894,9 @@ static bool nbd_strshift(const char **str, const char *prefix)
  * Handle queries to 'base' namespace. For now, only the base:allocation
  * context is available.  Return true if @query has been handled.
  */
-static bool nbd_meta_base_query(NBDClient *client, NBDMetaContexts *meta,
-                                const char *query)
+static coroutine_fn bool
+nbd_meta_base_query(NBDClient *client, NBDMetaContexts *meta,
+                    const char *query)
 {
     if (!nbd_strshift(&query, "base:")) {
         return false;
@@ -903,8 +915,9 @@ static bool nbd_meta_base_query(NBDClient *client, NBDMetaContexts *meta,
  * and qemu:allocation-depth contexts are available.  Return true if @query
  * has been handled.
  */
-static bool nbd_meta_qemu_query(NBDClient *client, NBDMetaContexts *meta,
-                                const char *query)
+static coroutine_fn bool
+nbd_meta_qemu_query(NBDClient *client, NBDMetaContexts *meta,
+                    const char *query)
 {
     size_t i;

@@ -968,8 +981,9 @@ static bool nbd_meta_qemu_query(NBDClient *client, NBDMetaContexts *meta,
  *
  * Return -errno on I/O error, 0 if option was completely handled by
  * sending a reply about inconsistent lengths, or 1 on success. */
-static int nbd_negotiate_meta_query(NBDClient *client,
-                                    NBDMetaContexts *meta, Error **errp)
+static coroutine_fn int
+nbd_negotiate_meta_query(NBDClient *client,
+                         NBDMetaContexts *meta, Error **errp)
 {
     int ret;
     g_autofree char *query = NULL;
@@ -1008,7 +1022,8 @@ static int nbd_negotiate_meta_query(NBDClient *client,
  * Handle NBD_OPT_LIST_META_CONTEXT and NBD_OPT_SET_META_CONTEXT
  *
  * Return -errno on I/O error, or 0 if option was completely handled. */
-static int nbd_negotiate_meta_queries(NBDClient *client, Error **errp)
+static coroutine_fn int
+nbd_negotiate_meta_queries(NBDClient *client, Error **errp)
 {
     int ret;
     g_autofree char *export_name = NULL;
@@ -1136,7 +1151,8 @@ static int nbd_negotiate_meta_queries(NBDClient *client, Error **errp)
  * 1       if client sent NBD_OPT_ABORT, i.e. on valid disconnect,
  *         errp is not set
  */
-static int nbd_negotiate_options(NBDClient *client, Error **errp)
+static coroutine_fn int
+nbd_negotiate_options(NBDClient *client, Error **errp)
 {
     uint32_t flags;
     bool fixedNewstyle = false;
-- 
2.44.0

Re: [PATCH v5 2/2] nbd/server: Mark negotiation functions as coroutine_fn

[Vladimir Sementsov-Ogievskiy]

On 08.04.24 19:00, Eric Blake wrote:
> nbd_negotiate() is already marked coroutine_fn.  And given the fix in
> the previous patch to have nbd_negotiate_handle_starttls not create
> and wait on a g_main_loop (as that would violate coroutine
> constraints), it is worth marking the rest of the related static
> functions reachable only during option negotiation as also being
> coroutine_fn.
> 
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   nbd/server.c | 102 +++++++++++++++++++++++++++++----------------------
>   1 file changed, 59 insertions(+), 43 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 98ae0e16326..1857fba51c1 100644

[..]

>   {
>       int rc;
>       g_autofree char *name = NULL;
> @@ -755,7 +764,8 @@ struct NBDTLSServerHandshakeData {
>       Coroutine *co;
>   };
> 
> -static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
> +static coroutine_fn void

This is not, that's a callback for tls handshake, which is not coroutine context as I understand.
without this hunk:


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>


> +nbd_server_tls_handshake(QIOTask *task, void *opaque)
>   {
>       struct NBDTLSServerHandshakeData *data = opaque;
> 
> @@ -768,8 +778,8 @@ static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
> 

[..]

-- 
Best regards,
Vladimir

Re: [PATCH v5 2/2] nbd/server: Mark negotiation functions as coroutine_fn

[Eric Blake]

On Tue, Apr 09, 2024 at 09:30:39AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 08.04.24 19:00, Eric Blake wrote:
> > nbd_negotiate() is already marked coroutine_fn.  And given the fix in
> > the previous patch to have nbd_negotiate_handle_starttls not create
> > and wait on a g_main_loop (as that would violate coroutine
> > constraints), it is worth marking the rest of the related static
> > functions reachable only during option negotiation as also being
> > coroutine_fn.
> > 
> > Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >   nbd/server.c | 102 +++++++++++++++++++++++++++++----------------------
> >   1 file changed, 59 insertions(+), 43 deletions(-)
> > 
> > diff --git a/nbd/server.c b/nbd/server.c
> > index 98ae0e16326..1857fba51c1 100644
> 
> [..]
> 
> >   {
> >       int rc;
> >       g_autofree char *name = NULL;
> > @@ -755,7 +764,8 @@ struct NBDTLSServerHandshakeData {
> >       Coroutine *co;
> >   };
> > 
> > -static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
> > +static coroutine_fn void
> 
> This is not, that's a callback for tls handshake, which is not coroutine context as I understand.

The call chain is:

nbd_negotiate() - coroutine_fn before this patch
  nbd_negotiate_options() - marked coroutine_fn here
    nbd_negotiate_handle_starttls() - marked coroutine_fn here
      qio_channel_tls_handshake() - in io/channel-tls.c; not marked coroutine_fn, but
                                    works both in or out of coroutine context
        ...
        nbd_server_tls_handshake() - renamed in 1/2 of this series

> without this hunk:

I can drop that particular marking.  There are cases where the
callback is reached without having done the qemu_coroutine_yield() in
nbd_negotiate_handle_starttls; but there are also cases where the IO
took enough time that the coroutine yielded and it is that callback
that reawakens it.

> 
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Thanks.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH v5 2/2] nbd/server: Mark negotiation functions as coroutine_fn

[Vladimir Sementsov-Ogievskiy]

On 09.04.24 18:49, Eric Blake wrote:
> On Tue, Apr 09, 2024 at 09:30:39AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 08.04.24 19:00, Eric Blake wrote:
>>> nbd_negotiate() is already marked coroutine_fn.  And given the fix in
>>> the previous patch to have nbd_negotiate_handle_starttls not create
>>> and wait on a g_main_loop (as that would violate coroutine
>>> constraints), it is worth marking the rest of the related static
>>> functions reachable only during option negotiation as also being
>>> coroutine_fn.
>>>
>>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>    nbd/server.c | 102 +++++++++++++++++++++++++++++----------------------
>>>    1 file changed, 59 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/nbd/server.c b/nbd/server.c
>>> index 98ae0e16326..1857fba51c1 100644
>>
>> [..]
>>
>>>    {
>>>        int rc;
>>>        g_autofree char *name = NULL;
>>> @@ -755,7 +764,8 @@ struct NBDTLSServerHandshakeData {
>>>        Coroutine *co;
>>>    };
>>>
>>> -static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
>>> +static coroutine_fn void
>>
>> This is not, that's a callback for tls handshake, which is not coroutine context as I understand.
> 
> The call chain is:
> 
> nbd_negotiate() - coroutine_fn before this patch
>    nbd_negotiate_options() - marked coroutine_fn here
>      nbd_negotiate_handle_starttls() - marked coroutine_fn here
>        qio_channel_tls_handshake() - in io/channel-tls.c; not marked coroutine_fn, but
>                                      works both in or out of coroutine context
>          ...
>          nbd_server_tls_handshake() - renamed in 1/2 of this series
> 
>> without this hunk:
> 
> I can drop that particular marking.  There are cases where the
> callback is reached without having done the qemu_coroutine_yield() in
> nbd_negotiate_handle_starttls; but there are also cases where the IO
> took enough time that the coroutine yielded and it is that callback
> that reawakens it.

The key thing for me is that in this case (when coroutine yielded), nbd_server_tls_handshake() would finally be called from glib IO handler, set in

    qio_channel_tls_handshake()
       qio_channel_tls_handshake_task()
          qio_channel_add_watch_full()
             g_source_set_callback(source, (GSourceFunc)func, user_data, notify);   <<<

, which would definitely not be a coroutine context.


Do I understand correctly, that "coroutine_fn" means "only call from coroutine context"[1], or does it mean "could be called from coroutine context"[2]?

The comment in osdep.h says:

  * Mark a function that executes in coroutine context                                                     |}
  *                                                                                                        |
  * Functions that execute in coroutine context cannot be called directly from                             |
  * normal functions. ...

So I assume, we mean [1].

> 
>>
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> Thanks.
> 

-- 
Best regards,
Vladimir

Re: [PATCH v5 2/2] nbd/server: Mark negotiation functions as coroutine_fn

[Eric Blake]

On Wed, Apr 10, 2024 at 10:05:28AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > @@ -755,7 +764,8 @@ struct NBDTLSServerHandshakeData {
> > > >        Coroutine *co;
> > > >    };
> > > > 
> > > > -static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
> > > > +static coroutine_fn void
> > > 
> > > This is not, that's a callback for tls handshake, which is not coroutine context as I understand.
> > 
> > The call chain is:
> > 
> > nbd_negotiate() - coroutine_fn before this patch
> >    nbd_negotiate_options() - marked coroutine_fn here
> >      nbd_negotiate_handle_starttls() - marked coroutine_fn here
> >        qio_channel_tls_handshake() - in io/channel-tls.c; not marked coroutine_fn, but
> >                                      works both in or out of coroutine context
> >          ...
> >          nbd_server_tls_handshake() - renamed in 1/2 of this series
> > 
> > > without this hunk:
> > 
> > I can drop that particular marking.  There are cases where the
> > callback is reached without having done the qemu_coroutine_yield() in
> > nbd_negotiate_handle_starttls; but there are also cases where the IO
> > took enough time that the coroutine yielded and it is that callback
> > that reawakens it.
> 
> The key thing for me is that in this case (when coroutine yielded), nbd_server_tls_handshake() would finally be called from glib IO handler, set in
> 
>    qio_channel_tls_handshake()
>       qio_channel_tls_handshake_task()
>          qio_channel_add_watch_full()
>             g_source_set_callback(source, (GSourceFunc)func, user_data, notify);   <<<
> 
> , which would definitely not be a coroutine context.
> 
> 
> Do I understand correctly, that "coroutine_fn" means "only call from coroutine context"[1], or does it mean "could be called from coroutine context"[2]?

I'm always fuzzy on that distinction myself.  But reading the docs helps...


> 
> The comment in osdep.h says:
> 
>  * Mark a function that executes in coroutine context                                                     |}
>  *                                                                                                        |
>  * Functions that execute in coroutine context cannot be called directly from                             |
>  * normal functions. ...
> 
> So I assume, we mean [1].

...[1] sounds more appropriate.  Adding the marker is more of a
promise that "I've audited that this function does not block and is
therefore safe for a function in coroutine context to use", but
sometimes additionally implies "this function assumes a coroutine is
present; if you are not in a coroutine, things might break".  But with
a bit of thought, it is obvious that a coroutine can call functions
that are not marked with coroutine_fn: any non-blocking syscall fits
in this category (since we don't have control over system headers to
add coroutine_fn annotations to those).  So on that grounds, it is
okay for a function marked coroutine_fn to call another function not
marked coroutine_fn - it just makes the auditing harder to ensure that
you aren't violating your promise of no non-blocking calls.

It's too close to 9.0-rc3 for my comfort to include this patch series.
Even though this bug can cause wedged migrations, I'd feel more
comfortable with preparing the pull request for this series (including
your fix for dropping this one annotation) for 9.1 and for qemu-stable
once the release is complete.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org