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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

Works for me.