Eric Blake <ebl...@redhat.com> 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 */.
