[libvirt] [PATCH 10/13] conf: Don't always recalculate initial memory size from NUMA size totals

[Michal Privoznik]

On 21.09.2015 19:21, Peter Krempa wrote:
> When implementing memory hotplug I've opted to recalculate the initial
> memory size (contents of the <memory> element) as a sum of the sizes of
> NUMA nodes when NUMA was enabled. This was based on an assumption that
> qemu did not allow starting when the NUMA node size total didn't equal
> to the initial memory size. Unfortunately the check was introduced to
> qemu just lately.
> 
> This patch uses the new XML parser flag to decide whether it's safe to
> update the memory size total from the NUMA cell sizes or not.
> 
> As an additional improvement we now report an error in case when the
> size of hotplug memory would exceed the total memory size.
> 
> The rest of the changes assures that the function is called with correct
> flags.
> ---
>  src/conf/domain_conf.c  | 37 ++++++++++++++++++++++++++++++-------
>  src/conf/domain_conf.h  |  1 +
>  src/qemu/qemu_command.c |  3 ++-
>  3 files changed, 33 insertions(+), 8 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d2a13ca..64cfd8c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3726,15 +3726,36 @@ virDomainDefRemoveDuplicateMetadata(virDomainDefPtr def)
> 
> 
>  static int
> -virDomainDefPostParseMemory(virDomainDefPtr def)
> +virDomainDefPostParseMemory(virDomainDefPtr def,
> +                            unsigned int parseFlags)
>  {
>      size_t i;
> +    unsigned long long numaMemory = 0;
> +    unsigned long long hotplugMemory = 0;
> 
> -    if ((def->mem.initial_memory = virDomainNumaGetMemorySize(def->numa)) == 0) {
> +    /* Attempt to infer the initial memory size from the sum NUMA memory sizes
> +     * in case ABI updates are allowed or the <memory> element wasn't specified */
> +    if (def->mem.total_memory == 0 ||
> +        parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE)
> +        numaMemory = virDomainNumaGetMemorySize(def->numa);
> +
> +    if (numaMemory) {
> +        def->mem.initial_memory = numaMemory;

Is there a reason to not use the setter function you've introduced in
previous patch?

> +    } else {
>          def->mem.initial_memory = def->mem.total_memory;
> 
> +        /* calculate the sizes of hotplug memory */
>          for (i = 0; i < def->nmems; i++)
> -            def->mem.initial_memory -= def->mems[i]->size;
> +            hotplugMemory += def->mems[i]->size;
> +
> +        if (hotplugMemory > def->mem.initial_memory) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("Total size of memory devices exceeds the total "
> +                             "memory size"));
> +            return -1;
> +        }
> +
> +        def->mem.initial_memory -= hotplugMemory;
>      }
> 
>      if (virDomainDefGetMemoryInitial(def) == 0) {
> @@ -3770,7 +3791,8 @@ virDomainDefPostParseMemory(virDomainDefPtr def)
> 
>  static int
>  virDomainDefPostParseInternal(virDomainDefPtr def,
> -                              virCapsPtr caps ATTRIBUTE_UNUSED)
> +                              virCapsPtr caps ATTRIBUTE_UNUSED,
> +                              unsigned int parseFlags)
>  {
>      size_t i;
> 
> @@ -3781,7 +3803,7 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
>          return -1;
>      }
> 
> -    if (virDomainDefPostParseMemory(def) < 0)
> +    if (virDomainDefPostParseMemory(def, parseFlags) < 0)
>          return -1;
> 
>      /*
> @@ -4274,6 +4296,7 @@ virDomainDefPostParseDeviceIterator(virDomainDefPtr def ATTRIBUTE_UNUSED,
>  int
>  virDomainDefPostParse(virDomainDefPtr def,
>                        virCapsPtr caps,
> +                      unsigned int parseFlags,
>                        virDomainXMLOptionPtr xmlopt)
>  {
>      int ret;
> @@ -4299,7 +4322,7 @@ virDomainDefPostParse(virDomainDefPtr def,
>          return ret;
> 
> 
> -    if ((ret = virDomainDefPostParseInternal(def, caps)) < 0)
> +    if ((ret = virDomainDefPostParseInternal(def, caps, parseFlags)) < 0)
>          return ret;
> 
>      return 0;
> @@ -16426,7 +16449,7 @@ virDomainDefParseXML(xmlDocPtr xml,
>          goto error;
> 
>      /* callback to fill driver specific domain aspects */
> -    if (virDomainDefPostParse(def, caps, xmlopt) < 0)
> +    if (virDomainDefPostParse(def, caps, flags, xmlopt) < 0)
>          goto error;
> 
>      /* Auto-add any implied controllers which aren't present */
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index ab250bd..25914b4 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2459,6 +2459,7 @@ virDomainXMLOptionGetNamespace(virDomainXMLOptionPtr xmlopt)
>  int
>  virDomainDefPostParse(virDomainDefPtr def,
>                        virCapsPtr caps,
> +                      unsigned int parseFlags,
>                        virDomainXMLOptionPtr xmlopt);
> 
>  static inline bool
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 3044b11..7a1c9fe 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -13958,7 +13958,8 @@ qemuParseCommandLine(virCapsPtr qemuCaps,
>      if (virDomainDefAddImplicitControllers(def) < 0)
>          goto error;
> 
> -    if (virDomainDefPostParse(def, qemuCaps, xmlopt) < 0)
> +    if (virDomainDefPostParse(def, qemuCaps, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE,
> +                              xmlopt) < 0)
>          goto error;
> 
>      if (cmd->num_args || cmd->num_env) {
> 

ACK

Michal