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

[Max Reitz]

On 2018-08-27 11:41, Max Reitz wrote:
> On 2018-08-24 00:08, 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>
>> ---
>>  block/backup.c            |  2 +-
>>  block/commit.c            |  2 +-
>>  block/create.c            |  5 ++---
>>  block/mirror.c            |  2 +-
>>  block/stream.c            |  2 +-
>>  include/qemu/job.h        | 10 ++++------
>>  job-qmp.c                 |  5 +++--
>>  job.c                     | 18 ++++++------------
>>  tests/test-bdrv-drain.c   |  2 +-
>>  tests/test-blockjob-txn.c |  2 +-
>>  tests/test-blockjob.c     |  2 +-
>>  11 files changed, 22 insertions(+), 30 deletions(-)
> 
> [...]
> 
>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>> index 9cf463d228..5c92c53ef0 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;
>> +
> 
> My question remains why you don't keep the iff condition here...
> 
>>      /** The completion function that will be called when the job completes. 
>>  */
>>      BlockCompletionFunc *cb;
>>  
> 
> [...]
> 
>> diff --git a/job.c b/job.c
>> index 76988f6678..bc1d970df4 100644
>> --- a/job.c
>> +++ b/job.c
> 
> [...]
> 
>> @@ -546,7 +546,7 @@ static void coroutine_fn job_co_entry(void *opaque)
>>  
>>      assert(job && job->driver && job->driver->run);
>>      job_pause_point(job);
>> -    job->ret = job->driver->run(job, NULL);
>> +    job->ret = job->driver->run(job, &job->err);
> 
> ...by e.g. calling job_update_rc() here.
> 
> (Which seems reasonable since this did update the return code.)
> 
> Rest looks good, although I'm missing a "jobs: remove @ret from
> job_completed" patch in one of the two series...

"Max can't read" confirmed

signature.asc
Description: OpenPGP digital signature