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
