Re: [Qemu-devel] [PATCH 5/5] qtest: Document calling conventions

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/20/2017 03:37 PM, Eric Blake wrote:
>>>> + * @fmt...: QMP message to send to qemu; only recognizes formats
>>>> + * understood by json-lexer.c
>>>>   *
>>>>   * Sends a QMP message to QEMU and consumes the response.
>>>>   */
>>>
>>> These formats are chosen so that gcc can help us check them.  Please add
>>> GCC_FMT_ATTR().  Precedence: qobject_from_jsonf().
>> 
>> Will do.  (This patch was originally part of my larger series trying to
>> get rid of qobject_from_jsonf() - I may still succeed at that, but
>> separating the easy stuff from the controversial means that the easy
>> stuff needs tweaking).
>
> Bleargh.  It's not that simple.
>
> With printf-style, hmp("literal") and hmp("%s", "literal") produce the
> same results.
>
> But with json-lexer style, %s MODIFIES its input.

Your assertion "MODIFIES its input" confused me briefly, because I read
it as "side effect on the argument string".  That would be bonkers.
What you mean is it doesn't emit the argument string unmodified.

> The original qmp("{'execute':\"foo\"}") sends a JSON object
>  {'execute':"foo"}
> but counterpart qmp("%s", "{'execute':'foo'}") sends a JSON string with
> added \ escaping:
>  "{'execute':\"foo\"}"
>
> So adding the format immediately causes lots of warnings, such as:
>
>   CC      tests/vhost-user-test.o
> tests/vhost-user-test.c: In function ‘test_migrate’:
> tests/vhost-user-test.c:668:5: error: format not a string literal and no
> format arguments [-Werror=format-security]
>      rsp = qmp(cmd);
>      ^~~

    cmd = g_strdup_printf("{ 'execute': 'migrate',"
                          "'arguments': { 'uri': '%s' } }",
                          uri);
    rsp = qmp(cmd);
    g_free(cmd);
    g_assert(qdict_haskey(rsp, "return"));
    QDECREF(rsp);

For this to work, @uri must not contain funny characters.

Shouldn't we simply use the built-in quoting here?

    rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
    g_assert(qdict_haskey(rsp, "return"));
    QDECREF(rsp);

>
>   CC      tests/boot-order-test.o
> tests/boot-order-test.c: In function ‘test_a_boot_order’:
> tests/boot-order-test.c:46:26: error: zero-length gnu_printf format
> string [-Werror=format-zero-length]
>      qmp_discard_response("");   /* HACK: wait for event */
>                           ^~

Should use qmp_eventwait().  Doesn't because it predates it.

> but we CAN'T rewrite those to qmp("%s", command).

Got more troublemakers?

> Now you can sort-of understand why my larger series wanted to get rid of
> qobject_from_jsonf(), since our use of GCC_FMT_ATTR() there is a lie?

I don't think it's a lie.  qobject_from_jsonf() satisfies the attribute
format(printf, ...) contract: it fetches varargs exactly like printf().
What it does with them is of no concern to this attribute.