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