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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]