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

[Kevin Wolf <kwolf@redhat.com>]

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

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. 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.

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

> > 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: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]