| Message ID | 8y2vfZuyQoZPUsO-9E_Vl_x5LG4S3-ewrNqvmbgOTUHPglYpU2A0-jjdIh78JySlGCqhHgfXXezC_HGTIbSdlsqcT9YzUKr0b_FKp1OLk00=@protonmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add missing coroutine_fn function signature to functions | expand |
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.
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
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
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);