[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/9] jobs: change start callback to run callb
Re: [Qemu-devel] [PATCH v2 1/9] jobs: change start callback to run callback
Wed, 29 Aug 2018 20:06:22 -0400
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0
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
>> 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.
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.
- [Qemu-devel] [PATCH v2 5/9] block/mirror: utilize job_exit shim, (continued)
- [Qemu-devel] [PATCH v2 5/9] block/mirror: utilize job_exit shim, John Snow, 2018/08/23
- [Qemu-devel] [PATCH v2 6/9] jobs: utilize job_exit shim, John Snow, 2018/08/23
- [Qemu-devel] [PATCH v2 9/9] jobs: remove job_defer_to_main_loop, John Snow, 2018/08/23
- [Qemu-devel] [PATCH v2 4/9] block/commit: utilize job_exit shim, John Snow, 2018/08/23
- [Qemu-devel] [PATCH v2 1/9] jobs: change start callback to run callback, John Snow, 2018/08/23
- [Qemu-devel] [PATCH v2 3/9] jobs: add exit shim, John Snow, 2018/08/23