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

[Eric Blake]

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.

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

signature.asc
Description: OpenPGP digital signature