Eric Blake <[email protected]> writes: > On 01/20/2016 12:03 PM, Markus Armbruster wrote: >> Eric Blake <[email protected]> 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 <[email protected]> >>> Reviewed-by: Marc-André Lureau <[email protected]> >> >> 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.
