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

[Kevin Wolf <kwolf@redhat.com>]

Am 08.08.2018 um 17:50 hat John Snow geschrieben:
> 
> 
> On 08/08/2018 10:57 AM, Kevin Wolf wrote:
> > 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 <jsnow@redhat.com>
> > 
> > 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?
> > 
> 
> Getting rid of job_completed and moving to our fairly ubiquitous "ret &
> Error *" combo.

Yup, with the context of the discussion for patch 2, if you make .start
(or .run) take an Error **errp, that sounds like a good plan to me.

> >> @@ -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
> > 
> 
> Yeah, it's not a good prefix, but if I wanted to use the error object in
> a general way across all jobs, I needed _some_ kind of prefix there...

Shouldn't this one work?

    error_setg(&job->err, strerror(-job->ret));

Kevin