Re: [Qemu-devel] [PATCH 4/9] tests: Clean up string interpolation into QMP input (simple cases)

[Eric Blake]

On 07/21/2017 08:53 AM, Markus Armbruster wrote:
> When you build QMP input manually like this
> 
>     cmd = g_strdup_printf("{ 'execute': 'migrate',"
>                           "'arguments': { 'uri': '%s' } }",
>                           uri);
>     rsp = qmp(cmd);
>     g_free(cmd);
> 
> you're responsible for escaping the interpolated values for JSON.  Not
> done here, and therefore works only for sufficiently nice @uri.  For
> instance, if @uri contained a single "'", qobject_from_jsonv() would
> fail, qmp_fd_sendv() would misinterpret the failure as empty input and
> do nothing, and the test would hang waiting for a response that never
> comes.
> 
> Leaving interpolation into JSON to qmp() is more robust:
> 
>     rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
> 
> It's also more concise.
> 
> Clean up the simple cases where we interpolate exactly a JSON value.
> 
> Bonus: gets rid of non-literal format strings.  A step towards
> compile-time format string checking without triggering
> -Wformat-nonliteral.
> 
> Signed-off-by: Markus Armbruster <address@hidden>

> ---
> +    qmp_fd_send(fixture->fd,
> +                "\xff{'execute': 'guest-sync-delimited',"
> +                " 'arguments': {'id': %u } }",

Ouch. Per 2/9:
 * json-lexer.c (only understands '%((l|ll|I64)?d|[ipsf])').

That will need to be %d now. Or, more likely, we need to update the
comments in json-lexer.c and in 2/9 (/me goes to read json-lexer.c...)
    /* escape */
    [IN_ESCAPE_LL] = {
        ['d'] = JSON_ESCAPE,
        ['u'] = JSON_ESCAPE,
    },
...
    [IN_ESCAPE] = {
        ['d'] = JSON_ESCAPE,
        ['i'] = JSON_ESCAPE,
        ['p'] = JSON_ESCAPE,
        ['s'] = JSON_ESCAPE,
        ['u'] = JSON_ESCAPE,
        ['f'] = JSON_ESCAPE,
        ['l'] = IN_ESCAPE_L,
        ['I'] = IN_ESCAPE_I,

Aha, that 'd' should be '[du]' in all of the comments.

> @@ -411,10 +408,10 @@ static void test_qga_file_ops(gconstpointer fix)
>  
>      enc = g_base64_encode(helloworld, sizeof(helloworld));
>      /* write */
> -    cmd = g_strdup_printf("{'execute': 'guest-file-write',"
> -                          " 'arguments': { 'handle': %" PRId64 ","
> -                          " 'buf-b64': '%s' } }", id, enc);
> -    ret = qmp_fd(fixture->fd, cmd);
> +    ret = qmp_fd(fixture->fd,
> +                 "{'execute': 'guest-file-write',"
> +                 " 'arguments': { 'handle': %" PRId64 ", 'buf-b64': %s } }",

Ouch; you are reverting commit 1792d7d0. We tried hard to make
json-lexer.c understand lots of different 64-bit format spellings, but
we KNOW that we don't support MacOS (see commit 29a6731).  We either
need to beef up json-lexer.c to understand %qd, or get rid of JSON
psuedo-strings, if we expect this to work; otherwise, you should use
%lld and long long instead of PRId64 and uint64_t.

Overall, I like the patch, but we need to fix the problems for the next
round of this series.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature