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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 11/04/2015 01:40 AM, Markus Armbruster wrote:
>
>> 
>>> 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.
>
> Will split.
>
>>> @@ -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");
>
> And this part wipes out data, so that data->err is once again NULL.
>
>>> -    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.
>
> No, you missed that this patch is relying on the magic in 3/27 that
> wipes out data on every new test.

You're right.

>> 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.
>
> The point was that by moving err _into_ data, then the resetting of err
> becomes as simple as resetting data, rather than having to be verbose at
> every use of data->err.  We just need a visitor_input_test_init() in
> between.

Reducing boilerplate is good.  However, I'm afraid hiding
error_free(err); err = NULL; like this sets an example that is easily
copied incorrectly.

Here's what error.h has to say about consuming an Error object:

 * Report an error to stderr:
 *     error_report_err(err);
 * This frees the error object.
 *
 * Report an error somewhere else:
 *     const char *msg = error_get_pretty(err);
 *     do with msg what needs to be done...
 *     error_free(err);
 *
 * Handle an error without reporting it (just for completeness):
 *     error_free(err);

Perhaps we want something like

 * Expect an error, abort() if there is none:
 *     error_free_or_abort(&err);
 * This frees the error object and clears err.  Convenient for tests.

Then the patch quoted above becomes

    @@ -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;
    +    error_free_or_abort(&err);
         qapi_free_AltStrBool(asb);

         /* 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);
         /* 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;
    +    error_free_or_abort(&err);
         qapi_free_AltStrNum(asn);

         v = visitor_input_test_init(data, "42");

Similar boilerplate reduction, but keeps the clearing of err more
visible.