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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

Okay, it's not a lie if you consider the whole program.  It looks like a
lie locally.

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

Works for me.

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

If it results in more readable commit messages.