[PATCH 0/5] qemu: Don't cache domCaps
See 4/5 for explanation. Michal Prívozník (5): qemu: Validate RNG model qemu: Validate video model conf: Drop virDomainCapsDeviceDefValidate() qemu: Don't cache domCaps in virQEMUDriverGetDomainCapabilities() qemu: Remove virQEMUDomainCapsCache code src/conf/domain_capabilities.c | 88 ------------ src/conf/domain_capabilities.h | 4 - src/libvirt_private.syms | 1 - src/qemu/qemu_capabilities.c | 129 ------------------ src/qemu/qemu_capabilities.h | 10 -- src/qemu/qemu_conf.c | 27 ++-- src/qemu/qemu_validate.c | 77 +++++++++-- .../default-video-type-x86_64-caps-test-0.err | 2 +- .../video-invalid-multiple-devices.err | 2 +- 9 files changed, 83 insertions(+), 257 deletions(-) -- 2.26.2
The aim is to eliminate virDomainCapsDeviceDefValidate(). And in order to do so, the domain RNG model has to be validated in qemuValidateDomainRNGDef(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.c | 18 +----------------- src/qemu/qemu_validate.c | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 6ee8730111..b886ba7090 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -640,20 +640,6 @@ virDomainCapsFormat(const virDomainCaps *caps) } while (0) -static int -virDomainCapsDeviceRNGDefValidate(const virDomainCaps *caps, - const virDomainRNGDef *dev) -{ - if (ENUM_VALUE_MISSING(caps->rng.model, dev->model)) { - ENUM_VALUE_ERROR("rng model", - virDomainRNGModelTypeToString(dev->model)); - return -1; - } - - return 0; -} - - static int virDomainCapsDeviceVideoDefValidate(const virDomainCaps *caps, const virDomainVideoDef *dev) @@ -676,9 +662,6 @@ virDomainCapsDeviceDefValidate(const virDomainCaps *caps, int ret = 0; switch ((virDomainDeviceType) dev->type) { - case VIR_DOMAIN_DEVICE_RNG: - ret = virDomainCapsDeviceRNGDefValidate(caps, dev->data.rng); - break; case VIR_DOMAIN_DEVICE_VIDEO: ret = virDomainCapsDeviceVideoDefValidate(caps, dev->data.video); break; @@ -706,6 +689,7 @@ virDomainCapsDeviceDefValidate(const virDomainCaps *caps, case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_RNG: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: case VIR_DOMAIN_DEVICE_IOMMU: diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 79e765a212..c18865367e 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1883,6 +1883,31 @@ qemuValidateDomainRNGDef(const virDomainRNGDef *def, return -1; } + switch ((virDomainRNGModel) def->model) { + case VIR_DOMAIN_RNG_MODEL_VIRTIO: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_RNG)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("domain configuration does not support rng model '%s'"), + virDomainRNGModelTypeToString(def->model)); + return -1; + } + break; + + case VIR_DOMAIN_RNG_MODEL_VIRTIO_TRANSITIONAL: + case VIR_DOMAIN_RNG_MODEL_VIRTIO_NON_TRANSITIONAL: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PCI_TRANSITIONAL) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("domain configuration does not support rng model '%s'"), + virDomainRNGModelTypeToString(def->model)); + return -1; + } + break; + + case VIR_DOMAIN_RNG_MODEL_LAST: + break; + } + if (qemuValidateDomainVirtioOptions(def->virtio, qemuCaps) < 0) return -1; -- 2.26.2
The aim is to eliminate virDomainCapsDeviceDefValidate(). And in order to do so, the domain video model has to be validated in qemuValidateDomainDeviceDefVideo(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.c | 19 +-------- src/qemu/qemu_validate.c | 42 +++++++++++++++++++ .../default-video-type-x86_64-caps-test-0.err | 2 +- .../video-invalid-multiple-devices.err | 2 +- 4 files changed, 45 insertions(+), 20 deletions(-) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index b886ba7090..0e0e6d1600 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -640,22 +640,8 @@ virDomainCapsFormat(const virDomainCaps *caps) } while (0) -static int -virDomainCapsDeviceVideoDefValidate(const virDomainCaps *caps, - const virDomainVideoDef *dev) -{ - if (ENUM_VALUE_MISSING(caps->video.modelType, dev->type)) { - ENUM_VALUE_ERROR("video model", - virDomainVideoTypeToString(dev->type)); - return -1; - } - - return 0; -} - - int -virDomainCapsDeviceDefValidate(const virDomainCaps *caps, +virDomainCapsDeviceDefValidate(const virDomainCaps *caps G_GNUC_UNUSED, const virDomainDeviceDef *dev, const virDomainDef *def G_GNUC_UNUSED) { @@ -663,9 +649,6 @@ virDomainCapsDeviceDefValidate(const virDomainCaps *caps, switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_VIDEO: - ret = virDomainCapsDeviceVideoDefValidate(caps, dev->data.video); - break; - case VIR_DOMAIN_DEVICE_AUDIO: /* TODO: add validation */ break; diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index c18865367e..4defdb02cf 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -2203,10 +2203,52 @@ static int qemuValidateDomainDeviceDefVideo(const virDomainVideoDef *video, virQEMUCapsPtr qemuCaps) { + virQEMUCapsFlags cap = QEMU_CAPS_LAST; + /* there's no properties to validate for NONE video devices */ if (video->type == VIR_DOMAIN_VIDEO_TYPE_NONE) return 0; + switch ((virDomainVideoType) video->type) { + case VIR_DOMAIN_VIDEO_TYPE_VGA: + cap = QEMU_CAPS_DEVICE_VGA; + break; + case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: + cap = QEMU_CAPS_DEVICE_CIRRUS_VGA; + break; + case VIR_DOMAIN_VIDEO_TYPE_VMVGA: + cap = QEMU_CAPS_DEVICE_VMWARE_SVGA; + break; + case VIR_DOMAIN_VIDEO_TYPE_QXL: + cap = QEMU_CAPS_DEVICE_QXL; + break; + case VIR_DOMAIN_VIDEO_TYPE_VIRTIO: + cap = QEMU_CAPS_DEVICE_VIRTIO_GPU; + break; + case VIR_DOMAIN_VIDEO_TYPE_BOCHS: + cap = QEMU_CAPS_DEVICE_BOCHS_DISPLAY; + break; + case VIR_DOMAIN_VIDEO_TYPE_RAMFB: + cap = QEMU_CAPS_DEVICE_RAMFB; + break; + case VIR_DOMAIN_VIDEO_TYPE_DEFAULT: + case VIR_DOMAIN_VIDEO_TYPE_XEN: + case VIR_DOMAIN_VIDEO_TYPE_VBOX: + case VIR_DOMAIN_VIDEO_TYPE_PARALLELS: + case VIR_DOMAIN_VIDEO_TYPE_GOP: + case VIR_DOMAIN_VIDEO_TYPE_NONE: + case VIR_DOMAIN_VIDEO_TYPE_LAST: + /* nada */ + break; + } + + if (!virQEMUCapsGet(qemuCaps, cap)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("domain configuration does not support video model '%s'"), + virDomainVideoTypeToString(video->type)); + return -1; + } + if (!video->primary && video->type != VIR_DOMAIN_VIDEO_TYPE_QXL && video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO) { diff --git a/tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-0.err b/tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-0.err index 82576471b7..f7b6b57926 100644 --- a/tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-0.err +++ b/tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-0.err @@ -1 +1 @@ -unsupported configuration: domain configuration does not support 'video model' value 'default' +unsupported configuration: domain configuration does not support video model 'default' diff --git a/tests/qemuxml2argvdata/video-invalid-multiple-devices.err b/tests/qemuxml2argvdata/video-invalid-multiple-devices.err index 7fc20b2370..5c1e557021 100644 --- a/tests/qemuxml2argvdata/video-invalid-multiple-devices.err +++ b/tests/qemuxml2argvdata/video-invalid-multiple-devices.err @@ -1 +1 @@ -unsupported configuration: domain configuration does not support 'video model' value 'qxl' +unsupported configuration: domain configuration does not support video model 'qxl' -- 2.26.2
Now that nothing uses virDomainCapsDeviceDefValidate() it can be removed. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.c | 55 ---------------------------------- src/conf/domain_capabilities.h | 4 --- src/libvirt_private.syms | 1 - src/qemu/qemu_validate.c | 10 ------- 4 files changed, 70 deletions(-) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 0e0e6d1600..8a690a57a3 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -628,58 +628,3 @@ virDomainCapsFormat(const virDomainCaps *caps) return virBufferContentAndReset(&buf); } - - -#define ENUM_VALUE_MISSING(capsEnum, value) !(capsEnum.values & (1 << value)) - -#define ENUM_VALUE_ERROR(valueLabel, valueString) \ - do { \ - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ - _("domain configuration does not support '%s' value '%s'"), \ - valueLabel, valueString); \ - } while (0) - - -int -virDomainCapsDeviceDefValidate(const virDomainCaps *caps G_GNUC_UNUSED, - const virDomainDeviceDef *dev, - const virDomainDef *def G_GNUC_UNUSED) -{ - int ret = 0; - - switch ((virDomainDeviceType) dev->type) { - case VIR_DOMAIN_DEVICE_VIDEO: - case VIR_DOMAIN_DEVICE_AUDIO: - /* TODO: add validation */ - break; - - case VIR_DOMAIN_DEVICE_DISK: - case VIR_DOMAIN_DEVICE_REDIRDEV: - case VIR_DOMAIN_DEVICE_NET: - case VIR_DOMAIN_DEVICE_CONTROLLER: - case VIR_DOMAIN_DEVICE_CHR: - case VIR_DOMAIN_DEVICE_SMARTCARD: - case VIR_DOMAIN_DEVICE_HOSTDEV: - case VIR_DOMAIN_DEVICE_MEMORY: - case VIR_DOMAIN_DEVICE_VSOCK: - case VIR_DOMAIN_DEVICE_INPUT: - case VIR_DOMAIN_DEVICE_SHMEM: - case VIR_DOMAIN_DEVICE_LEASE: - case VIR_DOMAIN_DEVICE_FS: - case VIR_DOMAIN_DEVICE_SOUND: - case VIR_DOMAIN_DEVICE_WATCHDOG: - case VIR_DOMAIN_DEVICE_GRAPHICS: - case VIR_DOMAIN_DEVICE_HUB: - case VIR_DOMAIN_DEVICE_MEMBALLOON: - case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_RNG: - case VIR_DOMAIN_DEVICE_TPM: - case VIR_DOMAIN_DEVICE_PANIC: - case VIR_DOMAIN_DEVICE_IOMMU: - case VIR_DOMAIN_DEVICE_NONE: - case VIR_DOMAIN_DEVICE_LAST: - break; - } - - return ret; -} diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 685d5e2a44..4ae6f0fbb1 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -232,10 +232,6 @@ void virDomainCapsEnumClear(virDomainCapsEnumPtr capsEnum); char * virDomainCapsFormat(const virDomainCaps *caps); -int virDomainCapsDeviceDefValidate(const virDomainCaps *caps, - const virDomainDeviceDef *dev, - const virDomainDef *def); - void virSEVCapabilitiesFree(virSEVCapability *capabilities); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 491bd5bd87..1d98f01334 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -212,7 +212,6 @@ virDomainCapsCPUModelsGet; virDomainCapsCPUModelsNew; virDomainCapsCPUUsableTypeFromString; virDomainCapsCPUUsableTypeToString; -virDomainCapsDeviceDefValidate; virDomainCapsEnumClear; virDomainCapsEnumSet; virDomainCapsFormat; diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 4defdb02cf..09cdcb20c2 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -4682,24 +4682,14 @@ qemuValidateDomainDeviceDef(const virDomainDeviceDef *dev, int ret = 0; virQEMUDriverPtr driver = opaque; g_autoptr(virQEMUCaps) qemuCaps = NULL; - g_autoptr(virDomainCaps) domCaps = NULL; if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator))) return -1; - if (!(domCaps = virQEMUDriverGetDomainCapabilities(driver, qemuCaps, - def->os.machine, - def->os.arch, - def->virtType))) - return -1; - if ((ret = qemuValidateDomainDeviceDefAddress(dev, qemuCaps)) < 0) return ret; - if ((ret = virDomainCapsDeviceDefValidate(domCaps, dev, def)) < 0) - return ret; - switch ((virDomainDeviceType)dev->type) { case VIR_DOMAIN_DEVICE_NET: ret = qemuValidateDomainDeviceDefNetwork(dev->data.net, qemuCaps); -- 2.26.2
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 <mprivozn@redhat.com> --- 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(&domCaps); } -- 2.26.2
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
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
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
Now that the domCaps cache is history, this code is no longer used and thus can be removed. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 129 ----------------------------------- src/qemu/qemu_capabilities.h | 10 --- 2 files changed, 139 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 212ddf9206..9f9f976754 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -648,25 +648,6 @@ struct _virQEMUCapsAccel { }; -typedef struct _virQEMUDomainCapsCache virQEMUDomainCapsCache; -typedef virQEMUDomainCapsCache *virQEMUDomainCapsCachePtr; -struct _virQEMUDomainCapsCache { - virObjectLockable parent; - - GHashTable *cache; -}; - -G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUDomainCapsCache, virObjectUnref); - -static virClassPtr virQEMUDomainCapsCacheClass; -static void virQEMUDomainCapsCacheDispose(void *obj) -{ - virQEMUDomainCapsCachePtr cache = obj; - - virHashFree(cache->cache); -} - - /* * Update the XML parser/formatter when adding more * information to this struct so that it gets cached @@ -700,8 +681,6 @@ struct _virQEMUCaps { virArch arch; - virQEMUDomainCapsCachePtr domCapsCache; - size_t ngicCapabilities; virGICCapability *gicCapabilities; @@ -726,9 +705,6 @@ static int virQEMUCapsOnceInit(void) if (!VIR_CLASS_NEW(virQEMUCaps, virClassForObject())) return -1; - if (!(VIR_CLASS_NEW(virQEMUDomainCapsCache, virClassForObjectLockable()))) - return -1; - return 0; } @@ -1774,24 +1750,6 @@ int virQEMUCapsGetDefaultVersion(virCapsPtr caps, } -static virQEMUDomainCapsCachePtr -virQEMUDomainCapsCacheNew(void) -{ - g_autoptr(virQEMUDomainCapsCache) cache = NULL; - - if (virQEMUCapsInitialize() < 0) - return NULL; - - if (!(cache = virObjectLockableNew(virQEMUDomainCapsCacheClass))) - return NULL; - - if (!(cache->cache = virHashNew(virObjectFreeHashData))) - return NULL; - - return g_steal_pointer(&cache); -} - - virQEMUCapsPtr virQEMUCapsNew(void) { @@ -1806,14 +1764,7 @@ virQEMUCapsNew(void) qemuCaps->invalidation = true; qemuCaps->flags = virBitmapNew(QEMU_CAPS_LAST); - if (!(qemuCaps->domCapsCache = virQEMUDomainCapsCacheNew())) - goto error; - return qemuCaps; - - error: - virObjectUnref(qemuCaps); - return NULL; } @@ -1992,7 +1943,6 @@ void virQEMUCapsDispose(void *obj) { virQEMUCapsPtr qemuCaps = obj; - virObjectUnref(qemuCaps->domCapsCache); virBitmapFree(qemuCaps->flags); VIR_FREE(qemuCaps->package); @@ -2147,85 +2097,6 @@ const char *virQEMUCapsGetPackage(virQEMUCapsPtr qemuCaps) } -struct virQEMUCapsSearchDomcapsData { - const char *path; - const char *machine; - virArch arch; - virDomainVirtType virttype; -}; - - -static int -virQEMUCapsSearchDomcaps(const void *payload, - const char *name G_GNUC_UNUSED, - const void *opaque) -{ - virDomainCapsPtr domCaps = (virDomainCapsPtr) payload; - struct virQEMUCapsSearchDomcapsData *data = (struct virQEMUCapsSearchDomcapsData *) opaque; - - if (STREQ_NULLABLE(data->path, domCaps->path) && - STREQ_NULLABLE(data->machine, domCaps->machine) && - data->arch == domCaps->arch && - data->virttype == domCaps->virttype) - return 1; - - return 0; -} - - -virDomainCapsPtr -virQEMUCapsGetDomainCapsCache(virQEMUCapsPtr qemuCaps, - const char *machine, - virArch arch, - virDomainVirtType virttype, - virArch hostarch, - bool privileged, - virFirmwarePtr *firmwares, - size_t nfirmwares) -{ - virQEMUDomainCapsCachePtr cache = qemuCaps->domCapsCache; - virDomainCapsPtr domCaps = NULL; - const char *path = virQEMUCapsGetBinary(qemuCaps); - struct virQEMUCapsSearchDomcapsData data = { - .path = path, - .machine = machine, - .arch = arch, - .virttype = virttype, - }; - - virObjectLock(cache); - - domCaps = virHashSearch(cache->cache, virQEMUCapsSearchDomcaps, &data, NULL); - - if (!domCaps) { - g_autoptr(virDomainCaps) tempDomCaps = NULL; - g_autofree char *key = NULL; - - /* hash miss, build new domcaps */ - if (!(tempDomCaps = virDomainCapsNew(path, machine, - arch, virttype))) - goto cleanup; - - if (virQEMUCapsFillDomainCaps(qemuCaps, hostarch, tempDomCaps, - privileged, firmwares, nfirmwares) < 0) - goto cleanup; - - key = g_strdup_printf("%d:%d:%s:%s", arch, virttype, - NULLSTR(machine), path); - - if (virHashAddEntry(cache->cache, key, tempDomCaps) < 0) - goto cleanup; - - domCaps = g_steal_pointer(&tempDomCaps); - } - - virObjectRef(domCaps); - cleanup: - virObjectUnlock(cache); - return domCaps; -} - - int virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps, virDomainVirtType type, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 6b40ff4f15..9d3f73b2ab 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -620,16 +620,6 @@ virArch virQEMUCapsGetArch(virQEMUCapsPtr qemuCaps); unsigned int virQEMUCapsGetVersion(virQEMUCapsPtr qemuCaps); const char *virQEMUCapsGetPackage(virQEMUCapsPtr qemuCaps); -virDomainCapsPtr -virQEMUCapsGetDomainCapsCache(virQEMUCapsPtr qemuCaps, - const char *machine, - virArch arch, - virDomainVirtType virttype, - virArch hostarch, - bool privileged, - virFirmwarePtr *firmwares, - size_t nfirmwares); - unsigned int virQEMUCapsGetKVMVersion(virQEMUCapsPtr qemuCaps); int virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps, virDomainVirtType type, -- 2.26.2
On 11/16/20 4:43 AM, Michal Privoznik wrote:
See 4/5 for explanation.
Michal Prívozník (5): qemu: Validate RNG model qemu: Validate video model conf: Drop virDomainCapsDeviceDefValidate() qemu: Don't cache domCaps in virQEMUDriverGetDomainCapabilities() qemu: Remove virQEMUDomainCapsCache code
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
src/conf/domain_capabilities.c | 88 ------------ src/conf/domain_capabilities.h | 4 - src/libvirt_private.syms | 1 - src/qemu/qemu_capabilities.c | 129 ------------------ src/qemu/qemu_capabilities.h | 10 -- src/qemu/qemu_conf.c | 27 ++-- src/qemu/qemu_validate.c | 77 +++++++++-- .../default-video-type-x86_64-caps-test-0.err | 2 +- .../video-invalid-multiple-devices.err | 2 +- 9 files changed, 83 insertions(+), 257 deletions(-)
participants (3)
-
Cole Robinson -
Daniel Henrique Barboza -
Michal Privoznik