Message ID | 20180830015734.19765-2-jsnow@redhat.com |
---|---|
State | New |
Headers | show |
Series | jobs: Job Exit Refactoring Pt 1 | expand |
On Wed, Aug 29, 2018 at 09:57:26PM -0400, John Snow wrote: > Presently we codify the entry point for a job as the "start" callback, > but a more apt name would be "run" to clarify the idea that when this > function returns we consider the job to have "finished," except for > any cleanup which occurs in separate callbacks later. > > As part of this clarification, change the signature to include an error > object and a return code. The error ptr is not yet used, and the return > code while captured, will be overwritten by actions in the job_completed > function. > > Signed-off-by: John Snow <jsnow@redhat.com> > Reviewed-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Jeff Cody <jcody@redhat.com> > --- > block/backup.c | 7 ++++--- > block/commit.c | 7 ++++--- > block/create.c | 8 +++++--- > block/mirror.c | 10 ++++++---- > block/stream.c | 7 ++++--- > include/qemu/job.h | 2 +- > job.c | 6 +++--- > tests/test-bdrv-drain.c | 7 ++++--- > tests/test-blockjob-txn.c | 16 ++++++++-------- > tests/test-blockjob.c | 7 ++++--- > 10 files changed, 43 insertions(+), 34 deletions(-) > > diff --git a/block/backup.c b/block/backup.c > index 8630d32926..5d47781840 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -480,9 +480,9 @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job) > bdrv_dirty_iter_free(dbi); > } > > -static void coroutine_fn backup_run(void *opaque) > +static int coroutine_fn backup_run(Job *opaque_job, Error **errp) > { > - BackupBlockJob *job = opaque; > + BackupBlockJob *job = container_of(opaque_job, BackupBlockJob, common.job); > BackupCompleteData *data; > BlockDriverState *bs = blk_bs(job->common.blk); > int64_t offset, nb_clusters; > @@ -587,6 +587,7 @@ static void coroutine_fn backup_run(void *opaque) > data = g_malloc(sizeof(*data)); > data->ret = ret; > job_defer_to_main_loop(&job->common.job, backup_complete, data); > + return ret; > } > > static const BlockJobDriver backup_job_driver = { > @@ -596,7 +597,7 @@ static const BlockJobDriver backup_job_driver = { > .free = block_job_free, > .user_resume = block_job_user_resume, > .drain = block_job_drain, > - .start = backup_run, > + .run = backup_run, > .commit = backup_commit, > .abort = backup_abort, > .clean = backup_clean, > diff --git a/block/commit.c b/block/commit.c > index eb414579bd..a0ea86ff64 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -134,9 +134,9 @@ static void commit_complete(Job *job, void *opaque) > bdrv_unref(top); > } > > -static void coroutine_fn commit_run(void *opaque) > +static int coroutine_fn commit_run(Job *job, Error **errp) > { > - CommitBlockJob *s = opaque; > + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); > CommitCompleteData *data; > int64_t offset; > uint64_t delay_ns = 0; > @@ -213,6 +213,7 @@ out: > data = g_malloc(sizeof(*data)); > data->ret = ret; > job_defer_to_main_loop(&s->common.job, commit_complete, data); > + return ret; > } > > static const BlockJobDriver commit_job_driver = { > @@ -222,7 +223,7 @@ static const BlockJobDriver commit_job_driver = { > .free = block_job_free, > .user_resume = block_job_user_resume, > .drain = block_job_drain, > - .start = commit_run, > + .run = commit_run, > }, > }; > > diff --git a/block/create.c b/block/create.c > index 915cd41bcc..04733c3618 100644 > --- a/block/create.c > +++ b/block/create.c > @@ -45,9 +45,9 @@ static void blockdev_create_complete(Job *job, void *opaque) > job_completed(job, s->ret, s->err); > } > > -static void coroutine_fn blockdev_create_run(void *opaque) > +static int coroutine_fn blockdev_create_run(Job *job, Error **errp) > { > - BlockdevCreateJob *s = opaque; > + BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common); > > job_progress_set_remaining(&s->common, 1); > s->ret = s->drv->bdrv_co_create(s->opts, &s->err); > @@ -55,12 +55,14 @@ static void coroutine_fn blockdev_create_run(void *opaque) > > qapi_free_BlockdevCreateOptions(s->opts); > job_defer_to_main_loop(&s->common, blockdev_create_complete, NULL); > + > + return s->ret; > } > > static const JobDriver blockdev_create_job_driver = { > .instance_size = sizeof(BlockdevCreateJob), > .job_type = JOB_TYPE_CREATE, > - .start = blockdev_create_run, > + .run = blockdev_create_run, > }; > > void qmp_blockdev_create(const char *job_id, BlockdevCreateOptions *options, > diff --git a/block/mirror.c b/block/mirror.c > index 6cc10df5c9..691763db41 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -812,9 +812,9 @@ static int mirror_flush(MirrorBlockJob *s) > return ret; > } > > -static void coroutine_fn mirror_run(void *opaque) > +static int coroutine_fn mirror_run(Job *job, Error **errp) > { > - MirrorBlockJob *s = opaque; > + MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job); > MirrorExitData *data; > BlockDriverState *bs = s->mirror_top_bs->backing->bs; > BlockDriverState *target_bs = blk_bs(s->target); > @@ -1041,7 +1041,9 @@ immediate_exit: > if (need_drain) { > bdrv_drained_begin(bs); > } > + > job_defer_to_main_loop(&s->common.job, mirror_exit, data); > + return ret; > } > > static void mirror_complete(Job *job, Error **errp) > @@ -1138,7 +1140,7 @@ static const BlockJobDriver mirror_job_driver = { > .free = block_job_free, > .user_resume = block_job_user_resume, > .drain = block_job_drain, > - .start = mirror_run, > + .run = mirror_run, > .pause = mirror_pause, > .complete = mirror_complete, > }, > @@ -1154,7 +1156,7 @@ static const BlockJobDriver commit_active_job_driver = { > .free = block_job_free, > .user_resume = block_job_user_resume, > .drain = block_job_drain, > - .start = mirror_run, > + .run = mirror_run, > .pause = mirror_pause, > .complete = mirror_complete, > }, > diff --git a/block/stream.c b/block/stream.c > index 9264b68a1e..b4b987df7e 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -97,9 +97,9 @@ out: > g_free(data); > } > > -static void coroutine_fn stream_run(void *opaque) > +static int coroutine_fn stream_run(Job *job, Error **errp) > { > - StreamBlockJob *s = opaque; > + StreamBlockJob *s = container_of(job, StreamBlockJob, common.job); > StreamCompleteData *data; > BlockBackend *blk = s->common.blk; > BlockDriverState *bs = blk_bs(blk); > @@ -206,6 +206,7 @@ out: > data = g_malloc(sizeof(*data)); > data->ret = ret; > job_defer_to_main_loop(&s->common.job, stream_complete, data); > + return ret; > } > > static const BlockJobDriver stream_job_driver = { > @@ -213,7 +214,7 @@ static const BlockJobDriver stream_job_driver = { > .instance_size = sizeof(StreamBlockJob), > .job_type = JOB_TYPE_STREAM, > .free = block_job_free, > - .start = stream_run, > + .run = stream_run, > .user_resume = block_job_user_resume, > .drain = block_job_drain, > }, > diff --git a/include/qemu/job.h b/include/qemu/job.h > index 18c9223e31..9cf463d228 100644 > --- a/include/qemu/job.h > +++ b/include/qemu/job.h > @@ -169,7 +169,7 @@ struct JobDriver { > JobType job_type; > > /** Mandatory: Entrypoint for the Coroutine. */ > - CoroutineEntry *start; > + int coroutine_fn (*run)(Job *job, Error **errp); > > /** > * If the callback is not NULL, it will be invoked when the job transitions > diff --git a/job.c b/job.c > index e36ebaafd8..76988f6678 100644 > --- a/job.c > +++ b/job.c > @@ -544,16 +544,16 @@ static void coroutine_fn job_co_entry(void *opaque) > { > Job *job = opaque; > > - assert(job && job->driver && job->driver->start); > + assert(job && job->driver && job->driver->run); > job_pause_point(job); > - job->driver->start(job); > + job->ret = job->driver->run(job, NULL); > } > > > void job_start(Job *job) > { > assert(job && !job_started(job) && job->paused && > - job->driver && job->driver->start); > + job->driver && job->driver->run); > job->co = qemu_coroutine_create(job_co_entry, job); > job->pause_count--; > job->busy = true; > diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c > index 17bb8508ae..a7533861f6 100644 > --- a/tests/test-bdrv-drain.c > +++ b/tests/test-bdrv-drain.c > @@ -757,9 +757,9 @@ static void test_job_completed(Job *job, void *opaque) > job_completed(job, 0, NULL); > } > > -static void coroutine_fn test_job_start(void *opaque) > +static int coroutine_fn test_job_run(Job *job, Error **errp) > { > - TestBlockJob *s = opaque; > + TestBlockJob *s = container_of(job, TestBlockJob, common.job); > > job_transition_to_ready(&s->common.job); > while (!s->should_complete) { > @@ -771,6 +771,7 @@ static void coroutine_fn test_job_start(void *opaque) > } > > job_defer_to_main_loop(&s->common.job, test_job_completed, NULL); > + return 0; > } > > static void test_job_complete(Job *job, Error **errp) > @@ -785,7 +786,7 @@ BlockJobDriver test_job_driver = { > .free = block_job_free, > .user_resume = block_job_user_resume, > .drain = block_job_drain, > - .start = test_job_start, > + .run = test_job_run, > .complete = test_job_complete, > }, > }; > diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c > index 58d9b87fb2..3194924071 100644 > --- a/tests/test-blockjob-txn.c > +++ b/tests/test-blockjob-txn.c > @@ -38,25 +38,25 @@ static void test_block_job_complete(Job *job, void *opaque) > bdrv_unref(bs); > } > > -static void coroutine_fn test_block_job_run(void *opaque) > +static int coroutine_fn test_block_job_run(Job *job, Error **errp) > { > - TestBlockJob *s = opaque; > - BlockJob *job = &s->common; > + TestBlockJob *s = container_of(job, TestBlockJob, common.job); > > while (s->iterations--) { > if (s->use_timer) { > - job_sleep_ns(&job->job, 0); > + job_sleep_ns(job, 0); > } else { > - job_yield(&job->job); > + job_yield(job); > } > > - if (job_is_cancelled(&job->job)) { > + if (job_is_cancelled(job)) { > break; > } > } > > - job_defer_to_main_loop(&job->job, test_block_job_complete, > + job_defer_to_main_loop(job, test_block_job_complete, > (void *)(intptr_t)s->rc); > + return s->rc; > } > > typedef struct { > @@ -80,7 +80,7 @@ static const BlockJobDriver test_block_job_driver = { > .free = block_job_free, > .user_resume = block_job_user_resume, > .drain = block_job_drain, > - .start = test_block_job_run, > + .run = test_block_job_run, > }, > }; > > diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c > index cb42f06e61..b0462bfdec 100644 > --- a/tests/test-blockjob.c > +++ b/tests/test-blockjob.c > @@ -176,9 +176,9 @@ static void cancel_job_complete(Job *job, Error **errp) > s->should_complete = true; > } > > -static void coroutine_fn cancel_job_start(void *opaque) > +static int coroutine_fn cancel_job_run(Job *job, Error **errp) > { > - CancelJob *s = opaque; > + CancelJob *s = container_of(job, CancelJob, common.job); > > while (!s->should_complete) { > if (job_is_cancelled(&s->common.job)) { > @@ -194,6 +194,7 @@ static void coroutine_fn cancel_job_start(void *opaque) > > defer: > job_defer_to_main_loop(&s->common.job, cancel_job_completed, s); > + return 0; > } > > static const BlockJobDriver test_cancel_driver = { > @@ -202,7 +203,7 @@ static const BlockJobDriver test_cancel_driver = { > .free = block_job_free, > .user_resume = block_job_user_resume, > .drain = block_job_drain, > - .start = cancel_job_start, > + .run = cancel_job_run, > .complete = cancel_job_complete, > }, > }; > -- > 2.14.4 >
diff --git a/block/backup.c b/block/backup.c index 8630d32926..5d47781840 100644 --- a/block/backup.c +++ b/block/backup.c @@ -480,9 +480,9 @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job) bdrv_dirty_iter_free(dbi); } -static void coroutine_fn backup_run(void *opaque) +static int coroutine_fn backup_run(Job *opaque_job, Error **errp) { - BackupBlockJob *job = opaque; + BackupBlockJob *job = container_of(opaque_job, BackupBlockJob, common.job); BackupCompleteData *data; BlockDriverState *bs = blk_bs(job->common.blk); int64_t offset, nb_clusters; @@ -587,6 +587,7 @@ static void coroutine_fn backup_run(void *opaque) data = g_malloc(sizeof(*data)); data->ret = ret; job_defer_to_main_loop(&job->common.job, backup_complete, data); + return ret; } static const BlockJobDriver backup_job_driver = { @@ -596,7 +597,7 @@ static const BlockJobDriver backup_job_driver = { .free = block_job_free, .user_resume = block_job_user_resume, .drain = block_job_drain, - .start = backup_run, + .run = backup_run, .commit = backup_commit, .abort = backup_abort, .clean = backup_clean, diff --git a/block/commit.c b/block/commit.c index eb414579bd..a0ea86ff64 100644 --- a/block/commit.c +++ b/block/commit.c @@ -134,9 +134,9 @@ static void commit_complete(Job *job, void *opaque) bdrv_unref(top); } -static void coroutine_fn commit_run(void *opaque) +static int coroutine_fn commit_run(Job *job, Error **errp) { - CommitBlockJob *s = opaque; + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); CommitCompleteData *data; int64_t offset; uint64_t delay_ns = 0; @@ -213,6 +213,7 @@ out: data = g_malloc(sizeof(*data)); data->ret = ret; job_defer_to_main_loop(&s->common.job, commit_complete, data); + return ret; } static const BlockJobDriver commit_job_driver = { @@ -222,7 +223,7 @@ static const BlockJobDriver commit_job_driver = { .free = block_job_free, .user_resume = block_job_user_resume, .drain = block_job_drain, - .start = commit_run, + .run = commit_run, }, }; diff --git a/block/create.c b/block/create.c index 915cd41bcc..04733c3618 100644 --- a/block/create.c +++ b/block/create.c @@ -45,9 +45,9 @@ static void blockdev_create_complete(Job *job, void *opaque) job_completed(job, s->ret, s->err); } -static void coroutine_fn blockdev_create_run(void *opaque) +static int coroutine_fn blockdev_create_run(Job *job, Error **errp) { - BlockdevCreateJob *s = opaque; + BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common); job_progress_set_remaining(&s->common, 1); s->ret = s->drv->bdrv_co_create(s->opts, &s->err); @@ -55,12 +55,14 @@ static void coroutine_fn blockdev_create_run(void *opaque) qapi_free_BlockdevCreateOptions(s->opts); job_defer_to_main_loop(&s->common, blockdev_create_complete, NULL); + + return s->ret; } static const JobDriver blockdev_create_job_driver = { .instance_size = sizeof(BlockdevCreateJob), .job_type = JOB_TYPE_CREATE, - .start = blockdev_create_run, + .run = blockdev_create_run, }; void qmp_blockdev_create(const char *job_id, BlockdevCreateOptions *options, diff --git a/block/mirror.c b/block/mirror.c index 6cc10df5c9..691763db41 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -812,9 +812,9 @@ static int mirror_flush(MirrorBlockJob *s) return ret; } -static void coroutine_fn mirror_run(void *opaque) +static int coroutine_fn mirror_run(Job *job, Error **errp) { - MirrorBlockJob *s = opaque; + MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job); MirrorExitData *data; BlockDriverState *bs = s->mirror_top_bs->backing->bs; BlockDriverState *target_bs = blk_bs(s->target); @@ -1041,7 +1041,9 @@ immediate_exit: if (need_drain) { bdrv_drained_begin(bs); } + job_defer_to_main_loop(&s->common.job, mirror_exit, data); + return ret; } static void mirror_complete(Job *job, Error **errp) @@ -1138,7 +1140,7 @@ static const BlockJobDriver mirror_job_driver = { .free = block_job_free, .user_resume = block_job_user_resume, .drain = block_job_drain, - .start = mirror_run, + .run = mirror_run, .pause = mirror_pause, .complete = mirror_complete, }, @@ -1154,7 +1156,7 @@ static const BlockJobDriver commit_active_job_driver = { .free = block_job_free, .user_resume = block_job_user_resume, .drain = block_job_drain, - .start = mirror_run, + .run = mirror_run, .pause = mirror_pause, .complete = mirror_complete, }, diff --git a/block/stream.c b/block/stream.c index 9264b68a1e..b4b987df7e 100644 --- a/block/stream.c +++ b/block/stream.c @@ -97,9 +97,9 @@ out: g_free(data); } -static void coroutine_fn stream_run(void *opaque) +static int coroutine_fn stream_run(Job *job, Error **errp) { - StreamBlockJob *s = opaque; + StreamBlockJob *s = container_of(job, StreamBlockJob, common.job); StreamCompleteData *data; BlockBackend *blk = s->common.blk; BlockDriverState *bs = blk_bs(blk); @@ -206,6 +206,7 @@ out: data = g_malloc(sizeof(*data)); data->ret = ret; job_defer_to_main_loop(&s->common.job, stream_complete, data); + return ret; } static const BlockJobDriver stream_job_driver = { @@ -213,7 +214,7 @@ static const BlockJobDriver stream_job_driver = { .instance_size = sizeof(StreamBlockJob), .job_type = JOB_TYPE_STREAM, .free = block_job_free, - .start = stream_run, + .run = stream_run, .user_resume = block_job_user_resume, .drain = block_job_drain, }, diff --git a/include/qemu/job.h b/include/qemu/job.h index 18c9223e31..9cf463d228 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -169,7 +169,7 @@ struct JobDriver { JobType job_type; /** Mandatory: Entrypoint for the Coroutine. */ - CoroutineEntry *start; + int coroutine_fn (*run)(Job *job, Error **errp); /** * If the callback is not NULL, it will be invoked when the job transitions diff --git a/job.c b/job.c index e36ebaafd8..76988f6678 100644 --- a/job.c +++ b/job.c @@ -544,16 +544,16 @@ static void coroutine_fn job_co_entry(void *opaque) { Job *job = opaque; - assert(job && job->driver && job->driver->start); + assert(job && job->driver && job->driver->run); job_pause_point(job); - job->driver->start(job); + job->ret = job->driver->run(job, NULL); } void job_start(Job *job) { assert(job && !job_started(job) && job->paused && - job->driver && job->driver->start); + job->driver && job->driver->run); job->co = qemu_coroutine_create(job_co_entry, job); job->pause_count--; job->busy = true; diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index 17bb8508ae..a7533861f6 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -757,9 +757,9 @@ static void test_job_completed(Job *job, void *opaque) job_completed(job, 0, NULL); } -static void coroutine_fn test_job_start(void *opaque) +static int coroutine_fn test_job_run(Job *job, Error **errp) { - TestBlockJob *s = opaque; + TestBlockJob *s = container_of(job, TestBlockJob, common.job); job_transition_to_ready(&s->common.job); while (!s->should_complete) { @@ -771,6 +771,7 @@ static void coroutine_fn test_job_start(void *opaque) } job_defer_to_main_loop(&s->common.job, test_job_completed, NULL); + return 0; } static void test_job_complete(Job *job, Error **errp) @@ -785,7 +786,7 @@ BlockJobDriver test_job_driver = { .free = block_job_free, .user_resume = block_job_user_resume, .drain = block_job_drain, - .start = test_job_start, + .run = test_job_run, .complete = test_job_complete, }, }; diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c index 58d9b87fb2..3194924071 100644 --- a/tests/test-blockjob-txn.c +++ b/tests/test-blockjob-txn.c @@ -38,25 +38,25 @@ static void test_block_job_complete(Job *job, void *opaque) bdrv_unref(bs); } -static void coroutine_fn test_block_job_run(void *opaque) +static int coroutine_fn test_block_job_run(Job *job, Error **errp) { - TestBlockJob *s = opaque; - BlockJob *job = &s->common; + TestBlockJob *s = container_of(job, TestBlockJob, common.job); while (s->iterations--) { if (s->use_timer) { - job_sleep_ns(&job->job, 0); + job_sleep_ns(job, 0); } else { - job_yield(&job->job); + job_yield(job); } - if (job_is_cancelled(&job->job)) { + if (job_is_cancelled(job)) { break; } } - job_defer_to_main_loop(&job->job, test_block_job_complete, + job_defer_to_main_loop(job, test_block_job_complete, (void *)(intptr_t)s->rc); + return s->rc; } typedef struct { @@ -80,7 +80,7 @@ static const BlockJobDriver test_block_job_driver = { .free = block_job_free, .user_resume = block_job_user_resume, .drain = block_job_drain, - .start = test_block_job_run, + .run = test_block_job_run, }, }; diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c index cb42f06e61..b0462bfdec 100644 --- a/tests/test-blockjob.c +++ b/tests/test-blockjob.c @@ -176,9 +176,9 @@ static void cancel_job_complete(Job *job, Error **errp) s->should_complete = true; } -static void coroutine_fn cancel_job_start(void *opaque) +static int coroutine_fn cancel_job_run(Job *job, Error **errp) { - CancelJob *s = opaque; + CancelJob *s = container_of(job, CancelJob, common.job); while (!s->should_complete) { if (job_is_cancelled(&s->common.job)) { @@ -194,6 +194,7 @@ static void coroutine_fn cancel_job_start(void *opaque) defer: job_defer_to_main_loop(&s->common.job, cancel_job_completed, s); + return 0; } static const BlockJobDriver test_cancel_driver = { @@ -202,7 +203,7 @@ static const BlockJobDriver test_cancel_driver = { .free = block_job_free, .user_resume = block_job_user_resume, .drain = block_job_drain, - .start = cancel_job_start, + .run = cancel_job_run, .complete = cancel_job_complete, }, };