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

[Christophe de Dinechin]

On 2020-07-29 at 10:34 CEST, Markus Armbruster wrote...
> Eric Blake <eblake@redhat.com> writes:
>
>> 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.
>
> It's forbidden, actually.  The QMP handler must either succeed and
> return a value, or fail cleanly.

OK. Then I guess Eric's suggestion to add an assert is the correct
approach, with the caveat you identified.

>
> Since it has to return a value even when it fails, it returns an error
> value then.  "Cleanly" means the error value does not require cleanup.
>
> The generated marshalling function relies on this: it *ignores* the
> error value.
>
>>> /* 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.
>
> Correct.
>
>>> In the
>>>    case of hmp_info_spice, there is no check for info==NULL, so on the
>
> I'm blind.  Where?

In hmp_info_spice, there is this code:

    info = qmp_query_spice(NULL);

    if (!info->enabled) {
        monitor_printf(mon, "Server: disabled\n");
        goto out;
    }

I guess this code relies on qmp_query_spice never returning an error.
Why is that a safe assumption?

This came to my attention because I wanted to return an error and a NULL
value for modular spice if the module is not available.

>
>>>    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.
>
> This is more complicated than it may seem.
>
> The "natural" error value for a pointer-valued function is NULL.  I'm
> confident the handlers use it.  assert(!retval) should work.
>
> For functions returning something else, people may have different ideas
> on what to return on error.  To make assert(!retval) work, they need to
> return something "falsish".  I'm not ready to bet my own money on all of
> them doing that.

That's what I was thinking too. Glad to confirm I was not reading that code
too wrong.

>
> Aside: only functions in pragma returns-whitelist can return
> non-pointer.
>
>>> - 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)
>
> No clean way exists, simply because there has been no need.  So far,
> we've always found a reasonable way to generate code that works whether
> types are pointers in C or not.
>
>> 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.
>
> Method c_name() returns a string you can interpolate into C identifiers.
>
> Method c_type() returns a string you can use as C type in generated
> code.

That part I understood. I was looking for something like method c_basetype()
or something like that for the non-pointer type of a pointer.

>
> The QAPI scripts could use more comments.

+1.

--
Cheers,
Christophe de Dinechin (IRC c3d)