[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 03/14] job: Add error message for failing jobs
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH 03/14] job: Add error message for failing jobs |
Date: |
Tue, 29 May 2018 13:01:27 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 |
On 2018-05-25 18:33, 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 <address@hidden>
> ---
> 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(-)
There are some tests that call job_completed(). Those should be updated
as well.
> 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;
> +
I think we should bind this directly to job->ret, i.e. this is NULL if
and only if job->ret == 0. (Just because more constraints tend to make
things nicer. And also because if you don't, then qmp_job_dismiss()
cannot assume that job->error is set on error, which would be a change
in behavior.)
> /** ret code passed to job_completed. */
> int ret;
>
[...]
> diff --git a/job.c b/job.c
> index f026661b0f..fc39eaaa5e 100644
> --- a/job.c
> +++ b/job.c
job_prepare() (called by job_do_finalize() through job_txn_apply()) may
set job->ret. If it does set it to a negative value, job_do_finalize()
will call job_completed_txn_abort(), which will eventually invoke
job_finalize_single(), which then runs job_update_rc() on the job. This
is a bit too much code between setting job->ret and job->error for my
taste, I'd rather set job->error much sooner (e.g. by calling
job_update_rc() directly in job_prepare(), which I don't think would
change anything).
But if you think that this is just fine, then OK, because I don't think
the user can do QMP queries in between (it would still break the iff
relationship between job->ret and job->error, which I find desirable).
[...]
> @@ -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));
> + }
If we do use an iff relationship between job->ret and job->error, this
should probably be inserted before job_state_transition().
Max
> }
> }
signature.asc
Description: OpenPGP digital signature
- [Qemu-block] [PATCH 00/14] block: Make blockdev-create a job and stable API, Kevin Wolf, 2018/05/25
- [Qemu-block] [PATCH 01/14] vdi: Fix vdi_co_do_create() return value, Kevin Wolf, 2018/05/25
- [Qemu-block] [PATCH 02/14] vhdx: Fix vhdx_co_create() return value, Kevin Wolf, 2018/05/25
- [Qemu-block] [PATCH 03/14] job: Add error message for failing jobs, Kevin Wolf, 2018/05/25
- [Qemu-block] [PATCH 04/14] block/create: Make x-blockdev-create a job, Kevin Wolf, 2018/05/25
- [Qemu-block] [PATCH 05/14] qemu-iotests: Add VM.get_qmp_events_filtered(), Kevin Wolf, 2018/05/25
- [Qemu-block] [PATCH 06/14] qemu-iotests: Add VM.qmp_log(), Kevin Wolf, 2018/05/25