From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59911) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZhXGq-0003od-FN for qemu-devel@nongnu.org; Thu, 01 Oct 2015 02:23:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZhXGn-0006Cj-6l for qemu-devel@nongnu.org; Thu, 01 Oct 2015 02:23:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43588) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZhXGm-0006Cf-W0 for qemu-devel@nongnu.org; Thu, 01 Oct 2015 02:23:09 -0400 From: Markus Armbruster References: <1443497249-15361-1-git-send-email-eblake@redhat.com> <1443497249-15361-9-git-send-email-eblake@redhat.com> <87k2r9ibw9.fsf@blackfin.pond.sub.org> <560AD353.5080802@redhat.com> Date: Thu, 01 Oct 2015 08:23:06 +0200 In-Reply-To: <560AD353.5080802@redhat.com> (Eric Blake's message of "Tue, 29 Sep 2015 12:07:15 -0600") Message-ID: <87si5vm7jp.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v6 08/16] qapi: Test use of 'number' within alternates List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Michael Roth , marcandre.lureau@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com Eric Blake writes: > On 09/29/2015 07:38 AM, Markus Armbruster wrote: >> Eric Blake writes: >> >>> Add some testsuite exposure for use of a 'number' as part of >>> an alternate. The current state of the tree has a few bugs >>> exposed by this: our input parser depends on the ordering of >>> how the qapi schema declared the alternate, and the parser >>> does not accept integers for a 'number' in an alternate even >>> though it does for numbers outside of an alternate. >>> >>> Mixing 'int' and 'number' in the same alternate is unusual, >>> since both are supplied by json-numbers, but there does not >>> seem to be a technical reason to forbid it given that our >>> json lexer distinguishes between json-numbers that can be >>> represented as an int vs. those that cannot. >>> >>> Improve the existing test_visitor_in_alternate() to match the >>> style of the new test_visitor_in_alternate_number(), and to >>> ensure full coverage of all possible qtype parsing. >>> >>> Signed-off-by: Eric Blake > >>> +++ b/tests/test-qmp-input-visitor.c >>> @@ -371,12 +371,133 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, >>> UserDefAlternate *tmp; >>> >>> v = visitor_input_test_init(data, "42"); >>> - >>> - visit_type_UserDefAlternate(v, &tmp, NULL, &err); >>> - g_assert(err == NULL); >>> + visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort); >>> g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I); >>> g_assert_cmpint(tmp->i, ==, 42); >>> qapi_free_UserDefAlternate(tmp); >>> + visitor_input_teardown(data, NULL); >> >> Ugly in this test: visitor_input_test_init() is to be paired with >> visitor_input_teardown(), but each test's last visitor_input_teardown() >> can be omitted, because we also pass visitor_input_teardown to >> g_test_add(). Not your patch's fault. Let's ignore it for now. > > v5 25/46 claims otherwise (valgrind was complaining about leaks that > additional calls to visitor_input_teardown() resolved; maybe the test > engine is not automatically doing the expected teardown?). And maybe it > would be smarter to rework the tests to allow reusing an existing > visitor on a new string, instead of completely allocating a new visitor > framework for every new string to parse. But indeed, any further > cleanups are better done in that later patch. Let me rephrase. visitor_input_test_init() creates what visitor_input_teardown() destroys. The two need to be paired. The pairing is done in an ugly way: the tests call visitor_input_teardown() for every visitor_input_test_init() *except* the last one. That one's teardown is set up by input_visitor_test_add(), which passes visitor_input_teardown() to g_test_add(). When a function calls visitor_input_test_init() just once, this ugliness isn't very visible. When it calls it multiple times, it's quite visible and actually confusing.