Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2266 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]