Re: [Qemu-devel] [PATCH v14 13/19] qmp: Tighten output visitor rules

[Eric Blake]

On 04/15/2016 03:02 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Add a new qmp_output_visitor_reset(), which must be called before
>> reusing an exising QmpOutputVisitor on a new root object.  Tighten
>> assertions to require that qmp_output_get_qobject() can only be
>> called after pairing a visit_end_* for every visit_start_* (rather
>> than allowing it to return a partially built object), and that it
>> must not be called unless at least one visit_type_* or visit_start/
>> visit_end pair has occurred since creation/reset (the accidental
>> return of NULL fixed by commit ab8bf1d7 would have been much
>> easier to diagnose).
>>
>> Also, check that we are encountering the expected object or list
>> type (both during visit_end*, and also by validating whether 'name'
>> was NULL - although the latter may need to change later if we
>> improve error messages by always passing in a sensible name).
>> This provides protection against mismatched push(struct)/pop(list)
>> or push(list)/pop(struct), similar to the qmp-input protection
>> added in commit bdd8e6b5.
>>
>> Signed-off-by: Eric Blake <address@hidden>
> 
> As written, the commit message makes me wonder why we add
> qmp_output_visitor_reset() in the same commit.  I think the reason is
> the tightened rules make it necessary.  The commit message could make
> that clearer by explaining the rule changes first, then point out we
> need a reset to comply with the rules.

I think I'll try splitting the addition of qmp_output_visitor_reset()
into a separate patch.

>> @@ -93,6 +92,9 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, 
>> const char *name,
>>              qdict_put_obj(qobject_to_qdict(cur), name, value);
>>              break;
>>          case QTYPE_QLIST:
>> +            /* FIXME: assertion needs adjustment if we fix visit-core
>> +             * to pass "name.0" style name during lists.  */
> 
> visit-core merely passes through whatever name it gets from the client.
> Thus, saying we 'fix visit-core to pass "name.0"' is a bit misleading.
> What we'd do is change it to require "name.0", then update its users to
> comply.

Maybe it's not too inaccurate - the only callers are the generated
visit_type_FOOList() functions, but having a common "name.%d" generator
in the core may be easier than bloating each generated visit_type_FOOList.

> 
> Moreover, this is a note, not a FIXME: nothing is broken here.  The
> closest we get to "broken" are the bad error messages, but they're
> elsewhere.
> 
> I'd simply drop the comment.

But this solution nicely sidesteps the "how will we fix error messages",
so I've dropped the comment.

> 
>> +            assert(!name);
> 
> PATCH 08 made this part of the contract.  It also added a bunch of
> contract-checking assertions.  Should this one be in PATCH 08, too?

Well, it's only weakly part of the contract unless (until?) we fix
callers/core to pass in "name.0", and then the assert would trigger.
However, checking the assertion in patch 8 is harder, without making the
core track whether it is currently in a list or struct visit (that is,
the only place where we know whether 'name' should be NULL or not is
where we've tracked a stack of our current visit_start_* calls; but the
core is not tracking a stack because that would be redundant with the
stacks in the qmp visitors).  So for now I think I'll just keep it here.


>> +++ b/tests/test-qmp-output-visitor.c
>> @@ -139,6 +139,7 @@ static void test_visitor_out_enum(TestOutputVisitorData 
>> *data,

>> @@ -455,6 +460,7 @@ static void 
>> test_visitor_out_alternate(TestOutputVisitorData *data,
>>      qapi_free_UserDefAlternate(tmp);
>>      qobject_decref(arg);
>>
>> +    qmp_output_visitor_reset(data->qov);
>>      tmp = g_new0(UserDefAlternate, 1);
>>      tmp->type = QTYPE_QDICT;
>>      tmp->u.udfu.integer = 1;
> 
> How did you find the places that now need qmp_output_visitor_reset()?

Ran the test, found what asserted, and added a reset() to make the test
pass again.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature