| Message ID | 20180823220856.10094-9-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | jobs: Job Exit Refactoring Pt 1 | expand |
On 2018-08-24 00:08, John Snow wrote: > Jobs are now expected to return their retcode on the stack, from the > .run callback, so we can remove that argument. > > job_cancel does not need to set -ECANCELED because job_completed will > update the return code itself if the job was canceled. > > While we're here, make job_completed static to job.c and remove it from > job.h; move the documentation of return code to the .run() callback and > to the job->ret property, accordingly. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > include/qemu/job.h | 24 +++++++++++------------- > job.c | 11 ++++++----- > trace-events | 2 +- > 3 files changed, 18 insertions(+), 19 deletions(-) Er, yeah. Sorry for not being able to read. Again. > diff --git a/include/qemu/job.h b/include/qemu/job.h > index c67f6a647e..2990f28edc 100644 > --- a/include/qemu/job.h > +++ b/include/qemu/job.h > @@ -124,7 +124,7 @@ typedef struct Job { > /** Estimated progress_current value at the completion of the job */ > int64_t progress_total; > > - /** ret code passed to job_completed. */ > + /** Return code from @run callback; 0 on success and -errno on failure. */ Hm. Not really, it's the general status of the whole job, isn't it? Besides being the return value from .run(), it's also set by .exit() (so it's presumably going to be the return value from .prepare() after part 2) and by job_update_rc() when the job has been cancelled. > int ret; > > /** Error object for a failed job **/ > @@ -168,7 +168,16 @@ struct JobDriver { > /** Enum describing the operation */ > JobType job_type; > > - /** Mandatory: Entrypoint for the Coroutine. */ > + /** > + * Mandatory: Entrypoint for the Coroutine. > + * > + * This callback will be invoked when moving from CREATED to RUNNING. > + * > + * If this callback returns nonzero, the job transaction it is part of is > + * aborted. If it returns zero, the job moves into the WAITING state. If it > + * is the last job to complete in its transaction, all jobs in the > + * transaction move from WAITING to PENDING. > + */ Moving this description from job_completed() seems to imply we do want to call job_update_rc() right after invoking .run(). > int coroutine_fn (*run)(Job *job, Error **errp); > > /** > @@ -492,17 +501,6 @@ void job_early_fail(Job *job); > /** Moves the @job from RUNNING to READY */ > void job_transition_to_ready(Job *job); > > -/** > - * @job: The job being completed. > - * @ret: The status code. > - * > - * Marks @job as completed. If @ret is non-zero, the job transaction it is part > - * of is aborted. If @ret is zero, the job moves into the WAITING state. If it > - * is the last job to complete in its transaction, all jobs in the transaction > - * move from WAITING to PENDING. > - */ > -void job_completed(Job *job, int ret); > - > /** Asynchronously complete the specified @job. */ > void job_complete(Job *job, Error **errp); > > diff --git a/job.c b/job.c > index bc8dad4e71..213042b762 100644 > --- a/job.c > +++ b/job.c > @@ -535,6 +535,8 @@ void job_drain(Job *job) > } > } > > +static void job_completed(Job *job); > + > static void job_exit(void *opaque) > { > Job *job = (Job *)opaque; > @@ -545,7 +547,7 @@ static void job_exit(void *opaque) > job->driver->exit(job); > aio_context_release(aio_context); > } > - job_completed(job, job->ret); > + job_completed(job); > } > > /** > @@ -883,13 +885,12 @@ static void job_completed_txn_success(Job *job) > } > } > > -void job_completed(Job *job, int ret) > +static void job_completed(Job *job) > { > assert(job && job->txn && !job_is_completed(job)); > > - job->ret = ret; > job_update_rc(job); I think we want to remove the job_update_rc() from here. It should be called after job->ret is updated, i.e. immediately after .run() and .exit() have been invoked. (Or presumably .prepare() in part 2.) Oh, and in job_cancel() before it invokes job_completed()? But then again, maybe it would be easiest to keep it here... It just doesn't feel quite right to me. Max > - trace_job_completed(job, ret, job->ret); > + trace_job_completed(job, job->ret); > if (job->ret) { > job_completed_txn_abort(job); > } else { > @@ -905,7 +906,7 @@ void job_cancel(Job *job, bool force) > } > job_cancel_async(job, force); > if (!job_started(job)) { > - job_completed(job, -ECANCELED); > + job_completed(job); > } else if (job->deferred_to_main_loop) { > job_completed_txn_abort(job); > } else { > diff --git a/trace-events b/trace-events > index c445f54773..4fd2cb4b97 100644 > --- a/trace-events > +++ b/trace-events > @@ -107,7 +107,7 @@ gdbstub_err_checksum_incorrect(uint8_t expected, uint8_t got) "got command packe > # job.c > job_state_transition(void *job, int ret, const char *legal, const char *s0, const char *s1) "job %p (ret: %d) attempting %s transition (%s-->%s)" > job_apply_verb(void *job, const char *state, const char *verb, const char *legal) "job %p in state %s; applying verb %s (%s)" > -job_completed(void *job, int ret, int jret) "job %p ret %d corrected ret %d" > +job_completed(void *job, int ret) "job %p ret %d" > > # job-qmp.c > qmp_job_cancel(void *job) "job %p" >
On 08/27/2018 06:52 AM, Max Reitz wrote: > On 2018-08-24 00:08, John Snow wrote: >> Jobs are now expected to return their retcode on the stack, from the >> .run callback, so we can remove that argument. >> >> job_cancel does not need to set -ECANCELED because job_completed will >> update the return code itself if the job was canceled. >> >> While we're here, make job_completed static to job.c and remove it from >> job.h; move the documentation of return code to the .run() callback and >> to the job->ret property, accordingly. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> include/qemu/job.h | 24 +++++++++++------------- >> job.c | 11 ++++++----- >> trace-events | 2 +- >> 3 files changed, 18 insertions(+), 19 deletions(-) > > Er, yeah. Sorry for not being able to read. Again. > >> diff --git a/include/qemu/job.h b/include/qemu/job.h >> index c67f6a647e..2990f28edc 100644 >> --- a/include/qemu/job.h >> +++ b/include/qemu/job.h >> @@ -124,7 +124,7 @@ typedef struct Job { >> /** Estimated progress_current value at the completion of the job */ >> int64_t progress_total; >> >> - /** ret code passed to job_completed. */ >> + /** Return code from @run callback; 0 on success and -errno on failure. */ > > Hm. Not really, it's the general status of the whole job, isn't it? > Besides being the return value from .run(), it's also set by .exit() (so > it's presumably going to be the return value from .prepare() after part > 2) and by job_update_rc() when the job has been cancelled. > You're right. I was trying to emphasize where it gets set in the normative case. I'll rephrase. What I want to say is effectively: "This is the return code for the job, which is what gets returned by the .run and/or .prepare callbacks, or gets set to -ECANCELED if the job is canceled and the job itself neglects to set a nonzero code." >> int ret; >> >> /** Error object for a failed job **/ >> @@ -168,7 +168,16 @@ struct JobDriver { >> /** Enum describing the operation */ >> JobType job_type; >> >> - /** Mandatory: Entrypoint for the Coroutine. */ >> + /** >> + * Mandatory: Entrypoint for the Coroutine. >> + * >> + * This callback will be invoked when moving from CREATED to RUNNING. >> + * >> + * If this callback returns nonzero, the job transaction it is part of is >> + * aborted. If it returns zero, the job moves into the WAITING state. If it >> + * is the last job to complete in its transaction, all jobs in the >> + * transaction move from WAITING to PENDING. >> + */ > > Moving this description from job_completed() seems to imply we do want > to call job_update_rc() right after invoking .run(). > Sure, I'll take a look at that. >> int coroutine_fn (*run)(Job *job, Error **errp); >> >> /** >> @@ -492,17 +501,6 @@ void job_early_fail(Job *job); >> /** Moves the @job from RUNNING to READY */ >> void job_transition_to_ready(Job *job); >> >> -/** >> - * @job: The job being completed. >> - * @ret: The status code. >> - * >> - * Marks @job as completed. If @ret is non-zero, the job transaction it is part >> - * of is aborted. If @ret is zero, the job moves into the WAITING state. If it >> - * is the last job to complete in its transaction, all jobs in the transaction >> - * move from WAITING to PENDING. >> - */ >> -void job_completed(Job *job, int ret); >> - >> /** Asynchronously complete the specified @job. */ >> void job_complete(Job *job, Error **errp); >> >> diff --git a/job.c b/job.c >> index bc8dad4e71..213042b762 100644 >> --- a/job.c >> +++ b/job.c >> @@ -535,6 +535,8 @@ void job_drain(Job *job) >> } >> } >> >> +static void job_completed(Job *job); >> + >> static void job_exit(void *opaque) >> { >> Job *job = (Job *)opaque; >> @@ -545,7 +547,7 @@ static void job_exit(void *opaque) >> job->driver->exit(job); >> aio_context_release(aio_context); >> } >> - job_completed(job, job->ret); >> + job_completed(job); >> } >> >> /** >> @@ -883,13 +885,12 @@ static void job_completed_txn_success(Job *job) >> } >> } >> >> -void job_completed(Job *job, int ret) >> +static void job_completed(Job *job) >> { >> assert(job && job->txn && !job_is_completed(job)); >> >> - job->ret = ret; >> job_update_rc(job); > > I think we want to remove the job_update_rc() from here. It should be > called after job->ret is updated, i.e. immediately after .run() and > .exit() have been invoked. (Or presumably .prepare() in part 2.) > Oh, and in job_cancel() before it invokes job_completed()? > > But then again, maybe it would be easiest to keep it here... It just > doesn't feel quite right to me. > > Max > It does feel slightly strange now. I'll see if I can find something that feels better. >> - trace_job_completed(job, ret, job->ret); >> + trace_job_completed(job, job->ret); >> if (job->ret) { >> job_completed_txn_abort(job); >> } else { >> @@ -905,7 +906,7 @@ void job_cancel(Job *job, bool force) >> } >> job_cancel_async(job, force); >> if (!job_started(job)) { >> - job_completed(job, -ECANCELED); >> + job_completed(job); >> } else if (job->deferred_to_main_loop) { >> job_completed_txn_abort(job); >> } else { >> diff --git a/trace-events b/trace-events >> index c445f54773..4fd2cb4b97 100644 >> --- a/trace-events >> +++ b/trace-events >> @@ -107,7 +107,7 @@ gdbstub_err_checksum_incorrect(uint8_t expected, uint8_t got) "got command packe >> # job.c >> job_state_transition(void *job, int ret, const char *legal, const char *s0, const char *s1) "job %p (ret: %d) attempting %s transition (%s-->%s)" >> job_apply_verb(void *job, const char *state, const char *verb, const char *legal) "job %p in state %s; applying verb %s (%s)" >> -job_completed(void *job, int ret, int jret) "job %p ret %d corrected ret %d" >> +job_completed(void *job, int ret) "job %p ret %d" >> >> # job-qmp.c >> qmp_job_cancel(void *job) "job %p" >> > >
diff --git a/include/qemu/job.h b/include/qemu/job.h index c67f6a647e..2990f28edc 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -124,7 +124,7 @@ typedef struct Job { /** Estimated progress_current value at the completion of the job */ int64_t progress_total; - /** ret code passed to job_completed. */ + /** Return code from @run callback; 0 on success and -errno on failure. */ int ret; /** Error object for a failed job **/ @@ -168,7 +168,16 @@ struct JobDriver { /** Enum describing the operation */ JobType job_type; - /** Mandatory: Entrypoint for the Coroutine. */ + /** + * Mandatory: Entrypoint for the Coroutine. + * + * This callback will be invoked when moving from CREATED to RUNNING. + * + * If this callback returns nonzero, the job transaction it is part of is + * aborted. If it returns zero, the job moves into the WAITING state. If it + * is the last job to complete in its transaction, all jobs in the + * transaction move from WAITING to PENDING. + */ int coroutine_fn (*run)(Job *job, Error **errp); /** @@ -492,17 +501,6 @@ void job_early_fail(Job *job); /** Moves the @job from RUNNING to READY */ void job_transition_to_ready(Job *job); -/** - * @job: The job being completed. - * @ret: The status code. - * - * Marks @job as completed. If @ret is non-zero, the job transaction it is part - * of is aborted. If @ret is zero, the job moves into the WAITING state. If it - * is the last job to complete in its transaction, all jobs in the transaction - * move from WAITING to PENDING. - */ -void job_completed(Job *job, int ret); - /** Asynchronously complete the specified @job. */ void job_complete(Job *job, Error **errp); diff --git a/job.c b/job.c index bc8dad4e71..213042b762 100644 --- a/job.c +++ b/job.c @@ -535,6 +535,8 @@ void job_drain(Job *job) } } +static void job_completed(Job *job); + static void job_exit(void *opaque) { Job *job = (Job *)opaque; @@ -545,7 +547,7 @@ static void job_exit(void *opaque) job->driver->exit(job); aio_context_release(aio_context); } - job_completed(job, job->ret); + job_completed(job); } /** @@ -883,13 +885,12 @@ static void job_completed_txn_success(Job *job) } } -void job_completed(Job *job, int ret) +static void job_completed(Job *job) { assert(job && job->txn && !job_is_completed(job)); - job->ret = ret; job_update_rc(job); - trace_job_completed(job, ret, job->ret); + trace_job_completed(job, job->ret); if (job->ret) { job_completed_txn_abort(job); } else { @@ -905,7 +906,7 @@ void job_cancel(Job *job, bool force) } job_cancel_async(job, force); if (!job_started(job)) { - job_completed(job, -ECANCELED); + job_completed(job); } else if (job->deferred_to_main_loop) { job_completed_txn_abort(job); } else { diff --git a/trace-events b/trace-events index c445f54773..4fd2cb4b97 100644 --- a/trace-events +++ b/trace-events @@ -107,7 +107,7 @@ gdbstub_err_checksum_incorrect(uint8_t expected, uint8_t got) "got command packe # job.c job_state_transition(void *job, int ret, const char *legal, const char *s0, const char *s1) "job %p (ret: %d) attempting %s transition (%s-->%s)" job_apply_verb(void *job, const char *state, const char *verb, const char *legal) "job %p in state %s; applying verb %s (%s)" -job_completed(void *job, int ret, int jret) "job %p ret %d corrected ret %d" +job_completed(void *job, int ret) "job %p ret %d" # job-qmp.c qmp_job_cancel(void *job) "job %p"
Jobs are now expected to return their retcode on the stack, from the .run callback, so we can remove that argument. job_cancel does not need to set -ECANCELED because job_completed will update the return code itself if the job was canceled. While we're here, make job_completed static to job.c and remove it from job.h; move the documentation of return code to the .run() callback and to the job->ret property, accordingly. Signed-off-by: John Snow <jsnow@redhat.com> --- include/qemu/job.h | 24 +++++++++++------------- job.c | 11 ++++++----- trace-events | 2 +- 3 files changed, 18 insertions(+), 19 deletions(-)