Re: [Qemu-devel] [Qemu-block] [PATCH v2 21/40] job: Convert block_job_cancel_async() to Job

[John Snow <jsnow@redhat.com>]

On 05/29/2018 08:30 AM, Max Reitz wrote:
> On 2018-05-29 13:59, Kashyap Chamarthy wrote:
>> On Fri, May 25, 2018 at 10:00:35AM +0200, Kevin Wolf wrote:
>>> Am 24.05.2018 um 19:42 hat John Snow geschrieben:
>>
>> [...]
>>
>> (Randomly chiming in for a small clarification.)
>>
>>>>>> or some other mechanism that accomplishes the same type of behavior. It
>>>>>> would be nice if it did not have to be determined at job creation time
>>>>>> but instead could be determined later.
>>>>>
>>>>> I agree. We already made sure that job-cancel really means cancel on the
>>>>> QAPI level, so we're free to do that. We just need to keep supporting
>>>>> block-job-cancel with the old semantics, so what I have is the easy
>>>>> conversion. We can change the internal implementation when we actually
>>>>> implement the selection of a completion mode.
>>>>>
>>>>> Kevin
>>>>>
>>>>
>>>> We need this before 3.0 though, yeah? unless we make job-cancel
>>>> x-job-cancel or some other warning that the way it works might change, yeah?
>>>>
>>>> Or do I misunderstand our leeway to change this at a later point in time?
>>>>
>>>> (can job-cancel apply to block jobs created with the legacy
>>>> infrastructure? My reading was "yes.")
>>>
>>> It can, and it already has its final semantics, so nothing has to change
>>> before 3.0. job-cancel is equivalent to block-job-cancel with fixed
>>> force=true. If you want the complete-by-cancel behaviour of mirror, you
>>> have to use block-job-cancel for now, because job-cancel doesn't provide
>>> that functionality.
>>
>> I think by "complete-by-cancel" you are referring to this[*] magic
>> behaviour, mentioned in the last sentence here:
>>
>>     When you cancel an in-progress ‘mirror’ job before the source and
>>     target are synchronized, block-job-cancel will emit the event
>>     BLOCK_JOB_CANCELLED. However, note that if you cancel a ‘mirror’ job
>>     after it has indicated (via the event BLOCK_JOB_READY) that the
>>     source and target have reached synchronization, then the event
>>     emitted by block-job-cancel changes to BLOCK_JOB_COMPLETED.
>>
>>
>> [*] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/live-block-operations.rst#l515
>>
>>> So what we can change later is adding a way to initiate this secondary
>>> completion mode with a job-* command (probably with a new option for
>>> job-complete). But we wouldn't change the semantics of exisiting
>>> commands.
>>
>> Ah, so if I'm understanding it correctly, the existing subtle magic of
>> "complete-by-cancel" will be rectified by separting the two distinct
>> operations: 'job-cancel' and 'job-complete'.
> 
> Not really, because we already had those two operations for block jobs.
> The thing is that block-job-complete (and thus job-complete) will pivot
> to the target disk, but block-job-cancel doesn't.
> 
> The special behavior is that you can use block-job-cancel after
> BLOCK_JOB_READY to complete the job, but not pivot to it.  I don't think
> we have a real plan on how to represent that with the generic job
> commands, we just know that we don't want to use job-cancel.
> 
> (Maybe we can add a flag to job-complete (which to me does not sound
> like a good idea), or you could set flags on jobs while they are
> running, so you can set a do-not-pivot flag on the mirror job before you
> complete it.)
> 
> Max
> 

Adding the completion-mode as a "setting" to the job has always been my
favorite way. Whether you set this setting at creation time or later
during runtime is not important.

When we issue job-complete, the job knows based on its own settings the
way it should do so, if there are multiple possibilities.

I only mentioned it as a flag to complete ... err, just in case it was
easier that way somehow. *shrug*.