[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2 31/40] job: Add job_is_ready()
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v2 31/40] job: Add job_is_ready() |
Date: |
Thu, 24 May 2018 10:30:20 +0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
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
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
- [Qemu-block] [PATCH v2 22/40] job: Add job_drain(), (continued)
- [Qemu-block] [PATCH v2 22/40] job: Add job_drain(), Kevin Wolf, 2018/05/18
- [Qemu-block] [PATCH v2 23/40] job: Move .complete callback to Job, Kevin Wolf, 2018/05/18
- [Qemu-block] [PATCH v2 24/40] job: Move job_finish_sync() to Job, Kevin Wolf, 2018/05/18
- [Qemu-block] [PATCH v2 28/40] block: Cancel job in bdrv_close_all() callers, Kevin Wolf, 2018/05/18
- [Qemu-block] [PATCH v2 25/40] job: Switch transactions to JobTxn, Kevin Wolf, 2018/05/18
- [Qemu-block] [PATCH v2 26/40] job: Move transactions to Job, Kevin Wolf, 2018/05/18
- [Qemu-block] [PATCH v2 29/40] job: Add job_yield(), Kevin Wolf, 2018/05/18
- [Qemu-block] [PATCH v2 27/40] job: Move completion and cancellation to Job, Kevin Wolf, 2018/05/18
- [Qemu-block] [PATCH v2 31/40] job: Add job_is_ready(), Kevin Wolf, 2018/05/18
[Qemu-block] [PATCH v2 30/40] job: Add job_dismiss(), Kevin Wolf, 2018/05/18
[Qemu-block] [PATCH v2 32/40] job: Add job_transition_to_ready(), Kevin Wolf, 2018/05/18
[Qemu-block] [PATCH v2 34/40] job: Introduce qapi/job.json, Kevin Wolf, 2018/05/18
[Qemu-block] [PATCH v2 33/40] job: Move progress fields to Job, Kevin Wolf, 2018/05/18
[Qemu-block] [PATCH v2 39/40] iotests: Move qmp_to_opts() to VM, Kevin Wolf, 2018/05/18
[Qemu-block] [PATCH v2 37/40] job: Add query-jobs QMP command, Kevin Wolf, 2018/05/18