On 01/20/2016 12:03 PM, Markus Armbruster wrote: > Eric Blake <ebl...@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 <ebl...@redhat.com> >> Reviewed-by: Marc-André Lureau <marcandre.lur...@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
signature.asc
Description: OpenPGP digital signature