Re: [Qemu-devel] [PATCH v3] qapi: add dirty-bitmaps to query-named-block-nodes result

[John Snow <jsnow@redhat.com>]

On 7/25/19 2:06 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 7/24/19 12:47 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>
>>>> Let's add a possibility to query dirty-bitmaps not only on root nodes.
>>>> It is useful when dealing both with snapshots and incremental backups.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> [Added deprecation information. --js]
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>  block/qapi.c         |  5 +++++
>>>>  qapi/block-core.json |  6 +++++-
>>>>  qemu-deprecated.texi | 12 ++++++++++++
>>>>  3 files changed, 22 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/block/qapi.c b/block/qapi.c
>>>> index 917435f022..15f1030264 100644
>>>> --- a/block/qapi.c
>>>> +++ b/block/qapi.c
>>>> @@ -79,6 +79,11 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>>>          info->backing_file = g_strdup(bs->backing_file);
>>>>      }
>>>>  
>>>> +    if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
>>>> +        info->has_dirty_bitmaps = true;
>>>> +        info->dirty_bitmaps = bdrv_query_dirty_bitmaps(bs);
>>>> +    }
>>>> +
>>>>      info->detect_zeroes = bs->detect_zeroes;
>>>>  
>>>>      if (blk && blk_get_public(blk)->throttle_group_member.throttle_state) {
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 0d43d4f37c..9210ae233d 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -360,6 +360,9 @@
>>>>  # @write_threshold: configured write threshold for the device.
>>>>  #                   0 if disabled. (Since 2.3)
>>>>  #
>>>> +# @dirty-bitmaps: dirty bitmaps information (only present if node
>>>> +#                 has one or more dirty bitmaps) (Since 4.2)
>>>> +#
>>>>  # Since: 0.14.0
>>>>  #
>>>>  ##
>>>> @@ -378,7 +381,7 @@
>>>>              '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
>>>>              '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
>>>>              '*iops_size': 'int', '*group': 'str', 'cache': 'BlockdevCacheInfo',
>>>> -            'write_threshold': 'int' } }
>>>> +            'write_threshold': 'int', '*dirty-bitmaps': ['BlockDirtyInfo'] } }
>>>>  
>>>>  ##
>>>>  # @BlockDeviceIoStatus:
>>>> @@ -656,6 +659,7 @@
>>>>  #
>>>>  # @dirty-bitmaps: dirty bitmaps information (only present if the
>>>>  #                 driver has one or more dirty bitmaps) (Since 2.0)
>>>> +#                 Deprecated in 4.2; see BlockDirtyInfo instead.
>>>>  #
>>>>  # @io-status: @BlockDeviceIoStatus. Only present if the device
>>>>  #             supports it and the VM is configured to stop on errors
>>>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
>>>> index c90b08d553..6374b66546 100644
>>>> --- a/qemu-deprecated.texi
>>>> +++ b/qemu-deprecated.texi
>>>> @@ -134,6 +134,18 @@ The ``status'' field of the ``BlockDirtyInfo'' structure, returned by
>>>>  the query-block command is deprecated. Two new boolean fields,
>>>>  ``recording'' and ``busy'' effectively replace it.
>>>>  
>>>> +@subsection query-block result field dirty-bitmaps (Since 4.2)
>>>> +
>>>> +The ``dirty-bitmaps`` field of the ``BlockInfo`` structure, returned by
>>>> +the query-block command is itself now deprecated. The ``dirty-bitmaps``
>>>> +field of the ``BlockDeviceInfo`` struct should be used instead, which is the
>>>> +type of the ``inserted`` field in query-block replies, as well as the
>>>> +type of array items in query-named-block-nodes.
>>>
>>> Would the text be clearer if it talked only about commands, not about
>>> types?
>>>
>>> Here's my (laconic) try:
>>>
>>>    @subsection query-block result field dirty-bitmaps (Since 4.2)
>>>
>>>    In the result of query-block, member ``dirty-bitmaps'' has been moved
>>>    into member ``inserted''.
>>>
>>
>> Yeah, that's probably better in terms of strictly what the deprecation
>> is. I was trying to imply that the output will also now be visible in
>> other commands as well, but that's not the deprecation -- that's the new
>> feature.
>>
>> ACK
>>
>>> Aside: same for existing @subsection query-block result field
>>> dirty-bitmaps[i].status (since 4.0).
>>>
>>
>> (Probably not worth editing deprecation text that was already published.)
> 
> Maybe, maybe not.  I'm not making demands.
> 
>>>> +Since the ``dirty-bitmaps`` field is optionally present in both the old and
>>>> +new locations, clients must use introspection to learn where to anticipate
>>>> +the field if/when it does appear in command output.
>>>> +
>>>
>>> I find this hint a bit confusing.  Do we need it?
>>>
>>
>> I think so, yes: it's nice to inform readers of how to cope with the
>> deprecation.
>>
>>> If yes, laconic me again:
>>>
>>>    Clients should use introspection to learn whether ``dirty-bitmaps'' is
>>>    in the new location.
>>>
>>
>> Too terse. I want my documentation to greet me in the morning by reading
>> me the local newspaper while I brush my teeth.
>>
>> Yours says the "how", but I think a hint should have the "why":
>>
>> "Since the ``dirty-bitmaps`` field is not always present in command
>> output, Clients should use introspection to learn the location of this
>> field."
> 
> This is clearer than the text in Vladimir's patch.  It made me

Now, now. That confusing text is entirely my own creation. Let's not
charge Vladimir with my error :)

> understand why you want to talk about optional.  See, I've been peddling
> the introspection kool-aid long enough to take "use introspection to
> detect interface changes" for granted.  The idea that anyone would try
> something like "if what query-block just gave me doesn't have
> dirty-bitmaps in the new location, look for it in the old location" just
> didn't come to me.
> 
> However, dirty-bitmaps being optional is *not* why you shouldn't do
> that!  In fact, doing it is not even wrong.  It only gets wrong when you
> do it wrongly.
> 
> Wrong: if what query-block just gave me doesn't have dirty-bitmaps in
> the new location, only look for it in the old location from now on.
> 
> Correct: if what query-block just gave me doesn't have dirty-bitmaps in
> the new location, look for it in the old location this time.  Next time,
> do the same: try the new location first, then the old location.
> 
> Also correct: if what query-block just gave me doesn't have
> dirty-bitmaps in the new location, look for it in the old location.
> Once I've found it in either location, keep looking for it only there in
> the future.  But why would I want to do that?  It's more complicated
> than the previous one for no gain.
> 
> Correct and preferred: use introspection.  You need to use it anyway to
> detect changes in arguments, so why do something else for changes in
> results.  Have some kool-aid!
> 
>> But I'm only willing to give you a self-deprecating joke and a final
>> nudge to keep a more informative hint, and then I'll capitulate and take
>> your suggestion if you give me a stern look.
> 
> No, I'm giving you a friendly "use your judgement" instead.  You may
> well be a better judge of what our users need here, because you're less
> deep into introspection than me, and so are our users.
> 

Aw, I was hoping you'd laugh.

I'll send a new patch, actually.

--js