On 2013年02月07日 05:35, John Ferlan wrote: ACK

On 2013年02月07日 05:35, John Ferlan wrote: ACK

On 2013年02月07日 05:35, John Ferlan wrote: ACK

On 2013年02月07日 05:35, John Ferlan wrote: ACK

On 2013年02月07日 05:35, John Ferlan wrote: 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); } ACK with fixing the similar hunk.

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

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, &micro, @@ -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

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

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

--- 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

--- 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

--- 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

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