The following changes since commit 19b599f7664b2ebfd0f405fb79c14dd241557452:

Merge remote-tracking branch 'remotes/armbru/tags/pull-error-2018-08-27-v2' into staging (2018-08-27 16:44:20 +0100)

https://git.xanclic.moe/XanClic/qemu.git tags/pull-block-2018-08-31-v2

for you to fetch changes up to e21a1c9831fc80ae3f3c1affdfa43350035d8588:

jobs: remove job_defer_to_main_loop (2018-08-31 16:28:33 +0200)

Block patches:

- (Block) job exit refactoring, part 1

(removing job_defer_to_main_loop())

- test-bdrv-drain leak fix

Marc-André Lureau (1):

tests: fix bdrv-drain leak

include/qemu/job.h | 70 ++++++++++++++++-----------------

block/backup.c | 81 ++++++++++++++++-----------------------

block/commit.c | 29 +++++---------

block/create.c | 19 +++------

block/mirror.c | 39 ++++++++-----------

block/stream.c | 29 ++++++--------

job-qmp.c | 5 ++-

job.c | 73 ++++++++++++-----------------------

tests/test-bdrv-drain.c | 14 +++----

tests/test-blockjob-txn.c | 25 +++++-------

tests/test-blockjob.c | 17 ++++----

trace-events | 2 +-

12 files changed, 161 insertions(+), 242 deletions(-)

2.17.1

From: John Snow <jsnow@redhat.com>

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>

Message-id: 20180830015734.19765-2-jsnow@redhat.com

Reviewed-by: Jeff Cody <jcody@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

include/qemu/job.h | 2 +-

block/backup.c | 7 ++++---

block/commit.c | 7 ++++---

block/create.c | 8 +++++---

block/mirror.c | 10 ++++++----

block/stream.c | 7 ++++---

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/include/qemu/job.h b/include/qemu/job.h

index XXXXXXX..XXXXXXX 100644

--- a/include/qemu/job.h

+++ b/include/qemu/job.h

@@ -XXX,XX +XXX,XX @@ 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/block/backup.c b/block/backup.c

index XXXXXXX..XXXXXXX 100644

--- a/block/backup.c

+++ b/block/backup.c

@@ -XXX,XX +XXX,XX @@ 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;

@@ -XXX,XX +XXX,XX @@ 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 = {

@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644

--- a/block/commit.c

+++ b/block/commit.c

@@ -XXX,XX +XXX,XX @@ 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;

@@ -XXX,XX +XXX,XX @@ 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 = {

@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644

--- a/block/create.c

+++ b/block/create.c

@@ -XXX,XX +XXX,XX @@ 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);

@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644

--- a/block/mirror.c

+++ b/block/mirror.c

@@ -XXX,XX +XXX,XX @@ 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);

@@ -XXX,XX +XXX,XX @@ 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)

@@ -XXX,XX +XXX,XX @@ 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,

},

@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644

--- a/block/stream.c

+++ b/block/stream.c

@@ -XXX,XX +XXX,XX @@ 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);

@@ -XXX,XX +XXX,XX @@ 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 = {

@@ -XXX,XX +XXX,XX @@ 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/job.c b/job.c

index XXXXXXX..XXXXXXX 100644

--- a/job.c

+++ b/job.c

@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644

--- a/tests/test-bdrv-drain.c

+++ b/tests/test-bdrv-drain.c

@@ -XXX,XX +XXX,XX @@ 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) {

@@ -XXX,XX +XXX,XX @@ 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)

@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644

--- a/tests/test-blockjob-txn.c

+++ b/tests/test-blockjob-txn.c

@@ -XXX,XX +XXX,XX @@ 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 {

@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644

--- a/tests/test-blockjob.c

+++ b/tests/test-blockjob.c

@@ -XXX,XX +XXX,XX @@ 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)) {

@@ -XXX,XX +XXX,XX @@ 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 = {

@@ -XXX,XX +XXX,XX @@ 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.17.1

From: John Snow <jsnow@redhat.com>

Jobs presently use both an Error object in the case of the create job,

and char strings in the case of generic errors elsewhere.

Unify the two paths as just j->err, and remove the extra argument from

job_completed. The integer error code for job_completed is kept for now,

to be removed shortly in a separate patch.

Signed-off-by: John Snow <jsnow@redhat.com>

Message-id: 20180830015734.19765-3-jsnow@redhat.com

[mreitz: Dropped a superfluous g_strdup()]

Reviewed-by: Eric Blake <eblake@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

include/qemu/job.h | 14 ++++++++------

block/backup.c | 2 +-

block/commit.c | 2 +-

block/create.c | 5 ++---

block/mirror.c | 2 +-

block/stream.c | 2 +-

job-qmp.c | 5 +++--

job.c | 18 ++++++------------

tests/test-bdrv-drain.c | 2 +-

tests/test-blockjob-txn.c | 2 +-

tests/test-blockjob.c | 2 +-

11 files changed, 26 insertions(+), 30 deletions(-)

diff --git a/include/qemu/job.h b/include/qemu/job.h

index XXXXXXX..XXXXXXX 100644

--- a/include/qemu/job.h

+++ b/include/qemu/job.h

@@ -XXX,XX +XXX,XX @@ typedef struct Job {

/** Estimated progress_current value at the completion of the job */

int64_t progress_total;

- /** Error string for a failed job (NULL if, and only if, job->ret == 0) */

- char *error;

-

/** ret code passed to job_completed. */

int ret;

+ /**

+ * Error object for a failed job.

+ * If job->ret is nonzero and an error object was not set, it will be set

+ * to strerror(-job->ret) during job_completed.

+ */

+ Error *err;

+

/** The completion function that will be called when the job completes. */

BlockCompletionFunc *cb;

@@ -XXX,XX +XXX,XX @@ void job_transition_to_ready(Job *job);

/**

* @job: The job being completed.

* @ret: The status code.

- * @error: The error message for a failing job (only with @ret < 0). If @ret is

- * negative, but NULL is given for @error, strerror() is used.

*

* Marks @job as completed. If @ret is non-zero, the job transaction it is part

* of is aborted. If @ret is zero, the job moves into the WAITING state. If it

* is the last job to complete in its transaction, all jobs in the transaction

* move from WAITING to PENDING.

*/

-void job_completed(Job *job, int ret, Error *error);

+void job_completed(Job *job, int ret);

/** Asynchronously complete the specified @job. */

void job_complete(Job *job, Error **errp);

diff --git a/block/backup.c b/block/backup.c

index XXXXXXX..XXXXXXX 100644

--- a/block/backup.c

+++ b/block/backup.c

@@ -XXX,XX +XXX,XX @@ static void backup_complete(Job *job, void *opaque)

{

BackupCompleteData *data = opaque;

- job_completed(job, data->ret, NULL);

+ job_completed(job, data->ret);

g_free(data);

}

diff --git a/block/commit.c b/block/commit.c

index XXXXXXX..XXXXXXX 100644

--- a/block/commit.c

+++ b/block/commit.c

@@ -XXX,XX +XXX,XX @@ static void commit_complete(Job *job, void *opaque)

* bdrv_set_backing_hd() to fail. */

block_job_remove_all_bdrv(bjob);

- job_completed(job, ret, NULL);

+ job_completed(job, ret);

g_free(data);

/* If bdrv_drop_intermediate() didn't already do that, remove the commit

diff --git a/block/create.c b/block/create.c

index XXXXXXX..XXXXXXX 100644

--- a/block/create.c

+++ b/block/create.c

@@ -XXX,XX +XXX,XX @@ typedef struct BlockdevCreateJob {

BlockDriver *drv;

BlockdevCreateOptions *opts;

int ret;

- Error *err;

} BlockdevCreateJob;

static void blockdev_create_complete(Job *job, void *opaque)

{

BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);

- job_completed(job, s->ret, s->err);

+ job_completed(job, s->ret);

}

static int coroutine_fn blockdev_create_run(Job *job, Error **errp)

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn blockdev_create_run(Job *job, Error **errp)

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);

+ s->ret = s->drv->bdrv_co_create(s->opts, errp);

job_progress_update(&s->common, 1);

qapi_free_BlockdevCreateOptions(s->opts);

diff --git a/block/mirror.c b/block/mirror.c

index XXXXXXX..XXXXXXX 100644

--- a/block/mirror.c

+++ b/block/mirror.c

@@ -XXX,XX +XXX,XX @@ static void mirror_exit(Job *job, void *opaque)

blk_insert_bs(bjob->blk, mirror_top_bs, &error_abort);

bs_opaque->job = NULL;

- job_completed(job, data->ret, NULL);

+ job_completed(job, data->ret);

g_free(data);

bdrv_drained_end(src);

diff --git a/block/stream.c b/block/stream.c

index XXXXXXX..XXXXXXX 100644

--- a/block/stream.c

+++ b/block/stream.c

@@ -XXX,XX +XXX,XX @@ out:

}

g_free(s->backing_file_str);

- job_completed(job, data->ret, NULL);

+ job_completed(job, data->ret);

g_free(data);

}

diff --git a/job-qmp.c b/job-qmp.c

index XXXXXXX..XXXXXXX 100644

--- a/job-qmp.c

+++ b/job-qmp.c

@@ -XXX,XX +XXX,XX @@ static JobInfo *job_query_single(Job *job, Error **errp)

.status = job->status,

.current_progress = job->progress_current,

.total_progress = job->progress_total,

- .has_error = !!job->error,

- .error = g_strdup(job->error),

+ .has_error = !!job->err,

+ .error = job->err ? \

+ g_strdup(error_get_pretty(job->err)) : NULL,

};

return info;

diff --git a/job.c b/job.c

index XXXXXXX..XXXXXXX 100644

--- a/job.c

+++ b/job.c

@@ -XXX,XX +XXX,XX @@ void job_unref(Job *job)

QLIST_REMOVE(job, job_list);

- g_free(job->error);

+ error_free(job->err);

g_free(job->id);

g_free(job);

}

@@ -XXX,XX +XXX,XX @@ static void coroutine_fn job_co_entry(void *opaque)

assert(job && job->driver && job->driver->run);

job_pause_point(job);

- job->ret = job->driver->run(job, NULL);

+ job->ret = job->driver->run(job, &job->err);

}

@@ -XXX,XX +XXX,XX @@ static void job_update_rc(Job *job)

job->ret = -ECANCELED;

}

if (job->ret) {

- if (!job->error) {

- job->error = g_strdup(strerror(-job->ret));

+ if (!job->err) {

+ error_setg(&job->err, "%s", strerror(-job->ret));

}

job_state_transition(job, JOB_STATUS_ABORTING);

}

@@ -XXX,XX +XXX,XX @@ static void job_completed_txn_success(Job *job)

}

}

-void job_completed(Job *job, int ret, Error *error)

+void job_completed(Job *job, int ret)

{

assert(job && job->txn && !job_is_completed(job));

job->ret = ret;

- if (error) {

- assert(job->ret < 0);

- job->error = g_strdup(error_get_pretty(error));

- error_free(error);

- }

-

job_update_rc(job);

trace_job_completed(job, ret, job->ret);

if (job->ret) {

@@ -XXX,XX +XXX,XX @@ void job_cancel(Job *job, bool force)

}

job_cancel_async(job, force);

if (!job_started(job)) {

- job_completed(job, -ECANCELED, NULL);

+ job_completed(job, -ECANCELED);

} else if (job->deferred_to_main_loop) {

job_completed_txn_abort(job);

} else {

diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c

index XXXXXXX..XXXXXXX 100644

--- a/tests/test-bdrv-drain.c

+++ b/tests/test-bdrv-drain.c

@@ -XXX,XX +XXX,XX @@ typedef struct TestBlockJob {

static void test_job_completed(Job *job, void *opaque)

{

- job_completed(job, 0, NULL);

+ job_completed(job, 0);

}

static int coroutine_fn test_job_run(Job *job, Error **errp)

diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c

index XXXXXXX..XXXXXXX 100644

--- a/tests/test-blockjob-txn.c

+++ b/tests/test-blockjob-txn.c

@@ -XXX,XX +XXX,XX @@ static void test_block_job_complete(Job *job, void *opaque)

rc = -ECANCELED;

}

- job_completed(job, rc, NULL);

+ job_completed(job, rc);

bdrv_unref(bs);

}

diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c

index XXXXXXX..XXXXXXX 100644

--- a/tests/test-blockjob.c

+++ b/tests/test-blockjob.c

@@ -XXX,XX +XXX,XX @@ static void cancel_job_completed(Job *job, void *opaque)

{

CancelJob *s = opaque;

s->completed = true;

- job_completed(job, 0, NULL);

+ job_completed(job, 0);

}

static void cancel_job_complete(Job *job, Error **errp)

--

2.17.1

From: John Snow <jsnow@redhat.com>

Change the manual deferment to commit_complete into the implicit

callback to job_exit, renaming commit_complete to commit_exit.

This conversion does change the timing of when job_completed is

called to after the bdrv_replace_node and bdrv_unref calls, which

could have implications for bjob->blk which will now be put down

after this cleanup.

Kevin highlights that we did not take any permissions for that backend

at job creation time, so it is safe to reorder these operations.

Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Max Reitz <mreitz@redhat.com>

Message-id: 20180830015734.19765-5-jsnow@redhat.com

Reviewed-by: Jeff Cody <jcody@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>

block/commit.c | 22 +++++-----------------

1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/block/commit.c b/block/commit.c

--- a/block/commit.c

+++ b/block/commit.c

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,

return 0;

}

-typedef struct {

- int ret;

-} CommitCompleteData;

-

-static void commit_complete(Job *job, void *opaque)

+static void commit_exit(Job *job)

{

CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);

BlockJob *bjob = &s->common;

- CommitCompleteData *data = opaque;

BlockDriverState *top = blk_bs(s->top);

BlockDriverState *base = blk_bs(s->base);

BlockDriverState *commit_top_bs = s->commit_top_bs;

- int ret = data->ret;

bool remove_commit_top_bs = false;

/* Make sure commit_top_bs and top stay around until bdrv_replace_node() */

@@ -XXX,XX +XXX,XX @@ static void commit_complete(Job *job, void *opaque)

* the normal backing chain can be restored. */

blk_unref(s->base);

- if (!job_is_cancelled(job) && ret == 0) {

+ if (!job_is_cancelled(job) && job->ret == 0) {

/* success */

- ret = bdrv_drop_intermediate(s->commit_top_bs, base,

- s->backing_file_str);

+ job->ret = bdrv_drop_intermediate(s->commit_top_bs, base,

+ s->backing_file_str);

} else {

/* XXX Can (or should) we somehow keep 'consistent read' blocked even

* after the failed/cancelled commit job is gone? If we already wrote

@@ -XXX,XX +XXX,XX @@ static void commit_complete(Job *job, void *opaque)

* bdrv_set_backing_hd() to fail. */

block_job_remove_all_bdrv(bjob);

- job_completed(job, ret);

- g_free(data);

-

/* If bdrv_drop_intermediate() didn't already do that, remove the commit

* filter driver from the backing chain. Do this as the final step so that

* the 'consistent read' permission can be granted. */

@@ -XXX,XX +XXX,XX @@ static void commit_complete(Job *job, void *opaque)

static int coroutine_fn commit_run(Job *job, Error **errp)

{

CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);

- CommitCompleteData *data;

int64_t offset;

uint64_t delay_ns = 0;

int ret = 0;

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn commit_run(Job *job, Error **errp)

out:

qemu_vfree(buf);

- data = g_malloc(sizeof(*data));

- data->ret = ret;

- job_defer_to_main_loop(&s->common.job, commit_complete, data);

return ret;

}

@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver commit_job_driver = {

.user_resume = block_job_user_resume,

.drain = block_job_drain,

.run = commit_run,

+ .exit = commit_exit,

},

};

2.17.1

From: John Snow <jsnow@redhat.com>

Utilize the job_exit shim by not calling job_defer_to_main_loop, and

where applicable, converting the deferred callback into the job_exit

callback.

This converts backup, stream, create, and the unit tests all at once.

Most of these jobs do not see any changes to the order in which they

clean up their resources, except the test-blockjob-txn test, which

now puts down its bs before job_completed is called.

This is safe for the same reason the reordering in the mirror job is

safe, because job_completed no longer runs under two locks, making

the unref safe even if it causes a flush.

Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Max Reitz <mreitz@redhat.com>

Message-id: 20180830015734.19765-7-jsnow@redhat.com

Signed-off-by: Max Reitz <mreitz@redhat.com>

---

block/backup.c | 16 ----------------

block/create.c | 14 +++-----------

block/stream.c | 22 +++++++---------------

tests/test-bdrv-drain.c | 6 ------

tests/test-blockjob-txn.c | 11 ++---------

tests/test-blockjob.c | 10 ++++------

6 files changed, 16 insertions(+), 63 deletions(-)

diff --git a/block/backup.c b/block/backup.c

index XXXXXXX..XXXXXXX 100644

--- a/block/backup.c

+++ b/block/backup.c

@@ -XXX,XX +XXX,XX @@ static BlockErrorAction backup_error_action(BackupBlockJob *job,

}

}

-typedef struct {

- int ret;

-} BackupCompleteData;

-

-static void backup_complete(Job *job, void *opaque)

-{

- BackupCompleteData *data = opaque;

-

- job_completed(job, data->ret);

- g_free(data);

-}

-

static bool coroutine_fn yield_and_check(BackupBlockJob *job)

{

uint64_t delay_ns;

@@ -XXX,XX +XXX,XX @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)

static int coroutine_fn backup_run(Job *opaque_job, Error **errp)

{

BackupBlockJob *job = container_of(opaque_job, BackupBlockJob, common.job);

- BackupCompleteData *data;

BlockDriverState *bs = blk_bs(job->common.blk);

int64_t offset, nb_clusters;

int ret = 0;

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn backup_run(Job *opaque_job, Error **errp)

qemu_co_rwlock_unlock(&job->flush_rwlock);

hbitmap_free(job->copy_bitmap);

- data = g_malloc(sizeof(*data));

- data->ret = ret;

- job_defer_to_main_loop(&job->common.job, backup_complete, data);

return ret;

}

diff --git a/block/create.c b/block/create.c

index XXXXXXX..XXXXXXX 100644

--- a/block/create.c

+++ b/block/create.c

@@ -XXX,XX +XXX,XX @@ typedef struct BlockdevCreateJob {

Job common;

BlockDriver *drv;

BlockdevCreateOptions *opts;

- int ret;

} BlockdevCreateJob;

-static void blockdev_create_complete(Job *job, void *opaque)

-{

- BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);

-

- job_completed(job, s->ret);

-}

-

static int coroutine_fn blockdev_create_run(Job *job, Error **errp)

{

BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);

+ int ret;

job_progress_set_remaining(&s->common, 1);

- s->ret = s->drv->bdrv_co_create(s->opts, errp);

+ ret = s->drv->bdrv_co_create(s->opts, errp);

job_progress_update(&s->common, 1);

qapi_free_BlockdevCreateOptions(s->opts);

- job_defer_to_main_loop(&s->common, blockdev_create_complete, NULL);

- return s->ret;

+ return ret;

}

static const JobDriver blockdev_create_job_driver = {

diff --git a/block/stream.c b/block/stream.c

index XXXXXXX..XXXXXXX 100644

--- a/block/stream.c

+++ b/block/stream.c

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn stream_populate(BlockBackend *blk,

return blk_co_preadv(blk, offset, qiov.size, &qiov, BDRV_REQ_COPY_ON_READ);

}

-typedef struct {

- int ret;

-} StreamCompleteData;

-

-static void stream_complete(Job *job, void *opaque)

+static void stream_exit(Job *job)

{

StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);

BlockJob *bjob = &s->common;

- StreamCompleteData *data = opaque;

BlockDriverState *bs = blk_bs(bjob->blk);

BlockDriverState *base = s->base;

Error *local_err = NULL;

+ int ret = job->ret;

- if (!job_is_cancelled(job) && bs->backing && data->ret == 0) {

+ if (!job_is_cancelled(job) && bs->backing && ret == 0) {

const char *base_id = NULL, *base_fmt = NULL;

if (base) {

base_id = s->backing_file_str;

@@ -XXX,XX +XXX,XX @@ static void stream_complete(Job *job, void *opaque)

base_fmt = base->drv->format_name;

}

}

- data->ret = bdrv_change_backing_file(bs, base_id, base_fmt);

+ ret = bdrv_change_backing_file(bs, base_id, base_fmt);

bdrv_set_backing_hd(bs, base, &local_err);

if (local_err) {

error_report_err(local_err);

- data->ret = -EPERM;

+ ret = -EPERM;

goto out;

}

}

@@ -XXX,XX +XXX,XX @@ out:

}

g_free(s->backing_file_str);

- job_completed(job, data->ret);

- g_free(data);

+ job->ret = ret;

}

static int coroutine_fn stream_run(Job *job, Error **errp)

{

StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);

- StreamCompleteData *data;

BlockBackend *blk = s->common.blk;

BlockDriverState *bs = blk_bs(blk);

BlockDriverState *base = s->base;

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn stream_run(Job *job, Error **errp)

out:

/* Modify backing chain and close BDSes in main loop */

- data = g_malloc(sizeof(*data));

- data->ret = ret;

- job_defer_to_main_loop(&s->common.job, stream_complete, data);

return ret;

}

@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver stream_job_driver = {

.job_type = JOB_TYPE_STREAM,

.free = block_job_free,

.run = stream_run,

+ .exit = stream_exit,

.user_resume = block_job_user_resume,

.drain = block_job_drain,

},

diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c

index XXXXXXX..XXXXXXX 100644

--- a/tests/test-bdrv-drain.c

+++ b/tests/test-bdrv-drain.c

@@ -XXX,XX +XXX,XX @@ typedef struct TestBlockJob {

bool should_complete;

} TestBlockJob;

-static void test_job_completed(Job *job, void *opaque)

-{

- job_completed(job, 0);

-}

-

static int coroutine_fn test_job_run(Job *job, Error **errp)

{

TestBlockJob *s = container_of(job, TestBlockJob, common.job);

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn test_job_run(Job *job, Error **errp)

job_pause_point(&s->common.job);

}

- job_defer_to_main_loop(&s->common.job, test_job_completed, NULL);

return 0;

}

diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c

index XXXXXXX..XXXXXXX 100644

--- a/tests/test-blockjob-txn.c

+++ b/tests/test-blockjob-txn.c

@@ -XXX,XX +XXX,XX @@ typedef struct {

int *result;

} TestBlockJob;

-static void test_block_job_complete(Job *job, void *opaque)

+static void test_block_job_exit(Job *job)

{

BlockJob *bjob = container_of(job, BlockJob, job);

BlockDriverState *bs = blk_bs(bjob->blk);

- int rc = (intptr_t)opaque;

- if (job_is_cancelled(job)) {

- rc = -ECANCELED;

- }

-

- job_completed(job, rc);

bdrv_unref(bs);

}

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn test_block_job_run(Job *job, Error **errp)

}

}

- job_defer_to_main_loop(job, test_block_job_complete,

- (void *)(intptr_t)s->rc);

return s->rc;

}

@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver test_block_job_driver = {

.user_resume = block_job_user_resume,

.drain = block_job_drain,

.run = test_block_job_run,

+ .exit = test_block_job_exit,

},

};