Re: [Qemu-devel] [PATCH v2 8/9] jobs: remove ret argument to job_completed; privatize it

[John Snow]

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 <address@hidden>
>> ---
>>  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"
>>
> 
>