[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/9] jobs: canonize Error object
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/9] jobs: canonize Error object |
Date: |
Mon, 3 Sep 2018 14:22:33 +0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
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