[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: |
Wed, 29 Jul 2015 19:22:11 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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.
>>
>>>> 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
>>
>> We use the same assumption all over the place, without asserting it.
>
> Okay, you've got a point there.
>
>>
>> Let's drop the useless initializer. As explained above, I don't like
>> the assertion for reasons explained above, but if you want it, I'm
>> willing to take it anyway, in a separate follow-up patch.
>>
>> I'd prefer to drop the initializer myself myself (with you credited in
>> the commit message), because it's certainly less total work, and quite
>> possibly less work for just for me.
>
> No need to add assertions. Maybe worth a patch to add a comment
> somewhere, maybe as a new section in docs/qapi-code-gen.txt, documenting
> how to write a handler and hook it into qmp-commands.hx, and what
> conditions the handler must obey. And I'm fine with you dropping the
> initializer as part of your v3 series.
Sounds like a plan. Thanks!
- Re: [Qemu-devel] [PATCH RFC v2 08/47] qapi-visit: Fix generated code when schema has forward refs, (continued)
- [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, 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 <=
- 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
[Qemu-devel] [PATCH RFC v2 31/47] qapi-event: Eliminate global variable event_enum_value, Markus Armbruster, 2015/07/01