Eric Blake <ebl...@redhat.com> writes: > 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.
Assigning to *obj, then fail is tolerable[*]. Relying on the caller to free it is not. If we do that, it's a bug. > 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. Yes. Would you like to propose a FIXME for me to put into this series? [*] Leaving *obj alone on failure is nicer, but may not always be practical.
