On 7/25/19 2:06 AM, Markus Armbruster wrote: > John Snow <jsnow@xxxxxxxxxx> writes: > >> On 7/24/19 12:47 AM, Markus Armbruster wrote: >>> John Snow <jsnow@xxxxxxxxxx> writes: >>> >>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> >>>> >>>> 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@xxxxxxxxxxxxx> >>>> [Added deprecation information. --js] >>>> Signed-off-by: John Snow <jsnow@xxxxxxxxxx> >>>> --- >>>> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list