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

[Eric Blake]

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.

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

signature.asc
Description: OpenPGP digital signature