| Message ID | 20180817190457.8292-2-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | jobs: remove job_defer_to_main_loop | expand |
On 08/17/2018 02:04 PM, 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> > --- > +++ 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); Hmm. Here, you used the naming pair 'opaque_job' vs. 'job',... > +++ 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); while in the majority of the other clients, it was 'job' vs. 's'. Is it worth making these names consistent, or is that too much bikeshed paint? Reviewed-by: Eric Blake <eblake@redhat.com>
On 08/20/2018 02:28 PM, Eric Blake wrote: > On 08/17/2018 02:04 PM, 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> >> --- > >> +++ 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); > > Hmm. Here, you used the naming pair 'opaque_job' vs. 'job',... > >> +++ 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); > > while in the majority of the other clients, it was 'job' vs. 's'. Is it > worth making these names consistent, or is that too much bikeshed paint? > > Reviewed-by: Eric Blake <eblake@redhat.com> > :) Was just trying to keep the static down, but it did annoy me that it was different. I can either change it for "v2" or send a follow-up, depending. --js
On 2018-08-17 21:04, 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> > --- > 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/job.c b/job.c > index fa671b431a..898260b2b3 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); > } Hmmm, this breaks the iff relationship with job->error. We might call job_update_rc() afterwards, but then job_completed() would need to free it if it overwrites it with the error description from a potential error object. Also, I suspect the following patches might fix the relationship anyway? (But then an "XXX: This does not hold right now, but will be fixed in a future patch" in the documentation of Job.error might help.) Max
On 08/22/2018 06:51 AM, Max Reitz wrote: > On 2018-08-17 21:04, 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> >> --- >> 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/job.c b/job.c >> index fa671b431a..898260b2b3 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); >> } > > Hmmm, this breaks the iff relationship with job->error. We might call > job_update_rc() afterwards, but then job_completed() would need to free > it if it overwrites it with the error description from a potential error > object. > > Also, I suspect the following patches might fix the relationship anyway? > (But then an "XXX: This does not hold right now, but will be fixed in a > future patch" in the documentation of Job.error might help.) > > Max > Hmm... does it? ... I guess you mean that we are setting job->ret earlier than we used to, which gives us a window where you can have ret set, but error unset. This will get settled out by the end of the series anyway: - char *error gets replaced with Error *err, - I remove the error object from job_completed - v2 will remove the ret argument, too. --js
On 2018-08-23 01:01, John Snow wrote: > > > On 08/22/2018 06:51 AM, Max Reitz wrote: >> On 2018-08-17 21:04, 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> >>> --- >>> 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/job.c b/job.c >>> index fa671b431a..898260b2b3 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); >>> } >> >> Hmmm, this breaks the iff relationship with job->error. We might call >> job_update_rc() afterwards, but then job_completed() would need to free >> it if it overwrites it with the error description from a potential error >> object. >> >> Also, I suspect the following patches might fix the relationship anyway? >> (But then an "XXX: This does not hold right now, but will be fixed in a >> future patch" in the documentation of Job.error might help.) >> >> Max >> > > Hmm... does it? ... I guess you mean that we are setting job->ret > earlier than we used to, which gives us a window where you can have ret > set, but error unset. > > This will get settled out by the end of the series anyway: Oh no, it appears I accidentally removed yet another chunk from my reply to patch 2... > - char *error gets replaced with Error *err, Which is basically the same. I noted in the deleted chunk that patch 2 just removes the iff relationship from the describing comment, but, well... > - I remove the error object from job_completed > - v2 will remove the ret argument, too. The most important bit of the chunk I removed was that I was complaining about Job.ret still being there. I don't really see the point of this patch here at this point. Unfortunately I can't quite recall... Having a central Error object doesn't really make sense. Whenever the block job wants to return an error, it should probably do so as "return" values of methods (like .run()). Same for ret, of course. I understand that this is probably really only possible after v2 when you've made more use of abort/commit. But still, I don't think this patch improves anything, so I would leave this clean-up until later when you can really do something. I suppose the idea here is that you want to drop the errp parameter from job_completed(), because it is not going to be called by .exit(). But the obvious way around this would be to pass an errp to .exit() and then pass the result on to job_completed(). Max
On 2018-08-25 15:33, Max Reitz wrote: [...] > Having a central Error object doesn't really make sense. Whenever the > block job wants to return an error, it should probably do so as "return" > values of methods (like .run()). Same for ret, of course. > > I understand that this is probably really only possible after v2 when > you've made more use of abort/commit. But still, I don't think this > patch improves anything, so I would leave this clean-up until later when > you can really do something. > > I suppose the idea here is that you want to drop the errp parameter from > job_completed(), because it is not going to be called by .exit(). But > the obvious way around this would be to pass an errp to .exit() and then > pass the result on to job_completed(). You know what, I wrote a really long reply and in the end I realized you basically did exactly what I wanted. (Or, rather, I gave two alternatives, and you did one of them.) I noticed that having a central error object may still make sense; .create() shows why, jobs usually fail in .run() and then you need to keep the error around for a bit. You can either pass it around, or you just keep it in Job (those are the two alternatives I gave). (So I said, either rip out Job.error/Job.err and Job.ret completely, or keep the iff relationship between Job.err and Job.ret, so together they give you a meaningful state.) Now by setting Job.ret and Job.err basically at the same time (both are results of .run(), and whenever Job.ret is set somewhere else, it is immediately followed by job_update_rc()), the iff relationship between Job.err and Job.ret persists. So that's good! (In that case I don't know why you removed the "iff" part from the comment, though.) The only remaining question I have is why you still pass job->ret to job_completed(). That seems superfluous to me. Max (No, I don't know what's up with me and completely misunderstanding this series.)
On 08/25/2018 09:33 AM, Max Reitz wrote: > On 2018-08-23 01:01, John Snow wrote: >> >> >> On 08/22/2018 06:51 AM, Max Reitz wrote: >>> On 2018-08-17 21:04, 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> >>>> --- >>>> 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/job.c b/job.c >>>> index fa671b431a..898260b2b3 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); >>>> } >>> >>> Hmmm, this breaks the iff relationship with job->error. We might call >>> job_update_rc() afterwards, but then job_completed() would need to free >>> it if it overwrites it with the error description from a potential error >>> object. >>> >>> Also, I suspect the following patches might fix the relationship anyway? >>> (But then an "XXX: This does not hold right now, but will be fixed in a >>> future patch" in the documentation of Job.error might help.) >>> >>> Max >>> >> >> Hmm... does it? ... I guess you mean that we are setting job->ret >> earlier than we used to, which gives us a window where you can have ret >> set, but error unset. >> >> This will get settled out by the end of the series anyway: > > Oh no, it appears I accidentally removed yet another chunk from my reply > to patch 2... > >> - char *error gets replaced with Error *err, > > Which is basically the same. I noted in the deleted chunk that patch 2 > just removes the iff relationship from the describing comment, but, well... > >> - I remove the error object from job_completed >> - v2 will remove the ret argument, too. > > The most important bit of the chunk I removed was that I was complaining > about Job.ret still being there. I don't really see the point of this > patch here at this point. > > Unfortunately I can't quite recall... > > Having a central Error object doesn't really make sense. Whenever the > block job wants to return an error, it should probably do so as "return" > values of methods (like .run()). Same for ret, of course. > > I understand that this is probably really only possible after v2 when > you've made more use of abort/commit. But still, I don't think this > patch improves anything, so I would leave this clean-up until later when > you can really do something. > > I suppose the idea here is that you want to drop the errp parameter from > job_completed(), because it is not going to be called by .exit(). But > the obvious way around this would be to pass an errp to .exit() and then > pass the result on to job_completed(). > > Max > That might be a good idea, but I need to look at the pathway for actually showing that error to the user, since we need to pass it down a few times anyway. It's certainly simpler to just stash it in the object. I'll take a look.
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 fa671b431a..898260b2b3 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, }, };
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> --- 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(-)