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

[John Snow]

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

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

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.

--js

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