Re: [Qemu-devel] [PATCH v9 04/27] qapi: Simplify error testing in test-qmp-*

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> By using &error_abort, we can avoid a local err variable in
> situations where we expect success.

This part is a no-brainer.

Bonus: before we abort(), we print

    Unexpected error in FUNC() at FILE:LINE:
    THE-ERROR-MESSAGE

to stdout on unexpected errors, which is a whole lot more useful than
what g_assert(!err) prints.

By the way: Coccinelle job :)

> By moving err into data, we can let test teardown take care
> of cleaning up any collected error; it also gives us fewer
> lines of code between repeated tests where init runs teardown
> on our behalf.

This part isn't as obvious.

Having two parts of differing obviousness indicates patch splitting
could make sense.  Especially when the parts are large and mechanical,
because reviewing large mechanical changes is much easier when there's
just one kind of it.

> Signed-off-by: Eric Blake <address@hidden>
[...]
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index 03c3682..d63c470 100644
[...]
> @@ -364,21 +341,17 @@ static void 
> test_visitor_in_alternate_number(TestInputVisitorData *data,
>      /* Parsing an int */
>
>      v = visitor_input_test_init(data, "42");
> -    visit_type_AltStrBool(v, &asb, NULL, &err);
> -    g_assert(err);
> -    error_free(err);
> -    err = NULL;
> +    visit_type_AltStrBool(v, &asb, NULL, &data->err);
> +    g_assert(data->err);
>      qapi_free_AltStrBool(asb);

This leaves data->err non-null.

>
>      /* FIXME: Order of alternate should not affect semantics; asn should
>       * parse the same as ans */
>      v = visitor_input_test_init(data, "42");
> -    visit_type_AltStrNum(v, &asn, NULL, &err);
> +    visit_type_AltStrNum(v, &asn, NULL, &data->err);

If visit_type_AltStrNum() errors out, its error will be thrown away by
its error_propagate(), because data->err is already non-null.

If it fails to error out, its error_propagate() does nothing, and we
won't detect the test failure.

Your patch makes forgetting to reset err an even easier mistake to make,
because it removes most of the error_free() that serve as reminders.

Is it a good idea anyway?  Let's discuss before I check the remainder of
this patch.

>      /* FIXME g_assert_cmpint(asn->type, == ALT_STR_NUM_KIND_N); */
>      /* FIXME g_assert_cmpfloat(asn->u.n, ==, 42); */
> -    g_assert(err);
> -    error_free(err);
> -    err = NULL;
> +    g_assert(data->err);
>      qapi_free_AltStrNum(asn);
>
>      v = visitor_input_test_init(data, "42");
[...]