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 <js...@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 <kw...@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.