On 04/15/2016 03:02 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com> > > 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