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

[John Snow]

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.

Thanks,
--js