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-de...@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-de...@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
