[Qemu-devel] [PATCH v2] qapi: block-core: Clarify events emitted by 'block-job-cancel'

[Qemu-devel] [PATCH v2] qapi: block-core: Clarify events emitted by 'block-job-cancel'

[Kashyap Chamarthy]

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:

    [...] # Snip `drive-mirror` invocation & outputs
    {
      "execute":"block-job-cancel",
      "arguments":{
        "device":"virtio0"
      }
    }

    {"return": {}}

It (`block-job-cancel`) will counterintuitively emit the event
'BLOCK_JOB_COMPLETED':

    {
      "timestamp":{
        "seconds":1510678024,
        "microseconds":526240
      },
      "event":"BLOCK_JOB_COMPLETED",
      "data":{
        "device":"virtio0",
        "len":41126400,
        "offset":41126400,
        "speed":0,
        "type":"mirror"
      }
    }

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.  (Thanks: Max Reitz for reminding
me of this on IRC.)

Signed-off-by: Kashyap Chamarthy <kchamart@redhat.com>
---
Changes in v2:
 - "Note:" seems to be a special construct in Patchew, my usage caused a
    build failure.  So do: s/Note:/Note that/
 - Add the missing 'Signed-off-by'
---
 qapi/block-core.json | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index ab96e348e6317bb42769ae20f4a4519bac02e93a..4ecfd1fbc5bc231c69da15d489c44e3e8b0706a5 100644
--- a/qapi/block-core.json
+++ 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.
+#
 # For streaming, the image file retains its backing file unless the streaming
 # operation happens to complete just as it is being cancelled.  A new streaming
 # operation can be started at a later time to finish copying all data from the
-- 
2.9.5

Re: [Qemu-devel] [Qemu-block] [PATCH v2] qapi: block-core: Clarify events emitted by 'block-job-cancel'

[John Snow]

On 11/15/2017 04:09 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:
> 
>     [...] # Snip `drive-mirror` invocation & outputs
>     {
>       "execute":"block-job-cancel",
>       "arguments":{
>         "device":"virtio0"
>       }
>     }
> 
>     {"return": {}}
> 
> It (`block-job-cancel`) will counterintuitively emit the event
> 'BLOCK_JOB_COMPLETED':
> 
>     {
>       "timestamp":{
>         "seconds":1510678024,
>         "microseconds":526240
>       },
>       "event":"BLOCK_JOB_COMPLETED",
>       "data":{
>         "device":"virtio0",
>         "len":41126400,
>         "offset":41126400,
>         "speed":0,
>         "type":"mirror"
>       }
>     }
> 
> 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.  (Thanks: Max Reitz for reminding
> me of this on IRC.)
> 

I suppose this difference probably isn't covered in what was the
bitmaps.md doc file (we don't bother covering mirror there, only
backup); is it covered sufficiently in live-block-operations.rst ?

--js

Re: [Qemu-devel] [Qemu-block] [PATCH v2] qapi: block-core: Clarify events emitted by 'block-job-cancel'

[Kashyap Chamarthy]

On Wed, Nov 15, 2017 at 02:15:57PM -0500, John Snow wrote:
> 
> 
> On 11/15/2017 04:09 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.  (Thanks: Max Reitz for reminding
> > me of this on IRC.)
> > 
> 
> I suppose this difference probably isn't covered in what was the
> bitmaps.md doc file (we don't bother covering mirror there, only
> backup); 

Your supposition is correct: Looking at the now-renamed
'bitmaps.rst'[1], it isn't covered there.

> is it covered sufficiently in live-block-operations.rst ?

I looked in there[2] too.  Short answer: no.  Long: In the "Live disk
synchronization — drive-mirror and blockdev-mirror" section, I simply
seemed to declare: 

    "Issuing the command ``block-job-cancel`` after it emits the event
    ``BLOCK_JOB_CANCELLED``"

As if that's the *only* event it emits, which is clearly not the case.
So while at it, wonder if should I also update it
('live-block-operations.rst') too.

[1] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/bitmaps.rst
[2] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/live-block-operations.rst#l513 

-- 
/kashyap

Re: [Qemu-devel] [Qemu-block] [PATCH v2] qapi: block-core: Clarify events emitted by 'block-job-cancel'

[John Snow]

On 11/15/2017 04:54 PM, Kashyap Chamarthy wrote:
> On Wed, Nov 15, 2017 at 02:15:57PM -0500, John Snow wrote:
>>
>>
>> On 11/15/2017 04:09 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.  (Thanks: Max Reitz for reminding
>>> me of this on IRC.)
>>>
>>
>> I suppose this difference probably isn't covered in what was the
>> bitmaps.md doc file (we don't bother covering mirror there, only
>> backup); 
> 
> Your supposition is correct: Looking at the now-renamed
> 'bitmaps.rst'[1], it isn't covered there.
> 

Makes sense, I don't think we need to correct this, and

>> is it covered sufficiently in live-block-operations.rst ?
> 
> I looked in there[2] too.  Short answer: no.  Long: In the "Live disk
> synchronization — drive-mirror and blockdev-mirror" section, I simply
> seemed to declare: 
> 
>     "Issuing the command ``block-job-cancel`` after it emits the event
>     ``BLOCK_JOB_CANCELLED``"
> 
> As if that's the *only* event it emits, which is clearly not the case.
> So while at it, wonder if should I also update it
> ('live-block-operations.rst') too.
> 

It's an interesting gotcha that I wasn't really acutely aware of myself,
so having it in the doc format for API programmers who aren't
necessarily digging through our source sounds like a pleasant courtesy.

> [1] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/bitmaps.rst
> [2] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/live-block-operations.rst#l513 
> 

Thanks Kashyap :)

Re: [Qemu-devel] [Qemu-block] [PATCH v2] qapi: block-core: Clarify events emitted by 'block-job-cancel'

[Kashyap Chamarthy]

On Wed, Nov 15, 2017 at 04:56:13PM -0500, John Snow wrote:
> 
> 
> On 11/15/2017 04:54 PM, Kashyap Chamarthy wrote:
> > On Wed, Nov 15, 2017 at 02:15:57PM -0500, John Snow wrote:

[...]

> >> is it covered sufficiently in live-block-operations.rst ?
> > 
> > I looked in there[2] too.  Short answer: no.  Long: In the "Live disk
> > synchronization — drive-mirror and blockdev-mirror" section, I simply
> > seemed to declare: 
> > 
> >     "Issuing the command ``block-job-cancel`` after it emits the event
> >     ``BLOCK_JOB_CANCELLED``"
> > 
> > As if that's the *only* event it emits, which is clearly not the case.
> > So while at it, wonder if should I also update it
> > ('live-block-operations.rst') too.
> > 
> 
> It's an interesting gotcha that I wasn't really acutely aware of myself,
> so having it in the doc format for API programmers who aren't
> necessarily digging through our source sounds like a pleasant courtesy.

Indeed, will do.  (Just for my own clarity, did you imply: don't update
it in block-core.json?  FWIW, my first instinct is to check the QAPI
documentation for such things, that's why I wrote there first :-))

Thanks for looking.

[...]

-- 
/kashyap

Re: [Qemu-devel] [Qemu-block] [PATCH v2] qapi: block-core: Clarify events emitted by 'block-job-cancel'

[John Snow]

On 11/16/2017 04:14 AM, Kashyap Chamarthy wrote:
> On Wed, Nov 15, 2017 at 04:56:13PM -0500, John Snow wrote:
>>
>>
>> On 11/15/2017 04:54 PM, Kashyap Chamarthy wrote:
>>> On Wed, Nov 15, 2017 at 02:15:57PM -0500, John Snow wrote:
> 
> [...]
> 
>>>> is it covered sufficiently in live-block-operations.rst ?
>>>
>>> I looked in there[2] too.  Short answer: no.  Long: In the "Live disk
>>> synchronization — drive-mirror and blockdev-mirror" section, I simply
>>> seemed to declare: 
>>>
>>>     "Issuing the command ``block-job-cancel`` after it emits the event
>>>     ``BLOCK_JOB_CANCELLED``"
>>>
>>> As if that's the *only* event it emits, which is clearly not the case.
>>> So while at it, wonder if should I also update it
>>> ('live-block-operations.rst') too.
>>>
>>
>> It's an interesting gotcha that I wasn't really acutely aware of myself,
>> so having it in the doc format for API programmers who aren't
>> necessarily digging through our source sounds like a pleasant courtesy.
> 
> Indeed, will do.  (Just for my own clarity, did you imply: don't update
> it in block-core.json?  FWIW, my first instinct is to check the QAPI
> documentation for such things, that's why I wrote there first :-))
> 

No, definitely update both.

> Thanks for looking.
> 
> [...]
> 

Re: [Qemu-devel] [Qemu-block] [PATCH v2] qapi: block-core: Clarify events emitted by 'block-job-cancel'

[Kevin Wolf]

Am 16.11.2017 um 10:14 hat Kashyap Chamarthy geschrieben:
> On Wed, Nov 15, 2017 at 04:56:13PM -0500, John Snow wrote:
> > 
> > 
> > On 11/15/2017 04:54 PM, Kashyap Chamarthy wrote:
> > > On Wed, Nov 15, 2017 at 02:15:57PM -0500, John Snow wrote:
> 
> [...]
> 
> > >> is it covered sufficiently in live-block-operations.rst ?
> > > 
> > > I looked in there[2] too.  Short answer: no.  Long: In the "Live disk
> > > synchronization — drive-mirror and blockdev-mirror" section, I simply
> > > seemed to declare: 
> > > 
> > >     "Issuing the command ``block-job-cancel`` after it emits the event
> > >     ``BLOCK_JOB_CANCELLED``"
> > > 
> > > As if that's the *only* event it emits, which is clearly not the case.
> > > So while at it, wonder if should I also update it
> > > ('live-block-operations.rst') too.
> > > 
> > 
> > It's an interesting gotcha that I wasn't really acutely aware of myself,
> > so having it in the doc format for API programmers who aren't
> > necessarily digging through our source sounds like a pleasant courtesy.
> 
> Indeed, will do.  (Just for my own clarity, did you imply: don't update
> it in block-core.json?  FWIW, my first instinct is to check the QAPI
> documentation for such things, that's why I wrote there first :-))

Will that be a separate patch or do you intend to send a v3?

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v2] qapi: block-core: Clarify events emitted by 'block-job-cancel'

[Kashyap Chamarthy]

On Fri, Nov 17, 2017 at 11:49:05AM +0100, Kevin Wolf wrote:
> Am 16.11.2017 um 10:14 hat Kashyap Chamarthy geschrieben:
> > On Wed, Nov 15, 2017 at 04:56:13PM -0500, John Snow wrote:

[...]

> > > It's an interesting gotcha that I wasn't really acutely aware of myself,
> > > so having it in the doc format for API programmers who aren't
> > > necessarily digging through our source sounds like a pleasant courtesy.
> > 
> > Indeed, will do.  (Just for my own clarity, did you imply: don't update
> > it in block-core.json?  FWIW, my first instinct is to check the QAPI
> > documentation for such things, that's why I wrote there first :-))
> 
> Will that be a separate patch or do you intend to send a v3?

I can send a separate one if you prefer, but I was intending to send a
v3 with both files fixed as it's just a related trivial change.  Is that
okay?

-- 
/kashyap