[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: John Snow
Subject: Re: [PATCH RESEND] hostmem: Don't report pmem attribute if unsupported
Date: Thu, 18 Feb 2021 13:27:21 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0

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.


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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]