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

[Stefan Hajnoczi]

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.

signature.asc
Description: PGP signature