5:06 a.m.
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
5:06 a.m.
New subject: [libvirt] [PATCH v2 1/3] nodedev: let compiler help us on switches
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(a)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
5:06 a.m.
New subject: [libvirt] [PATCH v2 2/3] nodedev: move pci express types to virpci.h
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(a)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
12:03 p.m.
New subject: [libvirt] [PATCH v2 2/3] nodedev: move pci express types to virpci.h
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(a)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
10:11 p.m.
New subject: [libvirt] [PATCH v2 2/3] nodedev: move pci express types to virpci.h
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
5:06 a.m.
New subject: [libvirt] [PATCH v2 3/3] nodedev: fix pci express memory leak
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(a)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
12:04 p.m.
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
3703
days inactive
3707
days old
6 comments
2 participants
participants (2)
-
Eric Blake -
Martin Kletzander