Re: [Qemu-devel] [PATCH 39/42] job: Add lifecycle QMP commands

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5126 bytes --]

On 2018-05-15 16:08, Kevin Wolf wrote:
> Am 15.05.2018 um 00:31 hat Max Reitz geschrieben:
>> On 2018-05-09 18:26, Kevin Wolf wrote:
>>> This adds QMP commands that control the transition between states of the
>>> job lifecycle.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  qapi/job.json | 112 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  job-qmp.c     | 140 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  MAINTAINERS   |   1 +
>>>  Makefile.objs |   2 +-
>>>  trace-events  |   9 ++++
>>>  5 files changed, 263 insertions(+), 1 deletion(-)
>>>  create mode 100644 job-qmp.c
>>>
>>> diff --git a/qapi/job.json b/qapi/job.json
>>> index bd88a358d0..7b84158292 100644
>>> --- a/qapi/job.json
>>> +++ b/qapi/job.json
>>> @@ -63,3 +63,115 @@
>>>  { 'event': 'JOB_STATUS_CHANGE',
>>>    'data': { 'id': 'str',
>>>              'status': 'JobStatus' } }
>>> +
>>> +##
>>> +# @job-pause:
>>> +#
>>> +# Pause an active job.
>>> +#
>>> +# This command returns immediately after marking the active job for pausing.
>>> +# Pausing an already paused job has no cumulative effect; a single job-resume
>>> +# command will resume the job.
>>
>> Pausing an already paused job is, in fact, an error.
>>
>> (Which should be noted here instead of making it appear like it'd be
>> idempotent.)
> 
> Aye. Interestingly, I managed to fix it below in job-resume, but missed
> it here. I should also stick in a patch somewhere early in the series
> that fixes the documentation of block-job-pause/resume.
> 
>>> +##
>>> +# @job-cancel:
>>> +#
>>> +# Instruct an active background job to cancel at the next opportunity.
>>> +# This command returns immediately after marking the active job for
>>> +# cancellation.
>>> +#
>>> +# The job will cancel as soon as possible and then emit a JOB_STATUS_CHANGE
>>> +# event. Usually, the status will change to ABORTING, but it is possible that
>>> +# a job successfully completes (e.g. because it was almost done and there was
>>> +# no opportunity to cancel earlier than completing the job) and transitions to
>>> +# PENDING instead.
>>> +#
>>> +# Note that if you issue 'job-cancel' after a mirror block job has indicated
>>> +# (via the event BLOCK_JOB_READY, and by transitioning into the READY state)
>>> +# that the source and destination are synchronized, then the job always
>>> +# completes successfully and transitions to PENDING as well as triggering the
>>> +# event BLOCK_JOB_COMPLETED, to indicate that the mirroring has ended and the
>>> +# destination now has a point-in-time copy tied to the time of the
>>> +# cancellation.
>>> +#
>>> +# @id: The job identifier.
>>> +#
>>> +# @force: If true, and the job is already in the READY state, abandon the job
>>> +#         immediately (even if it is paused) instead of waiting for the
>>> +#         destination to complete its final synchronization
>>
>> The note on "final synchronization" is extremely mirror-specific.  I see
>> three choices here:
>>
>> (1) If mirror stays the only job with this special cancel semantics,
>> then we are free to note that this is a mirror-specific flag here.
>>
>> (2) Even if some other job might come along at some point where use of
>> @force may make sense, that doesn't stop us from now noting that only
>> mirror supports this, which helps readers understand what "destination"
>> and "final synchronization" mean.
>>
>> (Yes, so (1) and (2) are basically the same.)
>>
>> (3) We try to find some general description and drop the last part.
>> Like "If a job would normally decide to complete instead of actually
>> aborting, this flag can be used to convince it otherwise."  But that's
>> so handwavy, I'd rather just mark it as a special mirror flag for now.
> 
> Or how about this one?
> 
> (4) Mirror is really abusing cancel for a second completion mode and we
>     don't want to have this kind of ugliness in job-cancel.

Yeah, well, right, that was implicit and I tried to be more diplomatic
by calling it "special semantics". :-)

>                                                             Remove
>     @force from the schema and internally always use force=true. For
>     now, block-job-cancel does the job (no pun intended) for mirror, and
>     maybe we can later introduce a way to select completion mode with
>     job-complete.

Sounds good to me.

> This would also get us rid of that whole long paragraph above that
> explains how mirror jobs have an exceptional behaviour.

Yes.

Max

>>> diff --git a/Makefile.objs b/Makefile.objs
>>> index 3df8d58e49..253e0356f3 100644
>>> --- a/Makefile.objs
>>> +++ b/Makefile.objs
>>> @@ -66,7 +66,7 @@ chardev-obj-y = chardev/
>>>  # block-obj-y is code used by both qemu system emulation and qemu-img
>>>  
>>>  block-obj-y += nbd/
>>> -block-obj-y += block.o blockjob.o job.o
>>> +block-obj-y += block.o blockjob.o job.o job-qmp.o
>>
>> Shouldn't this be in common-obj-y like blockdev?
> 
> Seems to build with that change, so it can't be wrong...
> 
> Kevin
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]