[libvirt] [PATCH 1/2] qemuDomainDetachChrDevice: Don't leak @charAlias
Moreover, since virAsprintf now does report OOM error, there's no need to call virReportOOMError in error path. --- src/qemu/qemu_hotplug.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 788ad47..a98de9d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3155,10 +3155,8 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0) return ret; - if (virAsprintf(&charAlias, "char%s", tmpChr->info.alias) < 0) { - virReportOOMError(); - return ret; - } + if (virAsprintf(&charAlias, "char%s", tmpChr->info.alias) < 0) + goto cleanup; qemuDomainObjEnterMonitor(driver, vm); if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { -- 1.8.1.5
If testQemuHotplugAttach succeeds, the vm->def steals the dev pointer. However, not the envelope, which needs to be freed. In addition, driver.config is allocated, but never freed. --- tests/qemuhotplugtest.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index d4971c2..5e8c5f0 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -89,6 +89,10 @@ testQemuHotplugAttach(virDomainObjPtr vm, switch (dev->type) { case VIR_DOMAIN_DEVICE_CHR: ret = qemuDomainAttachChrDevice(&driver, vm, dev->data.chr); + if (!ret) { + /* vm->def stolen dev->data.chr so we ought to avoid freeing it */ + dev->data.chr = NULL; + } break; default: if (virTestGetVerbose()) @@ -214,11 +218,6 @@ testQemuHotplug(const void *data) switch (test->action) { case ATTACH: ret = testQemuHotplugAttach(vm, dev); - if (!ret) { - /* avoid @dev double free on success, - * as @dev is part of vm->def now */ - dev = NULL; - } break; case DETACH: @@ -323,6 +322,7 @@ mymain(void) virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); + virObjectUnref(driver.config); return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.8.1.5
On 07/18/13 12:44, Michal Privoznik wrote:
If testQemuHotplugAttach succeeds, the vm->def steals the dev pointer. However, not the envelope, which needs to be freed. In addition, driver.config is allocated, but never freed. --- tests/qemuhotplugtest.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index d4971c2..5e8c5f0 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -89,6 +89,10 @@ testQemuHotplugAttach(virDomainObjPtr vm, switch (dev->type) { case VIR_DOMAIN_DEVICE_CHR: ret = qemuDomainAttachChrDevice(&driver, vm, dev->data.chr); + if (!ret) {
For integer comparisons I prefer "if (ret == 0)", this looks like returning a pointer at first glance.
+ /* vm->def stolen dev->data.chr so we ought to avoid freeing it */ + dev->data.chr = NULL; + } break; default: if (virTestGetVerbose())
ACK anyways. Peter
s/@charAlias/@devstr/ On 07/18/13 12:44, Michal Privoznik wrote:
Moreover, since virAsprintf now does report OOM error, there's no need to call virReportOOMError in error path. --- src/qemu/qemu_hotplug.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 788ad47..a98de9d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3155,10 +3155,8 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0) return ret;
- if (virAsprintf(&charAlias, "char%s", tmpChr->info.alias) < 0) { - virReportOOMError(); - return ret; - } + if (virAsprintf(&charAlias, "char%s", tmpChr->info.alias) < 0) + goto cleanup;
This actually would leak devstr from the function called above.
qemuDomainObjEnterMonitor(driver, vm); if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) {
ACK when you adjust the commit message Peter
participants (2)
-
Michal Privoznik -
Peter Krempa