Re: [Qemu-block] [PATCH 03/14] job: Add error message for failing jobs

[Max Reitz]

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