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

[John Snow]

On 05/24/2018 04:30 AM, Kevin Wolf wrote:
> 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 <address@hidden>
>>> Reviewed-by: Max Reitz <address@hidden>
>>> ---
>>>  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
> 

Haha! Sure!

> 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.
> 

I think the "default" case removes that benefit somewhat; it's nicer
when the compiler yelps at you for forgetting. The cases that might
cause an assertion could be harder to hit.

No need to change anything though, I was really just curious about your
style preferences.

(I often like to write switch/case statements without a default clause
hoping that the compiler will catch when I don't check all possible
enumerations, but this doesn't work well with QAPI values because of the
__MAX enumeration we like to add at the end. You can fix it by adding a
case MY_ENUM__MAX: g_assert_not_reached();, but I'm often afraid this
looks too hacky and silly to check in. I wish C had slightly nicer
enumerations as first class citizens a lot. I also wish I had a yacht
sometimes.)

> (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.)
> 

Makes much more sense.

Thanks!

> Kevin
>