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 <kw...@redhat.com> >>> Reviewed-by: Max Reitz <mre...@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 >