Re: [PATCH RESEND] hostmem: Don't report pmem attribute if unsupported

[John Snow <jsnow@redhat.com>]

On 2/17/21 2:31 AM, Michal Privoznik wrote:
> On 2/17/21 12:07 AM, John Snow wrote:
>> On 2/16/21 5:23 PM, Eduardo Habkost wrote:
>>> On Tue, Jan 26, 2021 at 08:48:25AM +0100, Michal Privoznik wrote:
>>>> When management applications (like Libvirt) want to check whether
>>>> memory-backend-file.pmem is supported they can list object
>>>> properties using 'qom-list-properties'. However, 'pmem' is
>>>> declared always (and thus reported always) and only at runtime
>>>> QEMU errors out if it was built without libpmem (and thus can not
>>>> guarantee write persistence). This is suboptimal since we have
>>>> ability to declare attributes at compile time.
>>>>
>>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1915216
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>>
>>> I'm not a fan of making availability of properties conditional
>>> (even if at compile time), but if this helps libvirt I guess it
>>> makes sense.
>>>
>>
>> Compile time might be OK, but if we want to describe everything via 
>> QAPI eventually, we just need to be able to describe that compile-time 
>> requisite appropriately.
>>
>> Conditional at run-time is I think the thing that absolutely has to go 
>> wherever it surfaces.
> 
> I'm open for discussion. How do you think libvirt (or any other mgmt 
> tool/user) should inspect whether given feature is available?
> What libvirt currently does it issues 'qom-list-properties' with 
> 'typename=memory-backend-file' and inspects whether pmem attribute is 
> available. Is 'qom-list' preferred?
> 
> 
>>
>>> CCing John, who has been thinking a lot about these questions.
>>>
>>
>> Thanks for the heads up. Good reminder that libvirt uses the existence 
>> of properties as a bellwether for feature support. I don't think I 
>> like that idea, but I like breaking libvirt even less.
> 
> That was at hand solution. If libvirt's not doing it right, I'm happy to 
> make things better.
> 
> Michal

No, libvirt is doing it exactly correct. QAPI/QMP was designed exactly 
in this way with exactly this use-case in mind.

(So far as I understand it.)

My concerns that may have guided some patches by Eduardo that might have 
caused problems for you relate to my ability to publish an SDK for 
generic builds of QEMU, where if-conditionals that actually compile 
fields out of certain data structures can be difficult to deal with at 
static analysis time.

Until we connect to the server, we don't know if type FooStruct has 
field XYZ or not. Generally the way you handle this is by always having 
that field in the SDK and erroring out at runtime if for some reason it 
is not supportable.

In the long term, we want to (I think) bridge the data gap between QOM 
and QAPI and provide a unified set of types that we can use to construct 
a "QEMU Config File" that can be validated statically against, say, 
"qemu 6.0."

In this scenario, I have some nebulous but not necessarily meticulously 
reasoned out concerns about compile-time conditional fields. In this 
scenario, using the presence or absence of a field in a data type 
becomes a poor way to do feature detection.

QMP offers the "features" flag for certain commands where we use the 
presence or absence of that flag as the introspection data in order to 
determine behavior. Going forward, I suspect I will push for 
representing formerly-compile-time flags as runtime introspection 
feature flags instead.

...But that's stuff that isn't here now, so just keep doing what you've 
been doing, and I will take careful notice not to break that kind of 
introspection without a well-advertised alternative.

Especially right now, QOM stuff isn't in QAPI, so we don't have those 
kind of feature flags at all, so I think there really isn't another way 
at all, short of adding more capabilities and complexity to the existing 
introspection stuff, which I don't think we'd do.

--js