Re: [Qemu-devel] [PATCH v3 07/12] qtest: Add a new helper qmp_cmd() and friends

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/28/2017 08:06 AM, Eric Blake wrote:
>> On 07/28/2017 07:57 AM, Stefan Hajnoczi wrote:
>>> On Tue, Jul 25, 2017 at 04:15:18PM -0500, Eric Blake wrote:
>>>> +QDict *qtest_qmp_cmd(QTestState *s, const char *cmd, QObject *args)
>>>> +{
>>>> +    QDict *dict = qdict_new();
>>>> +
>>>> +    qdict_put_str(dict, "execute", cmd);
>>>> +    if (args) {
>>>> +        qdict_put_obj(dict, "arguments", args);
>>>> +    }
>>>> +    return qtest_qmp(s, "%p", QOBJECT(dict));
>>>> +}
>>>
>>> qtest_qmp(s, "%p", QOBJECT(dict)) takes ownership of dict?
>> 
>> Hmm, I documented that for qtest_qmp_cmd(), but you have a good
>> question.  I need to double-check that it is true for
>> qobject_from_jsonf("%p", obj), but my recollection is that yes, wrapping
>> one object inside the other means freeing the outer object also reclaims
>> the inner (that is, qobject_from_jsonf wasn't increasing refcount).  At
>> any rate, I'll probably have to submit a followup patch (either to fix
>> the leak if I'm wrong, or to improve the documentation about %p
>> reference management if I'm right).
>
> $ valgrind --leak-check=full ./tests/check-qjson
> ...
> ==10596== LEAK SUMMARY:
> ==10596==    definitely lost: 0 bytes in 0 blocks
> ...
> $ grep -C5 %p tests/check-qjson.c
>             {}}));
>
>     embedded_obj = qobject_from_json("[32, 42]", &error_abort);
>     g_assert(embedded_obj != NULL);
>
>     obj = qobject_from_jsonf("[%d, 2, %p]", 1, embedded_obj);
>     g_assert(compare_litqobj_to_qobj(&decoded, obj) == 1);
>
>     qobject_decref(obj);
> }
>
> So given the clean bill of health from valgrind, we definitely DO turn
> over responsibility for freeing on object to its new wrapper, once it is
> passed through %p.  Documentation could be improved to make that clear.
>
> Eww, what happens if qobject_from_jsonf() can fail halfway through? If
> you mix %p with other stuff, how do you know on failure whether the
> reference was taken or not yet reached?  Maybe the testsuite needs an
> enhancement.

What we actually need is qobject_from_jsonf() behaving sanely failure!
Either it takes over all references (just like on success) or none.

It delegates the part of its job that's relevant here to json-parser.c.
Looks like parse_escape() takes over the reference greedily.  If parsing
fails, it's released along with all the refernces to objects the parser
built.

One way to do "none": delay taking over the reference until parsing
succeeded.  Increment it in parse_escape(), decrement it in
json_parser_parse_err().  Either hold %p arguments in JSONParserContext,
so json_parser_parse_err() can find them easily, or iterate over @tokens
and @ap again.

One way to do "all": on error, have json_parser_parse_err() iterate over
remaining @tokens and @ap to consume references.