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

[Eric Blake]

On 07/21/2017 01:42 AM, Markus Armbruster wrote:
>> 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.

Yes. I'm not sure how I could have worded that better; maybe "does not
do a straight passthrough of its input"

>> 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?

We can - but there are a lot of tests that have to be updated.

> 
>     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.

We can - but there are a lot of tests that have to be updated.

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

When I worked on getting rid of qobject_from_jsonf(), I was able to
reduce the tests down to JUST using %s, which I then handled locally in
qmp() rather than relying on qobject_from_jsonf().  Going the opposite
direction and more fully relying on qobject_from_jsonf() by fixing
multiple tests that were using older idioms is also an approach (in the
opposite direction I originally took) - but the more work it is, the
less likely that this patch is 2.10 material.

> 
>> 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.

I still find it odd that you can't safely turn foo(var) into foo("%s", var).

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

signature.asc
Description: OpenPGP digital signature