Change it so it really *always* eats the @toadd buffer. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virbuffer.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 96a0f16..0089d1b 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -1,7 +1,7 @@ /* * virbuffer.c: buffers for libvirt * - * Copyright (C) 2005-2008, 2010-2014 Red Hat, Inc. + * Copyright (C) 2005-2008, 2010-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -188,23 +188,27 @@ virBufferAddBuffer(virBufferPtr buf, virBufferPtr toadd) { unsigned int needSize; - if (!buf || !toadd) + if (!toadd) return; + if (!buf) + goto done; + if (buf->error || toadd->error) { if (!buf->error) buf->error = toadd->error; - virBufferFreeAndReset(toadd); - return; + goto done; } needSize = buf->use + toadd->use; if (virBufferGrow(buf, needSize - buf->use) < 0) - return; + goto done; memcpy(&buf->content[buf->use], toadd->content, toadd->use); buf->use += toadd->use; buf->content[buf->use] = '\0'; + + done: virBufferFreeAndReset(toadd); } -- 2.3.2

All the devices we have format their address as its last sub-element, so let's change memballoon to follow suit. Also adjust RNG to allow any order of them so 'virsh edit' doesn't shout at us. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/domaincommon.rng | 28 ++++++++++---------- src/conf/domain_conf.c | 30 ++++++++++------------ .../qemuxml2xmlout-balloon-device-period.xml | 30 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 4 files changed, 60 insertions(+), 29 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-period.xml diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b1d883f..b9d430a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3438,19 +3438,21 @@ <value>none</value> </choice> </attribute> - <optional> - <ref name="alias"/> - </optional> - <optional> - <ref name="address"/> - </optional> - <optional> - <element name="stats"> - <attribute name="period"> - <ref name="positiveInteger"/> - </attribute> - </element> - </optional> + <interleave> + <optional> + <ref name="alias"/> + </optional> + <optional> + <ref name="address"/> + </optional> + <optional> + <element name="stats"> + <attribute name="period"> + <ref name='positiveInteger'/> + </attribute> + </element> + </optional> + </interleave> </element> </define> <define name="parallel"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ae8688e..e010040 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18810,7 +18810,8 @@ virDomainMemballoonDefFormat(virBufferPtr buf, unsigned int flags) { const char *model = virDomainMemballoonModelTypeToString(def->model); - bool noopts = true; + virBuffer childrenBuf = VIR_BUFFER_INITIALIZER; + int indent = virBufferGetIndent(buf, false); if (!model) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -18819,27 +18820,24 @@ virDomainMemballoonDefFormat(virBufferPtr buf, } virBufferAsprintf(buf, "<memballoon model='%s'", model); - virBufferAdjustIndent(buf, 2); + virBufferAdjustIndent(&childrenBuf, indent + 2); - if (virDomainDeviceInfoNeedsFormat(&def->info, flags)) { - virBufferAddLit(buf, ">\n"); - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; - noopts = false; - } + if (def->period) + virBufferAsprintf(&childrenBuf, "<stats period='%u'/>\n", def->period); - if (def->period) { - if (noopts) - virBufferAddLit(buf, ">\n"); - virBufferAsprintf(buf, "<stats period='%u'/>\n", def->period); - noopts = false; + if (virDomainDeviceInfoNeedsFormat(&def->info, flags) && + virDomainDeviceInfoFormat(&childrenBuf, &def->info, flags) < 0) { + virBufferFreeAndReset(&childrenBuf); + return -1; } - virBufferAdjustIndent(buf, -2); - if (noopts) + if (!virBufferUse(&childrenBuf)) { virBufferAddLit(buf, "/>\n"); - else + } else { + virBufferAddLit(buf, ">\n"); + virBufferAddBuffer(buf, &childrenBuf); virBufferAddLit(buf, "</memballoon>\n"); + } return 0; } diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-period.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-period.xml new file mode 100644 index 0000000..79e465a --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-period.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <controller type='ide' index='0'/> + <memballoon model='virtio'> + <stats period='10'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x12' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 8e12e84..9e4b3a2 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -354,6 +354,7 @@ mymain(void) /* These tests generate different XML */ DO_TEST_DIFFERENT("balloon-device-auto"); + DO_TEST_DIFFERENT("balloon-device-period"); DO_TEST_DIFFERENT("channel-virtio-auto"); DO_TEST_DIFFERENT("console-compat-auto"); DO_TEST_DIFFERENT("disk-scsi-device-auto"); -- 2.3.2

In order not to leave old error messages set, this patch refactors the code so the error is reported only when acted upon. The only such place already rewrites any error, so cleaning up all the error reporting in qemuMonitorSetMemoryStatsPeriod() is enough. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_monitor.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index d869a72..18f866f 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1,7 +1,7 @@ /* * qemu_monitor.c: interaction with QEMU monitor console * - * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2015 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -1709,27 +1709,40 @@ int qemuMonitorGetMemoryStats(qemuMonitorPtr mon, return ret; } +/** + * qemuMonitorSetMemoryStatsPeriod: + * + * This function sets balloon stats update period. + * + * Returns 0 on success and -1 on error, but does *not* set an error. + */ int qemuMonitorSetMemoryStatsPeriod(qemuMonitorPtr mon, int period) { int ret = -1; VIR_DEBUG("mon=%p period=%d", mon, period); - if (!mon) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("monitor must not be NULL")); + if (!mon) return -1; - } - if (!mon->json) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("JSON monitor is required")); + if (!mon->json) + return -1; + + if (period < 0) return -1; - } if (qemuMonitorFindBalloonObjectPath(mon, "/") == 0) { ret = qemuMonitorJSONSetMemoryStatsPeriod(mon, mon->balloonpath, period); + + /* + * Most of the calls to this function are supposed to be + * non-fatal and the only one that should be fatal wants its + * own error message. More details for debugging will be in + * the log file. + */ + if (ret < 0) + virResetLastError(); } mon->ballooninit = true; return ret; -- 2.3.2

We're parsing memballoon status period as unsigned int, but when we're trying to set it, both we and qemu use signed int. That means large values will get wrapped around to negative one resulting in error. Basically the same problem as commit e3a7b874 was dealing with when updating live domain. QEMU changed the accepted value to int64 in commit 1f9296b5, but even values as INT_MAX don't make sense since the value passed means seconds. Hence adding capability flag for this change isn't worth it. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140958 Signed-off-by: Luyao Huang <lhuang@redhat.com> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/formatdomain.html.in | 2 ++ src/conf/domain_conf.c | 9 +++++++-- src/conf/domain_conf.h | 2 +- src/qemu/qemu_process.c | 2 +- 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 40e2b29..7a11cc7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5630,6 +5630,8 @@ qemu-kvm -net nic,model=? /dev/null only be made to the active guest. If the QEMU driver is not at the right revision, the attempt to set the period will fail. + Large values might be ignored, but this only affects + non-sensical numbers (i.e. many years). <span class='since'>Since 1.1.1, requires QEMU 1.5</span> </p> </dd> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e010040..b3d9999 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10432,6 +10432,7 @@ virDomainMemballoonDefParseXML(xmlNodePtr node, char *model; virDomainMemballoonDefPtr def; xmlNodePtr save = ctxt->node; + unsigned int period = 0; if (VIR_ALLOC(def) < 0) return NULL; @@ -10450,12 +10451,16 @@ virDomainMemballoonDefParseXML(xmlNodePtr node, } ctxt->node = node; - if (virXPathUInt("string(./stats/@period)", ctxt, &def->period) < -1) { + if (virXPathUInt("string(./stats/@period)", ctxt, &period) < -1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid statistics collection period")); goto error; } + def->period = period; + if (def->period < 0) + def->period = 0; + if (def->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) VIR_DEBUG("Ignoring device address for none model Memballoon"); else if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) @@ -18823,7 +18828,7 @@ virDomainMemballoonDefFormat(virBufferPtr buf, virBufferAdjustIndent(&childrenBuf, indent + 2); if (def->period) - virBufferAsprintf(&childrenBuf, "<stats period='%u'/>\n", def->period); + virBufferAsprintf(&childrenBuf, "<stats period='%i'/>\n", def->period); if (virDomainDeviceInfoNeedsFormat(&def->info, flags) && virDomainDeviceInfoFormat(&childrenBuf, &def->info, flags) < 0) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ea463cb..ee0f5fd 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1556,7 +1556,7 @@ enum { struct _virDomainMemballoonDef { int model; virDomainDeviceInfo info; - unsigned int period; /* seconds between collections */ + int period; /* seconds between collections */ }; struct _virDomainNVRAMDef { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d1f089d..0f357d5 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4390,7 +4390,7 @@ int qemuProcessStart(virConnectPtr conn, virCommandPtr cmd = NULL; struct qemuProcessHookData hookData; unsigned long cur_balloon; - unsigned int period = 0; + int period = 0; size_t i; bool rawio_set = false; char *nodeset = NULL; -- 2.3.2