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/31/2017 07:34 AM, Eric Blake wrote:
>> On 07/31/2017 03:16 AM, Markus Armbruster wrote:
>> 
>>>>>> qtest_qmp(s, "%p", QOBJECT(dict)) takes ownership of dict?
>>>>>
>> 
>>>> 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?
>
> Hmm. What if we assert that qobject_from_jsonf() can never fail halfway
> through?  Given my research on the other thread, gcc -Wformat will NOT
> flag "['%s', %p]",str,obj as a mismatch, although our current code will
> try to associate %p with str and probably die horribly when
> dereferencing char* as QObject* (and if it does NOT die, we don't even
> know that 'obj' was passed as a parameter).  Since the primary usage of
> qobject_from_jsonf() is the testsuite, an assertion failure (forcing all
> clients to use the interface correctly) is probably simpler than even
> trying to have to worry about cleanup after partial failure (regardless
> of whether we like the 'none' or 'all' approach).

I don't buy the "primarily for the testsuite argument".  It's for
whatever finds it useful.  Safer than formatting the JSON text (no JSON
injection accidents), much easier to read than building a QObject to
pass to the JSON output visitor.

I'd be willing to buy a "passing syntactically incorrect JSON to
qobject_from_jsonf() is a programming error" argument.  Assertion
failure is the appropriate way to deal with programming errors.  Needs
to be spelled out in function contracts, of course, starting with
json_parser_parse_err(), which (scandalously!) has none.