Re: [PATCH 4/5] qemu: Don't cache domCaps in virQEMUDriverGetDomainCapabilities()
On 11/17/20 2:50 AM, Michal Privoznik wrote: > On 11/16/20 8:40 PM, Cole Robinson wrote: >> On 11/16/20 2:43 AM, Michal Privoznik wrote: >>> Currently, whenever a domain capabilities is needed (fortunately, >>> after cleanup done by previous commits it is now only in >>> virConnectGetDomainCapabilities()), the object is stored in a >>> cache. But there is no invalidation mechanism for the cache >>> (except the implicit one - the cache is part of qemuCaps and thus >>> share its lifetime, but that is not enough). Therefore, if >>> something changes - for instance new firmware files are >>> installed, or old are removed these changes are not reflected in >>> the virConnectGetDomainCapabilities() output. >>> >>> Originally, the caching was there because domCaps were used >>> during device XML validation and they were used a lot from our >>> test suite. But this is no longer the case. And therefore, we >>> don't need the cache and can construct fresh domCaps on each >>> virConnectGetDomainCapabilities() call. >>> >> >> Maybe the caching is not needed or was never needed. I added it as a >> premature optimization to facilitate sharing validation data between >> qemu and domcaps code. There's more info here: >> >> https://www.redhat.com/archives/libvir-list/2019-April/msg00450.html >> >> I was trying to establish a pattern where there was only one source of >> truth for things like 'this qemu supports VGA video'. The linked mail >> spells it out. >> >> My fear is that if domcapabilities data is duplicating what is done at >> qemu validation time, it's much more likely that domcapabilities content >> will bitrot. It's not hard to imagine a scenario where qemu validation >> gets tighter or more nuanced based on changing qemu capabilities, but >> domcapabilities data is not updated to match, and now domcapabilities is >> advertising a feature that qemu_validation.c immediately rejects. If >> that feature is say a video model that an app like virt-install or >> virt-manager is programmatically setting by default, now those tools are >> generating invalid configurations that libvirt claims qemu supports. >> Inaccurate domcapabilities can be worse than if nothing was reported >> at all.\ > > Yes, I was thinking about this too when writing these patches. And this > can happen if our validation code doesn't match domCaps building code. > So what if, for instance, instead of building whole domCaps, I'd export > virQEMUCapsFillDomainDeviceVideoCaps() and call it from > qemuValidateDomainDeviceDefVideo()? And then used ENUM_VALUE_MISSING() > macro to check if the model is supported? This way we will "force" all > those nuances to be implemented in domCaps building code. > Yes that sounds good to me. Thanks! - Cole
Re: [PATCH 4/5] qemu: Don't cache domCaps in virQEMUDriverGetDomainCapabilities()
On 11/16/20 8:40 PM, Cole Robinson wrote: On 11/16/20 2:43 AM, Michal Privoznik wrote: Currently, whenever a domain capabilities is needed (fortunately, after cleanup done by previous commits it is now only in virConnectGetDomainCapabilities()), the object is stored in a cache. But there is no invalidation mechanism for the cache (except the implicit one - the cache is part of qemuCaps and thus share its lifetime, but that is not enough). Therefore, if something changes - for instance new firmware files are installed, or old are removed these changes are not reflected in the virConnectGetDomainCapabilities() output. Originally, the caching was there because domCaps were used during device XML validation and they were used a lot from our test suite. But this is no longer the case. And therefore, we don't need the cache and can construct fresh domCaps on each virConnectGetDomainCapabilities() call. Maybe the caching is not needed or was never needed. I added it as a premature optimization to facilitate sharing validation data between qemu and domcaps code. There's more info here: https://www.redhat.com/archives/libvir-list/2019-April/msg00450.html I was trying to establish a pattern where there was only one source of truth for things like 'this qemu supports VGA video'. The linked mail spells it out. My fear is that if domcapabilities data is duplicating what is done at qemu validation time, it's much more likely that domcapabilities content will bitrot. It's not hard to imagine a scenario where qemu validation gets tighter or more nuanced based on changing qemu capabilities, but domcapabilities data is not updated to match, and now domcapabilities is advertising a feature that qemu_validation.c immediately rejects. If that feature is say a video model that an app like virt-install or virt-manager is programmatically setting by default, now those tools are generating invalid configurations that libvirt claims qemu supports. Inaccurate domcapabilities can be worse than if nothing was reported at all.\ Yes, I was thinking about this too when writing these patches. And this can happen if our validation code doesn't match domCaps building code. So what if, for instance, instead of building whole domCaps, I'd export virQEMUCapsFillDomainDeviceVideoCaps() and call it from qemuValidateDomainDeviceDefVideo()? And then used ENUM_VALUE_MISSING() macro to check if the model is supported? This way we will "force" all those nuances to be implemented in domCaps building code. Michal
Re: [PATCH 4/5] qemu: Don't cache domCaps in virQEMUDriverGetDomainCapabilities()
On 11/16/20 2:43 AM, Michal Privoznik wrote: > Currently, whenever a domain capabilities is needed (fortunately, > after cleanup done by previous commits it is now only in > virConnectGetDomainCapabilities()), the object is stored in a > cache. But there is no invalidation mechanism for the cache > (except the implicit one - the cache is part of qemuCaps and thus > share its lifetime, but that is not enough). Therefore, if > something changes - for instance new firmware files are > installed, or old are removed these changes are not reflected in > the virConnectGetDomainCapabilities() output. > > Originally, the caching was there because domCaps were used > during device XML validation and they were used a lot from our > test suite. But this is no longer the case. And therefore, we > don't need the cache and can construct fresh domCaps on each > virConnectGetDomainCapabilities() call. > Maybe the caching is not needed or was never needed. I added it as a premature optimization to facilitate sharing validation data between qemu and domcaps code. There's more info here: https://www.redhat.com/archives/libvir-list/2019-April/msg00450.html I was trying to establish a pattern where there was only one source of truth for things like 'this qemu supports VGA video'. The linked mail spells it out. My fear is that if domcapabilities data is duplicating what is done at qemu validation time, it's much more likely that domcapabilities content will bitrot. It's not hard to imagine a scenario where qemu validation gets tighter or more nuanced based on changing qemu capabilities, but domcapabilities data is not updated to match, and now domcapabilities is advertising a feature that qemu_validation.c immediately rejects. If that feature is say a video model that an app like virt-install or virt-manager is programmatically setting by default, now those tools are generating invalid configurations that libvirt claims qemu supports. Inaccurate domcapabilities can be worse than if nothing was reported at all. - Cole
[PATCH 4/5] qemu: Don't cache domCaps in virQEMUDriverGetDomainCapabilities()
Currently, whenever a domain capabilities is needed (fortunately,
after cleanup done by previous commits it is now only in
virConnectGetDomainCapabilities()), the object is stored in a
cache. But there is no invalidation mechanism for the cache
(except the implicit one - the cache is part of qemuCaps and thus
share its lifetime, but that is not enough). Therefore, if
something changes - for instance new firmware files are
installed, or old are removed these changes are not reflected in
the virConnectGetDomainCapabilities() output.
Originally, the caching was there because domCaps were used
during device XML validation and they were used a lot from our
test suite. But this is no longer the case. And therefore, we
don't need the cache and can construct fresh domCaps on each
virConnectGetDomainCapabilities() call.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1807198
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_conf.c | 27 ++-
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 2fb2f021c2..518f82deaa 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1422,11 +1422,8 @@ virCapsPtr virQEMUDriverGetCapabilities(virQEMUDriverPtr
driver,
/**
* virQEMUDriverGetDomainCapabilities:
*
- * Get a reference to the virDomainCapsPtr instance from the virQEMUCapsPtr
- * domCapsCache. If there's no domcaps in the cache, create a new instance,
- * add it to the cache, and return a reference.
- *
- * The caller must release the reference with virObjetUnref
+ * Get a reference to the virDomainCapsPtr instance. The caller
+ * must release the reference with virObjetUnref().
*
* Returns: a reference to a virDomainCapsPtr instance or NULL
*/
@@ -1438,15 +1435,19 @@ virQEMUDriverGetDomainCapabilities(virQEMUDriverPtr
driver,
virDomainVirtType virttype)
{
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+g_autoptr(virDomainCaps) domCaps = NULL;
+const char *path = virQEMUCapsGetBinary(qemuCaps);
-return virQEMUCapsGetDomainCapsCache(qemuCaps,
- machine,
- arch,
- virttype,
- driver->hostarch,
- driver->privileged,
- cfg->firmwares,
- cfg->nfirmwares);
+if (!(domCaps = virDomainCapsNew(path, machine, arch, virttype)))
+return NULL;
+
+if (virQEMUCapsFillDomainCaps(qemuCaps, driver->hostarch,
+ domCaps, driver->privileged,
+ cfg->firmwares,
+ cfg->nfirmwares) < 0)
+return NULL;
+
+return g_steal_pointer();
}
--
2.26.2
