[PATCH] Add missing coroutine_fn function signature to functions

[PATCH] Add missing coroutine_fn function signature to functions

[cennedee]

From 447601c28d5ed0b1208a0560390f760e75ce5613 Mon Sep 17 00:00:00 2001
From: Cenne Dee <cennedee+qemu-devel@protonmail.com>
Date: Fri, 30 Apr 2021 15:52:28 -0400
Subject: [PATCH] Add missing coroutine_fn function signature to functions

Patch adds the signature for all relevant functions ending with _co
or those that use them.

Signed-off-by: Cenne Dee <cennedee+qemu-devel@protonmail.com>
---
 block/block-gen.h             | 2 +-
 migration/migration.c         | 2 +-
 monitor/hmp.c                 | 2 +-
 scsi/qemu-pr-helper.c         | 2 +-
 tests/unit/test-thread-pool.c | 4 ++--
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/block-gen.h b/block/block-gen.h
index f80cf48..4963372 100644
--- a/block/block-gen.h
+++ b/block/block-gen.h
@@ -36,7 +36,7 @@ typedef struct BdrvPollCo {
     Coroutine *co; /* Keep pointer here for debugging */
 } BdrvPollCo;

-static inline int bdrv_poll_co(BdrvPollCo *s)
+static inline int coroutine_fn bdrv_poll_co(BdrvPollCo *s)
 {
     assert(!qemu_in_coroutine());

diff --git a/migration/migration.c b/migration/migration.c
index 8ca0341..1acade8 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -539,7 +539,7 @@ static void process_incoming_migration_bh(void *opaque)
     migration_incoming_state_destroy();
 }

-static void process_incoming_migration_co(void *opaque)
+static void coroutine_fn process_incoming_migration_co(void *opaque)
 {
     MigrationIncomingState *mis = migration_incoming_get_current();
     PostcopyState ps;
diff --git a/monitor/hmp.c b/monitor/hmp.c
index 6c0b33a..0a16d61 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1068,7 +1068,7 @@ typedef struct HandleHmpCommandCo {
     bool done;
 } HandleHmpCommandCo;

-static void handle_hmp_command_co(void *opaque)
+static void coroutine_fn handle_hmp_command_co(void *opaque)
 {
     HandleHmpCommandCo *data = opaque;
     data->cmd->cmd(data->mon, data->qdict);
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 7b9389b..7c1ed2a 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -175,7 +175,7 @@ static int do_sgio_worker(void *opaque)
     return status;
 }

-static int do_sgio(int fd, const uint8_t *cdb, uint8_t *sense,
+static int coroutine_fn do_sgio(int fd, const uint8_t *cdb, uint8_t *sense,
                     uint8_t *buf, int *sz, int dir)
 {
     ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
diff --git a/tests/unit/test-thread-pool.c b/tests/unit/test-thread-pool.c
index 70dc631..21fc118 100644
--- a/tests/unit/test-thread-pool.c
+++ b/tests/unit/test-thread-pool.c
@@ -72,7 +72,7 @@ static void test_submit_aio(void)
     g_assert_cmpint(data.ret, ==, 0);
 }

-static void co_test_cb(void *opaque)
+static void coroutine_fn co_test_cb(void *opaque)
 {
     WorkerTestData *data = opaque;

@@ -90,7 +90,7 @@ static void co_test_cb(void *opaque)
     /* The test continues in test_submit_co, after aio_poll... */
 }

-static void test_submit_co(void)
+static void coroutine_fn test_submit_co(void)
 {
     WorkerTestData data;
     Coroutine *co = qemu_coroutine_create(co_test_cb, &data);
--
2.31.1

Re: [PATCH] Add missing coroutine_fn function signature to functions

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3024 bytes --]

On Fri, Apr 30, 2021 at 09:34:41PM +0000, cennedee wrote:
> From 447601c28d5ed0b1208a0560390f760e75ce5613 Mon Sep 17 00:00:00 2001
> From: Cenne Dee <cennedee+qemu-devel@protonmail.com>
> Date: Fri, 30 Apr 2021 15:52:28 -0400
> Subject: [PATCH] Add missing coroutine_fn function signature to functions
> 
> Patch adds the signature for all relevant functions ending with _co
> or those that use them.
> 
> Signed-off-by: Cenne Dee <cennedee+qemu-devel@protonmail.com>
> ---
>  block/block-gen.h             | 2 +-
>  migration/migration.c         | 2 +-
>  monitor/hmp.c                 | 2 +-
>  scsi/qemu-pr-helper.c         | 2 +-
>  tests/unit/test-thread-pool.c | 4 ++--
>  5 files changed, 6 insertions(+), 6 deletions(-)

Hi,
Thanks for discussing coroutine_fn on IRC! Here is some feedback on this
patch:

> diff --git a/block/block-gen.h b/block/block-gen.h
> index f80cf48..4963372 100644
> --- a/block/block-gen.h
> +++ b/block/block-gen.h
> @@ -36,7 +36,7 @@ typedef struct BdrvPollCo {
>      Coroutine *co; /* Keep pointer here for debugging */
>  } BdrvPollCo;
> 
> -static inline int bdrv_poll_co(BdrvPollCo *s)
> +static inline int coroutine_fn bdrv_poll_co(BdrvPollCo *s)
>  {
>      assert(!qemu_in_coroutine());

The assert indicates that this function must not be called from a
coroutine. Adding coroutine_fn is incorrect here.

> diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> index 7b9389b..7c1ed2a 100644
> --- a/scsi/qemu-pr-helper.c
> +++ b/scsi/qemu-pr-helper.c
> @@ -175,7 +175,7 @@ static int do_sgio_worker(void *opaque)
>      return status;
>  }
> 
> -static int do_sgio(int fd, const uint8_t *cdb, uint8_t *sense,
> +static int coroutine_fn do_sgio(int fd, const uint8_t *cdb, uint8_t *sense,
>                      uint8_t *buf, int *sz, int dir)
>  {
>      ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());

This is correct but there are several functions that call do_sgio() that
also need coroutine_fn. The eventual parent is prh_co_entry() and it's a
good place to start auditing the code.

> diff --git a/tests/unit/test-thread-pool.c b/tests/unit/test-thread-pool.c
> index 70dc631..21fc118 100644
> --- a/tests/unit/test-thread-pool.c
> +++ b/tests/unit/test-thread-pool.c
> @@ -72,7 +72,7 @@ static void test_submit_aio(void)
>      g_assert_cmpint(data.ret, ==, 0);
>  }
> 
> -static void co_test_cb(void *opaque)
> +static void coroutine_fn co_test_cb(void *opaque)
>  {
>      WorkerTestData *data = opaque;
> 
> @@ -90,7 +90,7 @@ static void co_test_cb(void *opaque)
>      /* The test continues in test_submit_co, after aio_poll... */
>  }
> 
> -static void test_submit_co(void)
> +static void coroutine_fn test_submit_co(void)

This function is not called from a coroutine and should not have
coroutine_fn. It's a regular function called by gtester:

  g_test_add_func("/thread-pool/submit-co", test_submit_co);

The above change to co_test_cb() is correct though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Add missing coroutine_fn function signature to functions

[cennedee]

Thank you for the feedback. Here is an updated patch.
Added more functions related to do_sgio() as suggested. Also found one related to prh_co_entry()

I have removed (edits on) two files as there are many more functions I found in those. So I think another thread might be more appropriate (where I'll send patches flie by file). Focusing on two files here now.

For reference the path of calls that end up on a coroutine_fn below

do_pr_in() --> do_sgio()
do_pr_out() --> do_sgio()
mpath_reconstruct_sense() --> do_sgio()
multipath_pr_out() --> mpath_reconstruct_sense() --> do_sgio()
multipath_pr_in() --> mpath_reconstruct_sense() --> do_sgio()
accept_client() --> prh_co_entry()



From 79f787c2ef5f713546b38a76367d273ff65742d6 Mon Sep 17 00:00:00 2001
From: Cenne Dee <cennedee+qemu-devel@protonmail.com>
Date: Fri, 30 Apr 2021 15:52:28 -0400
Subject: [PATCH] Add missing coroutine_fn function signature to some _co()
 functions

Patch adds the signature for relevant functions ending with _co
or those that use them.

Signed-off-by: Cenne Dee <cennedee+qemu-devel@protonmail.com>
---
 scsi/qemu-pr-helper.c         | 14 +++++++-------
 tests/unit/test-thread-pool.c |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 7b9389b..695539b 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -175,7 +175,7 @@ static int do_sgio_worker(void *opaque)
     return status;
 }

-static int do_sgio(int fd, const uint8_t *cdb, uint8_t *sense,
+static int coroutine_fn do_sgio(int fd, const uint8_t *cdb, uint8_t *sense,
                     uint8_t *buf, int *sz, int dir)
 {
     ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
@@ -318,7 +318,7 @@ static SCSISense mpath_generic_sense(int r)
     }
 }

-static int mpath_reconstruct_sense(int fd, int r, uint8_t *sense)
+static int coroutine_fn mpath_reconstruct_sense(int fd, int r, uint8_t *sense)
 {
     switch (r) {
     case MPATH_PR_SUCCESS:
@@ -370,7 +370,7 @@ static int mpath_reconstruct_sense(int fd, int r, uint8_t *sense)
     }
 }

-static int multipath_pr_in(int fd, const uint8_t *cdb, uint8_t *sense,
+static int coroutine_fn multipath_pr_in(int fd, const uint8_t *cdb, uint8_t *sense,
                            uint8_t *data, int sz)
 {
     int rq_servact = cdb[1];
@@ -425,7 +425,7 @@ static int multipath_pr_in(int fd, const uint8_t *cdb, uint8_t *sense,
     return mpath_reconstruct_sense(fd, r, sense);
 }

-static int multipath_pr_out(int fd, const uint8_t *cdb, uint8_t *sense,
+static int coroutine_fn multipath_pr_out(int fd, const uint8_t *cdb, uint8_t *sense,
                             const uint8_t *param, int sz)
 {
     int rq_servact = cdb[1];
@@ -543,7 +543,7 @@ static int multipath_pr_out(int fd, const uint8_t *cdb, uint8_t *sense,
 }
 #endif

-static int do_pr_in(int fd, const uint8_t *cdb, uint8_t *sense,
+static int coroutine_fn do_pr_in(int fd, const uint8_t *cdb, uint8_t *sense,
                     uint8_t *data, int *resp_sz)
 {
 #ifdef CONFIG_MPATH
@@ -561,7 +561,7 @@ static int do_pr_in(int fd, const uint8_t *cdb, uint8_t *sense,
                    SG_DXFER_FROM_DEV);
 }

-static int do_pr_out(int fd, const uint8_t *cdb, uint8_t *sense,
+static int coroutine_fn do_pr_out(int fd, const uint8_t *cdb, uint8_t *sense,
                      const uint8_t *param, int sz)
 {
     int resp_sz;
@@ -804,7 +804,7 @@ out:
     g_free(client);
 }

-static gboolean accept_client(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
+static gboolean coroutine_fn accept_client(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
 {
     QIOChannelSocket *cioc;
     PRHelperClient *prh;
diff --git a/tests/unit/test-thread-pool.c b/tests/unit/test-thread-pool.c
index 70dc631..dbaf72c 100644
--- a/tests/unit/test-thread-pool.c
+++ b/tests/unit/test-thread-pool.c
@@ -72,7 +72,7 @@ static void test_submit_aio(void)
     g_assert_cmpint(data.ret, ==, 0);
 }

-static void co_test_cb(void *opaque)
+static void coroutine_fn co_test_cb(void *opaque)
 {
     WorkerTestData *data = opaque;

--
2.31.1

Re: [PATCH] Add missing coroutine_fn function signature to functions

[cennedee]

Focusing on a single file at a time now, this particular revised patch adds
missing function signature `coroutine_fn` to definitions in scsi/qemu-pr-helper.c
Intend to do more files in a separate patch series once I get the full flow of this.

Compared to my previous e-mail, have also confirmed this edit passes checkpatch.pl

The following functions are affected.

do_sgio()
do_pr_in() --> do_sgio()
do_pr_out() --> do_sgio()
mpath_reconstruct_sense() --> do_sgio()
multipath_pr_out() --> mpath_reconstruct_sense() --> do_sgio()
multipath_pr_in() --> mpath_reconstruct_sense() --> do_sgio()
accept_client() --> prh_co_entry()


From 5bdef14027457d412972131dace76c3cabcc45a0 Mon Sep 17 00:00:00 2001
From: Cenne Dee <cennedee+qemu-devel@protonmail.com>
Date: Fri, 30 Apr 2021 15:52:28 -0400
Subject: [PATCH] Add missing coroutine_fn function signature to some _co()
 functions

Patch adds the signature for relevant functions ending with _co
or those that use them.

Signed-off-by: Cenne Dee <cennedee+qemu-devel@protonmail.com>
---
 scsi/qemu-pr-helper.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 7b9389b47b..7ed47c17c7 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -175,8 +175,8 @@ static int do_sgio_worker(void *opaque)
     return status;
 }

-static int do_sgio(int fd, const uint8_t *cdb, uint8_t *sense,
-                    uint8_t *buf, int *sz, int dir)
+static int coroutine_fn do_sgio(int fd, const uint8_t *cdb, uint8_t *sense,
+                                uint8_t *buf, int *sz, int dir)
 {
     ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
     int r;
@@ -318,7 +318,7 @@ static SCSISense mpath_generic_sense(int r)
     }
 }

-static int mpath_reconstruct_sense(int fd, int r, uint8_t *sense)
+static int coroutine_fn mpath_reconstruct_sense(int fd, int r, uint8_t *sense)
 {
     switch (r) {
     case MPATH_PR_SUCCESS:
@@ -370,8 +370,8 @@ static int mpath_reconstruct_sense(int fd, int r, uint8_t *sense)
     }
 }

-static int multipath_pr_in(int fd, const uint8_t *cdb, uint8_t *sense,
-                           uint8_t *data, int sz)
+static int coroutine_fn multipath_pr_in(int fd, const uint8_t *cdb,
+                                        uint8_t *sense, uint8_t *data, int sz)
 {
     int rq_servact = cdb[1];
     struct prin_resp resp;
@@ -425,8 +425,9 @@ static int multipath_pr_in(int fd, const uint8_t *cdb, uint8_t *sense,
     return mpath_reconstruct_sense(fd, r, sense);
 }

-static int multipath_pr_out(int fd, const uint8_t *cdb, uint8_t *sense,
-                            const uint8_t *param, int sz)
+static int coroutine_fn multipath_pr_out(int fd, const uint8_t *cdb,
+                                         uint8_t *sense, const uint8_t *param,
+                                         int sz)
 {
     int rq_servact = cdb[1];
     int rq_scope = cdb[2] >> 4;
@@ -543,8 +544,8 @@ static int multipath_pr_out(int fd, const uint8_t *cdb, uint8_t *sense,
 }
 #endif

-static int do_pr_in(int fd, const uint8_t *cdb, uint8_t *sense,
-                    uint8_t *data, int *resp_sz)
+static int coroutine_fn do_pr_in(int fd, const uint8_t *cdb, uint8_t *sense,
+                                 uint8_t *data, int *resp_sz)
 {
 #ifdef CONFIG_MPATH
     if (is_mpath(fd)) {
@@ -561,8 +562,8 @@ static int do_pr_in(int fd, const uint8_t *cdb, uint8_t *sense,
                    SG_DXFER_FROM_DEV);
 }

-static int do_pr_out(int fd, const uint8_t *cdb, uint8_t *sense,
-                     const uint8_t *param, int sz)
+static int coroutine_fn do_pr_out(int fd, const uint8_t *cdb, uint8_t *sense,
+                                  const uint8_t *param, int sz)
 {
     int resp_sz;

@@ -804,7 +805,8 @@ out:
     g_free(client);
 }

-static gboolean accept_client(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
+static gboolean coroutine_fn accept_client(QIOChannel *ioc, GIOCondition cond,
+                                           gpointer opaque)
 {
     QIOChannelSocket *cioc;
     PRHelperClient *prh;
--
2.31.1