Add a new state ABORTING.

This makes transitions from normative states to error states explicit
in the STM, and serves as a disambiguation for which states may complete
normally when normal end-states (CONCLUDED) are added in future commits.

Notably, Paused/Standby jobs do not transition directly to aborting,
as they must wake up first and cooperate in their cancellation.

Transitions:
Running -> Aborting: can be cancelled or encounter an error
Ready   -> Aborting: can be cancelled or encounter an error

Verbs:
None. The job must finish cleaning itself up and report its final status.

             +---------+
             |UNDEFINED|
             +--+------+
                |
             +--v----+
             |CREATED|
             +--+----+
                |
             +--v----+     +------+
   +---------+RUNNING<----->PAUSED|
   |         +--+----+     +------+
   |            |
   |         +--v--+       +-------+
   +---------+READY<------->STANDBY|
   |         +-----+       +-------+
   |
+--v-----+
|ABORTING|
+--------+

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockjob.c           | 31 ++++++++++++++++++-------------
 qapi/block-core.json |  7 ++++++-
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 4e424fef72..4c3fcda46c 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -44,22 +44,23 @@ 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 */
-    /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0, 0},
-    /* C: */ [BLOCK_JOB_STATUS_CREATED]   = {0, 0, 1, 0, 0, 0},
-    /* R: */ [BLOCK_JOB_STATUS_RUNNING]   = {0, 0, 0, 1, 1, 0},
-    /* P: */ [BLOCK_JOB_STATUS_PAUSED]    = {0, 0, 1, 0, 0, 0},
-    /* Y: */ [BLOCK_JOB_STATUS_READY]     = {0, 0, 0, 0, 0, 1},
-    /* S: */ [BLOCK_JOB_STATUS_STANDBY]   = {0, 0, 0, 0, 1, 0},
+                                          /* 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},
 };
 
 bool BlockJobVerbTable[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = {
-                                          /* U, C, R, P, Y, S */
-    [BLOCK_JOB_VERB_CANCEL]               = {0, 1, 1, 1, 1, 1},
-    [BLOCK_JOB_VERB_PAUSE]                = {0, 1, 1, 1, 1, 1},
-    [BLOCK_JOB_VERB_RESUME]               = {0, 1, 1, 1, 1, 1},
-    [BLOCK_JOB_VERB_SET_SPEED]            = {0, 1, 1, 1, 1, 1},
-    [BLOCK_JOB_VERB_COMPLETE]             = {0, 0, 0, 0, 1, 0},
+                                          /* 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},
 };
 
 static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
@@ -383,6 +384,10 @@ static void block_job_completed_single(BlockJob *job)
 {
     assert(job->completed);
 
+    if (job->ret || block_job_is_cancelled(job)) {
+        block_job_state_transition(job, BLOCK_JOB_STATUS_ABORTING);
+    }
+
     if (!job->ret) {
         if (job->driver->commit) {
             job->driver->commit(job);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 11659496c5..3f7d559fc0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -996,10 +996,15 @@
 # @standby: The job is ready, but paused. This is nearly identical to @paused.
 #           The job may return to @ready or otherwise be canceled.
 #
+# @aborting: The job is in the process of being aborted, and will finish with
+#            an error. The job will afterwards report that it is @concluded.
+#            This status may not be visible to the management process.
+#
 # Since: 2.12
 ##
 { 'enum': 'BlockJobStatus',
-  'data': ['undefined', 'created', 'running', 'paused', 'ready', 'standby'] }
+  'data': ['undefined', 'created', 'running', 'paused', 'ready', 'standby',
+           'aborting' ] }
 
 ##
 # @BlockJobInfo:
-- 
2.14.3

On 02/23/2018 05:51 PM, John Snow wrote:
> Add a new state ABORTING.
> 
> This makes transitions from normative states to error states explicit
> in the STM, and serves as a disambiguation for which states may complete
> normally when normal end-states (CONCLUDED) are added in future commits.
> 
> Notably, Paused/Standby jobs do not transition directly to aborting,
> as they must wake up first and cooperate in their cancellation.
> 
> Transitions:
> Running -> Aborting: can be cancelled or encounter an error
> Ready   -> Aborting: can be cancelled or encounter an error
> 
> Verbs:
> None. The job must finish cleaning itself up and report its final status.
> 
>               +---------+
>               |UNDEFINED|
>               +--+------+
>                  |
>               +--v----+
>               |CREATED|
>               +--+----+
>                  |
>               +--v----+     +------+
>     +---------+RUNNING<----->PAUSED|
>     |         +--+----+     +------+
>     |            |
>     |         +--v--+       +-------+
>     +---------+READY<------->STANDBY|
>     |         +-----+       +-------+
>     |
> +--v-----+
> |ABORTING|
> +--------+
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   blockjob.c           | 31 ++++++++++++++++++-------------
>   qapi/block-core.json |  7 ++++++-
>   2 files changed, 24 insertions(+), 14 deletions(-)
> 

> +++ b/qapi/block-core.json
> @@ -996,10 +996,15 @@
>   # @standby: The job is ready, but paused. This is nearly identical to @paused.
>   #           The job may return to @ready or otherwise be canceled.
>   #
> +# @aborting: The job is in the process of being aborted, and will finish with
> +#            an error. The job will afterwards report that it is @concluded.

@concluded isn't defined yet, but I'm okay with the minor future 
reference, as it's less churn to get to the description that works at 
the end of the series.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Am 24.02.2018 um 00:51 hat John Snow geschrieben:
> Add a new state ABORTING.
> 
> This makes transitions from normative states to error states explicit
> in the STM, and serves as a disambiguation for which states may complete
> normally when normal end-states (CONCLUDED) are added in future commits.
> 
> Notably, Paused/Standby jobs do not transition directly to aborting,
> as they must wake up first and cooperate in their cancellation.
> 
> Transitions:
> Running -> Aborting: can be cancelled or encounter an error
> Ready   -> Aborting: can be cancelled or encounter an error
> 
> Verbs:
> None. The job must finish cleaning itself up and report its final status.
> 
>              +---------+
>              |UNDEFINED|
>              +--+------+
>                 |
>              +--v----+
>              |CREATED|
>              +--+----+
>                 |
>              +--v----+     +------+
>    +---------+RUNNING<----->PAUSED|
>    |         +--+----+     +------+
>    |            |
>    |         +--v--+       +-------+
>    +---------+READY<------->STANDBY|
>    |         +-----+       +-------+
>    |
> +--v-----+
> |ABORTING|
> +--------+
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

> @@ -383,6 +384,10 @@ static void block_job_completed_single(BlockJob *job)
>  {
>      assert(job->completed);
>  
> +    if (job->ret || block_job_is_cancelled(job)) {
> +        block_job_state_transition(job, BLOCK_JOB_STATUS_ABORTING);
> +    }
> +
>      if (!job->ret) {
>          if (job->driver->commit) {
>              job->driver->commit(job);

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

But I do have a question about the code below the new lines: I wonder
where job->ret is set to an error value? I can clearly see that the job
is marked as cancelled, so your added code should be working, but
looking at the block job code, it looks to me as if the jobs were
completing with job->cancelled = true and job->ret = 0.

For block_job_completed_single(), this means that we would call the
.commit callback and .cb with a success return code, while sending a
CANCELLED event on QMP.

Of course, the only job type that supports transactions is the backup
job, and that one seems to compensate for this by explicitly checking
block_job_is_cancelled() in its .commit implementation, but is that
really how things should be working?

Kevin

On 02/28/2018 09:54 AM, Kevin Wolf wrote:
> Am 24.02.2018 um 00:51 hat John Snow geschrieben:
>> Add a new state ABORTING.
>>
>> This makes transitions from normative states to error states explicit
>> in the STM, and serves as a disambiguation for which states may complete
>> normally when normal end-states (CONCLUDED) are added in future commits.
>>
>> Notably, Paused/Standby jobs do not transition directly to aborting,
>> as they must wake up first and cooperate in their cancellation.
>>
>> Transitions:
>> Running -> Aborting: can be cancelled or encounter an error
>> Ready   -> Aborting: can be cancelled or encounter an error
>>
>> Verbs:
>> None. The job must finish cleaning itself up and report its final status.
>>
>>              +---------+
>>              |UNDEFINED|
>>              +--+------+
>>                 |
>>              +--v----+
>>              |CREATED|
>>              +--+----+
>>                 |
>>              +--v----+     +------+
>>    +---------+RUNNING<----->PAUSED|
>>    |         +--+----+     +------+
>>    |            |
>>    |         +--v--+       +-------+
>>    +---------+READY<------->STANDBY|
>>    |         +-----+       +-------+
>>    |
>> +--v-----+
>> |ABORTING|
>> +--------+
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
>> @@ -383,6 +384,10 @@ static void block_job_completed_single(BlockJob *job)
>>  {
>>      assert(job->completed);
>>  
>> +    if (job->ret || block_job_is_cancelled(job)) {
>> +        block_job_state_transition(job, BLOCK_JOB_STATUS_ABORTING);
>> +    }
>> +
>>      if (!job->ret) {
>>          if (job->driver->commit) {
>>              job->driver->commit(job);
> 
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> 
> But I do have a question about the code below the new lines: I wonder
> where job->ret is set to an error value? I can clearly see that the job
> is marked as cancelled, so your added code should be working, but
> looking at the block job code, it looks to me as if the jobs were
> completing with job->cancelled = true and job->ret = 0.
> 
> For block_job_completed_single(), this means that we would call the
> .commit callback and .cb with a success return code, while sending a
> CANCELLED event on QMP.
> 
> Of course, the only job type that supports transactions is the backup
> job, and that one seems to compensate for this by explicitly checking
> block_job_is_cancelled() in its .commit implementation, but is that
> really how things should be working?
> 
> Kevin
> 

You re-discovered the same nonsensical thing that I discovered, which
led to patch 12.