Re: Missing qapi_free_Type in error case for qapi generated code?

[Eric Blake]

On 7/28/20 10:26 AM, Christophe de Dinechin wrote:
> The qapi generated code for qmp_marshal_query_spice seems to be missing a
> resource deallocation for "retval". For example, for SpiceInfo:

>      retval = qmp_query_spice(&err);
>      error_propagate(errp, err);
>      if (err) {
> /* retval not freed here */

Because it should be NULL here.  Returning an error AND an object is 
frowned on.

> /* Missing: qapi_free_SpiceInfo(retval); */
>          goto out;
>      }
>
>      qmp_marshal_output_SpiceInfo(retval, ret, errp);

And here, retval was non-NULL, but is cleaned as a side-effect of 
qmp_marshal_output_SpiceInfo.

> out:

So no matter how you get to the label, retval is no longer valid memory 
that can be leaked.

>      visit_free(v);
>      v = qapi_dealloc_visitor_new();
>      visit_start_struct(v, NULL, NULL, 0, NULL);
>      visit_end_struct(v, NULL);
>      visit_free(v);
> }
> #endif /* defined(CONFIG_SPICE) */
>
> Questions:
>
> - Is the query code supposed to always return NULL in case of error?

Yes.  If not, that is a bug in qmp_query_spice.

> In the
>    case of hmp_info_spice, there is no check for info==NULL, so on the
>    contrary, it seems to indicate that a non-null result is always expected,
>    and that function does call qapi_free_SpiceInfo

Calling qapi_free_SpiceInfo(NULL) is a safe no-op.  Or if you expect the 
function to always succeed, you could pass &error_abort as the errp 
parameter.

> - If not, is there an existing shortcut to generate the correct deallocation
>    code for return types that need it? You can't just use
>    qapi_free_%(c_type)s because that would generate an extra * character,
>    i.e. I get "SpiceInfo *" and not "SpiceInfo".

Ah, you're debating about editing scripts/qapi/commands.py.  If 
anything, an edit to add 'assert(!retval)' if qmp_COMMAND failed might 
be smarter than trying to add code to free retval.

> - If not, is there any good way to know if the type is a pointer type?
>    (A quick look in cripts/qapi/types.py does not show anything obvious)

Look at scripts/qapi/schema.py; each QAPI metatype has implementations 
of .c_name and .c_type that determine how to represent that QAPI object 
in C.  You probably want c_name instead of c_type when constructing the 
name of a qapi_free_FOO function, but that goes back to my question of 
whether such a call is even needed.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org