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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

Now that the domCaps cache is history, this code is no longer used and thus can be removed. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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