[libvirt PATCH 0/7] valgrind-inspired fixes
First, clean up some valgrind noise. Then, fix some reported leaks in the test suite. Last, refactor some touched tests. Ján Tomko (7): tests: valgrind.supp: suppress g_type_register_static leaks tests: valgrind: do not trace system binaries qemumonitorjsontest: do not leak qapiData.schema qemumonitorjsontest: use virCPUDefNew() virsystemdtest: do not leak socket path qemumonitorjsontest: GetCPUModelComparison: use g_auto qemumonitorjsontest: GetCPUModelBaseline: use g_auto tests/.valgrind.supp | 13 +++++++++++++ tests/Makefile.am | 2 +- tests/qemumonitorjsontest.c | 29 ++++++++--------------------- tests/virsystemdtest.c | 5 ++++- 4 files changed, 26 insertions(+), 23 deletions(-) -- 2.24.1
When a type is registered, it holds allocated memory until the program exits. Add an exception to valgrind.supp to make the output of make -C tests valgrind more readable. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/.valgrind.supp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/.valgrind.supp b/tests/.valgrind.supp index 6fb724ff13..f78b3b1f72 100644 --- a/tests/.valgrind.supp +++ b/tests/.valgrind.supp @@ -148,3 +148,16 @@ fun:virObjectUnref fun:main } +# +# types registered with GLib are never freed +# +{ + glibTypeRegisterLeak + Memcheck:Leak + match-leak-kinds: possible + ... + fun:g_realloc + obj:*/lib*/libgobject* + fun:g_type_register_static + ... +} -- 2.24.1
Add /usr/bin/* to -trace-children-skip Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Makefile.am b/tests/Makefile.am index ed5255b62d..7c76b5cec6 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -482,7 +482,7 @@ TESTS_ENVIRONMENT = \ VALGRIND = valgrind --quiet --leak-check=full --trace-children=yes \ - --trace-children-skip="*/tools/virsh","*/tests/commandhelper" \ + --trace-children-skip="*/tools/virsh","*/tests/commandhelper","/usr/bin/*" \ --suppressions=$(abs_srcdir)/.valgrind.supp valgrind: $(MAKE) check VG="$(LIBTOOL) --mode=execute $(VALGRIND)" -- 2.24.1
Free the x86_64 schema before overwriting it with s390x schema. Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: eee09435eec46553aac4fdf1c2d8f3214167bded --- tests/qemumonitorjsontest.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index e9f95e317d..40fc40bddd 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -3375,6 +3375,7 @@ mymain(void) #undef DO_TEST_QUERY_JOBS + virHashFree(qapiData.schema); if (!(qapiData.schema = testQEMUSchemaLoad("s390x"))) { VIR_TEST_VERBOSE("failed to load qapi schema for s390x"); ret = -1; -- 2.24.1
virCPUDefPtr uses refcounting internally and must be allocated using virCPUDefNew, otherwise virCPUDefFree would be a no-op. Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: fa2404bf4f91813431beb797fc30a1237a743823 Fixes: eee09435eec46553aac4fdf1c2d8f3214167bded --- tests/qemumonitorjsontest.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 40fc40bddd..1a090b0cb5 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2997,8 +2997,8 @@ testQemuMonitorJSONqemuMonitorJSONGetCPUModelComparison(const void *opaque) "{\"return\":{\"result\":\"test\"}}") < 0) return -1; - if (VIR_ALLOC(cpu_a) < 0 || VIR_ALLOC(cpu_b) < 0) - goto cleanup; + cpu_a = virCPUDefNew(); + cpu_b = virCPUDefNew(); cpu_a->model = g_strdup("cpu_a"); cpu_b->model = g_strdup("cpu_b"); @@ -3049,8 +3049,8 @@ testQemuMonitorJSONqemuMonitorJSONGetCPUModelBaseline(const void *opaque) "}") < 0) return -1; - if (VIR_ALLOC(cpu_a) < 0 || VIR_ALLOC(cpu_b) < 0) - goto cleanup; + cpu_a = virCPUDefNew(); + cpu_b = virCPUDefNew(); cpu_a->model = g_strdup("cpu_a"); cpu_b->model = g_strdup("cpu_b"); -- 2.24.1
Use an autofree'd helper variable to store the socket path and free it after the function finishes. Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: 5b8569dd6e284b9159c701e8bffafb196983fc4a --- tests/virsystemdtest.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/virsystemdtest.c b/tests/virsystemdtest.c index a9b02af7db..2ba288d032 100644 --- a/tests/virsystemdtest.c +++ b/tests/virsystemdtest.c @@ -554,12 +554,15 @@ testActivation(bool useNames) size_t nfds = 0; g_autoptr(virSystemdActivation) act = NULL; g_auto(virBuffer) names = VIR_BUFFER_INITIALIZER; + g_autofree char *demo_socket_path = NULL; virBufferAddLit(&names, "demo-unix.socket"); if (testActivationCreateFDs(&sockUNIX, &sockIP, &nsockIP) < 0) return -1; + demo_socket_path = virNetSocketGetPath(sockUNIX); + for (i = 0; i < nsockIP; i++) virBufferAddLit(&names, ":demo-ip.socket"); @@ -576,7 +579,7 @@ testActivation(bool useNames) map[0].name = "demo-unix.socket"; map[0].family = AF_UNIX; - map[0].path = virNetSocketGetPath(sockUNIX); + map[0].path = demo_socket_path; map[1].name = "demo-ip.socket"; map[1].family = AF_INET; -- 2.24.1
Use g_autoptr for the virCPUDef variables and get rid of the cleanup label. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/qemumonitorjsontest.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 1a090b0cb5..168d6cd172 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2985,10 +2985,9 @@ testQemuMonitorJSONqemuMonitorJSONGetCPUModelComparison(const void *opaque) { const testGenericData *data = opaque; g_autoptr(qemuMonitorTest) test = NULL; - virCPUDefPtr cpu_a; - virCPUDefPtr cpu_b = NULL; + g_autoptr(virCPUDef) cpu_a = virCPUDefNew(); + g_autoptr(virCPUDef) cpu_b = virCPUDefNew(); g_autofree char *result = NULL; - int ret = -1; if (!(test = qemuMonitorTestNewSchema(data->xmlopt, data->schema))) return -1; @@ -2997,28 +2996,20 @@ testQemuMonitorJSONqemuMonitorJSONGetCPUModelComparison(const void *opaque) "{\"return\":{\"result\":\"test\"}}") < 0) return -1; - cpu_a = virCPUDefNew(); - cpu_b = virCPUDefNew(); - cpu_a->model = g_strdup("cpu_a"); cpu_b->model = g_strdup("cpu_b"); if (qemuMonitorJSONGetCPUModelComparison(qemuMonitorTestGetMonitor(test), cpu_a, cpu_b, &result) < 0) - goto cleanup; + return -1; if (!result || STRNEQ(result, "test")) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", "Compare result not set"); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - virCPUDefFree(cpu_a); - virCPUDefFree(cpu_b); - return ret; + return 0; } -- 2.24.1
Use g_autoptr for the virCPUDef variables. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/qemumonitorjsontest.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 168d6cd172..d3bee2043e 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -3018,8 +3018,8 @@ testQemuMonitorJSONqemuMonitorJSONGetCPUModelBaseline(const void *opaque) { const testGenericData *data = opaque; g_autoptr(qemuMonitorTest) test = NULL; - virCPUDefPtr cpu_a; - virCPUDefPtr cpu_b = NULL; + g_autoptr(virCPUDef) cpu_a = virCPUDefNew(); + g_autoptr(virCPUDef) cpu_b = virCPUDefNew(); qemuMonitorCPUModelInfoPtr baseline = NULL; int ret = -1; @@ -3040,9 +3040,6 @@ testQemuMonitorJSONqemuMonitorJSONGetCPUModelBaseline(const void *opaque) "}") < 0) return -1; - cpu_a = virCPUDefNew(); - cpu_b = virCPUDefNew(); - cpu_a->model = g_strdup("cpu_a"); cpu_b->model = g_strdup("cpu_b"); @@ -3082,8 +3079,6 @@ testQemuMonitorJSONqemuMonitorJSONGetCPUModelBaseline(const void *opaque) ret = 0; cleanup: - virCPUDefFree(cpu_a); - virCPUDefFree(cpu_b); qemuMonitorCPUModelInfoFree(baseline); return ret; } -- 2.24.1
On 22. 2. 2020 1:24, Ján Tomko wrote:
First, clean up some valgrind noise. Then, fix some reported leaks in the test suite. Last, refactor some touched tests.
Ján Tomko (7): tests: valgrind.supp: suppress g_type_register_static leaks tests: valgrind: do not trace system binaries qemumonitorjsontest: do not leak qapiData.schema qemumonitorjsontest: use virCPUDefNew() virsystemdtest: do not leak socket path qemumonitorjsontest: GetCPUModelComparison: use g_auto qemumonitorjsontest: GetCPUModelBaseline: use g_auto
tests/.valgrind.supp | 13 +++++++++++++ tests/Makefile.am | 2 +- tests/qemumonitorjsontest.c | 29 ++++++++--------------------- tests/virsystemdtest.c | 5 ++++- 4 files changed, 26 insertions(+), 23 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Ján Tomko -
Michal Prívozník