In case when <vcpu ... cpuset=""> is not specified, the vcpupin array is not guaranteed to be allocated to def->vcpus. This would cause a crash for TCG since it does not report thread IDs for vCPUs. --- src/qemu/qemu_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9c5d0f4..f2b2229 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2376,7 +2376,7 @@ qemuProcessSetVcpuAffinities(virDomainObjPtr vm) /* If any CPU has custom affinity that differs from the * VM default affinity, we must reject it */ - for (n = 0; n < def->vcpus; n++) { + for (n = 0; n < def->cputune.nvcpupin; n++) { if (!virBitmapEqual(def->cpumask, def->cputune.vcpupin[n]->cpumask)) { virReportError(VIR_ERR_OPERATION_INVALID, -- 2.4.1

Store the emulator pinning cpu mask as a pure virBitmap rather than the virDomainPinDef since it stores only the bitmap and refactor qemuDomainPinEmulator to do the same operations in a much saner way. As a side effect virDomainEmulatorPinAdd and virDomainEmulatorPinDel can be removed since they don't add any value. --- src/conf/domain_conf.c | 66 +++------------------------------ src/conf/domain_conf.h | 8 +--- src/libvirt_private.syms | 2 - src/qemu/qemu_cgroup.c | 2 +- src/qemu/qemu_driver.c | 96 +++++++++++++----------------------------------- src/qemu/qemu_process.c | 2 +- 6 files changed, 34 insertions(+), 142 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6e57425..8a45682 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2473,7 +2473,7 @@ void virDomainDefFree(virDomainDefPtr def) virDomainPinDefArrayFree(def->cputune.vcpupin, def->cputune.nvcpupin); - virDomainPinDefFree(def->cputune.emulatorpin); + virBitmapFree(def->cputune.emulatorpin); for (i = 0; i < def->cputune.nvcpusched; i++) virBitmapFree(def->cputune.vcpusched[i].ids); @@ -13565,36 +13565,26 @@ virDomainIOThreadPinDefParseXML(xmlNodePtr node, } - /* Parse the XML definition for emulatorpin. * emulatorpin has the form of * <emulatorpin cpuset='0'/> */ -static virDomainPinDefPtr +static virBitmapPtr virDomainEmulatorPinDefParseXML(xmlNodePtr node) { - virDomainPinDefPtr def; + virBitmapPtr def = NULL; char *tmp = NULL; - if (VIR_ALLOC(def) < 0) - return NULL; - if (!(tmp = virXMLPropString(node, "cpuset"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing cpuset for emulatorpin")); - goto error; + return NULL; } - if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) - goto error; + ignore_value(virBitmapParse(tmp, 0, &def, VIR_DOMAIN_CPUMASK_LEN)); VIR_FREE(tmp); return def; - - error: - VIR_FREE(tmp); - VIR_FREE(def); - return NULL; } @@ -17762,50 +17752,6 @@ virDomainPinDel(virDomainPinDefPtr **pindef_list, } } -int -virDomainEmulatorPinAdd(virDomainDefPtr def, - unsigned char *cpumap, - int maplen) -{ - virDomainPinDefPtr emulatorpin = NULL; - - if (!def->cputune.emulatorpin) { - /* No emulatorpin exists yet. */ - if (VIR_ALLOC(emulatorpin) < 0) - return -1; - - emulatorpin->id = -1; - emulatorpin->cpumask = virBitmapNewData(cpumap, maplen); - if (!emulatorpin->cpumask) { - virDomainPinDefFree(emulatorpin); - return -1; - } - - def->cputune.emulatorpin = emulatorpin; - } else { - /* Since there is only 1 emulatorpin for each vm, - * juest replace the old one. - */ - virBitmapFree(def->cputune.emulatorpin->cpumask); - def->cputune.emulatorpin->cpumask = virBitmapNewData(cpumap, maplen); - if (!def->cputune.emulatorpin->cpumask) - return -1; - } - - return 0; -} - -int -virDomainEmulatorPinDel(virDomainDefPtr def) -{ - if (!def->cputune.emulatorpin) - return 0; - - virDomainPinDefFree(def->cputune.emulatorpin); - def->cputune.emulatorpin = NULL; - - return 0; -} static int virDomainEventActionDefFormat(virBufferPtr buf, @@ -21099,7 +21045,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, char *cpumask; virBufferAddLit(buf, "<emulatorpin "); - if (!(cpumask = virBitmapFormat(def->cputune.emulatorpin->cpumask))) + if (!(cpumask = virBitmapFormat(def->cputune.emulatorpin))) goto error; virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0fcf52e..c36dfd1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2068,7 +2068,7 @@ struct _virDomainCputune { long long emulator_quota; size_t nvcpupin; virDomainPinDefPtr *vcpupin; - virDomainPinDefPtr emulatorpin; + virBitmapPtr emulatorpin; size_t nvcpusched; virDomainThreadSchedParamPtr vcpusched; @@ -2673,12 +2673,6 @@ void virDomainPinDel(virDomainPinDefPtr **pindef_list, size_t *npin, int vcpu); -int virDomainEmulatorPinAdd(virDomainDefPtr def, - unsigned char *cpumap, - int maplen); - -int virDomainEmulatorPinDel(virDomainDefPtr def); - void virDomainRNGDefFree(virDomainRNGDefPtr def); bool virDomainDiskDefDstDuplicates(virDomainDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6a95fb9..a90a1b7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -273,8 +273,6 @@ virDomainDiskSetFormat; virDomainDiskSetSource; virDomainDiskSetType; virDomainDiskSourceIsBlockType; -virDomainEmulatorPinAdd; -virDomainEmulatorPinDel; virDomainFSDefFree; virDomainFSIndexByName; virDomainFSInsert; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 96677dd..7d1f009 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1114,7 +1114,7 @@ qemuSetupCgroupForEmulator(virDomainObjPtr vm) goto cleanup; if (def->cputune.emulatorpin) - cpumask = def->cputune.emulatorpin->cpumask; + cpumask = def->cputune.emulatorpin; else if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) cpumask = priv->autoCpuset; else if (def->cpumask) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d1b00a2..e34cb6c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5374,23 +5374,19 @@ qemuDomainPinEmulator(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; virCgroupPtr cgroup_emulator = NULL; - pid_t pid; virDomainDefPtr persistentDef = NULL; int ret = -1; qemuDomainObjPrivatePtr priv; bool doReset = false; - size_t newVcpuPinNum = 0; - virDomainPinDefPtr *newVcpuPin = NULL; virBitmapPtr pcpumap = NULL; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; virObjectEventPtr event = NULL; - char * str = NULL; + char *str = NULL; virTypedParameterPtr eventParams = NULL; int eventNparams = 0; int eventMaxparams = 0; - virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -5436,65 +5432,33 @@ qemuDomainPinEmulator(virDomainPtr dom, if (virBitmapIsAllSet(pcpumap)) doReset = true; - pid = vm->pid; - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - - if (priv->vcpupids != NULL) { - if (VIR_ALLOC(newVcpuPin) < 0) + if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, + 0, false, &cgroup_emulator) < 0) goto endjob; - if (virDomainPinAdd(&newVcpuPin, &newVcpuPinNum, cpumap, maplen, -1) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update vcpupin")); - virDomainPinDefArrayFree(newVcpuPin, newVcpuPinNum); + if (qemuSetupCgroupCpusetCpus(cgroup_emulator, pcpumap) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("failed to set cpuset.cpus in cgroup" + " for emulator threads")); goto endjob; } - - if (virCgroupHasController(priv->cgroup, - VIR_CGROUP_CONTROLLER_CPUSET)) { - /* - * Configure the corresponding cpuset cgroup. - */ - if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, - 0, false, &cgroup_emulator) < 0) - goto endjob; - if (qemuSetupCgroupCpusetCpus(cgroup_emulator, - newVcpuPin[0]->cpumask) < 0) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("failed to set cpuset.cpus in cgroup" - " for emulator threads")); - goto endjob; - } - } else { - if (virProcessSetAffinity(pid, pcpumap) < 0) { - virReportError(VIR_ERR_SYSTEM_ERROR, "%s", - _("failed to set cpu affinity for " - "emulator threads")); - goto endjob; - } + } else { + if (virProcessSetAffinity(vm->pid, pcpumap) < 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", + _("failed to set cpu affinity for " + "emulator thread")); + goto endjob; } + } - if (doReset) { - if (virDomainEmulatorPinDel(vm->def) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to delete emulatorpin xml of " - "a running domain")); - goto endjob; - } - } else { - virDomainPinDefFree(vm->def->cputune.emulatorpin); - vm->def->cputune.emulatorpin = newVcpuPin[0]; - VIR_FREE(newVcpuPin); - } + virBitmapFree(vm->def->cputune.emulatorpin); + vm->def->cputune.emulatorpin = NULL; - if (newVcpuPin) - virDomainPinDefArrayFree(newVcpuPin, newVcpuPinNum); - } else { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cpu affinity is not supported")); + if (!doReset && + !(vm->def->cputune.emulatorpin = virBitmapNewCopy(pcpumap))) goto endjob; - } if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto endjob; @@ -5510,22 +5474,12 @@ qemuDomainPinEmulator(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + virBitmapFree(persistentDef->cputune.emulatorpin); + persistentDef->cputune.emulatorpin = NULL; - if (doReset) { - if (virDomainEmulatorPinDel(persistentDef) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to delete emulatorpin xml of " - "a persistent domain")); - goto endjob; - } - } else { - if (virDomainEmulatorPinAdd(persistentDef, cpumap, maplen) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add emulatorpin xml " - "of a persistent domain")); - goto endjob; - } - } + if (!doReset && + !(persistentDef->cputune.emulatorpin = virBitmapNewCopy(pcpumap))) + goto endjob; ret = virDomainSaveConfig(cfg->configDir, persistentDef); goto endjob; @@ -5592,7 +5546,7 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom, goto cleanup; if (targetDef->cputune.emulatorpin) { - cpumask = targetDef->cputune.emulatorpin->cpumask; + cpumask = targetDef->cputune.emulatorpin; } else if (targetDef->cpumask) { cpumask = targetDef->cpumask; } else { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f2b2229..cc588d7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2414,7 +2414,7 @@ qemuProcessSetEmulatorAffinity(virDomainObjPtr vm) int ret = -1; if (def->cputune.emulatorpin) - cpumask = def->cputune.emulatorpin->cpumask; + cpumask = def->cputune.emulatorpin; else if (def->cpumask) cpumask = def->cpumask; else -- 2.4.1

--- src/conf/domain_conf.h | 58 ++++++++++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c36dfd1..a0f68ea 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1926,24 +1926,6 @@ struct _virDomainClockDef { virDomainTimerDefPtr *timers; }; -# define VIR_DOMAIN_CPUMASK_LEN 1024 - -typedef struct _virDomainPinDef virDomainPinDef; -typedef virDomainPinDef *virDomainPinDefPtr; -struct _virDomainPinDef { - int id; - virBitmapPtr cpumask; -}; - -void virDomainPinDefFree(virDomainPinDefPtr def); -void virDomainPinDefArrayFree(virDomainPinDefPtr *def, int npin); - -virDomainPinDefPtr *virDomainPinDefCopy(virDomainPinDefPtr *src, - int npin); - -virDomainPinDefPtr virDomainPinFind(virDomainPinDefPtr *def, - int npin, - int id); typedef struct _virBlkioDevice virBlkioDevice; typedef virBlkioDevice *virBlkioDevicePtr; @@ -2044,6 +2026,8 @@ struct _virDomainHugePage { unsigned long long size; /* hugepage size in KiB */ }; +# define VIR_DOMAIN_CPUMASK_LEN 1024 + typedef struct _virDomainIOThreadIDDef virDomainIOThreadIDDef; typedef virDomainIOThreadIDDef *virDomainIOThreadIDDefPtr; @@ -2056,6 +2040,34 @@ struct _virDomainIOThreadIDDef { void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def); + +typedef struct _virDomainPinDef virDomainPinDef; +typedef virDomainPinDef *virDomainPinDefPtr; +struct _virDomainPinDef { + int id; + virBitmapPtr cpumask; +}; + +void virDomainPinDefFree(virDomainPinDefPtr def); +void virDomainPinDefArrayFree(virDomainPinDefPtr *def, int npin); + +virDomainPinDefPtr *virDomainPinDefCopy(virDomainPinDefPtr *src, + int npin); + +virDomainPinDefPtr virDomainPinFind(virDomainPinDefPtr *def, + int npin, + int id); + +int virDomainPinAdd(virDomainPinDefPtr **pindef_list, + size_t *npin, + unsigned char *cpumap, + int maplen, + int id); + +void virDomainPinDel(virDomainPinDefPtr **pindef_list, + size_t *npin, + int vcpu); + typedef struct _virDomainCputune virDomainCputune; typedef virDomainCputune *virDomainCputunePtr; @@ -2663,16 +2675,6 @@ int virDomainDefCompatibleDevice(virDomainDefPtr def, virDomainDeviceDefPtr dev, virDomainDeviceAction action); -int virDomainPinAdd(virDomainPinDefPtr **pindef_list, - size_t *npin, - unsigned char *cpumap, - int maplen, - int id); - -void virDomainPinDel(virDomainPinDefPtr **pindef_list, - size_t *npin, - int vcpu); - void virDomainRNGDefFree(virDomainRNGDefPtr def); bool virDomainDiskDefDstDuplicates(virDomainDefPtr def); -- 2.4.1

Since some functions can be optimized by reusing the buffers that they already have instead of allocating and copying new ones, lets split virBitmapToData to two functions where one only converts the data and the second one is a wrapper that allocates the buffer if necessary. --- src/libvirt_private.syms | 1 + src/util/virbitmap.c | 33 ++++++++++++++++++++++++++------- src/util/virbitmap.h | 3 +++ 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a90a1b7..7b502aa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1120,6 +1120,7 @@ virBitmapSetBit; virBitmapSize; virBitmapString; virBitmapToData; +virBitmapToDataBuf; # util/virbuffer.h diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index bf905ab..c6a6b51 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -492,25 +492,45 @@ virBitmapPtr virBitmapNewData(void *data, int len) * * Convert a bitmap to a chunk of data containing bits information. * Data consists of sequential bytes, with lower bytes containing - * lower bits. + * lower bits. This function allocates @data. * * Returns 0 on success, -1 otherwise. */ int virBitmapToData(virBitmapPtr bitmap, unsigned char **data, int *dataLen) { int len; - unsigned long *l; - size_t i, j; - unsigned char *bytes; len = (bitmap->max_bit + CHAR_BIT - 1) / CHAR_BIT; if (VIR_ALLOC_N(*data, len) < 0) return -1; - bytes = *data; *dataLen = len; + virBitmapToDataBuf(bitmap, *data, *dataLen); + + return 0; +} + + +/** + * virBitmapToDataBuf: + * @bytes: pointer to memory to fill + * @len: len of @bytes in byte + * + * Convert a bitmap to a chunk of data containing bits information. + * Data consists of sequential bytes, with lower bytes containing + * lower bits. + */ +void virBitmapToDataBuf(virBitmapPtr bitmap, + unsigned char *bytes, + size_t len) +{ + unsigned long *l; + size_t i, j; + + memset(bytes, 0, len); + /* htole64 is not provided by gnulib, so we do the conversion by hand */ l = bitmap->map; for (i = j = 0; i < len; i++, j++) { @@ -520,10 +540,9 @@ int virBitmapToData(virBitmapPtr bitmap, unsigned char **data, int *dataLen) } bytes[i] = *l >> (j * CHAR_BIT); } - - return 0; } + /** * virBitmapEqual: * @b1: bitmap 1 diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index d326c6a..47488de 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -88,6 +88,9 @@ virBitmapPtr virBitmapNewCopy(virBitmapPtr src) ATTRIBUTE_NONNULL(1); virBitmapPtr virBitmapNewData(void *data, int len) ATTRIBUTE_NONNULL(1); int virBitmapToData(virBitmapPtr bitmap, unsigned char **data, int *dataLen) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + +void virBitmapToDataBuf(virBitmapPtr bitmap, unsigned char *data, size_t len) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); bool virBitmapEqual(virBitmapPtr b1, virBitmapPtr b2); -- 2.4.1

Reuse the function so that we can get rid of a lot of temporary allocations. --- src/qemu/qemu_driver.c | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e34cb6c..f6107b7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5291,8 +5291,8 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, virDomainDefPtr targetDef = NULL; int ret = -1; int hostcpus, vcpu; - unsigned char *cpumap; virCapsPtr caps = NULL; + virBitmapPtr allcpumap = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -5319,6 +5319,11 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, if ((hostcpus = nodeGetCPUCount()) < 0) goto cleanup; + if (!(allcpumap = virBitmapNew(hostcpus))) + goto cleanup; + + virBitmapSetAll(allcpumap); + /* Clamp to actual number of vcpus */ if (ncpumaps > targetDef->vcpus) ncpumaps = targetDef->vcpus; @@ -5329,37 +5334,23 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, for (vcpu = 0; vcpu < ncpumaps; vcpu++) { virDomainPinDefPtr pininfo; virBitmapPtr bitmap = NULL; - unsigned char *tmpmap = NULL; - int tmpmaplen; pininfo = virDomainPinFind(targetDef->cputune.vcpupin, targetDef->cputune.nvcpupin, vcpu); - if (!pininfo) { - if (!(bitmap = virBitmapNew(hostcpus))) - goto cleanup; - virBitmapSetAll(bitmap); - } else { + + if (pininfo && pininfo->cpumask) bitmap = pininfo->cpumask; - } + else + bitmap = allcpumap; - if (virBitmapToData(bitmap, &tmpmap, &tmpmaplen) < 0) { - if (!pininfo) - virBitmapFree(bitmap); - goto cleanup; - } - if (tmpmaplen > maplen) - tmpmaplen = maplen; - cpumap = VIR_GET_CPUMAP(cpumaps, maplen, vcpu); - memcpy(cpumap, tmpmap, tmpmaplen); - if (!pininfo) - virBitmapFree(bitmap); - VIR_FREE(tmpmap); + virBitmapToDataBuf(bitmap, VIR_GET_CPUMAP(cpumaps, maplen, vcpu), maplen); } ret = ncpumaps; cleanup: + virBitmapFree(allcpumap); virDomainObjEndAPI(&vm); virObjectUnref(caps); return ret; -- 2.4.1

Add a macro that will allow to simplify overflow checks and make them more universal in case data types change. --- src/util/virutil.h | 11 +++++++++++ tests/utiltest.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/src/util/virutil.h b/src/util/virutil.h index c78b357..53025f7 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -247,4 +247,15 @@ unsigned long long virMemoryLimitTruncate(unsigned long long value); bool virMemoryLimitIsSet(unsigned long long value); unsigned long long virMemoryMaxValue(bool ulong); +/** + * VIR_ASSIGN_IS_OVERFLOW: + * @rvalue: value that is checked (evaluated twice) + * @lvalue: value that the check is against (used in typeof()) + * + * This macro assigns @lvalue to @rvalue and evaluates as true if the value of + * @rvalue did not fit into the @lvalue. + */ +# define VIR_ASSIGN_IS_OVERFLOW(lvalue, rvalue) \ + (((lvalue) = (rvalue)) != (rvalue)) + #endif /* __VIR_UTIL_H__ */ diff --git a/tests/utiltest.c b/tests/utiltest.c index dfa4290..3a1f8eb 100644 --- a/tests/utiltest.c +++ b/tests/utiltest.c @@ -172,6 +172,35 @@ testRoundValueToPowerOfTwo(const void *data ATTRIBUTE_UNUSED) } +#define TEST_OVERFLOW(var, val, expect) \ + tmp = val; \ + if (VIR_ASSIGN_IS_OVERFLOW(var, tmp) != expect) { \ + fprintf(stderr, "\noverflow check failed: " \ + "var: " #var " val: " #val "\n"); \ + return -1; \ + } + +static int +testOverflowCheckMacro(const void *data ATTRIBUTE_UNUSED) +{ + long long tmp; + unsigned char luchar; + char lchar; + + TEST_OVERFLOW(luchar, 254, false); + TEST_OVERFLOW(luchar, 255, false); + TEST_OVERFLOW(luchar, 256, true); + TEST_OVERFLOW(luchar, 767, true); + + TEST_OVERFLOW(lchar, 127, false); + TEST_OVERFLOW(lchar, -128, false); + TEST_OVERFLOW(lchar, -129, true); + TEST_OVERFLOW(lchar, 128, true); + + return 0; +} + + static int @@ -193,6 +222,7 @@ mymain(void) DO_TEST(DiskNameToIndex); DO_TEST(ParseVersionString); DO_TEST(RoundValueToPowerOfTwo); + DO_TEST(OverflowCheckMacro); return result == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.4.1

--- src/qemu/qemu_monitor.c | 8 ++++++-- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 12 +++++------- src/qemu/qemu_monitor_json.h | 2 +- src/qemu/qemu_monitor_text.c | 12 +++++------- src/qemu/qemu_monitor_text.h | 2 +- 6 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 45414cf..e9c57f1 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2022,11 +2022,15 @@ qemuMonitorExpirePassword(qemuMonitorPtr mon, } +/* + * Returns: 0 if balloon not supported, +1 if balloon adjust worked + * or -1 on failure + */ int qemuMonitorSetBalloon(qemuMonitorPtr mon, - unsigned long newmem) + unsigned long long newmem) { - VIR_DEBUG("newmem=%lu", newmem); + VIR_DEBUG("newmem=%llu", newmem); QEMU_CHECK_MONITOR(mon); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 76380a0..d30b514 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -401,7 +401,7 @@ int qemuMonitorExpirePassword(qemuMonitorPtr mon, int type, const char *expire_time); int qemuMonitorSetBalloon(qemuMonitorPtr mon, - unsigned long newmem); + unsigned long long newmem); int qemuMonitorSetCPU(qemuMonitorPtr mon, int cpu, bool online); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 2287398..e6da804 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2175,16 +2175,14 @@ int qemuMonitorJSONExpirePassword(qemuMonitorPtr mon, return ret; } -/* - * Returns: 0 if balloon not supported, +1 if balloon adjust worked - * or -1 on failure - */ -int qemuMonitorJSONSetBalloon(qemuMonitorPtr mon, - unsigned long newmem) + +int +qemuMonitorJSONSetBalloon(qemuMonitorPtr mon, + unsigned long long newmem) { int ret; virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("balloon", - "U:value", ((unsigned long long)newmem)*1024, + "U:value", newmem * 1024, NULL); virJSONValuePtr reply = NULL; if (!cmd) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 43adc90..05c9b29 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -94,7 +94,7 @@ int qemuMonitorJSONExpirePassword(qemuMonitorPtr mon, const char *protocol, const char *expire_time); int qemuMonitorJSONSetBalloon(qemuMonitorPtr mon, - unsigned long newmem); + unsigned long long newmem); int qemuMonitorJSONSetCPU(qemuMonitorPtr mon, int cpu, bool online); int qemuMonitorJSONEjectMedia(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index ad36dd0..1b6bc02 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -1110,12 +1110,10 @@ int qemuMonitorTextExpirePassword(qemuMonitorPtr mon, return ret; } -/* - * Returns: 0 if balloon not supported, +1 if balloon adjust worked - * or -1 on failure - */ -int qemuMonitorTextSetBalloon(qemuMonitorPtr mon, - unsigned long newmem) + +int +qemuMonitorTextSetBalloon(qemuMonitorPtr mon, + unsigned long long newmem) { char *cmd; char *reply = NULL; @@ -1125,7 +1123,7 @@ int qemuMonitorTextSetBalloon(qemuMonitorPtr mon, * 'newmem' is in KB, QEMU monitor works in MB, and we all wish * we just worked in bytes with unsigned long long everywhere. */ - if (virAsprintf(&cmd, "balloon %lu", VIR_DIV_UP(newmem, 1024)) < 0) + if (virAsprintf(&cmd, "balloon %llu", VIR_DIV_UP(newmem, 1024)) < 0) return -1; if (qemuMonitorHMPCommand(mon, cmd, &reply) < 0) { diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index 40edc9a..6f9a215 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -79,7 +79,7 @@ int qemuMonitorTextExpirePassword(qemuMonitorPtr mon, const char *protocol, const char *expire_time); int qemuMonitorTextSetBalloon(qemuMonitorPtr mon, - unsigned long newmem); + unsigned long long newmem); int qemuMonitorTextSetCPU(qemuMonitorPtr mon, int cpu, bool online); int qemuMonitorTextEjectMedia(qemuMonitorPtr mon, -- 2.4.1

After libvirt issues the balloon resize command, the current balloon size needs to be changed to the maximum memory size since the vCPUs were not started and thus the balloon driver could not return the memory. Since GetXMLDesc and other APIs return the balloon size without updating it in case they are not able to obtain the job and the memory balloon does not support the asynchronous event the sizing might be incorrect. --- src/qemu/qemu_process.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d5d9369..7f154f0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4897,6 +4897,11 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; } + /* Since CPUs were not started yet, the ballon could not return the memory + * to the host and thus cur_balloon needs to be updated so that GetXMLdesc + * and friends return the correct size in case they can't grab the job */ + vm->def->mem.cur_balloon = virDomainDefGetMemoryActual(vm->def); + VIR_DEBUG("Detecting actual memory size for video device"); if (qemuProcessUpdateVideoRamSize(driver, vm, asyncJob) < 0) goto cleanup; -- 2.4.1

When qemu does not support the balloon event the current memory size needs to be queried. Since there are two places that implement the same logic, split it out into a function and reuse. --- src/qemu/qemu_domain.c | 64 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 ++ src/qemu/qemu_driver.c | 84 +++++--------------------------------------------- 3 files changed, 75 insertions(+), 76 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index db8554b..661181f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3182,3 +3182,67 @@ qemuDomainMachineIsI440FX(const virDomainDef *def) STRPREFIX(def->os.machine, "pc-i440") || STRPREFIX(def->os.machine, "rhel")); } + + +/** + * qemuDomainUpdateCurrentMemorySize: + * + * Updates the current balloon size from the monitor if necessary. In case when + * the balloon is not present for the domain, the function recalculates the + * maximum size to reflect possible changes. + * + * Returns 0 on success and updates vm->def->mem.cur_balloon if necessary, -1 on + * error and reports libvirt error. + */ +int +qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver, + virDomainObjPtr vm) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + unsigned long long balloon; + int ret = -1; + + /* inactive domain doesn't need size update */ + if (!virDomainObjIsActive(vm)) + return 0; + + /* if no balloning is available, the current size equals to the current + * full memory size */ + if (!vm->def->memballoon || + vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) { + vm->def->mem.cur_balloon = virDomainDefGetMemoryActual(vm->def); + return 0; + } + + /* current size is always automagically updated via the event */ + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT)) + return 0; + + /* here we need to ask the monitor */ + + /* Don't delay if someone's using the monitor, just use existing most + * recent data instead */ + if (qemuDomainJobAllowed(priv, QEMU_JOB_QUERY)) { + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + return -1; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; + } + + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorGetBalloonInfo(priv->mon, &balloon); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + endjob: + qemuDomainObjEndJob(driver, vm); + + if (ret < 0) + return -1; + } + + return 0; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index a6df199..053607f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -466,4 +466,7 @@ virDomainChrSourceDefPtr qemuFindAgentConfig(virDomainDefPtr def); bool qemuDomainMachineIsQ35(const virDomainDef *def); bool qemuDomainMachineIsI440FX(const virDomainDef *def); +int qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver, + virDomainObjPtr vm); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f32b87e..1ff4237 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2618,8 +2618,6 @@ static int qemuDomainGetInfo(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; - int err; - unsigned long long balloon; if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -2642,43 +2640,10 @@ static int qemuDomainGetInfo(virDomainPtr dom, info->maxMem = virDomainDefGetMemoryActual(vm->def); if (virDomainObjIsActive(vm)) { - qemuDomainObjPrivatePtr priv = vm->privateData; - - if ((vm->def->memballoon != NULL) && - (vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)) { - info->memory = virDomainDefGetMemoryActual(vm->def); - } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT)) { - info->memory = vm->def->mem.cur_balloon; - } else if (qemuDomainJobAllowed(priv, QEMU_JOB_QUERY)) { - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) - goto cleanup; - if (!virDomainObjIsActive(vm)) { - err = 0; - } else { - qemuDomainObjEnterMonitor(driver, vm); - err = qemuMonitorGetBalloonInfo(priv->mon, &balloon); - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - qemuDomainObjEndJob(driver, vm); - goto cleanup; - } - } - qemuDomainObjEndJob(driver, vm); + if (qemuDomainUpdateCurrentMemorySize(driver, vm) < 0) + goto cleanup; - if (err < 0) { - /* We couldn't get current memory allocation but that's not - * a show stopper; we wouldn't get it if there was a job - * active either - */ - info->memory = vm->def->mem.cur_balloon; - } else if (err == 0) { - /* Balloon not supported, so maxmem is always the allocation */ - info->memory = virDomainDefGetMemoryActual(vm->def); - } else { - info->memory = balloon; - } - } else { - info->memory = vm->def->mem.cur_balloon; - } + info->memory = vm->def->mem.cur_balloon; } else { info->memory = 0; } @@ -7173,57 +7138,24 @@ qemuDomainObjRestore(virConnectPtr conn, } -static char *qemuDomainGetXMLDesc(virDomainPtr dom, - unsigned int flags) +static char +*qemuDomainGetXMLDesc(virDomainPtr dom, + unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; char *ret = NULL; - unsigned long long balloon; - int err = 0; - qemuDomainObjPrivatePtr priv; /* Flags checked by virDomainDefFormat */ if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; - priv = vm->privateData; - if (virDomainGetXMLDescEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - /* Refresh current memory based on balloon info if supported */ - if ((vm->def->memballoon != NULL) && - (vm->def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_NONE) && - !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT) && - (virDomainObjIsActive(vm))) { - /* Don't delay if someone's using the monitor, just use - * existing most recent data instead */ - if (qemuDomainJobAllowed(priv, QEMU_JOB_QUERY)) { - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) - goto cleanup; - - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto endjob; - } - - qemuDomainObjEnterMonitor(driver, vm); - err = qemuMonitorGetBalloonInfo(priv->mon, &balloon); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - err = -1; - - endjob: - qemuDomainObjEndJob(driver, vm); - if (err < 0) - goto cleanup; - if (err > 0) - vm->def->mem.cur_balloon = balloon; - /* err == 0 indicates no balloon support, so ignore it */ - } - } + if (qemuDomainUpdateCurrentMemorySize(driver, vm) < 0) + goto cleanup; if ((flags & VIR_DOMAIN_XML_MIGRATABLE)) flags |= QEMU_DOMAIN_FORMAT_LIVE_FLAGS; -- 2.4.1

Since the returned structure uses "unsigned long" for memory sizes add a few overflow checks to notify the user in case we are not able to represent given values. --- src/qemu/qemu_driver.c | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1ff4237..92da08d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2612,9 +2612,12 @@ static int qemuDomainSendKey(virDomainPtr domain, return ret; } -static int qemuDomainGetInfo(virDomainPtr dom, - virDomainInfoPtr info) + +static int +qemuDomainGetInfo(virDomainPtr dom, + virDomainInfoPtr info) { + unsigned long long maxmem; virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; @@ -2625,30 +2628,34 @@ static int qemuDomainGetInfo(virDomainPtr dom, if (virDomainGetInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup; + if (qemuDomainUpdateCurrentMemorySize(driver, vm) < 0) + goto cleanup; + + memset(info, 0, sizeof(*info)); + info->state = virDomainObjGetState(vm, NULL); - if (!virDomainObjIsActive(vm)) { - info->cpuTime = 0; - } else { - if (qemuGetProcessInfo(&(info->cpuTime), NULL, NULL, vm->pid, 0) < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("cannot read cputime for domain")); - goto cleanup; - } + maxmem = virDomainDefGetMemoryActual(vm->def); + if (VIR_ASSIGN_IS_OVERFLOW(info->maxMem, maxmem)) { + virReportError(VIR_ERR_OVERFLOW, "%s", + _("Initial memory size too large")); + goto cleanup; } - info->maxMem = virDomainDefGetMemoryActual(vm->def); - if (virDomainObjIsActive(vm)) { - if (qemuDomainUpdateCurrentMemorySize(driver, vm) < 0) + if (VIR_ASSIGN_IS_OVERFLOW(info->memory, vm->def->mem.cur_balloon)) { + virReportError(VIR_ERR_OVERFLOW, "%s", + _("Current memory size too large")); goto cleanup; + } - info->memory = vm->def->mem.cur_balloon; - } else { - info->memory = 0; + if (qemuGetProcessInfo(&(info->cpuTime), NULL, NULL, vm->pid, 0) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("cannot read cputime for domain")); + goto cleanup; + } } - info->nrVirtCpu = vm->def->vcpus; ret = 0; cleanup: -- 2.4.1

While we probably won't see machines with more than 65536 cpus for a while lets store the cpu count as an integer so that we can avoid quite a lot of overflow checks in our code. --- src/conf/domain_conf.c | 50 ++++++++++++++++++-------------------------------- src/conf/domain_conf.h | 4 ++-- src/libvirt-domain.c | 14 -------------- src/qemu/qemu_driver.c | 6 ------ 4 files changed, 20 insertions(+), 54 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8a45682..4a8c1b5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13868,7 +13868,6 @@ virDomainDefParseXML(xmlDocPtr xml, int n; long id = -1; virDomainDefPtr def; - unsigned long count; bool uuid_generated = false; virHashTablePtr bootHash = NULL; bool usb_none = false; @@ -14151,44 +14150,31 @@ virDomainDefParseXML(xmlDocPtr xml, &def->mem.swap_hard_limit) < 0) goto error; - n = virXPathULong("string(./vcpu[1])", ctxt, &count); - if (n == -2) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("maximum vcpus must be an integer")); - goto error; - } else if (n < 0) { - def->maxvcpus = 1; - } else { - def->maxvcpus = count; - if (count == 0 || (unsigned short) count != count) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid maximum number of vCPUs '%lu'"), count); + if ((n = virXPathUInt("string(./vcpu[1])", ctxt, &def->maxvcpus)) < 0) { + if (n == -2) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("maximum vcpus count must be an integer")); goto error; } + + def->maxvcpus = 1; } - n = virXPathULong("string(./vcpu[1]/@current)", ctxt, &count); - if (n == -2) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("current vcpus must be an integer")); - goto error; - } else if (n < 0) { - def->vcpus = def->maxvcpus; - } else { - def->vcpus = count; - if (count == 0 || (unsigned short) count != count) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid current number of vCPUs '%lu'"), count); + if ((n = virXPathUInt("string(./vcpu[1]/@current)", ctxt, &def->vcpus)) < 0) { + if (n == -2) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("current vcpus count must be an integer")); goto error; } - if (def->maxvcpus < count) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("maxvcpus must not be less than current vcpus " - "(%d < %lu)"), - def->maxvcpus, count); - goto error; - } + def->vcpus = def->maxvcpus; + } + + if (def->maxvcpus < def->vcpus) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("maxvcpus must not be less than current vcpus " + "(%u < %u)"), def->maxvcpus, def->vcpus); + goto error; } tmp = virXPathString("string(./vcpu[1]/@placement)", ctxt); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a0f68ea..34b669d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2158,8 +2158,8 @@ struct _virDomainDef { virDomainBlkiotune blkio; virDomainMemtune mem; - unsigned short vcpus; - unsigned short maxvcpus; + unsigned int vcpus; + unsigned int maxvcpus; int placement_mode; virBitmapPtr cpumask; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index c40826d..05990c7 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -7279,10 +7279,6 @@ virDomainSetVcpusFlags(virDomainPtr domain, unsigned int nvcpus, virCheckNonZeroArgGoto(nvcpus, error); - if ((unsigned short) nvcpus != nvcpus) { - virReportError(VIR_ERR_OVERFLOW, _("input too large: %u"), nvcpus); - goto error; - } conn = domain->conn; if (conn->driver->domainSetVcpusFlags) { @@ -7403,11 +7399,6 @@ virDomainPinVcpu(virDomainPtr domain, unsigned int vcpu, virCheckNonNullArgGoto(cpumap, error); virCheckPositiveArgGoto(maplen, error); - if ((unsigned short) vcpu != vcpu) { - virReportError(VIR_ERR_OVERFLOW, _("input too large: %u"), vcpu); - goto error; - } - if (conn->driver->domainPinVcpu) { int ret; ret = conn->driver->domainPinVcpu(domain, vcpu, cpumap, maplen); @@ -7475,11 +7466,6 @@ virDomainPinVcpuFlags(virDomainPtr domain, unsigned int vcpu, virCheckNonNullArgGoto(cpumap, error); virCheckPositiveArgGoto(maplen, error); - if ((unsigned short) vcpu != vcpu) { - virReportError(VIR_ERR_OVERFLOW, _("input too large: %u"), vcpu); - goto error; - } - if (conn->driver->domainPinVcpuFlags) { int ret; ret = conn->driver->domainPinVcpuFlags(domain, vcpu, cpumap, maplen, flags); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 92da08d..2be26c6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4866,12 +4866,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_VCPU_GUEST, -1); - if (!nvcpus || (unsigned short) nvcpus != nvcpus) { - virReportError(VIR_ERR_INVALID_ARG, - _("argument out of range: %d"), nvcpus); - return -1; - } - if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; -- 2.4.1

In the pre-NUMA ages pinning a vCPU to all pCPUs was eaqual to deleting the pinning info. Now it does not entirely work that way. Pinning a vCPU to all pCPUs might be a desired operation. Additionally removal of the pinning will result into using the default pinning information at the next boot which might be different from all vcpus. This patch removes the false assumption that we should remove the pinning after pinning to all vCPUs and tweaks the documentation for virsh. A later patch will implement a new flag for the virDomainPinVcpuFlags API that will allow to remove the pinning in a sane way. --- src/libxl/libxl_driver.c | 9 -------- src/qemu/qemu_driver.c | 57 ++++++++++++++++-------------------------------- tools/virsh.pod | 3 +-- 3 files changed, 20 insertions(+), 49 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 9be89f2..2ff9496 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2244,14 +2244,6 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, } } - /* full bitmap means reset the settings (if any). */ - if (virBitmapIsAllSet(pcpumap)) { - virDomainPinDel(&targetDef->cputune.vcpupin, - &targetDef->cputune.nvcpupin, - vcpu); - goto done; - } - if (!targetDef->cputune.vcpupin) { if (VIR_ALLOC(targetDef->cputune.vcpupin) < 0) goto endjob; @@ -2267,7 +2259,6 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, goto endjob; } - done: ret = 0; if (flags & VIR_DOMAIN_AFFECT_LIVE) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2be26c6..96fc0d4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5029,7 +5029,6 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, virCgroupPtr cgroup_vcpu = NULL; int ret = -1; qemuDomainObjPrivatePtr priv; - bool doReset = false; size_t newVcpuPinNum = 0; virDomainPinDefPtr *newVcpuPin = NULL; virBitmapPtr pcpumap = NULL; @@ -5088,12 +5087,6 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, goto endjob; } - /* pinning to all physical cpus means resetting, - * so check if we can reset setting. - */ - if (virBitmapIsAllSet(pcpumap)) - doReset = true; - if (flags & VIR_DOMAIN_AFFECT_LIVE) { if (priv->vcpupids == NULL) { @@ -5142,19 +5135,13 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, } } - if (doReset) { - virDomainPinDel(&vm->def->cputune.vcpupin, - &vm->def->cputune.nvcpupin, - vcpu); - } else { - if (vm->def->cputune.vcpupin) - virDomainPinDefArrayFree(vm->def->cputune.vcpupin, - vm->def->cputune.nvcpupin); + if (vm->def->cputune.vcpupin) + virDomainPinDefArrayFree(vm->def->cputune.vcpupin, + vm->def->cputune.nvcpupin); - vm->def->cputune.vcpupin = newVcpuPin; - vm->def->cputune.nvcpupin = newVcpuPinNum; - newVcpuPin = NULL; - } + vm->def->cputune.vcpupin = newVcpuPin; + vm->def->cputune.nvcpupin = newVcpuPinNum; + newVcpuPin = NULL; if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto endjob; @@ -5174,26 +5161,20 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (doReset) { - virDomainPinDel(&persistentDef->cputune.vcpupin, - &persistentDef->cputune.nvcpupin, - vcpu); - } else { - if (!persistentDef->cputune.vcpupin) { - if (VIR_ALLOC(persistentDef->cputune.vcpupin) < 0) - goto endjob; - persistentDef->cputune.nvcpupin = 0; - } - if (virDomainPinAdd(&persistentDef->cputune.vcpupin, - &persistentDef->cputune.nvcpupin, - cpumap, - maplen, - vcpu) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to update or add vcpupin xml of " - "a persistent domain")); + if (!persistentDef->cputune.vcpupin) { + if (VIR_ALLOC(persistentDef->cputune.vcpupin) < 0) goto endjob; - } + persistentDef->cputune.nvcpupin = 0; + } + if (virDomainPinAdd(&persistentDef->cputune.vcpupin, + &persistentDef->cputune.nvcpupin, + cpumap, + maplen, + vcpu) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to update or add vcpupin xml of " + "a persistent domain")); + goto endjob; } ret = virDomainSaveConfig(cfg->configDir, persistentDef); diff --git a/tools/virsh.pod b/tools/virsh.pod index d588e5a..4e3f82a 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2365,8 +2365,7 @@ I<vcpu> or omit I<vcpu> to list all at once. I<cpulist> is a list of physical CPU numbers. Its syntax is a comma separated list and a special markup using '-' and '^' (ex. '0-4', '0-3,^2') can also be allowed. The '-' denotes the range and the '^' denotes exclusive. -If you want to reset vcpupin setting, that is, to pin the I<vcpu> to all -physical cpus, simply specify 'r' as a I<cpulist>. +For pinning the I<vcpu> to all physical cpus specify 'r' as a I<cpulist>. If I<--live> is specified, affect a running guest. If I<--config> is specified, affect the next boot of a persistent guest. If I<--current> is specified, affect the current guest state. -- 2.4.1

The vCPU pinning definition gets removed when the domain definition is being freed later. If there is no next configuration it would remove the configured pinning. --- src/libxl/libxl_domain.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 632e5aa..592b80e 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -690,7 +690,6 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); int vnc_port; char *file; - size_t i; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; virHostdevReAttachDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, @@ -725,16 +724,6 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, } } - /* Remove any cputune settings */ - if (vm->def->cputune.nvcpupin) { - for (i = 0; i < vm->def->cputune.nvcpupin; ++i) { - virBitmapFree(vm->def->cputune.vcpupin[i]->cpumask); - VIR_FREE(vm->def->cputune.vcpupin[i]); - } - VIR_FREE(vm->def->cputune.vcpupin); - vm->def->cputune.nvcpupin = 0; - } - if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) > 0) { if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR) VIR_DEBUG("Failed to remove domain XML for %s", vm->def->name); -- 2.4.1

virDomainLiveConfigHelperMethod that is used for this job now does modify the flags but still requires the callers to extract the correct definition objects. In addition coverity and other static analyzers are usually unhappy as they don't grasp the fact that @flags are upadted according to the correct def to be present. To work this issue around and simplify the calling chain let's add a new helper that will work only on drivers that always copy the persistent def to a transient at start of a vm. This will allow to drop a few arguments. The new function syntax will also fill two definition pointers rather than modifying the @flags parameter. --- src/conf/domain_conf.c | 113 ++++++++++++++++++++++++++++++++++++++--------- src/conf/domain_conf.h | 8 ++++ src/libvirt_private.syms | 2 + 3 files changed, 102 insertions(+), 21 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4a8c1b5..e8bda73 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2823,24 +2823,25 @@ virDomainObjGetPersistentDef(virCapsPtr caps, return domain->def; } -/* - * Helper method for --current, --live, and --config options, and check - * whether domain is active or can get persistent domain configuration. + +/** + * virDomainObjUpdateModificationImpact: * - * Return 0 if success, also change the flags and get the persistent - * domain configuration if needed. Return -1 on error. + * @vm: domain object + * @flags: flags to update the modification impact on + * + * Resolves virDomainModificationImpact flags in @flags so that they correctly + * apply to the actual state of @vm. @flags may be modified after call to this + * function. + * + * Returns 0 on success if @flags point to a valid combination for @vm or -1 on + * error. */ int -virDomainLiveConfigHelperMethod(virCapsPtr caps, - virDomainXMLOptionPtr xmlopt, - virDomainObjPtr dom, - unsigned int *flags, - virDomainDefPtr *persistentDef) +virDomainObjUpdateModificationImpact(virDomainObjPtr vm, + unsigned int *flags) { - bool isActive; - int ret = -1; - - isActive = virDomainObjIsActive(dom); + bool isActive = virDomainObjIsActive(vm); if ((*flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == VIR_DOMAIN_AFFECT_CURRENT) { @@ -2853,29 +2854,99 @@ virDomainLiveConfigHelperMethod(virCapsPtr caps, if (!isActive && (*flags & VIR_DOMAIN_AFFECT_LIVE)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); - goto cleanup; + return -1; } if (*flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!dom->persistent) { + if (!vm->persistent) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("transient domains do not have any " "persistent config")); - goto cleanup; + return -1; } + } + + return 0; +} + + +/* + * Helper method for --current, --live, and --config options, and check + * whether domain is active or can get persistent domain configuration. + * + * Return 0 if success, also change the flags and get the persistent + * domain configuration if needed. Return -1 on error. + */ +int +virDomainLiveConfigHelperMethod(virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + virDomainObjPtr dom, + unsigned int *flags, + virDomainDefPtr *persistentDef) +{ + if (virDomainObjUpdateModificationImpact(dom, flags) < 0) + return -1; + + if (*flags & VIR_DOMAIN_AFFECT_CONFIG) { if (!(*persistentDef = virDomainObjGetPersistentDef(caps, xmlopt, dom))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Get persistent config failed")); - goto cleanup; + return -1; } } - ret = 0; + return 0; +} - cleanup: - return ret; + +/** + * virDomainObjGetDefs: + * + * @vm: domain object + * @flags: for virDomainModificationImpact + * @liveDef: Set to the pointer to the live definition of @vm. + * @persDef: Set to the pointer to the config definition of @vm. + * + * Helper function to resolve @flags and retrieve correct domain pointer + * objects. This function should be used only when the hypervisor driver always + * creates vm->newDef once the vm is started. (qemu driver does that) + * + * If @liveDef or @persDef are set it implies that @flags request modification + * of thereof. + * + * Returns 0 on success and sets @liveDef and @persDef; -1 if @flags are + * inappropriate. + */ +int +virDomainObjGetDefs(virDomainObjPtr vm, + unsigned int flags, + virDomainDefPtr *liveDef, + virDomainDefPtr *persDef) +{ + if (liveDef) + *liveDef = NULL; + + if (*persDef) + *persDef = NULL; + + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + return -1; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (liveDef) + *liveDef = vm->def; + + if (persDef) + *liveDef = vm->newDef; + } else { + if (persDef) + *persDef = vm->def; + } + + return 0; } + /* * The caller must hold a lock on the driver owning 'doms', * and must also have locked 'dom', to ensure no one else diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 34b669d..262d267 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2545,6 +2545,14 @@ virDomainObjGetPersistentDef(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, virDomainObjPtr domain); +int virDomainObjUpdateModificationImpact(virDomainObjPtr vm, + unsigned int *flags); + +int virDomainObjGetDefs(virDomainObjPtr vm, + unsigned int flags, + virDomainDefPtr *liveDef, + virDomainDefPtr *persDef); + int virDomainLiveConfigHelperMethod(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7b502aa..55a5e19 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -383,6 +383,7 @@ virDomainObjAssignDef; virDomainObjCopyPersistentDef; virDomainObjEndAPI; virDomainObjFormat; +virDomainObjGetDefs; virDomainObjGetMetadata; virDomainObjGetPersistentDef; virDomainObjGetState; @@ -408,6 +409,7 @@ virDomainObjSetDefTransient; virDomainObjSetMetadata; virDomainObjSetState; virDomainObjTaint; +virDomainObjUpdateModificationImpact; virDomainOSTypeFromString; virDomainOSTypeToString; virDomainParseMemory; -- 2.4.1

--- src/qemu/qemu_driver.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1c76a1f..d729e6c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2434,10 +2434,10 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period, virQEMUDriverPtr driver = dom->conn->privateData; qemuDomainObjPrivatePtr priv; virDomainObjPtr vm; - virDomainDefPtr persistentDef = NULL; + virDomainDefPtr def; + virDomainDefPtr persistentDef; int ret = -1, r; virQEMUDriverConfigPtr cfg = NULL; - virCapsPtr caps = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -2453,17 +2453,13 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period, if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) - goto endjob; - if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, - &persistentDef) < 0) + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) goto endjob; /* Set the balloon driver collection interval */ priv = vm->privateData; - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - + if (def) { qemuDomainObjEnterMonitor(driver, vm); r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period); if (qemuDomainObjExitMonitor(driver, vm) < 0) @@ -2474,13 +2470,12 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period, goto endjob; } - vm->def->memballoon->period = period; + def->memballoon->period = period; if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto endjob; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - sa_assert(persistentDef); + if (persistentDef) { persistentDef->memballoon->period = period; ret = virDomainSaveConfig(cfg->configDir, persistentDef); goto endjob; @@ -2492,7 +2487,6 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period, cleanup: virDomainObjEndAPI(&vm); - virObjectUnref(caps); virObjectUnref(cfg); return ret; } -- 2.4.1

--- src/qemu/qemu_driver.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5969f5c..b799d6e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5756,7 +5756,6 @@ qemuDomainGetIOThreadInfo(virDomainPtr dom, { virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; - virCapsPtr caps = NULL; virDomainDefPtr targetDef = NULL; int ret = -1; @@ -5769,27 +5768,16 @@ qemuDomainGetIOThreadInfo(virDomainPtr dom, if (virDomainGetIOThreadInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + if (virDomainObjGetDefs(vm, flags, NULL, &targetDef) < 0) goto cleanup; - if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, - &targetDef) < 0) - goto cleanup; - - if (flags & VIR_DOMAIN_AFFECT_LIVE) - targetDef = vm->def; - - /* Coverity didn't realize that targetDef must be set if we got here. */ - sa_assert(targetDef); - - if (flags & VIR_DOMAIN_AFFECT_LIVE) + if (!targetDef) ret = qemuDomainGetIOThreadsLive(driver, vm, info); else ret = qemuDomainGetIOThreadsConfig(targetDef, info); cleanup: virDomainObjEndAPI(&vm); - virObjectUnref(caps); return ret; } -- 2.4.1

--- src/qemu/qemu_driver.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c1dca36..5a01514 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6151,32 +6151,22 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, unsigned int flags) { virQEMUDriverConfigPtr cfg = NULL; - virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; + virDomainDefPtr def; virDomainDefPtr persistentDef; int ret = -1; cfg = virQEMUDriverGetConfig(driver); - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) - goto cleanup; - priv = vm->privateData; if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; - if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, - &persistentDef) < 0) + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) goto endjob; - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change IOThreads for an inactive domain")); - goto endjob; - } - + if (def) { if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("IOThreads not supported with this binary")); @@ -6195,7 +6185,7 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, goto endjob; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (persistentDef) { if (add) { if (!virDomainIOThreadIDAdd(persistentDef, iothread_id)) goto endjob; @@ -6227,7 +6217,6 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, qemuDomainObjEndJob(driver, vm); cleanup: - virObjectUnref(caps); virObjectUnref(cfg); return ret; } -- 2.4.1

--- src/qemu/qemu_driver.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2aa754d..1dfba58 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5198,13 +5198,11 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, int maplen, unsigned int flags) { - - virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; - virDomainDefPtr targetDef = NULL; + virDomainDefPtr def; + virDomainDefPtr targetDef; int ret = -1; int hostcpus, vcpu; - virCapsPtr caps = NULL; virBitmapPtr allcpumap = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -5216,18 +5214,11 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, if (virDomainGetVcpuPinInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) - goto cleanup; - - if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, - &targetDef) < 0) + if (virDomainObjGetDefs(vm, flags, &def, &targetDef) < 0) goto cleanup; - if (flags & VIR_DOMAIN_AFFECT_LIVE) - targetDef = vm->def; - - /* Coverity didn't realize that targetDef must be set if we got here. */ - sa_assert(targetDef); + if (def) + targetDef = def; if ((hostcpus = nodeGetCPUCount()) < 0) goto cleanup; @@ -5265,7 +5256,6 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, cleanup: virBitmapFree(allcpumap); virDomainObjEndAPI(&vm); - virObjectUnref(caps); return ret; } -- 2.4.1

--- src/qemu/qemu_driver.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9c0f7d3..ea4e75b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5398,14 +5398,13 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom, int maplen, unsigned int flags) { - virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; - virDomainDefPtr targetDef = NULL; + virDomainDefPtr def; + virDomainDefPtr targetDef; int ret = -1; int hostcpus; virBitmapPtr cpumask = NULL; virBitmapPtr bitmap = NULL; - virCapsPtr caps = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -5416,18 +5415,11 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom, if (virDomainGetEmulatorPinInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) - goto cleanup; - - if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, - vm, &flags, &targetDef) < 0) + if (virDomainObjGetDefs(vm, flags, &def, &targetDef) < 0) goto cleanup; - if (flags & VIR_DOMAIN_AFFECT_LIVE) - targetDef = vm->def; - - /* Coverity didn't realize that targetDef must be set if we got here. */ - sa_assert(targetDef); + if (def) + targetDef = def; if ((hostcpus = nodeGetCPUCount()) < 0) goto cleanup; @@ -5449,7 +5441,6 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); - virObjectUnref(caps); virBitmapFree(bitmap); return ret; } -- 2.4.1