[libvirt] [PATCH 1/2] conf: Remove pre-calculation of initial memory size

[John Ferlan]

On 06/15/2016 09:56 AM, Peter Krempa wrote:
> While we need to know the difference between the total memory stored in
> <memory> and the actual size not included in the possible memory modules
> we can't pre-calculate it reliably. This is due to the fact that
> libvirt's XML is copied via formatting and parsing the XML and the
> initial memory size can be reliably calculated only when certain
> conditions are met due to backwards compatibility.
> 
> This patch removes the storage of 'initial_memory' and fixes the helpers
> to recalculate the initial memory size all the time from the total
> memory size. This conversion is possible when we also make sure that
> memory hotplug accounts properly for the update of the total memory size
> and thus the helpers for inserting and removing memory devices need to
> be tweaked too.
> 
> This fixes a bug where a cold-plug and cold-remove of a memory device
> would increase the size reported in <memory> in the XML by the size of
> the memory device. This would happen as the persistent definition is
> copied before attaching the device and this would lead to the loss of
> data in 'initial_memory'.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1344892
> ---
>  src/conf/domain_conf.c   | 85 +++++++++++++++++++++++++-----------------------
>  src/conf/domain_conf.h   |  3 --
>  src/libvirt_private.syms |  1 -
>  src/qemu/qemu_domain.c   |  6 ++--
>  4 files changed, 49 insertions(+), 46 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 9504e5f..10e9e8b 100644

[...]

> 
> +/**
> + * virDomainMemoryInsert:
> + *
> + * Inserts a memory device definition into the domain definition. This helper
> + * should be used only in hotplug cases as it's blindly modifying the total
> + * memory size.
> + */

A nit:
I note that it's also called during the attach device config path...
While yes hotplug modifies both live & config, a casual reader may
mistake it for hotplug/live only.  Maybe it's just me.

>  int
>  virDomainMemoryInsert(virDomainDefPtr def,
>                        virDomainMemoryDefPtr mem)
>  {
> +    unsigned long long memory = virDomainDefGetMemoryActual(def);
>      int id = def->nmems;
> 
>      if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
> @@ -14565,24 +14556,38 @@ virDomainMemoryInsert(virDomainDefPtr def,
>          return -1;
>      }
> 
> -    if (VIR_APPEND_ELEMENT(def->mems, def->nmems, mem) < 0)
> +    if (VIR_APPEND_ELEMENT_COPY(def->mems, def->nmems, mem) < 0)
>          return -1;
> 
> +    virDomainDefSetMemoryTotal(def, memory + mem->size);
> +
>      return id;
>  }
> 
> 
> +/**
> + * virDomainMemoryInsert:

s/Insert/Remove

> + *
> + * Removes a memory device definition into the domain definition. This helper

s/into/from/

> + * should be used only in hotplug cases as it's blindly modifying the total
> + * memory size.
> + */
>  virDomainMemoryDefPtr
>  virDomainMemoryRemove(virDomainDefPtr def,
>                        int idx)
>  {
> +    unsigned long long memory = virDomainDefGetMemoryActual(def);
>      virDomainMemoryDefPtr ret = def->mems[idx];
> +
>      VIR_DELETE_ELEMENT(def->mems, idx, def->nmems);
> 
>      /* fix up balloon size */
>      if (def->mem.cur_balloon > virDomainDefGetMemoryActual(def))
>          def->mem.cur_balloon = virDomainDefGetMemoryActual(def);

Interesting that on the AttachDevice side there is a corollary, but the
Detach device side this code does it. Whether that's a different
inconsistency or problem is your call...

ACK w/ the function comments changed. Your call on this balloon
modification - it's just something I noted...

John
> 
> +    /* fix total memory size of the domain */
> +    virDomainDefSetMemoryTotal(def, memory - ret->size);
> +
>      return ret;
>  }
> 
[...]