[Qemu-devel] [PATCH 0/9] tests: Clean up around qmp() and hmp()

[Qemu-devel] [PATCH 0/9] tests: Clean up around qmp() and hmp()

[Markus Armbruster]

Eric Blake (2):
  qtest: Avoid passing raw strings through hmp()
  qtest: Document calling conventions

Markus Armbruster (7):
  tests: Pass literal format strings directly to qmp_FOO()
  tests: Clean up string interpolation into QMP input (simple cases)
  tests/libqos/usb: Clean up string interpolation into QMP input
  tests/libqos/pci: Clean up string interpolation into QMP input
  tests: Clean up wait for event
  tests/libqtest: Clean up how we read the QMP greeting
  tests/libqtest: Enable compile-time format string checking

 tests/ahci-test.c         |   4 +-
 tests/boot-order-test.c   |   2 +-
 tests/ide-test.c          |   4 +-
 tests/ivshmem-test.c      |  10 +--
 tests/libqos/libqos.c     |  16 +----
 tests/libqos/pci-pc.c     |  15 +----
 tests/libqos/pci.c        |  33 +++++-----
 tests/libqos/pci.h        |   2 +-
 tests/libqos/usb.c        |  30 ++++-----
 tests/libqtest.c          |   4 +-
 tests/libqtest.h          |  62 +++++++++++-------
 tests/postcopy-test.c     |   8 +--
 tests/tco-test.c          |   3 +-
 tests/test-hmp.c          |   4 +-
 tests/test-qga.c          | 160 ++++++++++++++++++++--------------------------
 tests/usb-hcd-uhci-test.c |   6 +-
 tests/usb-hcd-xhci-test.c |  12 +---
 tests/vhost-user-test.c   |   6 +-
 tests/virtio-blk-test.c   |   5 +-
 tests/wdt_ib700-test.c    |  35 +++-------
 20 files changed, 180 insertions(+), 241 deletions(-)

-- 
2.7.5

[Qemu-devel] [PATCH 1/9] qtest: Avoid passing raw strings through hmp()

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

The next patch will add __attribute__((__format__)) to hmp(), which
in turn causes gcc to warn about non-literal format strings.  Rather
than risk an arbitrary string containing % being mis-handled, always
pass variable strings along with a %s format.  It also makes it
easier to prove correctness locally, rather than auditing all the
source strings.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20170720214008.28494-4-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-hmp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/test-hmp.c b/tests/test-hmp.c
index d77b3c8..0af0664 100644
--- a/tests/test-hmp.c
+++ b/tests/test-hmp.c
@@ -80,7 +80,7 @@ static void test_commands(void)
         if (verbose) {
             fprintf(stderr, "\t%s\n", hmp_cmds[i]);
         }
-        response = hmp(hmp_cmds[i]);
+        response = hmp("%s", hmp_cmds[i]);
         g_free(response);
     }
 
@@ -103,7 +103,7 @@ static void test_info_commands(void)
         if (verbose) {
             fprintf(stderr, "\t%s\n", info);
         }
-        resp = hmp(info);
+        resp = hmp("%s", info);
         g_free(resp);
         /* And move forward to the next line */
         info = strchr(endp + 1, '\n');
-- 
2.7.5

Re: [Qemu-devel] [PATCH 1/9] qtest: Avoid passing raw strings through hmp()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 846 bytes --]

On Fri, Jul 21, 2017 at 03:53:18PM +0200, Markus Armbruster wrote:
> From: Eric Blake <eblake@redhat.com>
> 
> The next patch will add __attribute__((__format__)) to hmp(), which
> in turn causes gcc to warn about non-literal format strings.  Rather
> than risk an arbitrary string containing % being mis-handled, always
> pass variable strings along with a %s format.  It also makes it
> easier to prove correctness locally, rather than auditing all the
> source strings.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Message-Id: <20170720214008.28494-4-eblake@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/test-hmp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 2/9] qtest: Document calling conventions

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

We have two flavors of vararg usage in qtest; make it clear that
qmp() has different semantics than hmp(), and let the compiler
enforce that hmp() is used correctly. However, qmp() (and friends)
only accept a subset of printf flags look-alikes (namely, those
that our JSON parser understands), and what is worse, qmp("true")
(the JSON keyword 'true') is different from qmp("%s", "true")
(the JSON string '"true"'), so marking those as printf-like would
produce more harm from bogus warnings than it helps (we may have
made a mistake in previously marking qobject_from_jsonf(), but
this patch is not addressing that).

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20170720214008.28494-5-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/libqtest.h | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index 38bc1e9..ae57282 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -50,7 +50,8 @@ void qtest_quit(QTestState *s);
 /**
  * qtest_qmp_discard_response:
  * @s: #QTestState instance to operate on.
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; formats arguments through
+ * json-lexer.c (only understands '%((l|ll|I64)?d|[ipsf])').
  *
  * Sends a QMP message to QEMU and consumes the response.
  */
@@ -59,7 +60,8 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
 /**
  * qtest_qmp:
  * @s: #QTestState instance to operate on.
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; formats arguments through
+ * json-lexer.c (only understands '%((l|ll|I64)?d|[ipsf])').
  *
  * Sends a QMP message to QEMU and returns the response.
  */
@@ -68,7 +70,8 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
 /**
  * qtest_async_qmp:
  * @s: #QTestState instance to operate on.
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; formats arguments through
+ * json-lexer.c (only understands '%((l|ll|I64)?d|[ipsf])').
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
  */
@@ -134,7 +137,7 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
 /**
  * qtest_hmp:
  * @s: #QTestState instance to operate on.
- * @fmt...: HMP command to send to QEMU
+ * @fmt...: HMP command to send to QEMU, formats arguments like vsprintf().
  *
  * Send HMP command to QEMU via QMP's human-monitor-command.
  * QMP events are discarded.
@@ -535,7 +538,8 @@ static inline void qtest_end(void)
 
 /**
  * qmp:
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; formats arguments through
+ * json-lexer.c (only understands '%((l|ll|I64)?d|[ipsf])').
  *
  * Sends a QMP message to QEMU and returns the response.
  */
@@ -543,7 +547,8 @@ QDict *qmp(const char *fmt, ...);
 
 /**
  * qmp_async:
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; formats arguments through
+ * json-lexer.c (only understands '%((l|ll|I64)?d|[ipsf])').
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
  */
@@ -551,7 +556,8 @@ void qmp_async(const char *fmt, ...);
 
 /**
  * qmp_discard_response:
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; formats arguments through
+ * json-lexer.c (only understands '%((l|ll|I64)?d|[ipsf])').
  *
  * Sends a QMP message to QEMU and consumes the response.
  */
@@ -592,7 +598,7 @@ static inline QDict *qmp_eventwait_ref(const char *event)
 
 /**
  * hmp:
- * @fmt...: HMP command to send to QEMU
+ * @fmt...: HMP command to send to QEMU, formats arguments like vsprintf().
  *
  * Send HMP command to QEMU via QMP's human-monitor-command.
  *
-- 
2.7.5

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1845 bytes --]

On 07/21/2017 08:53 AM, Markus Armbruster wrote:
> From: Eric Blake <eblake@redhat.com>
> 
> We have two flavors of vararg usage in qtest; make it clear that
> qmp() has different semantics than hmp(), and let the compiler
> enforce that hmp() is used correctly. However, qmp() (and friends)
> only accept a subset of printf flags look-alikes (namely, those
> that our JSON parser understands), and what is worse, qmp("true")
> (the JSON keyword 'true') is different from qmp("%s", "true")
> (the JSON string '"true"'), so marking those as printf-like would
> produce more harm from bogus warnings than it helps (we may have
> made a mistake in previously marking qobject_from_jsonf(), but
> this patch is not addressing that).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Message-Id: <20170720214008.28494-5-eblake@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

As you pointed out on the other thread,

> @@ -134,7 +137,7 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
>  /**
>   * qtest_hmp:
>   * @s: #QTestState instance to operate on.
> - * @fmt...: HMP command to send to QEMU
> + * @fmt...: HMP command to send to QEMU, formats arguments like vsprintf().
>   *
>   * Send HMP command to QEMU via QMP's human-monitor-command.
>   * QMP events are discarded.

I accidentally killed the attribute here,

> @@ -592,7 +598,7 @@ static inline QDict *qmp_eventwait_ref(const char *event)
>  
>  /**
>   * hmp:
> - * @fmt...: HMP command to send to QEMU
> + * @fmt...: HMP command to send to QEMU, formats arguments like vsprintf().
>   *
>   * Send HMP command to QEMU via QMP's human-monitor-command.
>   *

and here.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/21/2017 08:53 AM, Markus Armbruster wrote:
>> From: Eric Blake <eblake@redhat.com>
>> 
>> We have two flavors of vararg usage in qtest; make it clear that
>> qmp() has different semantics than hmp(), and let the compiler
>> enforce that hmp() is used correctly. However, qmp() (and friends)
>> only accept a subset of printf flags look-alikes (namely, those
>> that our JSON parser understands), and what is worse, qmp("true")
>> (the JSON keyword 'true') is different from qmp("%s", "true")
>> (the JSON string '"true"'), so marking those as printf-like would
>> produce more harm from bogus warnings than it helps (we may have
>> made a mistake in previously marking qobject_from_jsonf(), but
>> this patch is not addressing that).
>> 
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Message-Id: <20170720214008.28494-5-eblake@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> As you pointed out on the other thread,
>
>> @@ -134,7 +137,7 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
>>  /**
>>   * qtest_hmp:
>>   * @s: #QTestState instance to operate on.
>> - * @fmt...: HMP command to send to QEMU
>> + * @fmt...: HMP command to send to QEMU, formats arguments like vsprintf().
>>   *
>>   * Send HMP command to QEMU via QMP's human-monitor-command.
>>   * QMP events are discarded.
>
> I accidentally killed the attribute here,
>
>> @@ -592,7 +598,7 @@ static inline QDict *qmp_eventwait_ref(const char *event)
>>  
>>  /**
>>   * hmp:
>> - * @fmt...: HMP command to send to QEMU
>> + * @fmt...: HMP command to send to QEMU, formats arguments like vsprintf().
>>   *
>>   * Send HMP command to QEMU via QMP's human-monitor-command.
>>   *
>
> and here.

Putting them back is easier than updating commit message 1/9.

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

On Fri, Jul 21, 2017 at 03:53:19PM +0200, Markus Armbruster wrote:
> From: Eric Blake <eblake@redhat.com>
> 
> We have two flavors of vararg usage in qtest; make it clear that
> qmp() has different semantics than hmp(), and let the compiler
> enforce that hmp() is used correctly. However, qmp() (and friends)
> only accept a subset of printf flags look-alikes (namely, those
> that our JSON parser understands), and what is worse, qmp("true")
> (the JSON keyword 'true') is different from qmp("%s", "true")
> (the JSON string '"true"'), so marking those as printf-like would
> produce more harm from bogus warnings than it helps (we may have
> made a mistake in previously marking qobject_from_jsonf(), but
> this patch is not addressing that).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Message-Id: <20170720214008.28494-5-eblake@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/libqtest.h | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 3/9] tests: Pass literal format strings directly to qmp_FOO()

[Markus Armbruster]

The qmp_FOO() take a printf-like format string.  In a few places, we
assign a string literal to a variable and pass that instead of simply
passing the literal.  Clean that up.

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 <armbru@redhat.com>
---
 tests/ahci-test.c | 4 +---
 tests/ide-test.c  | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 999121b..97622e0 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1350,7 +1350,6 @@ static void test_flush_migrate(void)
     AHCIQState *src, *dst;
     AHCICommand *cmd;
     uint8_t px;
-    const char *s;
     char *uri = g_strdup_printf("unix:%s", mig_socket);
 
     prepare_blkdebug_script(debug_path, "flush_to_disk");
@@ -1386,8 +1385,7 @@ static void test_flush_migrate(void)
     ahci_migrate(src, dst, uri);
 
     /* Complete the command */
-    s = "{'execute':'cont' }";
-    qmp_async(s);
+    qmp_async("{'execute':'cont' }");
     qmp_eventwait("RESUME");
     ahci_command_wait(dst, cmd);
     ahci_command_verify(dst, cmd);
diff --git a/tests/ide-test.c b/tests/ide-test.c
index bfd79dd..ea2657d 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -624,7 +624,6 @@ static void test_retry_flush(const char *machine)
     QPCIDevice *dev;
     QPCIBar bmdma_bar, ide_bar;
     uint8_t data;
-    const char *s;
 
     prepare_blkdebug_script(debug_path, "flush_to_disk");
 
@@ -652,8 +651,7 @@ static void test_retry_flush(const char *machine)
     qmp_eventwait("STOP");
 
     /* Complete the command */
-    s = "{'execute':'cont' }";
-    qmp_discard_response(s);
+    qmp_discard_response("{'execute':'cont' }");
 
     /* Check registers */
     data = qpci_io_readb(dev, ide_bar, reg_device);
-- 
2.7.5

Re: [Qemu-devel] [PATCH 3/9] tests: Pass literal format strings directly to qmp_FOO()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

On 07/21/2017 08:53 AM, Markus Armbruster wrote:
> The qmp_FOO() take a printf-like format string.  In a few places, we
> assign a string literal to a variable and pass that instead of simply
> passing the literal.  Clean that up.
> 
> 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 <armbru@redhat.com>
> ---
>  tests/ahci-test.c | 4 +---
>  tests/ide-test.c  | 4 +---
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 3/9] tests: Pass literal format strings directly to qmp_FOO()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 641 bytes --]

On Fri, Jul 21, 2017 at 03:53:20PM +0200, Markus Armbruster wrote:
> The qmp_FOO() take a printf-like format string.  In a few places, we
> assign a string literal to a variable and pass that instead of simply
> passing the literal.  Clean that up.
> 
> 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 <armbru@redhat.com>
> ---
>  tests/ahci-test.c | 4 +---
>  tests/ide-test.c  | 4 +---
>  2 files changed, 2 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Markus Armbruster]

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 <armbru@redhat.com>
---
 tests/libqos/libqos.c   |  16 +----
 tests/libqos/pci-pc.c   |   9 +--
 tests/postcopy-test.c   |   8 +--
 tests/test-qga.c        | 160 +++++++++++++++++++++---------------------------
 tests/vhost-user-test.c |   6 +-
 5 files changed, 77 insertions(+), 122 deletions(-)

diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
index 6226546..42c5315 100644
--- a/tests/libqos/libqos.c
+++ b/tests/libqos/libqos.c
@@ -86,20 +86,12 @@ void set_context(QOSState *s)
 
 static QDict *qmp_execute(const char *command)
 {
-    char *fmt;
-    QDict *rsp;
-
-    fmt = g_strdup_printf("{ 'execute': '%s' }", command);
-    rsp = qmp(fmt);
-    g_free(fmt);
-
-    return rsp;
+    return qmp("{ 'execute': %s }", command);
 }
 
 void migrate(QOSState *from, QOSState *to, const char *uri)
 {
     const char *st;
-    char *s;
     QDict *rsp, *sub;
     bool running;
 
@@ -114,11 +106,7 @@ void migrate(QOSState *from, QOSState *to, const char *uri)
     QDECREF(rsp);
 
     /* Issue the migrate command. */
-    s = g_strdup_printf("{ 'execute': 'migrate',"
-                        "'arguments': { 'uri': '%s' } }",
-                        uri);
-    rsp = qmp(s);
-    g_free(s);
+    rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
     g_assert(qdict_haskey(rsp, "return"));
     QDECREF(rsp);
 
diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
index ded1c54..d40aa9d 100644
--- a/tests/libqos/pci-pc.c
+++ b/tests/libqos/pci-pc.c
@@ -159,14 +159,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"));
     QDECREF(response);
diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index 8142f2a..e2ead87 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -358,7 +358,7 @@ static void test_migrate(void)
     char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
     QTestState *global = global_qtest, *from, *to;
     unsigned char dest_byte_a, dest_byte_b, dest_byte_c, dest_byte_d;
-    gchar *cmd, *cmd_src, *cmd_dst;
+    gchar *cmd_src, *cmd_dst;
     QDict *rsp;
 
     char *bootpath = g_strdup_printf("%s/bootsect", tmpfs);
@@ -445,11 +445,7 @@ static void test_migrate(void)
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
 
-    cmd = g_strdup_printf("{ 'execute': 'migrate',"
-                          "'arguments': { 'uri': '%s' } }",
-                          uri);
-    rsp = qmp(cmd);
-    g_free(cmd);
+    rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
     g_assert(qdict_haskey(rsp, "return"));
     QDECREF(rsp);
 
diff --git a/tests/test-qga.c b/tests/test-qga.c
index 06783e7..91a7b6e 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -144,12 +144,11 @@ static void test_qga_sync_delimited(gconstpointer fix)
     guint32 v, r = g_random_int();
     unsigned char c;
     QDict *ret;
-    gchar *cmd;
 
-    cmd = g_strdup_printf("\xff{'execute': 'guest-sync-delimited',"
-                          " 'arguments': {'id': %u } }", r);
-    qmp_fd_send(fixture->fd, cmd);
-    g_free(cmd);
+    qmp_fd_send(fixture->fd,
+                "\xff{'execute': 'guest-sync-delimited',"
+                " 'arguments': {'id': %u } }",
+                r);
 
     /*
      * Read and ignore garbage until resynchronized.
@@ -186,7 +185,6 @@ static void test_qga_sync(gconstpointer fix)
     const TestFixture *fixture = fix;
     guint32 v, r = g_random_int();
     QDict *ret;
-    gchar *cmd;
 
     /*
      * TODO guest-sync is inherently limited: we cannot distinguish
@@ -199,10 +197,9 @@ static void test_qga_sync(gconstpointer fix)
      * invalid JSON. Testing of '\xff' handling is done in
      * guest-sync-delimited instead.
      */
-    cmd = g_strdup_printf("{'execute': 'guest-sync',"
-                          " 'arguments': {'id': %u } }", r);
-    ret = qmp_fd(fixture->fd, cmd);
-    g_free(cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-sync', 'arguments': {'id': %u } }",
+                 r);
 
     g_assert_nonnull(ret);
     qmp_assert_no_error(ret);
@@ -393,7 +390,7 @@ static void test_qga_file_ops(gconstpointer fix)
     const TestFixture *fixture = fix;
     const unsigned char helloworld[] = "Hello World!\n";
     const char *b64;
-    gchar *cmd, *path, *enc;
+    gchar *path, *enc;
     unsigned char *dec;
     QDict *ret, *val;
     int64_t id, eof;
@@ -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 } }",
+                 id, enc);
     g_assert_nonnull(ret);
     qmp_assert_no_error(ret);
 
@@ -424,23 +421,20 @@ static void test_qga_file_ops(gconstpointer fix)
     g_assert_cmpint(count, ==, sizeof(helloworld));
     g_assert_cmpint(eof, ==, 0);
     QDECREF(ret);
-    g_free(cmd);
 
     /* flush */
-    cmd = g_strdup_printf("{'execute': 'guest-file-flush',"
-                          " 'arguments': {'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-flush',"
+                 " 'arguments': {'handle': %" PRId64 "} }",
+                 id);
     QDECREF(ret);
-    g_free(cmd);
 
     /* close */
-    cmd = g_strdup_printf("{'execute': 'guest-file-close',"
-                          " 'arguments': {'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-close',"
+                 " 'arguments': {'handle': %" PRId64 "} }",
+                 id);
     QDECREF(ret);
-    g_free(cmd);
 
     /* check content */
     path = g_build_filename(fixture->test_dir, "foo", NULL);
@@ -462,10 +456,10 @@ static void test_qga_file_ops(gconstpointer fix)
     QDECREF(ret);
 
     /* read */
-    cmd = g_strdup_printf("{'execute': 'guest-file-read',"
-                          " 'arguments': { 'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-read',"
+                 " 'arguments': { 'handle': %" PRId64 "} }",
+                 id);
     val = qdict_get_qdict(ret, "return");
     count = qdict_get_int(val, "count");
     eof = qdict_get_bool(val, "eof");
@@ -475,14 +469,13 @@ static void test_qga_file_ops(gconstpointer fix)
     g_assert_cmpstr(b64, ==, enc);
 
     QDECREF(ret);
-    g_free(cmd);
     g_free(enc);
 
     /* read eof */
-    cmd = g_strdup_printf("{'execute': 'guest-file-read',"
-                          " 'arguments': { 'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-read',"
+                 " 'arguments': { 'handle': %" PRId64 "} }",
+                 id);
     val = qdict_get_qdict(ret, "return");
     count = qdict_get_int(val, "count");
     eof = qdict_get_bool(val, "eof");
@@ -491,14 +484,13 @@ static void test_qga_file_ops(gconstpointer fix)
     g_assert(eof);
     g_assert_cmpstr(b64, ==, "");
     QDECREF(ret);
-    g_free(cmd);
 
     /* seek */
-    cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
-                          " 'arguments': { 'handle': %" PRId64 ", "
-                          " 'offset': %d, 'whence': '%s' } }",
-                          id, 6, "set");
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-seek',"
+                 " 'arguments': { 'handle': %" PRId64 ", "
+                 " 'offset': %d, 'whence': %s } }",
+                 id, 6, "set");
     qmp_assert_no_error(ret);
     val = qdict_get_qdict(ret, "return");
     count = qdict_get_int(val, "position");
@@ -506,13 +498,12 @@ static void test_qga_file_ops(gconstpointer fix)
     g_assert_cmpint(count, ==, 6);
     g_assert(!eof);
     QDECREF(ret);
-    g_free(cmd);
 
     /* partial read */
-    cmd = g_strdup_printf("{'execute': 'guest-file-read',"
-                          " 'arguments': { 'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-read',"
+                 " 'arguments': { 'handle': %" PRId64 "} }",
+                 id);
     val = qdict_get_qdict(ret, "return");
     count = qdict_get_int(val, "count");
     eof = qdict_get_bool(val, "eof");
@@ -525,15 +516,13 @@ static void test_qga_file_ops(gconstpointer fix)
     g_free(dec);
 
     QDECREF(ret);
-    g_free(cmd);
 
     /* close */
-    cmd = g_strdup_printf("{'execute': 'guest-file-close',"
-                          " 'arguments': {'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-close',"
+                 " 'arguments': {'handle': %" PRId64 "} }",
+                 id);
     QDECREF(ret);
-    g_free(cmd);
 }
 
 static void test_qga_file_write_read(gconstpointer fix)
@@ -541,7 +530,7 @@ static void test_qga_file_write_read(gconstpointer fix)
     const TestFixture *fixture = fix;
     const unsigned char helloworld[] = "Hello World!\n";
     const char *b64;
-    gchar *cmd, *enc;
+    gchar *enc;
     QDict *ret, *val;
     int64_t id, eof;
     gsize count;
@@ -556,10 +545,10 @@ static void test_qga_file_write_read(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 } }", id, enc);
     g_assert_nonnull(ret);
     qmp_assert_no_error(ret);
 
@@ -569,13 +558,12 @@ static void test_qga_file_write_read(gconstpointer fix)
     g_assert_cmpint(count, ==, sizeof(helloworld));
     g_assert_cmpint(eof, ==, 0);
     QDECREF(ret);
-    g_free(cmd);
 
     /* read (check implicit flush) */
-    cmd = g_strdup_printf("{'execute': 'guest-file-read',"
-                          " 'arguments': { 'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-read',"
+                 " 'arguments': { 'handle': %" PRId64 "} }",
+                 id);
     val = qdict_get_qdict(ret, "return");
     count = qdict_get_int(val, "count");
     eof = qdict_get_bool(val, "eof");
@@ -584,14 +572,13 @@ static void test_qga_file_write_read(gconstpointer fix)
     g_assert(eof);
     g_assert_cmpstr(b64, ==, "");
     QDECREF(ret);
-    g_free(cmd);
 
     /* seek to 0 */
-    cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
-                          " 'arguments': { 'handle': %" PRId64 ", "
-                          " 'offset': %d, 'whence': '%s' } }",
-                          id, 0, "set");
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-seek',"
+                 " 'arguments': { 'handle': %" PRId64 ", "
+                 " 'offset': %d, 'whence': %s } }",
+                 id, 0, "set");
     qmp_assert_no_error(ret);
     val = qdict_get_qdict(ret, "return");
     count = qdict_get_int(val, "position");
@@ -599,13 +586,12 @@ static void test_qga_file_write_read(gconstpointer fix)
     g_assert_cmpint(count, ==, 0);
     g_assert(!eof);
     QDECREF(ret);
-    g_free(cmd);
 
     /* read */
-    cmd = g_strdup_printf("{'execute': 'guest-file-read',"
-                          " 'arguments': { 'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-read',"
+                 " 'arguments': { 'handle': %" PRId64 "} }",
+                 id);
     val = qdict_get_qdict(ret, "return");
     count = qdict_get_int(val, "count");
     eof = qdict_get_bool(val, "eof");
@@ -614,16 +600,14 @@ static void test_qga_file_write_read(gconstpointer fix)
     g_assert(eof);
     g_assert_cmpstr(b64, ==, enc);
     QDECREF(ret);
-    g_free(cmd);
     g_free(enc);
 
     /* close */
-    cmd = g_strdup_printf("{'execute': 'guest-file-close',"
-                          " 'arguments': {'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-close',"
+                 " 'arguments': {'handle': %" PRId64 "} }",
+                 id);
     QDECREF(ret);
-    g_free(cmd);
 }
 
 static void test_qga_get_time(gconstpointer fix)
@@ -647,7 +631,6 @@ static void test_qga_set_time(gconstpointer fix)
     const TestFixture *fixture = fix;
     QDict *ret;
     int64_t current, time;
-    gchar *cmd;
 
     /* get current time */
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-time'}");
@@ -673,11 +656,10 @@ static void test_qga_set_time(gconstpointer fix)
     QDECREF(ret);
 
     /* set back current time */
-    cmd = g_strdup_printf("{'execute': 'guest-set-time',"
-                          " 'arguments': { 'time': %" PRId64 " } }",
-                          current + time * 1000);
-    ret = qmp_fd(fixture->fd, cmd);
-    g_free(cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-set-time',"
+                 " 'arguments': { 'time': %" PRId64 " } }",
+                 current + time * 1000);
     g_assert_nonnull(ret);
     qmp_assert_no_error(ret);
     QDECREF(ret);
@@ -864,7 +846,6 @@ static void test_qga_guest_exec(gconstpointer fix)
     int64_t pid, now, exitcode;
     gsize len;
     bool exited;
-    char *cmd;
 
     /* exec 'echo foo bar' */
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
@@ -879,10 +860,10 @@ static void test_qga_guest_exec(gconstpointer fix)
 
     /* wait for completion */
     now = g_get_monotonic_time();
-    cmd = g_strdup_printf("{'execute': 'guest-exec-status',"
-                          " 'arguments': { 'pid': %" PRId64 " } }", pid);
     do {
-        ret = qmp_fd(fixture->fd, cmd);
+        ret = qmp_fd(fixture->fd,
+                     "{'execute': 'guest-exec-status',"
+                     " 'arguments': { 'pid': %" PRId64 " } }", pid);
         g_assert_nonnull(ret);
         val = qdict_get_qdict(ret, "return");
         exited = qdict_get_bool(val, "exited");
@@ -892,7 +873,6 @@ static void test_qga_guest_exec(gconstpointer fix)
     } while (!exited &&
              g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
     g_assert(exited);
-    g_free(cmd);
 
     /* check stdout */
     exitcode = qdict_get_int(val, "exitcode");
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index d4da09f..f2a2b6c 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -662,11 +662,7 @@ static void test_migrate(void)
     g_assert(qdict_haskey(rsp, "return"));
     QDECREF(rsp);
 
-    cmd = g_strdup_printf("{ 'execute': 'migrate',"
-                          "'arguments': { 'uri': '%s' } }",
-                          uri);
-    rsp = qmp(cmd);
-    g_free(cmd);
+    rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
     g_assert(qdict_haskey(rsp, "return"));
     QDECREF(rsp);
 
-- 
2.7.5

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3155 bytes --]

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 <armbru@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> 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 <armbru@redhat.com>
>
>> ---
>> +    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.

Yes.  We really need %u, or else parse_escape() would call
qnum_from_int() instead of qnum_from_uint().

I think the doc fix for json-lexer.c can be squashed in.  Keeping it
separate could simplify commit message writing, though.  Your choice.

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

I forgot that PRId64 expands into non-standard crap on some systems.

Options:

1. Outlaw use of PRI macros, limit integer length modifiers for
   conversion specifiers "d" and "u" to "l" and "ll".  When PRI macros
   creep in, the build breaks on hosts where they expand to anything
   else (hopefully, our CI will catch that).  Interpolating int64_t
   values become a bit awkward.

   Required work: fix my patches not to use PRId64, drop support for
   length modifier "I64" from parse_escape().

2. Support PRId64 and PRIu64, whatever their actual value may be.

   a. Support all possible values.  This is what we've tried before.
      Nasty: fails only at run-time on hosts with sufficiently creative
      values.

      Required work: add support for length modifier "q", to unbreak
      MacOS.

      Optional work: try to turn the run-time failure into a compile-
      time failure, ideally in configure.

   b. Support exactly the host's PRId64 and PRIu64 values.

      Work required: replace support of "I64d" and "I64u" by support of
      PRId64 and PRIu64.  Easy enough in parse_escape(), but not in
      json-lexer.c.  We could perhaps have the lexer accept the shortest
      string that's followed by a conversion specifier as length
      modifier, then reject invalid length modifiers in a semantic
      action.

   Optional work: try to simplify code that currently works around
   unavailability of PRId64 and PRIu64.

Preferences?

I like 2b, but I'm not sure I'll like the code implementing it.  One way
to find out...

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

Yes.  

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3233 bytes --]

On 07/21/2017 11:48 AM, Markus Armbruster wrote:
> I forgot that PRId64 expands into non-standard crap on some systems.
> 
> Options:
> 
> 1. Outlaw use of PRI macros, limit integer length modifiers for
>    conversion specifiers "d" and "u" to "l" and "ll".  When PRI macros
>    creep in, the build breaks on hosts where they expand to anything
>    else (hopefully, our CI will catch that).  Interpolating int64_t
>    values become a bit awkward.
> 
>    Required work: fix my patches not to use PRId64, drop support for
>    length modifier "I64" from parse_escape().

Yuck.  (But motivation for my earlier series to nuke qobject_from_jsonf())

> 
> 2. Support PRId64 and PRIu64, whatever their actual value may be.
> 
>    a. Support all possible values.  This is what we've tried before.
>       Nasty: fails only at run-time on hosts with sufficiently creative
>       values.
> 
>       Required work: add support for length modifier "q", to unbreak
>       MacOS.
> 
>       Optional work: try to turn the run-time failure into a compile-
>       time failure, ideally in configure.

Accepts garbage (although -Wformat detection will help avoid the worst
of it).  You can't check string equality in the preprocessor, and you
can't do things like "case PRId64[0]: case 'l':" to force compilation
errors due to duplicate labels, but we CAN limit ourselves to the
preprocessor and grep that output to see what PRId64 expands to.  I'll
see if I can come up with a configure check.  Still, it may be easier to
just fail configure and tell people to report the bug on their odd libc,
at which point we update json-lexer.c and configure, than it is to try
and reuse configure results in json-lexer.c (since the PRId64 string is
not a constant width, it gets hard to code an exact value into our lexer
state machine, but easier to code every reported value).

> 
>    b. Support exactly the host's PRId64 and PRIu64 values.
> 
>       Work required: replace support of "I64d" and "I64u" by support of
>       PRId64 and PRIu64.  Easy enough in parse_escape(), but not in
>       json-lexer.c.  We could perhaps have the lexer accept the shortest
>       string that's followed by a conversion specifier as length
>       modifier, then reject invalid length modifiers in a semantic
>       action.

Interesting idea. I'm playing with it...

> 
>    Optional work: try to simplify code that currently works around
>    unavailability of PRId64 and PRIu64.
> 
> Preferences?
> 
> I like 2b, but I'm not sure I'll like the code implementing it.  One way
> to find out...

In the lexer, widen the state machine to accept up to three unknown
characters between % and d.  Hmm, there's also the possibility of
int64_t being mapped to %jd, in addition to our known culprits of %ld,
%lld, %I64d, and %qd.

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

At this point, I think I'll spin the next round. But it's not longer a
2.10 priority, so it may be a few days.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 935 bytes --]

On 07/21/2017 03:08 PM, Eric Blake wrote:
>> 2. Support PRId64 and PRIu64, whatever their actual value may be.
>>
>>    a. Support all possible values.  This is what we've tried before.

>>
>>    b. Support exactly the host's PRId64 and PRIu64 values.

>> Preferences?
>>
>> I like 2b, but I'm not sure I'll like the code implementing it.  One way
>> to find out...

I ended up implementing 2a in the C code, but 2b in the configure check
to make sure our C code gets updated if we ever encounter any more
oddballs.  Of course, I don't have easy access to Mac OS, so I'm relying
on others to test the patch for me; but once we get that patch going to
our liking, I can rebase the rest of this series on top of it.

https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg07039.html

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/21/2017 11:48 AM, Markus Armbruster wrote:
>> I forgot that PRId64 expands into non-standard crap on some systems.
>> 
>> Options:
>> 
>> 1. Outlaw use of PRI macros, limit integer length modifiers for
>>    conversion specifiers "d" and "u" to "l" and "ll".  When PRI macros
>>    creep in, the build breaks on hosts where they expand to anything
>>    else (hopefully, our CI will catch that).  Interpolating int64_t
>>    values become a bit awkward.
>> 
>>    Required work: fix my patches not to use PRId64, drop support for
>>    length modifier "I64" from parse_escape().
>
> Yuck.  (But motivation for my earlier series to nuke qobject_from_jsonf())
>
>> 
>> 2. Support PRId64 and PRIu64, whatever their actual value may be.
>> 
>>    a. Support all possible values.  This is what we've tried before.

Clarification: all the values we expect on the hosts we support.
Currently "l", "ll", "I64" and "q".

>>       Nasty: fails only at run-time on hosts with sufficiently creative
>>       values.
>> 
>>       Required work: add support for length modifier "q", to unbreak
>>       MacOS.

We already support the other three.

>>       Optional work: try to turn the run-time failure into a compile-
>>       time failure, ideally in configure.
>
> Accepts garbage (although -Wformat detection will help avoid the worst
> of it).  You can't check string equality in the preprocessor, and you
> can't do things like "case PRId64[0]: case 'l':" to force compilation
> errors due to duplicate labels, but we CAN limit ourselves to the
> preprocessor and grep that output to see what PRId64 expands to.  I'll
> see if I can come up with a configure check.  Still, it may be easier to
> just fail configure and tell people to report the bug on their odd libc,
> at which point we update json-lexer.c and configure, than it is to try
> and reuse configure results in json-lexer.c (since the PRId64 string is
> not a constant width, it gets hard to code an exact value into our lexer
> state machine, but easier to code every reported value).

Auto-configuring our handwritten lexer to recognize PRId64 and PRIu64
would be awkward.  Certainly more awkward than substituting an
auto-configured string into the input of flex.  It's 2b anyway.

>>    b. Support exactly the host's PRId64 and PRIu64 values.
>> 
>>       Work required: replace support of "I64d" and "I64u" by support of
>>       PRId64 and PRIu64.  Easy enough in parse_escape(), but not in
>>       json-lexer.c.  We could perhaps have the lexer accept the shortest
>>       string that's followed by a conversion specifier as length
>>       modifier, then reject invalid length modifiers in a semantic
>>       action.
>
> Interesting idea. I'm playing with it...
>
>> 
>>    Optional work: try to simplify code that currently works around
>>    unavailability of PRId64 and PRIu64.
>> 
>> Preferences?
>> 
>> I like 2b, but I'm not sure I'll like the code implementing it.  One way
>> to find out...
>
> In the lexer, widen the state machine to accept up to three unknown
> characters between % and d.  Hmm, there's also the possibility of
> int64_t being mapped to %jd, in addition to our known culprits of %ld,
> %lld, %I64d, and %qd.

The lexer could accept any length modifier matching a suitable regular
expression, leaving rejection of invalid ones to semantic actions.

The only hard restriction on non-standard length modifiers is that the
resulting conversion specifications must be syntactically unambiguous,
and the ones the standard specifies still mean the same.

Example: length modifier "f" to conversion specifier "d" is impossible,
because "%fd" must be parsed as conversion specifier "%f" followed by
ordinary character "d".

Example: similarly, length modifiers starting with a flag character, a
digit, '.' or '*' are not possible because such a character must be
parsed as flag, minimum field width or precision, respectively.

In practice, length modifiers starting with characters used elsewhere in
conversion specifications, or containing conversion specifier characters
would be too confusing.

Example: length modifier "qp" to conversion specifier "d" could
technically be done as long as "q" isn't also made a length modifier to
"p".  But it would be nuts.

That leaves us with the regular expression
[^-+ #0-9*.diouxXfFeEgGaAcspn%][^diouxXfFeEgGaAcspn%]*

I think we can rule out control characters, too.

>>> Overall, I like the patch, but we need to fix the problems for the next
>>> round of this series.
>> 
>> Yes.  
>
> At this point, I think I'll spin the next round. But it's not longer a
> 2.10 priority, so it may be a few days.

No need to rush this work into 2.10.

[Qemu-devel] [PATCH 5/9] tests/libqos/usb: Clean up string interpolation into QMP input

[Markus Armbruster]

Leaving interpolation into JSON to qmp() is more robust than building
QMP input manually, as explained in the previous commit.

The case in usb_test_hotplug() slightly more complicated: it
interpolates *into* JSON values.  Clean it up by building the values
separately, so we can again leave interpolation to qmp().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/libqos/usb.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
index 0cdfaec..f88d4a6 100644
--- a/tests/libqos/usb.c
+++ b/tests/libqos/usb.c
@@ -40,18 +40,20 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
 void usb_test_hotplug(const char *hcd_id, const int port,
                       void (*port_check)(void))
 {
+    char id[32];
+    char *bus;
     QDict *response;
-    char  *cmd;
 
-    cmd = g_strdup_printf("{'execute': 'device_add',"
-                          " 'arguments': {"
-                          "   'driver': 'usb-tablet',"
-                          "   'port': '%d',"
-                          "   'bus': '%s.0',"
-                          "   'id': 'usbdev%d'"
-                          "}}", port, hcd_id, port);
-    response = qmp(cmd);
-    g_free(cmd);
+    sprintf(id, "usbdev%d", port);
+    bus = g_strdup_printf("%s.0", hcd_id);
+    response = qmp("{'execute': 'device_add',"
+                   " 'arguments': {"
+                   "   'driver': 'usb-tablet',"
+                   "   'port': %s,"
+                   "   'bus': %s,"
+                   "   'id': %s"
+                   " }}", id + 6, bus, id);
+    g_free(bus);
     g_assert(response);
     g_assert(!qdict_haskey(response, "error"));
     QDECREF(response);
@@ -60,12 +62,8 @@ void usb_test_hotplug(const char *hcd_id, const int port,
         port_check();
     }
 
-    cmd = g_strdup_printf("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'usbdev%d'"
-                           "}}", port);
-    response = qmp(cmd);
-    g_free(cmd);
+    response = qmp("{'execute': 'device_del', 'arguments': { 'id': %s }}",
+                   id);
     g_assert(response);
     g_assert(qdict_haskey(response, "event"));
     g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-- 
2.7.5

Re: [Qemu-devel] [PATCH 5/9] tests/libqos/usb: Clean up string interpolation into QMP input

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1830 bytes --]

On 07/21/2017 08:53 AM, Markus Armbruster wrote:
> Leaving interpolation into JSON to qmp() is more robust than building
> QMP input manually, as explained in the previous commit.
> 
> The case in usb_test_hotplug() slightly more complicated: it

s/()/() is/

> interpolates *into* JSON values.  Clean it up by building the values
> separately, so we can again leave interpolation to qmp().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/libqos/usb.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
> index 0cdfaec..f88d4a6 100644
> --- a/tests/libqos/usb.c
> +++ b/tests/libqos/usb.c
> @@ -40,18 +40,20 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
>  void usb_test_hotplug(const char *hcd_id, const int port,
>                        void (*port_check)(void))
>  {
> +    char id[32];
> +    char *bus;
>      QDict *response;
> -    char  *cmd;
>  
> -    cmd = g_strdup_printf("{'execute': 'device_add',"
> -                          " 'arguments': {"
> -                          "   'driver': 'usb-tablet',"
> -                          "   'port': '%d',"
> -                          "   'bus': '%s.0',"
> -                          "   'id': 'usbdev%d'"
> -                          "}}", port, hcd_id, port);
> -    response = qmp(cmd);
> -    g_free(cmd);
> +    sprintf(id, "usbdev%d", port);

I know this fits, but do any of our compilers issue dumb warnings about
possible buffer overflow?  I guess we'll deal with that if someone
reports a problem.

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 5/9] tests/libqos/usb: Clean up string interpolation into QMP input

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/21/2017 08:53 AM, Markus Armbruster wrote:
>> Leaving interpolation into JSON to qmp() is more robust than building
>> QMP input manually, as explained in the previous commit.
>> 
>> The case in usb_test_hotplug() slightly more complicated: it
>
> s/()/() is/

Will fix.

>> interpolates *into* JSON values.  Clean it up by building the values
>> separately, so we can again leave interpolation to qmp().
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/libqos/usb.c | 30 ++++++++++++++----------------
>>  1 file changed, 14 insertions(+), 16 deletions(-)
>> 
>> diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
>> index 0cdfaec..f88d4a6 100644
>> --- a/tests/libqos/usb.c
>> +++ b/tests/libqos/usb.c
>> @@ -40,18 +40,20 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
>>  void usb_test_hotplug(const char *hcd_id, const int port,
>>                        void (*port_check)(void))
>>  {
>> +    char id[32];
>> +    char *bus;
>>      QDict *response;
>> -    char  *cmd;
>>  
>> -    cmd = g_strdup_printf("{'execute': 'device_add',"
>> -                          " 'arguments': {"
>> -                          "   'driver': 'usb-tablet',"
>> -                          "   'port': '%d',"
>> -                          "   'bus': '%s.0',"
>> -                          "   'id': 'usbdev%d'"
>> -                          "}}", port, hcd_id, port);
>> -    response = qmp(cmd);
>> -    g_free(cmd);
>> +    sprintf(id, "usbdev%d", port);
>
> I know this fits, but do any of our compilers issue dumb warnings about
> possible buffer overflow?  I guess we'll deal with that if someone
> reports a problem.

We do similar things elsewhere, just grep for sprintf.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 5/9] tests/libqos/usb: Clean up string interpolation into QMP input

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1927 bytes --]

On Fri, Jul 21, 2017 at 03:53:22PM +0200, Markus Armbruster wrote:
> Leaving interpolation into JSON to qmp() is more robust than building
> QMP input manually, as explained in the previous commit.
> 
> The case in usb_test_hotplug() slightly more complicated: it
> interpolates *into* JSON values.  Clean it up by building the values
> separately, so we can again leave interpolation to qmp().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/libqos/usb.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
> index 0cdfaec..f88d4a6 100644
> --- a/tests/libqos/usb.c
> +++ b/tests/libqos/usb.c
> @@ -40,18 +40,20 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
>  void usb_test_hotplug(const char *hcd_id, const int port,
>                        void (*port_check)(void))
>  {
> +    char id[32];
> +    char *bus;
>      QDict *response;
> -    char  *cmd;
>  
> -    cmd = g_strdup_printf("{'execute': 'device_add',"
> -                          " 'arguments': {"
> -                          "   'driver': 'usb-tablet',"
> -                          "   'port': '%d',"
> -                          "   'bus': '%s.0',"
> -                          "   'id': 'usbdev%d'"
> -                          "}}", port, hcd_id, port);
> -    response = qmp(cmd);
> -    g_free(cmd);
> +    sprintf(id, "usbdev%d", port);
> +    bus = g_strdup_printf("%s.0", hcd_id);
> +    response = qmp("{'execute': 'device_add',"
> +                   " 'arguments': {"
> +                   "   'driver': 'usb-tablet',"
> +                   "   'port': %s,"
> +                   "   'bus': %s,"
> +                   "   'id': %s"
> +                   " }}", id + 6, bus, id);

Or: id + strlen("usbdev")

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 6/9] tests/libqos/pci: Clean up string interpolation into QMP input

[Markus Armbruster]

Leaving interpolation into JSON to qmp() is more robust than building
QMP input manually, as explained in the commit before previous.

The case in qpci_plug_device_test() is a bit complicated: it
interpolates several JSON object members, not just a value.  Clean it
up by passing them in as QDict rather than string, so we can leave
interpolation to qmp() here and to qobject_from_jsonf() in callers.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/ivshmem-test.c    | 10 +++++-----
 tests/libqos/pci.c      | 33 ++++++++++++++++++---------------
 tests/libqos/pci.h      |  2 +-
 tests/virtio-blk-test.c |  5 ++++-
 4 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
index 3776342..38044bb 100644
--- a/tests/ivshmem-test.c
+++ b/tests/ivshmem-test.c
@@ -14,6 +14,7 @@
 #include "libqos/libqos-pc.h"
 #include "libqos/libqos-spapr.h"
 #include "libqtest.h"
+#include "qapi/qmp/qjson.h"
 #include "qemu-common.h"
 
 #define TMPSHMSIZE (1 << 20)
@@ -419,19 +420,18 @@ static void test_ivshmem_server_irq(void)
 static void test_ivshmem_hotplug(void)
 {
     const char *arch = qtest_get_arch();
-    gchar *opts;
+    QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
+                                             tmpshm);
 
     qtest_start("");
 
-    opts = g_strdup_printf("'shm': '%s', 'size': '1M'", tmpshm);
-
-    qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts);
+    qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP,
+                          qobject_to_qdict(extra_args));
     if (strcmp(arch, "ppc64") != 0) {
         qpci_unplug_acpi_device_test("iv1", PCI_SLOT_HP);
     }
 
     qtest_end();
-    g_free(opts);
 }
 
 static void test_ivshmem_memdev(void)
diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
index 2dcdead..4068305 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -14,6 +14,7 @@
 #include "libqos/pci.h"
 
 #include "hw/pci/pci_regs.h"
+#include "qapi/qmp/qjson.h"
 #include "qemu/host-utils.h"
 
 void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
@@ -392,23 +393,25 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
 }
 
 void qpci_plug_device_test(const char *driver, const char *id,
-                           uint8_t slot, const char *opts)
+                           uint8_t slot, QDict *extra_args)
 {
-    QDict *response;
-    char *cmd;
-
-    cmd = g_strdup_printf("{'execute': 'device_add',"
-                          " 'arguments': {"
-                          "   'driver': '%s',"
-                          "   'addr': '%d',"
-                          "   %s%s"
-                          "   'id': '%s'"
-                          "}}", driver, slot,
-                          opts ? opts : "", opts ? "," : "",
-                          id);
-    response = qmp(cmd);
-    g_free(cmd);
+    char addr[8];
+    QDict *args, *response;
+
+    sprintf(addr, "%d", slot);
+    args = qobject_to_qdict(
+        qobject_from_jsonf("{ 'driver': %s, 'addr': %s, 'id': %s}",
+                           driver, addr, id));
+
+    if (extra_args) {
+        qdict_join(args, extra_args, true);
+        QDECREF(extra_args);
+    }
+
+    response = qmp("{'execute': 'device_add', 'arguments': %p }", args);
+
     g_assert(response);
     g_assert(!qdict_haskey(response, "error"));
+    QDECREF(args);
     QDECREF(response);
 }
diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
index ed48061..c981061 100644
--- a/tests/libqos/pci.h
+++ b/tests/libqos/pci.h
@@ -109,6 +109,6 @@ void qpci_iounmap(QPCIDevice *dev, QPCIBar addr);
 QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr);
 
 void qpci_plug_device_test(const char *driver, const char *id,
-                           uint8_t slot, const char *opts);
+                           uint8_t slot, QDict *extra_args);
 void qpci_unplug_acpi_device_test(const char *id, uint8_t slot);
 #endif
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 0576cb1..64a48f4 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -16,6 +16,7 @@
 #include "libqos/virtio-pci.h"
 #include "libqos/virtio-mmio.h"
 #include "libqos/malloc-generic.h"
+#include "qapi/qmp/qjson.h"
 #include "qemu/bswap.h"
 #include "standard-headers/linux/virtio_ids.h"
 #include "standard-headers/linux/virtio_config.h"
@@ -658,12 +659,13 @@ static void pci_hotplug(void)
     QVirtioPCIDevice *dev;
     QOSState *qs;
     const char *arch = qtest_get_arch();
+    QObject *extra_args = qobject_from_jsonf("{ 'drive': 'drive1' }");
 
     qs = pci_test_start();
 
     /* plug secondary disk */
     qpci_plug_device_test("virtio-blk-pci", "drv1", PCI_SLOT_HP,
-                          "'drive': 'drive1'");
+                          qobject_to_qdict(extra_args));
 
     dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT_HP);
     g_assert(dev);
@@ -674,6 +676,7 @@ static void pci_hotplug(void)
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         qpci_unplug_acpi_device_test("drv1", PCI_SLOT_HP);
     }
+
     qtest_shutdown(qs);
 }
 
-- 
2.7.5

Re: [Qemu-devel] [PATCH 6/9] tests/libqos/pci: Clean up string interpolation into QMP input

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1353 bytes --]

On 07/21/2017 08:53 AM, Markus Armbruster wrote:
> Leaving interpolation into JSON to qmp() is more robust than building
> QMP input manually, as explained in the commit before previous.
> 
> The case in qpci_plug_device_test() is a bit complicated: it
> interpolates several JSON object members, not just a value.  Clean it
> up by passing them in as QDict rather than string, so we can leave
> interpolation to qmp() here and to qobject_from_jsonf() in callers.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> 
> +    response = qmp("{'execute': 'device_add', 'arguments': %p }", args);
> +

I like this; in fact, in my earlier series attempting to remove
qobject_from_jsonf(), I heavily relied on %p being useful, to the point
that I even added a helper function to make it easier (off-hand, it was
something like qmp_execute(const char *command, QDict *arguments))

> @@ -674,6 +676,7 @@ static void pci_hotplug(void)
>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>          qpci_unplug_acpi_device_test("drv1", PCI_SLOT_HP);
>      }
> +
>      qtest_shutdown(qs);

Spurious whitespace addition?

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 6/9] tests/libqos/pci: Clean up string interpolation into QMP input

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/21/2017 08:53 AM, Markus Armbruster wrote:
>> Leaving interpolation into JSON to qmp() is more robust than building
>> QMP input manually, as explained in the commit before previous.
>> 
>> The case in qpci_plug_device_test() is a bit complicated: it
>> interpolates several JSON object members, not just a value.  Clean it
>> up by passing them in as QDict rather than string, so we can leave
>> interpolation to qmp() here and to qobject_from_jsonf() in callers.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> 
>> +    response = qmp("{'execute': 'device_add', 'arguments': %p }", args);
>> +
>
> I like this; in fact, in my earlier series attempting to remove
> qobject_from_jsonf(), I heavily relied on %p being useful, to the point
> that I even added a helper function to make it easier (off-hand, it was
> something like qmp_execute(const char *command, QDict *arguments))
>
>> @@ -674,6 +676,7 @@ static void pci_hotplug(void)
>>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>>          qpci_unplug_acpi_device_test("drv1", PCI_SLOT_HP);
>>      }
>> +
>>      qtest_shutdown(qs);
>
> Spurious whitespace addition?

Is it spurious if the result looks better?  ;)

There's a blank line after pci_test_start(), and that makes me want one
before the matching qtest_shutdown().

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 6/9] tests/libqos/pci: Clean up string interpolation into QMP input

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

On Fri, Jul 21, 2017 at 03:53:23PM +0200, Markus Armbruster wrote:
> Leaving interpolation into JSON to qmp() is more robust than building
> QMP input manually, as explained in the commit before previous.
> 
> The case in qpci_plug_device_test() is a bit complicated: it
> interpolates several JSON object members, not just a value.  Clean it
> up by passing them in as QDict rather than string, so we can leave
> interpolation to qmp() here and to qobject_from_jsonf() in callers.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/ivshmem-test.c    | 10 +++++-----
>  tests/libqos/pci.c      | 33 ++++++++++++++++++---------------
>  tests/libqos/pci.h      |  2 +-
>  tests/virtio-blk-test.c |  5 ++++-
>  4 files changed, 28 insertions(+), 22 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 6/9] tests/libqos/pci: Clean up string interpolation into QMP input

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

On 07/21/2017 08:53 AM, Markus Armbruster wrote:
> Leaving interpolation into JSON to qmp() is more robust than building
> QMP input manually, as explained in the commit before previous.
> 
> The case in qpci_plug_device_test() is a bit complicated: it
> interpolates several JSON object members, not just a value.  Clean it
> up by passing them in as QDict rather than string, so we can leave
> interpolation to qmp() here and to qobject_from_jsonf() in callers.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> +    QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
> +                                             tmpshm);
>  
>      qtest_start("");
>  
> -    opts = g_strdup_printf("'shm': '%s', 'size': '1M'", tmpshm);
> -
> -    qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts);

Wait, 'size':'1M' works?  I guess because it's a string, rather than a
JSON number.  One of our intentional design choices in QMP was to
represent everything as bytes, rather than as suffixed numbers, since
machine-generated code can easily generate bytes anywhere that humans
prefer a suffixed number.  But as you are not changing this interface,
but merely refactoring how it is tested, it's more of a side comment
than something that affects review.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 6/9] tests/libqos/pci: Clean up string interpolation into QMP input

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/21/2017 08:53 AM, Markus Armbruster wrote:
>> Leaving interpolation into JSON to qmp() is more robust than building
>> QMP input manually, as explained in the commit before previous.
>> 
>> The case in qpci_plug_device_test() is a bit complicated: it
>> interpolates several JSON object members, not just a value.  Clean it
>> up by passing them in as QDict rather than string, so we can leave
>> interpolation to qmp() here and to qobject_from_jsonf() in callers.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> +    QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
>> +                                             tmpshm);
>>  
>>      qtest_start("");
>>  
>> -    opts = g_strdup_printf("'shm': '%s', 'size': '1M'", tmpshm);
>> -
>> -    qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts);
>
> Wait, 'size':'1M' works?  I guess because it's a string, rather than a
> JSON number.  One of our intentional design choices in QMP was to
> represent everything as bytes, rather than as suffixed numbers, since
> machine-generated code can easily generate bytes anywhere that humans
> prefer a suffixed number.  But as you are not changing this interface,
> but merely refactoring how it is tested, it's more of a side comment
> than something that affects review.

Design mistake in legacy device "ivshmem".  Fixed in "ivshmem-plain" and
"ivshmem-doorbell", see commit 5400c02.

[Qemu-devel] [PATCH 7/9] tests: Clean up wait for event

[Markus Armbruster]

We still use hacks like qmp("") to wait for an event, even though we
have qmp_eventwait() since commit 8fe941f, and qmp_eventwait_ref()
since commit 7ffe312.  Both commits neglected to convert all the
existing hacks.  Make up what they missed.

Bonus: gets rid of empty format strings.  A step towards compile-time
format string checking without triggering -Wformat-zero-length.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/boot-order-test.c   |  2 +-
 tests/libqos/pci-pc.c     |  6 +-----
 tests/tco-test.c          |  3 +--
 tests/usb-hcd-uhci-test.c |  6 +-----
 tests/usb-hcd-xhci-test.c | 12 ++----------
 tests/wdt_ib700-test.c    | 35 ++++++++++-------------------------
 6 files changed, 16 insertions(+), 48 deletions(-)

diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index fc1e794..9d51683 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -43,7 +43,7 @@ static void test_a_boot_order(const char *machine,
      * system_reset only requests reset.  We get a RESET event after
      * the actual reset completes.  Need to wait for that.
      */
-    qmp_discard_response("");   /* HACK: wait for event */
+    qmp_eventwait("RESET");
     actual = read_boot_order();
     g_assert_cmphex(actual, ==, expected_reboot);
     qtest_quit(global_qtest);
diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
index d40aa9d..e4be809 100644
--- a/tests/libqos/pci-pc.c
+++ b/tests/libqos/pci-pc.c
@@ -168,9 +168,5 @@ void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
 
     outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot);
 
-    response = qmp("");
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qmp_eventwait("DEVICE_DELETED");
 }
diff --git a/tests/tco-test.c b/tests/tco-test.c
index c4c264e..f2ed6ed 100644
--- a/tests/tco-test.c
+++ b/tests/tco-test.c
@@ -237,9 +237,8 @@ static void test_tco_max_timeout(void)
 
 static QDict *get_watchdog_action(void)
 {
-    QDict *ev = qmp("");
+    QDict *ev = qmp_eventwait_ref("WATCHDOG");
     QDict *data;
-    g_assert(!strcmp(qdict_get_str(ev, "event"), "WATCHDOG"));
 
     data = qdict_get_qdict(ev, "data");
     QINCREF(data);
diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index 5b500fe..0fb7f8d 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -68,11 +68,7 @@ static void test_usb_storage_hotplug(void)
     g_assert(!qdict_haskey(response, "error"));
     QDECREF(response);
 
-    response = qmp("");
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qmp_eventwait("DEVICE_DELETED");
 }
 
 int main(int argc, char **argv)
diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c
index 031764d..c05a339 100644
--- a/tests/usb-hcd-xhci-test.c
+++ b/tests/usb-hcd-xhci-test.c
@@ -57,11 +57,7 @@ static void test_usb_uas_hotplug(void)
     g_assert(!qdict_haskey(response, "error"));
     QDECREF(response);
 
-    response = qmp("");
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
-
+    qmp_eventwait("DEVICE_DELETED");
 
     response = qmp("{'execute': 'device_del',"
                            " 'arguments': {"
@@ -71,11 +67,7 @@ static void test_usb_uas_hotplug(void)
     g_assert(!qdict_haskey(response, "error"));
     QDECREF(response);
 
-    response = qmp("");
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qmp_eventwait("DEVICE_DELETED");
 }
 
 int main(int argc, char **argv)
diff --git a/tests/wdt_ib700-test.c b/tests/wdt_ib700-test.c
index 49f4f0c..4fc8eea 100644
--- a/tests/wdt_ib700-test.c
+++ b/tests/wdt_ib700-test.c
@@ -18,26 +18,10 @@ static void qmp_check_no_event(void)
     QDECREF(resp);
 }
 
-static QDict *qmp_get_event(const char *name)
-{
-    QDict *event = qmp("");
-    QDict *data;
-    g_assert(qdict_haskey(event, "event"));
-    g_assert(!strcmp(qdict_get_str(event, "event"), name));
-
-    if (qdict_haskey(event, "data")) {
-        data = qdict_get_qdict(event, "data");
-        QINCREF(data);
-    } else {
-        data = NULL;
-    }
-
-    QDECREF(event);
-    return data;
-}
-
 static QDict *ib700_program_and_wait(QTestState *s)
 {
+    QDict *event, *data;
+
     clock_step(NANOSECONDS_PER_SECOND * 40);
     qmp_check_no_event();
 
@@ -61,7 +45,11 @@ static QDict *ib700_program_and_wait(QTestState *s)
     clock_step(3 * NANOSECONDS_PER_SECOND);
     qmp_check_no_event();
     clock_step(2 * NANOSECONDS_PER_SECOND);
-    return qmp_get_event("WATCHDOG");
+    event = qmp_eventwait_ref("WATCHDOG");
+    data = qdict_get_qdict(event, "data");
+    QINCREF(data);
+    QDECREF(event);
+    return data;
 }
 
 
@@ -73,8 +61,7 @@ static void ib700_pause(void)
     d = ib700_program_and_wait(s);
     g_assert(!strcmp(qdict_get_str(d, "action"), "pause"));
     QDECREF(d);
-    d = qmp_get_event("STOP");
-    QDECREF(d);
+    qmp_eventwait("STOP");
     qtest_end();
 }
 
@@ -86,8 +73,7 @@ static void ib700_reset(void)
     d = ib700_program_and_wait(s);
     g_assert(!strcmp(qdict_get_str(d, "action"), "reset"));
     QDECREF(d);
-    d = qmp_get_event("RESET");
-    QDECREF(d);
+    qmp_eventwait("RESET");
     qtest_end();
 }
 
@@ -99,8 +85,7 @@ static void ib700_shutdown(void)
     d = ib700_program_and_wait(s);
     g_assert(!strcmp(qdict_get_str(d, "action"), "reset"));
     QDECREF(d);
-    d = qmp_get_event("SHUTDOWN");
-    QDECREF(d);
+    qmp_eventwait("SHUTDOWN");
     qtest_end();
 }
 
-- 
2.7.5

Re: [Qemu-devel] [PATCH 7/9] tests: Clean up wait for event

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

On 07/21/2017 08:53 AM, Markus Armbruster wrote:
> We still use hacks like qmp("") to wait for an event, even though we
> have qmp_eventwait() since commit 8fe941f, and qmp_eventwait_ref()
> since commit 7ffe312.  Both commits neglected to convert all the
> existing hacks.  Make up what they missed.
> 
> Bonus: gets rid of empty format strings.  A step towards compile-time
> format string checking without triggering -Wformat-zero-length.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 7/9] tests: Clean up wait for event

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 933 bytes --]

On Fri, Jul 21, 2017 at 03:53:24PM +0200, Markus Armbruster wrote:
> We still use hacks like qmp("") to wait for an event, even though we
> have qmp_eventwait() since commit 8fe941f, and qmp_eventwait_ref()
> since commit 7ffe312.  Both commits neglected to convert all the
> existing hacks.  Make up what they missed.
> 
> Bonus: gets rid of empty format strings.  A step towards compile-time
> format string checking without triggering -Wformat-zero-length.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/boot-order-test.c   |  2 +-
>  tests/libqos/pci-pc.c     |  6 +-----
>  tests/tco-test.c          |  3 +--
>  tests/usb-hcd-uhci-test.c |  6 +-----
>  tests/usb-hcd-xhci-test.c | 12 ++----------
>  tests/wdt_ib700-test.c    | 35 ++++++++++-------------------------
>  6 files changed, 16 insertions(+), 48 deletions(-)

Nice!

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 8/9] tests/libqtest: Clean up how we read the QMP greeting

[Markus Armbruster]

qtest_init() still uses the qtest_qmp_discard_response(s, "") hack to
receive the greeting, even though we have qtest_qmp_receive() since
commit 66e0c7b.  Put it to use.

Bonus: gets rid of an empty format string.  A step towards
compile-time format string checking without triggering
-Wformat-zero-length.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/libqtest.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 4a5492a..7e5425d 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -230,9 +230,11 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
 QTestState *qtest_init(const char *extra_args)
 {
     QTestState *s = qtest_init_without_qmp_handshake(extra_args);
+    QDict *greeting;
 
     /* Read the QMP greeting and then do the handshake */
-    qtest_qmp_discard_response(s, "");
+    greeting = qtest_qmp_receive(s);
+    QDECREF(greeting);
     qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
 
     return s;
-- 
2.7.5

Re: [Qemu-devel] [PATCH 8/9] tests/libqtest: Clean up how we read the QMP greeting

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 624 bytes --]

On 07/21/2017 08:53 AM, Markus Armbruster wrote:
> qtest_init() still uses the qtest_qmp_discard_response(s, "") hack to
> receive the greeting, even though we have qtest_qmp_receive() since
> commit 66e0c7b.  Put it to use.
> 
> Bonus: gets rid of an empty format string.  A step towards
> compile-time format string checking without triggering
> -Wformat-zero-length.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 8/9] tests/libqtest: Clean up how we read the QMP greeting

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

On Fri, Jul 21, 2017 at 03:53:25PM +0200, Markus Armbruster wrote:
> qtest_init() still uses the qtest_qmp_discard_response(s, "") hack to
> receive the greeting, even though we have qtest_qmp_receive() since
> commit 66e0c7b.  Put it to use.
> 
> Bonus: gets rid of an empty format string.  A step towards
> compile-time format string checking without triggering
> -Wformat-zero-length.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/libqtest.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 9/9] tests/libqtest: Enable compile-time format string checking

[Markus Armbruster]

qtest_qmp() & friends pass their format string and variable arguments
to qobject_from_jsonv().  Unlike qobject_from_jsonv(), they aren't
decorated with GCC_FMT_ATTR().  Fix that to get compile-time format
string checking.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/libqtest.h | 40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index ae57282..435012d 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -55,7 +55,8 @@ void qtest_quit(QTestState *s);
  *
  * Sends a QMP message to QEMU and consumes the response.
  */
-void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
+void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);
 
 /**
  * qtest_qmp:
@@ -65,7 +66,8 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
  *
  * Sends a QMP message to QEMU and returns the response.
  */
-QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
+QDict *qtest_qmp(QTestState *s, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);
 
 /**
  * qtest_async_qmp:
@@ -75,7 +77,8 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
  */
-void qtest_async_qmp(QTestState *s, const char *fmt, ...);
+void qtest_async_qmp(QTestState *s, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);
 
 /**
  * qtest_qmpv_discard_response:
@@ -85,7 +88,8 @@ void qtest_async_qmp(QTestState *s, const char *fmt, ...);
  *
  * Sends a QMP message to QEMU and consumes the response.
  */
-void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap);
+void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap)
+    GCC_FMT_ATTR(2, 0);
 
 /**
  * qtest_qmpv:
@@ -95,7 +99,8 @@ void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap);
  *
  * Sends a QMP message to QEMU and returns the response.
  */
-QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
+QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
+    GCC_FMT_ATTR(2, 0);
 
 /**
  * qtest_async_qmpv:
@@ -105,7 +110,8 @@ QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
  */
-void qtest_async_qmpv(QTestState *s, const char *fmt, va_list ap);
+void qtest_async_qmpv(QTestState *s, const char *fmt, va_list ap)
+    GCC_FMT_ATTR(2, 0);
 
 /**
  * qtest_receive:
@@ -144,7 +150,8 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
  *
  * Returns: the command's output.  The caller should g_free() it.
  */
-char *qtest_hmp(QTestState *s, const char *fmt, ...);
+char *qtest_hmp(QTestState *s, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);
 
 /**
  * qtest_hmpv:
@@ -157,7 +164,8 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...);
  *
  * Returns: the command's output.  The caller should g_free() it.
  */
-char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap);
+char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap)
+    GCC_FMT_ATTR(2, 0);
 
 /**
  * qtest_get_irq:
@@ -543,7 +551,7 @@ static inline void qtest_end(void)
  *
  * Sends a QMP message to QEMU and returns the response.
  */
-QDict *qmp(const char *fmt, ...);
+QDict *qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 
 /**
  * qmp_async:
@@ -552,7 +560,7 @@ QDict *qmp(const char *fmt, ...);
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
  */
-void qmp_async(const char *fmt, ...);
+void qmp_async(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 
 /**
  * qmp_discard_response:
@@ -561,7 +569,7 @@ void qmp_async(const char *fmt, ...);
  *
  * Sends a QMP message to QEMU and consumes the response.
  */
-void qmp_discard_response(const char *fmt, ...);
+void qmp_discard_response(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 
 /**
  * qmp_receive:
@@ -604,7 +612,7 @@ static inline QDict *qmp_eventwait_ref(const char *event)
  *
  * Returns: the command's output.  The caller should g_free() it.
  */
-char *hmp(const char *fmt, ...);
+char *hmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 
 /**
  * get_irq:
@@ -920,10 +928,10 @@ static inline int64_t clock_set(int64_t val)
 }
 
 QDict *qmp_fd_receive(int fd);
-void qmp_fd_sendv(int fd, const char *fmt, va_list ap);
-void qmp_fd_send(int fd, const char *fmt, ...);
-QDict *qmp_fdv(int fd, const char *fmt, va_list ap);
-QDict *qmp_fd(int fd, const char *fmt, ...);
+void qmp_fd_sendv(int fd, const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
+void qmp_fd_send(int fd, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
+QDict *qmp_fdv(int fd, const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
+QDict *qmp_fd(int fd, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
 
 /**
  * qtest_cb_for_every_machine:
-- 
2.7.5

Re: [Qemu-devel] [PATCH 9/9] tests/libqtest: Enable compile-time format string checking

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]

On 07/21/2017 08:53 AM, Markus Armbruster wrote:
> qtest_qmp() & friends pass their format string and variable arguments
> to qobject_from_jsonv().  Unlike qobject_from_jsonv(), they aren't
> decorated with GCC_FMT_ATTR().  Fix that to get compile-time format
> string checking.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/libqtest.h | 40 ++++++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 16 deletions(-)

Don't know how much of this to merge with my fixed version of 2/9.  But
matches what I had locally after my version 1, before I ripped it all
back out again prior to posting my v2 when dealing with gcc fallout,
that you've now corrected.

> @@ -65,7 +66,8 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
>   *
>   * Sends a QMP message to QEMU and returns the response.
>   */
> -QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
> +QDict *qtest_qmp(QTestState *s, const char *fmt, ...)
> +    GCC_FMT_ATTR(2, 3);

This would fit on one line; any reason we need the line wrap?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 9/9] tests/libqtest: Enable compile-time format string checking

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/21/2017 08:53 AM, Markus Armbruster wrote:
>> qtest_qmp() & friends pass their format string and variable arguments
>> to qobject_from_jsonv().  Unlike qobject_from_jsonv(), they aren't
>> decorated with GCC_FMT_ATTR().  Fix that to get compile-time format
>> string checking.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/libqtest.h | 40 ++++++++++++++++++++++++----------------
>>  1 file changed, 24 insertions(+), 16 deletions(-)
>
> Don't know how much of this to merge with my fixed version of 2/9.  But
> matches what I had locally after my version 1, before I ripped it all
> back out again prior to posting my v2 when dealing with gcc fallout,
> that you've now corrected.
>
>> @@ -65,7 +66,8 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
>>   *
>>   * Sends a QMP message to QEMU and returns the response.
>>   */
>> -QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
>> +QDict *qtest_qmp(QTestState *s, const char *fmt, ...)
>> +    GCC_FMT_ATTR(2, 3);
>
> This would fit on one line; any reason we need the line wrap?

Pretty sure I didn't have reasons.  But I can make some up after the
fact!  Putting the attribute on a separate line can be a bit more
legible.  It can also confuse simple tools to recognize a definition
instead of a declaration.  Anyway, I don't really care about this line
break.

Re: [Qemu-devel] [PATCH 9/9] tests/libqtest: Enable compile-time format string checking

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

On Fri, Jul 21, 2017 at 03:53:26PM +0200, Markus Armbruster wrote:
> qtest_qmp() & friends pass their format string and variable arguments
> to qobject_from_jsonv().  Unlike qobject_from_jsonv(), they aren't
> decorated with GCC_FMT_ATTR().  Fix that to get compile-time format
> string checking.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/libqtest.h | 40 ++++++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 16 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]