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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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.

We need to keep the introspection series reasonably focused.  I can't
afford a detour into visitor semantics right now, so we'll go with your
FIXMEs.

> 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.

Indeed.  We got a grand total of three one-liner comments, and one of
them is inaccurate: actual visitors don't obey /* Must be set */.