Re: [Qemu-devel] [PATCH 2/3] test-qga: Avoid qobject_from_jsonf("%"PRId64)

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  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().