If placement mode is AUTO, on some return paths char *cpumap or char *nodeset are leaked. --- src/qemu/qemu_process.c | 23 +++++++++++++---------- 1 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 19569cf..692fc32 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1796,6 +1796,7 @@ static int qemuProcessInitCpuAffinity(struct qemud_driver *driver, virDomainObjPtr vm) { + int ret = -1; int i, hostcpus, maxcpu = QEMUD_CPUMASK_LEN; virNodeInfo nodeinfo; unsigned char *cpumap; @@ -1824,19 +1825,21 @@ qemuProcessInitCpuAffinity(struct qemud_driver *driver, nodeset = qemuGetNumadAdvice(vm->def); if (!nodeset) - return -1; + goto cleanup; if (VIR_ALLOC_N(tmp_cpumask, VIR_DOMAIN_CPUMASK_LEN)< 0) { virReportOOMError(); - return -1; + VIR_FREE(nodeset); + goto cleanup; } if (virDomainCpuSetParse(nodeset, 0, tmp_cpumask, VIR_DOMAIN_CPUMASK_LEN)< 0) { VIR_FREE(tmp_cpumask); VIR_FREE(nodeset); - return -1; + goto cleanup; } + VIR_FREE(nodeset); for (i = 0; i< maxcpu&& i< VIR_DOMAIN_CPUMASK_LEN; i++) { if (tmp_cpumask[i]) @@ -1849,7 +1852,6 @@ qemuProcessInitCpuAffinity(struct qemud_driver *driver, VIR_WARN("Unable to save status on vm %s after state change", vm->def->name); } - VIR_FREE(nodeset); } else { if (vm->def->cpumask) { /* XXX why don't we keep 'cpumask' in the libvirt cpumap @@ -1872,13 +1874,14 @@ qemuProcessInitCpuAffinity(struct qemud_driver *driver, * running at this point */ if (virProcessInfoSetAffinity(0, /* Self */ - cpumap, cpumaplen, maxcpu)< 0) { - VIR_FREE(cpumap); - return -1; - } + cpumap, cpumaplen, maxcpu)< 0) + goto cleanup; + + ret = 0; + +cleanup: VIR_FREE(cpumap); - - return 0; + return ret; } /* set link states to down on interfaces at qemu start */

Since compilers are trying to optimize code they are allowed to reorder evaluation of conditions in if statement (okay, not in all cases, but they can in this one). Therefore if we do: if (stat(file, &st) == 0 && unlink(file) < 0) after compiler chews this it may get feeling that swapping order is a good idea. However, we obviously don't want to call stat() on just unlink()-ed file.

--- src/qemu/qemu_driver.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65ed290..037d45c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9998,9 +9998,14 @@ qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver *driver, VIR_WARN("Unable to restore security label on %s", disk->src); if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) VIR_WARN("Unable to release lock on %s", disk->src); + + /* Deliberately do not join these two ifs. Compiler may mix up + * the order of evaluation so unlink() may proceed stat() + * which is not what we want */ if (need_unlink && stat(disk->src, &st) == 0 && - st.st_size == 0 && S_ISREG(st.st_mode) && unlink(disk->src) < 0) - VIR_WARN("Unable to remove just-created %s", disk->src); + st.st_size == 0 && S_ISREG(st.st_mode)) + if (unlink(disk->src) < 0) + VIR_WARN("Unable to remove just-created %s", disk->src); /* Update vm in place to match changes. */ VIR_FREE(disk->src);