Re: [Qemu-devel] [PATCH v10 05/30] qapi: Plug leaks in test-qmp-*

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>
> ---
> 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);
>      }
[...]