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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

Worth mentioning in the commit message?  (I'm not sure)

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

Should've gotten them all in tests, because tests have trivial control
flow.  There's a risk of missing resets in QEMU proper.  I figure it's
small.  Generated visits shouldn't reuse visitor objects, thus should be
fine.  Manual visits need review: find the spots that create visitor
objects, trace the flow to their death, and convince yourself there's no
reuse.