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