[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/9] jobs: change start callback to run callb
|
From: |
Jeff Cody |
|
Subject: |
Re: [Qemu-devel] [PATCH v3 1/9] jobs: change start callback to run callback |
|
Date: |
Fri, 31 Aug 2018 09:27:09 -0400 |
|
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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 <address@hidden>
> Reviewed-by: Max Reitz <address@hidden>
Reviewed-by: Jeff Cody <address@hidden>
> ---
> 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
>