[libvirt] [PATCH] 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) While at it, fix switch statements to be a bit more vocal if we forget some cases when adding new devices. * src/conf/node_device_conf.c (virNodeDeviceDefFormat) (virNodeDevCapsDefParseXML): Drop default case. (virNodeDevCapsDefFree): Likewise, and clear pci_express under pci case. Signed-off-by: Eric Blake <eblake@redhat.com> --- I could be persuaded to split this into two patches. src/conf/node_device_conf.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 910a371..4b34fec 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); @@ -1665,6 +1667,7 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) VIR_FREE(data->pci_dev.iommuGroupDevices[i]); } VIR_FREE(data->pci_dev.iommuGroupDevices); + VIR_FREE(data->pci_dev.pci_express); break; case VIR_NODE_DEV_CAP_USB_DEV: VIR_FREE(data->usb_dev.product_name); @@ -1703,7 +1706,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
On Tue, Jul 22, 2014 at 10:42:29PM -0600, Eric Blake wrote:
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)
While at it, fix switch statements to be a bit more vocal if we forget some cases when adding new devices.
* src/conf/node_device_conf.c (virNodeDeviceDefFormat) (virNodeDevCapsDefParseXML): Drop default case. (virNodeDevCapsDefFree): Likewise, and clear pci_express under pci case.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
I could be persuaded to split this into two patches.
That would be nice, now that you've mentioned it :)
src/conf/node_device_conf.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 910a371..4b34fec 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c [...] @@ -1665,6 +1667,7 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) VIR_FREE(data->pci_dev.iommuGroupDevices[i]); } VIR_FREE(data->pci_dev.iommuGroupDevices); + VIR_FREE(data->pci_dev.pci_express);
There should be data->pci_dev.pci_express->link_{sta,cap} free()'d too. ACK with that changed. Martin
On 07/22/2014 11:20 PM, Martin Kletzander wrote:
On Tue, Jul 22, 2014 at 10:42:29PM -0600, Eric Blake wrote:
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)
While at it, fix switch statements to be a bit more vocal if we forget some cases when adding new devices.
* src/conf/node_device_conf.c (virNodeDeviceDefFormat) (virNodeDevCapsDefParseXML): Drop default case. (virNodeDevCapsDefFree): Likewise, and clear pci_express under pci case.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
I could be persuaded to split this into two patches.
That would be nice, now that you've mentioned it :)
Will do, in v2.
VIR_FREE(data->pci_dev.iommuGroupDevices); + VIR_FREE(data->pci_dev.pci_express);
There should be data->pci_dev.pci_express->link_{sta,cap} free()'d too.
ACK with that changed.
Then node_device_udev.c has the same bug in udevProcessPCI. All the more reason for me to respin.
Martin
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Martin Kletzander