[libvirt] [PATCH v2 0/3] fix pciexpress memory leak
diff in v2: split out trivial fixes, refactor pcie code to virpci.h, add proper free function to cover all leaks Eric Blake (3): nodedev: let compiler help us on switches nodedev: move pci express types to virpci.h nodedev: fix pci express memory leak src/conf/node_device_conf.c | 13 ++++++------- src/conf/node_device_conf.h | 29 +---------------------------- src/libvirt_private.syms | 1 + src/node_device/node_device_udev.c | 2 +- src/util/virpci.c | 15 +++++++++++++++ src/util/virpci.h | 33 ++++++++++++++++++++++++++++++++- 6 files changed, 56 insertions(+), 37 deletions(-) -- 1.9.3
The compiler can alert us to places where we need to expand switch statements because we add a new enum value, but only if we don't have a default case. * src/conf/node_device_conf.c (virNodeDeviceDefFormat) (virNodeDevCapsDefParseXML, virNodeDevCapsDefFree): Drop default case. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/node_device_conf.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 910a371..ac966d3 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -547,7 +547,6 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def) case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: case VIR_NODE_DEV_CAP_LAST: - default: break; } @@ -1405,7 +1404,10 @@ virNodeDevCapsDefParseXML(xmlXPathContextPtr ctxt, case VIR_NODE_DEV_CAP_STORAGE: ret = virNodeDevCapStorageParseXML(ctxt, def, node, &caps->data); break; - default: + case VIR_NODE_DEV_CAP_FC_HOST: + case VIR_NODE_DEV_CAP_VPORTS: + case VIR_NODE_DEV_CAP_SCSI_GENERIC: + case VIR_NODE_DEV_CAP_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown capability type '%d' for '%s'"), caps->type, def->name); @@ -1703,7 +1705,6 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: case VIR_NODE_DEV_CAP_LAST: - default: /* This case is here to shutup the compiler */ break; } -- 1.9.3
Finding virPCIE* code is more intuitive if located in virpci.h instead of node_device_conf.h. * src/conf/node_device_conf.h (virPCIELinkSpeed, virPCIELink) (virPCIEDeviceInfo): Move... * src/util/virpci.h: ...here. * src/conf/node_device_conf.c (virPCIELinkSpeed): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/node_device_conf.c | 3 --- src/conf/node_device_conf.h | 29 +---------------------------- src/util/virpci.c | 3 +++ src/util/virpci.h | 30 +++++++++++++++++++++++++++++- 4 files changed, 33 insertions(+), 32 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index ac966d3..b244a1f 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -58,9 +58,6 @@ VIR_ENUM_IMPL(virNodeDevNetCap, VIR_NODE_DEV_CAP_NET_LAST, "80203", "80211") -VIR_ENUM_IMPL(virPCIELinkSpeed, VIR_PCIE_LINK_SPEED_LAST, - "", "2.5", "5", "8") - static int virNodeDevCapsDefParseString(const char *xpath, xmlXPathContextPtr ctxt, diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index b5bfb7b..fd5d179 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -1,7 +1,7 @@ /* * node_device_conf.h: config handling for node devices * - * Copyright (C) 2009-2013 Red Hat, Inc. + * Copyright (C) 2009-2014 Red Hat, Inc. * Copyright (C) 2008 Virtual Iron Software, Inc. * Copyright (C) 2008 David F. Lively * @@ -81,33 +81,6 @@ typedef enum { VIR_NODE_DEV_CAP_FLAG_PCIE = (1 << 2), } virNodeDevPCICapFlags; -typedef enum { - VIR_PCIE_LINK_SPEED_NA = 0, - VIR_PCIE_LINK_SPEED_25, - VIR_PCIE_LINK_SPEED_5, - VIR_PCIE_LINK_SPEED_8, - VIR_PCIE_LINK_SPEED_LAST -} virPCIELinkSpeed; - -VIR_ENUM_DECL(virPCIELinkSpeed) - -typedef struct _virPCIELink virPCIELink; -typedef virPCIELink *virPCIELinkPtr; -struct _virPCIELink { - int port; - virPCIELinkSpeed speed; - unsigned int width; -}; - -typedef struct _virPCIEDeviceInfo virPCIEDeviceInfo; -typedef virPCIEDeviceInfo *virPCIEDeviceInfoPtr; -struct _virPCIEDeviceInfo { - /* Not all PCI Express devices has link. For example this 'Root Complex - * Integrated Endpoint' and 'Root Complex Event Collector' don't have it. */ - virPCIELink *link_cap; /* PCIe device link capabilities */ - virPCIELink *link_sta; /* Actually negotiated capabilities */ -}; - typedef struct _virNodeDevCapsDef virNodeDevCapsDef; typedef virNodeDevCapsDef *virNodeDevCapsDefPtr; struct _virNodeDevCapsDef { diff --git a/src/util/virpci.c b/src/util/virpci.c index 6f4f6af..b7400e9 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -52,6 +52,9 @@ VIR_LOG_INIT("util.pci"); #define PCI_ID_LEN 10 /* "XXXX XXXX" */ #define PCI_ADDR_LEN 13 /* "XXXX:XX:XX.X" */ +VIR_ENUM_IMPL(virPCIELinkSpeed, VIR_PCIE_LINK_SPEED_LAST, + "", "2.5", "5", "8") + struct _virPCIDevice { unsigned int domain; unsigned int bus; diff --git a/src/util/virpci.h b/src/util/virpci.h index d64c3e2..edec439 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -1,7 +1,7 @@ /* * virpci.h: helper APIs for managing host PCI devices * - * Copyright (C) 2009, 2011-2013 Red Hat, Inc. + * Copyright (C) 2009, 2011-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -26,6 +26,7 @@ # include "internal.h" # include "virobject.h" +# include "virutil.h" typedef struct _virPCIDevice virPCIDevice; typedef virPCIDevice *virPCIDevicePtr; @@ -41,6 +42,33 @@ struct _virPCIDeviceAddress { unsigned int function; }; +typedef enum { + VIR_PCIE_LINK_SPEED_NA = 0, + VIR_PCIE_LINK_SPEED_25, + VIR_PCIE_LINK_SPEED_5, + VIR_PCIE_LINK_SPEED_8, + VIR_PCIE_LINK_SPEED_LAST +} virPCIELinkSpeed; + +VIR_ENUM_DECL(virPCIELinkSpeed) + +typedef struct _virPCIELink virPCIELink; +typedef virPCIELink *virPCIELinkPtr; +struct _virPCIELink { + int port; + virPCIELinkSpeed speed; + unsigned int width; +}; + +typedef struct _virPCIEDeviceInfo virPCIEDeviceInfo; +typedef virPCIEDeviceInfo *virPCIEDeviceInfoPtr; +struct _virPCIEDeviceInfo { + /* Not all PCI Express devices has link. For example this 'Root Complex + * Integrated Endpoint' and 'Root Complex Event Collector' don't have it. */ + virPCIELink *link_cap; /* PCIe device link capabilities */ + virPCIELink *link_sta; /* Actually negotiated capabilities */ +}; + virPCIDevicePtr virPCIDeviceNew(unsigned int domain, unsigned int bus, unsigned int slot, -- 1.9.3
On Wed, Jul 23, 2014 at 09:06:07PM -0600, Eric Blake wrote:
Finding virPCIE* code is more intuitive if located in virpci.h instead of node_device_conf.h.
* src/conf/node_device_conf.h (virPCIELinkSpeed, virPCIELink) (virPCIEDeviceInfo): Move... * src/util/virpci.h: ...here. * src/conf/node_device_conf.c (virPCIELinkSpeed): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/node_device_conf.c | 3 --- src/conf/node_device_conf.h | 29 +---------------------------- src/util/virpci.c | 3 +++ src/util/virpci.h | 30 +++++++++++++++++++++++++++++- 4 files changed, 33 insertions(+), 32 deletions(-)
diff --git a/src/util/virpci.h b/src/util/virpci.h index d64c3e2..edec439 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h [...] +typedef struct _virPCIEDeviceInfo virPCIEDeviceInfo; +typedef virPCIEDeviceInfo *virPCIEDeviceInfoPtr; +struct _virPCIEDeviceInfo { + /* Not all PCI Express devices has link. For example this 'Root Complex
I know it's pre-existing, but reading "devices has link" makes the back of my head itch, shouldn't it be 'have'? Could you change it if I am right? Martin
On 07/24/2014 04:03 AM, Martin Kletzander wrote:
On Wed, Jul 23, 2014 at 09:06:07PM -0600, Eric Blake wrote:
Finding virPCIE* code is more intuitive if located in virpci.h instead of node_device_conf.h.
+struct _virPCIEDeviceInfo { + /* Not all PCI Express devices has link. For example this 'Root Complex
I know it's pre-existing, but reading "devices has link" makes the back of my head itch, shouldn't it be 'have'? Could you change it if I am right?
You are right; 'all ... devices' is plural, so it should pair with 'have'. Fixed, and series pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Leak introduced in commit 16ebf10f (v1.2.6), detected by valgrind: ==9816== 216 (96 direct, 120 indirect) bytes in 6 blocks are definitely lost in loss record 665 of 821 ==9816== at 0x4A081D4: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==9816== by 0x50836FB: virAlloc (viralloc.c:144) ==9816== by 0x1DBDBE27: udevProcessPCI (node_device_udev.c:546) ==9816== by 0x1DBDD79D: udevGetDeviceDetails (node_device_udev.c:1293) * src/util/virpci.h (virPCIEDeviceInfoFree): New prototype. * src/util/virpci.c (virPCIEDeviceInfoFree): New function. * src/conf/node_device_conf.c (virNodeDevCapsDefFree): Clear pci_express under pci case. (virNodeDevCapPCIDevParseXML): Avoid leak. * src/node_device/node_device_udev.c (udevProcessPCI): Likewise. * src/libvirt_private.syms (virpci.h): Export it. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/node_device_conf.c | 3 ++- src/libvirt_private.syms | 1 + src/node_device/node_device_udev.c | 2 +- src/util/virpci.c | 12 ++++++++++++ src/util/virpci.h | 3 +++ 5 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index b244a1f..78bc63f 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1291,7 +1291,7 @@ virNodeDevCapPCIDevParseXML(xmlXPathContextPtr ctxt, ret = 0; out: - VIR_FREE(pci_express); + virPCIEDeviceInfoFree(pci_express); ctxt->node = orignode; return ret; } @@ -1664,6 +1664,7 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) VIR_FREE(data->pci_dev.iommuGroupDevices[i]); } VIR_FREE(data->pci_dev.iommuGroupDevices); + virPCIEDeviceInfoFree(data->pci_dev.pci_express); break; case VIR_NODE_DEV_CAP_USB_DEV: VIR_FREE(data->usb_dev.product_name); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 51504d1..fe02f9c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1753,6 +1753,7 @@ virPCIDeviceSetUnbindFromStub; virPCIDeviceSetUsedBy; virPCIDeviceUnbind; virPCIDeviceWaitForCleanup; +virPCIEDeviceInfoFree; virPCIGetNetName; virPCIGetPhysicalFunction; virPCIGetVirtualFunctionIndex; diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 28d2953..0fe474d 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -570,7 +570,7 @@ static int udevProcessPCI(struct udev_device *device, out: virPCIDeviceFree(pciDev); - VIR_FREE(pci_express); + virPCIEDeviceInfoFree(pci_express); return ret; } diff --git a/src/util/virpci.c b/src/util/virpci.c index b7400e9..0098d6c 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2847,3 +2847,15 @@ virPCIDeviceGetLinkCapSta(virPCIDevicePtr dev, virPCIDeviceConfigClose(dev, fd); return ret; } + + +void +virPCIEDeviceInfoFree(virPCIEDeviceInfoPtr dev) +{ + if (!dev) + return; + + VIR_FREE(dev->link_cap); + VIR_FREE(dev->link_sta); + VIR_FREE(dev); +} diff --git a/src/util/virpci.h b/src/util/virpci.h index edec439..3da8eb3 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -212,4 +212,7 @@ int virPCIDeviceGetLinkCapSta(virPCIDevicePtr dev, unsigned int *cap_width, unsigned int *sta_speed, unsigned int *sta_width); + +void virPCIEDeviceInfoFree(virPCIEDeviceInfoPtr dev); + #endif /* __VIR_PCI_H__ */ -- 1.9.3
On Wed, Jul 23, 2014 at 09:06:05PM -0600, Eric Blake wrote:
diff in v2: split out trivial fixes, refactor pcie code to virpci.h, add proper free function to cover all leaks
Eric Blake (3): nodedev: let compiler help us on switches nodedev: move pci express types to virpci.h nodedev: fix pci express memory leak
src/conf/node_device_conf.c | 13 ++++++------- src/conf/node_device_conf.h | 29 +---------------------------- src/libvirt_private.syms | 1 + src/node_device/node_device_udev.c | 2 +- src/util/virpci.c | 15 +++++++++++++++ src/util/virpci.h | 33 ++++++++++++++++++++++++++++++++- 6 files changed, 56 insertions(+), 37 deletions(-)
ACK series with a tiny, tiny wish in 2/3. Martin
participants (2)
-
Eric Blake -
Martin Kletzander