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 <js...@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...
