From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40503) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fNfr3-0000e5-QL for qemu-devel@nongnu.org; Tue, 29 May 2018 10:44:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fNfqz-0003iu-Ds for qemu-devel@nongnu.org; Tue, 29 May 2018 10:44:05 -0400 Date: Tue, 29 May 2018 10:43:53 -0400 From: Jeff Cody Message-ID: <20180529144353.GF6999@localhost.localdomain> References: <20180525163327.23097-1-kwolf@redhat.com> <20180525163327.23097-4-kwolf@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180525163327.23097-4-kwolf@redhat.com> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 03/14] job: Add error message for failing jobs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, mreitz@redhat.com On Fri, May 25, 2018 at 06:33:16PM +0200, Kevin Wolf wrote: > 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 > --- > include/qemu/job.h | 7 ++++++- > block/backup.c | 2 +- > block/commit.c | 2 +- > block/mirror.c | 2 +- > block/stream.c | 2 +- > job-qmp.c | 9 ++------- > job.c | 15 +++++++++++++-- > 7 files changed, 25 insertions(+), 14 deletions(-) > > diff --git a/include/qemu/job.h b/include/qemu/job.h > index 8c8badf75e..b2e1dd00b9 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 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/block/backup.c b/block/backup.c > index 4e228e959b..5661435675 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 620666161b..e1814d9693 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 dcb66ec3be..435268bbbf 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 a5d6e0cf8a..9264b68a1e 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/job-qmp.c b/job-qmp.c > index 7f38f63336..410775df61 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 f026661b0f..fc39eaaa5e 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); > } > @@ -661,6 +662,9 @@ static void job_update_rc(Job *job) > } > if (job->ret) { > job_state_transition(job, JOB_STATUS_ABORTING); > + if (!job->error) { > + job->error = g_strdup(strerror(-job->ret)); > + } > } > } > > @@ -855,10 +859,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); The assert here implies that only job->ret values < 0 are valid for error. Elsewhere, we just check for non-zero values for error (for example, [1]). Maybe we should relax this to just assert(job->ret) here? > + job->error = g_strdup(error_get_pretty(error)); > + error_free(error); > + } > + > job_update_rc(job); > trace_job_completed(job, ret, job->ret); [1] > if (job->ret) { job_completed_txn_abort(job); } else { job_completed_txn_success(job); } > @@ -876,7 +887,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 { > -- > 2.13.6 > >