[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v10 06/30] qapi: Simplify non-error testing in t
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v10 06/30] qapi: Simplify non-error testing in test-qmp-* |
Date: |
Fri, 6 Nov 2015 08:54:23 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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
[Qemu-devel] [PATCH v10 06/30] qapi: Simplify non-error testing in test-qmp-*, Eric Blake, 2015/11/06
[Qemu-devel] [PATCH v10 13/30] qapi: Track simple union tag in object.local_members, Eric Blake, 2015/11/06
[Qemu-devel] [PATCH v10 17/30] qapi: Simplify QAPISchemaObjectTypeMember.check(), Eric Blake, 2015/11/06
[Qemu-devel] [PATCH v10 25/30] qapi: Hoist tag collision check to Variants.check(), Eric Blake, 2015/11/06
[Qemu-devel] [PATCH v10 21/30] qapi: Factor out QAPISchemaObjectTypeMember.check_clash(), Eric Blake, 2015/11/06
[Qemu-devel] [PATCH v10 24/30] qapi: Factor out QAPISchemaObjectType.check_clash(), Eric Blake, 2015/11/06