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

[Eric Blake]

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? 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!

Agreed.  Looks like my v4 series will have another up-front patch to
clean things up.

> Either it takes over all references (just like on success) or none.

Taking over references is probably more convenient (we usually succeed,
where it really is easier to just free the new owner recursively, so
always reducing a reference even on failure is something that can easily
be reasoned about - and if the caller wants to hang on, they can
increase refcount).

> 
> 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.

I'm leaning towards "all".  And of course, the very first thing is a
patch to the testsuite with two %p separated by something that causes a
mid-string parse error (but at the same time does not trip up -Wformat).

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

signature.asc
Description: OpenPGP digital signature