[libvirt] [PATCH 2/2] qapi: deprecate implicit filters

[John Snow]

(Peter: search for "pkrempa" down below.)

On 8/28/19 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> 27.08.2019 23:12, John Snow wrote:
>>
>>
>> On 8/23/19 5:22 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 14.08.2019 13:07, Vladimir Sementsov-Ogievskiy wrote:
>>>> To get rid of implicit filters related workarounds in future let's
>>>> deprecate them now.
>>>
>>> Interesting, could we deprecate implicit filter without deprecation of unnecessity of
>>> parameter? As actually, it's good when this parameter is not necessary, in most cases
>>> user is not interested in node-name.
>>>
>>
>> https://en.wiktionary.org/wiki/unnecessity -- I am surprised to learn
>> that this a real word in the language I speak. :)
>>
>> I assume you're referring to making the optional argument mandatory.
> 
> exactly, it's my a bit "google-translate-driven" English)
> 

It teaches me some fun words!

>>
>>> Obviously we can do the following:
>>>
>>> 1. In 4.2 we deprecate unnecessity, which implies deprecation of implicit filters
>>> 2. After some releases in 4.x we can drop deprecated functionality, so we drop it together with
>>> implicit filters. And, in same release 4.x we return it back (as it's compatible change :)
>>> but without implicit filters (so, if filter-node-name not specified, we just create
>>> explicit filter with autogenerated node-name)
>>>
>>> So, effectively we just drop "deprecation mark" together with implicit filters, which is nice
>>> but actually confusing.
>>>
>>> Instead, we may do
>>> 1. In 4.2 deprecate
>>> 2. In 4.x drop optionality together with implicit filters
>>> 3. In 4.y (y > x of course) return optionality back
>>>
>>
>> Ah, I see what you're digging at here now...
>>
>>> It's a bit safer, but for users who miss releases [4.x, 4.y) it's no difference..
>>>
>>> Or we just write in spec, that implicit filters are deprecated? But we have nothing about implicit
>>> filters in spec. More over, we directly write that we have filter, and if parameter is omitted
>>> it's node-name is autogenerated. So actually, the fact the filter is hidden when filter-node-name is
>>> unspecified is _undocumented_.
>>>
>>> So, finally, it looks like nothing to deprecated in specification, we can just drop implicit filters :)
>>>
>>> What do you think?
>>>
>>
>> What exactly _IS_ an implicit filter? How does it differ today from an
>> explicit filter? I assumed the only difference was if it was named or
>> not; but I think I must be mistaken now if you're proposing leaving the
>> interface alone entirely.
>>
>> Are they instantiated differently?
>>
> 
> As I understand, the only difference is their BlockDriverState.impicit field, and several places in code
> where we skip implicit filter when trying to find something in a chain starting from a device.
> 

Oh, oh, yes. I see.

> Hmm, OK, let's see:
> 
> 1. the only implicit filters are commit_top and mirror_top if user don't specify filter-node-name.
> 
> Where it make sense, i.e., where implicit field used?
> 

`git grep -E '(->|\.)implicit'` is what I used to find usages.

> 2. bdrv_query_info, bdrv_query_bds_stats, bdrv_block_device_info(only when called from bdrv_query_info), they'll
> report filter as top node if we don't mark it implicit.
> 

So that's a bit of a change, but only visually. The "reality" is still
the same, we just report it more "accurately." libvirt MIGHT need a
heads up here. I'm looping pkrempa back in for comment.

<pkrempa>
Would libvirt be negatively impacted by the revelation of formerly
internal ("implicit") nodes created by mirror and commit via query block
commands? At the moment, QEMU hides them from you if you do not name them.
</pkrempa>

> 3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate:
>    I think it's not a problem, just drop special case for implicit fitlers
>

I'm much less certain about what the impact of this would be and would
need to audit it (and don't have the time to, personally.)

Do you have a POC or RFC patch that demonstrates dropping these special
cases? It might be nice to see as proof that it's safe to deprecate.

> So, seems the only real change is query-block and query-blockstats output when mirror or commit is started
> without specifying filter-node-name (filter would be on top)
> 
> So, how should we deprecate this, or can we just change it?
> 

I'm not sure if it's worth it yet, what does dropping the implicit field
buy us? Conceptually I understand that it's simpler without the notion
of implicit fields, but I imagine there's some cleanup in particular
that motivated this.

I'd say to just change the behavior, we should:

- Give a standard three-release warning that the behavior will change in
an incompatible way
- Demonstrate with an RFC patch that special cases around ->implicit in
block.c can be removed and do not make the code more complex,
- Get blessings from Peter Krempa.

As always: Libvirt is not the end-all be-all of QEMU management, but if
libvirt is capable of working around design changes then I believe any
project out there today also could, so it's a good litmus test.

--js