Re: [Qemu-block] [PATCH 1/7] jobs: change start callback to run callback

[John Snow]

On 08/22/2018 06:51 AM, Max Reitz wrote:
> On 2018-08-17 21:04, John Snow wrote:
>> Presently we codify the entry point for a job as the "start" callback,
>> but a more apt name would be "run" to clarify the idea that when this
>> function returns we consider the job to have "finished," except for
>> any cleanup which occurs in separate callbacks later.
>>
>> As part of this clarification, change the signature to include an error
>> object and a return code. The error ptr is not yet used, and the return
>> code while captured, will be overwritten by actions in the job_completed
>> function.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  block/backup.c            |  7 ++++---
>>  block/commit.c            |  7 ++++---
>>  block/create.c            |  8 +++++---
>>  block/mirror.c            | 10 ++++++----
>>  block/stream.c            |  7 ++++---
>>  include/qemu/job.h        |  2 +-
>>  job.c                     |  6 +++---
>>  tests/test-bdrv-drain.c   |  7 ++++---
>>  tests/test-blockjob-txn.c | 16 ++++++++--------
>>  tests/test-blockjob.c     |  7 ++++---
>>  10 files changed, 43 insertions(+), 34 deletions(-)
> 
> [...]
> 
>> diff --git a/job.c b/job.c
>> index fa671b431a..898260b2b3 100644
>> --- a/job.c
>> +++ b/job.c
>> @@ -544,16 +544,16 @@ static void coroutine_fn job_co_entry(void *opaque)
>>  {
>>      Job *job = opaque;
>>  
>> -    assert(job && job->driver && job->driver->start);
>> +    assert(job && job->driver && job->driver->run);
>>      job_pause_point(job);
>> -    job->driver->start(job);
>> +    job->ret = job->driver->run(job, NULL);
>>  }
> 
> Hmmm, this breaks the iff relationship with job->error.  We might call
> job_update_rc() afterwards, but then job_completed() would need to free
> it if it overwrites it with the error description from a potential error
> object.
> 
> Also, I suspect the following patches might fix the relationship anyway?
>  (But then an "XXX: This does not hold right now, but will be fixed in a
> future patch" in the documentation of Job.error might help.)
> 
> Max
> 

Hmm... does it? ... I guess you mean that we are setting job->ret
earlier than we used to, which gives us a window where you can have ret
set, but error unset.

This will get settled out by the end of the series anyway:

- char *error gets replaced with Error *err,
- I remove the error object from job_completed
- v2 will remove the ret argument, too.

--js