[Qemu-devel] [PATCH 00/20] tests: Compile-time format string checking for libqtest.h

[Qemu-devel] [PATCH 00/20] tests: Compile-time format string checking for libqtest.h

[Markus Armbruster]

This is a reboot of "[PATCH 0/9] tests: Clean up around qmp() and
hmp()" I sent about a year ago.  Back then, Eric folded it into his
"Clean up around qmp() and hmp()" series, which stalled after v4.

My reboot tries to avoid that fate by reducing mission creep.  There
are plenty of good ideas in Eric's series we should mine some day, but
today is not that day.

Since the changes outside tests are limited to renames and a few new
helpers, all in qobject/qjson.c, I guess we could merge this for 3.0.
But it can just as well wait for the next development cycle.

Eric Blake (1):
  libqtest: Document calling conventions

Markus Armbruster (19):
  libqtest: Rename functions to send QMP messages
  libqtest: Clean up how we read device_del messages
  libqtest: Clean up how we read the QMP greeting
  qobject: Replace qobject_from_jsonf() by qobject_from_jsonf_nofail()
  qobject: New qobject_from_vjsonf_nofail(), qdict_from_vjsonf_nofail()
  libqtest: Simplify qmp_fd_vsend() a bit
  test-qobject-input-visitor: Avoid format string ambiguity
  qobject: qobject_from_jsonv() is dangerous, hide it away
  tests: Pass literal format strings directly to qmp_FOO()
  tests: Clean up string interpolation into QMP input (simple cases)
  cpu-plug-test: Don't pass integers as strings to device_add
  tests: Clean up string interpolation around qtest_qmp_device_add()
  migration-test: Make wait_command() return the "return" member
  tests: New helper qtest_qmp_receive_success()
  migration-test: Make wait_command() cope with '%'
  migration-test: Clean up string interpolation into QMP, part 1
  migration-test: Clean up string interpolation into QMP, part 2
  migration-test: Clean up string interpolation into QMP, part 3
  libqtest: Enable compile-time format string checking

 include/qapi/qmp/qjson.h           |  12 +-
 qobject/qjson.c                    |  89 +++++++++++----
 tests/ahci-test.c                  |  12 +-
 tests/check-qjson.c                |  15 +--
 tests/cpu-plug-test.c              |   7 +-
 tests/e1000e-test.c                |   6 +-
 tests/ide-test.c                   |   4 +-
 tests/ivshmem-test.c               |   8 +-
 tests/libqos/ahci.c                |   4 +-
 tests/libqos/pci-pc.c              |   9 +-
 tests/libqos/pci.c                 |   7 --
 tests/libqos/pci.h                 |   2 -
 tests/libqos/usb.c                 |  10 +-
 tests/libqos/usb.h                 |   2 +-
 tests/libqtest.c                   | 141 +++++++++++++----------
 tests/libqtest.h                   |  90 ++++++++++-----
 tests/migration-test.c             | 178 ++++++++++++-----------------
 tests/qmp-test.c                   |  18 +--
 tests/test-qga.c                   | 150 +++++++++++-------------
 tests/test-qobject-input-visitor.c |  19 +--
 tests/tpm-util.c                   |  41 ++-----
 tests/usb-hcd-ehci-test.c          |   2 +-
 tests/usb-hcd-ohci-test.c          |   2 +-
 tests/usb-hcd-uhci-test.c          |   4 +-
 tests/usb-hcd-xhci-test.c          |  10 +-
 tests/vhost-user-test.c            |   6 +-
 tests/virtio-blk-test.c            |   5 +-
 tests/virtio-net-test.c            |   3 +-
 tests/virtio-rng-test.c            |   3 +-
 tests/virtio-scsi-test.c           |   2 +-
 tests/virtio-serial-test.c         |   2 +-
 31 files changed, 439 insertions(+), 424 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 01/20] libqtest: Document calling conventions

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

We have two flavors of vararg usage in qtest: qtest_hmp() etc. work
like sprintf(), and qtest_qmp() etc. work like qobject_from_jsonf().
Spell that out in the comments.

Also add GCC_FMT_ATTR() to qtest_hmp() etc. so that the compiler can
flag incorrect use.

We have some cleanup work to do before we can do the same for
qtest_qmp() etc.  This would get us the same better-than-nothing
checking we already have for qobject_from_jsonf(): common incorrect
uses of supported conversion specifications will be flagged
(e.g. passing a double for %d), but use of unsupported ones won't.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
[Rebased, commit message rewritten]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/libqtest.h | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index ac52872cbe..c63551f14f 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -76,7 +76,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.
  */
@@ -85,7 +86,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.
  */
@@ -94,7 +96,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.
  */
@@ -103,7 +106,8 @@ void qtest_async_qmp(QTestState *s, const char *fmt, ...);
 /**
  * qtest_qmpv_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])').
  * @ap: QMP message arguments
  *
  * Sends a QMP message to QEMU and consumes the response.
@@ -113,7 +117,8 @@ void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap);
 /**
  * qtest_qmpv:
  * @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])').
  * @ap: QMP message arguments
  *
  * Sends a QMP message to QEMU and returns the response.
@@ -123,7 +128,8 @@ QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
 /**
  * qtest_async_qmpv:
  * @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])').
  * @ap: QMP message arguments
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
@@ -172,7 +178,7 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
 /**
  * qtest_hmpv:
  * @s: #QTestState instance to operate on.
- * @fmt: HMP command to send to QEMU
+ * @fmt: HMP command to send to QEMU, formats arguments like vsprintf().
  * @ap: HMP command arguments
  *
  * Send HMP command to QEMU via QMP's human-monitor-command.
@@ -180,7 +186,8 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
  *
  * 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:
@@ -561,7 +568,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.
  */
@@ -569,7 +577,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.
  */
@@ -577,7 +586,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.
  */
-- 
2.17.1

[Qemu-devel] [PATCH 02/20] libqtest: Rename functions to send QMP messages

[Markus Armbruster]

The functions to receive messages are called qtest_qmp_receive() and
qmp_receive(), qmp_fd_receive().  The ones to send messages are called
qtest_async_qmp(), qtest_async_qmpv(), qmp_async(), qmp_fd_send(),
qmp_fd_sendv().  Inconsistent.  Rename the *_async* ones to
qmp_send(), qtest_qmp_send(), qtest_qmp_vsend().  Rename
qmp_fd_sendv() to qmp_fd_vsend().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/ahci-test.c   | 10 +++++-----
 tests/libqos/ahci.c |  4 ++--
 tests/libqtest.c    | 20 ++++++++++----------
 tests/libqtest.h    | 14 +++++++-------
 tests/qmp-test.c    | 18 +++++++++---------
 5 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 1a7b761304..7e3491b5bd 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1389,7 +1389,7 @@ static void test_flush_migrate(void)
 
     /* Complete the command */
     s = "{'execute':'cont' }";
-    qmp_async(s);
+    qmp_send(s);
     qmp_eventwait("RESUME");
     ahci_command_wait(dst, cmd);
     ahci_command_verify(dst, cmd);
@@ -1592,8 +1592,8 @@ static void test_atapi_tray(void)
     atapi_wait_tray(false);
 
     /* Remove media */
-    qmp_async("{'execute': 'blockdev-open-tray', "
-               "'arguments': {'id': 'cd0'}}");
+    qmp_send("{'execute': 'blockdev-open-tray',"
+             " 'arguments': {'id': 'cd0'}}");
     atapi_wait_tray(true);
     rsp = qmp_receive();
     qobject_unref(rsp);
@@ -1619,8 +1619,8 @@ static void test_atapi_tray(void)
                                          "'node-name': 'node0' }}");
 
     /* Again, the event shows up first */
-    qmp_async("{'execute': 'blockdev-close-tray', "
-               "'arguments': {'id': 'cd0'}}");
+    qmp_send("{'execute': 'blockdev-close-tray',"
+             " 'arguments': {'id': 'cd0'}}");
     atapi_wait_tray(false);
     rsp = qmp_receive();
     qobject_unref(rsp);
diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 42d3f76933..63fbc9e3c9 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -674,7 +674,7 @@ void ahci_exec(AHCIQState *ahci, uint8_t port,
         g_assert_cmpint(rc, ==, 0);
     }
     if (opts->error) {
-        qtest_async_qmp(ahci->parent->qts, "{'execute':'cont' }");
+        qtest_qmp_send(ahci->parent->qts, "{'execute':'cont' }");
         qtest_qmp_eventwait(ahci->parent->qts, "RESUME");
     }
 
@@ -712,7 +712,7 @@ AHCICommand *ahci_guest_io_halt(AHCIQState *ahci, uint8_t port,
 void ahci_guest_io_resume(AHCIQState *ahci, AHCICommand *cmd)
 {
     /* Complete the command */
-    qtest_async_qmp(ahci->parent->qts, "{'execute':'cont' }");
+    qtest_qmp_send(ahci->parent->qts, "{'execute':'cont' }");
     qtest_qmp_eventwait(ahci->parent->qts, "RESUME");
     ahci_command_wait(ahci, cmd);
     ahci_command_verify(ahci, cmd);
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 098af6aec4..9cb4096639 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -484,7 +484,7 @@ QDict *qtest_qmp_receive(QTestState *s)
  * in the case that they choose to discard all replies up until
  * a particular EVENT is received.
  */
-void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
+void qmp_fd_vsend(int fd, const char *fmt, va_list ap)
 {
     va_list ap_copy;
     QObject *qobj;
@@ -529,21 +529,21 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
     }
 }
 
-void qtest_async_qmpv(QTestState *s, const char *fmt, va_list ap)
+void qtest_qmp_vsend(QTestState *s, const char *fmt, va_list ap)
 {
-    qmp_fd_sendv(s->qmp_fd, fmt, ap);
+    qmp_fd_vsend(s->qmp_fd, fmt, ap);
 }
 
 QDict *qmp_fdv(int fd, const char *fmt, va_list ap)
 {
-    qmp_fd_sendv(fd, fmt, ap);
+    qmp_fd_vsend(fd, fmt, ap);
 
     return qmp_fd_receive(fd);
 }
 
 QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
 {
-    qtest_async_qmpv(s, fmt, ap);
+    qtest_qmp_vsend(s, fmt, ap);
 
     /* Receive reply */
     return qtest_qmp_receive(s);
@@ -565,7 +565,7 @@ void qmp_fd_send(int fd, const char *fmt, ...)
     va_list ap;
 
     va_start(ap, fmt);
-    qmp_fd_sendv(fd, fmt, ap);
+    qmp_fd_vsend(fd, fmt, ap);
     va_end(ap);
 }
 
@@ -580,12 +580,12 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...)
     return response;
 }
 
-void qtest_async_qmp(QTestState *s, const char *fmt, ...)
+void qtest_qmp_send(QTestState *s, const char *fmt, ...)
 {
     va_list ap;
 
     va_start(ap, fmt);
-    qtest_async_qmpv(s, fmt, ap);
+    qtest_qmp_vsend(s, fmt, ap);
     va_end(ap);
 }
 
@@ -968,12 +968,12 @@ QDict *qmp(const char *fmt, ...)
     return response;
 }
 
-void qmp_async(const char *fmt, ...)
+void qmp_send(const char *fmt, ...)
 {
     va_list ap;
 
     va_start(ap, fmt);
-    qtest_async_qmpv(global_qtest, fmt, ap);
+    qtest_qmp_vsend(global_qtest, fmt, ap);
     va_end(ap);
 }
 
diff --git a/tests/libqtest.h b/tests/libqtest.h
index c63551f14f..85bd1cf7b7 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -94,14 +94,14 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
 QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
 
 /**
- * qtest_async_qmp:
+ * qtest_qmp_send:
  * @s: #QTestState instance to operate on.
  * @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.
  */
-void qtest_async_qmp(QTestState *s, const char *fmt, ...);
+void qtest_qmp_send(QTestState *s, const char *fmt, ...);
 
 /**
  * qtest_qmpv_discard_response:
@@ -126,7 +126,7 @@ void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap);
 QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
 
 /**
- * qtest_async_qmpv:
+ * qtest_qmp_vsend:
  * @s: #QTestState instance to operate on.
  * @fmt: QMP message to send to QEMU; formats arguments through
  * json-lexer.c (only understands '%((l|ll|I64)?d|[ipsf])').
@@ -134,7 +134,7 @@ 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_qmp_vsend(QTestState *s, const char *fmt, va_list ap);
 
 /**
  * qtest_receive:
@@ -576,13 +576,13 @@ static inline void qtest_end(void)
 QDict *qmp(const char *fmt, ...);
 
 /**
- * qmp_async:
+ * qmp_send:
  * @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.
  */
-void qmp_async(const char *fmt, ...);
+void qmp_send(const char *fmt, ...);
 
 /**
  * qmp_discard_response:
@@ -950,7 +950,7 @@ 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_vsend(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, ...);
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index ceaf4a6789..60685bba3c 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -159,12 +159,12 @@ static void cleanup_blocking_cmd(void)
 
 static void send_cmd_that_blocks(QTestState *s, const char *id)
 {
-    qtest_async_qmp(s, "{ 'execute': 'blockdev-add',  'id': %s,"
-                    " 'arguments': {"
-                    " 'driver': 'blkdebug', 'node-name': %s,"
-                    " 'config': %s,"
-                    " 'image': { 'driver': 'null-co' } } }",
-                    id, id, fifo_name);
+    qtest_qmp_send(s, "{ 'execute': 'blockdev-add',  'id': %s,"
+                   " 'arguments': {"
+                   " 'driver': 'blkdebug', 'node-name': %s,"
+                   " 'config': %s,"
+                   " 'image': { 'driver': 'null-co' } } }",
+                   id, id, fifo_name);
 }
 
 static void unblock_blocked_cmd(void)
@@ -176,7 +176,7 @@ static void unblock_blocked_cmd(void)
 
 static void send_oob_cmd_that_fails(QTestState *s, const char *id)
 {
-    qtest_async_qmp(s, "{ 'exec-oob': 'migrate-pause', 'id': %s }", id);
+    qtest_qmp_send(s, "{ 'exec-oob': 'migrate-pause', 'id': %s }", id);
 }
 
 static void recv_cmd_id(QTestState *s, const char *id)
@@ -235,7 +235,7 @@ static void test_qmp_oob(void)
     /* OOB command overtakes slow in-band command */
     setup_blocking_cmd();
     send_cmd_that_blocks(qts, "ib-blocks-1");
-    qtest_async_qmp(qts, "{ 'execute': 'query-name', 'id': 'ib-quick-1' }");
+    qtest_qmp_send(qts, "{ 'execute': 'query-name', 'id': 'ib-quick-1' }");
     send_oob_cmd_that_fails(qts, "oob-1");
     recv_cmd_id(qts, "oob-1");
     unblock_blocked_cmd();
@@ -244,7 +244,7 @@ static void test_qmp_oob(void)
 
     /* Even malformed in-band command fails in-band */
     send_cmd_that_blocks(qts, "blocks-2");
-    qtest_async_qmp(qts, "{ 'id': 'err-2' }");
+    qtest_qmp_send(qts, "{ 'id': 'err-2' }");
     unblock_blocked_cmd();
     recv_cmd_id(qts, "blocks-2");
     recv_cmd_id(qts, "err-2");
-- 
2.17.1

[Qemu-devel] [PATCH 03/20] libqtest: Clean up how we read device_del messages

[Markus Armbruster]

qtest_qmp_device_del() still uses the qmp("") hack to receive a
message, even though we have qmp_receive() since commit 66e0c7b187e.
Put it to use.

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/libqtest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 9cb4096639..071d7eb7b1 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -1090,7 +1090,7 @@ void qtest_qmp_device_del(const char *id)
     g_assert(response1);
     g_assert(!qdict_haskey(response1, "error"));
 
-    response2 = qmp("");
+    response2 = qmp_receive();
     g_assert(response2);
     g_assert(!qdict_haskey(response2, "error"));
 
-- 
2.17.1

[Qemu-devel] [PATCH 04/20] 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 66e0c7b187e.  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 071d7eb7b1..c2c08a890c 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -249,9 +249,11 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
 QTestState *qtest_init(const char *extra_args)
 {
     QTestState *s = qtest_init_without_qmp_handshake(false, extra_args);
+    QDict *greeting;
 
     /* Read the QMP greeting and then do the handshake */
-    qtest_qmp_discard_response(s, "");
+    greeting = qtest_qmp_receive(s);
+    qobject_unref(greeting);
     qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
 
     return s;
-- 
2.17.1

[Qemu-devel] [PATCH 05/20] qobject: Replace qobject_from_jsonf() by qobject_from_jsonf_nofail()

[Markus Armbruster]

Commit ab45015a968 "qobject: Let qobject_from_jsonf() fail instead of
abort" removed only one of two ways qobject_from_jsonf() can abort.
Wrong.  Its rationale for letting it fail is that all remaining users
cope fine with failure.  Well, they're just fine with aborting, too;
it's what they do on failure.

Simply reverting the broken commit would bring back the unfortunate
asymmetry between qobject_from_jsonf() and qobject_from_jsonv(): one
aborts, the other returns null.  So also rename it to
qobject_from_jsonf_nofail().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/qmp/qjson.h |  6 ++++--
 qobject/qjson.c          |  8 +++++++-
 tests/check-qjson.c      | 15 ++++++++-------
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index 43b2ce2f33..dc509d51ae 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -15,11 +15,13 @@
 #define QJSON_H
 
 QObject *qobject_from_json(const char *string, Error **errp);
-QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
 QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
     GCC_FMT_ATTR(1, 0);
 
-QDict *qdict_from_jsonf_nofail(const char *string, ...) GCC_FMT_ATTR(1, 2);
+QObject *qobject_from_jsonf_nofail(const char *string, ...)
+    GCC_FMT_ATTR(1, 2);
+QDict *qdict_from_jsonf_nofail(const char *string, ...)
+    GCC_FMT_ATTR(1, 2);
 
 QString *qobject_to_json(const QObject *obj);
 QString *qobject_to_json_pretty(const QObject *obj);
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 2f6a590e44..4a9dcff343 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -59,7 +59,12 @@ QObject *qobject_from_json(const char *string, Error **errp)
     return qobject_from_jsonv(string, NULL, errp);
 }
 
-QObject *qobject_from_jsonf(const char *string, ...)
+/*
+ * Parse @string as JSON value with %-escapes interpolated.
+ * Abort on error.  Do not use with untrusted @string.
+ * Return the resulting QObject.  It is never null.
+ */
+QObject *qobject_from_jsonf_nofail(const char *string, ...)
 {
     QObject *obj;
     va_list ap;
@@ -68,6 +73,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
     obj = qobject_from_jsonv(string, &ap, &error_abort);
     va_end(ap);
 
+    assert(obj);
     return obj;
 }
 
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index da582df3e9..eaf5d20663 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -865,7 +865,8 @@ static void vararg_string(void)
         QString *str;
 
         str = qobject_to(QString,
-                         qobject_from_jsonf("%s", test_cases[i].decoded));
+                         qobject_from_jsonf_nofail("%s",
+                                                   test_cases[i].decoded));
         g_assert(str);
         g_assert(strcmp(qstring_get_str(str), test_cases[i].decoded) == 0);
 
@@ -998,17 +999,17 @@ static void vararg_number(void)
     double valuef = 2.323423423;
     int64_t val;
 
-    qnum = qobject_to(QNum, qobject_from_jsonf("%d", value));
+    qnum = qobject_to(QNum, qobject_from_jsonf_nofail("%d", value));
     g_assert(qnum_get_try_int(qnum, &val));
     g_assert_cmpint(val, ==, value);
     qobject_unref(qnum);
 
-    qnum = qobject_to(QNum, qobject_from_jsonf("%lld", value_ll));
+    qnum = qobject_to(QNum, qobject_from_jsonf_nofail("%lld", value_ll));
     g_assert(qnum_get_try_int(qnum, &val));
     g_assert_cmpint(val, ==, value_ll);
     qobject_unref(qnum);
 
-    qnum = qobject_to(QNum, qobject_from_jsonf("%f", valuef));
+    qnum = qobject_to(QNum, qobject_from_jsonf_nofail("%f", valuef));
     g_assert(qnum_get_double(qnum) == valuef);
     qobject_unref(qnum);
 }
@@ -1042,13 +1043,13 @@ static void keyword_literal(void)
 
     qobject_unref(qbool);
 
-    qbool = qobject_to(QBool, qobject_from_jsonf("%i", false));
+    qbool = qobject_to(QBool, qobject_from_jsonf_nofail("%i", false));
     g_assert(qbool);
     g_assert(qbool_get_bool(qbool) == false);
     qobject_unref(qbool);
 
     /* Test that non-zero values other than 1 get collapsed to true */
-    qbool = qobject_to(QBool, qobject_from_jsonf("%i", 2));
+    qbool = qobject_to(QBool, qobject_from_jsonf_nofail("%i", 2));
     g_assert(qbool);
     g_assert(qbool_get_bool(qbool) == true);
     qobject_unref(qbool);
@@ -1298,7 +1299,7 @@ static void simple_varargs(void)
     embedded_obj = qobject_from_json("[32, 42]", &error_abort);
     g_assert(embedded_obj != NULL);
 
-    obj = qobject_from_jsonf("[%d, 2, %p]", 1, embedded_obj);
+    obj = qobject_from_jsonf_nofail("[%d, 2, %p]", 1, embedded_obj);
     g_assert(qlit_equal_qobject(&decoded, obj));
 
     qobject_unref(obj);
-- 
2.17.1

[Qemu-devel] [PATCH 06/20] qobject: New qobject_from_vjsonf_nofail(), qdict_from_vjsonf_nofail()

[Markus Armbruster]

Every printf()-like function sooner or later needs its vprintf()-like
buddy.  The next commit will need qobject_from_jsonf_nofail()'s buddy,
and qdict_from_jsonf_nofail()'s buddy will be used later in this
series.  Add both.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/qmp/qjson.h |  4 ++++
 qobject/qjson.c          | 44 +++++++++++++++++++++++++++++++++-------
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index dc509d51ae..0db121bef9 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -18,8 +18,12 @@ QObject *qobject_from_json(const char *string, Error **errp);
 QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
     GCC_FMT_ATTR(1, 0);
 
+QObject *qobject_from_vjsonf_nofail(const char *string, va_list)
+    GCC_FMT_ATTR(1, 0);
 QObject *qobject_from_jsonf_nofail(const char *string, ...)
     GCC_FMT_ATTR(1, 2);
+QDict *qdict_from_vjsonf_nofail(const char *string, va_list ap)
+    GCC_FMT_ATTR(1, 0);
 QDict *qdict_from_jsonf_nofail(const char *string, ...)
     GCC_FMT_ATTR(1, 2);
 
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 4a9dcff343..2e450231ff 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -59,6 +59,25 @@ QObject *qobject_from_json(const char *string, Error **errp)
     return qobject_from_jsonv(string, NULL, errp);
 }
 
+/*
+ * Parse @string as JSON value with %-escapes interpolated.
+ * Abort on error.  Do not use with untrusted @string.
+ * Return the resulting QObject.  It is never null.
+ */
+QObject *qobject_from_vjsonf_nofail(const char *string, va_list ap)
+{
+    va_list ap_copy;
+    QObject *obj;
+
+    /* va_copy() is needed when va_list is an array type */
+    va_copy(ap_copy, ap);
+    obj = qobject_from_jsonv(string, &ap_copy, &error_abort);
+    va_end(ap_copy);
+
+    assert(obj);
+    return obj;
+}
+
 /*
  * Parse @string as JSON value with %-escapes interpolated.
  * Abort on error.  Do not use with untrusted @string.
@@ -70,13 +89,26 @@ QObject *qobject_from_jsonf_nofail(const char *string, ...)
     va_list ap;
 
     va_start(ap, string);
-    obj = qobject_from_jsonv(string, &ap, &error_abort);
+    obj = qobject_from_vjsonf_nofail(string, ap);
     va_end(ap);
 
-    assert(obj);
     return obj;
 }
 
+/*
+ * Parse @string as JSON object with %-escapes interpolated.
+ * Abort on error.  Do not use with untrusted @string.
+ * Return the resulting QDict.  It is never null.
+ */
+QDict *qdict_from_vjsonf_nofail(const char *string, va_list ap)
+{
+    QDict *qdict;
+
+    qdict = qobject_to(QDict, qobject_from_vjsonf_nofail(string, ap));
+    assert(qdict);
+    return qdict;
+}
+
 /*
  * Parse @string as JSON object with %-escapes interpolated.
  * Abort on error.  Do not use with untrusted @string.
@@ -84,15 +116,13 @@ QObject *qobject_from_jsonf_nofail(const char *string, ...)
  */
 QDict *qdict_from_jsonf_nofail(const char *string, ...)
 {
-    QDict *obj;
+    QDict *qdict;
     va_list ap;
 
     va_start(ap, string);
-    obj = qobject_to(QDict, qobject_from_jsonv(string, &ap, &error_abort));
+    qdict = qdict_from_vjsonf_nofail(string, ap);
     va_end(ap);
-
-    assert(obj);
-    return obj;
+    return qdict;
 }
 
 typedef struct ToJsonIterState
-- 
2.17.1

[Qemu-devel] [PATCH 07/20] libqtest: Simplify qmp_fd_vsend() a bit

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/libqtest.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index c2c08a890c..3bfb062fcb 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -488,24 +488,20 @@ QDict *qtest_qmp_receive(QTestState *s)
  */
 void qmp_fd_vsend(int fd, const char *fmt, va_list ap)
 {
-    va_list ap_copy;
     QObject *qobj;
 
-    /* qobject_from_jsonv() silently eats leading 0xff as invalid
-     * JSON, but we want to test sending them over the wire to force
-     * resyncs */
+    /*
+     * qobject_from_vjsonf_nofail() chokes on leading 0xff as invalid
+     * JSON, but tests/test-qga.c needs to send that to test QGA
+     * synchronization
+     */
     if (*fmt == '\377') {
         socket_send(fd, fmt, 1);
         fmt++;
     }
 
-    /* Going through qobject ensures we escape strings properly.
-     * This seemingly unnecessary copy is required in case va_list
-     * is an array type.
-     */
-    va_copy(ap_copy, ap);
-    qobj = qobject_from_jsonv(fmt, &ap_copy, &error_abort);
-    va_end(ap_copy);
+    /* Going through qobject ensures we escape strings properly */
+    qobj = qobject_from_vjsonf_nofail(fmt, ap);
 
     /* No need to send anything for an empty QObject.  */
     if (qobj) {
-- 
2.17.1

[Qemu-devel] [PATCH 08/20] test-qobject-input-visitor: Avoid format string ambiguity

[Markus Armbruster]

When visitor_input_test_init_internal()'s argument @ap is null, then
@json_string is interpreted literally, else it's gets %-escapes
interpolated.  This is awkward.

One caller always passes null @ap, and the others never do.  Lift the
building of the QObject into the callers, where it can be done without
such ambiguity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qobject-input-visitor.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 0f4d234c3f..caa90b3d7e 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -47,15 +47,13 @@ static void visitor_input_teardown(TestInputVisitorData *data,
 /* The various test_init functions are provided instead of a test setup
    function so that the JSON string used by the tests are kept in the test
    functions (and not in main()). */
-static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data,
-                                                 bool keyval,
-                                                 const char *json_string,
-                                                 va_list *ap)
+
+static Visitor *test_init_internal(TestInputVisitorData *data, bool keyval,
+                                   QObject *obj)
 {
     visitor_input_teardown(data, NULL);
 
-    data->obj = qobject_from_jsonv(json_string, ap, &error_abort);
-    g_assert(data->obj);
+    data->obj = obj;
 
     if (keyval) {
         data->qiv = qobject_input_visitor_new_keyval(data->obj);
@@ -75,7 +73,8 @@ Visitor *visitor_input_test_init_full(TestInputVisitorData *data,
     va_list ap;
 
     va_start(ap, json_string);
-    v = visitor_input_test_init_internal(data, keyval, json_string, &ap);
+    v = test_init_internal(data, keyval,
+                           qobject_from_vjsonf_nofail(json_string, ap));
     va_end(ap);
     return v;
 }
@@ -88,7 +87,8 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
     va_list ap;
 
     va_start(ap, json_string);
-    v = visitor_input_test_init_internal(data, false, json_string, &ap);
+    v = test_init_internal(data, false,
+                           qobject_from_vjsonf_nofail(json_string, ap));
     va_end(ap);
     return v;
 }
@@ -103,7 +103,8 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
 static Visitor *visitor_input_test_init_raw(TestInputVisitorData *data,
                                             const char *json_string)
 {
-    return visitor_input_test_init_internal(data, false, json_string, NULL);
+    return test_init_internal(data, false,
+                              qobject_from_json(json_string, &error_abort));
 }
 
 static void test_visitor_in_int(TestInputVisitorData *data,
-- 
2.17.1

[Qemu-devel] [PATCH 09/20] qobject: qobject_from_jsonv() is dangerous, hide it away

[Markus Armbruster]

qobject_from_jsonv() takes ownership of %p arguments.  On failure, we
can't generally know whether we failed before or after %p, so
ownership becomes indeterminate.  To avoid leaks, callers passing %p
must terminate on error, e.g. by passing &error_abort.  Trap for the
unwary; document and give the function internal linkage.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/qmp/qjson.h |  2 --
 qobject/qjson.c          | 13 ++++++++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index 0db121bef9..4a225cf7d1 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -15,8 +15,6 @@
 #define QJSON_H
 
 QObject *qobject_from_json(const char *string, Error **errp);
-QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
-    GCC_FMT_ATTR(1, 0);
 
 QObject *qobject_from_vjsonf_nofail(const char *string, va_list)
     GCC_FMT_ATTR(1, 0);
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 2e450231ff..ab4040f235 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -39,7 +39,18 @@ static void parse_json(JSONMessageParser *parser, GQueue *tokens)
     s->result = json_parser_parse_err(tokens, s->ap, &s->err);
 }
 
-QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
+/*
+ * Parse @string as JSON value.
+ * If @ap is non-null, interpolate %-escapes.
+ * Takes ownership of %p arguments.
+ * On success, return the JSON value.
+ * On failure, store an error through @errp and return NULL.
+ * Ownership of %p arguments becomes indeterminate then.  To avoid
+ * leaks, callers passing %p must terminate on error, e.g. by passing
+ * &error_abort.
+ */
+static QObject *qobject_from_jsonv(const char *string, va_list *ap,
+                                   Error **errp)
 {
     JSONParsingState state = {};
 
-- 
2.17.1

[Qemu-devel] [PATCH 10/20] 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 7e3491b5bd..19a7ab1bca 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1352,7 +1352,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");
@@ -1388,8 +1387,7 @@ static void test_flush_migrate(void)
     ahci_migrate(src, dst, uri);
 
     /* Complete the command */
-    s = "{'execute':'cont' }";
-    qmp_send(s);
+    qmp_send("{'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 2384c2c3e2..834ee9005b 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -694,7 +694,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");
 
@@ -722,8 +721,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.17.1

[Qemu-devel] [PATCH 11/20] 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_vjsonf_nofail()
would abort.  A sufficiently nasty @uri could even inject unwanted
members into the arguments object.

Leaving interpolation into JSON to qmp() is more robust:

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

It's also more concise.

Clean up the simple cases where we interpolate exactly a JSON value.

Bonus: gets rid of non-literal format strings.  A step towards
compile-time format string checking without triggering
-Wformat-nonliteral.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/libqos/pci-pc.c   |   9 +--
 tests/libqtest.c        |   7 +-
 tests/migration-test.c  |   8 +--
 tests/test-qga.c        | 150 ++++++++++++++++++----------------------
 tests/tpm-util.c        |   9 +--
 tests/vhost-user-test.c |   6 +-
 6 files changed, 77 insertions(+), 112 deletions(-)

diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
index a7803308b7..585f5289ec 100644
--- a/tests/libqos/pci-pc.c
+++ b/tests/libqos/pci-pc.c
@@ -160,14 +160,9 @@ void qpci_free_pc(QPCIBus *bus)
 void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
 {
     QDict *response;
-    char *cmd;
 
-    cmd = g_strdup_printf("{'execute': 'device_del',"
-                          " 'arguments': {"
-                          "   'id': '%s'"
-                          "}}", id);
-    response = qmp(cmd);
-    g_free(cmd);
+    response = qmp("{'execute': 'device_del', 'arguments': {'id': %s}}",
+                   id);
     g_assert(response);
     g_assert(!qdict_haskey(response, "error"));
     qobject_unref(response);
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 3bfb062fcb..e36cc5ede3 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -1079,12 +1079,9 @@ void qtest_qmp_device_add(const char *driver, const char *id, const char *fmt,
 void qtest_qmp_device_del(const char *id)
 {
     QDict *response1, *response2, *event = NULL;
-    char *cmd;
 
-    cmd = g_strdup_printf("{'execute': 'device_del',"
-                          " 'arguments': { 'id': '%s' }}", id);
-    response1 = qmp(cmd);
-    g_free(cmd);
+    response1 = qmp("{'execute': 'device_del', 'arguments': {'id': %s}}",
+                    id);
     g_assert(response1);
     g_assert(!qdict_haskey(response1, "error"));
 
diff --git a/tests/migration-test.c b/tests/migration-test.c
index 086f727b34..bbb9d3da31 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -529,14 +529,12 @@ static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
 static void deprecated_set_downtime(QTestState *who, const double value)
 {
     QDict *rsp;
-    gchar *cmd;
     char *expected;
     int64_t result_int;
 
-    cmd = g_strdup_printf("{ 'execute': 'migrate_set_downtime',"
-                          "'arguments': { 'value': %g } }", value);
-    rsp = qtest_qmp(who, cmd);
-    g_free(cmd);
+    rsp = qtest_qmp(who,
+                    "{ 'execute': 'migrate_set_downtime',"
+                    " 'arguments': { 'value': %f } }", value);
     g_assert(qdict_haskey(rsp, "return"));
     qobject_unref(rsp);
     result_int = value * 1000L;
diff --git a/tests/test-qga.c b/tests/test-qga.c
index d638b1571a..c552cc0125 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -146,12 +146,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.
@@ -188,7 +187,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
@@ -201,10 +199,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);
@@ -428,7 +425,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;
@@ -446,10 +443,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);
 
@@ -459,23 +456,20 @@ static void test_qga_file_ops(gconstpointer fix)
     g_assert_cmpint(count, ==, sizeof(helloworld));
     g_assert_cmpint(eof, ==, 0);
     qobject_unref(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);
     qobject_unref(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);
     qobject_unref(ret);
-    g_free(cmd);
 
     /* check content */
     path = g_build_filename(fixture->test_dir, "foo", NULL);
@@ -497,10 +491,10 @@ static void test_qga_file_ops(gconstpointer fix)
     qobject_unref(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");
@@ -510,14 +504,13 @@ static void test_qga_file_ops(gconstpointer fix)
     g_assert_cmpstr(b64, ==, enc);
 
     qobject_unref(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");
@@ -526,14 +519,13 @@ static void test_qga_file_ops(gconstpointer fix)
     g_assert(eof);
     g_assert_cmpstr(b64, ==, "");
     qobject_unref(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");
@@ -541,13 +533,12 @@ static void test_qga_file_ops(gconstpointer fix)
     g_assert_cmpint(count, ==, 6);
     g_assert(!eof);
     qobject_unref(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");
@@ -560,15 +551,13 @@ static void test_qga_file_ops(gconstpointer fix)
     g_free(dec);
 
     qobject_unref(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);
     qobject_unref(ret);
-    g_free(cmd);
 }
 
 static void test_qga_file_write_read(gconstpointer fix)
@@ -576,7 +565,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;
@@ -591,10 +580,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);
 
@@ -604,13 +593,12 @@ static void test_qga_file_write_read(gconstpointer fix)
     g_assert_cmpint(count, ==, sizeof(helloworld));
     g_assert_cmpint(eof, ==, 0);
     qobject_unref(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");
@@ -619,14 +607,13 @@ static void test_qga_file_write_read(gconstpointer fix)
     g_assert(eof);
     g_assert_cmpstr(b64, ==, "");
     qobject_unref(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");
@@ -634,13 +621,12 @@ static void test_qga_file_write_read(gconstpointer fix)
     g_assert_cmpint(count, ==, 0);
     g_assert(!eof);
     qobject_unref(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");
@@ -649,16 +635,14 @@ static void test_qga_file_write_read(gconstpointer fix)
     g_assert(eof);
     g_assert_cmpstr(b64, ==, enc);
     qobject_unref(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);
     qobject_unref(ret);
-    g_free(cmd);
 }
 
 static void test_qga_get_time(gconstpointer fix)
@@ -814,7 +798,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': {"
@@ -829,10 +812,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");
@@ -842,7 +825,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/tpm-util.c b/tests/tpm-util.c
index 672cedf905..3bd2887f1e 100644
--- a/tests/tpm-util.c
+++ b/tests/tpm-util.c
@@ -239,13 +239,10 @@ void tpm_util_swtpm_kill(GPid pid)
 void tpm_util_migrate(QTestState *who, const char *uri)
 {
     QDict *rsp;
-    gchar *cmd;
 
-    cmd = g_strdup_printf("{ 'execute': 'migrate',"
-                          "'arguments': { 'uri': '%s' } }",
-                          uri);
-    rsp = qtest_qmp(who, cmd);
-    g_free(cmd);
+    rsp = qtest_qmp(who,
+                    "{ 'execute': 'migrate', 'arguments': { 'uri': %s } }",
+                    uri);
     g_assert(qdict_haskey(rsp, "return"));
     qobject_unref(rsp);
 }
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 8ff2106d32..30389dbc7d 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -707,11 +707,7 @@ static void test_migrate(void)
     g_assert(qdict_haskey(rsp, "return"));
     qobject_unref(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"));
     qobject_unref(rsp);
 
-- 
2.17.1

[Qemu-devel] [PATCH 12/20] cpu-plug-test: Don't pass integers as strings to device_add

[Markus Armbruster]

test_plug_with_device_add_x86() plugs Haswell-i386-cpu and
Haswell-x86_64-cpu with device_add.  It passes socket-id, core-id,
thread-id as JSON strings.  The properties are actually integers.

test_plug_with_device_add_coreid() plugs power8_v2.0-spapr-cpu-core
and qemu-s390x-cpu with device_add.  It passes core-id as JSON string.
The properties are actually integers.

Passing JSON string values to integer properties works only due to
device_add implementation accidents.  Fix the test to pass JSON
numbers.  While there, use %u rather than %i with unsigned int.

Cc: Thomas Huth <thuth@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/cpu-plug-test.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/cpu-plug-test.c b/tests/cpu-plug-test.c
index 5f39ba0df3..ab3bf6df90 100644
--- a/tests/cpu-plug-test.c
+++ b/tests/cpu-plug-test.c
@@ -88,8 +88,8 @@ static void test_plug_with_device_add_x86(gconstpointer data)
         for (c = 0; c < td->cores; c++) {
             for (t = 0; t < td->threads; t++) {
                 char *id = g_strdup_printf("id-%i-%i-%i", s, c, t);
-                qtest_qmp_device_add(td->device_model, id, "'socket-id':'%i', "
-                                     "'core-id':'%i', 'thread-id':'%i'",
+                qtest_qmp_device_add(td->device_model, id, "'socket-id':%u, "
+                                     "'core-id':%u, 'thread-id':%u",
                                      s, c, t);
                 g_free(id);
             }
@@ -114,7 +114,7 @@ static void test_plug_with_device_add_coreid(gconstpointer data)
 
     for (c = td->cores; c < td->maxcpus / td->sockets / td->threads; c++) {
         char *id = g_strdup_printf("id-%i", c);
-        qtest_qmp_device_add(td->device_model, id, "'core-id':'%i'", c);
+        qtest_qmp_device_add(td->device_model, id, "'core-id':%u", c);
         g_free(id);
     }
 
-- 
2.17.1

[Qemu-devel] [PATCH 13/20] tests: Clean up string interpolation around qtest_qmp_device_add()

[Markus Armbruster]

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

qtest_qmp_device_add() and its wrappers interpolate into JSON as
follows:

* qtest_qmp_device_add() interpolates members into a JSON object.

* So do its wrappers qpci_plug_device_test() and usb_test_hotplug().

* usb_test_hotplug() additionally interpolates strings and numbers
  into JSON strings.

Clean them up:

* Have qtest_qmp_device_add() take its extra device properties as
  arguments for for qdict_from_jsonf_nofail() instead of a string
  containing JSON members.

* Drop qpci_plug_device_test(), use qtest_qmp_device_add()
  directly.

* Change usb_test_hotplug() parameter @port to string, to avoid
  interpolation.  Interpolate @hcd_id separately.

Bonus: gets rid of a non-literal format string.  A step towards
compile-time format string checking without triggering
-Wformat-nonliteral.

Cc: Thomas Huth <thuth@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/cpu-plug-test.c      |  7 ++++---
 tests/e1000e-test.c        |  6 ++----
 tests/ivshmem-test.c       |  8 +++-----
 tests/libqos/pci.c         |  7 -------
 tests/libqos/pci.h         |  2 --
 tests/libqos/usb.c         | 10 ++++++----
 tests/libqos/usb.h         |  2 +-
 tests/libqtest.c           | 27 +++++++++++----------------
 tests/libqtest.h           |  3 ++-
 tests/usb-hcd-ehci-test.c  |  2 +-
 tests/usb-hcd-ohci-test.c  |  2 +-
 tests/usb-hcd-uhci-test.c  |  4 ++--
 tests/usb-hcd-xhci-test.c  | 10 +++++-----
 tests/virtio-blk-test.c    |  5 +++--
 tests/virtio-net-test.c    |  3 ++-
 tests/virtio-rng-test.c    |  3 ++-
 tests/virtio-scsi-test.c   |  2 +-
 tests/virtio-serial-test.c |  2 +-
 18 files changed, 47 insertions(+), 58 deletions(-)

diff --git a/tests/cpu-plug-test.c b/tests/cpu-plug-test.c
index ab3bf6df90..f5d57da60e 100644
--- a/tests/cpu-plug-test.c
+++ b/tests/cpu-plug-test.c
@@ -88,8 +88,9 @@ static void test_plug_with_device_add_x86(gconstpointer data)
         for (c = 0; c < td->cores; c++) {
             for (t = 0; t < td->threads; t++) {
                 char *id = g_strdup_printf("id-%i-%i-%i", s, c, t);
-                qtest_qmp_device_add(td->device_model, id, "'socket-id':%u, "
-                                     "'core-id':%u, 'thread-id':%u",
+                qtest_qmp_device_add(td->device_model, id,
+                                     "{'socket-id':%u, 'core-id':%u,"
+                                     " 'thread-id':%u}",
                                      s, c, t);
                 g_free(id);
             }
@@ -114,7 +115,7 @@ static void test_plug_with_device_add_coreid(gconstpointer data)
 
     for (c = td->cores; c < td->maxcpus / td->sockets / td->threads; c++) {
         char *id = g_strdup_printf("id-%i", c);
-        qtest_qmp_device_add(td->device_model, id, "'core-id':%u", c);
+        qtest_qmp_device_add(td->device_model, id, "{'core-id':%u}", c);
         g_free(id);
     }
 
diff --git a/tests/e1000e-test.c b/tests/e1000e-test.c
index 32aa738b72..c9408a5d1f 100644
--- a/tests/e1000e-test.c
+++ b/tests/e1000e-test.c
@@ -456,12 +456,10 @@ static void test_e1000e_multiple_transfers(gconstpointer data)
 
 static void test_e1000e_hotplug(gconstpointer data)
 {
-    static const uint8_t slot = 0x06;
-
     qtest_start("-device e1000e");
 
-    qpci_plug_device_test("e1000e", "e1000e_net", slot, NULL);
-    qpci_unplug_acpi_device_test("e1000e_net", slot);
+    qtest_qmp_device_add("e1000e", "e1000e_net", "{'addr': '0x06'}");
+    qpci_unplug_acpi_device_test("e1000e_net", 0x06);
 
     qtest_end();
 }
diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
index 9b407a3e42..225fde7fff 100644
--- a/tests/ivshmem-test.c
+++ b/tests/ivshmem-test.c
@@ -420,19 +420,17 @@ static void test_ivshmem_server_irq(void)
 static void test_ivshmem_hotplug(void)
 {
     const char *arch = qtest_get_arch();
-    gchar *opts;
 
     qtest_start("");
 
-    opts = g_strdup_printf("'shm': '%s', 'size': '1M'", tmpshm);
-
-    qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts);
+    qtest_qmp_device_add("ivshmem",
+                         "iv1", "{'addr': %s, 'shm': '%s', 'size': '1M'}",
+                         stringify(PCI_SLOT_HP), tmpshm);
     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 0b73cb23d0..e8c342c257 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -395,10 +395,3 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
     QPCIBar bar = { .addr = addr };
     return bar;
 }
-
-void qpci_plug_device_test(const char *driver, const char *id,
-                           uint8_t slot, const char *opts)
-{
-    qtest_qmp_device_add(driver, id, "'addr': '%d'%s%s", slot,
-                         opts ? ", " : "", opts ? opts : "");
-}
diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
index 429c382282..0b7e936174 100644
--- a/tests/libqos/pci.h
+++ b/tests/libqos/pci.h
@@ -109,7 +109,5 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr);
 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);
 void qpci_unplug_acpi_device_test(const char *id, uint8_t slot);
 #endif
diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
index 2a476049a8..49e2f4bc0a 100644
--- a/tests/libqos/usb.c
+++ b/tests/libqos/usb.c
@@ -37,13 +37,14 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
     g_assert((value & mask) == (expect & mask));
 }
 
-void usb_test_hotplug(const char *hcd_id, const int port,
+void usb_test_hotplug(const char *hcd_id, const char *port,
                       void (*port_check)(void))
 {
-    char  *id = g_strdup_printf("usbdev%d", port);
+    char *id = g_strdup_printf("usbdev%s", port);
+    char *bus = g_strdup_printf("%s.0", hcd_id);
 
-    qtest_qmp_device_add("usb-tablet", id, "'port': '%d', 'bus': '%s.0'",
-                         port, hcd_id);
+    qtest_qmp_device_add("usb-tablet", id, "{'port': %s, 'bus': %s}",
+                         port, bus);
 
     if (port_check) {
         port_check();
@@ -51,5 +52,6 @@ void usb_test_hotplug(const char *hcd_id, const int port,
 
     qtest_qmp_device_del(id);
 
+    g_free(bus);
     g_free(id);
 }
diff --git a/tests/libqos/usb.h b/tests/libqos/usb.h
index 297cfc564d..c506418a13 100644
--- a/tests/libqos/usb.h
+++ b/tests/libqos/usb.h
@@ -13,6 +13,6 @@ void qusb_pci_init_one(QPCIBus *pcibus, struct qhc *hc,
 void uhci_port_test(struct qhc *hc, int port, uint16_t expect);
 void uhci_deinit(struct qhc *hc);
 
-void usb_test_hotplug(const char *bus_name, const int port,
+void usb_test_hotplug(const char *bus_name, const char *port,
                       void (*port_check)(void));
 #endif
diff --git a/tests/libqtest.c b/tests/libqtest.c
index e36cc5ede3..dfca3af89d 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -1032,26 +1032,21 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine))
 /*
  * Generic hot-plugging test via the device_add QMP command.
  */
-void qtest_qmp_device_add(const char *driver, const char *id, const char *fmt,
-                          ...)
+void qtest_qmp_device_add(const char *driver, const char *id,
+                          const char *fmt, ...)
 {
-    QDict *response;
-    char *cmd, *opts = NULL;
-    va_list va;
+    QDict *args, *response;
+    va_list ap;
 
-    if (fmt) {
-        va_start(va, fmt);
-        opts = g_strdup_vprintf(fmt, va);
-        va_end(va);
-    }
+    va_start(ap, fmt);
+    args = qdict_from_vjsonf_nofail(fmt, ap);
+    va_end(ap);
 
-    cmd = g_strdup_printf("{'execute': 'device_add',"
-                          " 'arguments': { 'driver': '%s', 'id': '%s'%s%s }}",
-                          driver, id, opts ? ", " : "", opts ? opts : "");
-    g_free(opts);
+    g_assert(!qdict_haskey(args, "driver") && !qdict_haskey(args, "id"));
+    qdict_put_str(args, "driver", driver);
+    qdict_put_str(args, "id", id);
 
-    response = qmp(cmd);
-    g_free(cmd);
+    response = qmp("{'execute': 'device_add', 'arguments': %p}", args);
     g_assert(response);
     g_assert(!qdict_haskey(response, "event")); /* We don't expect any events */
     g_assert(!qdict_haskey(response, "error"));
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 85bd1cf7b7..53364849bf 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -967,7 +967,8 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine));
  * qtest_qmp_device_add:
  * @driver: Name of the device that should be added
  * @id: Identification string
- * @fmt: printf-like format string for further options to device_add
+ * @fmt...: Additional arguments for device_add; formatted through
+ * json-lexer.c (only understands '%((l|ll|I64)?d|[ipsf])').
  *
  * Generic hot-plugging test via the device_add QMP command.
  */
diff --git a/tests/usb-hcd-ehci-test.c b/tests/usb-hcd-ehci-test.c
index 55d4743a2a..f28ea27f37 100644
--- a/tests/usb-hcd-ehci-test.c
+++ b/tests/usb-hcd-ehci-test.c
@@ -139,7 +139,7 @@ static void pci_ehci_port_3_hotplug(void)
 
 static void pci_ehci_port_hotplug(void)
 {
-    usb_test_hotplug("ich9-ehci-1", 3, pci_ehci_port_3_hotplug);
+    usb_test_hotplug("ich9-ehci-1", "3", pci_ehci_port_3_hotplug);
 }
 
 
diff --git a/tests/usb-hcd-ohci-test.c b/tests/usb-hcd-ohci-test.c
index 4758813d78..48ddbfd26d 100644
--- a/tests/usb-hcd-ohci-test.c
+++ b/tests/usb-hcd-ohci-test.c
@@ -19,7 +19,7 @@ static void test_ohci_init(void)
 
 static void test_ohci_hotplug(void)
 {
-    usb_test_hotplug("ohci", 1, NULL);
+    usb_test_hotplug("ohci", "1", NULL);
 }
 
 int main(int argc, char **argv)
diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index 6a7e5a2fed..a119d6d5c8 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -43,12 +43,12 @@ static void test_port_2(void)
 
 static void test_uhci_hotplug(void)
 {
-    usb_test_hotplug("uhci", 2, test_port_2);
+    usb_test_hotplug("uhci", "2", test_port_2);
 }
 
 static void test_usb_storage_hotplug(void)
 {
-    qtest_qmp_device_add("usb-storage", "usbdev0", "'drive': 'drive0'");
+    qtest_qmp_device_add("usb-storage", "usbdev0", "{'drive': 'drive0'}");
 
     qtest_qmp_device_del("usbdev0");
 }
diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c
index 5b1b681bf2..9eb24b00e4 100644
--- a/tests/usb-hcd-xhci-test.c
+++ b/tests/usb-hcd-xhci-test.c
@@ -18,13 +18,13 @@ static void test_xhci_init(void)
 
 static void test_xhci_hotplug(void)
 {
-    usb_test_hotplug("xhci", 1, NULL);
+    usb_test_hotplug("xhci", "1", NULL);
 }
 
 static void test_usb_uas_hotplug(void)
 {
-    qtest_qmp_device_add("usb-uas", "uas", NULL);
-    qtest_qmp_device_add("scsi-hd", "scsihd", "'drive': 'drive0'");
+    qtest_qmp_device_add("usb-uas", "uas", "{}");
+    qtest_qmp_device_add("scsi-hd", "scsihd", "{'drive': 'drive0'}");
 
     /* TODO:
         UAS HBA driver in libqos, to check that
@@ -37,10 +37,10 @@ static void test_usb_uas_hotplug(void)
 
 static void test_usb_ccid_hotplug(void)
 {
-    qtest_qmp_device_add("usb-ccid", "ccid", NULL);
+    qtest_qmp_device_add("usb-ccid", "ccid", "{}");
     qtest_qmp_device_del("ccid");
     /* check the device can be added again */
-    qtest_qmp_device_add("usb-ccid", "ccid", NULL);
+    qtest_qmp_device_add("usb-ccid", "ccid", "{}");
     qtest_qmp_device_del("ccid");
 }
 
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 9be9ffb378..d96b4c69e1 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -662,8 +662,9 @@ static void pci_hotplug(void)
     qs = pci_test_start();
 
     /* plug secondary disk */
-    qpci_plug_device_test("virtio-blk-pci", "drv1", PCI_SLOT_HP,
-                          "'drive': 'drive1'");
+    qtest_qmp_device_add("virtio-blk-pci", "drv1",
+                         "{'addr': %s, 'drive': 'drive1'}",
+                         stringify(PCI_SLOT_HP));
 
     dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT_HP);
     g_assert(dev);
diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index b285a262e9..dcb87a8b6e 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -249,7 +249,8 @@ static void hotplug(void)
 
     qtest_start("-device virtio-net-pci");
 
-    qpci_plug_device_test("virtio-net-pci", "net1", PCI_SLOT_HP, NULL);
+    qtest_qmp_device_add("virtio-net-pci", "net1",
+                         "{'addr': %s}", stringify(PCI_SLOT_HP));
 
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         qpci_unplug_acpi_device_test("net1", PCI_SLOT_HP);
diff --git a/tests/virtio-rng-test.c b/tests/virtio-rng-test.c
index dcecf77463..657d9a4105 100644
--- a/tests/virtio-rng-test.c
+++ b/tests/virtio-rng-test.c
@@ -22,7 +22,8 @@ static void hotplug(void)
 {
     const char *arch = qtest_get_arch();
 
-    qpci_plug_device_test("virtio-rng-pci", "rng1", PCI_SLOT_HP, NULL);
+    qtest_qmp_device_add("virtio-rng-pci", "rng1",
+                         "{'addr': %s}", stringify(PCI_SLOT_HP));
 
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         qpci_unplug_acpi_device_test("rng1", PCI_SLOT_HP);
diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index 037872bb98..0d4f25d369 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -199,7 +199,7 @@ static void hotplug(void)
 
     qs = qvirtio_scsi_start(
             "-drive id=drv1,if=none,file=null-co://,format=raw");
-    qtest_qmp_device_add("scsi-hd", "scsihd", "'drive': 'drv1'");
+    qtest_qmp_device_add("scsi-hd", "scsihd", "{'drive': 'drv1'}");
     qtest_qmp_device_del("scsihd");
     qvirtio_scsi_stop(qs);
 }
diff --git a/tests/virtio-serial-test.c b/tests/virtio-serial-test.c
index 7cc7060264..e4b18b1c8a 100644
--- a/tests/virtio-serial-test.c
+++ b/tests/virtio-serial-test.c
@@ -18,7 +18,7 @@ static void virtio_serial_nop(void)
 
 static void hotplug(void)
 {
-    qtest_qmp_device_add("virtserialport", "hp-port", NULL);
+    qtest_qmp_device_add("virtserialport", "hp-port", "{}");
 
     qtest_qmp_device_del("hp-port");
 }
-- 
2.17.1

[Qemu-devel] [PATCH 14/20] migration-test: Make wait_command() return the "return" member

[Markus Armbruster]

All callers of wait_command() are only interested in the success
response's "return" member.  Lift its extraction into wait_command().

Cc: Juan Quintela <quintela@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/migration-test.c | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index bbb9d3da31..f9f2c0148a 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -152,7 +152,7 @@ static void wait_for_serial(const char *side)
 static QDict *wait_command(QTestState *who, const char *command)
 {
     const char *event_string;
-    QDict *response;
+    QDict *response, *ret;
 
     response = qtest_qmp(who, command);
 
@@ -165,7 +165,12 @@ static QDict *wait_command(QTestState *who, const char *command)
         qobject_unref(response);
         response = qtest_qmp_receive(who);
     }
-    return response;
+
+    ret = qdict_get_qdict(response, "return");
+    g_assert(ret);
+    qobject_ref(ret);
+    qobject_unref(response);
+    return ret;
 }
 
 /*
@@ -174,15 +179,7 @@ static QDict *wait_command(QTestState *who, const char *command)
  */
 static QDict *migrate_query(QTestState *who)
 {
-    QDict *rsp, *rsp_return;
-
-    rsp = wait_command(who, "{ 'execute': 'query-migrate' }");
-    rsp_return = qdict_get_qdict(rsp, "return");
-    g_assert(rsp_return);
-    qobject_ref(rsp_return);
-    qobject_unref(rsp);
-
-    return rsp_return;
+    return wait_command(who, "{ 'execute': 'query-migrate' }");
 }
 
 /*
@@ -324,16 +321,16 @@ static void cleanup(const char *filename)
 static void migrate_check_parameter(QTestState *who, const char *parameter,
                                     const char *value)
 {
-    QDict *rsp, *rsp_return;
+    QDict *rsp_return;
     char *result;
 
-    rsp = wait_command(who, "{ 'execute': 'query-migrate-parameters' }");
-    rsp_return = qdict_get_qdict(rsp, "return");
+    rsp_return = wait_command(who,
+                              "{ 'execute': 'query-migrate-parameters' }");
     result = g_strdup_printf("%" PRId64,
                              qdict_get_try_int(rsp_return,  parameter, -1));
     g_assert_cmpstr(result, ==, value);
     g_free(result);
-    qobject_unref(rsp);
+    qobject_unref(rsp_return);
 }
 
 static void migrate_set_parameter(QTestState *who, const char *parameter,
@@ -357,7 +354,6 @@ static void migrate_pause(QTestState *who)
     QDict *rsp;
 
     rsp = wait_command(who, "{ 'execute': 'migrate-pause' }");
-    g_assert(qdict_haskey(rsp, "return"));
     qobject_unref(rsp);
 }
 
@@ -370,7 +366,6 @@ static void migrate_recover(QTestState *who, const char *uri)
         "  'arguments': { 'uri': '%s' } }", uri);
 
     rsp = wait_command(who, cmd);
-    g_assert(qdict_haskey(rsp, "return"));
     g_free(cmd);
     qobject_unref(rsp);
 }
@@ -411,7 +406,6 @@ static void migrate_postcopy_start(QTestState *from, QTestState *to)
     QDict *rsp;
 
     rsp = wait_command(from, "{ 'execute': 'migrate-start-postcopy' }");
-    g_assert(qdict_haskey(rsp, "return"));
     qobject_unref(rsp);
 
     if (!got_stop) {
@@ -689,7 +683,7 @@ static void test_postcopy_recovery(void)
 static void test_baddest(void)
 {
     QTestState *from, *to;
-    QDict *rsp, *rsp_return;
+    QDict *rsp_return;
     char *status;
     bool failed;
 
@@ -705,12 +699,10 @@ static void test_baddest(void)
     } while (!failed);
 
     /* Is the machine currently running? */
-    rsp = wait_command(from, "{ 'execute': 'query-status' }");
-    g_assert(qdict_haskey(rsp, "return"));
-    rsp_return = qdict_get_qdict(rsp, "return");
+    rsp_return = wait_command(from, "{ 'execute': 'query-status' }");
     g_assert(qdict_haskey(rsp_return, "running"));
     g_assert(qdict_get_bool(rsp_return, "running"));
-    qobject_unref(rsp);
+    qobject_unref(rsp_return);
 
     test_migrate_end(from, to, false);
 }
-- 
2.17.1

[Qemu-devel] [PATCH 15/20] tests: New helper qtest_qmp_receive_success()

[Markus Armbruster]

Commit b21373d0713 copied wait_command() from tests/migration-test.c
to tests/tpm-util.c.  Replace both copies by new libqtest helper
qtest_qmp_receive_success().  Also use it to simplify
qtest_qmp_device_del().

Bonus: gets rid of a non-literal format string.  A step towards
compile-time format string checking without triggering
-Wformat-nonliteral.

Cc: Thomas Huth <thuth@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/libqtest.c       | 69 ++++++++++++++++++++++++++++++------------
 tests/libqtest.h       | 17 +++++++++++
 tests/migration-test.c | 29 ++++++------------
 tests/tpm-util.c       | 32 +++-----------------
 4 files changed, 80 insertions(+), 67 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index dfca3af89d..ed832cd8e6 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -1029,6 +1029,35 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine))
     qobject_unref(response);
 }
 
+QDict *qtest_qmp_receive_success(QTestState *s,
+                                 void (*event_cb)(void *opaque,
+                                                  const char *event,
+                                                  QDict *data),
+                                 void *opaque)
+{
+    QDict *response, *ret, *data;
+    const char *event;
+
+    for (;;) {
+        response = qtest_qmp_receive(s);
+        g_assert(!qdict_haskey(response, "error"));
+        ret = qdict_get_qdict(response, "return");
+        if (ret) {
+            break;
+        }
+        event = qdict_get_str(response, "event");
+        data = qdict_get_qdict(response, "data");
+        if (event_cb) {
+            event_cb(opaque, event, data);
+        }
+        qobject_unref(response);
+    }
+
+    qobject_ref(ret);
+    qobject_unref(response);
+    return ret;
+}
+
 /*
  * Generic hot-plugging test via the device_add QMP command.
  */
@@ -1053,6 +1082,14 @@ void qtest_qmp_device_add(const char *driver, const char *id,
     qobject_unref(response);
 }
 
+static void device_deleted_cb(void *opaque, const char *name, QDict *data)
+{
+    bool *got_event = opaque;
+
+    g_assert_cmpstr(name, ==, "DEVICE_DELETED");
+    *got_event = true;
+}
+
 /*
  * Generic hot-unplugging test via the device_del QMP command.
  * Device deletion will get one response and one event. For example:
@@ -1073,27 +1110,21 @@ void qtest_qmp_device_add(const char *driver, const char *id,
  */
 void qtest_qmp_device_del(const char *id)
 {
-    QDict *response1, *response2, *event = NULL;
+    bool got_event = false;
+    QDict *rsp;
 
-    response1 = qmp("{'execute': 'device_del', 'arguments': {'id': %s}}",
-                    id);
-    g_assert(response1);
-    g_assert(!qdict_haskey(response1, "error"));
-
-    response2 = qmp_receive();
-    g_assert(response2);
-    g_assert(!qdict_haskey(response2, "error"));
-
-    if (qdict_haskey(response1, "event")) {
-        event = response1;
-    } else if (qdict_haskey(response2, "event")) {
-        event = response2;
+    qtest_qmp_send(global_qtest,
+                   "{'execute': 'device_del', 'arguments': {'id': %s}}",
+                   id);
+    rsp = qtest_qmp_receive_success(global_qtest, device_deleted_cb,
+                                    &got_event);
+    qobject_unref(rsp);
+    if (!got_event) {
+        rsp = qmp_receive();
+        g_assert_cmpstr(qdict_get_try_str(rsp, "event"),
+                        ==, "DEVICE_DELETED");
+        qobject_unref(rsp);
     }
-    g_assert(event);
-    g_assert_cmpstr(qdict_get_str(event, "event"), ==, "DEVICE_DELETED");
-
-    qobject_unref(response1);
-    qobject_unref(response2);
 }
 
 bool qmp_rsp_is_err(QDict *rsp)
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 53364849bf..68b767fc88 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -163,6 +163,23 @@ void qtest_qmp_eventwait(QTestState *s, const char *event);
  */
 QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
 
+/**
+ * qtest_qmp_receive_success:
+ * @s: #QTestState instance to operate on
+ * @event_cb: Event callback
+ * @opaque: Argument for @event_cb
+ *
+ * Poll QMP messages until a command success response is received.
+ * If @event_cb, call it for each event received, passing @opaque,
+ * the event's name and data.
+ * Return the success response's "return" member.
+ */
+QDict *qtest_qmp_receive_success(QTestState *s,
+                                 void (*event_cb)(void *opaque,
+                                                  const char *name,
+                                                  QDict *data),
+                                 void *opaque);
+
 /**
  * qtest_hmp:
  * @s: #QTestState instance to operate on.
diff --git a/tests/migration-test.c b/tests/migration-test.c
index f9f2c0148a..267342fd3e 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -146,31 +146,20 @@ static void wait_for_serial(const char *side)
     } while (true);
 }
 
+static void stop_cb(void *opaque, const char *name, QDict *data)
+{
+    if (!strcmp(name, "STOP")) {
+        got_stop = true;
+    }
+}
+
 /*
  * Events can get in the way of responses we are actually waiting for.
  */
 static QDict *wait_command(QTestState *who, const char *command)
 {
-    const char *event_string;
-    QDict *response, *ret;
-
-    response = qtest_qmp(who, command);
-
-    while (qdict_haskey(response, "event")) {
-        /* OK, it was an event */
-        event_string = qdict_get_str(response, "event");
-        if (!strcmp(event_string, "STOP")) {
-            got_stop = true;
-        }
-        qobject_unref(response);
-        response = qtest_qmp_receive(who);
-    }
-
-    ret = qdict_get_qdict(response, "return");
-    g_assert(ret);
-    qobject_ref(ret);
-    qobject_unref(response);
-    return ret;
+    qtest_qmp_send(who, command);
+    return qtest_qmp_receive_success(who, stop_cb, NULL);
 }
 
 /*
diff --git a/tests/tpm-util.c b/tests/tpm-util.c
index 3bd2887f1e..9f3f156e42 100644
--- a/tests/tpm-util.c
+++ b/tests/tpm-util.c
@@ -22,8 +22,6 @@
 #define TIS_REG(LOCTY, REG) \
     (TPM_TIS_ADDR_BASE + ((LOCTY) << 12) + REG)
 
-static bool got_stop;
-
 void tpm_util_crb_transfer(QTestState *s,
                            const unsigned char *req, size_t req_size,
                            unsigned char *rsp, size_t rsp_size)
@@ -247,41 +245,19 @@ void tpm_util_migrate(QTestState *who, const char *uri)
     qobject_unref(rsp);
 }
 
-/*
- * Events can get in the way of responses we are actually waiting for.
- */
-static QDict *tpm_util_wait_command(QTestState *who, const char *command)
-{
-    const char *event_string;
-    QDict *response;
-
-    response = qtest_qmp(who, command);
-
-    while (qdict_haskey(response, "event")) {
-        /* OK, it was an event */
-        event_string = qdict_get_str(response, "event");
-        if (!strcmp(event_string, "STOP")) {
-            got_stop = true;
-        }
-        qobject_unref(response);
-        response = qtest_qmp_receive(who);
-    }
-    return response;
-}
-
 void tpm_util_wait_for_migration_complete(QTestState *who)
 {
     while (true) {
-        QDict *rsp, *rsp_return;
+        QDict *rsp_return;
         bool completed;
         const char *status;
 
-        rsp = tpm_util_wait_command(who, "{ 'execute': 'query-migrate' }");
-        rsp_return = qdict_get_qdict(rsp, "return");
+        qtest_qmp_send(who, "{ 'execute': 'query-migrate' }");
+        rsp_return = qtest_qmp_receive_success(who, NULL, NULL);
         status = qdict_get_str(rsp_return, "status");
         completed = strcmp(status, "completed") == 0;
         g_assert_cmpstr(status, !=,  "failed");
-        qobject_unref(rsp);
+        qobject_unref(rsp_return);
         if (completed) {
             return;
         }
-- 
2.17.1

[Qemu-devel] [PATCH 16/20] migration-test: Make wait_command() cope with '%'

[Markus Armbruster]

wait_command() passes its argument @command to qtest_qmp_send().
Falls apart if @command contain '%'.  Two ways to disarm this trap:
suppress interpretation of '%' by passing @command as argument to
format string "%s", or fix it by having wait_command() take the
variable arguments to go with @command.  Do the latter.

This is another step towards compile-time format string checking
without triggering -Wformat-nonliteral.

Cc: Juan Quintela <quintela@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/migration-test.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index 267342fd3e..55f9d006ec 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -156,9 +156,15 @@ static void stop_cb(void *opaque, const char *name, QDict *data)
 /*
  * Events can get in the way of responses we are actually waiting for.
  */
-static QDict *wait_command(QTestState *who, const char *command)
+GCC_FMT_ATTR(2, 3)
+static QDict *wait_command(QTestState *who, const char *command, ...)
 {
-    qtest_qmp_send(who, command);
+    va_list ap;
+
+    va_start(ap, command);
+    qtest_qmp_vsend(who, command, ap);
+    va_end(ap);
+
     return qtest_qmp_receive_success(who, stop_cb, NULL);
 }
 
-- 
2.17.1

[Qemu-devel] [PATCH 17/20] migration-test: Clean up string interpolation into QMP, part 1

[Markus Armbruster]

Leaving interpolation into JSON to qmp() is more robust than building
QMP input manually, as explained in the recent commit "tests: Clean up
string interpolation into QMP input (simple cases)".

migrate_recover() builds QMP input manually because wait_command()
can't interpolate.  Well, it can since the previous commit.  Simplify
accordingly.

Bonus: gets rid of a non-literal format string.  A step towards
compile-time format string checking without triggering
-Wformat-nonliteral.

Cc: Juan Quintela <quintela@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/migration-test.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index 55f9d006ec..66a4cd2395 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -355,13 +355,12 @@ static void migrate_pause(QTestState *who)
 static void migrate_recover(QTestState *who, const char *uri)
 {
     QDict *rsp;
-    gchar *cmd = g_strdup_printf(
-        "{ 'execute': 'migrate-recover', "
-        "  'id': 'recover-cmd', "
-        "  'arguments': { 'uri': '%s' } }", uri);
 
-    rsp = wait_command(who, cmd);
-    g_free(cmd);
+    rsp = wait_command(who,
+                       "{ 'execute': 'migrate-recover', "
+                       "  'id': 'recover-cmd', "
+                       "  'arguments': { 'uri': %s } }",
+                       uri);
     qobject_unref(rsp);
 }
 
-- 
2.17.1

[Qemu-devel] [PATCH 18/20] migration-test: Clean up string interpolation into QMP, part 2

[Markus Armbruster]

Leaving interpolation into JSON to qmp() is more robust than building
QMP input manually, as explained in the recent commit "tests: Clean up
string interpolation into QMP input (simple cases)".

migrate() interpolates members into a JSON object.  Change it to take
its extra QMP arguments as arguments for qdict_from_jsonf_nofail()
instead of a string containing JSON members.

Bonus: gets rid of a non-literal format string.  A step towards
compile-time format string checking without triggering
-Wformat-nonliteral.

Cc: Juan Quintela <quintela@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/migration-test.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index 66a4cd2395..4a4db88b61 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -14,6 +14,7 @@
 
 #include "libqtest.h"
 #include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qjson.h"
 #include "qemu/option.h"
 #include "qemu/range.h"
 #include "qemu/sockets.h"
@@ -381,16 +382,19 @@ static void migrate_set_capability(QTestState *who, const char *capability,
     qobject_unref(rsp);
 }
 
-static void migrate(QTestState *who, const char *uri, const char *extra)
+static void migrate(QTestState *who, const char *uri, const char *fmt, ...)
 {
-    QDict *rsp;
-    gchar *cmd;
+    va_list ap;
+    QDict *args, *rsp;
 
-    cmd = g_strdup_printf("{ 'execute': 'migrate',"
-                          "  'arguments': { 'uri': '%s' %s } }",
-                          uri, extra ? extra : "");
-    rsp = qtest_qmp(who, cmd);
-    g_free(cmd);
+    va_start(ap, fmt);
+    args = qdict_from_vjsonf_nofail(fmt, ap);
+    va_end(ap);
+
+    g_assert(!qdict_haskey(args, "uri"));
+    qdict_put_str(args, "uri", uri);
+
+    rsp = qmp("{ 'execute': 'migrate', 'arguments': %p}", args);
     g_assert(qdict_haskey(rsp, "return"));
     qobject_unref(rsp);
 }
@@ -582,7 +586,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
 
-    migrate(from, uri, NULL);
+    migrate(from, uri, "{}");
     g_free(uri);
 
     wait_for_migration_pass(from);
@@ -665,7 +669,7 @@ static void test_postcopy_recovery(void)
      * the newly created channel
      */
     wait_for_migration_status(from, "postcopy-paused");
-    migrate(from, uri, ", 'resume': true");
+    migrate(from, uri, "{'resume': true}");
     g_free(uri);
 
     /* Restore the postcopy bandwidth to unlimited */
@@ -684,7 +688,7 @@ static void test_baddest(void)
     if (test_migrate_start(&from, &to, "tcp:0:0", true)) {
         return;
     }
-    migrate(from, "tcp:0:0", NULL);
+    migrate(from, "tcp:0:0", "{}");
     do {
         status = migrate_query_status(from);
         g_assert(!strcmp(status, "setup") || !(strcmp(status, "failed")));
@@ -722,7 +726,7 @@ static void test_precopy_unix(void)
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
 
-    migrate(from, uri, NULL);
+    migrate(from, uri, "{}");
 
     wait_for_migration_pass(from);
 
-- 
2.17.1

[Qemu-devel] [PATCH 19/20] migration-test: Clean up string interpolation into QMP, part 3

[Markus Armbruster]

Leaving interpolation into JSON to qmp() is more robust than building
QMP input manually, as explained in the recent commit "tests: Clean up
string interpolation into QMP input (simple cases)".

migration-test.c interpolates strings into JSON in a few places:

* migrate_set_parameter() interpolates string parameter @value as a
  JSON number.  Change it to long long.  This requires changing
  migrate_check_parameter() similarly.

* migrate_set_capability() interpolates string parameter @value as a
  JSON boolean.  Change it to bool.

* deprecated_set_speed() interpolates string parameter @value as a
  JSON number.  Change it to long long.

Bonus: gets rid of non-literal format strings.  A step towards
compile-time format string checking without triggering
-Wformat-nonliteral.

Cc: Juan Quintela <quintela@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/migration-test.c | 74 +++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 45 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index 4a4db88b61..8359e2ff78 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -315,31 +315,25 @@ static void cleanup(const char *filename)
 }
 
 static void migrate_check_parameter(QTestState *who, const char *parameter,
-                                    const char *value)
+                                    long long value)
 {
     QDict *rsp_return;
-    char *result;
 
     rsp_return = wait_command(who,
                               "{ 'execute': 'query-migrate-parameters' }");
-    result = g_strdup_printf("%" PRId64,
-                             qdict_get_try_int(rsp_return,  parameter, -1));
-    g_assert_cmpstr(result, ==, value);
-    g_free(result);
+    g_assert_cmpint(qdict_get_int(rsp_return,  parameter), ==, value);
     qobject_unref(rsp_return);
 }
 
 static void migrate_set_parameter(QTestState *who, const char *parameter,
-                                  const char *value)
+                                  long long value)
 {
     QDict *rsp;
-    gchar *cmd;
 
-    cmd = g_strdup_printf("{ 'execute': 'migrate-set-parameters',"
-                          "'arguments': { '%s': %s } }",
-                          parameter, value);
-    rsp = qtest_qmp(who, cmd);
-    g_free(cmd);
+    rsp = qtest_qmp(who,
+                    "{ 'execute': 'migrate-set-parameters',"
+                    "'arguments': { %s: %lld } }",
+                    parameter, value);
     g_assert(qdict_haskey(rsp, "return"));
     qobject_unref(rsp);
     migrate_check_parameter(who, parameter, value);
@@ -366,18 +360,16 @@ static void migrate_recover(QTestState *who, const char *uri)
 }
 
 static void migrate_set_capability(QTestState *who, const char *capability,
-                                   const char *value)
+                                   bool value)
 {
     QDict *rsp;
-    gchar *cmd;
 
-    cmd = g_strdup_printf("{ 'execute': 'migrate-set-capabilities',"
-                          "'arguments': { "
-                          "'capabilities': [ { "
-                          "'capability': '%s', 'state': %s } ] } }",
-                          capability, value);
-    rsp = qtest_qmp(who, cmd);
-    g_free(cmd);
+    rsp = qtest_qmp(who,
+                    "{ 'execute': 'migrate-set-capabilities',"
+                    "'arguments': { "
+                    "'capabilities': [ { "
+                    "'capability': %s, 'state': %i } ] } }",
+                    capability, value);
     g_assert(qdict_haskey(rsp, "return"));
     qobject_unref(rsp);
 }
@@ -521,29 +513,21 @@ static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
 static void deprecated_set_downtime(QTestState *who, const double value)
 {
     QDict *rsp;
-    char *expected;
-    int64_t result_int;
 
     rsp = qtest_qmp(who,
                     "{ 'execute': 'migrate_set_downtime',"
                     " 'arguments': { 'value': %f } }", value);
     g_assert(qdict_haskey(rsp, "return"));
     qobject_unref(rsp);
-    result_int = value * 1000L;
-    expected = g_strdup_printf("%" PRId64, result_int);
-    migrate_check_parameter(who, "downtime-limit", expected);
-    g_free(expected);
+    migrate_check_parameter(who, "downtime-limit", value * 1000);
 }
 
-static void deprecated_set_speed(QTestState *who, const char *value)
+static void deprecated_set_speed(QTestState *who, long long value)
 {
     QDict *rsp;
-    gchar *cmd;
 
-    cmd = g_strdup_printf("{ 'execute': 'migrate_set_speed',"
-                          "'arguments': { 'value': %s } }", value);
-    rsp = qtest_qmp(who, cmd);
-    g_free(cmd);
+    rsp = qtest_qmp(who, "{ 'execute': 'migrate_set_speed',"
+                          "'arguments': { 'value': %lld } }", value);
     g_assert(qdict_haskey(rsp, "return"));
     qobject_unref(rsp);
     migrate_check_parameter(who, "max-bandwidth", value);
@@ -556,7 +540,7 @@ static void test_deprecated(void)
     from = qtest_start("");
 
     deprecated_set_downtime(from, 0.12345);
-    deprecated_set_speed(from, "12345");
+    deprecated_set_speed(from, 12345);
 
     qtest_quit(from);
 }
@@ -572,16 +556,16 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
         return -1;
     }
 
-    migrate_set_capability(from, "postcopy-ram", "true");
-    migrate_set_capability(to, "postcopy-ram", "true");
-    migrate_set_capability(to, "postcopy-blocktime", "true");
+    migrate_set_capability(from, "postcopy-ram", true);
+    migrate_set_capability(to, "postcopy-ram", true);
+    migrate_set_capability(to, "postcopy-blocktime", true);
 
     /* We want to pick a speed slow enough that the test completes
      * quickly, but that it doesn't complete precopy even on a slow
      * machine, so also set the downtime.
      */
-    migrate_set_parameter(from, "max-bandwidth", "100000000");
-    migrate_set_parameter(from, "downtime-limit", "1");
+    migrate_set_parameter(from, "max-bandwidth", 100000000);
+    migrate_set_parameter(from, "downtime-limit", 1);
 
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
@@ -632,7 +616,7 @@ static void test_postcopy_recovery(void)
     }
 
     /* Turn postcopy speed down, 4K/s is slow enough on any machines */
-    migrate_set_parameter(from, "max-postcopy-bandwidth", "4096");
+    migrate_set_parameter(from, "max-postcopy-bandwidth", 4096);
 
     /* Now we start the postcopy */
     migrate_postcopy_start(from, to);
@@ -673,7 +657,7 @@ static void test_postcopy_recovery(void)
     g_free(uri);
 
     /* Restore the postcopy bandwidth to unlimited */
-    migrate_set_parameter(from, "max-postcopy-bandwidth", "0");
+    migrate_set_parameter(from, "max-postcopy-bandwidth", 0);
 
     migrate_postcopy_complete(from, to);
 }
@@ -719,9 +703,9 @@ static void test_precopy_unix(void)
      * machine, so also set the downtime.
      */
     /* 1 ms should make it not converge*/
-    migrate_set_parameter(from, "downtime-limit", "1");
+    migrate_set_parameter(from, "downtime-limit", 1);
     /* 1GB/s */
-    migrate_set_parameter(from, "max-bandwidth", "1000000000");
+    migrate_set_parameter(from, "max-bandwidth", 1000000000);
 
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");
@@ -731,7 +715,7 @@ static void test_precopy_unix(void)
     wait_for_migration_pass(from);
 
     /* 300 ms should converge */
-    migrate_set_parameter(from, "downtime-limit", "300");
+    migrate_set_parameter(from, "downtime-limit", 300);
 
     if (!got_stop) {
         qtest_qmp_eventwait(from, "STOP");
-- 
2.17.1

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

[Markus Armbruster]

qtest_qmp() & friends pass their format string and variable arguments
to qobject_from_vjsonf_nofail().  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 | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index 68b767fc88..087d6793eb 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -81,7 +81,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:
@@ -91,7 +92,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_qmp_send:
@@ -101,7 +103,8 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
  */
-void qtest_qmp_send(QTestState *s, const char *fmt, ...);
+void qtest_qmp_send(QTestState *s, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);
 
 /**
  * qtest_qmpv_discard_response:
@@ -112,7 +115,8 @@ void qtest_qmp_send(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:
@@ -123,7 +127,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_qmp_vsend:
@@ -134,7 +139,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_qmp_vsend(QTestState *s, const char *fmt, va_list ap);
+void qtest_qmp_vsend(QTestState *s, const char *fmt, va_list ap)
+    GCC_FMT_ATTR(2, 0);
 
 /**
  * qtest_receive:
@@ -590,7 +596,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_send:
@@ -599,7 +605,7 @@ QDict *qmp(const char *fmt, ...);
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
  */
-void qmp_send(const char *fmt, ...);
+void qmp_send(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 
 /**
  * qmp_discard_response:
@@ -608,7 +614,7 @@ void qmp_send(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:
@@ -967,10 +973,10 @@ static inline int64_t clock_set(int64_t val)
 }
 
 QDict *qmp_fd_receive(int fd);
-void qmp_fd_vsend(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_vsend(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.17.1

Re: [Qemu-devel] [PATCH 14/20] migration-test: Make wait_command() return the "return" member

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> All callers of wait_command() are only interested in the success
> response's "return" member.  Lift its extraction into wait_command().
>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Nice simplification.

Re: [Qemu-devel] [PATCH 15/20] tests: New helper qtest_qmp_receive_success()

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Commit b21373d0713 copied wait_command() from tests/migration-test.c
> to tests/tpm-util.c.  Replace both copies by new libqtest helper
> qtest_qmp_receive_success().  Also use it to simplify
> qtest_qmp_device_del().
>
> Bonus: gets rid of a non-literal format string.  A step towards
> compile-time format string checking without triggering
> -Wformat-nonliteral.
>
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>


Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH 16/20] migration-test: Make wait_command() cope with '%'

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> wait_command() passes its argument @command to qtest_qmp_send().
> Falls apart if @command contain '%'.  Two ways to disarm this trap:
> suppress interpretation of '%' by passing @command as argument to
> format string "%s", or fix it by having wait_command() take the
> variable arguments to go with @command.  Do the latter.
>
> This is another step towards compile-time format string checking
> without triggering -Wformat-nonliteral.
>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

much better.

Re: [Qemu-devel] [PATCH 17/20] migration-test: Clean up string interpolation into QMP, part 1

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Leaving interpolation into JSON to qmp() is more robust than building
> QMP input manually, as explained in the recent commit "tests: Clean up
> string interpolation into QMP input (simple cases)".
>
> migrate_recover() builds QMP input manually because wait_command()
> can't interpolate.  Well, it can since the previous commit.  Simplify
> accordingly.
>
> Bonus: gets rid of a non-literal format string.  A step towards
> compile-time format string checking without triggering
> -Wformat-nonliteral.
>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Why, o why it needs to be so difficult!!!

Re: [Qemu-devel] [PATCH 18/20] migration-test: Clean up string interpolation into QMP, part 2

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Leaving interpolation into JSON to qmp() is more robust than building
> QMP input manually, as explained in the recent commit "tests: Clean up
> string interpolation into QMP input (simple cases)".
>
> migrate() interpolates members into a JSON object.  Change it to take
> its extra QMP arguments as arguments for qdict_from_jsonf_nofail()
> instead of a string containing JSON members.
>
> Bonus: gets rid of a non-literal format string.  A step towards
> compile-time format string checking without triggering
> -Wformat-nonliteral.
>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>


Reviewed-by: Juan Quintela <quintela@redhat.com>

Again, much clearen.  Thanks.

Re: [Qemu-devel] [PATCH 19/20] migration-test: Clean up string interpolation into QMP, part 3

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Leaving interpolation into JSON to qmp() is more robust than building
> QMP input manually, as explained in the recent commit "tests: Clean up
> string interpolation into QMP input (simple cases)".
>
> migration-test.c interpolates strings into JSON in a few places:
>
> * migrate_set_parameter() interpolates string parameter @value as a
>   JSON number.  Change it to long long.  This requires changing
>   migrate_check_parameter() similarly.
>
> * migrate_set_capability() interpolates string parameter @value as a
>   JSON boolean.  Change it to bool.
>
> * deprecated_set_speed() interpolates string parameter @value as a
>   JSON number.  Change it to long long.
>
> Bonus: gets rid of non-literal format strings.  A step towards
> compile-time format string checking without triggering
> -Wformat-nonliteral.
>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

I did it someway because at some point I was also using strings, not
only ints.  But I came to conclusion that it is much better to just
having different functions for different types.

Thanks, again.

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

[Philippe Mathieu-Daudé]

On 07/12/2018 08:12 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>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  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 7e3491b5bd..19a7ab1bca 100644
> --- a/tests/ahci-test.c
> +++ b/tests/ahci-test.c
> @@ -1352,7 +1352,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");
> @@ -1388,8 +1387,7 @@ static void test_flush_migrate(void)
>      ahci_migrate(src, dst, uri);
>  
>      /* Complete the command */
> -    s = "{'execute':'cont' }";
> -    qmp_send(s);
> +    qmp_send("{'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 2384c2c3e2..834ee9005b 100644
> --- a/tests/ide-test.c
> +++ b/tests/ide-test.c
> @@ -694,7 +694,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");
>  
> @@ -722,8 +721,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);
> 

Re: [Qemu-devel] [PATCH 06/20] qobject: New qobject_from_vjsonf_nofail(), qdict_from_vjsonf_nofail()

[Philippe Mathieu-Daudé]

On 07/12/2018 08:12 AM, Markus Armbruster wrote:
> Every printf()-like function sooner or later needs its vprintf()-like
> buddy.  The next commit will need qobject_from_jsonf_nofail()'s buddy,
> and qdict_from_jsonf_nofail()'s buddy will be used later in this
> series.  Add both.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/qmp/qjson.h |  4 ++++
>  qobject/qjson.c          | 44 +++++++++++++++++++++++++++++++++-------
>  2 files changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
> index dc509d51ae..0db121bef9 100644
> --- a/include/qapi/qmp/qjson.h
> +++ b/include/qapi/qmp/qjson.h
> @@ -18,8 +18,12 @@ QObject *qobject_from_json(const char *string, Error **errp);
>  QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
>      GCC_FMT_ATTR(1, 0);
>  
> +QObject *qobject_from_vjsonf_nofail(const char *string, va_list)

..., va_list ap)?

> +    GCC_FMT_ATTR(1, 0);
>  QObject *qobject_from_jsonf_nofail(const char *string, ...)
>      GCC_FMT_ATTR(1, 2);
> +QDict *qdict_from_vjsonf_nofail(const char *string, va_list ap)
> +    GCC_FMT_ATTR(1, 0);
>  QDict *qdict_from_jsonf_nofail(const char *string, ...)
>      GCC_FMT_ATTR(1, 2);
>  
> diff --git a/qobject/qjson.c b/qobject/qjson.c
> index 4a9dcff343..2e450231ff 100644
> --- a/qobject/qjson.c
> +++ b/qobject/qjson.c
> @@ -59,6 +59,25 @@ QObject *qobject_from_json(const char *string, Error **errp)
>      return qobject_from_jsonv(string, NULL, errp);
>  }
>  
> +/*
> + * Parse @string as JSON value with %-escapes interpolated.
> + * Abort on error.  Do not use with untrusted @string.
> + * Return the resulting QObject.  It is never null.
> + */
> +QObject *qobject_from_vjsonf_nofail(const char *string, va_list ap)
> +{
> +    va_list ap_copy;
> +    QObject *obj;
> +
> +    /* va_copy() is needed when va_list is an array type */
> +    va_copy(ap_copy, ap);
> +    obj = qobject_from_jsonv(string, &ap_copy, &error_abort);
> +    va_end(ap_copy);
> +
> +    assert(obj);
> +    return obj;
> +}
> +
>  /*
>   * Parse @string as JSON value with %-escapes interpolated.
>   * Abort on error.  Do not use with untrusted @string.
> @@ -70,13 +89,26 @@ QObject *qobject_from_jsonf_nofail(const char *string, ...)
>      va_list ap;
>  
>      va_start(ap, string);
> -    obj = qobject_from_jsonv(string, &ap, &error_abort);
> +    obj = qobject_from_vjsonf_nofail(string, ap);
>      va_end(ap);
>  
> -    assert(obj);
>      return obj;
>  }
>  
> +/*
> + * Parse @string as JSON object with %-escapes interpolated.
> + * Abort on error.  Do not use with untrusted @string.
> + * Return the resulting QDict.  It is never null.
> + */
> +QDict *qdict_from_vjsonf_nofail(const char *string, va_list ap)
> +{
> +    QDict *qdict;
> +
> +    qdict = qobject_to(QDict, qobject_from_vjsonf_nofail(string, ap));
> +    assert(qdict);
> +    return qdict;
> +}
> +
>  /*
>   * Parse @string as JSON object with %-escapes interpolated.
>   * Abort on error.  Do not use with untrusted @string.
> @@ -84,15 +116,13 @@ QObject *qobject_from_jsonf_nofail(const char *string, ...)
>   */
>  QDict *qdict_from_jsonf_nofail(const char *string, ...)
>  {
> -    QDict *obj;
> +    QDict *qdict;
>      va_list ap;
>  
>      va_start(ap, string);
> -    obj = qobject_to(QDict, qobject_from_jsonv(string, &ap, &error_abort));
> +    qdict = qdict_from_vjsonf_nofail(string, ap);
>      va_end(ap);
> -
> -    assert(obj);
> -    return obj;
> +    return qdict;
>  }
>  
>  typedef struct ToJsonIterState
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [PATCH 07/20] libqtest: Simplify qmp_fd_vsend() a bit

[Philippe Mathieu-Daudé]

On 07/12/2018 08:12 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/libqtest.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index c2c08a890c..3bfb062fcb 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -488,24 +488,20 @@ QDict *qtest_qmp_receive(QTestState *s)
>   */
>  void qmp_fd_vsend(int fd, const char *fmt, va_list ap)
>  {
> -    va_list ap_copy;
>      QObject *qobj;
>  
> -    /* qobject_from_jsonv() silently eats leading 0xff as invalid
> -     * JSON, but we want to test sending them over the wire to force
> -     * resyncs */
> +    /*
> +     * qobject_from_vjsonf_nofail() chokes on leading 0xff as invalid
> +     * JSON, but tests/test-qga.c needs to send that to test QGA
> +     * synchronization
> +     */
>      if (*fmt == '\377') {
>          socket_send(fd, fmt, 1);
>          fmt++;
>      }
>  
> -    /* Going through qobject ensures we escape strings properly.
> -     * This seemingly unnecessary copy is required in case va_list
> -     * is an array type.
> -     */
> -    va_copy(ap_copy, ap);
> -    qobj = qobject_from_jsonv(fmt, &ap_copy, &error_abort);
> -    va_end(ap_copy);
> +    /* Going through qobject ensures we escape strings properly */
> +    qobj = qobject_from_vjsonf_nofail(fmt, ap);
>  
>      /* No need to send anything for an empty QObject.  */
>      if (qobj) {
> 

Re: [Qemu-devel] [PATCH 09/20] qobject: qobject_from_jsonv() is dangerous, hide it away

[Philippe Mathieu-Daudé]

On 07/12/2018 08:12 AM, Markus Armbruster wrote:
> qobject_from_jsonv() takes ownership of %p arguments.  On failure, we
> can't generally know whether we failed before or after %p, so
> ownership becomes indeterminate.  To avoid leaks, callers passing %p
> must terminate on error, e.g. by passing &error_abort.  Trap for the
> unwary; document and give the function internal linkage.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  include/qapi/qmp/qjson.h |  2 --
>  qobject/qjson.c          | 13 ++++++++++++-
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
> index 0db121bef9..4a225cf7d1 100644
> --- a/include/qapi/qmp/qjson.h
> +++ b/include/qapi/qmp/qjson.h
> @@ -15,8 +15,6 @@
>  #define QJSON_H
>  
>  QObject *qobject_from_json(const char *string, Error **errp);
> -QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
> -    GCC_FMT_ATTR(1, 0);
>  
>  QObject *qobject_from_vjsonf_nofail(const char *string, va_list)
>      GCC_FMT_ATTR(1, 0);
> diff --git a/qobject/qjson.c b/qobject/qjson.c
> index 2e450231ff..ab4040f235 100644
> --- a/qobject/qjson.c
> +++ b/qobject/qjson.c
> @@ -39,7 +39,18 @@ static void parse_json(JSONMessageParser *parser, GQueue *tokens)
>      s->result = json_parser_parse_err(tokens, s->ap, &s->err);
>  }
>  
> -QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
> +/*
> + * Parse @string as JSON value.
> + * If @ap is non-null, interpolate %-escapes.
> + * Takes ownership of %p arguments.
> + * On success, return the JSON value.
> + * On failure, store an error through @errp and return NULL.
> + * Ownership of %p arguments becomes indeterminate then.  To avoid
> + * leaks, callers passing %p must terminate on error, e.g. by passing
> + * &error_abort.
> + */
> +static QObject *qobject_from_jsonv(const char *string, va_list *ap,
> +                                   Error **errp)
>  {
>      JSONParsingState state = {};
>  
> 

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

[Philippe Mathieu-Daudé]

On 07/12/2018 08:12 AM, Markus Armbruster wrote:
> qtest_qmp() & friends pass their format string and variable arguments
> to qobject_from_vjsonf_nofail().  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>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/libqtest.h | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 68b767fc88..087d6793eb 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -81,7 +81,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:
> @@ -91,7 +92,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_qmp_send:
> @@ -101,7 +103,8 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
>   *
>   * Sends a QMP message to QEMU and leaves the response in the stream.
>   */
> -void qtest_qmp_send(QTestState *s, const char *fmt, ...);
> +void qtest_qmp_send(QTestState *s, const char *fmt, ...)
> +    GCC_FMT_ATTR(2, 3);
>  
>  /**
>   * qtest_qmpv_discard_response:
> @@ -112,7 +115,8 @@ void qtest_qmp_send(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:
> @@ -123,7 +127,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_qmp_vsend:
> @@ -134,7 +139,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_qmp_vsend(QTestState *s, const char *fmt, va_list ap);
> +void qtest_qmp_vsend(QTestState *s, const char *fmt, va_list ap)
> +    GCC_FMT_ATTR(2, 0);
>  
>  /**
>   * qtest_receive:
> @@ -590,7 +596,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_send:
> @@ -599,7 +605,7 @@ QDict *qmp(const char *fmt, ...);
>   *
>   * Sends a QMP message to QEMU and leaves the response in the stream.
>   */
> -void qmp_send(const char *fmt, ...);
> +void qmp_send(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>  
>  /**
>   * qmp_discard_response:
> @@ -608,7 +614,7 @@ void qmp_send(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:
> @@ -967,10 +973,10 @@ static inline int64_t clock_set(int64_t val)
>  }
>  
>  QDict *qmp_fd_receive(int fd);
> -void qmp_fd_vsend(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_vsend(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:
> 

Re: [Qemu-devel] [PATCH 02/20] libqtest: Rename functions to send QMP messages

[Philippe Mathieu-Daudé]

Hi Markus,

On 07/12/2018 08:12 AM, Markus Armbruster wrote:
> The functions to receive messages are called qtest_qmp_receive() and
> qmp_receive(), qmp_fd_receive().  The ones to send messages are called
> qtest_async_qmp(), qtest_async_qmpv(), qmp_async(), qmp_fd_send(),
> qmp_fd_sendv().  Inconsistent.  Rename the *_async* ones to
> qmp_send(), qtest_qmp_send(), qtest_qmp_vsend().  Rename
> qmp_fd_sendv() to qmp_fd_vsend().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/ahci-test.c   | 10 +++++-----
>  tests/libqos/ahci.c |  4 ++--
>  tests/libqtest.c    | 20 ++++++++++----------
>  tests/libqtest.h    | 14 +++++++-------
>  tests/qmp-test.c    | 18 +++++++++---------

You should setup scripts/git.orderfile so when reviewing we first see
the changes in the header, then in the C files.

>  5 files changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/tests/ahci-test.c b/tests/ahci-test.c
> index 1a7b761304..7e3491b5bd 100644
> --- a/tests/ahci-test.c
> +++ b/tests/ahci-test.c
> @@ -1389,7 +1389,7 @@ static void test_flush_migrate(void)
>  
>      /* Complete the command */
>      s = "{'execute':'cont' }";
> -    qmp_async(s);
> +    qmp_send(s);
>      qmp_eventwait("RESUME");
>      ahci_command_wait(dst, cmd);
>      ahci_command_verify(dst, cmd);
> @@ -1592,8 +1592,8 @@ static void test_atapi_tray(void)
>      atapi_wait_tray(false);
>  
>      /* Remove media */
> -    qmp_async("{'execute': 'blockdev-open-tray', "
> -               "'arguments': {'id': 'cd0'}}");
> +    qmp_send("{'execute': 'blockdev-open-tray',"
> +             " 'arguments': {'id': 'cd0'}}");
>      atapi_wait_tray(true);
>      rsp = qmp_receive();
>      qobject_unref(rsp);
> @@ -1619,8 +1619,8 @@ static void test_atapi_tray(void)
>                                           "'node-name': 'node0' }}");
>  
>      /* Again, the event shows up first */
> -    qmp_async("{'execute': 'blockdev-close-tray', "
> -               "'arguments': {'id': 'cd0'}}");
> +    qmp_send("{'execute': 'blockdev-close-tray',"
> +             " 'arguments': {'id': 'cd0'}}");
>      atapi_wait_tray(false);
>      rsp = qmp_receive();
>      qobject_unref(rsp);
> diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
> index 42d3f76933..63fbc9e3c9 100644
> --- a/tests/libqos/ahci.c
> +++ b/tests/libqos/ahci.c
> @@ -674,7 +674,7 @@ void ahci_exec(AHCIQState *ahci, uint8_t port,
>          g_assert_cmpint(rc, ==, 0);
>      }
>      if (opts->error) {
> -        qtest_async_qmp(ahci->parent->qts, "{'execute':'cont' }");
> +        qtest_qmp_send(ahci->parent->qts, "{'execute':'cont' }");
>          qtest_qmp_eventwait(ahci->parent->qts, "RESUME");
>      }
>  
> @@ -712,7 +712,7 @@ AHCICommand *ahci_guest_io_halt(AHCIQState *ahci, uint8_t port,
>  void ahci_guest_io_resume(AHCIQState *ahci, AHCICommand *cmd)
>  {
>      /* Complete the command */
> -    qtest_async_qmp(ahci->parent->qts, "{'execute':'cont' }");
> +    qtest_qmp_send(ahci->parent->qts, "{'execute':'cont' }");
>      qtest_qmp_eventwait(ahci->parent->qts, "RESUME");
>      ahci_command_wait(ahci, cmd);
>      ahci_command_verify(ahci, cmd);
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 098af6aec4..9cb4096639 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -484,7 +484,7 @@ QDict *qtest_qmp_receive(QTestState *s)
>   * in the case that they choose to discard all replies up until
>   * a particular EVENT is received.
>   */
> -void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
> +void qmp_fd_vsend(int fd, const char *fmt, va_list ap)
>  {
>      va_list ap_copy;
>      QObject *qobj;
> @@ -529,21 +529,21 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
>      }
>  }
>  
> -void qtest_async_qmpv(QTestState *s, const char *fmt, va_list ap)
> +void qtest_qmp_vsend(QTestState *s, const char *fmt, va_list ap)
>  {
> -    qmp_fd_sendv(s->qmp_fd, fmt, ap);
> +    qmp_fd_vsend(s->qmp_fd, fmt, ap);
>  }
>  
>  QDict *qmp_fdv(int fd, const char *fmt, va_list ap)
>  {
> -    qmp_fd_sendv(fd, fmt, ap);
> +    qmp_fd_vsend(fd, fmt, ap);
>  
>      return qmp_fd_receive(fd);
>  }
>  
>  QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
>  {
> -    qtest_async_qmpv(s, fmt, ap);
> +    qtest_qmp_vsend(s, fmt, ap);
>  
>      /* Receive reply */
>      return qtest_qmp_receive(s);
> @@ -565,7 +565,7 @@ void qmp_fd_send(int fd, const char *fmt, ...)
>      va_list ap;
>  
>      va_start(ap, fmt);
> -    qmp_fd_sendv(fd, fmt, ap);
> +    qmp_fd_vsend(fd, fmt, ap);
>      va_end(ap);
>  }
>  
> @@ -580,12 +580,12 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...)
>      return response;
>  }
>  
> -void qtest_async_qmp(QTestState *s, const char *fmt, ...)
> +void qtest_qmp_send(QTestState *s, const char *fmt, ...)
>  {
>      va_list ap;
>  
>      va_start(ap, fmt);
> -    qtest_async_qmpv(s, fmt, ap);
> +    qtest_qmp_vsend(s, fmt, ap);
>      va_end(ap);
>  }
>  
> @@ -968,12 +968,12 @@ QDict *qmp(const char *fmt, ...)
>      return response;
>  }
>  
> -void qmp_async(const char *fmt, ...)
> +void qmp_send(const char *fmt, ...)
>  {
>      va_list ap;
>  
>      va_start(ap, fmt);
> -    qtest_async_qmpv(global_qtest, fmt, ap);
> +    qtest_qmp_vsend(global_qtest, fmt, ap);
>      va_end(ap);
>  }
>  
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index c63551f14f..85bd1cf7b7 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -94,14 +94,14 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
>  QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
>  
>  /**
> - * qtest_async_qmp:
> + * qtest_qmp_send:
>   * @s: #QTestState instance to operate on.
>   * @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.
>   */
> -void qtest_async_qmp(QTestState *s, const char *fmt, ...);
> +void qtest_qmp_send(QTestState *s, const char *fmt, ...);
>  
>  /**
>   * qtest_qmpv_discard_response:
> @@ -126,7 +126,7 @@ void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap);
>  QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
>  
>  /**
> - * qtest_async_qmpv:
> + * qtest_qmp_vsend:
>   * @s: #QTestState instance to operate on.
>   * @fmt: QMP message to send to QEMU; formats arguments through
>   * json-lexer.c (only understands '%((l|ll|I64)?d|[ipsf])').
> @@ -134,7 +134,7 @@ 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_qmp_vsend(QTestState *s, const char *fmt, va_list ap);
>  
>  /**
>   * qtest_receive:
> @@ -576,13 +576,13 @@ static inline void qtest_end(void)
>  QDict *qmp(const char *fmt, ...);
>  
>  /**
> - * qmp_async:
> + * qmp_send:
>   * @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.
>   */
> -void qmp_async(const char *fmt, ...);
> +void qmp_send(const char *fmt, ...);
>  
>  /**
>   * qmp_discard_response:
> @@ -950,7 +950,7 @@ 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_vsend(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, ...);
> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> index ceaf4a6789..60685bba3c 100644
> --- a/tests/qmp-test.c
> +++ b/tests/qmp-test.c
> @@ -159,12 +159,12 @@ static void cleanup_blocking_cmd(void)
>  
>  static void send_cmd_that_blocks(QTestState *s, const char *id)
>  {
> -    qtest_async_qmp(s, "{ 'execute': 'blockdev-add',  'id': %s,"
> -                    " 'arguments': {"
> -                    " 'driver': 'blkdebug', 'node-name': %s,"
> -                    " 'config': %s,"
> -                    " 'image': { 'driver': 'null-co' } } }",
> -                    id, id, fifo_name);
> +    qtest_qmp_send(s, "{ 'execute': 'blockdev-add',  'id': %s,"
> +                   " 'arguments': {"
> +                   " 'driver': 'blkdebug', 'node-name': %s,"
> +                   " 'config': %s,"
> +                   " 'image': { 'driver': 'null-co' } } }",
> +                   id, id, fifo_name);
>  }
>  
>  static void unblock_blocked_cmd(void)
> @@ -176,7 +176,7 @@ static void unblock_blocked_cmd(void)
>  
>  static void send_oob_cmd_that_fails(QTestState *s, const char *id)
>  {
> -    qtest_async_qmp(s, "{ 'exec-oob': 'migrate-pause', 'id': %s }", id);
> +    qtest_qmp_send(s, "{ 'exec-oob': 'migrate-pause', 'id': %s }", id);
>  }
>  
>  static void recv_cmd_id(QTestState *s, const char *id)
> @@ -235,7 +235,7 @@ static void test_qmp_oob(void)
>      /* OOB command overtakes slow in-band command */
>      setup_blocking_cmd();
>      send_cmd_that_blocks(qts, "ib-blocks-1");
> -    qtest_async_qmp(qts, "{ 'execute': 'query-name', 'id': 'ib-quick-1' }");
> +    qtest_qmp_send(qts, "{ 'execute': 'query-name', 'id': 'ib-quick-1' }");
>      send_oob_cmd_that_fails(qts, "oob-1");
>      recv_cmd_id(qts, "oob-1");
>      unblock_blocked_cmd();
> @@ -244,7 +244,7 @@ static void test_qmp_oob(void)
>  
>      /* Even malformed in-band command fails in-band */
>      send_cmd_that_blocks(qts, "blocks-2");
> -    qtest_async_qmp(qts, "{ 'id': 'err-2' }");
> +    qtest_qmp_send(qts, "{ 'id': 'err-2' }");
>      unblock_blocked_cmd();
>      recv_cmd_id(qts, "blocks-2");
>      recv_cmd_id(qts, "err-2");
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [PATCH 03/20] libqtest: Clean up how we read device_del messages

[Philippe Mathieu-Daudé]

On 07/12/2018 08:12 AM, Markus Armbruster wrote:
> qtest_qmp_device_del() still uses the qmp("") hack to receive a
> message, even though we have qmp_receive() since commit 66e0c7b187e.
> Put it to use.
> 
> 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: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/libqtest.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 9cb4096639..071d7eb7b1 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -1090,7 +1090,7 @@ void qtest_qmp_device_del(const char *id)
>      g_assert(response1);
>      g_assert(!qdict_haskey(response1, "error"));
>  
> -    response2 = qmp("");
> +    response2 = qmp_receive();
>      g_assert(response2);
>      g_assert(!qdict_haskey(response2, "error"));
>  
> 

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

[Philippe Mathieu-Daudé]

Hi Markus,

On 07/12/2018 08:12 AM, Markus Armbruster wrote:
> When you build QMP input manually like this
> 
>     cmd = g_strdup_printf("{ 'execute': 'migrate',"
>                           "'arguments': { 'uri': '%s' } }",
>                           uri);
>     rsp = qmp(cmd);
>     g_free(cmd);
> 
> you're responsible for escaping the interpolated values for JSON.  Not
> done here, and therefore works only for sufficiently nice @uri.  For
> instance, if @uri contained a single "'", qobject_from_vjsonf_nofail()
> would abort.  A sufficiently nasty @uri could even inject unwanted
> members into the arguments object.
> 
> Leaving interpolation into JSON to qmp() is more robust:
> 
>     rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
> 
> It's also more concise.
> 
> Clean up the simple cases where we interpolate exactly a JSON value.
> 
> Bonus: gets rid of non-literal format strings.  A step towards
> compile-time format string checking without triggering
> -Wformat-nonliteral.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/libqos/pci-pc.c   |   9 +--
>  tests/libqtest.c        |   7 +-
>  tests/migration-test.c  |   8 +--
>  tests/test-qga.c        | 150 ++++++++++++++++++----------------------
>  tests/tpm-util.c        |   9 +--
>  tests/vhost-user-test.c |   6 +-
>  6 files changed, 77 insertions(+), 112 deletions(-)
> 
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index a7803308b7..585f5289ec 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -160,14 +160,9 @@ void qpci_free_pc(QPCIBus *bus)
>  void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
>  {
>      QDict *response;
> -    char *cmd;
>  
> -    cmd = g_strdup_printf("{'execute': 'device_del',"
> -                          " 'arguments': {"
> -                          "   'id': '%s'"
> -                          "}}", id);
> -    response = qmp(cmd);
> -    g_free(cmd);
> +    response = qmp("{'execute': 'device_del', 'arguments': {'id': %s}}",
> +                   id);
>      g_assert(response);
>      g_assert(!qdict_haskey(response, "error"));
>      qobject_unref(response);
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 3bfb062fcb..e36cc5ede3 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -1079,12 +1079,9 @@ void qtest_qmp_device_add(const char *driver, const char *id, const char *fmt,
>  void qtest_qmp_device_del(const char *id)
>  {
>      QDict *response1, *response2, *event = NULL;
> -    char *cmd;
>  
> -    cmd = g_strdup_printf("{'execute': 'device_del',"
> -                          " 'arguments': { 'id': '%s' }}", id);
> -    response1 = qmp(cmd);
> -    g_free(cmd);
> +    response1 = qmp("{'execute': 'device_del', 'arguments': {'id': %s}}",
> +                    id);
>      g_assert(response1);
>      g_assert(!qdict_haskey(response1, "error"));
>  
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index 086f727b34..bbb9d3da31 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -529,14 +529,12 @@ static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
>  static void deprecated_set_downtime(QTestState *who, const double value)
>  {
>      QDict *rsp;
> -    gchar *cmd;
>      char *expected;
>      int64_t result_int;
>  
> -    cmd = g_strdup_printf("{ 'execute': 'migrate_set_downtime',"
> -                          "'arguments': { 'value': %g } }", value);
> -    rsp = qtest_qmp(who, cmd);
> -    g_free(cmd);
> +    rsp = qtest_qmp(who,
> +                    "{ 'execute': 'migrate_set_downtime',"
> +                    " 'arguments': { 'value': %f } }", value);

I suppose you changed %g -> %f because the downtime is expected
to be greater than 1e-4 :)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>      g_assert(qdict_haskey(rsp, "return"));
>      qobject_unref(rsp);
>      result_int = value * 1000L;
> diff --git a/tests/test-qga.c b/tests/test-qga.c
> index d638b1571a..c552cc0125 100644
> --- a/tests/test-qga.c
> +++ b/tests/test-qga.c
> @@ -146,12 +146,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.
> @@ -188,7 +187,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
> @@ -201,10 +199,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);
> @@ -428,7 +425,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;
> @@ -446,10 +443,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);
>  
> @@ -459,23 +456,20 @@ static void test_qga_file_ops(gconstpointer fix)
>      g_assert_cmpint(count, ==, sizeof(helloworld));
>      g_assert_cmpint(eof, ==, 0);
>      qobject_unref(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);
>      qobject_unref(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);
>      qobject_unref(ret);
> -    g_free(cmd);
>  
>      /* check content */
>      path = g_build_filename(fixture->test_dir, "foo", NULL);
> @@ -497,10 +491,10 @@ static void test_qga_file_ops(gconstpointer fix)
>      qobject_unref(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");
> @@ -510,14 +504,13 @@ static void test_qga_file_ops(gconstpointer fix)
>      g_assert_cmpstr(b64, ==, enc);
>  
>      qobject_unref(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");
> @@ -526,14 +519,13 @@ static void test_qga_file_ops(gconstpointer fix)
>      g_assert(eof);
>      g_assert_cmpstr(b64, ==, "");
>      qobject_unref(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");
> @@ -541,13 +533,12 @@ static void test_qga_file_ops(gconstpointer fix)
>      g_assert_cmpint(count, ==, 6);
>      g_assert(!eof);
>      qobject_unref(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");
> @@ -560,15 +551,13 @@ static void test_qga_file_ops(gconstpointer fix)
>      g_free(dec);
>  
>      qobject_unref(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);
>      qobject_unref(ret);
> -    g_free(cmd);
>  }
>  
>  static void test_qga_file_write_read(gconstpointer fix)
> @@ -576,7 +565,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;
> @@ -591,10 +580,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);
>  
> @@ -604,13 +593,12 @@ static void test_qga_file_write_read(gconstpointer fix)
>      g_assert_cmpint(count, ==, sizeof(helloworld));
>      g_assert_cmpint(eof, ==, 0);
>      qobject_unref(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");
> @@ -619,14 +607,13 @@ static void test_qga_file_write_read(gconstpointer fix)
>      g_assert(eof);
>      g_assert_cmpstr(b64, ==, "");
>      qobject_unref(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");
> @@ -634,13 +621,12 @@ static void test_qga_file_write_read(gconstpointer fix)
>      g_assert_cmpint(count, ==, 0);
>      g_assert(!eof);
>      qobject_unref(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");
> @@ -649,16 +635,14 @@ static void test_qga_file_write_read(gconstpointer fix)
>      g_assert(eof);
>      g_assert_cmpstr(b64, ==, enc);
>      qobject_unref(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);
>      qobject_unref(ret);
> -    g_free(cmd);
>  }
>  
>  static void test_qga_get_time(gconstpointer fix)
> @@ -814,7 +798,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': {"
> @@ -829,10 +812,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");
> @@ -842,7 +825,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/tpm-util.c b/tests/tpm-util.c
> index 672cedf905..3bd2887f1e 100644
> --- a/tests/tpm-util.c
> +++ b/tests/tpm-util.c
> @@ -239,13 +239,10 @@ void tpm_util_swtpm_kill(GPid pid)
>  void tpm_util_migrate(QTestState *who, const char *uri)
>  {
>      QDict *rsp;
> -    gchar *cmd;
>  
> -    cmd = g_strdup_printf("{ 'execute': 'migrate',"
> -                          "'arguments': { 'uri': '%s' } }",
> -                          uri);
> -    rsp = qtest_qmp(who, cmd);
> -    g_free(cmd);
> +    rsp = qtest_qmp(who,
> +                    "{ 'execute': 'migrate', 'arguments': { 'uri': %s } }",
> +                    uri);
>      g_assert(qdict_haskey(rsp, "return"));
>      qobject_unref(rsp);
>  }
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index 8ff2106d32..30389dbc7d 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -707,11 +707,7 @@ static void test_migrate(void)
>      g_assert(qdict_haskey(rsp, "return"));
>      qobject_unref(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"));
>      qobject_unref(rsp);
>  
> 

Re: [Qemu-devel] [PATCH 12/20] cpu-plug-test: Don't pass integers as strings to device_add

[Philippe Mathieu-Daudé]

On 07/12/2018 08:12 AM, Markus Armbruster wrote:
> test_plug_with_device_add_x86() plugs Haswell-i386-cpu and
> Haswell-x86_64-cpu with device_add.  It passes socket-id, core-id,
> thread-id as JSON strings.  The properties are actually integers.
> 
> test_plug_with_device_add_coreid() plugs power8_v2.0-spapr-cpu-core
> and qemu-s390x-cpu with device_add.  It passes core-id as JSON string.
> The properties are actually integers.
> 
> Passing JSON string values to integer properties works only due to
> device_add implementation accidents.  Fix the test to pass JSON
> numbers.  While there, use %u rather than %i with unsigned int.
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/cpu-plug-test.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/cpu-plug-test.c b/tests/cpu-plug-test.c
> index 5f39ba0df3..ab3bf6df90 100644
> --- a/tests/cpu-plug-test.c
> +++ b/tests/cpu-plug-test.c
> @@ -88,8 +88,8 @@ static void test_plug_with_device_add_x86(gconstpointer data)
>          for (c = 0; c < td->cores; c++) {
>              for (t = 0; t < td->threads; t++) {
>                  char *id = g_strdup_printf("id-%i-%i-%i", s, c, t);
> -                qtest_qmp_device_add(td->device_model, id, "'socket-id':'%i', "
> -                                     "'core-id':'%i', 'thread-id':'%i'",
> +                qtest_qmp_device_add(td->device_model, id, "'socket-id':%u, "
> +                                     "'core-id':%u, 'thread-id':%u",
>                                       s, c, t);
>                  g_free(id);
>              }
> @@ -114,7 +114,7 @@ static void test_plug_with_device_add_coreid(gconstpointer data)
>  
>      for (c = td->cores; c < td->maxcpus / td->sockets / td->threads; c++) {
>          char *id = g_strdup_printf("id-%i", c);
> -        qtest_qmp_device_add(td->device_model, id, "'core-id':'%i'", c);
> +        qtest_qmp_device_add(td->device_model, id, "'core-id':%u", c);
>          g_free(id);
>      }
>  
> 

Re: [Qemu-devel] [PATCH 12/20] cpu-plug-test: Don't pass integers as strings to device_add

[Thomas Huth]

On 12.07.2018 13:12, Markus Armbruster wrote:
> test_plug_with_device_add_x86() plugs Haswell-i386-cpu and
> Haswell-x86_64-cpu with device_add.  It passes socket-id, core-id,
> thread-id as JSON strings.  The properties are actually integers.
> 
> test_plug_with_device_add_coreid() plugs power8_v2.0-spapr-cpu-core
> and qemu-s390x-cpu with device_add.  It passes core-id as JSON string.
> The properties are actually integers.
> 
> Passing JSON string values to integer properties works only due to
> device_add implementation accidents.  Fix the test to pass JSON
> numbers.  While there, use %u rather than %i with unsigned int.
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/cpu-plug-test.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/cpu-plug-test.c b/tests/cpu-plug-test.c
> index 5f39ba0df3..ab3bf6df90 100644
> --- a/tests/cpu-plug-test.c
> +++ b/tests/cpu-plug-test.c
> @@ -88,8 +88,8 @@ static void test_plug_with_device_add_x86(gconstpointer data)
>          for (c = 0; c < td->cores; c++) {
>              for (t = 0; t < td->threads; t++) {
>                  char *id = g_strdup_printf("id-%i-%i-%i", s, c, t);
> -                qtest_qmp_device_add(td->device_model, id, "'socket-id':'%i', "
> -                                     "'core-id':'%i', 'thread-id':'%i'",
> +                qtest_qmp_device_add(td->device_model, id, "'socket-id':%u, "
> +                                     "'core-id':%u, 'thread-id':%u",
>                                       s, c, t);
>                  g_free(id);
>              }
> @@ -114,7 +114,7 @@ static void test_plug_with_device_add_coreid(gconstpointer data)
>  
>      for (c = td->cores; c < td->maxcpus / td->sockets / td->threads; c++) {
>          char *id = g_strdup_printf("id-%i", c);
> -        qtest_qmp_device_add(td->device_model, id, "'core-id':'%i'", c);
> +        qtest_qmp_device_add(td->device_model, id, "'core-id':%u", c);
>          g_free(id);
>      }

Ooops. Learned something today :-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 03/20] libqtest: Clean up how we read device_del messages

[Thomas Huth]

On 12.07.2018 13:12, Markus Armbruster wrote:
> qtest_qmp_device_del() still uses the qmp("") hack to receive a
> message, even though we have qmp_receive() since commit 66e0c7b187e.
> Put it to use.
> 
> 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/libqtest.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 9cb4096639..071d7eb7b1 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -1090,7 +1090,7 @@ void qtest_qmp_device_del(const char *id)
>      g_assert(response1);
>      g_assert(!qdict_haskey(response1, "error"));
>  
> -    response2 = qmp("");
> +    response2 = qmp_receive();
>      g_assert(response2);
>      g_assert(!qdict_haskey(response2, "error"));

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 04/20] libqtest: Clean up how we read the QMP greeting

[Thomas Huth]

On 12.07.2018 13:12, 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 66e0c7b187e.  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 071d7eb7b1..c2c08a890c 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -249,9 +249,11 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
>  QTestState *qtest_init(const char *extra_args)
>  {
>      QTestState *s = qtest_init_without_qmp_handshake(false, extra_args);
> +    QDict *greeting;
>  
>      /* Read the QMP greeting and then do the handshake */
> -    qtest_qmp_discard_response(s, "");
> +    greeting = qtest_qmp_receive(s);
> +    qobject_unref(greeting);
>      qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
>  
>      return s;

I wonder whether we should actually check the greeting for some expected
information? Anyway, that's something for later, and not related to your
patch.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 05/20] qobject: Replace qobject_from_jsonf() by qobject_from_jsonf_nofail()

[Thomas Huth]

On 12.07.2018 13:12, Markus Armbruster wrote:
> Commit ab45015a968 "qobject: Let qobject_from_jsonf() fail instead of
> abort" removed only one of two ways qobject_from_jsonf() can abort.
> Wrong.

Wrong because it calls qobject_from_jsonv() with error_abort. Maybe
mentioned that in the patch description - since it took me a while to
understand what you mean here.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 15/20] tests: New helper qtest_qmp_receive_success()

[Stefan Berger]

On 07/12/2018 07:12 AM, Markus Armbruster wrote:
> Commit b21373d0713 copied wait_command() from tests/migration-test.c
> to tests/tpm-util.c.  Replace both copies by new libqtest helper
> qtest_qmp_receive_success().  Also use it to simplify
> qtest_qmp_device_del().
>
> Bonus: gets rid of a non-literal format string.  A step towards
> compile-time format string checking without triggering
> -Wformat-nonliteral.
>
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>

Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/libqtest.c       | 69 ++++++++++++++++++++++++++++++------------
>   tests/libqtest.h       | 17 +++++++++++
>   tests/migration-test.c | 29 ++++++------------
>   tests/tpm-util.c       | 32 +++-----------------
>   4 files changed, 80 insertions(+), 67 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index dfca3af89d..ed832cd8e6 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -1029,6 +1029,35 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine))
>       qobject_unref(response);
>   }
>
> +QDict *qtest_qmp_receive_success(QTestState *s,
> +                                 void (*event_cb)(void *opaque,
> +                                                  const char *event,
> +                                                  QDict *data),
> +                                 void *opaque)
> +{
> +    QDict *response, *ret, *data;
> +    const char *event;
> +
> +    for (;;) {
> +        response = qtest_qmp_receive(s);
> +        g_assert(!qdict_haskey(response, "error"));
> +        ret = qdict_get_qdict(response, "return");
> +        if (ret) {
> +            break;
> +        }
> +        event = qdict_get_str(response, "event");
> +        data = qdict_get_qdict(response, "data");
> +        if (event_cb) {
> +            event_cb(opaque, event, data);
> +        }
> +        qobject_unref(response);
> +    }
> +
> +    qobject_ref(ret);
> +    qobject_unref(response);
> +    return ret;
> +}
> +
>   /*
>    * Generic hot-plugging test via the device_add QMP command.
>    */
> @@ -1053,6 +1082,14 @@ void qtest_qmp_device_add(const char *driver, const char *id,
>       qobject_unref(response);
>   }
>
> +static void device_deleted_cb(void *opaque, const char *name, QDict *data)
> +{
> +    bool *got_event = opaque;
> +
> +    g_assert_cmpstr(name, ==, "DEVICE_DELETED");
> +    *got_event = true;
> +}
> +
>   /*
>    * Generic hot-unplugging test via the device_del QMP command.
>    * Device deletion will get one response and one event. For example:
> @@ -1073,27 +1110,21 @@ void qtest_qmp_device_add(const char *driver, const char *id,
>    */
>   void qtest_qmp_device_del(const char *id)
>   {
> -    QDict *response1, *response2, *event = NULL;
> +    bool got_event = false;
> +    QDict *rsp;
>
> -    response1 = qmp("{'execute': 'device_del', 'arguments': {'id': %s}}",
> -                    id);
> -    g_assert(response1);
> -    g_assert(!qdict_haskey(response1, "error"));
> -
> -    response2 = qmp_receive();
> -    g_assert(response2);
> -    g_assert(!qdict_haskey(response2, "error"));
> -
> -    if (qdict_haskey(response1, "event")) {
> -        event = response1;
> -    } else if (qdict_haskey(response2, "event")) {
> -        event = response2;
> +    qtest_qmp_send(global_qtest,
> +                   "{'execute': 'device_del', 'arguments': {'id': %s}}",
> +                   id);
> +    rsp = qtest_qmp_receive_success(global_qtest, device_deleted_cb,
> +                                    &got_event);
> +    qobject_unref(rsp);
> +    if (!got_event) {
> +        rsp = qmp_receive();
> +        g_assert_cmpstr(qdict_get_try_str(rsp, "event"),
> +                        ==, "DEVICE_DELETED");
> +        qobject_unref(rsp);
>       }
> -    g_assert(event);
> -    g_assert_cmpstr(qdict_get_str(event, "event"), ==, "DEVICE_DELETED");
> -
> -    qobject_unref(response1);
> -    qobject_unref(response2);
>   }
>
>   bool qmp_rsp_is_err(QDict *rsp)
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 53364849bf..68b767fc88 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -163,6 +163,23 @@ void qtest_qmp_eventwait(QTestState *s, const char *event);
>    */
>   QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
>
> +/**
> + * qtest_qmp_receive_success:
> + * @s: #QTestState instance to operate on
> + * @event_cb: Event callback
> + * @opaque: Argument for @event_cb
> + *
> + * Poll QMP messages until a command success response is received.
> + * If @event_cb, call it for each event received, passing @opaque,
> + * the event's name and data.
> + * Return the success response's "return" member.
> + */
> +QDict *qtest_qmp_receive_success(QTestState *s,
> +                                 void (*event_cb)(void *opaque,
> +                                                  const char *name,
> +                                                  QDict *data),
> +                                 void *opaque);
> +
>   /**
>    * qtest_hmp:
>    * @s: #QTestState instance to operate on.
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index f9f2c0148a..267342fd3e 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -146,31 +146,20 @@ static void wait_for_serial(const char *side)
>       } while (true);
>   }
>
> +static void stop_cb(void *opaque, const char *name, QDict *data)
> +{
> +    if (!strcmp(name, "STOP")) {
> +        got_stop = true;
> +    }
> +}
> +
>   /*
>    * Events can get in the way of responses we are actually waiting for.
>    */
>   static QDict *wait_command(QTestState *who, const char *command)
>   {
> -    const char *event_string;
> -    QDict *response, *ret;
> -
> -    response = qtest_qmp(who, command);
> -
> -    while (qdict_haskey(response, "event")) {
> -        /* OK, it was an event */
> -        event_string = qdict_get_str(response, "event");
> -        if (!strcmp(event_string, "STOP")) {
> -            got_stop = true;
> -        }
> -        qobject_unref(response);
> -        response = qtest_qmp_receive(who);
> -    }
> -
> -    ret = qdict_get_qdict(response, "return");
> -    g_assert(ret);
> -    qobject_ref(ret);
> -    qobject_unref(response);
> -    return ret;
> +    qtest_qmp_send(who, command);
> +    return qtest_qmp_receive_success(who, stop_cb, NULL);
>   }
>
>   /*
> diff --git a/tests/tpm-util.c b/tests/tpm-util.c
> index 3bd2887f1e..9f3f156e42 100644
> --- a/tests/tpm-util.c
> +++ b/tests/tpm-util.c
> @@ -22,8 +22,6 @@
>   #define TIS_REG(LOCTY, REG) \
>       (TPM_TIS_ADDR_BASE + ((LOCTY) << 12) + REG)
>
> -static bool got_stop;
> -
>   void tpm_util_crb_transfer(QTestState *s,
>                              const unsigned char *req, size_t req_size,
>                              unsigned char *rsp, size_t rsp_size)
> @@ -247,41 +245,19 @@ void tpm_util_migrate(QTestState *who, const char *uri)
>       qobject_unref(rsp);
>   }
>
> -/*
> - * Events can get in the way of responses we are actually waiting for.
> - */
> -static QDict *tpm_util_wait_command(QTestState *who, const char *command)
> -{
> -    const char *event_string;
> -    QDict *response;
> -
> -    response = qtest_qmp(who, command);
> -
> -    while (qdict_haskey(response, "event")) {
> -        /* OK, it was an event */
> -        event_string = qdict_get_str(response, "event");
> -        if (!strcmp(event_string, "STOP")) {
> -            got_stop = true;
> -        }
> -        qobject_unref(response);
> -        response = qtest_qmp_receive(who);
> -    }
> -    return response;
> -}
> -
>   void tpm_util_wait_for_migration_complete(QTestState *who)
>   {
>       while (true) {
> -        QDict *rsp, *rsp_return;
> +        QDict *rsp_return;
>           bool completed;
>           const char *status;
>
> -        rsp = tpm_util_wait_command(who, "{ 'execute': 'query-migrate' }");
> -        rsp_return = qdict_get_qdict(rsp, "return");
> +        qtest_qmp_send(who, "{ 'execute': 'query-migrate' }");
> +        rsp_return = qtest_qmp_receive_success(who, NULL, NULL);
>           status = qdict_get_str(rsp_return, "status");
>           completed = strcmp(status, "completed") == 0;
>           g_assert_cmpstr(status, !=,  "failed");
> -        qobject_unref(rsp);
> +        qobject_unref(rsp_return);
>           if (completed) {
>               return;
>           }

Re: [Qemu-devel] [PATCH 04/20] libqtest: Clean up how we read the QMP greeting

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 12.07.2018 13:12, 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 66e0c7b187e.  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 071d7eb7b1..c2c08a890c 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -249,9 +249,11 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
>>  QTestState *qtest_init(const char *extra_args)
>>  {
>>      QTestState *s = qtest_init_without_qmp_handshake(false, extra_args);
>> +    QDict *greeting;
>>  
>>      /* Read the QMP greeting and then do the handshake */
>> -    qtest_qmp_discard_response(s, "");
>> +    greeting = qtest_qmp_receive(s);
>> +    qobject_unref(greeting);
>>      qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
>>  
>>      return s;
>
> I wonder whether we should actually check the greeting for some expected
> information? Anyway, that's something for later, and not related to your
> patch.

This kind of sloppy testing is quite common.

Checking test results manually in code maximizes flexibility.  It also
maximizes temptation to cut corners, because doing a complete job is so
tedious.  One reason I prefer "normalize and diff against expected
results".  The closest we got to support for that way of testing is
qlit_equal_qobject().

Digression: QOM also maximizes flexibility by doing stuff in code rather
than data.  The two go back to the same stratum of QEMU development.
Hardly coincidence, in my opinion.  We've since extended QOM to support
doing more in data, but there's still plenty of old code doing it in
code, and plenty of new code following that old code's lead.  Recovering
from such damage is hard work.

> Reviewed-by: Thomas Huth <thuth@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 05/20] qobject: Replace qobject_from_jsonf() by qobject_from_jsonf_nofail()

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 12.07.2018 13:12, Markus Armbruster wrote:
>> Commit ab45015a968 "qobject: Let qobject_from_jsonf() fail instead of
>> abort" removed only one of two ways qobject_from_jsonf() can abort.
>> Wrong.
>
> Wrong because it calls qobject_from_jsonv() with error_abort. Maybe
> mentioned that in the patch description - since it took me a while to
> understand what you mean here.

Can do.

> Reviewed-by: Thomas Huth <thuth@redhat.com>

Thanks!

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

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Hi Markus,
>
> On 07/12/2018 08:12 AM, Markus Armbruster wrote:
>> When you build QMP input manually like this
>> 
>>     cmd = g_strdup_printf("{ 'execute': 'migrate',"
>>                           "'arguments': { 'uri': '%s' } }",
>>                           uri);
>>     rsp = qmp(cmd);
>>     g_free(cmd);
>> 
>> you're responsible for escaping the interpolated values for JSON.  Not
>> done here, and therefore works only for sufficiently nice @uri.  For
>> instance, if @uri contained a single "'", qobject_from_vjsonf_nofail()
>> would abort.  A sufficiently nasty @uri could even inject unwanted
>> members into the arguments object.
>> 
>> Leaving interpolation into JSON to qmp() is more robust:
>> 
>>     rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
>> 
>> It's also more concise.
>> 
>> Clean up the simple cases where we interpolate exactly a JSON value.
>> 
>> Bonus: gets rid of non-literal format strings.  A step towards
>> compile-time format string checking without triggering
>> -Wformat-nonliteral.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/libqos/pci-pc.c   |   9 +--
>>  tests/libqtest.c        |   7 +-
>>  tests/migration-test.c  |   8 +--
>>  tests/test-qga.c        | 150 ++++++++++++++++++----------------------
>>  tests/tpm-util.c        |   9 +--
>>  tests/vhost-user-test.c |   6 +-
>>  6 files changed, 77 insertions(+), 112 deletions(-)
>> 
>> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
>> index a7803308b7..585f5289ec 100644
>> --- a/tests/libqos/pci-pc.c
>> +++ b/tests/libqos/pci-pc.c
>> @@ -160,14 +160,9 @@ void qpci_free_pc(QPCIBus *bus)
>>  void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
>>  {
>>      QDict *response;
>> -    char *cmd;
>>  
>> -    cmd = g_strdup_printf("{'execute': 'device_del',"
>> -                          " 'arguments': {"
>> -                          "   'id': '%s'"
>> -                          "}}", id);
>> -    response = qmp(cmd);
>> -    g_free(cmd);
>> +    response = qmp("{'execute': 'device_del', 'arguments': {'id': %s}}",
>> +                   id);
>>      g_assert(response);
>>      g_assert(!qdict_haskey(response, "error"));
>>      qobject_unref(response);
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index 3bfb062fcb..e36cc5ede3 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -1079,12 +1079,9 @@ void qtest_qmp_device_add(const char *driver, const char *id, const char *fmt,
>>  void qtest_qmp_device_del(const char *id)
>>  {
>>      QDict *response1, *response2, *event = NULL;
>> -    char *cmd;
>>  
>> -    cmd = g_strdup_printf("{'execute': 'device_del',"
>> -                          " 'arguments': { 'id': '%s' }}", id);
>> -    response1 = qmp(cmd);
>> -    g_free(cmd);
>> +    response1 = qmp("{'execute': 'device_del', 'arguments': {'id': %s}}",
>> +                    id);
>>      g_assert(response1);
>>      g_assert(!qdict_haskey(response1, "error"));
>>  
>> diff --git a/tests/migration-test.c b/tests/migration-test.c
>> index 086f727b34..bbb9d3da31 100644
>> --- a/tests/migration-test.c
>> +++ b/tests/migration-test.c
>> @@ -529,14 +529,12 @@ static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
>>  static void deprecated_set_downtime(QTestState *who, const double value)
>>  {
>>      QDict *rsp;
>> -    gchar *cmd;
>>      char *expected;
>>      int64_t result_int;
>>  
>> -    cmd = g_strdup_printf("{ 'execute': 'migrate_set_downtime',"
>> -                          "'arguments': { 'value': %g } }", value);
>> -    rsp = qtest_qmp(who, cmd);
>> -    g_free(cmd);
>> +    rsp = qtest_qmp(who,
>> +                    "{ 'execute': 'migrate_set_downtime',"
>> +                    " 'arguments': { 'value': %f } }", value);
>
> I suppose you changed %g -> %f because the downtime is expected
> to be greater than 1e-4 :)

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

qtest_qmp()'s function comment provides a clue:

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

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

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

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

> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks!

[...]

Re: [Qemu-devel] [PATCH 17/20] migration-test: Clean up string interpolation into QMP, part 1

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> wrote:
>> Leaving interpolation into JSON to qmp() is more robust than building
>> QMP input manually, as explained in the recent commit "tests: Clean up
>> string interpolation into QMP input (simple cases)".
>>
>> migrate_recover() builds QMP input manually because wait_command()
>> can't interpolate.  Well, it can since the previous commit.  Simplify
>> accordingly.
>>
>> Bonus: gets rid of a non-literal format string.  A step towards
>> compile-time format string checking without triggering
>> -Wformat-nonliteral.
>>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
>
> Why, o why it needs to be so difficult!!!

At least the series improves the interface from Rusty's API level -4
"Follow common convention and you'll get it wrong" to API level 4
"Follow common convention and you'll get it right", and for some kind of
mistakes even to API level 8 "The compiler will warn if you get it
wrong."

Thanks!