[libvirt] [PATCH 0/3] Couple of cleanups in qemu code
Michal Privoznik (3): qemu: Fix mem leak in qemuProcessInitCpuAffinity gitignore: Reorder alphabetically qemu: Split if condition in qemuDomainSnapshotUndoSingleDiskActive .gitignore | 6 +++--- src/qemu/qemu_driver.c | 9 +++++++-- src/qemu/qemu_process.c | 23 +++++++++++++---------- 3 files changed, 23 insertions(+), 15 deletions(-) -- 1.7.8.5
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 */ -- 1.7.8.5
On 04/13/2012 05:12 PM, Michal Privoznik wrote:
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 */
ACK.
On 13.04.2012 12:24, Osier Yang wrote:
On 04/13/2012 05:12 PM, Michal Privoznik wrote:
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(-)
ACK.
Thanks, pushed. Michal
Recent git reorders .gitignore alphabetically. However, changes are not committed and I am tired of discarding these changes from my patches. --- .gitignore | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index 5aa9c9b..14a21d0 100644 --- a/.gitignore +++ b/.gitignore @@ -48,12 +48,12 @@ /daemon/*_dispatch.h /daemon/libvirt_qemud /daemon/libvirtd -/daemon/libvirtd.init -/daemon/libvirtd.service /daemon/libvirtd*.logrotate /daemon/libvirtd.8 /daemon/libvirtd.8.in +/daemon/libvirtd.init /daemon/libvirtd.pod +/daemon/libvirtd.service /docs/devhelp/libvirt.devhelp /docs/hvsupport.html.in /docs/libvirt-api.xml @@ -118,6 +118,7 @@ /tests/eventtest /tests/hashtest /tests/jsontest +/tests/libvirtdconftest /tests/networkxml2argvtest /tests/nodeinfotest /tests/nwfilterxml2xmltest @@ -150,7 +151,6 @@ /tests/vmx2xmltest /tests/xencapstest /tests/xmconfigtest -/tests/libvirtdconftest /tools/*.[18] /tools/libvirt-guests.init /tools/virsh -- 1.7.8.5
On 04/13/2012 05:12 PM, Michal Privoznik wrote:
Recent git reorders .gitignore alphabetically. However, changes are not committed and I am tired of discarding these changes from my patches. --- .gitignore | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/.gitignore b/.gitignore index 5aa9c9b..14a21d0 100644 --- a/.gitignore +++ b/.gitignore @@ -48,12 +48,12 @@ /daemon/*_dispatch.h /daemon/libvirt_qemud /daemon/libvirtd -/daemon/libvirtd.init -/daemon/libvirtd.service /daemon/libvirtd*.logrotate /daemon/libvirtd.8 /daemon/libvirtd.8.in +/daemon/libvirtd.init /daemon/libvirtd.pod +/daemon/libvirtd.service /docs/devhelp/libvirt.devhelp /docs/hvsupport.html.in /docs/libvirt-api.xml @@ -118,6 +118,7 @@ /tests/eventtest /tests/hashtest /tests/jsontest +/tests/libvirtdconftest /tests/networkxml2argvtest /tests/nodeinfotest /tests/nwfilterxml2xmltest @@ -150,7 +151,6 @@ /tests/vmx2xmltest /tests/xencapstest /tests/xmconfigtest -/tests/libvirtdconftest /tools/*.[18] /tools/libvirt-guests.init /tools/virsh
ACK Osier
On 13.04.2012 14:28, Osier Yang wrote:
On 04/13/2012 05:12 PM, Michal Privoznik wrote:
Recent git reorders .gitignore alphabetically. However, changes are not committed and I am tired of discarding these changes from my patches. --- .gitignore | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
ACK
Osier
Thanks, pushed. Michal
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); -- 1.7.8.5
On Fri, Apr 13, 2012 at 11:12:54AM +0200, Michal Privoznik wrote:
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.
Really ? I'm not sure I believe that. IIRC in-order short-circuit evaluation is a part of the C standard. Compilers can't do any optimization which changes the order of evalation without breaking countless C programs.
--- 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);
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 04/13/2012 03:39 AM, Daniel P. Berrange wrote:
On Fri, Apr 13, 2012 at 11:12:54AM +0200, Michal Privoznik wrote:
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.
Really ? I'm not sure I believe that. IIRC in-order short-circuit evaluation is a part of the C standard. Compilers can't do any optimization which changes the order of evalation without breaking countless C programs.
I concur - NACK to this patch. Any C compiler that violates short-circuiting semantics is too severely broken to be worth working around in our code. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Although it should be harmless to do: disk = disk = def->disks[i] some not-so-wise compilers may fool around. Besides, such assignment is useless here. --- src/conf/domain_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c6b97e1..a9c5cbc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7124,7 +7124,7 @@ void virDomainDiskInsertPreAlloced(virDomainDefPtr def, virDomainDiskDefPtr virDomainDiskRemove(virDomainDefPtr def, size_t i) { - virDomainDiskDefPtr disk = disk = def->disks[i]; + virDomainDiskDefPtr disk = def->disks[i]; if (def->ndisks > 1) { memmove(def->disks + i, -- 1.7.8.5
On 04/13/2012 05:24 PM, Michal Privoznik wrote:
Although it should be harmless to do: disk = disk = def->disks[i] some not-so-wise compilers may fool around. Besides, such assignment is useless here. --- src/conf/domain_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c6b97e1..a9c5cbc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7124,7 +7124,7 @@ void virDomainDiskInsertPreAlloced(virDomainDefPtr def, virDomainDiskDefPtr virDomainDiskRemove(virDomainDefPtr def, size_t i) { - virDomainDiskDefPtr disk = disk = def->disks[i]; + virDomainDiskDefPtr disk = def->disks[i];
if (def->ndisks> 1) { memmove(def->disks + i,
ACK
On 13.04.2012 12:25, Osier Yang wrote:
On 04/13/2012 05:24 PM, Michal Privoznik wrote:
Although it should be harmless to do: disk = disk = def->disks[i] some not-so-wise compilers may fool around. Besides, such assignment is useless here. --- src/conf/domain_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
ACK
Thanks, pushed. Michal
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik -
Osier Yang