Re: [Qemu-devel] [RFC v4 08/21] blockjobs: add ABORTING state

[Kevin Wolf <kwolf@redhat.com>]

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