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

[Markus Armbruster]

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.