Name the validation function distinctively since it's called in the parser. Later patches will add function that will validate disk definitions that are invalid but need to be parsed to avoid losing domains. --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b445469..12f9da2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6932,7 +6932,7 @@ virDomainDiskDefGeometryParse(virDomainDiskDefPtr def, static int -virDomainDiskDefValidate(const virDomainDiskDef *def) +virDomainDiskDefParseValidate(const virDomainDiskDef *def) { if (def->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { if (def->event_idx != VIR_TRISTATE_SWITCH_ABSENT) { @@ -7537,7 +7537,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - if (virDomainDiskDefValidate(def) < 0) + if (virDomainDiskDefParseValidate(def) < 0) goto error; cleanup: -- 2.8.2

--- src/qemu/qemu_driver.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 37d970e..7f8dfc8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7241,7 +7241,10 @@ qemuDomainCreate(virDomainPtr dom) return qemuDomainCreateWithFlags(dom, 0); } -static virDomainPtr qemuDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) +static virDomainPtr +qemuDomainDefineXMLFlags(virConnectPtr conn, + const char *xml, + unsigned int flags) { virQEMUDriverPtr driver = conn->privateData; virDomainDefPtr def = NULL; @@ -8192,8 +8195,10 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps, } -static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, - unsigned int flags) +static int +qemuDomainAttachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; @@ -8438,8 +8443,10 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, } -static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, - unsigned int flags) +static int +qemuDomainDetachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; -- 2.8.2

Recently there were a few patches adding checks to the post parse callback infrastructure that rejected previously accepted configuration. This unfortunately was not acceptable since we would fail to load the configs from the disk and the VMs would vanish. This patch adds helpers which are called in the appropriate places after the config is parsed when defining/starting a VM but not when reloading the configs. This infrastructure is meant for invalid configurations that were accepted previously and thus it's not executed when starting a VM from any saved state. Any checks that are necessary to be executed at that point still need to be added to qemuProcessValidateXML. --- src/conf/domain_conf.c | 67 ++++++++++++++++++++++++++++++++++++++---------- src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_domain.c | 21 +++++++++++++++ src/qemu/qemu_driver.c | 6 +++++ src/qemu/qemu_process.c | 22 +++++++++++----- 7 files changed, 100 insertions(+), 21 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 12f9da2..7359dc7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4145,20 +4145,6 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, } } - /* Validate LUN configuration */ - if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { - /* volumes haven't been translated at this point, so accept them */ - if (!(disk->src->type == VIR_STORAGE_TYPE_BLOCK || - disk->src->type == VIR_STORAGE_TYPE_VOLUME || - (disk->src->type == VIR_STORAGE_TYPE_NETWORK && - disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI))) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("disk '%s' improperly configured for a " - "device='lun'"), disk->dst); - return -1; - } - } - if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0) return -1; @@ -24357,3 +24343,56 @@ virDomainObjGetShortName(virDomainObjPtr vm) return ret; } + + +static int +virDomainDiskDefValidate(const virDomainDiskDef *disk) +{ + /* Validate LUN configuration */ + if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { + /* volumes haven't been translated at this point, so accept them */ + if (!(disk->src->type == VIR_STORAGE_TYPE_BLOCK || + disk->src->type == VIR_STORAGE_TYPE_VOLUME || + (disk->src->type == VIR_STORAGE_TYPE_NETWORK && + disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk '%s' improperly configured for a " + "device='lun'"), disk->dst); + return -1; + } + } + + return 0; +} + + +#define VIR_DOMAIN_DEF_VALIDATE_DEVICES(dev, func) \ + for (i = 0; i < def->n ## dev ## s; i++) { \ + if (func(def->dev ## s[i]) < 0) \ + return -1; \ + } + +/** + * virDomainDefValidate: + * @def: domain definition + * + * This validation function is designed to take checks of globally invalid + * configurations that the parser needs to accept so that VMs don't vanish upon + * daemon restart. Such definition can be rejected upon startup or define, where + * this function shall be called. + * + * Returns 0 if domain definition is valid, -1 on error and reports an + * appropriate message. + */ +int +virDomainDefValidate(const virDomainDef *def) +{ + size_t i; + + /* check configuration of individual devices */ + VIR_DOMAIN_DEF_VALIDATE_DEVICES(disk, virDomainDiskDefValidate); + + return 0; +} + +#undef VIR_DOMAIN_DEF_VALIDATE_DEVICES diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b9e696d..f2704c0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3178,4 +3178,6 @@ bool virDomainDefHasMemballoon(const virDomainDef *def) ATTRIBUTE_NONNULL(1); char *virDomainObjGetShortName(virDomainObjPtr vm); +int virDomainDefValidate(const virDomainDef *def); + #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8408081..cc5d6c9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -243,6 +243,7 @@ virDomainDefSetMemoryInitial; virDomainDefSetMemoryTotal; virDomainDefSetVcpus; virDomainDefSetVcpusMax; +virDomainDefValidate; virDomainDeleteConfig; virDomainDeviceAddressIsValid; virDomainDeviceAddressTypeToString; diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index a714b84..cbc5008 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -333,4 +333,6 @@ int qemuTranslateSnapshotDiskSourcePool(virConnectPtr conn, char * qemuGetHugepagePath(virHugeTLBFSPtr hugepage); char * qemuGetDefaultHugepath(virHugeTLBFSPtr hugetlbfs, size_t nhugetlbfs); + +int qemuDomainDefValidate(const virDomainDef *def); #endif /* __QEMUD_CONF_H */ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0733872..5ccb483 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5343,3 +5343,24 @@ qemuDomainDefValidateDiskLunSource(const virStorageSource *src) return 0; } + + +/** + * qemuDomainDefValidate: + * @def: domain definition + * + * Validates that the domain definition is valid for the qemu driver. This check + * is run when the domain is defined or started as new. This check is explicitly + * not executed when starting a VM from snapshot/saved state/migration to allow + * partially invalid configs to still run. + * + * Returns 0 on success, -1 on error. + */ +int +qemuDomainDefValidate(const virDomainDef *def) +{ + if (virDomainDefValidate(def) < 0) + return -1; + + return 0; +} diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7f8dfc8..e6d22ec 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1799,6 +1799,9 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, if (virDomainCreateXMLEnsureACL(conn, def) < 0) goto cleanup; + if (qemuDomainDefValidate(def) < 0) + goto cleanup; + if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator))) goto cleanup; @@ -7275,6 +7278,9 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) goto cleanup; + if (qemuDomainDefValidate(def) < 0) + goto cleanup; + if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator))) goto cleanup; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index eddf3a7..d079b18 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4620,13 +4620,6 @@ qemuProcessStartValidateXML(virDomainObjPtr vm, * If back compat isn't a concern, XML validation should probably * be done at parse time. */ - if (qemuValidateCpuCount(vm->def, qemuCaps) < 0) - return -1; - - if (!migration && !snapshot && - virDomainDefCheckDuplicateDiskInfo(vm->def) < 0) - return -1; - if (vm->def->mem.min_guarantee) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Parameter 'min_guarantee' " @@ -4634,6 +4627,21 @@ qemuProcessStartValidateXML(virDomainObjPtr vm, return -1; } + /* checks below should not be executed when starting a qemu process for a + * VM that was running before (migration, snapshots, save). It's more + * important to start such VM than keep the configuration clean */ + if (!migration && !snapshot) { + if (qemuDomainDefValidate(vm->def) < 0) + return -1; + + if (virDomainDefCheckDuplicateDiskInfo(vm->def) < 0) + return -1; + } + + /* checks below validate config that would make qemu fail to start */ + if (qemuValidateCpuCount(vm->def, qemuCaps) < 0) + return -1; + return 0; } -- 2.8.2

On Tue, May 17, 2016 at 04:25:39PM +0200, Peter Krempa wrote: [...] [...] [...] I would suggest to do the opposite. That would mean adding the virDomainDefValidate() after postParse callbacks and there you would call xmlopt->config.domainDefValidate(). Calling virDomainDefValidate() after postParse would be conditional based on a new VIR_DOMAIN_DEF_PARSE_... flag which would have to be set at the appropriate places so we would not run this new validation while starting a libvirtd and the migration/snapshot/save case. The motivation to do it this way is: 1. reuse existing code, 2. all places, that has to be skipped would be covered and we wouldn't miss calling the qemuDomainDefValidate() for tests like this patch do, 3. if you add something new, where the validation needs to be skipped, you would notice it while testing the new code and that would force you to think about it. Pavel

Now that we have a good place for this check let's move it there. --- src/qemu/qemu_domain.c | 6 ++++++ src/qemu/qemu_process.c | 6 ------ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5ccb483..f172d6f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5362,5 +5362,11 @@ qemuDomainDefValidate(const virDomainDef *def) if (virDomainDefValidate(def) < 0) return -1; + if (def->mem.min_guarantee) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Parameter 'min_guarantee' not supported by QEMU.")); + return -1; + } + return 0; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d079b18..ae435e4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4620,12 +4620,6 @@ qemuProcessStartValidateXML(virDomainObjPtr vm, * If back compat isn't a concern, XML validation should probably * be done at parse time. */ - if (vm->def->mem.min_guarantee) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Parameter 'min_guarantee' " - "not supported by QEMU.")); - return -1; - } /* checks below should not be executed when starting a qemu process for a * VM that was running before (migration, snapshots, save). It's more -- 2.8.2

Similarly to domain definition it would be possible to introduce invalid config via device coldplug. Add a similar infrastructure to do the checking. --- src/conf/domain_conf.c | 52 +++++++++++++++++++++++++++++++++++++++++++++--- src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 11 ++++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_driver.c | 3 +++ 6 files changed, 70 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2beb825..3a809d4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -24366,9 +24366,54 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk) } -#define VIR_DOMAIN_DEF_VALIDATE_DEVICES(dev, func) \ +/** + * virDomainDeviceDefValidate: + * @def: domain definition + * @dev: device definition + * + */ +int +virDomainDeviceDefValidate(const virDomainDef *def ATTRIBUTE_UNUSED, + const virDomainDeviceDef *dev) +{ + switch ((virDomainDeviceType) dev->type) { + case VIR_DOMAIN_DEVICE_DISK: + return virDomainDiskDefValidate(dev->data.disk); + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_NET: + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_HOSTDEV: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_REDIRDEV: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_LAST: + break; + } + + return 0; +} + + +#define VIR_DOMAIN_DEF_VALIDATE_DEVICES(dev, devtype) \ + device.type = devtype; \ for (i = 0; i < def->n ## dev ## s; i++) { \ - if (func(def->dev ## s[i]) < 0) \ + device.data.dev = def->dev ## s[i]; \ + if (virDomainDeviceDefValidate(def, &device) < 0) \ return -1; \ } @@ -24388,9 +24433,10 @@ int virDomainDefValidate(const virDomainDef *def) { size_t i; + virDomainDeviceDef device; /* check configuration of individual devices */ - VIR_DOMAIN_DEF_VALIDATE_DEVICES(disk, virDomainDiskDefValidate); + VIR_DOMAIN_DEF_VALIDATE_DEVICES(disk, VIR_DOMAIN_DEVICE_DISK); if (virDomainDefCheckDuplicateDiskInfo(def) < 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3c219ea..8f40feb 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3176,6 +3176,9 @@ bool virDomainDefHasMemballoon(const virDomainDef *def) ATTRIBUTE_NONNULL(1); char *virDomainObjGetShortName(virDomainObjPtr vm); +int virDomainDeviceDefValidate(const virDomainDef *def, + const virDomainDeviceDef *dev); + int virDomainDefValidate(const virDomainDef *def); #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 99975ed..08d8f54 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -249,6 +249,7 @@ virDomainDeviceAddressTypeToString; virDomainDeviceDefCopy; virDomainDeviceDefFree; virDomainDeviceDefParse; +virDomainDeviceDefValidate; virDomainDeviceFindControllerModel; virDomainDeviceGetInfo; virDomainDeviceInfoCopy; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f172d6f..58bd412 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5345,6 +5345,17 @@ qemuDomainDefValidateDiskLunSource(const virStorageSource *src) } +int +qemuDomainDeviceDefValidate(const virDomainDef *def, + const virDomainDeviceDef *dev) +{ + if (virDomainDeviceDefValidate(def, dev) < 0) + return -1; + + return 0; +} + + /** * qemuDomainDefValidate: * @def: domain definition diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index baf8bd8..aee38c6 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -667,6 +667,9 @@ void qemuDomainSecretDestroy(virDomainObjPtr vm) int qemuDomainSecretPrepare(virConnectPtr conn, virDomainObjPtr vm) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int qemuDomainDeviceDefValidate(const virDomainDef *def, + const virDomainDeviceDef *dev); + int qemuDomainDefValidateDiskLunSource(const virStorageSource *src) ATTRIBUTE_NONNULL(1); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e6d22ec..fd2c0be 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8248,6 +8248,9 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, if (dev == NULL) goto endjob; + if (qemuDomainDeviceDefValidate(vm->def, dev) < 0) + goto endjob; + if (flags & VIR_DOMAIN_AFFECT_CONFIG && flags & VIR_DOMAIN_AFFECT_LIVE) { /* If we are affecting both CONFIG and LIVE -- 2.8.2