Re: [Qemu-devel] [PATCH v2 31/40] job: Add job_is_ready()

[Kevin Wolf <kwolf@redhat.com>]

Am 24.05.2018 um 01:42 hat John Snow geschrieben:
> On 05/18/2018 09:21 AM, Kevin Wolf wrote:
> > Instead of having a 'bool ready' in BlockJob, add a function that
> > derives its value from the job status.
> > 
> > At the same time, this fixes the behaviour to match what the QAPI
> > documentation promises for query-block-job: 'true if the job may be
> > completed'. When the ready flag was introduced in commit ef6dbf1e46e,
> > the flag never had to be reset to match the description because after
> > being ready, the jobs would immediately complete and disappear.
> > 
> > Job transactions and manual job finalisation were introduced only later.
> > With these changes, jobs may stay around even after having completed
> > (and they are not ready to be completed a second time), however their
> > patches forgot to reset the ready flag.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  include/block/blockjob.h |  5 -----
> >  include/qemu/job.h       |  3 +++
> >  blockjob.c               |  3 +--
> >  job.c                    | 22 ++++++++++++++++++++++
> >  qemu-img.c               |  2 +-
> >  tests/test-blockjob.c    |  2 +-
> >  6 files changed, 28 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> > index 5a81afc6c3..8e1e1ee0de 100644
> > --- a/include/block/blockjob.h
> > +++ b/include/block/blockjob.h
> > @@ -49,11 +49,6 @@ typedef struct BlockJob {
> >      /** The block device on which the job is operating.  */
> >      BlockBackend *blk;
> >  
> > -    /**
> > -     * Set to true when the job is ready to be completed.
> > -     */
> > -    bool ready;
> > -
> >      /** Status that is published by the query-block-jobs QMP API */
> >      BlockDeviceIoStatus iostatus;
> >  
> > diff --git a/include/qemu/job.h b/include/qemu/job.h
> > index 1e8050c6fb..487f9d9a32 100644
> > --- a/include/qemu/job.h
> > +++ b/include/qemu/job.h
> > @@ -367,6 +367,9 @@ bool job_is_cancelled(Job *job);
> >  /** Returns whether the job is in a completed state. */
> >  bool job_is_completed(Job *job);
> >  
> > +/** Returns whether the job is ready to be completed. */
> > +bool job_is_ready(Job *job);
> > +
> >  /**
> >   * Request @job to pause at the next pause point. Must be paired with
> >   * job_resume(). If the job is supposed to be resumed by user action, call
> > diff --git a/blockjob.c b/blockjob.c
> > index 3ca009bea5..38f18e9ba3 100644
> > --- a/blockjob.c
> > +++ b/blockjob.c
> > @@ -269,7 +269,7 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
> >      info->offset    = job->offset;
> >      info->speed     = job->speed;
> >      info->io_status = job->iostatus;
> > -    info->ready     = job->ready;
> > +    info->ready     = job_is_ready(&job->job),
> >      info->status    = job->job.status;
> >      info->auto_finalize = job->job.auto_finalize;
> >      info->auto_dismiss  = job->job.auto_dismiss;
> > @@ -436,7 +436,6 @@ void block_job_user_resume(Job *job)
> >  void block_job_event_ready(BlockJob *job)
> >  {
> >      job_state_transition(&job->job, JOB_STATUS_READY);
> > -    job->ready = true;
> >  
> >      if (block_job_is_internal(job)) {
> >          return;
> > diff --git a/job.c b/job.c
> > index af31de4669..66ee26f2a0 100644
> > --- a/job.c
> > +++ b/job.c
> > @@ -199,6 +199,28 @@ bool job_is_cancelled(Job *job)
> >      return job->cancelled;
> >  }
> >  
> > +bool job_is_ready(Job *job)
> > +{
> > +    switch (job->status) {
> > +    case JOB_STATUS_UNDEFINED:
> > +    case JOB_STATUS_CREATED:
> > +    case JOB_STATUS_RUNNING:
> > +    case JOB_STATUS_PAUSED:
> > +    case JOB_STATUS_WAITING:
> > +    case JOB_STATUS_PENDING:
> > +    case JOB_STATUS_ABORTING:
> > +    case JOB_STATUS_CONCLUDED:
> > +    case JOB_STATUS_NULL:
> > +        return false;
> > +    case JOB_STATUS_READY:
> > +    case JOB_STATUS_STANDBY:
> > +        return true;
> > +    default:
> > +        g_assert_not_reached();
> > +    }
> > +    return false;
> > +}
> > +
> 
> What's the benefit to a switch statement with a default clause here,
> over the shorter:
> 
> if (job->status == READY || job->status == STANDBY) {
>   return true;
> }
> return false;
> 
> (Yes, I realize you already merged this code, but I'm still curious and
> I need to read the series anyway to see what's changed...)

That it's easy to copy and paste from job_is_completed()? :-P

I guess you could argue that the switch ensures that we don't forget to
explicitly handle every state if we ever add a new one, but the real
reason is more like, job_is_completed() was already there and I didn't
see a reason to do something different here.

(And actually, I seem to remember that I initially had everything after
READY return true as well, but then I noticed that you can reach those
states through ABORTED as well, which should not result in
job_is_ready() returning true.)

Kevin