From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52744) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avIyu-0003aY-KL for qemu-devel@nongnu.org; Wed, 27 Apr 2016 02:29:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1avIyr-0007kk-9t for qemu-devel@nongnu.org; Wed, 27 Apr 2016 02:29:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39900) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avIyr-0007ka-3q for qemu-devel@nongnu.org; Wed, 27 Apr 2016 02:29:49 -0400 From: Markus Armbruster References: <1460131992-32278-1-git-send-email-eblake@redhat.com> <1460131992-32278-14-git-send-email-eblake@redhat.com> <877ffzutw8.fsf@dusky.pond.sub.org> <57201615.4040801@redhat.com> Date: Wed, 27 Apr 2016 08:29:45 +0200 In-Reply-To: <57201615.4040801@redhat.com> (Eric Blake's message of "Tue, 26 Apr 2016 19:29:57 -0600") Message-ID: <87zisfr2cm.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v14 13/19] qmp: Tighten output visitor rules List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake writes: > On 04/15/2016 03:02 AM, Markus Armbruster wrote: >> Eric Blake 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 >> >> 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.