Re: [Qemu-devel] [PATCH v10 06/30] qapi: Simplify non-error testing in test-qmp-*

[Eric Blake]

On 11/06/2015 08:36 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> By using &error_abort, we can avoid a local err variable in
>> situations where we expect success.  It also has the nice
>> effect that if the test breaks, the error message from
>> error_abort tends to be nicer than that of g_assert().
>>
>> Signed-off-by: Eric Blake <address@hidden>
> [Boring mechanical changes snipped...]

>>      pt_copy->type = pt->type;
>> -    ops->serialize(pt, &serialize_data, visit_primitive_type, &err);
>> -    ops->deserialize((void **)&pt_copy, serialize_data, 
>> visit_primitive_type, &err);
>> +    ops->serialize(pt, &serialize_data, visit_primitive_type, &error_abort);
>> +    ops->deserialize((void **)&pt_copy, serialize_data, 
>> visit_primitive_type,
>> +                     &error_abort);
>>
>> -    g_assert(err == NULL);
> 
> This looks like a (very minor) bug fix / cleanup: you're not supposed to
> pass the same &err to multiple functions without checking and clearing
> it in between, because the second failure trips assert(*errp == NULL) in
> error_setv().  Harmless here, but it's nice to get rid of a bad example.

Harmless here because we are asserting that err is still NULL after the
chain (which means it was NULL at all points during the chain). But I
agree that it is nice to get rid of poor practice, and that adding a
paragraph to the commit message to point it out would be a nice idea.

> 
> Suggest to note the cleanup in the commit message.

We may be close enough to take the series without needing a v11; if
that's the case, and you are the one squashing in the change, how about
this text:

This patch has an additional bonus of fixing several call sites that
were passing &err to two different functions without checking it in
between.  In general that is unsafe practice; because if the first
function sets an error, the second function could abort() if it tries to
set a different error. We got away with it because we were asserting
that err was NULL through the entire chain, but switching to
&error_abort avoids the questionable practice up front.

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

signature.asc
Description: OpenPGP digital signature