From a04691e941048c14853d40cbb2a174e4e9b473a5 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 26 Jun 2018 09:48:39 +0200 Subject: [PATCH 131/268] job: Add error message for failing jobs RH-Author: Kevin Wolf Message-id: <20180626094856.6924-57-kwolf@redhat.com> Patchwork-id: 81110 O-Subject: [RHV-7.6 qemu-kvm-rhev PATCH v2 56/73] job: Add error message for failing jobs Bugzilla: 1513543 RH-Acked-by: Jeffrey Cody RH-Acked-by: Max Reitz RH-Acked-by: Fam Zheng So far we relied on job->ret and strerror() to produce an error message for failed jobs. Not surprisingly, this tends to result in completely useless messages. This adds a Job.error field that can contain an error string for a failing job, and a parameter to job_completed() that sets the field. As a default, if NULL is passed, we continue to use strerror(job->ret). All existing callers are changed to pass NULL. They can be improved in separate patches. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz Reviewed-by: Jeff Cody (cherry picked from commit 1266c9b9f5fa05877b979eece5963a2bd99c3bfd) Signed-off-by: Kevin Wolf Signed-off-by: Miroslav Rezanina --- block/backup.c | 2 +- block/commit.c | 2 +- block/mirror.c | 2 +- block/stream.c | 2 +- include/qemu/job.h | 7 ++++++- job-qmp.c | 9 ++------- job.c | 16 ++++++++++++++-- tests/test-bdrv-drain.c | 2 +- tests/test-blockjob-txn.c | 2 +- tests/test-blockjob.c | 2 +- 10 files changed, 29 insertions(+), 17 deletions(-) diff --git a/block/backup.c b/block/backup.c index 4e228e9..5661435 100644 --- a/block/backup.c +++ b/block/backup.c @@ -321,7 +321,7 @@ static void backup_complete(Job *job, void *opaque) { BackupCompleteData *data = opaque; - job_completed(job, data->ret); + job_completed(job, data->ret, NULL); g_free(data); } diff --git a/block/commit.c b/block/commit.c index 6206661..e1814d9 100644 --- a/block/commit.c +++ b/block/commit.c @@ -117,7 +117,7 @@ static void commit_complete(Job *job, void *opaque) * bdrv_set_backing_hd() to fail. */ block_job_remove_all_bdrv(bjob); - job_completed(job, ret); + job_completed(job, ret, NULL); g_free(data); /* If bdrv_drop_intermediate() didn't already do that, remove the commit diff --git a/block/mirror.c b/block/mirror.c index dcb66ec..435268b 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -581,7 +581,7 @@ static void mirror_exit(Job *job, void *opaque) blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort); blk_insert_bs(bjob->blk, mirror_top_bs, &error_abort); - job_completed(job, data->ret); + job_completed(job, data->ret, NULL); g_free(data); bdrv_drained_end(src); diff --git a/block/stream.c b/block/stream.c index a5d6e0c..9264b68 100644 --- a/block/stream.c +++ b/block/stream.c @@ -93,7 +93,7 @@ out: } g_free(s->backing_file_str); - job_completed(job, data->ret); + job_completed(job, data->ret, NULL); g_free(data); } diff --git a/include/qemu/job.h b/include/qemu/job.h index 8c8badf..1d82053 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -124,6 +124,9 @@ 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; @@ -466,13 +469,15 @@ 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); +void job_completed(Job *job, int ret, Error *error); /** Asynchronously complete the specified @job. */ void job_complete(Job *job, Error **errp); diff --git a/job-qmp.c b/job-qmp.c index 7f38f63..410775d 100644 --- a/job-qmp.c +++ b/job-qmp.c @@ -136,14 +136,9 @@ void qmp_job_dismiss(const char *id, Error **errp) static JobInfo *job_query_single(Job *job, Error **errp) { JobInfo *info; - const char *errmsg = NULL; assert(!job_is_internal(job)); - if (job->ret < 0) { - errmsg = strerror(-job->ret); - } - info = g_new(JobInfo, 1); *info = (JobInfo) { .id = g_strdup(job->id), @@ -151,8 +146,8 @@ static JobInfo *job_query_single(Job *job, Error **errp) .status = job->status, .current_progress = job->progress_current, .total_progress = job->progress_total, - .has_error = !!errmsg, - .error = g_strdup(errmsg), + .has_error = !!job->error, + .error = g_strdup(job->error), }; return info; diff --git a/job.c b/job.c index f026661..84e1402 100644 --- a/job.c +++ b/job.c @@ -369,6 +369,7 @@ void job_unref(Job *job) QLIST_REMOVE(job, job_list); + g_free(job->error); g_free(job->id); g_free(job); } @@ -660,6 +661,9 @@ static void job_update_rc(Job *job) job->ret = -ECANCELED; } if (job->ret) { + if (!job->error) { + job->error = g_strdup(strerror(-job->ret)); + } job_state_transition(job, JOB_STATUS_ABORTING); } } @@ -782,6 +786,7 @@ static int job_prepare(Job *job) { if (job->ret == 0 && job->driver->prepare) { job->ret = job->driver->prepare(job); + job_update_rc(job); } return job->ret; } @@ -855,10 +860,17 @@ static void job_completed_txn_success(Job *job) } } -void job_completed(Job *job, int ret) +void job_completed(Job *job, int ret, Error *error) { 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) { @@ -876,7 +888,7 @@ void job_cancel(Job *job, bool force) } job_cancel_async(job, force); if (!job_started(job)) { - job_completed(job, -ECANCELED); + job_completed(job, -ECANCELED, NULL); } 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 2cba63b..a11c4cf 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -498,7 +498,7 @@ typedef struct TestBlockJob { static void test_job_completed(Job *job, void *opaque) { - job_completed(job, 0); + job_completed(job, 0, NULL); } static void coroutine_fn test_job_start(void *opaque) diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c index fce8366..58d9b87 100644 --- a/tests/test-blockjob-txn.c +++ b/tests/test-blockjob-txn.c @@ -34,7 +34,7 @@ static void test_block_job_complete(Job *job, void *opaque) rc = -ECANCELED; } - job_completed(job, rc); + job_completed(job, rc, NULL); bdrv_unref(bs); } diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c index e408d52..cb42f06 100644 --- a/tests/test-blockjob.c +++ b/tests/test-blockjob.c @@ -167,7 +167,7 @@ static void cancel_job_completed(Job *job, void *opaque) { CancelJob *s = opaque; s->completed = true; - job_completed(job, 0); + job_completed(job, 0, NULL); } static void cancel_job_complete(Job *job, Error **errp) -- 1.8.3.1