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.
