Re: [libvirt] [PATCH 3/6] conf: Introduce infrastructure to add config validation to define time

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; }