| Message ID | 1446618049-13596-4-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Make valgrind happy with the current state of the tests, so that > it is easier to see if future patches introduce new memory problems > without being drowned in noise. Many of the leaks were due to > calling a second init without tearing down the data from an earlier > visit. But since teardown is already idempotent, and we already > register teardown as part of input_visitor_test_add(), it is nicer > to just make init() safe to call multiple times than it is to have > to make all tests call teardown. > > Another common leak was forgetting to clean up an error object, > after testing that an error was raised. > > Another leak was in test_visitor_in_struct_nested(), failing to > clean the base member of UserDefTwo. Cleaning that up left > check_and_free_str() as dead code (since using the qapi_free_* > takes care of recursion, and we don't want double frees). > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v9: move earlier in series (was 13/17) > v8: no change > v7: no change > v6: make init repeatable rather than adding teardown everywhere, > fix additional leak with UserDefTwo base, plug additional files > --- > tests/test-qmp-input-strict.c | 10 ++++++++++ > tests/test-qmp-input-visitor.c | 41 +++++++---------------------------------- > tests/test-qmp-output-visitor.c | 3 ++- > 3 files changed, 19 insertions(+), 35 deletions(-) No leaks to plug in test/-qmp-commands.c and test-qmp-event.c? > diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c > index b44184f..910e2f9 100644 > --- a/tests/test-qmp-input-strict.c > +++ b/tests/test-qmp-input-strict.c > @@ -77,6 +77,8 @@ static Visitor *validate_test_init_raw(TestInputVisitorData *data, > { > Visitor *v; > > + validate_teardown(data, NULL); > + > data->obj = qobject_from_json(json_string); > g_assert(data->obj != NULL); > A test added with validate_test_add() may call validate_test_init_raw() any number of time. Since validate_test_add() passes validate_teardown() as fixture teardown function, the last one will be cleaned up on test finalization. The others will be cleaned up by the next validate_test_init_raw(). Okay. Actually, the whole fixture business starts to make sense only now. But why only validate_test_init_raw() and not validate_test_init()? The two duplicate code, by the way. > @@ -193,6 +195,8 @@ static void test_validate_fail_struct(TestInputVisitorData *data, > > visit_type_TestStruct(v, &p, NULL, &err); > g_assert(err); > + error_free(err); > + /* FIXME: visitor should not allocate p when returning error */ Indeed. Recommend to always mention new FIXMEs in the commit message. > if (p) { > g_free(p->string); > } > @@ -210,6 +214,7 @@ static void test_validate_fail_struct_nested(TestInputVisitorData *data, > > visit_type_UserDefTwo(v, &udp, NULL, &err); > g_assert(err); > + error_free(err); > qapi_free_UserDefTwo(udp); > } > > @@ -224,6 +229,7 @@ static void test_validate_fail_list(TestInputVisitorData *data, > > visit_type_UserDefOneList(v, &head, NULL, &err); > g_assert(err); > + error_free(err); > qapi_free_UserDefOneList(head); > } > > @@ -239,6 +245,7 @@ static void test_validate_fail_union_native_list(TestInputVisitorData *data, > > visit_type_UserDefNativeListUnion(v, &tmp, NULL, &err); > g_assert(err); > + error_free(err); > qapi_free_UserDefNativeListUnion(tmp); > } > > @@ -253,6 +260,7 @@ static void test_validate_fail_union_flat(TestInputVisitorData *data, > > visit_type_UserDefFlatUnion(v, &tmp, NULL, &err); > g_assert(err); > + error_free(err); > qapi_free_UserDefFlatUnion(tmp); > } > > @@ -268,6 +276,7 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data, > > visit_type_UserDefFlatUnion2(v, &tmp, NULL, &err); > g_assert(err); > + error_free(err); > qapi_free_UserDefFlatUnion2(tmp); > } > > @@ -282,6 +291,7 @@ static void test_validate_fail_alternate(TestInputVisitorData *data, > > visit_type_UserDefAlternate(v, &tmp, NULL, &err); > g_assert(err); > + error_free(err); > qapi_free_UserDefAlternate(tmp); > } > > diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c > index 3f6bc4d..03c3682 100644 > --- a/tests/test-qmp-input-visitor.c > +++ b/tests/test-qmp-input-visitor.c > @@ -46,6 +46,8 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data, > Visitor *v; > va_list ap; > > + visitor_input_teardown(data, NULL); > + > va_start(ap, json_string); > data->obj = qobject_from_jsonv(json_string, &ap); > va_end(ap); Here, you add it to visitor_input_test_init(), but not visitor_input_test_init_raw(). These two duplicate code, too. > @@ -177,12 +179,7 @@ static void test_visitor_in_enum(TestInputVisitorData *data, > visit_type_EnumOne(v, &res, NULL, &err); > g_assert(!err); > g_assert_cmpint(i, ==, res); > - > - visitor_input_teardown(data, NULL); > } > - > - data->obj = NULL; > - data->qiv = NULL; > } > > > @@ -205,12 +202,6 @@ static void test_visitor_in_struct(TestInputVisitorData *data, > g_free(p); > } > > -static void check_and_free_str(char *str, const char *cmp) > -{ > - g_assert_cmpstr(str, ==, cmp); > - g_free(str); > -} > - > static void test_visitor_in_struct_nested(TestInputVisitorData *data, > const void *unused) > { > @@ -226,17 +217,14 @@ static void test_visitor_in_struct_nested(TestInputVisitorData *data, > visit_type_UserDefTwo(v, &udp, NULL, &err); > g_assert(!err); > > - check_and_free_str(udp->string0, "string0"); > - check_and_free_str(udp->dict1->string1, "string1"); > + g_assert_cmpstr(udp->string0, ==, "string0"); > + g_assert_cmpstr(udp->dict1->string1, ==, "string1"); > g_assert_cmpint(udp->dict1->dict2->userdef->integer, ==, 42); > - check_and_free_str(udp->dict1->dict2->userdef->string, "string"); > - check_and_free_str(udp->dict1->dict2->string, "string2"); > + g_assert_cmpstr(udp->dict1->dict2->userdef->string, ==, "string"); > + g_assert_cmpstr(udp->dict1->dict2->string, ==, "string2"); > g_assert(udp->dict1->has_dict3 == false); > > - g_free(udp->dict1->dict2->userdef); > - g_free(udp->dict1->dict2); > - g_free(udp->dict1); > - g_free(udp); > + qapi_free_UserDefTwo(udp); > } > > static void test_visitor_in_list(TestInputVisitorData *data, > @@ -346,14 +334,12 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, > g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_I); > g_assert_cmpint(tmp->u.i, ==, 42); > qapi_free_UserDefAlternate(tmp); > - visitor_input_teardown(data, NULL); > > v = visitor_input_test_init(data, "'string'"); > visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort); > g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_S); > g_assert_cmpstr(tmp->u.s, ==, "string"); > qapi_free_UserDefAlternate(tmp); > - visitor_input_teardown(data, NULL); > > v = visitor_input_test_init(data, "false"); > visit_type_UserDefAlternate(v, &tmp, NULL, &err); > @@ -361,7 +347,6 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, > error_free(err); > err = NULL; > qapi_free_UserDefAlternate(tmp); > - visitor_input_teardown(data, NULL); > } > > static void test_visitor_in_alternate_number(TestInputVisitorData *data, > @@ -384,7 +369,6 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data, > error_free(err); > err = NULL; > qapi_free_AltStrBool(asb); > - visitor_input_teardown(data, NULL); > > /* FIXME: Order of alternate should not affect semantics; asn should > * parse the same as ans */ > @@ -396,35 +380,30 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data, > error_free(err); > err = NULL; > qapi_free_AltStrNum(asn); > - visitor_input_teardown(data, NULL); > > v = visitor_input_test_init(data, "42"); > visit_type_AltNumStr(v, &ans, NULL, &error_abort); > g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N); > g_assert_cmpfloat(ans->u.n, ==, 42); > qapi_free_AltNumStr(ans); > - visitor_input_teardown(data, NULL); > > v = visitor_input_test_init(data, "42"); > visit_type_AltStrInt(v, &asi, NULL, &error_abort); > g_assert_cmpint(asi->type, ==, ALT_STR_INT_KIND_I); > g_assert_cmpint(asi->u.i, ==, 42); > qapi_free_AltStrInt(asi); > - visitor_input_teardown(data, NULL); > > v = visitor_input_test_init(data, "42"); > visit_type_AltIntNum(v, &ain, NULL, &error_abort); > g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_I); > g_assert_cmpint(ain->u.i, ==, 42); > qapi_free_AltIntNum(ain); > - visitor_input_teardown(data, NULL); > > v = visitor_input_test_init(data, "42"); > visit_type_AltNumInt(v, &ani, NULL, &error_abort); > g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_I); > g_assert_cmpint(ani->u.i, ==, 42); > qapi_free_AltNumInt(ani); > - visitor_input_teardown(data, NULL); > > /* Parsing a double */ > > @@ -434,21 +413,18 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data, > error_free(err); > err = NULL; > qapi_free_AltStrBool(asb); > - visitor_input_teardown(data, NULL); > > v = visitor_input_test_init(data, "42.5"); > visit_type_AltStrNum(v, &asn, NULL, &error_abort); > g_assert_cmpint(asn->type, ==, ALT_STR_NUM_KIND_N); > g_assert_cmpfloat(asn->u.n, ==, 42.5); > qapi_free_AltStrNum(asn); > - visitor_input_teardown(data, NULL); > > v = visitor_input_test_init(data, "42.5"); > visit_type_AltNumStr(v, &ans, NULL, &error_abort); > g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N); > g_assert_cmpfloat(ans->u.n, ==, 42.5); > qapi_free_AltNumStr(ans); > - visitor_input_teardown(data, NULL); > > v = visitor_input_test_init(data, "42.5"); > visit_type_AltStrInt(v, &asi, NULL, &err); > @@ -456,21 +432,18 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data, > error_free(err); > err = NULL; > qapi_free_AltStrInt(asi); > - visitor_input_teardown(data, NULL); > > v = visitor_input_test_init(data, "42.5"); > visit_type_AltIntNum(v, &ain, NULL, &error_abort); > g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_N); > g_assert_cmpfloat(ain->u.n, ==, 42.5); > qapi_free_AltIntNum(ain); > - visitor_input_teardown(data, NULL); > > v = visitor_input_test_init(data, "42.5"); > visit_type_AltNumInt(v, &ani, NULL, &error_abort); > g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_N); > g_assert_cmpfloat(ani->u.n, ==, 42.5); > qapi_free_AltNumInt(ani); > - visitor_input_teardown(data, NULL); > } > > static void test_native_list_integer_helper(TestInputVisitorData *data, > diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c > index 9364843..8606111 100644 > --- a/tests/test-qmp-output-visitor.c > +++ b/tests/test-qmp-output-visitor.c > @@ -391,6 +391,7 @@ static void test_visitor_out_any(TestOutputVisitorData *data, > qobj = QOBJECT(qdict); > visit_type_any(data->ov, &qobj, NULL, &err); > g_assert(!err); > + qobject_decref(qobj); > obj = qmp_output_get_qobject(data->qov); > g_assert(obj != NULL); > qdict = qobject_to_qdict(obj); > @@ -411,7 +412,6 @@ static void test_visitor_out_any(TestOutputVisitorData *data, qobj = qdict_get(qdict, "string"); g_assert(qobj); qstring = qobject_to_qstring(qobj); > g_assert(qstring); > g_assert_cmpstr(qstring_get_str(qstring), ==, "foo"); > qobject_decref(obj); > - qobject_decref(qobj); Hmm... obj is an alias for qdict, qobj is a member of qdict, freeing obj frees qobj (unless there's another reference to qobj I can't see). The line you delete then is a use-after-free bug that underflows the reference counter. Correct? If yes, commit message should mention it briefly, because this isn't just a leak. Actually, I'd make it a separate commit, to keep commit messages simple, particularly the headlines. Aside: qobject_decref() neglects to assert(!obj || obj->refcnt > 0). > } > > static void test_visitor_out_union_flat(TestOutputVisitorData *data, > @@ -463,6 +463,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, > g_assert_cmpint(qint_get_int(qobject_to_qint(arg)), ==, 42); > > qapi_free_UserDefAlternate(tmp); > + qobject_decref(arg); > } > > static void test_visitor_out_empty(TestOutputVisitorData *data,
On 11/04/2015 01:19 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Make valgrind happy with the current state of the tests, so that >> it is easier to see if future patches introduce new memory problems >> without being drowned in noise. Many of the leaks were due to >> calling a second init without tearing down the data from an earlier >> visit. But since teardown is already idempotent, and we already >> register teardown as part of input_visitor_test_add(), it is nicer >> to just make init() safe to call multiple times than it is to have >> to make all tests call teardown. >> >> Another common leak was forgetting to clean up an error object, >> after testing that an error was raised. >> >> Another leak was in test_visitor_in_struct_nested(), failing to >> clean the base member of UserDefTwo. Cleaning that up left >> check_and_free_str() as dead code (since using the qapi_free_* >> takes care of recursion, and we don't want double frees). >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> --- >> v9: move earlier in series (was 13/17) >> v8: no change >> v7: no change >> v6: make init repeatable rather than adding teardown everywhere, >> fix additional leak with UserDefTwo base, plug additional files >> --- >> tests/test-qmp-input-strict.c | 10 ++++++++++ >> tests/test-qmp-input-visitor.c | 41 +++++++---------------------------------- >> tests/test-qmp-output-visitor.c | 3 ++- >> 3 files changed, 19 insertions(+), 35 deletions(-) > > No leaks to plug in test/-qmp-commands.c and test-qmp-event.c? Didn't check. I'll do that. > >> diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c >> index b44184f..910e2f9 100644 >> --- a/tests/test-qmp-input-strict.c >> +++ b/tests/test-qmp-input-strict.c >> @@ -77,6 +77,8 @@ static Visitor *validate_test_init_raw(TestInputVisitorData *data, >> { >> Visitor *v; >> >> + validate_teardown(data, NULL); >> + >> data->obj = qobject_from_json(json_string); >> g_assert(data->obj != NULL); >> > > A test added with validate_test_add() may call validate_test_init_raw() > any number of time. Since validate_test_add() passes > validate_teardown() as fixture teardown function, the last one will be > cleaned up on test finalization. The others will be cleaned up by the > next validate_test_init_raw(). Okay. Actually, the whole fixture > business starts to make sense only now. > > But why only validate_test_init_raw() and not validate_test_init()? > Umm, because I didn't look, and just plugged holes? Will fix. > The two duplicate code, by the way. And the fix will probably be by having one call the other. > >> @@ -193,6 +195,8 @@ static void test_validate_fail_struct(TestInputVisitorData *data, >> >> visit_type_TestStruct(v, &p, NULL, &err); >> g_assert(err); >> + error_free(err); >> + /* FIXME: visitor should not allocate p when returning error */ > > Indeed. > > Recommend to always mention new FIXMEs in the commit message. This fixme is part of the answer to your question about 6/27 - we do have test coverage on non-arrays. >> +++ b/tests/test-qmp-input-visitor.c >> @@ -46,6 +46,8 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data, >> Visitor *v; >> va_list ap; >> >> + visitor_input_teardown(data, NULL); >> + >> va_start(ap, json_string); >> data->obj = qobject_from_jsonv(json_string, &ap); >> va_end(ap); > > Here, you add it to visitor_input_test_init(), but not > visitor_input_test_init_raw(). > > These two duplicate code, too. Looks like I get to fix this too. >> +++ b/tests/test-qmp-output-visitor.c >> @@ -391,6 +391,7 @@ static void test_visitor_out_any(TestOutputVisitorData *data, >> qobj = QOBJECT(qdict); Here, qobj is an alias to qdict... >> visit_type_any(data->ov, &qobj, NULL, &err); >> g_assert(!err); >> + qobject_decref(qobj); >> obj = qmp_output_get_qobject(data->qov); >> g_assert(obj != NULL); >> qdict = qobject_to_qdict(obj); >> @@ -411,7 +412,6 @@ static void test_visitor_out_any(TestOutputVisitorData *data, > qobj = qdict_get(qdict, "string"); ...but then we are reassigning it to instead be an alias within qdict. > g_assert(qobj); > qstring = qobject_to_qstring(qobj); >> g_assert(qstring); >> g_assert_cmpstr(qstring_get_str(qstring), ==, "foo"); >> qobject_decref(obj); >> - qobject_decref(qobj); Dereferencing a subset of the qdict leaks the overal qdict. > > Hmm... obj is an alias for qdict, qobj is a member of qdict, freeing > obj frees qobj (unless there's another reference to qobj I can't see). > The line you delete then is a use-after-free bug that underflows the > reference counter. Correct? Valgrind complained about a leak, not a use-after-free. But there indeed may be more than one issue that got solved by correctly dropping the reference at the right point in time, prior to reassigning qobj for use as pointing to a different portion of the qdict. > > If yes, commit message should mention it briefly, because this isn't > just a leak. Actually, I'd make it a separate commit, to keep commit > messages simple, particularly the headlines. I'll have to refresh my memory what else is going on, but I can indeed split this out if it is different than a simple memleak fix. > > Aside: qobject_decref() neglects to assert(!obj || obj->refcnt > 0). Sounds like a separate patch.
Eric Blake <eblake@redhat.com> writes: > On 11/04/2015 01:19 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> Make valgrind happy with the current state of the tests, so that >>> it is easier to see if future patches introduce new memory problems >>> without being drowned in noise. Many of the leaks were due to >>> calling a second init without tearing down the data from an earlier >>> visit. But since teardown is already idempotent, and we already >>> register teardown as part of input_visitor_test_add(), it is nicer >>> to just make init() safe to call multiple times than it is to have >>> to make all tests call teardown. >>> >>> Another common leak was forgetting to clean up an error object, >>> after testing that an error was raised. >>> >>> Another leak was in test_visitor_in_struct_nested(), failing to >>> clean the base member of UserDefTwo. Cleaning that up left >>> check_and_free_str() as dead code (since using the qapi_free_* >>> takes care of recursion, and we don't want double frees). >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> >>> --- >>> v9: move earlier in series (was 13/17) >>> v8: no change >>> v7: no change >>> v6: make init repeatable rather than adding teardown everywhere, >>> fix additional leak with UserDefTwo base, plug additional files >>> --- >>> tests/test-qmp-input-strict.c | 10 ++++++++++ >>> tests/test-qmp-input-visitor.c | 41 >>> +++++++---------------------------------- >>> tests/test-qmp-output-visitor.c | 3 ++- >>> 3 files changed, 19 insertions(+), 35 deletions(-) >> >> No leaks to plug in test/-qmp-commands.c and test-qmp-event.c? > > Didn't check. I'll do that. > >> >>> diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c >>> index b44184f..910e2f9 100644 >>> --- a/tests/test-qmp-input-strict.c >>> +++ b/tests/test-qmp-input-strict.c >>> @@ -77,6 +77,8 @@ static Visitor *validate_test_init_raw(TestInputVisitorData *data, >>> { >>> Visitor *v; >>> >>> + validate_teardown(data, NULL); >>> + >>> data->obj = qobject_from_json(json_string); >>> g_assert(data->obj != NULL); >>> >> >> A test added with validate_test_add() may call validate_test_init_raw() >> any number of time. Since validate_test_add() passes >> validate_teardown() as fixture teardown function, the last one will be >> cleaned up on test finalization. The others will be cleaned up by the >> next validate_test_init_raw(). Okay. Actually, the whole fixture >> business starts to make sense only now. >> >> But why only validate_test_init_raw() and not validate_test_init()? >> > > Umm, because I didn't look, and just plugged holes? Will fix. > >> The two duplicate code, by the way. > > And the fix will probably be by having one call the other. > >> >>> @@ -193,6 +195,8 @@ static void >>> test_validate_fail_struct(TestInputVisitorData *data, >>> >>> visit_type_TestStruct(v, &p, NULL, &err); >>> g_assert(err); >>> + error_free(err); >>> + /* FIXME: visitor should not allocate p when returning error */ >> >> Indeed. >> >> Recommend to always mention new FIXMEs in the commit message. > > This fixme is part of the answer to your question about 6/27 - we do > have test coverage on non-arrays. What we (royal we) don't have is memory going back from 06/27 to 03/27 %-} >>> +++ b/tests/test-qmp-input-visitor.c >>> @@ -46,6 +46,8 @@ Visitor >>> *visitor_input_test_init(TestInputVisitorData *data, >>> Visitor *v; >>> va_list ap; >>> >>> + visitor_input_teardown(data, NULL); >>> + >>> va_start(ap, json_string); >>> data->obj = qobject_from_jsonv(json_string, &ap); >>> va_end(ap); >> >> Here, you add it to visitor_input_test_init(), but not >> visitor_input_test_init_raw(). >> >> These two duplicate code, too. > > Looks like I get to fix this too. > > >>> +++ b/tests/test-qmp-output-visitor.c >>> @@ -391,6 +391,7 @@ static void >>> test_visitor_out_any(TestOutputVisitorData *data, >>> qobj = QOBJECT(qdict); > > Here, qobj is an alias to qdict... > >>> visit_type_any(data->ov, &qobj, NULL, &err); >>> g_assert(!err); >>> + qobject_decref(qobj); >>> obj = qmp_output_get_qobject(data->qov); >>> g_assert(obj != NULL); >>> qdict = qobject_to_qdict(obj); >>> @@ -411,7 +412,6 @@ static void >>> test_visitor_out_any(TestOutputVisitorData *data, >> qobj = qdict_get(qdict, "string"); > > ...but then we are reassigning it to instead be an alias within qdict. > >> g_assert(qobj); >> qstring = qobject_to_qstring(qobj); >>> g_assert(qstring); >>> g_assert_cmpstr(qstring_get_str(qstring), ==, "foo"); >>> qobject_decref(obj); >>> - qobject_decref(qobj); > > Dereferencing a subset of the qdict leaks the overal qdict. > >> >> Hmm... obj is an alias for qdict, qobj is a member of qdict, freeing >> obj frees qobj (unless there's another reference to qobj I can't see). >> The line you delete then is a use-after-free bug that underflows the >> reference counter. Correct? > > Valgrind complained about a leak, not a use-after-free. But there indeed > may be more than one issue that got solved by correctly dropping the > reference at the right point in time, prior to reassigning qobj for use > as pointing to a different portion of the qdict. > >> >> If yes, commit message should mention it briefly, because this isn't >> just a leak. Actually, I'd make it a separate commit, to keep commit >> messages simple, particularly the headlines. > > I'll have to refresh my memory what else is going on, but I can indeed > split this out if it is different than a simple memleak fix. I really, really like bug fixes coming with an impact assessment. However, since this is just *tests*, I recommend *not* to spend time on figuring out what exactly was broken, and simply adjust the commit messages claims a bit. >> Aside: qobject_decref() neglects to assert(!obj || obj->refcnt > 0). > > Sounds like a separate patch. Definitely.
On 11/04/2015 10:44 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 11/04/2015 01:19 AM, Markus Armbruster wrote: >>> Eric Blake <eblake@redhat.com> writes: >>> >>>> Make valgrind happy with the current state of the tests, so that >>>> tests/test-qmp-input-strict.c | 10 ++++++++++ >>>> tests/test-qmp-input-visitor.c | 41 >>>> +++++++---------------------------------- >>>> tests/test-qmp-output-visitor.c | 3 ++- >>>> 3 files changed, 19 insertions(+), 35 deletions(-) >>> >>> No leaks to plug in test/-qmp-commands.c and test-qmp-event.c? >> >> Didn't check. I'll do that. Checked; no leaks to fix. But they do include some 'err' fixes for 4/27 to add.
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c index b44184f..910e2f9 100644 --- a/tests/test-qmp-input-strict.c +++ b/tests/test-qmp-input-strict.c @@ -77,6 +77,8 @@ static Visitor *validate_test_init_raw(TestInputVisitorData *data, { Visitor *v; + validate_teardown(data, NULL); + data->obj = qobject_from_json(json_string); g_assert(data->obj != NULL); @@ -193,6 +195,8 @@ static void test_validate_fail_struct(TestInputVisitorData *data, visit_type_TestStruct(v, &p, NULL, &err); g_assert(err); + error_free(err); + /* FIXME: visitor should not allocate p when returning error */ if (p) { g_free(p->string); } @@ -210,6 +214,7 @@ static void test_validate_fail_struct_nested(TestInputVisitorData *data, visit_type_UserDefTwo(v, &udp, NULL, &err); g_assert(err); + error_free(err); qapi_free_UserDefTwo(udp); } @@ -224,6 +229,7 @@ static void test_validate_fail_list(TestInputVisitorData *data, visit_type_UserDefOneList(v, &head, NULL, &err); g_assert(err); + error_free(err); qapi_free_UserDefOneList(head); } @@ -239,6 +245,7 @@ static void test_validate_fail_union_native_list(TestInputVisitorData *data, visit_type_UserDefNativeListUnion(v, &tmp, NULL, &err); g_assert(err); + error_free(err); qapi_free_UserDefNativeListUnion(tmp); } @@ -253,6 +260,7 @@ static void test_validate_fail_union_flat(TestInputVisitorData *data, visit_type_UserDefFlatUnion(v, &tmp, NULL, &err); g_assert(err); + error_free(err); qapi_free_UserDefFlatUnion(tmp); } @@ -268,6 +276,7 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data, visit_type_UserDefFlatUnion2(v, &tmp, NULL, &err); g_assert(err); + error_free(err); qapi_free_UserDefFlatUnion2(tmp); } @@ -282,6 +291,7 @@ static void test_validate_fail_alternate(TestInputVisitorData *data, visit_type_UserDefAlternate(v, &tmp, NULL, &err); g_assert(err); + error_free(err); qapi_free_UserDefAlternate(tmp); } diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index 3f6bc4d..03c3682 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -46,6 +46,8 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data, Visitor *v; va_list ap; + visitor_input_teardown(data, NULL); + va_start(ap, json_string); data->obj = qobject_from_jsonv(json_string, &ap); va_end(ap); @@ -177,12 +179,7 @@ static void test_visitor_in_enum(TestInputVisitorData *data, visit_type_EnumOne(v, &res, NULL, &err); g_assert(!err); g_assert_cmpint(i, ==, res); - - visitor_input_teardown(data, NULL); } - - data->obj = NULL; - data->qiv = NULL; } @@ -205,12 +202,6 @@ static void test_visitor_in_struct(TestInputVisitorData *data, g_free(p); } -static void check_and_free_str(char *str, const char *cmp) -{ - g_assert_cmpstr(str, ==, cmp); - g_free(str); -} - static void test_visitor_in_struct_nested(TestInputVisitorData *data, const void *unused) { @@ -226,17 +217,14 @@ static void test_visitor_in_struct_nested(TestInputVisitorData *data, visit_type_UserDefTwo(v, &udp, NULL, &err); g_assert(!err); - check_and_free_str(udp->string0, "string0"); - check_and_free_str(udp->dict1->string1, "string1"); + g_assert_cmpstr(udp->string0, ==, "string0"); + g_assert_cmpstr(udp->dict1->string1, ==, "string1"); g_assert_cmpint(udp->dict1->dict2->userdef->integer, ==, 42); - check_and_free_str(udp->dict1->dict2->userdef->string, "string"); - check_and_free_str(udp->dict1->dict2->string, "string2"); + g_assert_cmpstr(udp->dict1->dict2->userdef->string, ==, "string"); + g_assert_cmpstr(udp->dict1->dict2->string, ==, "string2"); g_assert(udp->dict1->has_dict3 == false); - g_free(udp->dict1->dict2->userdef); - g_free(udp->dict1->dict2); - g_free(udp->dict1); - g_free(udp); + qapi_free_UserDefTwo(udp); } static void test_visitor_in_list(TestInputVisitorData *data, @@ -346,14 +334,12 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_I); g_assert_cmpint(tmp->u.i, ==, 42); qapi_free_UserDefAlternate(tmp); - visitor_input_teardown(data, NULL); v = visitor_input_test_init(data, "'string'"); visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort); g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_S); g_assert_cmpstr(tmp->u.s, ==, "string"); qapi_free_UserDefAlternate(tmp); - visitor_input_teardown(data, NULL); v = visitor_input_test_init(data, "false"); visit_type_UserDefAlternate(v, &tmp, NULL, &err); @@ -361,7 +347,6 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, error_free(err); err = NULL; qapi_free_UserDefAlternate(tmp); - visitor_input_teardown(data, NULL); } static void test_visitor_in_alternate_number(TestInputVisitorData *data, @@ -384,7 +369,6 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data, error_free(err); err = NULL; qapi_free_AltStrBool(asb); - visitor_input_teardown(data, NULL); /* FIXME: Order of alternate should not affect semantics; asn should * parse the same as ans */ @@ -396,35 +380,30 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data, error_free(err); err = NULL; qapi_free_AltStrNum(asn); - visitor_input_teardown(data, NULL); v = visitor_input_test_init(data, "42"); visit_type_AltNumStr(v, &ans, NULL, &error_abort); g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N); g_assert_cmpfloat(ans->u.n, ==, 42); qapi_free_AltNumStr(ans); - visitor_input_teardown(data, NULL); v = visitor_input_test_init(data, "42"); visit_type_AltStrInt(v, &asi, NULL, &error_abort); g_assert_cmpint(asi->type, ==, ALT_STR_INT_KIND_I); g_assert_cmpint(asi->u.i, ==, 42); qapi_free_AltStrInt(asi); - visitor_input_teardown(data, NULL); v = visitor_input_test_init(data, "42"); visit_type_AltIntNum(v, &ain, NULL, &error_abort); g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_I); g_assert_cmpint(ain->u.i, ==, 42); qapi_free_AltIntNum(ain); - visitor_input_teardown(data, NULL); v = visitor_input_test_init(data, "42"); visit_type_AltNumInt(v, &ani, NULL, &error_abort); g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_I); g_assert_cmpint(ani->u.i, ==, 42); qapi_free_AltNumInt(ani); - visitor_input_teardown(data, NULL); /* Parsing a double */ @@ -434,21 +413,18 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data, error_free(err); err = NULL; qapi_free_AltStrBool(asb); - visitor_input_teardown(data, NULL); v = visitor_input_test_init(data, "42.5"); visit_type_AltStrNum(v, &asn, NULL, &error_abort); g_assert_cmpint(asn->type, ==, ALT_STR_NUM_KIND_N); g_assert_cmpfloat(asn->u.n, ==, 42.5); qapi_free_AltStrNum(asn); - visitor_input_teardown(data, NULL); v = visitor_input_test_init(data, "42.5"); visit_type_AltNumStr(v, &ans, NULL, &error_abort); g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N); g_assert_cmpfloat(ans->u.n, ==, 42.5); qapi_free_AltNumStr(ans); - visitor_input_teardown(data, NULL); v = visitor_input_test_init(data, "42.5"); visit_type_AltStrInt(v, &asi, NULL, &err); @@ -456,21 +432,18 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data, error_free(err); err = NULL; qapi_free_AltStrInt(asi); - visitor_input_teardown(data, NULL); v = visitor_input_test_init(data, "42.5"); visit_type_AltIntNum(v, &ain, NULL, &error_abort); g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_N); g_assert_cmpfloat(ain->u.n, ==, 42.5); qapi_free_AltIntNum(ain); - visitor_input_teardown(data, NULL); v = visitor_input_test_init(data, "42.5"); visit_type_AltNumInt(v, &ani, NULL, &error_abort); g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_N); g_assert_cmpfloat(ani->u.n, ==, 42.5); qapi_free_AltNumInt(ani); - visitor_input_teardown(data, NULL); } static void test_native_list_integer_helper(TestInputVisitorData *data, diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index 9364843..8606111 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -391,6 +391,7 @@ static void test_visitor_out_any(TestOutputVisitorData *data, qobj = QOBJECT(qdict); visit_type_any(data->ov, &qobj, NULL, &err); g_assert(!err); + qobject_decref(qobj); obj = qmp_output_get_qobject(data->qov); g_assert(obj != NULL); qdict = qobject_to_qdict(obj); @@ -411,7 +412,6 @@ static void test_visitor_out_any(TestOutputVisitorData *data, g_assert(qstring); g_assert_cmpstr(qstring_get_str(qstring), ==, "foo"); qobject_decref(obj); - qobject_decref(qobj); } static void test_visitor_out_union_flat(TestOutputVisitorData *data, @@ -463,6 +463,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, g_assert_cmpint(qint_get_int(qobject_to_qint(arg)), ==, 42); qapi_free_UserDefAlternate(tmp); + qobject_decref(arg); } static void test_visitor_out_empty(TestOutputVisitorData *data,
Make valgrind happy with the current state of the tests, so that it is easier to see if future patches introduce new memory problems without being drowned in noise. Many of the leaks were due to calling a second init without tearing down the data from an earlier visit. But since teardown is already idempotent, and we already register teardown as part of input_visitor_test_add(), it is nicer to just make init() safe to call multiple times than it is to have to make all tests call teardown. Another common leak was forgetting to clean up an error object, after testing that an error was raised. Another leak was in test_visitor_in_struct_nested(), failing to clean the base member of UserDefTwo. Cleaning that up left check_and_free_str() as dead code (since using the qapi_free_* takes care of recursion, and we don't want double frees). Signed-off-by: Eric Blake <eblake@redhat.com> --- v9: move earlier in series (was 13/17) v8: no change v7: no change v6: make init repeatable rather than adding teardown everywhere, fix additional leak with UserDefTwo base, plug additional files --- tests/test-qmp-input-strict.c | 10 ++++++++++ tests/test-qmp-input-visitor.c | 41 +++++++---------------------------------- tests/test-qmp-output-visitor.c | 3 ++- 3 files changed, 19 insertions(+), 35 deletions(-)