Re: [Qemu-devel] [PATCH v3 for-2.11] QAPI & interop: Clarify events emitted by 'block-job-cancel'

[Kashyap Chamarthy]

On Fri, Nov 17, 2017 at 11:07:41AM -0600, Eric Blake wrote:
> On 11/17/2017 10:25 AM, Kashyap Chamarthy wrote:

[...]

> Sorry for not wordsmithing on earlier versions:

That's fine.  Always appreciate your wordsmithing :-)

> > +'mirror' job *after* it has indicated (by emitting the event
> > +``BLOCK_JOB_READY``) that the source and target now remain
> > +synchronized, then ``block-job-cancel`` will emit the event
> 
> Might read slightly nicer as:
> 
> the source and target have reached synchronization,

Ah, that sounds nicer indeed.  Will fix.

[...]

> > +# Note that the 'block-job-cancel' command will emit the event
> > +# BLOCK_JOB_COMPLETED if you issue it ('block-job-cancel') after 
> > 'drive-mirror'
> > +# has indicated (by emitting the event BLOCK_JOB_READY) that the source and
> > +# destination remain synchronized.  In this case, the BLOCK_JOB_COMPLETED
> > +# event indicates that synchronization (from 'drive-mirror') has 
> > successfully
> > +# ended and the destination now has a point-in-time copy, which is at the 
> > time
> > +# of cancel.
> 
> 
> Accurate, but a bit hard to follow the flow of the sentence.

Yeah, I wasn't satisfied with my phrasing too.  I sat on it for nearly
half an hour, and still it came out super clunky.

> Might read nicer as:
> 
> Note that if you issue 'block-job-cancel' after 'drive-mirror' has
> indicated (via the event BLOCK_JOB_READY) that the source and
> destination are synchronized, then the event triggered by this command
> changes to 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.

Thanks, yours reads much nicer.

> Documentation is worth of inclusion in 2.11.  Whether you keep your
> wording, or incorporate mine in a v4, you can add:
> Reviewed-by: Eric Blake 

I'll definitely incorporate in v4.  

I'll also make similar wording adjustment in live-block-operations.rst.

Thanks for the quick review.

-- 
/kashyap

Re: [Qemu-devel] [PATCH v3 for-2.11] QAPI & interop: Clarify events emitted by 'block-job-cancel'

[Eric Blake]

On 11/17/2017 10:25 AM, Kashyap Chamarthy wrote:
> When you cancel an in-progress live block operation with QMP
> `block-job-cancel`, it emits the event: BLOCK_JOB_CANCELLED.  However,
> when `block-job-cancel` is issued after `drive-mirror` has indicated (by
> emitting the event BLOCK_JOB_READY) that the source and destination
> remain synchronized:
> 

> But this is expected behaviour, where the _COMPLETED event indicates
> that synchronization has successfully ended (and the destination has a
> point-in-time copy, which is at the time of cancel).
> 
> So add a small note to this effect in 'block-core.json'.  While at it,
> also update the "Live disk synchronization -- drive-mirror and
> blockdev-mirror" section in 'live-block-operations.rst'.
> 
> (Thanks: Max Reitz for reminding me of this caveat on IRC.)

Sorry for not wordsmithing on earlier versions:

> 
> Signed-off-by: Kashyap Chamarthy 
> ---

> +.. note::
> +
> +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 (by emitting the event
> +``BLOCK_JOB_READY``) that the source and target now remain
> +synchronized, then ``block-job-cancel`` will emit the event

Might read slightly nicer as:

the source and target have reached synchronization,


> +++ b/qapi/block-core.json
> @@ -2065,6 +2065,14 @@
>  # BLOCK_JOB_CANCELLED event.  Before that happens the job is still visible 
> when
>  # enumerated using query-block-jobs.
>  #
> +# Note that the 'block-job-cancel' command will emit the event
> +# BLOCK_JOB_COMPLETED if you issue it ('block-job-cancel') after 
> 'drive-mirror'
> +# has indicated (by emitting the event BLOCK_JOB_READY) that the source and
> +# destination remain synchronized.  In this case, the BLOCK_JOB_COMPLETED
> +# event indicates that synchronization (from 'drive-mirror') has successfully
> +# ended and the destination now has a point-in-time copy, which is at the 
> time
> +# of cancel.


Accurate, but a bit hard to follow the flow of the sentence.  Might read
nicer as:

Note that if you issue 'block-job-cancel' after 'drive-mirror' has
indicated (via the event BLOCK_JOB_READY) that the source and
destination are synchronized, then the event triggered by this command
changes to 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.

Documentation is worth of inclusion in 2.11.  Whether you keep your
wording, or incorporate mine in a v4, you can add:
Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature