On 07/29/2015 11:22 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes: > >> On 07/29/2015 02:32 AM, Markus Armbruster wrote: >> >>>>> 2. We can leak retval only when qmp_FOO() returns non-null and local_err >>>>> is non-null. This must not happen, because: >>>>> >>>>> a. local_err must be null before the call, and >>>>> >>>>> b. the call must not return non-null when it sets local_err. >>>> >>>> We don't state that contract anywhere, but I doubt any of the qmp_FOO() >>>> functions violate it, so it is worth making it part of the contract. >>> >>> It's a general Error API rule: set an error exactly on failure. It >>> applies to any function returning errors through an Error **errp >>> parameter, and we generally don't bother to spell it out for the >>> individual functions. >>> >>> The part that needs to be spelling out is what success and failure mean. >>> A qmp_FOO() returning an object returns null on failure.
For qmp_FOO(), this is a reasonable contract. But our very own generated code does not follow these rules: visit_type_FOO() can assign into *obj even when setting an error, if it encounters a parse error halfway through the struct, leaving the caller responsible to still clean up the mess if it wants to avoid a memory leak. Maybe that means our generated code needs to be reworked to properly clean up on a failed parse, such that *obj is guaranteed to be NULL if an error is returned. As a separate patch, of course. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
