On 05/17/2016 10:25 AM, Peter Krempa wrote:
> 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;
> - }
> - }
> -
Callers via virDomainDeviceDefParse now lose this check.
John
> 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;
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list