[RFC,v2,29/47] qapi: Replace dirty is_c_ptr() by method c_null()

is_c_ptr() looks whether the end of the C text for the type looks like
a pointer.  Works, but is fragile.

We now have a better tool: use QAPISchemaType method c_null().  The
initializers for non-pointers become prettier: 0, false or the
enumeration constant with the value 0 instead of {0}.

One place looks suspicious: we initialize pointers, but not
non-pointers.  Either the initialization is superfluous and should be
deleted, or the non-pointers need it as well, or something subtle is
going on and needs a comment.  Since I lack the time to figure it out
now, mark it FIXME.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-commands.py | 19 +++++++------------
 scripts/qapi.py          |  3 ---
 2 files changed, 7 insertions(+), 15 deletions(-)

On 07/01/2015 02:22 PM, Markus Armbruster wrote:
> is_c_ptr() looks whether the end of the C text for the type looks like
> a pointer.  Works, but is fragile.
> 
> We now have a better tool: use QAPISchemaType method c_null().  The
> initializers for non-pointers become prettier: 0, false or the
> enumeration constant with the value 0 instead of {0}.
> 
> One place looks suspicious: we initialize pointers, but not
> non-pointers.  Either the initialization is superfluous and should be
> deleted, or the non-pointers need it as well, or something subtle is
> going on and needs a comment.  Since I lack the time to figure it out
> now, mark it FIXME.

Commenting on just this part for now (out of time to review this patch
proper for today):

> @@ -214,7 +208,8 @@ def gen_marshal_input(name, args, ret_type, middle_mode):
>                  header=hdr)
>  
>      if ret_type:
> -        if is_c_ptr(ret_type.c_type()):
> +        # FIXME fishy: only pointers are initialized
> +        if ret_type.c_null() == 'NULL':
>              retval = "    %s retval = NULL;" % ret_type.c_type()
>          else:
>              retval = "    %s retval;" % ret_type.c_type()

Here's an example function impacted by this:

static void qmp_marshal_input_guest_file_open(QDict *args, QObject
**ret, Error
**errp)
{
    Error *local_err = NULL;
    int64_t retval;
    QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
    QapiDeallocVisitor *md;
    Visitor *v;
    char *path = NULL;
    bool has_mode = false;
    char *mode = NULL;
...
    retval = qmp_guest_file_open(path, has_mode, mode, &local_err);
    if (local_err) {
        goto out;
    }

    qmp_marshal_output_guest_file_open(retval, ret, &local_err);

But compare that to any other function that returns a pointer, and
you'll see the same pattern (the only use of retval is in the final call
to qmp_marshal_output..., right after assigning retval); that is,
initializing to NULL is dead code, and you could get away with just
always declaring it instead of worrying about c_null() in this code.

Or maybe we have a leak - if the 'if (local_err)' can ever trigger even
when a function returned non-NULL, then our out: label is missing a
free(retval), but only when retval is a pointer.  Or maybe we change the
code to assert that retval is NULL if local_err is set after calling the
user's function, to prove we don't have a leak.

On 07/01/2015 02:22 PM, Markus Armbruster wrote:
> is_c_ptr() looks whether the end of the C text for the type looks like
> a pointer.  Works, but is fragile.
> 
> We now have a better tool: use QAPISchemaType method c_null().  The
> initializers for non-pointers become prettier: 0, false or the
> enumeration constant with the value 0 instead of {0}.
> 
> One place looks suspicious: we initialize pointers, but not
> non-pointers.  Either the initialization is superfluous and should be
> deleted, or the non-pointers need it as well, or something subtle is
> going on and needs a comment.  Since I lack the time to figure it out
> now, mark it FIXME.

I commented on that in another mail - either we have a leak on failure
(and need the initializer only for pointers), or we should add an assert
(and don't need the initializer at all), depending on what semantics we
want to enforce on all handler functions that set their error parameter.
 Probably worth cleaning that up in a separate pre-req patch.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-commands.py | 19 +++++++------------
>  scripts/qapi.py          |  3 ---
>  2 files changed, 7 insertions(+), 15 deletions(-)
> 

Nice reduction in size.  If the marshal code FIXME disappears due to a
separate cleanup, then the rest of this commit is good to go:
Reviewed-by: Eric Blake <eblake@redhat.com>

Eric Blake <eblake@redhat.com> writes:

> On 07/01/2015 02:22 PM, Markus Armbruster wrote:
>> is_c_ptr() looks whether the end of the C text for the type looks like
>> a pointer.  Works, but is fragile.
>> 
>> We now have a better tool: use QAPISchemaType method c_null().  The
>> initializers for non-pointers become prettier: 0, false or the
>> enumeration constant with the value 0 instead of {0}.
>> 
>> One place looks suspicious: we initialize pointers, but not
>> non-pointers.  Either the initialization is superfluous and should be
>> deleted, or the non-pointers need it as well, or something subtle is
>> going on and needs a comment.  Since I lack the time to figure it out
>> now, mark it FIXME.
>
> Commenting on just this part for now (out of time to review this patch
> proper for today):
>
>> @@ -214,7 +208,8 @@ def gen_marshal_input(name, args, ret_type, middle_mode):
>>                  header=hdr)
>>  
>>      if ret_type:
>> -        if is_c_ptr(ret_type.c_type()):
>> +        # FIXME fishy: only pointers are initialized
>> +        if ret_type.c_null() == 'NULL':
>>              retval = "    %s retval = NULL;" % ret_type.c_type()
>>          else:
>>              retval = "    %s retval;" % ret_type.c_type()
>
> Here's an example function impacted by this:
>
> static void qmp_marshal_input_guest_file_open(QDict *args, QObject
> **ret, Error
> **errp)
> {
>     Error *local_err = NULL;
>     int64_t retval;
>     QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
>     QapiDeallocVisitor *md;
>     Visitor *v;
>     char *path = NULL;
>     bool has_mode = false;
>     char *mode = NULL;
> ...
>     retval = qmp_guest_file_open(path, has_mode, mode, &local_err);
>     if (local_err) {
>         goto out;
>     }
>
>     qmp_marshal_output_guest_file_open(retval, ret, &local_err);
>
> But compare that to any other function that returns a pointer, and
> you'll see the same pattern (the only use of retval is in the final call
> to qmp_marshal_output..., right after assigning retval); that is,

Correct.  Inspection of qapi-commands.py shows retval is only ever read
in the call of qmp_marshal_output_FOO().

> initializing to NULL is dead code, and you could get away with just
> always declaring it instead of worrying about c_null() in this code.
>
> Or maybe we have a leak - if the 'if (local_err)' can ever trigger even
> when a function returned non-NULL, then our out: label is missing a
> free(retval), but only when retval is a pointer.  Or maybe we change the
> code to assert that retval is NULL if local_err is set after calling the
> user's function, to prove we don't have a leak.

Let me rephrase to make sure I understand.

Ignore the (not rets) case, because retval doesn't exist then.

qmp_marshal_output_FOO() visits retval twice.  First, with a QMP output
visitor to do the actual marshalling.  Second, with a QAPI dealloc
visitor to destroy it.

If we execute the assignment to retval, we must go on to call
qmp_marshal_output_FOO(), or else we have a leak.

If we can reach qmp_marshal_output_FOO() without executing the
assignment, we must initialize retval.  If we can't, any initialization
is unused.

gen_call() generates code of the form

        retval = qmp_FOO(... args ..., &local_err);
        if (local_err) {
            goto out;
        }

        qmp_marshal_output_FOO(retval, ret, &local_err);

Its caller then generates

    out:
        error_propagate(errp, local_err);

and so forth.

Observe:

1. The assignment dominates the only use.  Therefore, the initialization
   is unused.  Let's drop it in a separate cleanup patch.

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 could right after out: assert(!local_err || !retval).  Not sure
   it's worthwhile.

TL;DR: I concur with your analysis.

Eric Blake <eblake@redhat.com> writes:

> On 07/01/2015 02:22 PM, Markus Armbruster wrote:
>> is_c_ptr() looks whether the end of the C text for the type looks like
>> a pointer.  Works, but is fragile.
>> 
>> We now have a better tool: use QAPISchemaType method c_null().  The
>> initializers for non-pointers become prettier: 0, false or the
>> enumeration constant with the value 0 instead of {0}.
>> 
>> One place looks suspicious: we initialize pointers, but not
>> non-pointers.  Either the initialization is superfluous and should be
>> deleted, or the non-pointers need it as well, or something subtle is
>> going on and needs a comment.  Since I lack the time to figure it out
>> now, mark it FIXME.
>
> I commented on that in another mail - either we have a leak on failure
> (and need the initializer only for pointers), or we should add an assert
> (and don't need the initializer at all), depending on what semantics we
> want to enforce on all handler functions that set their error parameter.
>  Probably worth cleaning that up in a separate pre-req patch.

A function call should either fail completely or succeed completely.

On success, it must not set an error (d'oh!).

On failure, it must set an error (d'oh again!), and should not return
stuff for the caller to clean up.

The rules on setting errors together with the rule that a &local_err
argument must contain null before the call ensures that afterwards
local_err is set exactly when the call failed.

The rule on returning stuff lets us keep the error path simple: no need
for cleaning up anything returned by the function.  This is pervasive in
the code.  We generally don't bother to assert() the function actually
obeys the rule.

Perhaps I should fix up my pending patch "error: Revamp interface
documentation"[*] to cover the "should not return stuff for the caller
to clean up" part.

Apply to handler functions returning objects in need of clean up:

        retval = qmp_FOO(... args ..., &local_err);
        if (local_err) {
            goto out;
        }

        qmp_marshal_output_FOO(retval, ret, &local_err);

On success, qmp_FOO() must not touch local_err.

On failure, qmp_FOO() must set local_err, and return null.

Bypassing the cleanup qmp_marshal_output_FOO() is therfore perfectly
fine.

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi-commands.py | 19 +++++++------------
>>  scripts/qapi.py          |  3 ---
>>  2 files changed, 7 insertions(+), 15 deletions(-)
>> 
>
> Nice reduction in size.  If the marshal code FIXME disappears due to a
> separate cleanup, then the rest of this commit is good to go:
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!


[*] https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg04756.html

On 07/28/2015 01:34 AM, Markus Armbruster wrote:
> Let me rephrase to make sure I understand.
> 
> Ignore the (not rets) case, because retval doesn't exist then.
> 
> qmp_marshal_output_FOO() visits retval twice.  First, with a QMP output
> visitor to do the actual marshalling.  Second, with a QAPI dealloc
> visitor to destroy it.

And the use of the dealloc visitor is buried inside the
qmp_marshal_output_FOO() call.

> 
> If we execute the assignment to retval, we must go on to call
> qmp_marshal_output_FOO(), or else we have a leak.
> 
> If we can reach qmp_marshal_output_FOO() without executing the
> assignment, we must initialize retval.  If we can't, any initialization
> is unused.
> 
> gen_call() generates code of the form
> 
>         retval = qmp_FOO(... args ..., &local_err);
>         if (local_err) {
>             goto out;
>         }
> 
>         qmp_marshal_output_FOO(retval, ret, &local_err);
> 

> Observe:
> 
> 1. The assignment dominates the only use.  Therefore, the initialization
>    is unused.  Let's drop it in a separate cleanup patch.
> 
> 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.

> 
>    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
sure if after out: is the right place (then you'd need an initializer to
cover any other code that jumps to out), but this would do the same

retval = qmp_FOO(...);
if (local_err) {
    assert(!retval);
    goto out;
}
qmp_marshal_output_FOO(retval, ...);

> 
> TL;DR: I concur with your analysis.

Is it worth dropping the dead initializer and adding the assert in the
same pre-req cleanup patch?  Do you want me to submit it since I did the
analysis?

Eric Blake <eblake@redhat.com> writes:

> On 07/28/2015 01:34 AM, Markus Armbruster wrote:
>> Let me rephrase to make sure I understand.
>> 
>> Ignore the (not rets) case, because retval doesn't exist then.
>> 
>> qmp_marshal_output_FOO() visits retval twice.  First, with a QMP output
>> visitor to do the actual marshalling.  Second, with a QAPI dealloc
>> visitor to destroy it.
>
> And the use of the dealloc visitor is buried inside the
> qmp_marshal_output_FOO() call.
>
>> 
>> If we execute the assignment to retval, we must go on to call
>> qmp_marshal_output_FOO(), or else we have a leak.
>> 
>> If we can reach qmp_marshal_output_FOO() without executing the
>> assignment, we must initialize retval.  If we can't, any initialization
>> is unused.
>> 
>> gen_call() generates code of the form
>> 
>>         retval = qmp_FOO(... args ..., &local_err);
>>         if (local_err) {
>>             goto out;
>>         }
>> 
>>         qmp_marshal_output_FOO(retval, ret, &local_err);
>> 
>
>> Observe:
>> 
>> 1. The assignment dominates the only use.  Therefore, the initialization
>>    is unused.  Let's drop it in a separate cleanup patch.
>> 
>> 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.

The boilerplate around calls using the Error API is bad enough as it is.
I'm reluctant to make it worse by adding assertions.

Less of an issue in generated code, but violations are even less likely
there.

However, I'm reluctant to assert in some places, but not all, because
inconsistency breeds confusion.

In general, I prefer a function's pre- and post-conditions to be
asserted in the function itself, not in its callers.  Asserting selected
parts of the post-condition in callers can be occasionally helpful to
help human readers and static analyzers lacking insight into the
function.

> sure if after out: is the right place (then you'd need an initializer to
> cover any other code that jumps to out), but this would do the same
>
> retval = qmp_FOO(...);
> if (local_err) {
>     assert(!retval);
>     goto out;
> }
> qmp_marshal_output_FOO(retval, ...);

Not quite the same: it doesn't catch the case !local_err && !retval.
But it doesn't matter, because that case is bound to die in
qmp_marshal_output_FOO().

>> TL;DR: I concur with your analysis.
>
> Is it worth dropping the dead initializer and adding the assert in the
> same pre-req cleanup patch?  Do you want me to submit it since I did the
> analysis?

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.

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> writes:

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

Sounds like a plan.  Thanks!

On 07/29/2015 11:22 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> 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.

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.

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.

Eric Blake <eblake@redhat.com> writes:

> On 07/29/2015 11:22 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> 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.
>
> 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?


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

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