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 <js...@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"
>>
>
>
