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