[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [Qemu-devel] [PATCH v9 18/37] qapi: Drop unused error arg
From: |
Eric Blake |
Subject: |
Re: [Qemu-ppc] [Qemu-devel] [PATCH v9 18/37] qapi: Drop unused error argument for list and implicit struct |
Date: |
Wed, 20 Jan 2016 14:58:39 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 |
On 01/20/2016 12:03 PM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
>
>> No backend was setting an error when ending the visit of a list
>> or implicit struct.
>
> That's a lie: qmp_input_end_list() does. But it shouldn't, as you
> explain below. Rephrase the commit message?
I'm not sure why you call it a lie - qmp_input_end_list() will not set
an error unless it is mistakenly paired with a push(struct), which none
of our code base does. Or put another way, although qmp_input_pop()
[called by qmp_input_end_list()] has a signature that can set an error,
closer inspection shows that it will only do so when invoked to close
out a struct, and not when closing out a list. But that's a blatant
programmer mismatch, which none of our code base does, so no well-formed
use of visitors can cause qmp_input_end_list() to set an error.
>
>> Make the callers a bit easier to follow by
>> making this a part of the contract, and removing the errp
>> argument - callers can then unconditionally end an object as
>> part of cleanup without having to think about whether a second
>> error is dominated by a first, because there is no second error.
>>
>> The only addition of &error_abort in this patch, in the function
>> qmp_input_end_list(), will never trigger unless a programming
>> bug creates a push(struct)/pop(list) or push(list)/pop(struct)
>> mismatch.
I'm open to wording suggestions.
Maybe replace all of the above with:
None of the existing .end_implicit_struct() implementations use errp.
And of the existing .end_list() implementations, only
qmp_input_end_list() even uses errp, but closer inspection shows that it
will never be modified (errp is only passed to qmp_input_pop(), which
will only set an error if the corresponding push was a struct rather
than a list). We can turn that internal usage into an &error_abort, to
protect against programmer mistakes of push(struct)/pop(list) or
push(list)/pop(struct) mismatch.
With that done, we can then make all public uses of
visit_end_implicit_struct() and visit_end_list() easier to follow by
removing the errp argument and making error-free operation part of the
contract. Callers can then unconditionally end an object as part of
cleanup without having to think about whether a second error is
dominated by a first, because there is no possibility of a second error.
>>
>> A later patch will then tackle the larger task of splitting
>> visit_end_struct(), which can indeed set an error (and that
>> cleanup will also have the side-effect of removing the use of
>> error_abort added here).
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> Reviewed-by: Marc-André Lureau <address@hidden>
>
> Patch looks good. I like the simplification.
Would help to split this into two patches, one switching from
qmp_input_pop(errp) into qmp_input_pop(&error_abort), and the other then
removing unused errp argument?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature