From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35208) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zf8wy-0005qK-Px for qemu-devel@nongnu.org; Thu, 24 Sep 2015 12:00:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zf8wv-0002Rx-CC for qemu-devel@nongnu.org; Thu, 24 Sep 2015 12:00:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49038) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zf8wu-0002RQ-Vv for qemu-devel@nongnu.org; Thu, 24 Sep 2015 12:00:45 -0400 References: <1442872682-6523-1-git-send-email-eblake@redhat.com> <1442872682-6523-6-git-send-email-eblake@redhat.com> <87a8sbga01.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56041E2A.1070509@redhat.com> Date: Thu, 24 Sep 2015 10:00:42 -0600 MIME-Version: 1.0 In-Reply-To: <87a8sbga01.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="p65DHp6qhkB59pUGSb5NtKH9O8jXwp5d0" Subject: Re: [Qemu-devel] [PATCH v5 05/46] qapi: Test use of 'number' within alternates List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre.lureau@redhat.com, DirtY.iCE.hu@gmail.com, qemu-devel@nongnu.org, ehabkost@redhat.com, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --p65DHp6qhkB59pUGSb5NtKH9O8jXwp5d0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/24/2015 08:36 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> 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. >> >> Signed-off-by: Eric Blake >> --- >> tests/qapi-schema/qapi-schema-test.json | 8 ++ >> tests/qapi-schema/qapi-schema-test.out | 24 ++++++ >> tests/test-qmp-input-visitor.c | 129 +++++++++++++++++++++++= ++++++++- >> 3 files changed, 158 insertions(+), 3 deletions(-) >> >> +++ b/tests/test-qmp-input-visitor.c >> @@ -368,15 +368,136 @@ static void test_visitor_in_alternate(TestInput= VisitorData *data, >> { >> Visitor *v; >> Error *err =3D NULL; >> - UserDefAlternate *tmp; >> + UserDefAlternate *tmp =3D NULL; >=20 > Any particular reason for adding the initializer? >=20 >> >> v =3D visitor_input_test_init(data, "42"); >> >> - visit_type_UserDefAlternate(v, &tmp, NULL, &err); >> - g_assert(err =3D=3D NULL); >> + visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort); Hmm - I don't know if we have a clear contract for what happens if you call visit_type_FOO on an uninitialized pointer. It may have been succeeding by mere luck. >=20 > The pattern >=20 > foo(..., &err); > g_assert(err =3D=3D NULL); >=20 > is pretty common in tests. Can't see what it buys us over straight > &error_abort. Perhaps I'll spatch it away. >=20 >> g_assert_cmpint(tmp->kind, =3D=3D, USER_DEF_ALTERNATE_KIND_I); >> g_assert_cmpint(tmp->i, =3D=3D, 42); >> qapi_free_UserDefAlternate(tmp); >> + tmp =3D NULL; >=20 > Why do you need to clear tmp? If we were succeeding on a single call by mere luck where tmp started life as all 0 due to stack contents, but the second call has tmp pointing to stale memory, then that would be an obvious reason. I'll have to revisit what happens, because I don't recall any specific reason for why I did this other than the symmetry of making sure each parse had clean state (that is, I don't recall a crash happening if I didn't do it, and haven't yet tested under valgrind to see if we are provably using memory incorrectly if we don't initialize). >> + >> + /* FIXME: Integers should parse as numbers */ >=20 > Suggest to augment or replace this comment... >=20 >> + v =3D visitor_input_test_init(data, "42"); >> + visit_type_AltTwo(v, &two, NULL, &err); >=20 > ... with >=20 > /* FIXME g_assert_cmpint(two->kind, =3D=3D, ALT_TWO_KIND_N); */ > /* FIXME g_assert_cmpfloat(two->n, =3D=3D, 42); */ Ah, to better document what the test will look like in the future when the bugs are fixed. Sure, I can do that. >=20 >> + g_assert(err); >> + error_free(err); >> + err =3D NULL; >> + qapi_free_AltTwo(two); >> + one =3D NULL; >=20 > *chuckle* Why do you clear one here? More of the same below. Too much copy-and-paste. Will fix. >=20 >> + >> + /* FIXME: Order of alternate should not affect semantics */ >=20 > Inhowfar does it affect semantics? Or asked differently: what exactly > is wrong with this test now? >=20 >> + v =3D visitor_input_test_init(data, "42"); >> + visit_type_AltThree(v, &three, NULL, &error_abort); >> + g_assert_cmpint(three->kind, =3D=3D, ALT_THREE_KIND_N); >> + g_assert_cmpfloat(three->n, =3D=3D, 42); >> + qapi_free_AltThree(three); >> + one =3D NULL; AltTwo and AltThree are ostensibly the same struct (two branches, one for 'str' and one for 'number', just in a different order), but they parsed differently (AltTwo failed, AltThree succeeded). The bug is fixed later when the order of the branch declaration no longer impacts the result of the parse. >=20 > Reading this, I had to refer back to the definition of AltOne, ..., > AltSix all the time. Let's rename them to AltStrBool, AltStrNum, ..., > AltNumInt. Good idea, will do. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --p65DHp6qhkB59pUGSb5NtKH9O8jXwp5d0 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWBB4rAAoJEKeha0olJ0NqGRcH/R8gTf6FPLa9AlTjttFiZLe2 +iFnEJOOkabvxmcTHnQJSSzhsMjfK9zife5GOkSwpptNlDmYTw87e2NgBbC5y4fB WO8WIzg3OLsWcBHqSkbXZpHIcF4WCYS6Uan8mXnEvEPJkmlWXfBiyK5jTrUJHJ5O MiuIq0dW8tnUJ/QTGD03b/JZAtweyGmXcRMcNi10eG+QKHGALgq/bzESAFV8XGZl q2TcpqDLi2XtLhQO43yPnS+OCo4yZ4+XYIrEpAXmhwhovLHLjqmCd8oAVzmruC6Y fPilgH6pHETSjde5KvG4ZEKxFcIeTdXa7VKa9js+MLdWafPigShbx7NIpiPCBos= =+U4W -----END PGP SIGNATURE----- --p65DHp6qhkB59pUGSb5NtKH9O8jXwp5d0--