From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56330) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c9YAr-0001PX-JY for qemu-devel@nongnu.org; Wed, 23 Nov 2016 09:05:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c9YAn-00063r-Jl for qemu-devel@nongnu.org; Wed, 23 Nov 2016 09:05:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37828) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c9YAn-00062o-B5 for qemu-devel@nongnu.org; Wed, 23 Nov 2016 09:05:17 -0500 From: Markus Armbruster References: <1479874588-1969-1-git-send-email-eblake@redhat.com> <1479874588-1969-3-git-send-email-eblake@redhat.com> Date: Wed, 23 Nov 2016 15:05:14 +0100 In-Reply-To: <1479874588-1969-3-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 22 Nov 2016 22:16:27 -0600") Message-ID: <87r362pa05.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain 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: Eric Blake Cc: qemu-devel@nongnu.org, programmingkidx@gmail.com, pbonzini@redhat.com Eric Blake writes: > 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. > > Reported by: G 3 > Signed-off-by: Eric Blake > --- > tests/test-qga.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/tests/test-qga.c b/tests/test-qga.c > index 40af649..421e27c 100644 > --- a/tests/test-qga.c > +++ b/tests/test-qga.c > @@ -852,8 +852,13 @@ static void test_qga_guest_exec(gconstpointer fix) > /* wait for completion */ > now = g_get_monotonic_time(); > do { > - ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec-status'," > - " 'arguments': { 'pid': %" PRId64 " } }", pid); > + char *cmd; > + > + cmd = g_strdup_printf("{'execute': 'guest-exec-status'," > + " 'arguments': { 'pid': %" PRId64 " } }", > + pid); > + ret = qmp_fd(fixture->fd, cmd); > + g_free(cmd); > g_assert_nonnull(ret); > val = qdict_get_qdict(ret, "return"); > exited = qdict_get_bool(val, "exited"); Same problem as in the previous patch, but here you replace it by g_strdup_printf(), where the previous patch replaced it by manual QObject construction, Manual QObject construction tends to be less readable. g_strdup_printf() doesn't have that problem, but it has a more serious one: escaping for JSON is no longer below the hood. Since the string gets passed to qmp_fd(), we additionally need to escape '%'. Interfaces that require callers to escape almost inevitably result in bugs if experience is any guide. Safer, less low level interfaces are preferable. Nothing actually needs escaping here, so your code isn't wrong. It's just a bad example. You've pointed out that the file is chock-full of bad examples already, so one more won't make a difference. Point taken regarding the immediate fix. But I doubt it a sane strategy for replacing _jsonf().