On Mon, Aug 09, 2010 at 07:38:32PM +0100, Daniel P. Berrange wrote:
> On Mon, Aug 09, 2010 at 08:34:23PM +0200, Daniel Veillard wrote:
> > Grumpf ... that mean at the internal stucture level we need to add an
> > extra field, that is detected at a completely different time in parsing
> > too ... more complex in general, but I can understand the purity POV.
>
> I don't see this as a real problem. It is no different from the way that
> we automatically add <controller> devices at the end of parsing, if we saw
> any <disk> or <channel> devices previously.
Huh ??? You can have a controller without a disk and you don't
automagically add a controller even if nothing was suggested by the
user, it is widely different. First you need to keep a tristate for
def->balloonable, was that attribute defined, or not, then add the
error case if it's not "yes" or "no", then you need to care for
all the different cases of the tristate when you actually parse
the devices section, how do you handle <memballon> definition when
balloonable="no" was found, discard, error ?
I find this different because you provide the information in 2 places
at different levels and you ned to cope with conflict ... while still
maintaining the current behaviour of autoadding.
This then breaks nearly all the qemu XML tests due to the extra
balloonable="yes" added automatically to the output since the qemu
driver makes that a default.
And now you have to explain in documentation all the various cases
instead of a simple one liner about using "none".
When data is defined at 2 different places by design, well you designed
a mess. And in that case that's cerainly true. And after looking at the
situation in detail, no I don't by the "purity" point of view, this
actually make things *harder* both for user and from a implementation
point of view.
After messing with this for an hour, my patch is now 160 lines, it's
very hard to tell from the code what's the behaviour is in each cases
and 3 kind of regression tests need to be fixed to have all XMLs changed
to add balloonable='yes' to the memory section.
I firmly think that in that case the proper way to do this is to
keep the information in one place and that place is the <memballoon>
device, type='none' may look ugly to you, but at least the behaviour
will be predictable, users will know where to look and that can be
explained to the in a single line/sentence.
Current patch provided, I strongly suggest to not continue down this
line and apply the previous patch instead.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
index b2783b0..f18881a 100644
--- a/docs/schemas/domain.rng
+++ b/docs/schemas/domain.rng
@@ -283,6 +283,14 @@
<define name="resources">
<interleave>
<element name="memory">
+ <optional>
+ <attribute name="balloonable">
+ <choice>
+ <value>yes</value>
+ <value>no</value>
+ </choice>
+ </attribute>
+ </optional>
<ref name="memoryKB"/>
</element>
<optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bfe01f0..2a21998 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4208,10 +4208,24 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
"%s", _("missing memory element"));
goto error;
}
-
if (virXPathULong("string(./currentMemory[1])", ctxt, &def->memory) < 0)
def->memory = def->maxmem;
+ tmp = virXPathString("string(./memory[1]/@balloonable)", ctxt);
+ if (tmp) {
+ if (STREQ(tmp, "yes"))
+ def->balloonable = 1;
+ else if (STREQ(tmp, "no"))
+ def->balloonable = -1;
+ else {
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+ _("memory balloonable attribute: expected yes or no got %s"),
+ tmp);
+ goto error;
+ }
+ VIR_FREE(tmp);
+ }
+
node = virXPathNode("./memoryBacking/hugepages", ctxt);
if (node)
def->hugepage_backed = 1;
@@ -4829,18 +4843,22 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
goto error;
}
if (n > 0) {
- virDomainMemballoonDefPtr memballoon =
- virDomainMemballoonDefParseXML(nodes[0], flags);
- if (!memballoon)
- goto error;
+ if (def->balloonable >= 0) {
+ virDomainMemballoonDefPtr memballoon =
+ virDomainMemballoonDefParseXML(nodes[0], flags);
+ if (!memballoon)
+ goto error;
- def->memballoon = memballoon;
+ def->memballoon = memballoon;
+ def->balloonable = 1;
+ }
VIR_FREE(nodes);
} else {
- if (def->virtType == VIR_DOMAIN_VIRT_XEN ||
- def->virtType == VIR_DOMAIN_VIRT_QEMU ||
- def->virtType == VIR_DOMAIN_VIRT_KQEMU ||
- def->virtType == VIR_DOMAIN_VIRT_KVM) {
+ if ((def->balloonable >= 0) &&
+ (def->virtType == VIR_DOMAIN_VIRT_XEN ||
+ def->virtType == VIR_DOMAIN_VIRT_QEMU ||
+ def->virtType == VIR_DOMAIN_VIRT_KQEMU ||
+ def->virtType == VIR_DOMAIN_VIRT_KVM)) {
virDomainMemballoonDefPtr memballoon;
if (VIR_ALLOC(memballoon) < 0)
goto no_memory;
@@ -6349,7 +6367,13 @@ char *virDomainDefFormat(virDomainDefPtr def,
virBufferEscapeString(&buf, " <description>%s</description>\n",
def->description);
- virBufferVSprintf(&buf, " <memory>%lu</memory>\n", def->maxmem);
+ if (def->balloonable == 1)
+ virBufferAddLit(&buf, " <memory balloonable='yes'>");
+ else if (def->balloonable == -1)
+ virBufferAddLit(&buf, " <memory balloonable='no'>");
+ else
+ virBufferAddLit(&buf, " <memory>");
+ virBufferVSprintf(&buf, "%lu</memory>\n", def->maxmem);
virBufferVSprintf(&buf, " <currentMemory>%lu</currentMemory>\n",
def->memory);
if (def->hugepage_backed) {
@@ -6577,7 +6601,7 @@ char *virDomainDefFormat(virDomainDefPtr def,
if (def->watchdog)
virDomainWatchdogDefFormat (&buf, def->watchdog, flags);
- if (def->memballoon)
+ if ((def->balloonable >= 0) && (def->memballoon))
virDomainMemballoonDefFormat (&buf, def->memballoon, flags);
virBufferAddLit(&buf, " </devices>\n");
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index e32188f..8bf5c11 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -854,6 +854,7 @@ struct _virDomainDef {
unsigned long memory;
unsigned long maxmem;
+ int balloonable;
unsigned char hugepage_backed;
unsigned long vcpus;
int cpumasklen;
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 376cd10..9e21f5d 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -2434,7 +2434,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs)
}
/* VirtIO balloon */
- if (def->memballoon &&
+ if (def->balloonable >= 0 && def->memballoon &&
def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO &&
def->memballoon->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
if (qemuDomainPCIAddressSetNextAddr(addrs, &def->memballoon->info) < 0)
@@ -4958,7 +4958,7 @@ int qemudBuildCommandLine(virConnectPtr conn,
* NB: Earlier we declared that VirtIO balloon will always be in
* slot 0x3 on bus 0x0
*/
- if (def->memballoon) {
+ if ((def->balloonable >= 0) && (def->memballoon)) {
if (def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) {
qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Memory balloon device type '%s' is not supported by this version of qemu"),
@@ -6575,13 +6575,18 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
def->videos[def->nvideos++] = vid;
}
- if (!def->memballoon) {
+ /*
+ * if we were not told explicitely that memory was not baloonable
+ * activate the baloon driver
+ */
+ if ((def->balloonable >= 0) && (!def->memballoon)) {
virDomainMemballoonDefPtr memballoon;
if (VIR_ALLOC(memballoon) < 0)
goto no_memory;
memballoon->model = VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO;
def->memballoon = memballoon;
+ def->balloonable = 1;
}
VIR_FREE(nics);
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list