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

[Eric Blake]

On 07/30/2015 09:57 AM, Markus Armbruster wrote:
>> 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?

Done (see 12.5/47).

> 
> 
> [*] Leaving *obj alone on failure is nicer, but may not always be
> practical.

I'm wondering if visit_end_struct() should be changed to accept a bool
parameter that is true if the struct is ended normally, and false if an
error has been detected.  We may also need to alter visit_start_struct
to return a bool (true if an object was allocated), to help feed our
visitor logic on whether to pass true or false to the visit_end_struct().

We did something like that for visit_start_union()/visit_end_union(),
but I suspect those visitor interfaces are also a bit screwy.  Oh well,
I'm not going to try and tweak it today, but am happy with just adding
the FIXME.

Also, it would be really nice if we had docs in visitor.h and/or
visitor-impl.h, to get some sort of feel for how the visitor is supposed
to be used.

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

signature.asc
Description: OpenPGP digital signature