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

[John Snow]

On 7/17/19 3:13 PM, Eric Blake wrote:
> On 7/17/19 12:39 PM, John Snow wrote:
>> 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(-)
> 
>> +++ 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)
>> +#
> 
> Naming-wise, everything else in this struct uses 'foo_bar' while your
> addition uses 'foo-bar'.  But at this point, I don't know if it's worth
> uglifying this addition just to fit in.
> 
>>  # 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.
> 
> s/BlockDirtyInfo/BlockDeviceInfo/
> 
> With the spelling fix,
> 

Sigh, oops.

> Reviewed-by: Eric Blake <address@hidden>
> 
> Is this worth squeezing into 4.1, to start the deprecation clock one
> cycle earlier (on the grounds that the missing information for anonymous
> nodes is a bug)?  Or am I pushing the boundaries too far, where keeping
> this as 4.2 material remains the best course of action?
> 

Appealing option. If you think the deprecation plan is actionable enough
for libvirt, I'm in favor.

--js