[libvirt] [PATCH 0/4] Resolve some recent Valgrind errors
Ran Valgrind today and found, investigated, and resolved the errors seen. John Ferlan (4): Resolve valgrind error in virNetDevVlanParse() Resolve valgrind error in virStorageBackendCreateQemuImgCmd() Resolve valgrind error in remoteConfigGetStringList() Resolve valgrind errors for nodedev cap parsing daemon/libvirtd-config.c | 6 ++++++ src/conf/netdev_vlan_conf.c | 24 +++++++++++++----------- src/conf/node_device_conf.c | 3 +++ src/storage/storage_backend.c | 4 +++- 4 files changed, 25 insertions(+), 12 deletions(-) -- 1.8.1.4
Commit '861d4056' introduced the following: TEST: networkxml2xmltest .................. 18 OK ==25504== 7 bytes in 1 blocks are definitely lost in loss record 5 of 23 ==25504== at 0x4A0887C: malloc (vg_replace_malloc.c:270) ==25504== by 0x37C1085D71: strdup (strdup.c:42) ==25504== by 0x4CB835F: virStrdup (virstring.c:546) ==25504== by 0x4CC5179: virXPathString (virxml.c:90) ==25504== by 0x4CC75C2: virNetDevVlanParse (netdev_vlan_conf.c:78) ==25504== by 0x4CF928A: virNetworkPortGroupParseXML (network_conf.c:1555) ==25504== by 0x4CFE385: virNetworkDefParseXML (network_conf.c:2049) ==25504== by 0x4D0113B: virNetworkDefParseNode (network_conf.c:2273) ==25504== by 0x4D01254: virNetworkDefParse (network_conf.c:2234) ==25504== by 0x401E80: testCompareXMLToXMLHelper (networkxml2xmltest.c:32) ==25504== by 0x402D4F: virtTestRun (testutils.c:158) ==25504== by 0x401CE9: mymain (networkxml2xmltest.c:110) ==25504== PASS: networkxml2xmltest Also changed the label from error to cleanup and adjusted code since it's all one exit path --- src/conf/netdev_vlan_conf.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/conf/netdev_vlan_conf.c b/src/conf/netdev_vlan_conf.c index 82ff9e8..880a7ce 100644 --- a/src/conf/netdev_vlan_conf.c +++ b/src/conf/netdev_vlan_conf.c @@ -45,18 +45,18 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr de nTags = virXPathNodeSet("./tag", ctxt, &tagNodes); if (nTags < 0) - goto error; + goto cleanup; if (nTags == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing tag id - each <vlan> must have " "at least one <tag id='n'/> subelement")); - goto error; + goto cleanup; } if (VIR_ALLOC_N(def->tag, nTags) < 0) { virReportOOMError(); - goto error; + goto cleanup; } def->nativeMode = 0; @@ -68,18 +68,18 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr de if (virXPathULong("string(./@id)", ctxt, &id) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing or invalid vlan tag id attribute")); - goto error; + goto cleanup; } if (id > 4095) { virReportError(VIR_ERR_XML_ERROR, _("vlan tag id %lu too large (maximum 4095)"), id); - goto error; + goto cleanup; } if ((nativeMode = virXPathString("string(./@nativeMode)", ctxt))) { if (def->nativeMode != 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("duplicate native vlan setting")); - goto error; + goto cleanup; } if ((def->nativeMode = virNativeVlanModeTypeFromString(nativeMode)) <= 0) { @@ -87,8 +87,9 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr de _("Invalid \"nativeMode='%s'\" " "in vlan <tag> element"), nativeMode); - goto error; + goto cleanup; } + VIR_FREE(nativeMode); def->nativeTag = id; } def->tag[ii] = id; @@ -110,29 +111,30 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr de virReportError(VIR_ERR_XML_ERROR, _("invalid \"trunk='%s'\" in <vlan> - trunk='yes' " "is required for more than one vlan tag"), trunk); - goto error; + goto cleanup; } if (def->nativeMode != 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid configuration in <vlan> - \"trunk='no'\" is " "not allowed with a native vlan id")); - goto error; + goto cleanup; } /* allow (but discard) "trunk='no' if there is a single tag */ if (STRCASENEQ(trunk, "no")) { virReportError(VIR_ERR_XML_ERROR, _("invalid \"trunk='%s'\" in <vlan> " "- must be yes or no"), trunk); - goto error; + goto cleanup; } } } ret = 0; -error: +cleanup: ctxt->node = save; VIR_FREE(tagNodes); VIR_FREE(trunk); + VIR_FREE(nativeMode); if (ret < 0) virNetDevVlanClear(def); return ret; -- 1.8.1.4
On 06/28/2013 03:25 PM, John Ferlan wrote:
Commit '861d4056' introduced the following:
TEST: networkxml2xmltest .................. 18 OK ==25504== 7 bytes in 1 blocks are definitely lost in loss record 5 of 23 ==25504== at 0x4A0887C: malloc (vg_replace_malloc.c:270) ==25504== by 0x37C1085D71: strdup (strdup.c:42) ==25504== by 0x4CB835F: virStrdup (virstring.c:546) ==25504== by 0x4CC5179: virXPathString (virxml.c:90) ==25504== by 0x4CC75C2: virNetDevVlanParse (netdev_vlan_conf.c:78) ==25504== by 0x4CF928A: virNetworkPortGroupParseXML (network_conf.c:1555) ==25504== by 0x4CFE385: virNetworkDefParseXML (network_conf.c:2049) ==25504== by 0x4D0113B: virNetworkDefParseNode (network_conf.c:2273) ==25504== by 0x4D01254: virNetworkDefParse (network_conf.c:2234) ==25504== by 0x401E80: testCompareXMLToXMLHelper (networkxml2xmltest.c:32) ==25504== by 0x402D4F: virtTestRun (testutils.c:158) ==25504== by 0x401CE9: mymain (networkxml2xmltest.c:110) ==25504== PASS: networkxml2xmltest
Also changed the label from error to cleanup and adjusted code since it's all one exit path ---
ACK.
Commit id '53d5967c' introduced the following: TEST: storagevolxml2argvtest .............. 14 OK ==25636== 358 (264 direct, 94 indirect) bytes in 1 blocks are definitely lost in loss record 67 of 75 ==25636== at 0x4A06B6F: calloc (vg_replace_malloc.c:593) ==25636== by 0x4C95791: virAlloc (viralloc.c:124) ==25636== by 0x4CA0BB4: virCommandNewArgs (vircommand.c:805) ==25636== by 0x4CA0C88: virCommandNew (vircommand.c:789) ==25636== by 0x408602: virStorageBackendCreateQemuImgCmd (storage_backend.c:849) ==25636== by 0x405427: testCompareXMLToArgvHelper (storagevolxml2argvtest.c:61) ==25636== by 0x4064DF: virtTestRun (testutils.c:158) ==25636== by 0x40516F: mymain (storagevolxml2argvtest.c:195) ==25636== by 0x406B1A: virtTestMain (testutils.c:722) ==25636== by 0x37C1021A04: (below main) (libc-start.c:225) ==25636== PASS: storagevolxml2argvtest --- src/storage/storage_backend.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index ae25c89..9a3bcf8 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -865,8 +865,10 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, do_encryption, preallocate, vol->target.format, vol->target.compat, - vol->target.features) < 0) + vol->target.features) < 0) { + virCommandFree(cmd); return NULL; + } if (opts) virCommandAddArgList(cmd, "-o", opts, NULL); VIR_FREE(opts); -- 1.8.1.4
On 06/28/2013 03:25 PM, John Ferlan wrote:
Commit id '53d5967c' introduced the following:
TEST: storagevolxml2argvtest .............. 14 OK ==25636== 358 (264 direct, 94 indirect) bytes in 1 blocks are definitely lost in loss record 67 of 75 ==25636== at 0x4A06B6F: calloc (vg_replace_malloc.c:593) ==25636== by 0x4C95791: virAlloc (viralloc.c:124) ==25636== by 0x4CA0BB4: virCommandNewArgs (vircommand.c:805) ==25636== by 0x4CA0C88: virCommandNew (vircommand.c:789) ==25636== by 0x408602: virStorageBackendCreateQemuImgCmd (storage_backend.c:849) ==25636== by 0x405427: testCompareXMLToArgvHelper (storagevolxml2argvtest.c:61) ==25636== by 0x4064DF: virtTestRun (testutils.c:158) ==25636== by 0x40516F: mymain (storagevolxml2argvtest.c:195) ==25636== by 0x406B1A: virtTestMain (testutils.c:722) ==25636== by 0x37C1021A04: (below main) (libc-start.c:225) ==25636== PASS: storagevolxml2argvtest --- src/storage/storage_backend.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index ae25c89..9a3bcf8 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -865,8 +865,10 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, do_encryption, preallocate, vol->target.format, vol->target.compat, - vol->target.features) < 0) + vol->target.features) < 0) { + virCommandFree(cmd); return NULL; + } if (opts) virCommandAddArgList(cmd, "-o", opts, NULL); VIR_FREE(opts);
ACK.
Commit id 'ed3bac71' introduced the following: TEST: libvirtdconftest ........................................ 40 OK ==25875== 690 (480 direct, 210 indirect) bytes in 30 blocks are definitely lost in loss record 18 of 24 ==25875== at 0x4A06B6F: calloc (vg_replace_malloc.c:593) ==25875== by 0x4C737DF: virAllocN (viralloc.c:152) ==25875== by 0x403BC8: remoteConfigGetStringList (libvirtd-config.c:74) ==25875== by 0x4042CF: daemonConfigLoadOptions (libvirtd-config.c:382) ==25875== by 0x4052F5: daemonConfigLoadData (libvirtd-config.c:479) ==25875== by 0x40222C: testCorrupt (libvirtdconftest.c:112) ==25875== by 0x40321F: virtTestRun (testutils.c:158) ==25875== by 0x401FEE: mymain (libvirtdconftest.c:228) ==25875== by 0x40385A: virtTestMain (testutils.c:722) ==25875== by 0x37C1021A04: (below main) (libc-start.c:225) ==25875== PASS: libvirtdconftest --- daemon/libvirtd-config.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c index 6f60256..017d470 100644 --- a/daemon/libvirtd-config.c +++ b/daemon/libvirtd-config.c @@ -316,6 +316,12 @@ daemonConfigFree(struct daemonConfig *data) VIR_FREE(data->listen_addr); VIR_FREE(data->tls_port); VIR_FREE(data->tcp_port); + tmp = data->access_drivers; + while (tmp && *tmp) { + VIR_FREE(*tmp); + tmp++; + } + VIR_FREE(data->access_drivers); VIR_FREE(data->unix_sock_ro_perms); VIR_FREE(data->unix_sock_rw_perms); -- 1.8.1.4
On 06/28/2013 03:25 PM, John Ferlan wrote:
Commit id 'ed3bac71' introduced the following:
TEST: libvirtdconftest ........................................ 40 OK ==25875== 690 (480 direct, 210 indirect) bytes in 30 blocks are definitely lost in loss record 18 of 24 ==25875== at 0x4A06B6F: calloc (vg_replace_malloc.c:593) ==25875== by 0x4C737DF: virAllocN (viralloc.c:152) ==25875== by 0x403BC8: remoteConfigGetStringList (libvirtd-config.c:74) ==25875== by 0x4042CF: daemonConfigLoadOptions (libvirtd-config.c:382) ==25875== by 0x4052F5: daemonConfigLoadData (libvirtd-config.c:479) ==25875== by 0x40222C: testCorrupt (libvirtdconftest.c:112) ==25875== by 0x40321F: virtTestRun (testutils.c:158) ==25875== by 0x401FEE: mymain (libvirtdconftest.c:228) ==25875== by 0x40385A: virtTestMain (testutils.c:722) ==25875== by 0x37C1021A04: (below main) (libc-start.c:225) ==25875== PASS: libvirtdconftest --- daemon/libvirtd-config.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c index 6f60256..017d470 100644 --- a/daemon/libvirtd-config.c +++ b/daemon/libvirtd-config.c @@ -316,6 +316,12 @@ daemonConfigFree(struct daemonConfig *data) VIR_FREE(data->listen_addr); VIR_FREE(data->tls_port); VIR_FREE(data->tcp_port); + tmp = data->access_drivers; + while (tmp && *tmp) { + VIR_FREE(*tmp); + tmp++; + } + VIR_FREE(data->access_drivers);
VIR_FREE(data->unix_sock_ro_perms); VIR_FREE(data->unix_sock_rw_perms);
ACK.
There were two errors, one as a direct result of commit id '8807b285' and the other from cut-n-paste TEST: nodedevxml2xmltest .............. 14 OK ==25735== 3 bytes in 1 blocks are definitely lost in loss record 1 of 24 ==25735== at 0x4A0887C: malloc (vg_replace_malloc.c:270) ==25735== by 0x344D2AF275: xmlStrndup (in /usr/lib64/libxml2.so.2.9.1) ==25735== by 0x4D0C767: virNodeDeviceDefParseNode (node_device_conf.c:997) ==25735== by 0x4D0D3D2: virNodeDeviceDefParse (node_device_conf.c:1337) ==25735== by 0x401CA4: testCompareXMLToXMLHelper (nodedevxml2xmltest.c:28) ==25735== by 0x402B2F: virtTestRun (testutils.c:158) ==25735== by 0x401B27: mymain (nodedevxml2xmltest.c:81) ==25735== by 0x40316A: virtTestMain (testutils.c:722) ==25735== by 0x37C1021A04: (below main) (libc-start.c:225) ==25735== ==25735== 16 bytes in 1 blocks are definitely lost in loss record 10 of 24 ==25735== at 0x4A08A6E: realloc (vg_replace_malloc.c:662) ==25735== by 0x4C7385E: virReallocN (viralloc.c:184) ==25735== by 0x4C73906: virExpandN (viralloc.c:214) ==25735== by 0x4C73B4A: virInsertElementsN (viralloc.c:324) ==25735== by 0x4D0C84C: virNodeDeviceDefParseNode (node_device_conf.c:1026) ==25735== by 0x4D0D3D2: virNodeDeviceDefParse (node_device_conf.c:1337) ==25735== by 0x401CA4: testCompareXMLToXMLHelper (nodedevxml2xmltest.c:28) ==25735== by 0x402B2F: virtTestRun (testutils.c:158) ==25735== by 0x401B27: mymain (nodedevxml2xmltest.c:81) ==25735== by 0x40316A: virtTestMain (testutils.c:722) ==25735== by 0x37C1021A04: (below main) (libc-start.c:225) ==25735== PASS: nodedevxml2xmltest The first error was resolved by adding a missing VIR_FREE(numberStr); in the new function virNodeDevCapPciDevIommuGroupParseXML(). The second error was a bit more opaque as the error was a result of copying the free methodolgy of the existing code in virNodeDevCapsDefFree(). The code would free each of the entries in the array, but not the memory for the array itself. Added the necessary VIR_FREE(data->pci_dev.iommuGroupDevices) and while at it added the missing VIR_FREE(data->pci_dev.virtual_functions) although there wasn't a test that tripped across it (thus it's been lurking since commit id 'a010165d'). --- src/conf/node_device_conf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 96742ef..087cebb 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1034,6 +1034,7 @@ virNodeDevCapPciDevIommuGroupParseXML(xmlXPathContextPtr ctxt, ret = 0; cleanup: ctxt->node = origNode; + VIR_FREE(numberStr); VIR_FREE(addrNodes); VIR_FREE(pciAddr); return ret; @@ -1466,9 +1467,11 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) for (i = 0; i < data->pci_dev.num_virtual_functions; i++) { VIR_FREE(data->pci_dev.virtual_functions[i]); } + VIR_FREE(data->pci_dev.virtual_functions); for (i = 0; i < data->pci_dev.nIommuGroupDevices; i++) { VIR_FREE(data->pci_dev.iommuGroupDevices[i]); } + VIR_FREE(data->pci_dev.iommuGroupDevices); break; case VIR_NODE_DEV_CAP_USB_DEV: VIR_FREE(data->usb_dev.product_name); -- 1.8.1.4
On 06/28/2013 03:25 PM, John Ferlan wrote:
There were two errors, one as a direct result of commit id '8807b285' and the other from cut-n-paste
TEST: nodedevxml2xmltest .............. 14 OK ==25735== 3 bytes in 1 blocks are definitely lost in loss record 1 of 24 ==25735== at 0x4A0887C: malloc (vg_replace_malloc.c:270) ==25735== by 0x344D2AF275: xmlStrndup (in /usr/lib64/libxml2.so.2.9.1) ==25735== by 0x4D0C767: virNodeDeviceDefParseNode (node_device_conf.c:997) ==25735== by 0x4D0D3D2: virNodeDeviceDefParse (node_device_conf.c:1337) ==25735== by 0x401CA4: testCompareXMLToXMLHelper (nodedevxml2xmltest.c:28) ==25735== by 0x402B2F: virtTestRun (testutils.c:158) ==25735== by 0x401B27: mymain (nodedevxml2xmltest.c:81) ==25735== by 0x40316A: virtTestMain (testutils.c:722) ==25735== by 0x37C1021A04: (below main) (libc-start.c:225) ==25735== ==25735== 16 bytes in 1 blocks are definitely lost in loss record 10 of 24 ==25735== at 0x4A08A6E: realloc (vg_replace_malloc.c:662) ==25735== by 0x4C7385E: virReallocN (viralloc.c:184) ==25735== by 0x4C73906: virExpandN (viralloc.c:214) ==25735== by 0x4C73B4A: virInsertElementsN (viralloc.c:324) ==25735== by 0x4D0C84C: virNodeDeviceDefParseNode (node_device_conf.c:1026) ==25735== by 0x4D0D3D2: virNodeDeviceDefParse (node_device_conf.c:1337) ==25735== by 0x401CA4: testCompareXMLToXMLHelper (nodedevxml2xmltest.c:28) ==25735== by 0x402B2F: virtTestRun (testutils.c:158) ==25735== by 0x401B27: mymain (nodedevxml2xmltest.c:81) ==25735== by 0x40316A: virtTestMain (testutils.c:722) ==25735== by 0x37C1021A04: (below main) (libc-start.c:225) ==25735== PASS: nodedevxml2xmltest
The first error was resolved by adding a missing VIR_FREE(numberStr); in the new function virNodeDevCapPciDevIommuGroupParseXML().
The second error was a bit more opaque as the error was a result of copying the free methodolgy of the existing code in virNodeDevCapsDefFree(). The code would free each of the entries in the array, but not the memory for the array itself. Added the necessary VIR_FREE(data->pci_dev.iommuGroupDevices) and while at it added the missing VIR_FREE(data->pci_dev.virtual_functions) although there wasn't a test that tripped across it
Yeah, I noticed when I added the iommuGroupDevices that neither virtual_functions nor physical_function are included the in nodedev parser, nor are they tested in the nodedevxml2xml test. I have that on my mental list of things to put in after this release. ACK to this patch too. I really should run valgrind more often (used to do it all the time, but I've fallen out of the habit...)
On 06/28/2013 03:25 PM, John Ferlan wrote:
Ran Valgrind today and found, investigated, and resolved the errors seen.
John Ferlan (4): Resolve valgrind error in virNetDevVlanParse() Resolve valgrind error in virStorageBackendCreateQemuImgCmd() Resolve valgrind error in remoteConfigGetStringList() Resolve valgrind errors for nodedev cap parsing
daemon/libvirtd-config.c | 6 ++++++ src/conf/netdev_vlan_conf.c | 24 +++++++++++++----------- src/conf/node_device_conf.c | 3 +++ src/storage/storage_backend.c | 4 +++- 4 files changed, 25 insertions(+), 12 deletions(-)
Thanks for the reviews - these were pushed. John
participants (2)
-
John Ferlan -
Laine Stump