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

for you to fetch changes up to 40954cc7831c4f95f9ce6402ae3d6761f44f31ff:

jobs: remove job_defer_to_main_loop (2018-08-31 16:11:27 +0200)

Block patches:

- (Block) job exit refactoring, part 1

(removing job_defer_to_main_loop())

- Locking fix for the file-posix block driver

- test-bdrv-drain leak fix

John Snow (9):

jobs: change start callback to run callback

jobs: canonize Error object

jobs: add exit shim

block/commit: utilize job_exit shim

block/mirror: utilize job_exit shim

jobs: utilize job_exit shim

block/backup: make function variables consistently named

jobs: remove ret argument to job_completed; privatize it

jobs: remove job_defer_to_main_loop

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/file-posix.c | 41 +++++++++++++++-----

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 +-

13 files changed, 192 insertions(+), 252 deletions(-)

2.17.1

From: John Snow <jsnow@redhat.com>

Now that the job infrastructure is handling the job_completed call for

all implemented jobs, we can remove the interface that allowed jobs to

schedule their own completion.

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

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

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

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

include/qemu/job.h | 17 -----------------

job.c | 40 ++--------------------------------------

2 files changed, 2 insertions(+), 55 deletions(-)

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

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

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

@@ -XXX,XX +XXX,XX @@ void job_finalize(Job *job, Error **errp);

*/

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

-typedef void JobDeferToMainLoopFn(Job *job, void *opaque);

-

-/**

- * @job: The job

- * @fn: The function to run in the main loop

- * @opaque: The opaque value that is passed to @fn

- *

- * This function must be called by the main job coroutine just before it

- * returns. @fn is executed in the main loop with the job AioContext acquired.

- *

- * Block jobs must call bdrv_unref(), bdrv_close(), and anything that uses

- * bdrv_drain_all() in the main loop.

- *

- * The @job AioContext is held while @fn executes.

- */

-void job_defer_to_main_loop(Job *job, JobDeferToMainLoopFn *fn, void *opaque);

-

/**

* Synchronously finishes the given @job. If @finish is given, it is called to

* trigger completion or cancellation of the job.

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)

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

job_pause_point(job);

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

- if (!job->deferred_to_main_loop) {

- job->deferred_to_main_loop = true;

- aio_bh_schedule_oneshot(qemu_get_aio_context(),

- job_exit,

- job);

- }

+ job->deferred_to_main_loop = true;

+ aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job);

}

@@ -XXX,XX +XXX,XX @@ void job_complete(Job *job, Error **errp)

job->driver->complete(job, errp);

}

-

-typedef struct {

- Job *job;

- JobDeferToMainLoopFn *fn;

- void *opaque;

-} JobDeferToMainLoopData;

-

-static void job_defer_to_main_loop_bh(void *opaque)

-{

- JobDeferToMainLoopData *data = opaque;

- Job *job = data->job;

- AioContext *aio_context = job->aio_context;

-

- aio_context_acquire(aio_context);

- data->fn(data->job, data->opaque);

- aio_context_release(aio_context);

-

- g_free(data);

-}

-

-void job_defer_to_main_loop(Job *job, JobDeferToMainLoopFn *fn, void *opaque)

-{

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

- data->job = job;

- data->fn = fn;

- data->opaque = opaque;

- job->deferred_to_main_loop = true;

-

- aio_bh_schedule_oneshot(qemu_get_aio_context(),

- job_defer_to_main_loop_bh, data);

-}

-

int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp)

{

Error *local_err = NULL;

2.17.1

From: John Snow <jsnow@redhat.com>

Jobs are now expected to return their retcode on the stack, from the

.run callback, so we can remove that argument.

job_cancel does not need to set -ECANCELED because job_completed will

update the return code itself if the job was canceled.

While we're here, make job_completed static to job.c and remove it from

job.h; move the documentation of return code to the .run() callback and

to the job->ret property, accordingly.

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

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

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

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

---

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

job.c | 11 ++++++-----

trace-events | 2 +-

3 files changed, 22 insertions(+), 19 deletions(-)

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

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

- /** ret code passed to job_completed. */

+ /**

+ * Return code from @run and/or @prepare callback(s).

+ * Not final until the job has reached the CONCLUDED status.

+ * 0 on success, -errno on failure.

+ */

int ret;

/**

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

/** Enum describing the operation */

JobType job_type;

- /** Mandatory: Entrypoint for the Coroutine. */

+ /**

+ * Mandatory: Entrypoint for the Coroutine.

+ *

+ * This callback will be invoked when moving from CREATED to RUNNING.

+ *

+ * If this callback returns nonzero, the job transaction it is part of is

+ * aborted. If it returns 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.

+ */

int coroutine_fn (*run)(Job *job, Error **errp);

/**

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

/** Moves the @job from RUNNING to READY */

void job_transition_to_ready(Job *job);

-/**

- * @job: The job being completed.

- * @ret: The status code.

- *

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

-

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

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

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

index XXXXXXX..XXXXXXX 100644

--- a/job.c

+++ b/job.c

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

+static void job_completed(Job *job);

+

static void job_exit(void *opaque)

{

Job *job = (Job *)opaque;

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

job->driver->exit(job);

aio_context_release(aio_context);

}

- job_completed(job, job->ret);

+ job_completed(job);

}

/**

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

}

}

-void job_completed(Job *job, int ret)

+static void job_completed(Job *job)

{

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

- job->ret = ret;

job_update_rc(job);

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

+ trace_job_completed(job, job->ret);

if (job->ret) {

job_completed_txn_abort(job);

} else {

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

}

job_cancel_async(job, force);

if (!job_started(job)) {

- job_completed(job, -ECANCELED);

+ job_completed(job);

} else if (job->deferred_to_main_loop) {

job_completed_txn_abort(job);

} else {

diff --git a/trace-events b/trace-events

index XXXXXXX..XXXXXXX 100644

--- a/trace-events

+++ b/trace-events

@@ -XXX,XX +XXX,XX @@ gdbstub_err_checksum_incorrect(uint8_t expected, uint8_t got) "got command packe

# job.c

job_state_transition(void *job, int ret, const char *legal, const char *s0, const char *s1) "job %p (ret: %d) attempting %s transition (%s-->%s)"

job_apply_verb(void *job, const char *state, const char *verb, const char *legal) "job %p in state %s; applying verb %s (%s)"

-job_completed(void *job, int ret, int jret) "job %p ret %d corrected ret %d"

+job_completed(void *job, int ret) "job %p ret %d"

# job-qmp.c

qmp_job_cancel(void *job) "job %p"

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

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

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