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

[John Snow <jsnow@redhat.com>]

On 08/25/2018 09:33 AM, Max Reitz wrote:
> On 2018-08-23 01:01, John Snow wrote:
>>
>>
>> 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 <jsnow@redhat.com>
>>>> ---
>>>>  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:
> 
> Oh no, it appears I accidentally removed yet another chunk from my reply
> to patch 2...
> 
>> - char *error gets replaced with Error *err,
> 
> Which is basically the same.  I noted in the deleted chunk that patch 2
> just removes the iff relationship from the describing comment, but, well...
> 
>> - I remove the error object from job_completed
>> - v2 will remove the ret argument, too.
> 
> The most important bit of the chunk I removed was that I was complaining
> about Job.ret still being there.  I don't really see the point of this
> patch here at this point.
> 
> Unfortunately I can't quite recall...
> 
> Having a central Error object doesn't really make sense.  Whenever the
> block job wants to return an error, it should probably do so as "return"
> values of methods (like .run()).  Same for ret, of course.
> 
> I understand that this is probably really only possible after v2 when
> you've made more use of abort/commit.  But still, I don't think this
> patch improves anything, so I would leave this clean-up until later when
> you can really do something.
> 
> I suppose the idea here is that you want to drop the errp parameter from
> job_completed(), because it is not going to be called by .exit().  But
> the obvious way around this would be to pass an errp to .exit() and then
> pass the result on to job_completed().
> 
> Max
> 

That might be a good idea, but I need to look at the pathway for
actually showing that error to the user, since we need to pass it down a
few times anyway. It's certainly simpler to just stash it in the object.

I'll take a look.