[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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




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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]