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;
+}
+
bool job_is_completed(Job *job)
{
switch (job->status) {
diff --git a/qemu-img.c b/qemu-img.c
index b2bcfc3df1..eecb7d3228 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -867,7 +867,7 @@ static void run_block_job(BlockJob *job, Error **errp)
aio_poll(aio_context, true);
qemu_progress_print(job->len ?
((float)job->offset / job->len * 100.f) : 0.0f, 0);
- } while (!job->ready && !job_is_completed(&job->job));
+ } while (!job_is_ready(&job->job) && !job_is_completed(&job->job));
if (!job_is_completed(&job->job)) {
ret = job_complete_sync(&job->job, errp);
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 7131cabb16..8180d03a5f 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -185,7 +185,7 @@ static void coroutine_fn cancel_job_start(void *opaque)
goto defer;
}
- if (!s->common.ready && s->should_converge) {
+ if (!job_is_ready(&s->common.job) && s->should_converge) {
block_job_event_ready(&s->common);
}
--
2.13.6
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...)
> bool job_is_completed(Job *job)
> {
> switch (job->status) {
> diff --git a/qemu-img.c b/qemu-img.c
> index b2bcfc3df1..eecb7d3228 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -867,7 +867,7 @@ static void run_block_job(BlockJob *job, Error **errp)
> aio_poll(aio_context, true);
> qemu_progress_print(job->len ?
> ((float)job->offset / job->len * 100.f) : 0.0f, 0);
> - } while (!job->ready && !job_is_completed(&job->job));
> + } while (!job_is_ready(&job->job) && !job_is_completed(&job->job));
>
Semantic change in this variable, but due to the way this function is
written it doesn't matter.
> if (!job_is_completed(&job->job)) {
> ret = job_complete_sync(&job->job, errp);
> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index 7131cabb16..8180d03a5f 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -185,7 +185,7 @@ static void coroutine_fn cancel_job_start(void *opaque)
> goto defer;
> }
>
> - if (!s->common.ready && s->should_converge) {
> + if (!job_is_ready(&s->common.job) && s->should_converge) {
> block_job_event_ready(&s->common);
> }
>
>
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
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 <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
>
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
>
Am 24.05.2018 um 19:25 hat John Snow geschrieben:
> >>> 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.
Good point. I'm not sure which compilers would actually warn here, while
asserting makes sure to crash some tests, but in this context I think we
could actually have both: All enum values that are handled return inside
the switch statement. So if we move the g_assert_not_reached() just
after the switch block, we should get both the compiler warning and the
crash.
Would you like to send a patch?
Kevin
© 2016 - 2025 Red Hat, Inc.