On 07/28/2015 01:34 AM, Markus Armbruster wrote: > Let me rephrase to make sure I understand. > > Ignore the (not rets) case, because retval doesn't exist then. > > qmp_marshal_output_FOO() visits retval twice. First, with a QMP output > visitor to do the actual marshalling. Second, with a QAPI dealloc > visitor to destroy it.
And the use of the dealloc visitor is buried inside the
qmp_marshal_output_FOO() call.
>
> If we execute the assignment to retval, we must go on to call
> qmp_marshal_output_FOO(), or else we have a leak.
>
> If we can reach qmp_marshal_output_FOO() without executing the
> assignment, we must initialize retval. If we can't, any initialization
> is unused.
>
> gen_call() generates code of the form
>
> retval = qmp_FOO(... args ..., &local_err);
> if (local_err) {
> goto out;
> }
>
> qmp_marshal_output_FOO(retval, ret, &local_err);
>
> Observe:
>
> 1. The assignment dominates the only use. Therefore, the initialization
> is unused. Let's drop it in a separate cleanup patch.
>
> 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.
>
> We could right after out: assert(!local_err || !retval). Not sure
> it's worthwhile.
I think it IS worthwhile, because it would catch buggy callers. Not
sure if after out: is the right place (then you'd need an initializer to
cover any other code that jumps to out), but this would do the same
retval = qmp_FOO(...);
if (local_err) {
assert(!retval);
goto out;
}
qmp_marshal_output_FOO(retval, ...);
>
> TL;DR: I concur with your analysis.
Is it worth dropping the dead initializer and adding the assert in the
same pre-req cleanup patch? Do you want me to submit it since I did the
analysis?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
