[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH RFC v2 29/47] qapi: Replace dirty is_c_ptr() by method c_null()
Date: Wed, 22 Jul 2015 17:22:33 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0

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

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]