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

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.

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