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

[Eric Blake]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature