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

[John Snow]

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"

--js

>> With that fixed,
>> Reviewed-by: Eric Blake <address@hidden>