Re: [Qemu-devel] [PATCH 01/21] jobs: canonize Error object

[Kevin Wolf]

Am 07.08.2018 um 06:33 hat John Snow geschrieben:
> 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.
> 
> Signed-off-by: John Snow <address@hidden>

Hm, not sure. Overall, this feels like a step backwards.

> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 18c9223e31..845ad00c03 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -124,12 +124,12 @@ 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 **/
> +    Error *err;
> +
>      /** The completion function that will be called when the job completes.  
> */
>      BlockCompletionFunc *cb;

This is the change that I could agree with, though I don't think it
makes a big difference: Whether you store the string immediately or an
Error object from which you get the string later, doesn't really make a
big difference.

Maybe we find more uses and having an Error object is common practice in
QEMU, so no objections to this change.

> @@ -484,15 +484,13 @@ 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);

I don't like this one, though.

Before this change, job_completed(..., NULL) was a clear sign that the
error message probably needed an improvement, because an errno string
doesn't usually describe error situations very well. We may not have a
much better message in some cases, but in most cases we just don't pass
one because an error message after job creation is still a quite new
thing in the QAPI schema.

What we should get rid of in the long term is the int ret, not the Error
*error. I suspect callers really just distinguish success/error without
actually looking at the error code.

With this change applied, what's your new conversion plan for making
sure that every failing caller of job_completed() has set job->error
first?

> @@ -666,8 +665,8 @@ 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_errno(&job->err, -job->ret, "job failed");
>          }
>          job_state_transition(job, JOB_STATUS_ABORTING);
>      }

This hunk just makes the error message more verbose with a "job failed"
prefix that doesn't add information. If it's the error string for a job,
of course the job failed.

Kevin