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 <js...@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
signature.asc
Description: OpenPGP digital signature
