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

[Max Reitz]

On 2018-08-30 02:06, John Snow wrote:
> 
> 
> On 08/27/2018 05:30 AM, Max Reitz wrote:
>> On 2018-08-24 00:08, 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(-)
>>
>> Reviewed-by: Max Reitz <address@hidden>
>>
>> But I see a discrepancy in the upcoming s->ret <=> s->err relationship
>> now.  And that is if .run() doesn't return an Error *...
>>
>> That could be remedied immediately in job_co_entry(), though, either by
>> calling job_update_rc(), or by inlining its "if (!job->err)" part.
>>
>> Max
>>
> 
> Jobs currently exist in ... five-ish phases.
> 
> Phase 0: Not started. (Always UNDEFINED or CREATED.)
> Phase 1: In the coroutine. (RUNNING, READY, STANDBY, PAUSED.)
> Phase 2: Deferred to main, but job_completed not yet called. [Not
> dignified with a formal status, but job->deferred_to_main_loop set.]
> Phase 3: job_completed has been called. (ABORTING, WAITING, PENDING)
> Phase 4: job_finalize_single has been called. (CONCLUDED, NULL)
> 
> Broadly, though, we separate these out into two main clusters:
> 
> (A): job_is_completed == FALSE; Phases 0, 1 and 2 above.
> (B): job_is_completed == TRUE; Phases 3 and 4 above.
> 
> The ABORTING status as it exists now is a phase 3 status. It never gets
> set before this call, so it is a reliable indicator of being in phase 3.
> 
> If I adjust the usage of job_update_rc like you asked in several
> reviews, it changes it to being a status that can exist in either Phase
> 2 *or* 3, which complicates the code a bit as it requires an audit of
> every caller to job_is_completed and replacing it with something more
> appropriate. Worse, we have no way to identify phase 2 anymore without
> adding a new status or a new boolean.
> 
> I think this is a change worth making, but I must beg to defer this
> change for a later patchset for the time-being, and leave the
> job_update_rc calls alone for the present patchset so I can focus on
> more pressing matters.
> 
> It might be simplest to say that at CONCLUDED time, the "err iff ret"
> relationship will be true but potentially not before then. I think this
> is reasonable as the error code cannot be held to be final until, well,
> the job has finished.

It's OK making the change at a later point.

Maybe it would make more sense to pull out the
"set @err if @ret && address@hidden" part from job_update_rc() into an own
function, and then call that every time @ret has been updated.  (And
call the rest of the function, which does a possible state transition,
only where that makes sense.)

Max

signature.asc
Description: OpenPGP digital signature