[libvirt] [PATCH 00/13] Resolve resource leaks found by Valgrind
This set of patches resolves issues seen in the Valgrind tests. The update to the .valgrind.supp file is from my environment and is very basic. Some paths in the previous version don't exist anymore. Based on recent (and continuing) commandtest failures, adjustments still may be necessary depending on environment. To assist in that I also updated the hacking document to describe a bit more about the Valgrind tests with respect to success, failure, and false positives. In particular, how to hopefully recognize each and decide whether what's seen is a false positive. Prior to the recent vircommand and commandtest changes, the commandtest was passing, but now it fails. The failure may be related to: https://www.redhat.com/archives/libvir-list/2013-February/msg00269.html I spent some time looking at the output, but could not figure out a root cause. I'll keep trying, but figured at the very least the other changes are still valid. John Ferlan (13): virnettlscontexttest: Resolve memory leak found by Valgrind qemuxml2argvtest: Resolve resource leaks found by Valgrind netdev_vlan_conf: Resolve memory leak found by Valgrind. vport_profile_conf: Resolve memory leak found by Valgrind domain_conf: Resolve resource leaks found by Valgrind qemu_command: Resolve resource leaks found by Valgrind qemumonitorjsontest: Resolve resource leaks found by Valgrind qemumonitortestutils: Resolve resource leaks found by Valgrind virnetttlcontext: Resolve issues found by Valgrind valgrind: Adjust the suppression file hacking: Add more information about Valgrind HACKING: Sync with docs/hacking.html.in cfg.mk: Add hacking.in.html to sc_prohibit_raw_allocation HACKING | 93 ++++++++++- cfg.mk | 2 +- docs/hacking.html.in | 96 +++++++++++- src/conf/domain_conf.c | 12 +- src/conf/netdev_vlan_conf.c | 3 +- src/conf/netdev_vport_profile_conf.c | 1 + src/qemu/qemu_command.c | 68 ++++---- src/rpc/virnettlscontext.c | 4 +- tests/.valgrind.supp | 292 +++++++---------------------------- tests/qemumonitorjsontest.c | 23 ++- tests/qemumonitortestutils.c | 7 +- tests/qemuxml2argvtest.c | 6 +- tests/virnettlscontexttest.c | 8 +- 13 files changed, 319 insertions(+), 296 deletions(-) -- 1.7.11.7
testTLSDerEncode() will allocate memory for der.data, it wasn't VIR_FREE()'d. also don't initialized der to use static buffer. --- tests/virnettlscontexttest.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/virnettlscontexttest.c b/tests/virnettlscontexttest.c index e0a2788..3df8a70 100644 --- a/tests/virnettlscontexttest.c +++ b/tests/virnettlscontexttest.c @@ -160,7 +160,7 @@ testTLSGenerateCert(struct testTLSCertReq *req) static char buffer[1024*1024]; size_t size = sizeof(buffer); char serial[5] = { 1, 2, 3, 4, 0 }; - gnutls_datum_t der = { (unsigned char *)buffer, size }; + gnutls_datum_t der; time_t start = time(NULL) + (60*60*req->start_offset); time_t expire = time(NULL) + (60*60*(req->expire_offset ? req->expire_offset : 24)); @@ -294,9 +294,11 @@ testTLSGenerateCert(struct testTLSCertReq *req) der.size, req->basicConstraintsCritical)) < 0) { VIR_WARN("Failed to set certificate basic constraints %s", gnutls_strerror(err)); + VIR_FREE(der.data); abort(); } asn1_delete_structure(&ext); + VIR_FREE(der.data); } /* @@ -320,9 +322,11 @@ testTLSGenerateCert(struct testTLSCertReq *req) der.size, req->keyUsageCritical)) < 0) { VIR_WARN("Failed to set certificate key usage %s", gnutls_strerror(err)); + VIR_FREE(der.data); abort(); } asn1_delete_structure(&ext); + VIR_FREE(der.data); } /* @@ -350,9 +354,11 @@ testTLSGenerateCert(struct testTLSCertReq *req) der.size, req->keyPurposeCritical)) < 0) { VIR_WARN("Failed to set certificate key purpose %s", gnutls_strerror(err)); + VIR_FREE(der.data); abort(); } asn1_delete_structure(&ext); + VIR_FREE(der.data); } /* -- 1.7.11.7
On 2013年02月07日 05:35, John Ferlan wrote:
testTLSDerEncode() will allocate memory for der.data, it wasn't VIR_FREE()'d. also don't initialized der to use static buffer. --- tests/virnettlscontexttest.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tests/virnettlscontexttest.c b/tests/virnettlscontexttest.c index e0a2788..3df8a70 100644 --- a/tests/virnettlscontexttest.c +++ b/tests/virnettlscontexttest.c @@ -160,7 +160,7 @@ testTLSGenerateCert(struct testTLSCertReq *req) static char buffer[1024*1024]; size_t size = sizeof(buffer); char serial[5] = { 1, 2, 3, 4, 0 }; - gnutls_datum_t der = { (unsigned char *)buffer, size }; + gnutls_datum_t der; time_t start = time(NULL) + (60*60*req->start_offset); time_t expire = time(NULL) + (60*60*(req->expire_offset ? req->expire_offset : 24)); @@ -294,9 +294,11 @@ testTLSGenerateCert(struct testTLSCertReq *req) der.size, req->basicConstraintsCritical))< 0) { VIR_WARN("Failed to set certificate basic constraints %s", gnutls_strerror(err)); + VIR_FREE(der.data); abort(); } asn1_delete_structure(&ext); + VIR_FREE(der.data); }
/* @@ -320,9 +322,11 @@ testTLSGenerateCert(struct testTLSCertReq *req) der.size, req->keyUsageCritical))< 0) { VIR_WARN("Failed to set certificate key usage %s", gnutls_strerror(err)); + VIR_FREE(der.data); abort(); } asn1_delete_structure(&ext); + VIR_FREE(der.data); }
/* @@ -350,9 +354,11 @@ testTLSGenerateCert(struct testTLSCertReq *req) der.size, req->keyPurposeCritical))< 0) { VIR_WARN("Failed to set certificate key purpose %s", gnutls_strerror(err)); + VIR_FREE(der.data); abort(); } asn1_delete_structure(&ext); + VIR_FREE(der.data); }
/*
ACK
Valgrind deterimined that fakeSecretGetValue() was using the secret value without checking validity. Returning NULL causes the caller to emit a message and results in failure. Additionally commit 'b090aa7d' changes leaked vncSASLdir and vncTLSx509certdir --- tests/qemuxml2argvtest.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 4e90b26..938dc32 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -31,6 +31,9 @@ fakeSecretGetValue(virSecretPtr obj ATTRIBUTE_UNUSED, unsigned int internalFlags ATTRIBUTE_UNUSED) { char *secret = strdup("AQCVn5hO6HzFAhAAq0NCv8jtJcIcE+HOBlMQ1A"); + if (!secret) { + return NULL; + } *value_size = strlen(secret); return (unsigned char *) secret; } @@ -559,7 +562,8 @@ mymain(void) driver.config->vncTLSx509verify = 1; DO_TEST("graphics-vnc-tls", QEMU_CAPS_VNC); driver.config->vncSASL = driver.config->vncTLSx509verify = driver.config->vncTLS = 0; - driver.config->vncSASLdir = driver.config->vncTLSx509certdir = NULL; + VIR_FREE(driver.config->vncSASLdir); + VIR_FREE(driver.config->vncTLSx509certdir); DO_TEST("graphics-sdl", NONE); DO_TEST("graphics-sdl-fullscreen", NONE); -- 1.7.11.7
On 2013年02月07日 05:35, John Ferlan wrote:
Valgrind deterimined that fakeSecretGetValue() was using the secret value without checking validity. Returning NULL causes the caller to emit a message and results in failure.
Additionally commit 'b090aa7d' changes leaked vncSASLdir and vncTLSx509certdir --- tests/qemuxml2argvtest.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 4e90b26..938dc32 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -31,6 +31,9 @@ fakeSecretGetValue(virSecretPtr obj ATTRIBUTE_UNUSED, unsigned int internalFlags ATTRIBUTE_UNUSED) { char *secret = strdup("AQCVn5hO6HzFAhAAq0NCv8jtJcIcE+HOBlMQ1A"); + if (!secret) { + return NULL; + } *value_size = strlen(secret); return (unsigned char *) secret; } @@ -559,7 +562,8 @@ mymain(void) driver.config->vncTLSx509verify = 1; DO_TEST("graphics-vnc-tls", QEMU_CAPS_VNC); driver.config->vncSASL = driver.config->vncTLSx509verify = driver.config->vncTLS = 0; - driver.config->vncSASLdir = driver.config->vncTLSx509certdir = NULL; + VIR_FREE(driver.config->vncSASLdir); + VIR_FREE(driver.config->vncTLSx509certdir);
DO_TEST("graphics-sdl", NONE); DO_TEST("graphics-sdl-fullscreen", NONE);
ACK
The 'trunk' is filled in with virXPathString() value, but was never VIR_FREE()'d. --- src/conf/netdev_vlan_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/netdev_vlan_conf.c b/src/conf/netdev_vlan_conf.c index 9207184..13ba8c6 100644 --- a/src/conf/netdev_vlan_conf.c +++ b/src/conf/netdev_vlan_conf.c @@ -32,7 +32,7 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr de { int ret = -1; xmlNodePtr save = ctxt->node; - const char *trunk; + const char *trunk = NULL; xmlNodePtr *tagNodes = NULL; int nTags, ii; @@ -103,6 +103,7 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr de error: ctxt->node = save; VIR_FREE(tagNodes); + VIR_FREE(trunk); if (ret < 0) virNetDevVlanClear(def); return ret; -- 1.7.11.7
On 2013年02月07日 05:35, John Ferlan wrote:
The 'trunk' is filled in with virXPathString() value, but was never VIR_FREE()'d. --- src/conf/netdev_vlan_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/conf/netdev_vlan_conf.c b/src/conf/netdev_vlan_conf.c index 9207184..13ba8c6 100644 --- a/src/conf/netdev_vlan_conf.c +++ b/src/conf/netdev_vlan_conf.c @@ -32,7 +32,7 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr de { int ret = -1; xmlNodePtr save = ctxt->node; - const char *trunk; + const char *trunk = NULL; xmlNodePtr *tagNodes = NULL; int nTags, ii;
@@ -103,6 +103,7 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr de error: ctxt->node = save; VIR_FREE(tagNodes); + VIR_FREE(trunk); if (ret< 0) virNetDevVlanClear(def); return ret;
ACK
The 'virtPortInterfaceID' was not VIR_FREE()'d --- src/conf/netdev_vport_profile_conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/netdev_vport_profile_conf.c b/src/conf/netdev_vport_profile_conf.c index 701e17e..47f51f9 100644 --- a/src/conf/netdev_vport_profile_conf.c +++ b/src/conf/netdev_vport_profile_conf.c @@ -198,6 +198,7 @@ cleanup: VIR_FREE(virtPortInstanceID); VIR_FREE(virtPortProfileID); VIR_FREE(virtPortType); + VIR_FREE(virtPortInterfaceID); return virtPort; -- 1.7.11.7
On 2013年02月07日 05:35, John Ferlan wrote:
The 'virtPortInterfaceID' was not VIR_FREE()'d --- src/conf/netdev_vport_profile_conf.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/conf/netdev_vport_profile_conf.c b/src/conf/netdev_vport_profile_conf.c index 701e17e..47f51f9 100644 --- a/src/conf/netdev_vport_profile_conf.c +++ b/src/conf/netdev_vport_profile_conf.c @@ -198,6 +198,7 @@ cleanup: VIR_FREE(virtPortInstanceID); VIR_FREE(virtPortProfileID); VIR_FREE(virtPortType); + VIR_FREE(virtPortInterfaceID);
return virtPort;
ACK
Fix various resource leaks discovered while parsing through Valgrind output --- src/conf/domain_conf.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 27f5b5e..62a604f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7604,6 +7604,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node, VIR_FREE(ram); VIR_FREE(vram); VIR_FREE(heads); + VIR_FREE(primary); return def; @@ -9587,6 +9588,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, } if ((value = virDomainFeatureStateTypeFromString(tmp)) < 0) { + VIR_FREE(tmp); virReportError(VIR_ERR_XML_ERROR, _("invalid value of state argument " "for HyperV Enlightenment feature '%s'"), @@ -9594,6 +9596,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, goto error; } + VIR_FREE(tmp); def->hyperv_features[feature] = value; break; @@ -9922,6 +9925,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) { if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { if (usb_other || usb_none) { + virDomainControllerDefFree(controller); virReportError(VIR_ERR_XML_DETAIL, "%s", _("Can't add another USB controller: " "USB is disabled for this domain")); @@ -9930,6 +9934,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, usb_none = true; } else { if (usb_none) { + virDomainControllerDefFree(controller); virReportError(VIR_ERR_XML_DETAIL, "%s", _("Can't add another USB controller: " "USB is disabled for this domain")); @@ -10227,6 +10232,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, /* Check if USB bus is required */ if (input->bus == VIR_DOMAIN_INPUT_BUS_USB && usb_none) { + virDomainInputDefFree(input); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Can't add USB input device. " "USB bus is disabled")); @@ -10324,6 +10330,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (video->primary) { if (primaryVideo) { + virDomainVideoDefFree(video); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Only one primary video device is supported")); goto error; @@ -10335,8 +10342,10 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (VIR_INSERT_ELEMENT_INPLACE(def->videos, ii, def->nvideos, - video) < 0) + video) < 0) { + virDomainVideoDefFree(video); goto error; + } } VIR_FREE(nodes); @@ -10452,6 +10461,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, goto error; if (hub->type == VIR_DOMAIN_HUB_TYPE_USB && usb_none) { + virDomainHubDefFree(hub); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Can't add USB hub: " "USB is disabled for this domain")); -- 1.7.11.7
On 2013年02月07日 05:35, John Ferlan wrote:
Fix various resource leaks discovered while parsing through Valgrind output --- src/conf/domain_conf.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 27f5b5e..62a604f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7604,6 +7604,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node, VIR_FREE(ram); VIR_FREE(vram); VIR_FREE(heads); + VIR_FREE(primary);
return def;
@@ -9587,6 +9588,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, }
if ((value = virDomainFeatureStateTypeFromString(tmp))< 0) { + VIR_FREE(tmp); virReportError(VIR_ERR_XML_ERROR, _("invalid value of state argument " "for HyperV Enlightenment feature '%s'"), @@ -9594,6 +9596,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, goto error; }
+ VIR_FREE(tmp); def->hyperv_features[feature] = value; break;
Good to fix the leak in the similar hunk as well: tmp = virXPathString("string(./features/apic/@eoi)", ctxt); if (tmp) { int eoi; if ((eoi = virDomainFeatureStateTypeFromString(tmp)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown value for attribute eoi: %s"), tmp); goto error; } def->apic_eoi = eoi; VIR_FREE(tmp); }
@@ -9922,6 +9925,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) { if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { if (usb_other || usb_none) { + virDomainControllerDefFree(controller); virReportError(VIR_ERR_XML_DETAIL, "%s", _("Can't add another USB controller: " "USB is disabled for this domain")); @@ -9930,6 +9934,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, usb_none = true; } else { if (usb_none) { + virDomainControllerDefFree(controller); virReportError(VIR_ERR_XML_DETAIL, "%s", _("Can't add another USB controller: " "USB is disabled for this domain")); @@ -10227,6 +10232,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
/* Check if USB bus is required */ if (input->bus == VIR_DOMAIN_INPUT_BUS_USB&& usb_none) { + virDomainInputDefFree(input); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Can't add USB input device. " "USB bus is disabled")); @@ -10324,6 +10330,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
if (video->primary) { if (primaryVideo) { + virDomainVideoDefFree(video); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Only one primary video device is supported")); goto error; @@ -10335,8 +10342,10 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (VIR_INSERT_ELEMENT_INPLACE(def->videos, ii, def->nvideos, - video)< 0) + video)< 0) { + virDomainVideoDefFree(video); goto error; + } } VIR_FREE(nodes);
@@ -10452,6 +10461,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, goto error;
if (hub->type == VIR_DOMAIN_HUB_TYPE_USB&& usb_none) { + virDomainHubDefFree(hub); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Can't add USB hub: " "USB is disabled for this domain"));
ACK with fixing the similar hunk.
On 02/06/2013 09:06 PM, Osier Yang wrote:
On 2013年02月07日 05:35, John Ferlan wrote:
Fix various resource leaks discovered while parsing through Valgrind output --- src/conf/domain_conf.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 27f5b5e..62a604f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7604,6 +7604,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node, VIR_FREE(ram); VIR_FREE(vram); VIR_FREE(heads); + VIR_FREE(primary);
return def;
@@ -9587,6 +9588,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, }
if ((value = virDomainFeatureStateTypeFromString(tmp))< 0) { + VIR_FREE(tmp); virReportError(VIR_ERR_XML_ERROR, _("invalid value of state argument " "for HyperV Enlightenment feature '%s'"), @@ -9594,6 +9596,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, goto error; }
+ VIR_FREE(tmp); def->hyperv_features[feature] = value; break;
Good to fix the leak in the similar hunk as well:
tmp = virXPathString("string(./features/apic/@eoi)", ctxt); if (tmp) { int eoi; if ((eoi = virDomainFeatureStateTypeFromString(tmp)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown value for attribute eoi: %s"), tmp); goto error; } def->apic_eoi = eoi; VIR_FREE(tmp); }
@@ -9922,6 +9925,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) { if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { if (usb_other || usb_none) { + virDomainControllerDefFree(controller); virReportError(VIR_ERR_XML_DETAIL, "%s", _("Can't add another USB controller: " "USB is disabled for this domain")); @@ -9930,6 +9934,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, usb_none = true; } else { if (usb_none) { + virDomainControllerDefFree(controller); virReportError(VIR_ERR_XML_DETAIL, "%s", _("Can't add another USB controller: " "USB is disabled for this domain")); @@ -10227,6 +10232,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
/* Check if USB bus is required */ if (input->bus == VIR_DOMAIN_INPUT_BUS_USB&& usb_none) { + virDomainInputDefFree(input); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Can't add USB input device. " "USB bus is disabled")); @@ -10324,6 +10330,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
if (video->primary) { if (primaryVideo) { + virDomainVideoDefFree(video); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Only one primary video device is supported")); goto error; @@ -10335,8 +10342,10 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (VIR_INSERT_ELEMENT_INPLACE(def->videos, ii, def->nvideos, - video)< 0) + video)< 0) { + virDomainVideoDefFree(video); goto error; + } } VIR_FREE(nodes);
@@ -10452,6 +10461,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, goto error;
if (hub->type == VIR_DOMAIN_HUB_TYPE_USB&& usb_none) { + virDomainHubDefFree(hub); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Can't add USB hub: " "USB is disabled for this domain"));
ACK with fixing the similar hunk.
In retrospect, the virDomainDefParseXML() will VIR_FREE(tmp) in the error: path, so rather than add within the error path as I did at line 9592 and the error path for "./features/apic/@eoi", I removed that one line. I probably added that one as a result of adding the one in the non error path and hyper-focusing on the code on the screen around the change. John
The qemuParseGlusterString() replaced dst->src without a VIR_FREE() of what was in there before. The qemuBuildCommandLine() did not properly free the boot_buf depending on various usages. The qemuParseCommandLineDisk() had numerous paths that didn't clean up the virDomainDiskDefPtr def properly. Adjust the logic to go through an error: label before cleanup in order to free the resource. --- src/qemu/qemu_command.c | 68 ++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 37 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index fbc5481..8a92d04 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2147,6 +2147,7 @@ qemuParseGlusterString(virDomainDiskDefPtr def) } } volimg = uri->path + 1; /* skip the prefix slash */ + VIR_FREE(def->src); def->src = strdup(volimg); if (!def->src) goto no_memory; @@ -5629,6 +5630,7 @@ qemuBuildCommandLine(virConnectPtr conn, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("reboot timeout is not supported " "by this QEMU binary")); + virBufferFreeAndReset(&boot_buf); goto error; } @@ -5645,10 +5647,13 @@ qemuBuildCommandLine(virConnectPtr conn, if (boot_nparams < 2 || emitBootindex) { virCommandAddArgBuffer(cmd, &boot_buf); + virBufferFreeAndReset(&boot_buf); } else { + char *str = virBufferContentAndReset(&boot_buf); virCommandAddArgFormat(cmd, "order=%s", - virBufferContentAndReset(&boot_buf)); + str); + VIR_FREE(str); } } @@ -7425,24 +7430,23 @@ qemuParseCommandLineDisk(virCapsPtr caps, virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse nbd filename '%s'"), def->src); - def = NULL; - goto cleanup; + goto error; } *port++ = '\0'; if (VIR_ALLOC(def->hosts) < 0) { virReportOOMError(); - goto cleanup; + goto error; } def->nhosts = 1; def->hosts->name = strdup(host); if (!def->hosts->name) { virReportOOMError(); - goto cleanup; + goto error; } def->hosts->port = strdup(port); if (!def->hosts->port) { virReportOOMError(); - goto cleanup; + goto error; } def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; def->hosts->socket = NULL; @@ -7457,7 +7461,7 @@ qemuParseCommandLineDisk(virCapsPtr caps, def->src = strdup(p + strlen("rbd:")); if (!def->src) { virReportOOMError(); - goto cleanup; + goto error; } /* old-style CEPH_ARGS env variable is parsed later */ if (!old_style_ceph_args && qemuParseRBDString(def) < 0) @@ -7469,7 +7473,7 @@ qemuParseCommandLineDisk(virCapsPtr caps, def->protocol = VIR_DOMAIN_DISK_PROTOCOL_GLUSTER; if (qemuParseGlusterString(def) < 0) - goto cleanup; + goto error; } else if (STRPREFIX(def->src, "sheepdog:")) { char *p = def->src; char *port, *vdi; @@ -7479,7 +7483,7 @@ qemuParseCommandLineDisk(virCapsPtr caps, def->src = strdup(p + strlen("sheepdog:")); if (!def->src) { virReportOOMError(); - goto cleanup; + goto error; } /* def->src must be [vdiname] or [host]:[port]:[vdiname] */ @@ -7488,29 +7492,28 @@ qemuParseCommandLineDisk(virCapsPtr caps, *port++ = '\0'; vdi = strchr(port, ':'); if (!vdi) { - def = NULL; virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse sheepdog filename '%s'"), p); - goto cleanup; + goto error; } *vdi++ = '\0'; if (VIR_ALLOC(def->hosts) < 0) { virReportOOMError(); - goto cleanup; + goto error; } def->nhosts = 1; def->hosts->name = def->src; def->hosts->port = strdup(port); if (!def->hosts->port) { virReportOOMError(); - goto cleanup; + goto error; } def->hosts->transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; def->hosts->socket = NULL; def->src = strdup(vdi); if (!def->src) { virReportOOMError(); - goto cleanup; + goto error; } } @@ -7538,13 +7541,10 @@ qemuParseCommandLineDisk(virCapsPtr caps, } else if (STREQ(keywords[i], "format")) { def->driverName = strdup("qemu"); if (!def->driverName) { - virDomainDiskDefFree(def); - def = NULL; virReportOOMError(); - goto cleanup; + goto error; } def->format = virStorageFileFormatTypeFromString(values[i]); - values[i] = NULL; } else if (STREQ(keywords[i], "cache")) { if (STREQ(values[i], "off") || STREQ(values[i], "none")) @@ -7576,27 +7576,21 @@ qemuParseCommandLineDisk(virCapsPtr caps, def->rerror_policy = VIR_DOMAIN_DISK_ERROR_POLICY_IGNORE; } else if (STREQ(keywords[i], "index")) { if (virStrToLong_i(values[i], NULL, 10, &idx) < 0) { - virDomainDiskDefFree(def); - def = NULL; virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse drive index '%s'"), val); - goto cleanup; + goto error; } } else if (STREQ(keywords[i], "bus")) { if (virStrToLong_i(values[i], NULL, 10, &busid) < 0) { - virDomainDiskDefFree(def); - def = NULL; virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse drive bus '%s'"), val); - goto cleanup; + goto error; } } else if (STREQ(keywords[i], "unit")) { if (virStrToLong_i(values[i], NULL, 10, &unitid) < 0) { - virDomainDiskDefFree(def); - def = NULL; virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse drive unit '%s'"), val); - goto cleanup; + goto error; } } else if (STREQ(keywords[i], "readonly")) { if ((values[i] == NULL) || STREQ(values[i], "on")) @@ -7655,9 +7649,7 @@ qemuParseCommandLineDisk(virCapsPtr caps, def->type != VIR_DOMAIN_DISK_TYPE_NETWORK) { virReportError(VIR_ERR_INTERNAL_ERROR, _("missing file parameter in drive '%s'"), val); - virDomainDiskDefFree(def); - def = NULL; - goto cleanup; + goto error; } if (idx == -1 && def->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) @@ -7667,10 +7659,9 @@ qemuParseCommandLineDisk(virCapsPtr caps, unitid == -1 && busid == -1) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing index/unit/bus parameter in drive '%s'"), val); - virDomainDiskDefFree(def); - def = NULL; - goto cleanup; + _("missing index/unit/bus parameter in drive '%s'"), + val); + goto error; } if (idx == -1) { @@ -7704,10 +7695,8 @@ qemuParseCommandLineDisk(virCapsPtr caps, } if (!def->dst) { - virDomainDiskDefFree(def); - def = NULL; virReportOOMError(); - goto cleanup; + goto error; } if (STREQ(def->dst, "xvda")) def->dst[3] = 'a' + idx; @@ -7730,6 +7719,11 @@ cleanup: VIR_FREE(keywords); VIR_FREE(values); return def; + +error: + virDomainDiskDefFree(def); + def = NULL; + goto cleanup; } /* -- 1.7.11.7
On Wed, Feb 06, 2013 at 04:35:40PM -0500, John Ferlan wrote:
The qemuParseGlusterString() replaced dst->src without a VIR_FREE() of what was in there before.
The qemuBuildCommandLine() did not properly free the boot_buf depending on various usages.
The qemuParseCommandLineDisk() had numerous paths that didn't clean up the virDomainDiskDefPtr def properly. Adjust the logic to go through an error: label before cleanup in order to free the resource. --- src/qemu/qemu_command.c | 68 ++++++++++++++++++++++---------------------------
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The 'package' string returned by qemuMonitorGetVersion() needs to be VIR_FREE()'d. testQemuMonitorJSONGetMachines(), testQemuMonitorJSONGetCPUDefinitions(), and testQemuMonitorJSONGetCommands() did not VIR_FREE() the array and array elements allocated by their respective qemuMonitorGet* routines. --- tests/qemumonitorjsontest.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 35f2da6..c4d4bc5 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -131,7 +131,7 @@ testQemuMonitorJSONGetVersion(const void *data) int major; int minor; int micro; - char *package; + char *package = NULL; if (!test) return -1; @@ -188,6 +188,7 @@ testQemuMonitorJSONGetVersion(const void *data) "Package %s was not ''", package); goto cleanup; } + VIR_FREE(package); if (qemuMonitorGetVersion(qemuMonitorTestGetMonitor(test), &major, &minor, µ, @@ -220,6 +221,7 @@ testQemuMonitorJSONGetVersion(const void *data) cleanup: qemuMonitorTestFree(test); + VIR_FREE(package); return ret; } @@ -230,8 +232,9 @@ testQemuMonitorJSONGetMachines(const void *data) qemuMonitorTestPtr test = qemuMonitorTestNew(true, caps); int ret = -1; qemuMonitorMachineInfoPtr *info; - int ninfo; + int ninfo = 0; const char *null = NULL; + int i; if (!test) return -1; @@ -296,6 +299,10 @@ testQemuMonitorJSONGetMachines(const void *data) cleanup: qemuMonitorTestFree(test); + for (i = 0; i < ninfo; i++) + qemuMonitorMachineInfoFree(info[i]); + VIR_FREE(info); + return ret; } @@ -307,7 +314,8 @@ testQemuMonitorJSONGetCPUDefinitions(const void *data) qemuMonitorTestPtr test = qemuMonitorTestNew(true, caps); int ret = -1; char **cpus = NULL; - int ncpus; + int ncpus = 0; + int i; if (!test) return -1; @@ -358,6 +366,9 @@ testQemuMonitorJSONGetCPUDefinitions(const void *data) cleanup: qemuMonitorTestFree(test); + for (i = 0; i < ncpus; i++) + VIR_FREE(cpus[i]); + VIR_FREE(cpus); return ret; } @@ -369,7 +380,8 @@ testQemuMonitorJSONGetCommands(const void *data) qemuMonitorTestPtr test = qemuMonitorTestNew(true, caps); int ret = -1; char **commands = NULL; - int ncommands; + int ncommands = 0; + int i; if (!test) return -1; @@ -419,6 +431,9 @@ testQemuMonitorJSONGetCommands(const void *data) cleanup: qemuMonitorTestFree(test); + for (i = 0; i < ncommands; i++) + VIR_FREE(commands[i]); + VIR_FREE(commands); return ret; } -- 1.7.11.7
On Wed, Feb 06, 2013 at 04:35:41PM -0500, John Ferlan wrote:
The 'package' string returned by qemuMonitorGetVersion() needs to be VIR_FREE()'d.
testQemuMonitorJSONGetMachines(), testQemuMonitorJSONGetCPUDefinitions(), and testQemuMonitorJSONGetCommands() did not VIR_FREE() the array and array elements allocated by their respective qemuMonitorGet* routines. --- tests/qemumonitorjsontest.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
When Valgrind runs the 'qemumonitorjsontest' it would claim that the thread created is leaked. That's because the virThreadJoin won't get called due to the 'running' flag being cleared. In order to avoid that, call virThreadJoin unconditionally at cleanup time. Also noted that the qemuMonitorTestWorker() didn't get the test mutex lock on the failure path. The incoming and outgoing buffers allocated by qemuMonitorTestIO() and qemuMonitorTestAddReponse() were never VIR_FREE()'d in qemuMonitorTestFree(). --- tests/qemumonitortestutils.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 7b90c38..1ed42ce 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -308,6 +308,7 @@ static void qemuMonitorTestWorker(void *opaque) virMutexUnlock(&test->lock); if (virEventRunDefaultImpl() < 0) { + virMutexLock(&test->lock); test->quit = true; break; } @@ -370,12 +371,14 @@ void qemuMonitorTestFree(qemuMonitorTestPtr test) virObjectUnref(test->vm); - if (test->running) - virThreadJoin(&test->thread); + virThreadJoin(&test->thread); if (timer != -1) virEventRemoveTimeout(timer); + VIR_FREE(test->incoming); + VIR_FREE(test->outgoing); + for (i = 0 ; i < test->nitems ; i++) qemuMonitorTestItemFree(test->items[i]); VIR_FREE(test->items); -- 1.7.11.7
On Wed, Feb 06, 2013 at 04:35:42PM -0500, John Ferlan wrote:
When Valgrind runs the 'qemumonitorjsontest' it would claim that the thread created is leaked. That's because the virThreadJoin won't get called due to the 'running' flag being cleared. In order to avoid that, call virThreadJoin unconditionally at cleanup time. Also noted that the qemuMonitorTestWorker() didn't get the test mutex lock on the failure path.
The incoming and outgoing buffers allocated by qemuMonitorTestIO() and qemuMonitorTestAddReponse() were never VIR_FREE()'d in qemuMonitorTestFree(). --- tests/qemumonitortestutils.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
AKC Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Need to initialize 'usage' and 'critical' since the VIR_DEBUG will attempt to use them. --- src/rpc/virnettlscontext.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 3e7e5e0..6665268 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -217,8 +217,8 @@ static int virNetTLSContextCheckCertKeyUsage(gnutls_x509_crt_t cert, bool isCA) { int status; - unsigned int usage; - unsigned int critical; + unsigned int usage = 0; + unsigned int critical = 0; status = gnutls_x509_crt_get_key_usage(cert, &usage, &critical); -- 1.7.11.7
On 2013年02月07日 05:35, John Ferlan wrote:
Need to initialize 'usage' and 'critical' since the VIR_DEBUG will attempt to use them. --- src/rpc/virnettlscontext.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 3e7e5e0..6665268 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -217,8 +217,8 @@ static int virNetTLSContextCheckCertKeyUsage(gnutls_x509_crt_t cert, bool isCA) { int status; - unsigned int usage; - unsigned int critical; + unsigned int usage = 0; + unsigned int critical = 0;
status = gnutls_x509_crt_get_key_usage(cert,&usage,&critical);
ACK.
--- tests/.valgrind.supp | 292 ++++++++++----------------------------------------- 1 file changed, 55 insertions(+), 237 deletions(-) diff --git a/tests/.valgrind.supp b/tests/.valgrind.supp index 7190103..10cc3c0 100644 --- a/tests/.valgrind.supp +++ b/tests/.valgrind.supp @@ -4,256 +4,74 @@ fun:malloc fun:xmalloc ... - obj:/bin/bash + fun:execute_command_internal + ... + obj:*/bin/bash } { bashMemoryLeak2 Memcheck:Leak fun:malloc fun:xmalloc - fun:make_bare_simple_command - fun:make_simple_command + ... fun:yyparse fun:parse_command fun:read_command - fun:reader_loop - fun:main -} -{ - xenDriverGlobalRegexes1 - Memcheck:Leak - fun:* - fun:regcomp - fun:xenHypervisorInit - fun:xenUnifiedRegister - fun:virInitialize - fun:virConnectOpenReadOnly - fun:testCompareHelper - fun:virtTestRun - fun:mymain - fun:virtTestMain -} -{ - xenDriverGlobalRegexes2 - Memcheck:Leak - fun:* - fun:* - fun:regcomp - fun:xenHypervisorInit - fun:xenUnifiedRegister - fun:virInitialize - fun:virConnectOpenReadOnly - fun:testCompareHelper - fun:virtTestRun - fun:mymain - fun:virtTestMain -} -{ - xenDriverGlobalRegexes3 - Memcheck:Leak - fun:* - fun:* - fun:* - fun:regcomp - fun:xenHypervisorInit - fun:xenUnifiedRegister - fun:virInitialize - fun:virConnectOpenReadOnly - fun:testCompareHelper - fun:virtTestRun - fun:mymain - fun:virtTestMain -} -{ - xenDriverGlobalRegexes4 - Memcheck:Leak - fun:* - fun:* - fun:* - fun:* - fun:regcomp - fun:xenHypervisorInit - fun:xenUnifiedRegister - fun:virInitialize - fun:virConnectOpenReadOnly - fun:testCompareHelper - fun:virtTestRun - fun:mymain - fun:virtTestMain -} -{ - libXMLGlobals1 - Memcheck:Leak - fun:malloc - fun:xmlNewMutex - fun:xmlInitGlobals - fun:xmlInitParser - fun:xmlParseDocument - obj:/usr/lib64/libxml2.so.2.6.32 - fun:virDomainDefParseString - fun:testOpen - fun:do_open - fun:testCompareHelper - fun:virtTestRun - fun:mymain -} -{ - xenDriverGlobalRegexes1 - Memcheck:Leak - fun:* - fun:regcomp - fun:xenHypervisorInit - fun:xenUnifiedRegister - fun:virInitialize - fun:virConnectOpenReadOnly - fun:testCompareHelper - fun:virtTestRun - fun:mymain - fun:virtTestMain -} -{ - xenDriverGlobalRegexes2 - Memcheck:Leak - fun:* - fun:* - fun:regcomp - fun:xenHypervisorInit - fun:xenUnifiedRegister - fun:virInitialize - fun:virConnectOpenReadOnly - fun:testCompareHelper - fun:virtTestRun - fun:mymain - fun:virtTestMain -} -{ - xenDriverGlobalRegexes3 - Memcheck:Leak - fun:* - fun:* - fun:* - fun:regcomp - fun:xenHypervisorInit - fun:xenUnifiedRegister - fun:virInitialize - fun:virConnectOpenReadOnly - fun:testCompareHelper - fun:virtTestRun - fun:mymain - fun:virtTestMain -} -{ - xenDriverGlobalRegexes4 - Memcheck:Leak - fun:* - fun:* - fun:* - fun:* - fun:regcomp - fun:xenHypervisorInit - fun:xenUnifiedRegister - fun:virInitialize - fun:virConnectOpenReadOnly - fun:testCompareHelper - fun:virtTestRun - fun:mymain - fun:virtTestMain -} -{ - libXMLGlobals1 - Memcheck:Leak - fun:malloc - fun:xmlNewMutex - fun:xmlInitGlobals - fun:xmlInitParser - fun:xmlParseDocument - obj:/usr/lib64/libxml2.so.2.6.32 - fun:virDomainDefParseString - fun:testOpen - fun:do_open - fun:testCompareHelper - fun:virtTestRun - fun:mymain -} -{ - ignoreThreadLocalErrorObject - Memcheck:Leak - fun:calloc - fun:virAlloc - fun:virLastErrorObject - fun:virRaiseError - fun:statsErrorFunc - fun:xenLinuxDomainDeviceID - fun:testDeviceHelper - fun:virtTestRun - fun:mymain - fun:virtTestMain - fun:main -} -{ - cg1 - Memcheck:Param - capget(data) - fun:capget - fun:* - fun:capng_clear - fun:virClearCapabilities - fun:__virExec - fun:virExecWithHook -} -{ - libnlMemoryLeak1 - Memcheck:Leak - fun:malloc - fun:strdup - obj:/usr/lib/libnl.so.1.1 -} -{ - libnlMemoryLeak2 - Memcheck:Leak - fun:calloc - obj:/usr/lib/libnl.so.1.1 -} -{ - libnlMemoryLeak3 - Memcheck:Leak - fun:?alloc ... - obj:/lib64/libnl.so.1.1 -} -{ - libselinuxMemoryLeak1 - Memcheck:Leak - fun:malloc - fun:getdelim - obj:/lib/libselinux.so.1 -} -{ - dashMemoryLeak1 - Memcheck:Leak - fun:malloc - obj:/bin/dash + obj:*/bin/bash } { - dashMemoryLeak2 + bashMemoryLeak3 Memcheck:Leak fun:malloc - fun:strdup - obj:/bin/dash -} -{ - vboxMemoryLeak1 - Memcheck:Leak + fun:xmalloc + fun:array_create + fun:array_copy + fun:run_exit_trap + fun:exit_shell ... - fun:VBoxNsxpNS_InitXPCOM2 -} -{ - libnetcfMemoryLeak1 - fun:malloc - fun:xmlStrndup - fun:xmlHashUpdateEntry3 - fun:* - fun:xsltRegisterAllExtras - fun:drv_init - fun:interfaceOpenInterface + obj:*/bin/bash +} +# +# Failure seen in /usr/lib64/ld-2.15.so +# +{ + dlInitMemoryLeak1 + Memcheck:Leak + fun:?alloc + ... + fun:call_init.part.0 + fun:_dl_init + ... + obj:*/lib*/ld-2.*so* +} +# +# Failure seen in +# p11_kit_registered_module_to_name: /usr/lib64/libp11-kit.so.0.0.0 +# gnutls_pkcs11_init: /usr/lib64/libgnutls.so.26.22.4 +# +{ + gnutlsInitMemoryLeak + Memcheck:Leak + fun:malloc + fun:strdup + fun:p11_kit_registered_module_to_name + fun:gnutls_pkcs11_init + fun:gnutls_global_init + ... + obj:*/lib*/libc-2.*so* +} +# +# Failure seen in eventtest +# +{ + eventtestMemoryLeak + Memcheck:Leak + fun:calloc + fun:_dl_allocate_tls + fun:pthread_create* + fun:mymain + fun:virtTestMain + ... + obj:*/lib*/libc-2.*so* } -- 1.7.11.7
On Wed, Feb 06, 2013 at 04:35:44PM -0500, John Ferlan wrote:
--- tests/.valgrind.supp | 292 ++++++++++----------------------------------------- 1 file changed, 55 insertions(+), 237 deletions(-)
ACK, been meaning todo this for a while now. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--- docs/hacking.html.in | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 93 insertions(+), 3 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index fe4324a..7ef826c 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -109,8 +109,9 @@ make syntax-check make -C tests valgrind </pre> - <p> - The latter test checks for memory leaks. + <p><a href="http://valgrind.org/">Valgrind</a> is a test that checks + for memory management issues, such as leaks or use of uninitialized + variables. </p> <p> @@ -134,7 +135,96 @@ <p>There is also a <code>./run</code> script at the top level, to make it easier to run programs that have not yet been installed, as well as to wrap invocations of various tests - under gdb or valgrind.</p> + under gdb or Valgrind. + </p> + + </li> + <li><p>The Valgrind test should produce similar output to + <code>make check</code>. If the output has traces within libvirt + API's, then investigation is required in order to determine the + cause of the issue. Output such as the following indicates some + sort of leak: + </p> +<pre> +==5414== 4 bytes in 1 blocks are definitely lost in loss record 3 of 89 +==5414== at 0x4A0881C: malloc (vg_replace_malloc.c:270) +==5414== by 0x34DE0AAB85: xmlStrndup (in /usr/lib64/libxml2.so.2.7.8) +==5414== by 0x4CC97A6: virDomainVideoDefParseXML (domain_conf.c:7410) +==5414== by 0x4CD581D: virDomainDefParseXML (domain_conf.c:10188) +==5414== by 0x4CD8C73: virDomainDefParseNode (domain_conf.c:10640) +==5414== by 0x4CD8DDB: virDomainDefParse (domain_conf.c:10590) +==5414== by 0x41CB1D: testCompareXMLToArgvHelper (qemuxml2argvtest.c:100) +==5414== by 0x41E20F: virtTestRun (testutils.c:161) +==5414== by 0x41C7CB: mymain (qemuxml2argvtest.c:866) +==5414== by 0x41E84A: virtTestMain (testutils.c:723) +==5414== by 0x34D9021734: (below main) (in /usr/lib64/libc-2.15.so) +</pre> + <p>In this example, the <code>virDomainDefParseXML()</code> had + an error path where the <code>virDomainVideoDefPtr video</code> + pointer was not properly disposed. By simply adding a + <code>virDomainVideoDefFree(video);</code> in the error path, + the issue was resolved. + </p> + + <p>Another common mistake is calling a printing function, such as + <code>VIR_DEBUG()</code> without initializing a variable to be + printed. The following example involved a call which could return + an error, but not set variables passed by reference to the call. + The solution was to initialize the variables prior to the call. + </p> +<pre> +==4749== Use of uninitialised value of size 8 +==4749== at 0x34D904650B: _itoa_word (in /usr/lib64/libc-2.15.so) +==4749== by 0x34D9049118: vfprintf (in /usr/lib64/libc-2.15.so) +==4749== by 0x34D9108F60: __vasprintf_chk (in /usr/lib64/libc-2.15.so) +==4749== by 0x4CAEEF7: virVasprintf (stdio2.h:199) +==4749== by 0x4C8A55E: virLogVMessage (virlog.c:814) +==4749== by 0x4C8AA96: virLogMessage (virlog.c:751) +==4749== by 0x4DA0056: virNetTLSContextCheckCertKeyUsage (virnettlscontext.c:225) +==4749== by 0x4DA06DB: virNetTLSContextCheckCert (virnettlscontext.c:439) +==4749== by 0x4DA1620: virNetTLSContextNew (virnettlscontext.c:562) +==4749== by 0x4DA26FC: virNetTLSContextNewServer (virnettlscontext.c:927) +==4749== by 0x409C39: testTLSContextInit (virnettlscontexttest.c:467) +==4749== by 0x40AB8F: virtTestRun (testutils.c:161) +</pre> + <p>Valgrind will also find some false positives or code paths + which cannot be resolved by making changes to the libvirt code. + For these paths, it is possible to add a filter to avoid the + errors. For example: + </p> +<pre> +==4643== 7 bytes in 1 blocks are possibly lost in loss record 4 of 20 +==4643== at 0x4A0881C: malloc (vg_replace_malloc.c:270) +==4643== by 0x34D90853F1: strdup (in /usr/lib64/libc-2.15.so) +==4643== by 0x34EEC2C08A: ??? (in /usr/lib64/libnl.so.1.1) +==4643== by 0x34EEC15B81: ??? (in /usr/lib64/libnl.so.1.1) +==4643== by 0x34D8C0EE15: call_init.part.0 (in /usr/lib64/ld-2.15.so) +==4643== by 0x34D8C0EECF: _dl_init (in /usr/lib64/ld-2.15.so) +==4643== by 0x34D8C01569: ??? (in /usr/lib64/ld-2.15.so) + +</pre> + <p>In this instance, it is acceptible to modify the + <code>tests/.valgrind.supp</code> file in order to add a + suppression filter. The filter should be unique enough to + not suppress real leaks, but it should be generic enough to + cover multiple code paths. The format of the entry can be + found in the documentation found at the + <a href="http://valgrind.org/">Valgrind home page.</a> + The following trace was added to <code>tests/.valgrind.supp</code> + in order to suppress the warning: + </p> +<pre> +{ + dlInitMemoryLeak1 + Memcheck:Leak + fun:?alloc + ... + fun:call_init.part.0 + fun:_dl_init + ... + obj:*/lib*/ld-2.*so* +} +</pre> </li> <li>Update tests and/or documentation, particularly if you are adding a new feature or changing the output of a program.</li> -- 1.7.11.7
On Wed, Feb 06, 2013 at 04:35:45PM -0500, John Ferlan wrote:
--- docs/hacking.html.in | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 93 insertions(+), 3 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--- HACKING | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 87 insertions(+), 6 deletions(-) diff --git a/HACKING b/HACKING index c7df3f3..e1e0b9b 100644 --- a/HACKING +++ b/HACKING @@ -103,7 +103,11 @@ and run the tests: make syntax-check make -C tests valgrind -The latter test checks for memory leaks. + + + Valgrind + http://valgrind.org/is a test that checks for memory management issues, such as leaks or use of +uninitialized variables. If you encounter any failing tests, the VIR_TEST_DEBUG environment variable may provide extra information to debug the failures. Larger values of @@ -118,11 +122,88 @@ Also, individual tests can be run from inside the "tests/" directory, like: There is also a "./run" script at the top level, to make it easier to run programs that have not yet been installed, as well as to wrap invocations of -various tests under gdb or valgrind. - - - -(7) Update tests and/or documentation, particularly if you are adding a new +various tests under gdb or Valgrind. + + + +(7) The Valgrind test should produce similar output to "make check". If the output +has traces within libvirt API's, then investigation is required in order to +determine the cause of the issue. Output such as the following indicates some +sort of leak: + +==5414== 4 bytes in 1 blocks are definitely lost in loss record 3 of 89 +==5414== at 0x4A0881C: malloc (vg_replace_malloc.c:270) +==5414== by 0x34DE0AAB85: xmlStrndup (in /usr/lib64/libxml2.so.2.7.8) +==5414== by 0x4CC97A6: virDomainVideoDefParseXML (domain_conf.c:7410) +==5414== by 0x4CD581D: virDomainDefParseXML (domain_conf.c:10188) +==5414== by 0x4CD8C73: virDomainDefParseNode (domain_conf.c:10640) +==5414== by 0x4CD8DDB: virDomainDefParse (domain_conf.c:10590) +==5414== by 0x41CB1D: testCompareXMLToArgvHelper (qemuxml2argvtest.c:100) +==5414== by 0x41E20F: virtTestRun (testutils.c:161) +==5414== by 0x41C7CB: mymain (qemuxml2argvtest.c:866) +==5414== by 0x41E84A: virtTestMain (testutils.c:723) +==5414== by 0x34D9021734: (below main) (in /usr/lib64/libc-2.15.so) + +In this example, the "virDomainDefParseXML()" had an error path where the +"virDomainVideoDefPtr video" pointer was not properly disposed. By simply +adding a "virDomainVideoDefFree(video);" in the error path, the issue was +resolved. + +Another common mistake is calling a printing function, such as "VIR_DEBUG()" +without initializing a variable to be printed. The following example involved +a call which could return an error, but not set variables passed by reference +to the call. The solution was to initialize the variables prior to the call. + +==4749== Use of uninitialised value of size 8 +==4749== at 0x34D904650B: _itoa_word (in /usr/lib64/libc-2.15.so) +==4749== by 0x34D9049118: vfprintf (in /usr/lib64/libc-2.15.so) +==4749== by 0x34D9108F60: __vasprintf_chk (in /usr/lib64/libc-2.15.so) +==4749== by 0x4CAEEF7: virVasprintf (stdio2.h:199) +==4749== by 0x4C8A55E: virLogVMessage (virlog.c:814) +==4749== by 0x4C8AA96: virLogMessage (virlog.c:751) +==4749== by 0x4DA0056: virNetTLSContextCheckCertKeyUsage (virnettlscontext.c:225) +==4749== by 0x4DA06DB: virNetTLSContextCheckCert (virnettlscontext.c:439) +==4749== by 0x4DA1620: virNetTLSContextNew (virnettlscontext.c:562) +==4749== by 0x4DA26FC: virNetTLSContextNewServer (virnettlscontext.c:927) +==4749== by 0x409C39: testTLSContextInit (virnettlscontexttest.c:467) +==4749== by 0x40AB8F: virtTestRun (testutils.c:161) + +Valgrind will also find some false positives or code paths which cannot be +resolved by making changes to the libvirt code. For these paths, it is +possible to add a filter to avoid the errors. For example: + +==4643== 7 bytes in 1 blocks are possibly lost in loss record 4 of 20 +==4643== at 0x4A0881C: malloc (vg_replace_malloc.c:270) +==4643== by 0x34D90853F1: strdup (in /usr/lib64/libc-2.15.so) +==4643== by 0x34EEC2C08A: ??? (in /usr/lib64/libnl.so.1.1) +==4643== by 0x34EEC15B81: ??? (in /usr/lib64/libnl.so.1.1) +==4643== by 0x34D8C0EE15: call_init.part.0 (in /usr/lib64/ld-2.15.so) +==4643== by 0x34D8C0EECF: _dl_init (in /usr/lib64/ld-2.15.so) +==4643== by 0x34D8C01569: ??? (in /usr/lib64/ld-2.15.so) + + +In this instance, it is acceptible to modify the "tests/.valgrind.supp" file +in order to add a suppression filter. The filter should be unique enough to +not suppress real leaks, but it should be generic enough to cover multiple +code paths. The format of the entry can be found in the documentation found at +the + + Valgrind home page. + http://valgrind.org/The following trace was added to "tests/.valgrind.supp" in order to suppress +the warning: + +{ + dlInitMemoryLeak1 + Memcheck:Leak + fun:?alloc + ... + fun:call_init.part.0 + fun:_dl_init + ... + obj:*/lib*/ld-2.*so* +} + +(8) Update tests and/or documentation, particularly if you are adding a new feature or changing the output of a program. -- 1.7.11.7
On Wed, Feb 06, 2013 at 04:35:46PM -0500, John Ferlan wrote:
--- HACKING | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 87 insertions(+), 6 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The Valgrind examples show traces of output with alloc functions in them --- cfg.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfg.mk b/cfg.mk index 2dfde01..47f59dd 100644 --- a/cfg.mk +++ b/cfg.mk @@ -816,7 +816,7 @@ exclude_file_name_regexp--sc_prohibit_nonreentrant = \ ^((po|tests)/|docs/.*py|run.in$$) exclude_file_name_regexp--sc_prohibit_raw_allocation = \ - ^(src/util/viralloc\.[ch]|examples/.*|tests/securityselinuxhelper.c)$$ + ^(docs/hacking\.html\.in)|(src/util/viralloc\.[ch]|examples/.*|tests/securityselinuxhelper.c)$$ exclude_file_name_regexp--sc_prohibit_readlink = \ ^src/(util/virutil|lxc/lxc_container)\.c$$ -- 1.7.11.7
On Wed, Feb 06, 2013 at 04:35:47PM -0500, John Ferlan wrote:
The Valgrind examples show traces of output with alloc functions in them --- cfg.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cfg.mk b/cfg.mk index 2dfde01..47f59dd 100644 --- a/cfg.mk +++ b/cfg.mk @@ -816,7 +816,7 @@ exclude_file_name_regexp--sc_prohibit_nonreentrant = \ ^((po|tests)/|docs/.*py|run.in$$)
exclude_file_name_regexp--sc_prohibit_raw_allocation = \ - ^(src/util/viralloc\.[ch]|examples/.*|tests/securityselinuxhelper.c)$$ + ^(docs/hacking\.html\.in)|(src/util/viralloc\.[ch]|examples/.*|tests/securityselinuxhelper.c)$$
exclude_file_name_regexp--sc_prohibit_readlink = \ ^src/(util/virutil|lxc/lxc_container)\.c$$
ACK, but you should merge this into the patch to hacking.html.in which introduces the problem Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 02/06/2013 04:35 PM, John Ferlan wrote:
This set of patches resolves issues seen in the Valgrind tests.
The update to the .valgrind.supp file is from my environment and is very basic. Some paths in the previous version don't exist anymore. Based on recent (and continuing) commandtest failures, adjustments still may be necessary depending on environment.
To assist in that I also updated the hacking document to describe a bit more about the Valgrind tests with respect to success, failure, and false positives. In particular, how to hopefully recognize each and decide whether what's seen is a false positive.
Prior to the recent vircommand and commandtest changes, the commandtest was passing, but now it fails. The failure may be related to:
https://www.redhat.com/archives/libvir-list/2013-February/msg00269.html
I spent some time looking at the output, but could not figure out a root cause. I'll keep trying, but figured at the very least the other changes are still valid.
John Ferlan (13): virnettlscontexttest: Resolve memory leak found by Valgrind qemuxml2argvtest: Resolve resource leaks found by Valgrind netdev_vlan_conf: Resolve memory leak found by Valgrind. vport_profile_conf: Resolve memory leak found by Valgrind domain_conf: Resolve resource leaks found by Valgrind qemu_command: Resolve resource leaks found by Valgrind qemumonitorjsontest: Resolve resource leaks found by Valgrind qemumonitortestutils: Resolve resource leaks found by Valgrind virnetttlcontext: Resolve issues found by Valgrind valgrind: Adjust the suppression file hacking: Add more information about Valgrind HACKING: Sync with docs/hacking.html.in cfg.mk: Add hacking.in.html to sc_prohibit_raw_allocation
HACKING | 93 ++++++++++- cfg.mk | 2 +- docs/hacking.html.in | 96 +++++++++++- src/conf/domain_conf.c | 12 +- src/conf/netdev_vlan_conf.c | 3 +- src/conf/netdev_vport_profile_conf.c | 1 + src/qemu/qemu_command.c | 68 ++++---- src/rpc/virnettlscontext.c | 4 +- tests/.valgrind.supp | 292 +++++++---------------------------- tests/qemumonitorjsontest.c | 23 ++- tests/qemumonitortestutils.c | 7 +- tests/qemuxml2argvtest.c | 6 +- tests/virnettlscontexttest.c | 8 +- 13 files changed, 319 insertions(+), 296 deletions(-)
Pushed with changes including merging cfg.mk w/ hacking.html.in Thanks, John
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Osier Yang