From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56892) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c9VCI-0000jo-VI for qemu-devel@nongnu.org; Wed, 23 Nov 2016 05:54:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c9VCD-0007Gz-4V for qemu-devel@nongnu.org; Wed, 23 Nov 2016 05:54:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33526) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c9VCC-0007GP-Ra for qemu-devel@nongnu.org; Wed, 23 Nov 2016 05:54:33 -0500 References: <1479874588-1969-1-git-send-email-eblake@redhat.com> <1479874588-1969-4-git-send-email-eblake@redhat.com> <898038411.1379870.1479893131197.JavaMail.zimbra@redhat.com> From: Eric Blake Message-ID: Date: Wed, 23 Nov 2016 04:54:30 -0600 MIME-Version: 1.0 In-Reply-To: <898038411.1379870.1479893131197.JavaMail.zimbra@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="S07XdUTiE0pJf4jD93MC6trwb9aDWXD1p" Subject: Re: [Qemu-devel] [PATCH 3/3] qapi: Drop support for qobject_from_jsonf("%"PRId64) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, programmingkidx@gmail.com, armbru@redhat.com, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --S07XdUTiE0pJf4jD93MC6trwb9aDWXD1p From: Eric Blake To: Paolo Bonzini Cc: qemu-devel@nongnu.org, programmingkidx@gmail.com, armbru@redhat.com, Michael Roth Message-ID: Subject: Re: [PATCH 3/3] qapi: Drop support for qobject_from_jsonf("%"PRId64) References: <1479874588-1969-1-git-send-email-eblake@redhat.com> <1479874588-1969-4-git-send-email-eblake@redhat.com> <898038411.1379870.1479893131197.JavaMail.zimbra@redhat.com> In-Reply-To: <898038411.1379870.1479893131197.JavaMail.zimbra@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/23/2016 03:25 AM, Paolo Bonzini wrote: >> The qobject_from_jsonf() function implements a pseudo-printf >> language for creating a QObject; however, it is hard-coded to >> only parse a subset of formats understood by printf(). In >> particular, any use of a 64-bit integer works only if the >> system's definition of PRId64 matches what the parser expects; >> which works on glibc (%lld) and mingw (%I64d), but not on >> Mac OS (%qd). Rather than enhance the parser, we have already >> converted almost all clients to use an alternative method; >> convert or eliminate the remaining uses in the testsuite, and >> rip out this code from the parser. >> >> Ripping it all out means that we will now uniformly get >> failures on all platforms that try to use dynamic JSON with >> 64-bit numbers. Ultimately, I plan for later patches to rip >> out dynamic JSON altogether, but that is more invasive and >> therefore not appropriate for the 2.8 release, while this >> patch fixes an actual testsuite failure of check-qjson on >> Mac OS. >> >> Reported by: G 3 >> Signed-off-by: Eric Blake >=20 > This is throwing out the baby with the bathwater. %lld works > just fine for long long. Throwing away %I64d is fine though... On 64-bit systems where int64_t is 'long' rather than 'long long', PRId64 is %ld, not %lld. And on 32-bit MacOS, PRId64 is %qd, which is NOT covered by the existing JSON parser. If you write: int64_t foo; qobject_from_jsonf("%lld", foo) then gcc will complain on all platforms where %lld is not the right string to match against int64_t, thanks to -Wformat. We could instead write: int64_t foo; qobject_from_jsonf("%lld", (long long) foo); and have that work everywhere, but then you have to explicitly cast. And our current qdict_get_int() code returns int64_t, not long long, so it's hard to convince people to write: long long foo; foo =3D qdict_get_int(...); qobject_from_jsonf("%lld", foo); since 'long long' is more typing than 'int64_t'. I suppose we could do that, but my next question is why bother. As I've proven in these three patches, there were VERY FEW clients that were trying to use dynamic JSON on a 64-bit value in the first place. Ripping out ALL 64-bit support _proves_ that we can't mess it up in the future, vs. leaving %lld there and getting it right for some versions of glibc but still failing on other platforms when someone uses PRId64 instead of an explicit %lld. My other argument is that I _do_ intend to rip out ALL of the dynamic JSON support, at which point we no longer have %d, let along %lld. Until you see that followup series and decide whether it was too invasive for 2.9, it's hard to say that we are throwing out anything useful in this short-term fix for 2.8. So I guess that gives me a reason to hurry up and finish my work on that series to post it today before I take a long holiday weekend. >> +++ b/tests/test-qobject-input-visitor.c >> @@ -83,10 +83,11 @@ static Visitor >> *visitor_input_test_init_raw(TestInputVisitorData *data, >> static void test_visitor_in_int(TestInputVisitorData *data, >> const void *unused) >> { >> - int64_t res =3D 0, value =3D -42; >> + int64_t res =3D 0; >> + int value =3D -42; >> Visitor *v; >> >> - v =3D visitor_input_test_init(data, "%" PRId64, value); >> + v =3D visitor_input_test_init(data, "%d", value); >> >> visit_type_int(v, NULL, &res, &error_abort); >> g_assert_cmpint(res, =3D=3D, value); >> -- >> 2.7.4 >> >> >=20 > This part is fine I guess. If desired, I can respin this patch to _just_ the changes under tests/, as that is all the more that is needed to fix MacOS runtime for 2.8, and leave the ripping out of %lld for 2.9 for the same time when I rip out all other % support. Personally, I found it easier to prove that nothing was relying on 64-bit parsing by ripping it out completely, so that glibc platforms would also flag improper uses at runtime, rather than hoping that I had caught everything by mere grep. But I guess that even though 'make check' now passes (and so it appears that we no longer have any runtime dependency on 64-bit values), going with the more conservative approach of just fixing the two tests that relied on 64-bit values but leaving existing support in will avoid problems on glibc and mingw (but not MacOS) if we still managed to miss something not covered by 'make check'. On the bright side, mere grep shows that we really only have 3 remaining clients of qobject_from_jsonf() in the main code body, none of which use 64-bit ints; and that the only clients of qobject_from_jsonv() are in the testsuite; so the fact that 'make check' passes is a pretty good indication that I did not manage to miss anything that still wants to use dynamic JSON with a 64-bit int. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --S07XdUTiE0pJf4jD93MC6trwb9aDWXD1p 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/ iQEcBAEBCAAGBQJYNXVmAAoJEKeha0olJ0NqfYQIAIsH4oJxdDyPx8OO4xfwUCLZ ZZNe4OGBgsblH5cViw4We+inM3+viBwNeIwdFPRy2/pAy2zQwVRCQnscfwa+eKso lkv9VzBa/y/p9ADsR7IfiE0S1gRtMdjkpQyul203aDd8RxDU/j7sSEcp9xeCkbjz iy2e9FdUQZztxOKAWUq+w9Ie/bSqisRDT0jtTKuDLu0Hx4IBxmPOSHI1ZV334cjK jdrzmtdD16++Qd8gPPxMtu9aOF3lnCxPOnv6Jw055K7mntr5hPxo3bcnESGauZVi j5GnpVjonwRdcG5w2TYo7EYo7CR++bq7HvcMf/ReItV6R7qBN5wbM7627Y8757o= =q6A+ -----END PGP SIGNATURE----- --S07XdUTiE0pJf4jD93MC6trwb9aDWXD1p--