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

[Markus Armbruster]

Philippe Mathieu-Daudé <address@hidden> writes:

> Hi Markus,
>
> On 07/12/2018 08:12 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_vjsonf_nofail()
>> would abort.  A sufficiently nasty @uri could even inject unwanted
>> members into the arguments object.
>> 
>> 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>
>> ---
>>  tests/libqos/pci-pc.c   |   9 +--
>>  tests/libqtest.c        |   7 +-
>>  tests/migration-test.c  |   8 +--
>>  tests/test-qga.c        | 150 ++++++++++++++++++----------------------
>>  tests/tpm-util.c        |   9 +--
>>  tests/vhost-user-test.c |   6 +-
>>  6 files changed, 77 insertions(+), 112 deletions(-)
>> 
>> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
>> index a7803308b7..585f5289ec 100644
>> --- a/tests/libqos/pci-pc.c
>> +++ b/tests/libqos/pci-pc.c
>> @@ -160,14 +160,9 @@ void qpci_free_pc(QPCIBus *bus)
>>  void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
>>  {
>>      QDict *response;
>> -    char *cmd;
>>  
>> -    cmd = g_strdup_printf("{'execute': 'device_del',"
>> -                          " 'arguments': {"
>> -                          "   'id': '%s'"
>> -                          "}}", id);
>> -    response = qmp(cmd);
>> -    g_free(cmd);
>> +    response = qmp("{'execute': 'device_del', 'arguments': {'id': %s}}",
>> +                   id);
>>      g_assert(response);
>>      g_assert(!qdict_haskey(response, "error"));
>>      qobject_unref(response);
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index 3bfb062fcb..e36cc5ede3 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -1079,12 +1079,9 @@ void qtest_qmp_device_add(const char *driver, const 
>> char *id, const char *fmt,
>>  void qtest_qmp_device_del(const char *id)
>>  {
>>      QDict *response1, *response2, *event = NULL;
>> -    char *cmd;
>>  
>> -    cmd = g_strdup_printf("{'execute': 'device_del',"
>> -                          " 'arguments': { 'id': '%s' }}", id);
>> -    response1 = qmp(cmd);
>> -    g_free(cmd);
>> +    response1 = qmp("{'execute': 'device_del', 'arguments': {'id': %s}}",
>> +                    id);
>>      g_assert(response1);
>>      g_assert(!qdict_haskey(response1, "error"));
>>  
>> diff --git a/tests/migration-test.c b/tests/migration-test.c
>> index 086f727b34..bbb9d3da31 100644
>> --- a/tests/migration-test.c
>> +++ b/tests/migration-test.c
>> @@ -529,14 +529,12 @@ static void test_migrate_end(QTestState *from, 
>> QTestState *to, bool test_dest)
>>  static void deprecated_set_downtime(QTestState *who, const double value)
>>  {
>>      QDict *rsp;
>> -    gchar *cmd;
>>      char *expected;
>>      int64_t result_int;
>>  
>> -    cmd = g_strdup_printf("{ 'execute': 'migrate_set_downtime',"
>> -                          "'arguments': { 'value': %g } }", value);
>> -    rsp = qtest_qmp(who, cmd);
>> -    g_free(cmd);
>> +    rsp = qtest_qmp(who,
>> +                    "{ 'execute': 'migrate_set_downtime',"
>> +                    " 'arguments': { 'value': %f } }", value);
>
> I suppose you changed %g -> %f because the downtime is expected
> to be greater than 1e-4 :)

Actually, I changed it to %f, because %g would crash :)

qtest_qmp()'s function comment provides a clue:

 * @fmt...: QMP message to send to qemu; formats arguments through
 * json-lexer.c (only understands '%((l|ll|I64)?d|[ipsf])').

This isn't your ordinary printf()-like formatting, it's JSON
interpolation.  JSON interpolation borrows its escapes from printf() to
get some compile-time checking out of GCC function attribute 'format',
namely catching arguments that don't match escapes.  It can't catch
escapes JSON interpolation doesn't support.  Much better than nothing.

In particular, JSON interpolation supports %f but not %g.

Hmm, the comment's claim "json-lexer.c" is perhaps overly specific.
json-lexer.c recognizes valid %-escapes, but the actual interpolation is
done by json-parser.c's parse_escape().

> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

Thanks!

[...]