[libvirt] [PATCH 0/7] Various bugfixes related to device management
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.
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
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
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
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
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
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
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
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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!
participants (2)
-
Eric Blake -
Laine Stump