| Message ID | 1479874588-1969-3-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
----- Original Message ----- > From: "Eric Blake" <eblake@redhat.com> > 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. > > Reported by: G 3 <programmingkidx@gmail.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > 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); 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? Paolo > + 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"); > -- > 2.7.4 > >
On 11/23/2016 03:23 AM, Paolo Bonzini wrote: > > > ----- Original Message ----- >> From: "Eric Blake" <eblake@redhat.com> >> 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 = 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); _THIS_ patch merely moves the (pre-existing, ugly) association of "%"PRId64, pid from qobject_from_jsonf() to g_strdup_printf(). But your question... > > 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? ...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; ... val = qdict_get_qdict(ret, "return"); pid = 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.
On 11/23/2016 04:36 AM, Eric Blake wrote: > 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. https://bugzilla.redhat.com/show_bug.cgi?id=1397787
On 23/11/2016 11:36, Eric Blake wrote: > 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. > Oh well... I guess with a better commit message the patch is fine. Paolo
Eric Blake <eblake@redhat.com> 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 <programmingkidx@gmail.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > 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().
On 11/23/2016 08:05 AM, Markus Armbruster wrote: > 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. Are there things we can do to make it more readable to the point where it would be tolerable in the situations where it is needed? One of the patches on my dynamic-JSON removal series adds a new: qdict_put_int(dict, "key", 1); which is a lot more legible than: qdict_put(dict, "key", qint_from_int(1)); > > 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 > '%'. Worse, the escaping of %s differs between the two (in printf, %s just concatenates strings, in dynamic JSON, it adds outer "" and escapes inner " into \"). > > 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(). > Well, until I post my conversion series that eliminates _json[fv](), we don't have any hard numbers on how many bad examples remain, or whether the cleanup looks worth it.
Eric Blake <eblake@redhat.com> writes: > On 11/23/2016 08:05 AM, Markus Armbruster wrote: > >> 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. > > Are there things we can do to make it more readable to the point where > it would be tolerable in the situations where it is needed? > > One of the patches on my dynamic-JSON removal series adds a new: > > qdict_put_int(dict, "key", 1); > > which is a lot more legible than: > > qdict_put(dict, "key", qint_from_int(1)); It's more legible, but I wouldn't call it "a lot more legible". >> 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 >> '%'. > > Worse, the escaping of %s differs between the two (in printf, %s just > concatenates strings, in dynamic JSON, it adds outer "" and escapes > inner " into \"). That's a feature. It actually escapes much more than just '"'. Have a look at to_json() case QTYPE_QSTRING. The imporant bit here is: _jsonf() is not printf()! The part it shares with printf() is the argument types associated with conversion specifiers, and it shares them just because that way the compiler can help us catch type errors. What it does with the arguments is *different*, because what it does is different. It does *not* format a string. Not even conceptually. It builds a QObject from a string template. >> 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(). > > Well, until I post my conversion series that eliminates _json[fv](), we > don't have any hard numbers on how many bad examples remain, or whether > the cleanup looks worth it. Yes. Without patches, the discussion is speculative.
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");
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 <programmingkidx@gmail.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/test-qga.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)