Am 01.09.2018 um 09:54 hat Markus Armbruster geschrieben: > John Snow <js...@redhat.com> writes: > > > On 08/31/2018 02:08 AM, Markus Armbruster wrote: > >> Eric Blake <ebl...@redhat.com> 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 <js...@redhat.com> > >>>> --- > >>> > >>>> +++ 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 > > qemu-devel@nongnu.org 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