[libvirt] [PATCH 4/7] util: eliminate device object leaks related to virDomain*Remove*()
Laine Stump
laine at laine.org
Thu Mar 8 09:24:19 UTC 2012
There are several functions in domain_conf.c that remove a device
object from the domain's list of that object type, but don't free the
object or return it to the caller to free. In many cases this isn't a
problem because the caller already had a pointer to the object and
frees it afterward, but in several cases the removed object was just
left floating around with no references to it.
In particular, the function qemuDomainDetachDeviceConfig() calls
functions to locate and remove net (virDomainNetRemoveByMac), disk
(virDomainDiskRemoveByName()), and lease (virDomainLeaseRemove())
devices, but neither it nor its caller qemuDomainModifyDeviceConfig()
ever obtain a pointer to the device being removed, much less free it.
This patch modifies the following "remove" functions to return a
pointer to the device object being removed from the domain device
arrays, to give the caller the option of freeing the device object
using that pointer if needed. In places where the object was
previously leaked, it is now freed:
virDomainDiskRemove
virDomainDiskRemoveByName
virDomainNetRemove
virDomainNetRemoveByMac
virDomainHostdevRemove
virDomainLeaseRemove
virDomainLeaseRemoveAt
The functions that had been leaking:
libxlDomainDetachConfig - leaked a virDomainDiskDef
qemuDomainDetachDeviceConfig - could leak a virDomainDiskDef,
a virDomainNetDef, or a
virDomainLeaseDef
qemuDomainDetachLease - leaked a virDomainLeaseDef
---
src/conf/domain_conf.c | 48 +++++++++++++++++++++++++++++----------------
src/conf/domain_conf.h | 23 ++++++++++++++-------
src/libxl/libxl_driver.c | 5 ++-
src/qemu/qemu_driver.c | 15 ++++++++-----
src/qemu/qemu_hotplug.c | 4 ++-
5 files changed, 61 insertions(+), 34 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 22be8b9..fd45525 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6856,9 +6856,11 @@ virDomainHostdevInsert(virDomainDefPtr def, virDomainHostdevDefPtr hostdev)
return 0;
}
-void
+virDomainHostdevDefPtr
virDomainHostdevRemove(virDomainDefPtr def, size_t i)
{
+ virDomainHostdevDefPtr hostdev = def->hostdevs[i];
+
if (def->nhostdevs > 1) {
memmove(def->hostdevs + i,
def->hostdevs + i + 1,
@@ -6872,6 +6874,7 @@ virDomainHostdevRemove(virDomainDefPtr def, size_t i)
VIR_FREE(def->hostdevs);
def->nhostdevs = 0;
}
+ return hostdev;
}
/* Find an entry in hostdevs that matches the source spec in
@@ -7035,8 +7038,11 @@ void virDomainDiskInsertPreAlloced(virDomainDefPtr def,
}
-void virDomainDiskRemove(virDomainDefPtr def, size_t i)
+virDomainDiskDefPtr
+virDomainDiskRemove(virDomainDefPtr def, size_t i)
{
+ virDomainDiskDefPtr disk = disk = def->disks[i];
+
if (def->ndisks > 1) {
memmove(def->disks + i,
def->disks + i + 1,
@@ -7050,15 +7056,16 @@ void virDomainDiskRemove(virDomainDefPtr def, size_t i)
VIR_FREE(def->disks);
def->ndisks = 0;
}
+ return disk;
}
-int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name)
+virDomainDiskDefPtr
+virDomainDiskRemoveByName(virDomainDefPtr def, const char *name)
{
int i = virDomainDiskIndexByName(def, name, false);
if (i < 0)
- return -1;
- virDomainDiskRemove(def, i);
- return 0;
+ return NULL;
+ return virDomainDiskRemove(def, i);
}
int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net)
@@ -7084,7 +7091,8 @@ int virDomainNetIndexByMac(virDomainDefPtr def, const unsigned char *mac)
return -1;
}
-void virDomainNetRemove(virDomainDefPtr def, size_t i)
+virDomainNetDefPtr
+virDomainNetRemove(virDomainDefPtr def, size_t i)
{
virDomainNetDefPtr net = def->nets[i];
@@ -7115,16 +7123,17 @@ void virDomainNetRemove(virDomainDefPtr def, size_t i)
VIR_FREE(def->nets);
def->nnets = 0;
}
+ return net;
}
-int virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac)
+virDomainNetDefPtr
+virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac)
{
int i = virDomainNetIndexByMac(def, mac);
if (i < 0)
- return -1;
- virDomainNetRemove(def, i);
- return 0;
+ return NULL;
+ return virDomainNetRemove(def, i);
}
@@ -7233,8 +7242,12 @@ void virDomainLeaseInsertPreAlloced(virDomainDefPtr def,
}
-void virDomainLeaseRemoveAt(virDomainDefPtr def, size_t i)
+virDomainLeaseDefPtr
+virDomainLeaseRemoveAt(virDomainDefPtr def, size_t i)
{
+
+ virDomainLeaseDefPtr lease = def->leases[i];
+
if (def->nleases > 1) {
memmove(def->leases + i,
def->leases + i + 1,
@@ -7245,17 +7258,18 @@ void virDomainLeaseRemoveAt(virDomainDefPtr def, size_t i)
VIR_FREE(def->leases);
def->nleases = 0;
}
+ return lease;
}
-int virDomainLeaseRemove(virDomainDefPtr def,
- virDomainLeaseDefPtr lease)
+virDomainLeaseDefPtr
+virDomainLeaseRemove(virDomainDefPtr def,
+ virDomainLeaseDefPtr lease)
{
int i = virDomainLeaseIndex(def, lease);
if (i < 0)
- return -1;
- virDomainLeaseRemoveAt(def, i);
- return 0;
+ return NULL;
+ return virDomainLeaseRemoveAt(def, i);
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9138ea2..7720fbc 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1898,16 +1898,21 @@ void virDomainDiskInsertPreAlloced(virDomainDefPtr def,
virDomainDiskDefPtr disk);
int virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def);
-void virDomainDiskRemove(virDomainDefPtr def, size_t i);
-int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name);
+virDomainDiskDefPtr
+virDomainDiskRemove(virDomainDefPtr def, size_t i);
+virDomainDiskDefPtr
+virDomainDiskRemoveByName(virDomainDefPtr def, const char *name);
int virDomainNetIndexByMac(virDomainDefPtr def, const unsigned char *mac);
int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net);
-void virDomainNetRemove(virDomainDefPtr def, size_t i);
-int virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac);
+virDomainNetDefPtr
+virDomainNetRemove(virDomainDefPtr def, size_t i);
+virDomainNetDefPtr
+virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac);
int virDomainHostdevInsert(virDomainDefPtr def, virDomainHostdevDefPtr hostdev);
-void virDomainHostdevRemove(virDomainDefPtr def, size_t i);
+virDomainHostdevDefPtr
+virDomainHostdevRemove(virDomainDefPtr def, size_t i);
int virDomainHostdevFind(virDomainDefPtr def, virDomainHostdevDefPtr match,
virDomainHostdevDefPtr *found);
@@ -1952,9 +1957,11 @@ int virDomainLeaseInsert(virDomainDefPtr def,
int virDomainLeaseInsertPreAlloc(virDomainDefPtr def);
void virDomainLeaseInsertPreAlloced(virDomainDefPtr def,
virDomainLeaseDefPtr lease);
-void virDomainLeaseRemoveAt(virDomainDefPtr def, size_t i);
-int virDomainLeaseRemove(virDomainDefPtr def,
- virDomainLeaseDefPtr lease);
+virDomainLeaseDefPtr
+virDomainLeaseRemoveAt(virDomainDefPtr def, size_t i);
+virDomainLeaseDefPtr
+virDomainLeaseRemove(virDomainDefPtr def,
+ virDomainLeaseDefPtr lease);
int virDomainSaveXML(const char *configDir,
virDomainDefPtr def,
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index a7bb751..177b218 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3089,17 +3089,18 @@ libxlDomainDetachDeviceLive(libxlDomainObjPrivatePtr priv, virDomainObjPtr vm,
static int
libxlDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
{
- virDomainDiskDefPtr disk;
+ virDomainDiskDefPtr disk, detach;
int ret = -1;
switch (dev->type) {
case VIR_DOMAIN_DEVICE_DISK:
disk = dev->data.disk;
- if (virDomainDiskRemoveByName(vmdef, disk->dst)) {
+ if (!(detach = virDomainDiskRemoveByName(vmdef, disk->dst))) {
libxlError(VIR_ERR_INVALID_ARG,
_("no target device %s"), disk->dst);
break;
}
+ virDomainDiskDefFree(detach);
ret = 0;
break;
default:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7b6e747..af89029 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5434,23 +5434,24 @@ static int
qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
virDomainDeviceDefPtr dev)
{
- virDomainDiskDefPtr disk;
- virDomainNetDefPtr net;
- virDomainLeaseDefPtr lease;
+ virDomainDiskDefPtr disk, det_disk;
+ virDomainNetDefPtr net, det_net;
+ virDomainLeaseDefPtr lease, det_lease;
switch (dev->type) {
case VIR_DOMAIN_DEVICE_DISK:
disk = dev->data.disk;
- if (virDomainDiskRemoveByName(vmdef, disk->dst)) {
+ if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) {
qemuReportError(VIR_ERR_INVALID_ARG,
_("no target device %s"), disk->dst);
return -1;
}
+ virDomainDiskDefFree(det_disk);
break;
case VIR_DOMAIN_DEVICE_NET:
net = dev->data.net;
- if (virDomainNetRemoveByMac(vmdef, net->mac)) {
+ if (!(det_net = virDomainNetRemoveByMac(vmdef, net->mac))) {
char macbuf[VIR_MAC_STRING_BUFLEN];
virMacAddrFormat(net->mac, macbuf);
@@ -5458,16 +5459,18 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
_("no nic of mac %s"), macbuf);
return -1;
}
+ virDomainNetDefFree(det_net);
break;
case VIR_DOMAIN_DEVICE_LEASE:
lease = dev->data.lease;
- if (virDomainLeaseRemove(vmdef, lease) < 0) {
+ if (!(det_lease = virDomainLeaseRemove(vmdef, lease))) {
qemuReportError(VIR_ERR_INVALID_ARG,
_("Lease %s in lockspace %s does not exist"),
lease->key, NULLSTR(lease->lockspace));
return -1;
}
+ virDomainLeaseDefFree(det_lease);
break;
default:
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 22d0231..1e56354 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2286,6 +2286,7 @@ int qemuDomainDetachLease(struct qemud_driver *driver,
virDomainObjPtr vm,
virDomainLeaseDefPtr lease)
{
+ virDomainLeaseDefPtr det_lease;
int i;
if ((i = virDomainLeaseIndex(vm->def, lease)) < 0) {
@@ -2298,6 +2299,7 @@ int qemuDomainDetachLease(struct qemud_driver *driver,
if (virDomainLockLeaseDetach(driver->lockManager, vm, lease) < 0)
return -1;
- virDomainLeaseRemoveAt(vm->def, i);
+ det_lease = virDomainLeaseRemoveAt(vm->def, i);
+ virDomainLeaseDefFree(det_lease);
return 0;
}
--
1.7.7.6
More information about the libvir-list
mailing list