Re: [Qemu-devel] [PATCH 01/21] jobs: canonize Error object

[John Snow <jsnow@redhat.com>]

On 08/08/2018 10:57 AM, Kevin Wolf wrote:
> Am 07.08.2018 um 06:33 hat John Snow geschrieben:
>> 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.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> Hm, not sure. Overall, this feels like a step backwards.
> 
>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>> index 18c9223e31..845ad00c03 100644
>> --- a/include/qemu/job.h
>> +++ b/include/qemu/job.h
>> @@ -124,12 +124,12 @@ typedef struct Job {
>>      /** Estimated progress_current value at the completion of the job */
>>      int64_t progress_total;
>>  
>> -    /** Error string for a failed job (NULL if, and only if, job->ret == 0) */
>> -    char *error;
>> -
>>      /** ret code passed to job_completed. */
>>      int ret;
>>  
>> +    /** Error object for a failed job **/
>> +    Error *err;
>> +
>>      /** The completion function that will be called when the job completes.  */
>>      BlockCompletionFunc *cb;
> 
> This is the change that I could agree with, though I don't think it
> makes a big difference: Whether you store the string immediately or an
> Error object from which you get the string later, doesn't really make a
> big difference.
> 
> Maybe we find more uses and having an Error object is common practice in
> QEMU, so no objections to this change.
> 
>> @@ -484,15 +484,13 @@ void job_transition_to_ready(Job *job);
>>  /**
>>   * @job: The job being completed.
>>   * @ret: The status code.
>> - * @error: The error message for a failing job (only with @ret < 0). If @ret is
>> - *         negative, but NULL is given for @error, strerror() is used.
>>   *
>>   * Marks @job as completed. If @ret is non-zero, the job transaction it is part
>>   * of is aborted. If @ret is zero, the job moves into the WAITING state. If it
>>   * is the last job to complete in its transaction, all jobs in the transaction
>>   * move from WAITING to PENDING.
>>   */
>> -void job_completed(Job *job, int ret, Error *error);
>> +void job_completed(Job *job, int ret);
> 
> I don't like this one, though.
> 
> Before this change, job_completed(..., NULL) was a clear sign that the
> error message probably needed an improvement, because an errno string
> doesn't usually describe error situations very well. We may not have a
> much better message in some cases, but in most cases we just don't pass
> one because an error message after job creation is still a quite new
> thing in the QAPI schema.
> 
> What we should get rid of in the long term is the int ret, not the Error
> *error. I suspect callers really just distinguish success/error without
> actually looking at the error code.
> 
> With this change applied, what's your new conversion plan for making
> sure that every failing caller of job_completed() has set job->error
> first?
> 

Getting rid of job_completed and moving to our fairly ubiquitous "ret &
Error *" combo.

>> @@ -666,8 +665,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_errno(&job->err, -job->ret, "job failed");
>>          }
>>          job_state_transition(job, JOB_STATUS_ABORTING);
>>      }
> 
> This hunk just makes the error message more verbose with a "job failed"
> prefix that doesn't add information. If it's the error string for a job,
> of course the job failed.
> 
> Kevin
> 

Yeah, it's not a good prefix, but if I wanted to use the error object in
a general way across all jobs, I needed _some_ kind of prefix there...