[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null() |
Date: |
Tue, 28 Jul 2015 08:53:08 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 |
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
- Re: [Qemu-devel] [PATCH RFC v2 18/47] qapi-commands: Don't feed output of mcgen() to mcgen() again, (continued)
- [Qemu-devel] [PATCH RFC v2 08/47] qapi-visit: Fix generated code when schema has forward refs, Markus Armbruster, 2015/07/01
- [Qemu-devel] [PATCH RFC v2 06/47] qapi: Drop unused and useless parameters and variables, Markus Armbruster, 2015/07/01
- [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null(), Markus Armbruster, 2015/07/01
- Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null(), Eric Blake, 2015/07/22
- Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null(), Markus Armbruster, 2015/07/28
- Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null(),
Eric Blake <=
- Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null(), Markus Armbruster, 2015/07/29
- Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null(), Eric Blake, 2015/07/29
- Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null(), Markus Armbruster, 2015/07/29
- Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null(), Eric Blake, 2015/07/30
- Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null(), Markus Armbruster, 2015/07/30
- Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null(), Eric Blake, 2015/07/30
- Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null(), Markus Armbruster, 2015/07/31
Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null(), Eric Blake, 2015/07/23
[Qemu-devel] [PATCH RFC v2 23/47] qapi: New QAPISchemaVisitor, Markus Armbruster, 2015/07/01