Re: [Qemu-devel] [RFC v4 09/21] blockjobs: add CONCLUDED state
On 02/28/2018 10:37 AM, Kevin Wolf wrote:
> Am 24.02.2018 um 00:51 hat John Snow geschrieben:
>> add a new state "CONCLUDED" that identifies a job that has ceased all
>> operations. The wording was chosen to avoid any phrasing that might
>> imply success, error, or cancellation. The task has simply ceased all
>> operation and can never again perform any work.
>>
>> ("finished", "done", and "completed" might all imply success.)
>>
>> Transitions:
>> Running -> Concluded: normal completion
>> Ready-> Concluded: normal completion
>> Aborting -> Concluded: error and cancellations
>>
>> Verbs:
>> None as of this commit. (a future commit adds 'dismiss')
>>
>> +-+
>> |UNDEFINED|
>> +--+--+
>> |
>> +--v+
>> |CREATED|
>> +--++
>> |
>> +--v+ +--+
>>+-+RUNNING<->PAUSED|
>>| +--+-+--+ +--+
>>|| |
>>|| +--+
>>|||
>>| +--v--+ +---+ |
>>+-+READY<--->STANDBY| |
>>| +--+--+ +---+ |
>>|||
>> +--v-+ +--v--+ |
>> |ABORTING+--->CONCLUDED<-+
>> ++ +-+
>>
>> Signed-off-by: John Snow
>
>> +static void block_job_event_concluded(BlockJob *job)
>> +{
>> +if (block_job_is_internal(job) || !job->manual) {
>> +return;
>> +}
>> +block_job_state_transition(job, BLOCK_JOB_STATUS_CONCLUDED);
>> +}
>
> I don't understand why internal or automatic jobs should follow a
> different state machine. Sure, they won't be in this state for long
The very simple answer is because I overlooked this change from when I
did implement separate graphs for the old and new models.
> because the job is immediately unref'ed. Though if someone holds an
> additional reference, it might be visible - I would consider this a bug
> fix because otherwise the job stays in READY and continues to accept the
> verbs for that state.
>
> Kevin
>
Re: [Qemu-devel] [RFC v4 09/21] blockjobs: add CONCLUDED state
Am 24.02.2018 um 00:51 hat John Snow geschrieben:
> add a new state "CONCLUDED" that identifies a job that has ceased all
> operations. The wording was chosen to avoid any phrasing that might
> imply success, error, or cancellation. The task has simply ceased all
> operation and can never again perform any work.
>
> ("finished", "done", and "completed" might all imply success.)
>
> Transitions:
> Running -> Concluded: normal completion
> Ready-> Concluded: normal completion
> Aborting -> Concluded: error and cancellations
>
> Verbs:
> None as of this commit. (a future commit adds 'dismiss')
>
> +-+
> |UNDEFINED|
> +--+--+
> |
> +--v+
> |CREATED|
> +--++
> |
> +--v+ +--+
>+-+RUNNING<->PAUSED|
>| +--+-+--+ +--+
>|| |
>|| +--+
>|||
>| +--v--+ +---+ |
>+-+READY<--->STANDBY| |
>| +--+--+ +---+ |
>|||
> +--v-+ +--v--+ |
> |ABORTING+--->CONCLUDED<-+
> ++ +-+
>
> Signed-off-by: John Snow
> +static void block_job_event_concluded(BlockJob *job)
> +{
> +if (block_job_is_internal(job) || !job->manual) {
> +return;
> +}
> +block_job_state_transition(job, BLOCK_JOB_STATUS_CONCLUDED);
> +}
I don't understand why internal or automatic jobs should follow a
different state machine. Sure, they won't be in this state for long
because the job is immediately unref'ed. Though if someone holds an
additional reference, it might be visible - I would consider this a bug
fix because otherwise the job stays in READY and continues to accept the
verbs for that state.
Kevin
Re: [Qemu-devel] [RFC v4 09/21] blockjobs: add CONCLUDED state
On 02/27/2018 02:38 PM, Eric Blake wrote:
> On 02/23/2018 05:51 PM, John Snow wrote:
>> add a new state "CONCLUDED" that identifies a job that has ceased all
>> operations. The wording was chosen to avoid any phrasing that might
>> imply success, error, or cancellation. The task has simply ceased all
>> operation and can never again perform any work.
>>
>> ("finished", "done", and "completed" might all imply success.)
>>
>> Transitions:
>> Running -> Concluded: normal completion
>> Ready -> Concluded: normal completion
>> Aborting -> Concluded: error and cancellations
>>
>> Verbs:
>> None as of this commit. (a future commit adds 'dismiss')
>>
>
>
>> @@ -620,7 +623,9 @@ void block_job_user_resume(BlockJob *job, Error
>> **errp)
>> void block_job_cancel(BlockJob *job)
>> {
>> - if (block_job_started(job)) {
>> + if (job->status == BLOCK_JOB_STATUS_CONCLUDED) {
>> + return;
>> + } else if (block_job_started(job)) {
>
> It's also possible to do:
>
> if () {
> return ;
> }
> if (...)
>
> instead of chaining with an 'else if'. Matter of personal taste, so I
> won't make you change it.
>
> Reviewed-by: Eric Blake
>
I guess because in this case, what I did adds two SLOC instead of three.
If the other code did not need to be guarded by an if(), I'd otherwise
agree.
Re: [Qemu-devel] [RFC v4 09/21] blockjobs: add CONCLUDED state
On 02/23/2018 05:51 PM, John Snow wrote:
add a new state "CONCLUDED" that identifies a job that has ceased all
operations. The wording was chosen to avoid any phrasing that might
imply success, error, or cancellation. The task has simply ceased all
operation and can never again perform any work.
("finished", "done", and "completed" might all imply success.)
Transitions:
Running -> Concluded: normal completion
Ready-> Concluded: normal completion
Aborting -> Concluded: error and cancellations
Verbs:
None as of this commit. (a future commit adds 'dismiss')
@@ -620,7 +623,9 @@ void block_job_user_resume(BlockJob *job, Error **errp)
void block_job_cancel(BlockJob *job)
{
-if (block_job_started(job)) {
+if (job->status == BLOCK_JOB_STATUS_CONCLUDED) {
+return;
+} else if (block_job_started(job)) {
It's also possible to do:
if () {
return ;
}
if (...)
instead of chaining with an 'else if'. Matter of personal taste, so I
won't make you change it.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[Qemu-devel] [RFC v4 09/21] blockjobs: add CONCLUDED state
add a new state "CONCLUDED" that identifies a job that has ceased all
operations. The wording was chosen to avoid any phrasing that might
imply success, error, or cancellation. The task has simply ceased all
operation and can never again perform any work.
("finished", "done", and "completed" might all imply success.)
Transitions:
Running -> Concluded: normal completion
Ready-> Concluded: normal completion
Aborting -> Concluded: error and cancellations
Verbs:
None as of this commit. (a future commit adds 'dismiss')
+-+
|UNDEFINED|
+--+--+
|
+--v+
|CREATED|
+--++
|
+--v+ +--+
+-+RUNNING<->PAUSED|
| +--+-+--+ +--+
|| |
|| +--+
|||
| +--v--+ +---+ |
+-+READY<--->STANDBY| |
| +--+--+ +---+ |
|||
+--v-+ +--v--+ |
|ABORTING+--->CONCLUDED<-+
++ +-+
Signed-off-by: John Snow
---
blockjob.c | 43 ---
qapi/block-core.json | 5 -
2 files changed, 32 insertions(+), 16 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index 4c3fcda46c..93b0a36306 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -44,23 +44,24 @@ static QemuMutex block_job_mutex;
/* BlockJob State Transition Table */
bool BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = {
- /* U, C, R, P, Y, S, X */
-/* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0},
-/* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 0},
-/* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0, 1},
-/* P: */ [BLOCK_JOB_STATUS_PAUSED]= {0, 0, 1, 0, 0, 0, 0},
-/* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1, 1},
-/* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0, 0},
-/* X: */ [BLOCK_JOB_STATUS_ABORTING] = {0, 0, 0, 0, 0, 0, 0},
+ /* U, C, R, P, Y, S, X, E */
+/* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0, 0, 0},
+/* C: */ [BLOCK_JOB_STATUS_CREATED] = {0, 0, 1, 0, 0, 0, 0, 0},
+/* R: */ [BLOCK_JOB_STATUS_RUNNING] = {0, 0, 0, 1, 1, 0, 1, 1},
+/* P: */ [BLOCK_JOB_STATUS_PAUSED]= {0, 0, 1, 0, 0, 0, 0, 0},
+/* Y: */ [BLOCK_JOB_STATUS_READY] = {0, 0, 0, 0, 0, 1, 1, 1},
+/* S: */ [BLOCK_JOB_STATUS_STANDBY] = {0, 0, 0, 0, 1, 0, 0, 0},
+/* X: */ [BLOCK_JOB_STATUS_ABORTING] = {0, 0, 0, 0, 0, 0, 0, 1},
+/* E: */ [BLOCK_JOB_STATUS_CONCLUDED] = {0, 0, 0, 0, 0, 0, 0, 0},
};
bool BlockJobVerbTable[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = {
- /* U, C, R, P, Y, S, X */
-[BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 0},
-[BLOCK_JOB_VERB_PAUSE]= {0, 1, 1, 1, 1, 1, 0},
-[BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1, 0},
-[BLOCK_JOB_VERB_SET_SPEED]= {0, 1, 1, 1, 1, 1, 0},
-[BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0},
+ /* U, C, R, P, Y, S, X, E */
+[BLOCK_JOB_VERB_CANCEL] = {0, 1, 1, 1, 1, 1, 0, 0},
+[BLOCK_JOB_VERB_PAUSE]= {0, 1, 1, 1, 1, 1, 0, 0},
+[BLOCK_JOB_VERB_RESUME] = {0, 1, 1, 1, 1, 1, 0, 0},
+[BLOCK_JOB_VERB_SET_SPEED]= {0, 1, 1, 1, 1, 1, 0, 0},
+[BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0},
};
static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
@@ -114,6 +115,7 @@ static void __attribute__((__constructor__))
block_job_init(void)
static void block_job_event_cancelled(BlockJob *job);
static void block_job_event_completed(BlockJob *job, const char *msg);
+static void block_job_event_concluded(BlockJob *job);
static void block_job_enter_cond(BlockJob *job, bool(*fn)(BlockJob *job));
/* Transactional group of block jobs */
@@ -420,6 +422,7 @@ static void block_job_completed_single(BlockJob *job)
QLIST_REMOVE(job, txn_list);
block_job_txn_unref(job->txn);
+block_job_event_concluded(job);
block_job_unref(job);
}
@@ -620,7 +623,9 @@ void block_job_user_resume(BlockJob *job, Error **errp)
void block_job_cancel(BlockJob *job)
{
-if (block_job_started(job)) {
+if (job->status == BLOCK_JOB_STATUS_CONCLUDED) {
+return;
+} else if (block_job_started(job)) {
block_job_cancel_async(job);
block_job_enter(job);
} else {
@@ -727,6 +732,14 @@ static void block_job_event_completed(BlockJob *job, const
char *msg)
_abort);
}
+static void block_job_event_concluded(BlockJob *job)
+{
+if
