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
