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

[Markus Armbruster]

John Snow <address@hidden> writes:

> On 7/24/19 12:47 AM, Markus Armbruster wrote:
>> John Snow <address@hidden> writes:
>> 
>>> From: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>
>>> 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 <address@hidden>
>>> [Added deprecation information. --js]
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>>  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
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.

> --js
>
>>>  @subsection query-cpus (since 2.12.0)
>>>  
>>>  The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.