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 <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(-) >> >> Reviewed-by: Max Reitz <mre...@redhat.com> >> >> 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 && !@err" 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
