Re: [Qemu-devel] [PATCH v3 2/9] jobs: canonize Error object

[Kevin Wolf]

Am 01.09.2018 um 09:54 hat Markus Armbruster geschrieben:
> John Snow <address@hidden> writes:
> 
> > On 08/31/2018 02:08 AM, Markus Armbruster wrote:
> >> Eric Blake <address@hidden> writes:
> >> 
> >>> On 08/29/2018 08:57 PM, John Snow wrote:
> >>>> 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. The integer error code for job_completed is kept for now,
> >>>> to be removed shortly in a separate patch.
> >>>>
> >>>> Signed-off-by: John Snow <address@hidden>
> >>>> ---
> >>>
> >>>> +++ b/job.c
> >>>
> >>>> @@ -666,8 +666,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(&job->err, "%s", g_strdup(strerror(-job->ret)));
> >>>
> >>> Memleak. Drop the g_strdup(), and just directly pass strerror()
> >>> results to error_setg().  (I guess we can't quite use
> >>> error_setg_errno() unless we add additional text beyond the strerror()
> >>> results).
> >> 
> >> Adding such text might well be an improvement.  I'm not telling you to
> >> do so (not having looked at the context myself), just to think about it.
> >> 
> >
> > In this case, and I agree with Kevin who suggested it; we ought to be
> > moving away from the retcode in general and using first-class error
> > objects for all of our jobs anyway.
> >
> > In this case, the job has failed with a retcode and we wish to give the
> > user some hope of understanding why, but at this point in the code all
> > we know is what the strerror can tell us, so a generic prefix like "The
> > job failed" is not helpful because it will already be clear by events
> > and other things that the job failed.
> >
> > The only text I can think of that would be useful is: "The job failed
> > and didn't give a more specific error message. Please email
> > address@hidden and harass the authors until they fix it. Anyway,
> > nice chatting to you, the generic error message is: %s"
> 
> That might well be an improvement ;)
> 
> Since I don't have a realistic example ready, I'm making up a contrieved
> one:
> 
> --> {"execute": "job-frobnicate"}
> <-- {"error": {"class": "GenericError", "desc": "Device or resource busy"}}
> 
> Would a reply
> 
> <-- {"error": {"class": "GenericError", "desc": "Job failed: Device or 
> resource busy"}}
> 
> be better, worse, or a wash?
> 
> If it's a wash, maintainability breaks the tie.  So let's have a look at
> the code.  It's either
> 
>             error_setg(&job->err, "%s", strerror(-job->ret));
> 
> or
> 
>             error_setg_errno(&job->err, -job->ret, "Job failed");
> 
> I'd prefer the latter, because it's the common way to put an errno code
> into an Error object, and it lets me grep for the message more easily.

Basically, if I call "job-frobnicate", I don't want an error message to
tell me "job-frobnicate failed: foo". I already know that I called
"job-frobnicate", so the actually useful message is only "foo".

A prefix like "job-frobnicate failed" should be added when it's one of
multiple possible error sources. For example, if a higher level monitor
command internally involves job-frobnicate, but also three other
operations, this is the place where the prefix should be added to any
error message returned by job-frobnicate so that we can distinguish
which part of the operation failed.

Kevin