From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52520) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c9UuQ-0007DJ-J8 for qemu-devel@nongnu.org; Wed, 23 Nov 2016 05:36:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c9UuM-0000r4-FM for qemu-devel@nongnu.org; Wed, 23 Nov 2016 05:36:10 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52356) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c9UuM-0000pu-78 for qemu-devel@nongnu.org; Wed, 23 Nov 2016 05:36:06 -0500 References: <1479874588-1969-1-git-send-email-eblake@redhat.com> <1479874588-1969-3-git-send-email-eblake@redhat.com> <1983787067.1379664.1479893001061.JavaMail.zimbra@redhat.com> From: Eric Blake Message-ID: <21053836-664b-d6a2-d324-bf9b942e3a15@redhat.com> Date: Wed, 23 Nov 2016 04:36:03 -0600 MIME-Version: 1.0 In-Reply-To: <1983787067.1379664.1479893001061.JavaMail.zimbra@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="9An1c9sGsRm2Uu87G1kFM1hH1jedqgP6h" Subject: Re: [Qemu-devel] [PATCH 2/3] test-qga: Avoid 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 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --9An1c9sGsRm2Uu87G1kFM1hH1jedqgP6h From: Eric Blake To: Paolo Bonzini Cc: qemu-devel@nongnu.org, programmingkidx@gmail.com, armbru@redhat.com Message-ID: <21053836-664b-d6a2-d324-bf9b942e3a15@redhat.com> Subject: Re: [PATCH 2/3] test-qga: Avoid qobject_from_jsonf("%"PRId64) References: <1479874588-1969-1-git-send-email-eblake@redhat.com> <1479874588-1969-3-git-send-email-eblake@redhat.com> <1983787067.1379664.1479893001061.JavaMail.zimbra@redhat.com> In-Reply-To: <1983787067.1379664.1479893001061.JavaMail.zimbra@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/23/2016 03:23 AM, Paolo Bonzini wrote: >=20 >=20 > ----- Original Message ----- >> From: "Eric Blake" >> To: qemu-devel@nongnu.org >> Cc: programmingkidx@gmail.com, armbru@redhat.com, pbonzini@redhat.com >> Sent: Wednesday, November 23, 2016 5:16:27 AM >> Subject: [PATCH 2/3] test-qga: Avoid qobject_from_jsonf("%"PRId64) >> >> 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, it is just as >> easy to use normal printf() for this particular conversion, >> matching what is done elsewhere in this file. This is the key, look at: $ git grep -A2 strdup_printf tests/test-qga.c and you'll see that my change is no grosser than the rest of the file. >> - ret =3D qmp_fd(fixture->fd, "{'execute': 'guest-exec-status',= " >> - " 'arguments': { 'pid': %" PRId64 " } }", pid);= >> + char *cmd; >> + >> + cmd =3D g_strdup_printf("{'execute': 'guest-exec-status'," >> + " 'arguments': { 'pid': %" PRId64 " } }= ", >> + pid); _THIS_ patch merely moves the (pre-existing, ugly) association of "%"PRId64, pid from qobject_from_jsonf() to g_strdup_printf(). But your question... >=20 > This is too ugly to see. :) Why not use %lld, or just make pid an > int? Are there really systems with 64-bit pid_t? =2E..is whether the pre-existing code is correct. For the purposes of fixing Mac OS compilation, I argue that your question doesn't matter. I agree with you that the code looks gross, but that's not my fault. But here's why I think changing the ugliness is wrong: The existing code already used 64-bit pid (note, this is 'pid', not 'pid_t'), because of: int64_t pid, now, exitcode; =2E.. val =3D qdict_get_qdict(ret, "return"); pid =3D qdict_get_int(val, "pid"); g_assert_cmpint(pid, >, 0); And yes, the qapi schema currently lists a 64-bit type (anything without an explicit size is 64 bits over JSON): { 'struct': 'GuestExec', 'data': { 'pid': 'int'} } And yes, 64-bit mingw has a 64-bit pid_t (ewwww) /usr/x86_64-w64-mingw32/sys-root/mingw/include/sys/types.h: #ifndef _PID_T_ #define _PID_T_ #ifndef _WIN64 typedef int _pid_t; #else __MINGW_EXTENSION typedef __int64 _pid_t; #endif although there is a long-standing bug in mingw headers, since getpid() returns a different type than pid_t: /usr/x86_64-w64-mingw32/sys-root/mingw/include/unistd.h: int __cdecl getpid(void) __MINGW_ATTRIB_DEPRECATED_MSVC2005; and sadly, mingw lacks the POSIX-required id_t type (which must be at least as large as any of pid_t, uid_t, or gid_t) to know if it was intentional. At any rate, I worry that changing the QGA definition to specify 'int32' may break compilation on mingw, where the qga code would have to start casting from the (possibly-broken) 64-bit pid_t down to a 32-bit value when populating the QAPI struct to pass back over the wire. I _really_ wish mingw would fix their headers (decide if 64-bit pid_t is really correct or not, and then make pid_t and getpid() match as well as supply id_t), but this isn't the forum to get that accomplished. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --9An1c9sGsRm2Uu87G1kFM1hH1jedqgP6h 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/ iQEcBAEBCAAGBQJYNXETAAoJEKeha0olJ0NqshcH/AunRNYm3Ud62Q6m6a/3P7CF WD7yvDs70zAlkqnSEnA2E3o/PmqeTMftSWsyFH267LgOrHK4UNTGuCKAYLHraQlB Tmgk+TT4mlf2S64HDxPVfGDSNFWN/FGjI2anjLlYuL5K2ZUj6VbM4Gj97x1kD3M5 74To7tVccrkkWwpuIIm6d0z5m5I2shZkR5dKMl54ZgNjYitAf8kcxsU5UVtajI42 vYe/TP/rL/TPcfenkWBKM8hyG0YMkAw4xXWF59CpUINqmQvdfsEJt5JvfuKp5ue5 YzW3W85oJfqEgyDK0nvdE2RsuDHUHkfnfBM0vAGLI/MVq3G0MdkbaU01XbDqhC8= =qot5 -----END PGP SIGNATURE----- --9An1c9sGsRm2Uu87G1kFM1hH1jedqgP6h--