Re: [Qemu-devel] [PATCH v9 18/37] qapi: Drop unused error argument for list and implicit struct

[Eric Blake <eblake@redhat.com>]

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

On 01/20/2016 12:03 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> 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


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