[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
Fri, 31 Aug 2018 11:06:53 +0200
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1
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
>>> 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.
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.)
Description: OpenPGP digital signature