Re: [Qemu-devel] [PATCH v4 24/28] qapi: Add JSON output visitor

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 06/03/2016 01:39 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> We have several places that want to go from qapi to JSON; right now,
>>> they have to create an intermediate QObject to do the work.  That
>>> also has the drawback that the JSON formatting of a QDict will
>>> rearrange keys (according to a deterministic, but unpredictable,
>>> hash), when humans have an easier time if dicts are produced in
>>> the same order as the qapi type.
>>>
>
>>> +struct JsonOutputVisitor {
>>> +    Visitor visitor;
>>> +    QString *str;
>>> +    bool comma;
>>> +    unsigned int depth;
>>> +    char **result;
>>> +};
>>> +
>
>>> +static void json_output_complete(Visitor *v, void *result)
>>> +{
>>> +    JsonOutputVisitor *jov = to_jov(v);
>>> +
>>> +    assert(!jov->depth);
>>> +    assert(qstring_get_length(jov->str));
>>> +    assert(jov->result == result);
>>> +    *jov->result = qstring_consume_str(jov->str);
>>> +    jov->str = NULL;
>>> +    jov->result = NULL;
>>> +}
>> 
>> Related: discussion of complete() semantics in review of PATCH 12.
>> Non-idempotent semantics save us a copy, like in
>> string_output_complete().
>> 
>>> +
>>> +static void json_output_free(Visitor *v)
>>> +{
>>> +    JsonOutputVisitor *jov = to_jov(v);
>>> +
>>> +    QDECREF(jov->str);
>>> +    g_free(jov);
>> 
>> I'm afraid this leaks jov->result when the visitor is destroyed without
>> calling its complete() method.
>
> Not true. jov->result is owned by the caller, and not something we
> allocate locally.  We set jov->result to NULL to make sure complete() is
> not called twice, but we are not responsible for freeing it, since we
> didn't allocate it.
>
>> 
>> string_output_free() appears to have the same leak.
>
> Same non-bug.

You're right.