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; > } > [...] -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list