3:24 a.m.
This series contains 7 patches that are mostly related by the face
that I noticed the problems they're solving while writing the
<interface type='hostdev'> code during the past couple weeks. After
that series was pushed, I sat down to fix everything I'd noticed
before I forgot about it. I'm sending it as a single series rather
than individual patches just to that they're easier to keep track of.
3:24 a.m.
New subject: [libvirt] [PATCH 1/7] util: consolidate duplicated error messages in virnetlink.c
There are special stub versions of all public functions in this file
that are compiled when either libnl isn't available or the platform
isn't linux. Each of these functions had two almost identical message,
differing only in the function name included in the message. Since log
messages already contain the function name, we can just define a const
char* with the common part of the string, and use that same string for
all the log messages.
Also, rather than doing #if defined ... #else ... #endif *inside the
error log macro invocation*, this patch does #if defined ... just
once, using it to decide which single string to define. This turns the
error log in each function from 6 lines, to 1 line.
---
src/util/virnetlink.c | 47 ++++++++++++-----------------------------------
1 files changed, 12 insertions(+), 35 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index 59f3e39..77dde9f 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -525,17 +525,18 @@ cleanup:
#else
+# if defined(__linux) && !defined(HAVE_LIBNL)
+static const char *unsupported = _("libnl was not available at build time");
+# else
+static const char *unsupported = _("not supported on non-linux platforms");
+# endif
+
int virNetlinkCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED,
unsigned char **respbuf ATTRIBUTE_UNUSED,
unsigned int *respbuflen ATTRIBUTE_UNUSED,
int nl_pid ATTRIBUTE_UNUSED)
{
- netlinkError(VIR_ERR_INTERNAL_ERROR, "%s",
-# if defined(__linux__) && !defined(HAVE_LIBNL)
- _("virNetlinkCommand is not supported since libnl was not
available"));
-# else
- _("virNetlinkCommand is not supported on non-linux
platforms"));
-# endif
+ netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", unsupported);
return -1;
}
@@ -545,11 +546,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED,
*/
int virNetlinkEventServiceStop(void)
{
-# if defined(__linux__) && !defined(HAVE_LIBNL)
- netlinkError(VIR_ERR_INTERNAL_ERROR,
- "%s",
- _("virNetlinkEventServiceStop is not supported since libnl was not
available"));
-# endif
+ netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", unsupported);
return 0;
}
@@ -559,11 +556,7 @@ int virNetlinkEventServiceStop(void)
*/
int virNetlinkEventServiceStart(void)
{
-# if defined(__linux__) && !defined(HAVE_LIBNL)
- netlinkError(VIR_ERR_INTERNAL_ERROR,
- "%s",
- _("virNetlinkEventServiceStart is not supported since libnl was not
available"));
-# endif
+ netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", unsupported);
return 0;
}
@@ -573,11 +566,7 @@ int virNetlinkEventServiceStart(void)
*/
bool virNetlinkEventServiceIsRunning(void)
{
-# if defined(__linux__) && !defined(HAVE_LIBNL)
- netlinkError(VIR_ERR_INTERNAL_ERROR,
- "%s",
- _("virNetlinkEventServiceIsRunning is not supported since libnl was
not available"));
-# endif
+ netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", unsupported);
return 0;
}
@@ -590,13 +579,7 @@ int virNetlinkEventAddClient(virNetlinkEventHandleCallback handleCB
ATTRIBUTE_UN
void *opaque ATTRIBUTE_UNUSED,
const unsigned char *macaddr ATTRIBUTE_UNUSED)
{
- netlinkError(VIR_ERR_INTERNAL_ERROR,
- "%s",
-# if defined(__linux__) && !defined(HAVE_LIBNL)
- _("virNetlinkEventServiceAddClient is not supported since libnl was
not available"));
-# else
- _("virNetlinkEventServiceAddClient is not supported on non-linux
platforms"));
-# endif
+ netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", unsupported);
return -1;
}
@@ -606,13 +589,7 @@ int virNetlinkEventAddClient(virNetlinkEventHandleCallback handleCB
ATTRIBUTE_UN
int virNetlinkEventRemoveClient(int watch ATTRIBUTE_UNUSED,
const unsigned char *macaddr ATTRIBUTE_UNUSED)
{
- netlinkError(VIR_ERR_INTERNAL_ERROR,
- "%s",
-# if defined(__linux__) && !defined(HAVE_LIBNL)
- _("virNetlinkEventRemoveClient is not supported since libnl was not
available"));
-# else
- _("virNetlinkEventRemoveClient is not supported on non-linux
platforms"));
-# endif
+ netlinkError(VIR_ERR_INTERNAL_ERROR, "%s", unsupported);
return -1;
}
--
1.7.7.6
12:29 p.m.
New subject: [libvirt] [PATCH 1/7] util: consolidate duplicated error messages in virnetlink.c
On 03/08/2012 02:24 AM, Laine Stump wrote:
There are special stub versions of all public functions in this file
that are compiled when either libnl isn't available or the platform
isn't linux. Each of these functions had two almost identical message,
differing only in the function name included in the message. Since log
messages already contain the function name, we can just define a const
char* with the common part of the string, and use that same string for
all the log messages.
Also, rather than doing #if defined ... #else ... #endif *inside the
error log macro invocation*, this patch does #if defined ... just
once, using it to decide which single string to define. This turns the
error log in each function from 6 lines, to 1 line.
---
src/util/virnetlink.c | 47 ++++++++++++-----------------------------------
1 files changed, 12 insertions(+), 35 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index 59f3e39..77dde9f 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -525,17 +525,18 @@ cleanup:
#else
This else matches up to an earlier:
#if defined(__linux__) && defined(HAVE_LIBNL)
+# if defined(__linux) && !defined(HAVE_LIBNL)
Which means the && !defined(HAVE_LIBNL) is redundant here; you could
simplify to:
# ifdef __linux
and get the same results.
+static const char *unsupported = _("libnl was not available at
build time");
+# else
+static const char *unsupported = _("not supported on non-linux platforms");
+# endif
GCC correctly complains that the initializer is not a constant. This
needs to be N_()...
+ netlinkError(VIR_ERR_INTERNAL_ERROR, "%s",
unsupported);
and all of these changed to _(unsupported).
ACK to the idea, once you fix the compilation errors that you get on
non-Linux or with HAVE_LIBNL forcefully undefined in config.h.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:24 a.m.
New subject: [libvirt] [PATCH 2/7] util: make virDomainLeaseDefFree global
It will be used in a different file in an upcoming patch.
---
src/conf/domain_conf.c | 2 +-
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
3 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 601dc8b..22be8b9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -887,7 +887,7 @@ void virDomainInputDefFree(virDomainInputDefPtr def)
VIR_FREE(def);
}
-static void virDomainLeaseDefFree(virDomainLeaseDefPtr def)
+void virDomainLeaseDefFree(virDomainLeaseDefPtr def)
{
if (!def)
return;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 23c1947..9138ea2 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1771,6 +1771,7 @@ bool virDomainObjTaint(virDomainObjPtr obj,
void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def);
void virDomainInputDefFree(virDomainInputDefPtr def);
void virDomainDiskDefFree(virDomainDiskDefPtr def);
+void virDomainLeaseDefFree(virDomainLeaseDefPtr def);
void virDomainDiskHostDefFree(virDomainDiskHostDefPtr def);
int virDomainDiskFindControllerModel(virDomainDefPtr def,
virDomainDiskDefPtr disk,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1439d3b..b570922 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -365,6 +365,7 @@ virDomainHubTypeToString;
virDomainInputDefFree;
virDomainIoEventFdTypeFromString;
virDomainIoEventFdTypeToString;
+virDomainLeaseDefFree;
virDomainLeaseIndex;
virDomainLeaseInsert;
virDomainLeaseInsertPreAlloc;
--
1.7.7.6
3:24 a.m.
New subject: [libvirt] [PATCH 3/7] qemu: don't 'remove' hostdev objects from domain if operation fails
There were certain paths through the hostdev detach code that could
lead to the lower level function failing (and not removing the object
from the domain's hostdevs list), but the higher level function
free'ing the hostdev object anyway. This would leave a stale
hostdevdef pointer in the list, which would surely cause a problem
eventually.
This patch relocates virDomainHostdevRemove from the lower level
functions qemuDomainDetachThisHostDevice and
qemuDomainDetachHostPciDevice, to their caller
qemuDomainDetachThisHostDevice, placing it just before the call to
virDomainHostdevDefFree. This makes it easy to verify that either both
operations are done, or neither.
NB: The "dangling pointer" part of this problem was introduced in
commit 13d5a6, so it is not present in libvirt versions prior to
0.9.9. Earlier versions would return failure in certain cases even
though the the device object was removed/deleted, but the removal and
deletion operations would always both happen or neither.
---
src/qemu/qemu_hotplug.c | 28 ++++++++++++----------------
1 files changed, 12 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c5ed9f7..22d0231 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1852,8 +1852,7 @@ cleanup:
static int
qemuDomainDetachHostPciDevice(struct qemud_driver *driver,
virDomainObjPtr vm,
- virDomainHostdevDefPtr detach,
- int idx)
+ virDomainHostdevDefPtr detach)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainHostdevSubsysPtr subsys = &detach->source.subsys;
@@ -1914,15 +1913,13 @@ qemuDomainDetachHostPciDevice(struct qemud_driver *driver,
detach->info->addr.pci.slot) < 0)
VIR_WARN("Unable to release PCI address on host device");
- virDomainHostdevRemove(vm->def, idx);
return ret;
}
static int
qemuDomainDetachHostUsbDevice(struct qemud_driver *driver,
virDomainObjPtr vm,
- virDomainHostdevDefPtr detach,
- int idx)
+ virDomainHostdevDefPtr detach)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainHostdevSubsysPtr subsys = &detach->source.subsys;
@@ -1956,8 +1953,6 @@ qemuDomainDetachHostUsbDevice(struct qemud_driver *driver,
VIR_WARN("Unable to find device %03d.%03d in list of used USB
devices",
subsys->u.usb.bus, subsys->u.usb.device);
}
-
- virDomainHostdevRemove(vm->def, idx);
return ret;
}
@@ -1987,10 +1982,10 @@ int qemuDomainDetachThisHostDevice(struct qemud_driver *driver,
switch (detach->source.subsys.type) {
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
- ret = qemuDomainDetachHostPciDevice(driver, vm, detach, idx);
- break;
+ ret = qemuDomainDetachHostPciDevice(driver, vm, detach);
+ break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
- ret = qemuDomainDetachHostUsbDevice(driver, vm, detach, idx);
+ ret = qemuDomainDetachHostUsbDevice(driver, vm, detach);
break;
default:
qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -1999,13 +1994,14 @@ int qemuDomainDetachThisHostDevice(struct qemud_driver *driver,
return -1;
}
- if (ret == 0 &&
- virSecurityManagerRestoreHostdevLabel(driver->securityManager,
- vm->def, detach) < 0) {
- VIR_WARN("Failed to restore host device labelling");
+ if (!ret) {
+ if (virSecurityManagerRestoreHostdevLabel(driver->securityManager,
+ vm->def, detach) < 0) {
+ VIR_WARN("Failed to restore host device labelling");
+ }
+ virDomainHostdevRemove(vm->def, idx);
+ virDomainHostdevDefFree(detach);
}
-
- virDomainHostdevDefFree(detach);
return ret;
}
--
1.7.7.6
3:24 a.m.
New subject: [libvirt] [PATCH 4/7] util: eliminate device object leaks related to virDomain*Remove*()
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
3:24 a.m.
New subject: [libvirt] [PATCH 5/7] util: standardize return from functions calling virNetlinkCommand
There are several functions that call virNetlinkCommand, and they all
follow a common pattern, with three exit labels: err_exit (or
cleanup), malformed_resp, and buffer_too_small. All three of these
labels do their own cleanup and have their own return. However, the
malformed_resp label usually frees the same items as the
cleanup/err_exit label, and the buffer_too_small label just doesn't
free recvbuf (because it's known to always be NULL at the time we goto
buffer_too_small.
In order to simplify and standardize the code, I've made the following
changes to all of these functions:
1) err_exit is replaced with the more libvirt-ish "cleanup", which
makes sense because in all cases this code is also executed in the
case of success, so labelling it err_exit may be confusing.
2) rc is initialized to -1, and set to 0 just before the cleanup
label. Any code that currently sets rc = -1 is made to instead goto
cleanup.
3) malformed_resp and buffer_too_small just log their error and goto
cleanup. This gives us a single return path, and a single place to
free up resources.
4) In one instance, rather then logging an error immediately, a char*
msg was pointed to an error string, then goto cleanup (and cleanup
would log an error if msg != NULL). It takes no more lines of code
to just log the message as we encounter it.
This patch should have 0 functional effects.
---
src/util/virnetdev.c | 38 ++++------------
src/util/virnetdevmacvlan.c | 37 +++++-----------
src/util/virnetdevvportprofile.c | 87 +++++++++++++++++---------------------
3 files changed, 60 insertions(+), 102 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 2cc699a..6eec5cf 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1249,7 +1249,7 @@ virNetDevLinkDump(const char *ifname, int ifindex,
unsigned char **recvbuf,
uint32_t (*getPidFunc)(void))
{
- int rc = 0;
+ int rc = -1;
struct nlmsghdr *resp;
struct nlmsgerr *err;
struct ifinfomsg ifinfo = {
@@ -1284,15 +1284,12 @@ virNetDevLinkDump(const char *ifname, int ifindex,
if (!nltarget_kernel) {
pid = getPidFunc();
if (pid == 0) {
- rc = -1;
goto cleanup;
}
}
- if (virNetlinkCommand(nl_msg, recvbuf, &recvbuflen, pid) < 0) {
- rc = -1;
+ if (virNetlinkCommand(nl_msg, recvbuf, &recvbuflen, pid) < 0)
goto cleanup;
- }
if (recvbuflen < NLMSG_LENGTH(0) || *recvbuf == NULL)
goto malformed_resp;
@@ -1309,7 +1306,7 @@ virNetDevLinkDump(const char *ifname, int ifindex,
virReportSystemError(-err->error,
_("error dumping %s (%d) interface"),
ifname, ifindex);
- rc = -1;
+ goto cleanup;
}
break;
@@ -1324,29 +1321,22 @@ virNetDevLinkDump(const char *ifname, int ifindex,
default:
goto malformed_resp;
}
-
- if (rc != 0)
- VIR_FREE(*recvbuf);
-
+ rc = 0;
cleanup:
+ if (rc < 0)
+ VIR_FREE(*recvbuf);
nlmsg_free(nl_msg);
-
return rc;
malformed_resp:
- nlmsg_free(nl_msg);
-
virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
_("malformed netlink response message"));
- VIR_FREE(*recvbuf);
- return -1;
+ goto cleanup;
buffer_too_small:
- nlmsg_free(nl_msg);
-
virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
_("allocated netlink buffer is too small"));
- return -1;
+ goto cleanup;
}
static int
@@ -1457,28 +1447,20 @@ virNetDevSetVfConfig(const char *ifname, int ifindex, int vf,
}
rc = 0;
-
cleanup:
nlmsg_free(nl_msg);
-
VIR_FREE(recvbuf);
-
return rc;
malformed_resp:
- nlmsg_free(nl_msg);
-
virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
_("malformed netlink response message"));
- VIR_FREE(recvbuf);
- return rc;
+ goto cleanup;
buffer_too_small:
- nlmsg_free(nl_msg);
-
virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
_("allocated netlink buffer is too small"));
- return rc;
+ goto cleanup;
}
static int
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 647679f..d467990 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -100,7 +100,7 @@ virNetDevMacVLanCreate(const char *ifname,
uint32_t macvlan_mode,
int *retry)
{
- int rc = 0;
+ int rc = -1;
struct nlmsghdr *resp;
struct nlmsgerr *err;
struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
@@ -155,7 +155,6 @@ virNetDevMacVLanCreate(const char *ifname,
nla_nest_end(nl_msg, linkinfo);
if (virNetlinkCommand(nl_msg, &recvbuf, &recvbuflen, 0) < 0) {
- rc = -1;
goto cleanup;
}
@@ -177,14 +176,13 @@ virNetDevMacVLanCreate(const char *ifname,
case -EEXIST:
*retry = 1;
- rc = -1;
- break;
+ goto cleanup;
default:
virReportSystemError(-err->error,
_("error creating %s type of interface"),
type);
- rc = -1;
+ goto cleanup;
}
break;
@@ -195,27 +193,21 @@ virNetDevMacVLanCreate(const char *ifname,
goto malformed_resp;
}
+ rc = 0;
cleanup:
nlmsg_free(nl_msg);
-
VIR_FREE(recvbuf);
-
return rc;
malformed_resp:
- nlmsg_free(nl_msg);
-
virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
_("malformed netlink response message"));
- VIR_FREE(recvbuf);
- return -1;
+ goto cleanup;
buffer_too_small:
- nlmsg_free(nl_msg);
-
virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
_("allocated netlink buffer is too small"));
- return -1;
+ goto cleanup;
}
/**
@@ -229,7 +221,7 @@ buffer_too_small:
*/
int virNetDevMacVLanDelete(const char *ifname)
{
- int rc = 0;
+ int rc = -1;
struct nlmsghdr *resp;
struct nlmsgerr *err;
struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
@@ -251,7 +243,6 @@ int virNetDevMacVLanDelete(const char *ifname)
goto buffer_too_small;
if (virNetlinkCommand(nl_msg, &recvbuf, &recvbuflen, 0) < 0) {
- rc = -1;
goto cleanup;
}
@@ -270,7 +261,7 @@ int virNetDevMacVLanDelete(const char *ifname)
virReportSystemError(-err->error,
_("error destroying %s interface"),
ifname);
- rc = -1;
+ goto cleanup;
}
break;
@@ -281,27 +272,21 @@ int virNetDevMacVLanDelete(const char *ifname)
goto malformed_resp;
}
+ rc = 0;
cleanup:
nlmsg_free(nl_msg);
-
VIR_FREE(recvbuf);
-
return rc;
malformed_resp:
- nlmsg_free(nl_msg);
-
virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
_("malformed netlink response message"));
- VIR_FREE(recvbuf);
- return -1;
+ goto cleanup;
buffer_too_small:
- nlmsg_free(nl_msg);
-
virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
_("allocated netlink buffer is too small"));
- return -1;
+ goto cleanup;
}
diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c
index bd356d8..5b562be 100644
--- a/src/util/virnetdevvportprofile.c
+++ b/src/util/virnetdevvportprofile.c
@@ -179,19 +179,20 @@ virNetDevVPortProfileGetStatus(struct nlattr **tb, int32_t vf,
uint16_t *status)
{
int rc = -1;
- const char *msg = NULL;
struct nlattr *tb_port[IFLA_PORT_MAX + 1] = { NULL, };
if (vf == PORT_SELF_VF && nltarget_kernel) {
if (tb[IFLA_PORT_SELF]) {
if (nla_parse_nested(tb_port, IFLA_PORT_MAX, tb[IFLA_PORT_SELF],
ifla_port_policy)) {
- msg = _("error parsing IFLA_PORT_SELF part");
- goto err_exit;
+ virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("error parsing IFLA_PORT_SELF part"));
+ goto cleanup;
}
} else {
- msg = _("IFLA_PORT_SELF is missing");
- goto err_exit;
+ virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("IFLA_PORT_SELF is missing"));
+ goto cleanup;
}
} else {
if (tb[IFLA_VF_PORTS]) {
@@ -202,14 +203,17 @@ virNetDevVPortProfileGetStatus(struct nlattr **tb, int32_t vf,
nla_for_each_nested(tb_vf_ports, tb[IFLA_VF_PORTS], rem) {
if (nla_type(tb_vf_ports) != IFLA_VF_PORT) {
- msg = _("error while iterating over IFLA_VF_PORTS part");
- goto err_exit;
+ virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("error while iterating over "
+ "IFLA_VF_PORTS part"));
+ goto cleanup;
}
if (nla_parse_nested(tb_port, IFLA_PORT_MAX, tb_vf_ports,
ifla_port_policy)) {
- msg = _("error parsing IFLA_VF_PORT part");
- goto err_exit;
+ virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("error parsing IFLA_VF_PORT part"));
+ goto cleanup;
}
if (instanceId &&
@@ -226,13 +230,15 @@ virNetDevVPortProfileGetStatus(struct nlattr **tb, int32_t vf,
}
if (!found) {
- msg = _("Could not find netlink response with "
- "expected parameters");
- goto err_exit;
+ virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not find netlink response with "
+ "expected parameters"));
+ goto cleanup;
}
} else {
- msg = _("IFLA_VF_PORTS is missing");
- goto err_exit;
+ virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("IFLA_VF_PORTS is missing"));
+ goto cleanup;
}
}
@@ -245,15 +251,12 @@ virNetDevVPortProfileGetStatus(struct nlattr **tb, int32_t vf,
*status = PORT_PROFILE_RESPONSE_INPROGRESS;
rc = 0;
} else {
- msg = _("no IFLA_PORT_RESPONSE found in netlink message");
- goto err_exit;
+ virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("no IFLA_PORT_RESPONSE found in netlink
message"));
+ goto cleanup;
}
}
-
-err_exit:
- if (msg)
- virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s", msg);
-
+cleanup:
return rc;
}
@@ -389,11 +392,11 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
if (!nltarget_kernel) {
pid = virNetDevVPortProfileGetLldpadPid();
if (pid == 0)
- goto err_exit;
+ goto cleanup;
}
if (virNetlinkCommand(nl_msg, &recvbuf, &recvbuflen, pid) < 0)
- goto err_exit;
+ goto cleanup;
if (recvbuflen < NLMSG_LENGTH(0) || recvbuf == NULL)
goto malformed_resp;
@@ -410,7 +413,7 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
virReportSystemError(-err->error,
_("error during virtual port configuration of ifindex %d"),
ifindex);
- goto err_exit;
+ goto cleanup;
}
break;
@@ -422,28 +425,20 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
}
rc = 0;
-
-err_exit:
+cleanup:
nlmsg_free(nl_msg);
-
VIR_FREE(recvbuf);
-
return rc;
malformed_resp:
- nlmsg_free(nl_msg);
-
virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
_("malformed netlink response message"));
- VIR_FREE(recvbuf);
- return rc;
+ goto cleanup;
buffer_too_small:
- nlmsg_free(nl_msg);
-
virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
_("allocated netlink buffer is too small"));
- return rc;
+ goto cleanup;
}
@@ -558,12 +553,12 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex,
rc = virNetDevLinkDump(NULL, ifindex, nltarget_kernel, tb,
&recvbuf, virNetDevVPortProfileGetLldpadPid);
if (rc < 0)
- goto err_exit;
+ goto cleanup;
rc = virNetDevVPortProfileGetStatus(tb, vf, instanceId, nltarget_kernel,
is8021Qbg, &status);
if (rc < 0)
- goto err_exit;
+ goto cleanup;
if (status == PORT_PROFILE_RESPONSE_SUCCESS ||
status == PORT_VDP_RESPONSE_SUCCESS) {
break;
@@ -589,7 +584,7 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex,
rc = -2;
}
-err_exit:
+cleanup:
VIR_FREE(recvbuf);
return rc;
@@ -634,7 +629,7 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname,
enum virNetDevVPortProfileLinkOp virtPortOp,
bool setlink_only)
{
- int rc = 0;
+ int rc = -1;
int op = PORT_REQUEST_ASSOCIATE;
struct ifla_port_vsi portVsi = {
.vsi_mgr_id = virtPort->u.virtPort8021Qbg.managerID,
@@ -650,10 +645,9 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname,
vf = PORT_SELF_VF;
- if (virNetDevVPortProfileGetPhysdevAndVlan(ifname, &physdev_ifindex,
physdev_ifname,
- &vlanid) < 0) {
- rc = -1;
- goto err_exit;
+ if (virNetDevVPortProfileGetPhysdevAndVlan(ifname, &physdev_ifindex,
+ physdev_ifname, &vlanid) < 0) {
+ goto cleanup;
}
if (vlanid < 0)
@@ -676,8 +670,7 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname,
default:
virNetDevError(VIR_ERR_INTERNAL_ERROR,
_("operation type %d not supported"), virtPortOp);
- rc = -1;
- goto err_exit;
+ goto cleanup;
}
rc = virNetDevVPortProfileOpCommon(physdev_ifname, physdev_ifindex,
@@ -691,9 +684,7 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname,
vf,
op,
setlink_only);
-
-err_exit:
-
+cleanup:
return rc;
}
--
1.7.7.6
3:24 a.m.
New subject: [libvirt] [PATCH 6/7] qemu: support persistent hotplug of <hostdev> devices
For some reason, although live hotplug of <hostdev> devices is
supported, persistent hotplug is not. This patch adds the proper
VIR_DOMAIN_DEVICE_HOSTDEV cases to the switches in
qemuDomainAttachDeviceConfig and qemuDomainDetachDeviceConfig.
---
src/qemu/qemu_driver.c | 32 ++++++++++++++++++++++++++++++++
1 files changed, 32 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index af89029..32b9386 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5365,6 +5365,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
{
virDomainDiskDefPtr disk;
virDomainNetDefPtr net;
+ virDomainHostdevDefPtr hostdev;
virDomainLeaseDefPtr lease;
switch (dev->type) {
@@ -5406,6 +5407,22 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
return -1;
break;
+ case VIR_DOMAIN_DEVICE_HOSTDEV:
+ hostdev = dev->data.hostdev;
+ if (virDomainHostdevFind(vmdef, hostdev, NULL) >= 0) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("device is already in the domain
configuration"));
+ return -1;
+ }
+ if (virDomainHostdevInsert(vmdef, hostdev)) {
+ virReportOOMError();
+ return -1;
+ }
+ dev->data.hostdev = NULL;
+ if (qemuDomainAssignAddresses(vmdef) < 0)
+ return -1;
+ break;
+
case VIR_DOMAIN_DEVICE_LEASE:
lease = dev->data.lease;
if (virDomainLeaseIndex(vmdef, lease) >= 0) {
@@ -5436,6 +5453,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
{
virDomainDiskDefPtr disk, det_disk;
virDomainNetDefPtr net, det_net;
+ virDomainHostdevDefPtr hostdev, det_hostdev;
virDomainLeaseDefPtr lease, det_lease;
switch (dev->type) {
@@ -5462,6 +5480,20 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
virDomainNetDefFree(det_net);
break;
+ case VIR_DOMAIN_DEVICE_HOSTDEV: {
+ int idx;
+
+ hostdev = dev->data.hostdev;
+ if ((idx = virDomainHostdevFind(vmdef, hostdev, &det_hostdev)) < 0) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("device not present in domain configuration"));
+ return -1;
+ }
+ virDomainHostdevRemove(vmdef, idx);
+ virDomainHostdevDefFree(det_hostdev);
+ break;
+ }
+
case VIR_DOMAIN_DEVICE_LEASE:
lease = dev->data.lease;
if (!(det_lease = virDomainLeaseRemove(vmdef, lease))) {
--
1.7.7.6
3:24 a.m.
New subject: [libvirt] [PATCH 7/7] qemu: eliminate memory leak in qemuDomainUpdateDeviceConfig
This function was freeing a virDomainNetDef with
VIR_FREE(). virDomainNetDef is a complex structure with many pointers
to other dynamically allocated data; to properly free it
virDomainNetDefFree() must be called instead, otherwise several
strings (and potentially other things) will be leaked.
---
src/qemu/qemu_driver.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 32b9386..112266c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5568,7 +5568,7 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
return -1;
}
- VIR_FREE(vmdef->nets[pos]);
+ virDomainNetDefFree(vmdef->nets[pos]);
vmdef->nets[pos] = net;
dev->data.net = NULL;
--
1.7.7.6
12:41 p.m.
On 03/08/2012 02:24 AM, Laine Stump wrote:
This series contains 7 patches that are mostly related by the face
that I noticed the problems they're solving while writing the
<interface type='hostdev'> code during the past couple weeks. After
that series was pushed, I sat down to fix everything I'd noticed
before I forgot about it. I'm sending it as a single series rather
than individual patches just to that they're easier to keep track of.
ACK series, once you fix the problems in 1/7.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:01 p.m.
On 03/08/2012 01:41 PM, Eric Blake wrote:
On 03/08/2012 02:24 AM, Laine Stump wrote:
> This series contains 7 patches that are mostly related by the face
> that I noticed the problems they're solving while writing the
> <interface type='hostdev'> code during the past couple weeks. After
> that series was pushed, I sat down to fix everything I'd noticed
> before I forgot about it. I'm sending it as a single series rather
> than individual patches just to that they're easier to keep track of.
ACK series, once you fix the problems in 1/7.
Okay, I pushed them all (after making the changes you suggested, and
running autobuild.sh successfully). Thanks!
4643
days inactive
4643
days old
10 comments
2 participants
participants (2)
-
Eric Blake -
Laine Stump