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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2342 bytes --]

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.

> 
> 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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]