[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: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null() |
Date: |
Thu, 30 Jul 2015 17:57:49 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 07/29/2015 11:22 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> 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.
- Re: [Qemu-devel] [PATCH RFC v2 06/47] qapi: Drop unused and useless parameters and variables, (continued)
- [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, 2015/07/28
- 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 <=
- 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
[Qemu-devel] [PATCH RFC v2 31/47] qapi-event: Eliminate global variable event_enum_value, Markus Armbruster, 2015/07/01
[Qemu-devel] [PATCH RFC v2 22/47] qapi: QAPISchema code generation helper methods, Markus Armbruster, 2015/07/01