On Thu, Jun 16, 2016 at 13:39:39 -0400, John Ferlan wrote:
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.
No, the statement indeed implies just hotplug. I'll modify it to hot/cold-plug to be clear. [...]
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...
This is actually a valid concern. The handling of updating of the current size is still broken to some extent. I plan to address it separately though since it has semantic implications in the qemu driver.
ACK w/ the function comments changed. Your call on this balloon modification - it's just something I noted...
Thanks! Fixed && pushed.
John