From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47027) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zuipy-0002Xf-7B for qemu-devel@nongnu.org; Fri, 06 Nov 2015 10:21:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zuipu-00069a-8X for qemu-devel@nongnu.org; Fri, 06 Nov 2015 10:21:58 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39554) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zuipu-00069W-2z for qemu-devel@nongnu.org; Fri, 06 Nov 2015 10:21:54 -0500 From: Markus Armbruster References: <1446791754-23823-1-git-send-email-eblake@redhat.com> <1446791754-23823-6-git-send-email-eblake@redhat.com> Date: Fri, 06 Nov 2015 16:21:50 +0100 In-Reply-To: <1446791754-23823-6-git-send-email-eblake@redhat.com> (Eric Blake's message of "Thu, 5 Nov 2015 23:35:29 -0700") Message-ID: <87bnb717ch.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v10 05/30] qapi: Plug leaks in test-qmp-* List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake 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). > > A final leak was in test_visitor_out_any(), which was reassigning > the qobj local variable to a subset of the overall structure > needing freeing; it did not result in a use-after-free, but > was not cleaning up all the qdict. > > test-qmp-event and test-qmp-commands were already clean. > > Signed-off-by: Eric Blake > > --- > v10: improve commit message, split out refactor of test init so > that both init and init_raw benefit from change > 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 | 9 +++++++++ > tests/test-qmp-input-visitor.c | 41 +++++++---------------------------------- > tests/test-qmp-output-visitor.c | 3 ++- > 3 files changed, 18 insertions(+), 35 deletions(-) > > diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c > index 77151de..95cd8e1 100644 > --- a/tests/test-qmp-input-strict.c > +++ b/tests/test-qmp-input-strict.c > @@ -49,6 +49,8 @@ static Visitor *validate_test_init_internal(TestInputVisitorData *data, > { > Visitor *v; > > + validate_teardown(data, NULL); > + > data->obj = qobject_from_jsonv(json_string, ap); > g_assert(data->obj); > > @@ -191,6 +193,7 @@ static void test_validate_fail_struct(TestInputVisitorData *data, > > visit_type_TestStruct(v, &p, NULL, &err); > g_assert(err); > + error_free(err); v9 had /* FIXME: visitor should not allocate p when returning error */ here. Did you drop it intentionally? If you put it back, please mention it in the commit message. > if (p) { > g_free(p->string); > } [...]