[Qemu-devel] [PATCH v3 00/12] Clean up around qmp() and hmp()

[Qemu-devel] [PATCH v3 00/12] Clean up around qmp() and hmp()

[Eric Blake]

I've merged the best aspects of Markus' and my previous posts on
the topic.  While it mostly touches test code (and thus could
be considered 2.10 material at a stretch), it's a big enough
series that it's probably safer to leave for 2.11.

If you like the idea of qmp_cmd() taking a command name and a
QObject (easier than QDict) for arguments, there may be more
places to clean up to use that paradigm (for this round of
posting, I focused more on cleaning up the areas touched in
earlier patches, rather than a global search).

Eric Blake (5):
  qobject: Accept "%"PRId64 in qobject_from_jsonf()
  qtest: Avoid passing raw strings through hmp()
  qtest: Document calling conventions
  qtest: Add a new helper qmp_cmd() and friends
  qtests: convert tests to use qmp_cmd

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

 configure                      |  24 +++++++
 tests/libqos/pci.h             |   2 +-
 tests/libqtest.h               | 103 ++++++++++++++++++--------
 qobject/json-lexer.c           |  21 +++---
 qobject/json-parser.c          |  10 +--
 tests/ahci-test.c              |   4 +-
 tests/boot-order-test.c        |   2 +-
 tests/check-qjson.c            |   7 ++
 tests/device-introspect-test.c |   3 +-
 tests/ide-test.c               |   4 +-
 tests/ivshmem-test.c           |  10 +--
 tests/libqos/libqos.c          |  17 +----
 tests/libqos/pci-pc.c          |  15 +---
 tests/libqos/pci.c             |  32 +++++----
 tests/libqos/usb.c             |  30 ++++----
 tests/libqtest.c               |  29 +++++++-
 tests/pc-cpu-test.c            |  10 +--
 tests/postcopy-test.c          |  15 ++--
 tests/tco-test.c               |   3 +-
 tests/test-hmp.c               |   4 +-
 tests/test-qga.c               | 160 ++++++++++++++++++-----------------------
 tests/usb-hcd-uhci-test.c      |   6 +-
 tests/usb-hcd-xhci-test.c      |  12 +---
 tests/vhost-user-test.c        |  16 ++---
 tests/virtio-blk-test.c        |   5 +-
 tests/wdt_ib700-test.c         |  35 +++------
 26 files changed, 303 insertions(+), 276 deletions(-)

-- 
2.13.3

[Qemu-devel] [PATCH v3 01/12] qobject: Accept "%"PRId64 in qobject_from_jsonf()

[Eric Blake]

Commit 1792d7d0 was written because PRId64 expands to non-portable
crap for some libc, and we had testsuite failures on Mac OS as a
result.  This in turn makes it difficult to rely on the obvious
conversions of 64-bit values into JSON, requiring things such as
casting int64_t to long long so we can use a reliable %lld and
still keep -Wformat happy.  So now it's time to fix that.

We are already lexing %I64d (hello mingw); now expand the lexer
to parse %qd (hello Mac OS). In the process, we can drop one
state: IN_ESCAPE_I64 is redundant with IN_ESCAPE_LL, and reused
again by %qd, so rename it to IN_ESCAPE_64.  And fix a comment
that mistakenly omitted %u as a supported escape.

Next, tweak the parser to accept the exact spelling of PRId64
regardless of what it expands to (note that there are are now dead
branches in the 'if' chain for some platforms, like glibc; but
there are other platforms where all branches are necessary).  We
are at least safe in that we are parsing the correct size 32-bit or
a 64-bit quantity on whatever branch we end up in.  This also means
that we no longer parse everything that the lexer will consume (no
more %I64d on glibc), but that is intentional.  And of course,
update the testsuite for coverage of the feature.

Finally, update configure to barf on any libc that uses yet
another form of PRId64 that we have not yet coded for, so that we
can once again update json-lexer.c to cater to those quirks (my
guess? we might see %jd next) (on the bright side, json-parser.c
should no longer need updates).  Yes, the only way I could find
to quickly learn what spelling is preferred when cross-compiling
is to grep a compiled binary; C does not make it easy to turn a
string constant into an integer constant, let alone make
preprocessor decisions; and even parsing '$CC -E' output is
fragile, since 64-bit glibc pre-processes as "l" "d" rather than
"ld", and newer gcc splits macro expansions across multiple lines.
I'm assuming that 'strings' is portable enough during configure;
if that assumption fails in some constrained docker environment,
an easy hack is to add this to configure:
  strings () { tr -d '\0' < "$1"; }

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

---
v3: incorporate review comments from Markus
---
 configure             | 24 ++++++++++++++++++++++++
 qobject/json-lexer.c  | 21 +++++++++------------
 qobject/json-parser.c | 10 ++++++----
 tests/check-qjson.c   |  7 +++++++
 4 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/configure b/configure
index 987f59ba88..5b0eddec3c 100755
--- a/configure
+++ b/configure
@@ -3222,6 +3222,30 @@ for i in $glib_modules; do
     fi
 done

+# Sanity check that we can parse PRId64 and friends in json-lexer.c
+# (Sadly, the "easiest" way to do this is to grep the compiled binary,
+# since we can't control whether the preprocessor would output "ld" vs.
+# "l" "d", nor even portably ensure it fits output on the same line as
+# a witness marker.)
+cat > $TMPC <<EOF
+#include <inttypes.h>
+int main(void) {
+    static const char findme[] = "\nUnLiKeLyToClAsH_" PRId64 "\n";
+    return !findme[0];
+}
+EOF
+if ! compile_prog "$CFLAGS" "$LIBS" ; then
+    error_exit "can't determine value of PRId64"
+fi
+nl='
+'
+case $(strings $TMPE | grep ^UnLiKeLyToClAsH) in
+    '' | *"$nl"* ) error_exit "can't determine value of PRId64" ;;
+    *_ld | *_lld | *_I64d | *_qd) ;;
+    *) error_exit "unexepected value of PRId64, please add %$(strings $TMPE |
+       sed -n s/^UnLiKeLyToClAsH_//p) support to json-lexer.c" ;;
+esac
+
 # Sanity check that the current size_t matches the
 # size that glib thinks it should be. This catches
 # problems on multi-arch where people try to build
diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
index 980ba159d6..9a0fc58444 100644
--- a/qobject/json-lexer.c
+++ b/qobject/json-lexer.c
@@ -29,9 +29,11 @@
  *
  * '([^\\']|\\[\"'\\/bfnrt]|\\u[0-9a-fA-F]{4})*'
  *
- * Extension for vararg handling in JSON construction:
+ * Extension for vararg handling in JSON construction [this lexer
+ * actually accepts multiple forms of PRId64, but parse_escape() later
+ * filters to only parse the current platform's spelling]:
  *
- * %((l|ll|I64)?d|[ipsf])
+ * %(PRI[du]64|(l|ll)?[ud]|[ipsf])
  *
  */

@@ -60,10 +62,9 @@ enum json_lexer_state {
     IN_KEYWORD,
     IN_ESCAPE,
     IN_ESCAPE_L,
-    IN_ESCAPE_LL,
+    IN_ESCAPE_64,
     IN_ESCAPE_I,
     IN_ESCAPE_I6,
-    IN_ESCAPE_I64,
     IN_WHITESPACE,
     IN_START,
 };
@@ -225,24 +226,19 @@ static const uint8_t json_lexer[][256] =  {
     },

     /* escape */
-    [IN_ESCAPE_LL] = {
+    [IN_ESCAPE_64] = {
         ['d'] = JSON_ESCAPE,
         ['u'] = JSON_ESCAPE,
     },

     [IN_ESCAPE_L] = {
         ['d'] = JSON_ESCAPE,
-        ['l'] = IN_ESCAPE_LL,
-        ['u'] = JSON_ESCAPE,
-    },
-
-    [IN_ESCAPE_I64] = {
-        ['d'] = JSON_ESCAPE,
+        ['l'] = IN_ESCAPE_64,
         ['u'] = JSON_ESCAPE,
     },

     [IN_ESCAPE_I6] = {
-        ['4'] = IN_ESCAPE_I64,
+        ['4'] = IN_ESCAPE_64,
     },

     [IN_ESCAPE_I] = {
@@ -257,6 +253,7 @@ static const uint8_t json_lexer[][256] =  {
         ['u'] = JSON_ESCAPE,
         ['f'] = JSON_ESCAPE,
         ['l'] = IN_ESCAPE_L,
+        ['q'] = IN_ESCAPE_64,
         ['I'] = IN_ESCAPE_I,
     },

diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 724ca240e4..388aa7a386 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -470,16 +470,18 @@ static QObject *parse_escape(JSONParserContext *ctxt, va_list *ap)
         return QOBJECT(qnum_from_int(va_arg(*ap, int)));
     } else if (!strcmp(token->str, "%ld")) {
         return QOBJECT(qnum_from_int(va_arg(*ap, long)));
-    } else if (!strcmp(token->str, "%lld") ||
-               !strcmp(token->str, "%I64d")) {
+    } else if (!strcmp(token->str, "%lld")) {
         return QOBJECT(qnum_from_int(va_arg(*ap, long long)));
+    } else if (!strcmp(token->str, "%" PRId64)) {
+        return QOBJECT(qnum_from_int(va_arg(*ap, int64_t)));
     } else if (!strcmp(token->str, "%u")) {
         return QOBJECT(qnum_from_uint(va_arg(*ap, unsigned int)));
     } else if (!strcmp(token->str, "%lu")) {
         return QOBJECT(qnum_from_uint(va_arg(*ap, unsigned long)));
-    } else if (!strcmp(token->str, "%llu") ||
-               !strcmp(token->str, "%I64u")) {
+    } else if (!strcmp(token->str, "%llu")) {
         return QOBJECT(qnum_from_uint(va_arg(*ap, unsigned long long)));
+    } else if (!strcmp(token->str, "%" PRIu64)) {
+        return QOBJECT(qnum_from_uint(va_arg(*ap, uint64_t)));
     } else if (!strcmp(token->str, "%s")) {
         return QOBJECT(qstring_from_str(va_arg(*ap, const char *)));
     } else if (!strcmp(token->str, "%f")) {
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index a3a97b0d99..1ad1f41a52 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -990,8 +990,10 @@ static void vararg_number(void)
     QNum *qnum;
     int value = 0x2342;
     long long value_ll = 0x2342342343LL;
+    uint64_t value_u64 = 0x2342342343ULL;
     double valuef = 2.323423423;
     int64_t val;
+    uint64_t uval;

     qnum = qobject_to_qnum(qobject_from_jsonf("%d", value));
     g_assert(qnum_get_try_int(qnum, &val));
@@ -1003,6 +1005,11 @@ static void vararg_number(void)
     g_assert_cmpint(val, ==, value_ll);
     QDECREF(qnum);

+    qnum = qobject_to_qnum(qobject_from_jsonf("%" PRIu64, value_u64));
+    g_assert(qnum_get_try_uint(qnum, &uval));
+    g_assert_cmpint(uval, ==, value_u64);
+    QDECREF(qnum);
+
     qnum = qobject_to_qnum(qobject_from_jsonf("%f", valuef));
     g_assert(qnum_get_double(qnum) == valuef);
     QDECREF(qnum);
-- 
2.13.3

[Qemu-devel] [PATCH v3 02/12] qtest: Avoid passing raw strings through hmp()

[Eric Blake]

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

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

diff --git a/tests/test-hmp.c b/tests/test-hmp.c
index d77b3c8710..0af066487c 100644
--- a/tests/test-hmp.c
+++ b/tests/test-hmp.c
@@ -80,7 +80,7 @@ static void test_commands(void)
         if (verbose) {
             fprintf(stderr, "\t%s\n", hmp_cmds[i]);
         }
-        response = hmp(hmp_cmds[i]);
+        response = hmp("%s", hmp_cmds[i]);
         g_free(response);
     }

@@ -103,7 +103,7 @@ static void test_info_commands(void)
         if (verbose) {
             fprintf(stderr, "\t%s\n", info);
         }
-        resp = hmp(info);
+        resp = hmp("%s", info);
         g_free(resp);
         /* And move forward to the next line */
         info = strchr(endp + 1, '\n');
-- 
2.13.3

[Qemu-devel] [PATCH v3 03/12] qtest: Document calling conventions

[Eric Blake]

We have two flavors of vararg usage in qtest; make it clear that
qmp() has different semantics than hmp(), and let the compiler
enforce that hmp() is used correctly. However, qmp() (and friends)
only accept a subset of printf flags look-alikes (namely, those
that our JSON parser understands), and what is worse, qmp("true")
(the JSON keyword 'true') is different from qmp("%s", "true")
(the JSON string '"true"'), and we have some intermediate cleanup
patches to do before we can mark those as printf-like.

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

---
v3: restore lost attributes, add comments on va_list forms, tweak
commit message to mention upcoming qmp cleanups
v2: several comment tweaks, explain why qmp() can't be marked
---
 tests/libqtest.h | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index 38bc1e9953..82e89b4d4f 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -50,7 +50,8 @@ void qtest_quit(QTestState *s);
 /**
  * qtest_qmp_discard_response:
  * @s: #QTestState instance to operate on.
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; formats arguments through
+ * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
  *
  * Sends a QMP message to QEMU and consumes the response.
  */
@@ -59,7 +60,8 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
 /**
  * qtest_qmp:
  * @s: #QTestState instance to operate on.
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; formats arguments through
+ * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
  *
  * Sends a QMP message to QEMU and returns the response.
  */
@@ -68,7 +70,8 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
 /**
  * qtest_async_qmp:
  * @s: #QTestState instance to operate on.
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; formats arguments through
+ * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
  */
@@ -77,7 +80,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 '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
  * @ap: QMP message arguments
  *
  * Sends a QMP message to QEMU and consumes the response.
@@ -87,7 +91,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 '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
  * @ap: QMP message arguments
  *
  * Sends a QMP message to QEMU and returns the response.
@@ -97,7 +102,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 '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
  * @ap: QMP message arguments
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
@@ -134,19 +140,19 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
 /**
  * qtest_hmp:
  * @s: #QTestState instance to operate on.
- * @fmt...: HMP command to send to QEMU
+ * @fmt...: HMP command to send to QEMU, formats arguments like vsprintf().
  *
  * Send HMP command to QEMU via QMP's human-monitor-command.
  * QMP events are discarded.
  *
  * Returns: the command's output.  The caller should g_free() it.
  */
-char *qtest_hmp(QTestState *s, const char *fmt, ...);
+char *qtest_hmp(QTestState *s, const char *fmt, ...) GCC_FMT_ATTR(2, 3);

 /**
  * qtest_hmpv:
  * @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.
@@ -154,7 +160,8 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...);
  *
  * Returns: the command's output.  The caller should g_free() it.
  */
-char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap);
+char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap)
+    GCC_FMT_ATTR(2, 0);

 /**
  * qtest_get_irq:
@@ -535,7 +542,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 '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
  *
  * Sends a QMP message to QEMU and returns the response.
  */
@@ -543,7 +551,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 '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
  */
@@ -551,7 +560,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 '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
  *
  * Sends a QMP message to QEMU and consumes the response.
  */
@@ -592,13 +602,13 @@ static inline QDict *qmp_eventwait_ref(const char *event)

 /**
  * hmp:
- * @fmt...: HMP command to send to QEMU
+ * @fmt...: HMP command to send to QEMU, formats arguments like vsprintf().
  *
  * Send HMP command to QEMU via QMP's human-monitor-command.
  *
  * Returns: the command's output.  The caller should g_free() it.
  */
-char *hmp(const char *fmt, ...);
+char *hmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);

 /**
  * get_irq:
-- 
2.13.3

[Qemu-devel] [PATCH v3 04/12] tests: Pass literal format strings directly to qmp_FOO()

[Eric Blake]

From: Markus Armbruster <armbru@redhat.com>

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>
Message-Id: <1500645206-13559-4-git-send-email-armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@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 999121bb7c..97622e0044 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1350,7 +1350,6 @@ static void test_flush_migrate(void)
     AHCIQState *src, *dst;
     AHCICommand *cmd;
     uint8_t px;
-    const char *s;
     char *uri = g_strdup_printf("unix:%s", mig_socket);

     prepare_blkdebug_script(debug_path, "flush_to_disk");
@@ -1386,8 +1385,7 @@ static void test_flush_migrate(void)
     ahci_migrate(src, dst, uri);

     /* Complete the command */
-    s = "{'execute':'cont' }";
-    qmp_async(s);
+    qmp_async("{'execute':'cont' }");
     qmp_eventwait("RESUME");
     ahci_command_wait(dst, cmd);
     ahci_command_verify(dst, cmd);
diff --git a/tests/ide-test.c b/tests/ide-test.c
index bfd79ddbdc..ea2657d3d1 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -624,7 +624,6 @@ static void test_retry_flush(const char *machine)
     QPCIDevice *dev;
     QPCIBar bmdma_bar, ide_bar;
     uint8_t data;
-    const char *s;

     prepare_blkdebug_script(debug_path, "flush_to_disk");

@@ -652,8 +651,7 @@ static void test_retry_flush(const char *machine)
     qmp_eventwait("STOP");

     /* Complete the command */
-    s = "{'execute':'cont' }";
-    qmp_discard_response(s);
+    qmp_discard_response("{'execute':'cont' }");

     /* Check registers */
     data = qpci_io_readb(dev, ide_bar, reg_device);
-- 
2.13.3

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

[Eric Blake]

From: Markus Armbruster <armbru@redhat.com>

When you build QMP input manually like this

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

you're responsible for escaping the interpolated values for JSON.  Not
done here, and therefore works only for sufficiently nice @uri.  For
instance, if @uri contained a single "'", qobject_from_jsonv() would
fail, qmp_fd_sendv() would misinterpret the failure as empty input and
do nothing, and the test would hang waiting for a response that never
comes.

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

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

It's also more concise.

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

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

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1500645206-13559-5-git-send-email-armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqos/libqos.c   |  16 +----
 tests/libqos/pci-pc.c   |   9 +--
 tests/postcopy-test.c   |   8 +--
 tests/test-qga.c        | 160 +++++++++++++++++++++---------------------------
 tests/vhost-user-test.c |   6 +-
 5 files changed, 77 insertions(+), 122 deletions(-)

diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
index 6226546c28..42c5315423 100644
--- a/tests/libqos/libqos.c
+++ b/tests/libqos/libqos.c
@@ -86,20 +86,12 @@ void set_context(QOSState *s)

 static QDict *qmp_execute(const char *command)
 {
-    char *fmt;
-    QDict *rsp;
-
-    fmt = g_strdup_printf("{ 'execute': '%s' }", command);
-    rsp = qmp(fmt);
-    g_free(fmt);
-
-    return rsp;
+    return qmp("{ 'execute': %s }", command);
 }

 void migrate(QOSState *from, QOSState *to, const char *uri)
 {
     const char *st;
-    char *s;
     QDict *rsp, *sub;
     bool running;

@@ -114,11 +106,7 @@ void migrate(QOSState *from, QOSState *to, const char *uri)
     QDECREF(rsp);

     /* Issue the migrate command. */
-    s = g_strdup_printf("{ 'execute': 'migrate',"
-                        "'arguments': { 'uri': '%s' } }",
-                        uri);
-    rsp = qmp(s);
-    g_free(s);
+    rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
     g_assert(qdict_haskey(rsp, "return"));
     QDECREF(rsp);

diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
index ded1c54c06..d40aa9dffd 100644
--- a/tests/libqos/pci-pc.c
+++ b/tests/libqos/pci-pc.c
@@ -159,14 +159,9 @@ void qpci_free_pc(QPCIBus *bus)
 void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
 {
     QDict *response;
-    char *cmd;

-    cmd = g_strdup_printf("{'execute': 'device_del',"
-                          " 'arguments': {"
-                          "   'id': '%s'"
-                          "}}", id);
-    response = qmp(cmd);
-    g_free(cmd);
+    response = qmp("{'execute': 'device_del', 'arguments': {'id': %s}}",
+                   id);
     g_assert(response);
     g_assert(!qdict_haskey(response, "error"));
     QDECREF(response);
diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index 8142f2ab90..e2ead875a8 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -358,7 +358,7 @@ static void test_migrate(void)
     char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
     QTestState *global = global_qtest, *from, *to;
     unsigned char dest_byte_a, dest_byte_b, dest_byte_c, dest_byte_d;
-    gchar *cmd, *cmd_src, *cmd_dst;
+    gchar *cmd_src, *cmd_dst;
     QDict *rsp;

     char *bootpath = g_strdup_printf("%s/bootsect", tmpfs);
@@ -445,11 +445,7 @@ static void test_migrate(void)
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");

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

diff --git a/tests/test-qga.c b/tests/test-qga.c
index 06783e7585..91a7b6e16b 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -144,12 +144,11 @@ static void test_qga_sync_delimited(gconstpointer fix)
     guint32 v, r = g_random_int();
     unsigned char c;
     QDict *ret;
-    gchar *cmd;

-    cmd = g_strdup_printf("\xff{'execute': 'guest-sync-delimited',"
-                          " 'arguments': {'id': %u } }", r);
-    qmp_fd_send(fixture->fd, cmd);
-    g_free(cmd);
+    qmp_fd_send(fixture->fd,
+                "\xff{'execute': 'guest-sync-delimited',"
+                " 'arguments': {'id': %u } }",
+                r);

     /*
      * Read and ignore garbage until resynchronized.
@@ -186,7 +185,6 @@ static void test_qga_sync(gconstpointer fix)
     const TestFixture *fixture = fix;
     guint32 v, r = g_random_int();
     QDict *ret;
-    gchar *cmd;

     /*
      * TODO guest-sync is inherently limited: we cannot distinguish
@@ -199,10 +197,9 @@ static void test_qga_sync(gconstpointer fix)
      * invalid JSON. Testing of '\xff' handling is done in
      * guest-sync-delimited instead.
      */
-    cmd = g_strdup_printf("{'execute': 'guest-sync',"
-                          " 'arguments': {'id': %u } }", r);
-    ret = qmp_fd(fixture->fd, cmd);
-    g_free(cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-sync', 'arguments': {'id': %u } }",
+                 r);

     g_assert_nonnull(ret);
     qmp_assert_no_error(ret);
@@ -393,7 +390,7 @@ static void test_qga_file_ops(gconstpointer fix)
     const TestFixture *fixture = fix;
     const unsigned char helloworld[] = "Hello World!\n";
     const char *b64;
-    gchar *cmd, *path, *enc;
+    gchar *path, *enc;
     unsigned char *dec;
     QDict *ret, *val;
     int64_t id, eof;
@@ -411,10 +408,10 @@ static void test_qga_file_ops(gconstpointer fix)

     enc = g_base64_encode(helloworld, sizeof(helloworld));
     /* write */
-    cmd = g_strdup_printf("{'execute': 'guest-file-write',"
-                          " 'arguments': { 'handle': %" PRId64 ","
-                          " 'buf-b64': '%s' } }", id, enc);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-write',"
+                 " 'arguments': { 'handle': %" PRId64 ", 'buf-b64': %s } }",
+                 id, enc);
     g_assert_nonnull(ret);
     qmp_assert_no_error(ret);

@@ -424,23 +421,20 @@ static void test_qga_file_ops(gconstpointer fix)
     g_assert_cmpint(count, ==, sizeof(helloworld));
     g_assert_cmpint(eof, ==, 0);
     QDECREF(ret);
-    g_free(cmd);

     /* flush */
-    cmd = g_strdup_printf("{'execute': 'guest-file-flush',"
-                          " 'arguments': {'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-flush',"
+                 " 'arguments': {'handle': %" PRId64 "} }",
+                 id);
     QDECREF(ret);
-    g_free(cmd);

     /* close */
-    cmd = g_strdup_printf("{'execute': 'guest-file-close',"
-                          " 'arguments': {'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-close',"
+                 " 'arguments': {'handle': %" PRId64 "} }",
+                 id);
     QDECREF(ret);
-    g_free(cmd);

     /* check content */
     path = g_build_filename(fixture->test_dir, "foo", NULL);
@@ -462,10 +456,10 @@ static void test_qga_file_ops(gconstpointer fix)
     QDECREF(ret);

     /* read */
-    cmd = g_strdup_printf("{'execute': 'guest-file-read',"
-                          " 'arguments': { 'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-read',"
+                 " 'arguments': { 'handle': %" PRId64 "} }",
+                 id);
     val = qdict_get_qdict(ret, "return");
     count = qdict_get_int(val, "count");
     eof = qdict_get_bool(val, "eof");
@@ -475,14 +469,13 @@ static void test_qga_file_ops(gconstpointer fix)
     g_assert_cmpstr(b64, ==, enc);

     QDECREF(ret);
-    g_free(cmd);
     g_free(enc);

     /* read eof */
-    cmd = g_strdup_printf("{'execute': 'guest-file-read',"
-                          " 'arguments': { 'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-read',"
+                 " 'arguments': { 'handle': %" PRId64 "} }",
+                 id);
     val = qdict_get_qdict(ret, "return");
     count = qdict_get_int(val, "count");
     eof = qdict_get_bool(val, "eof");
@@ -491,14 +484,13 @@ static void test_qga_file_ops(gconstpointer fix)
     g_assert(eof);
     g_assert_cmpstr(b64, ==, "");
     QDECREF(ret);
-    g_free(cmd);

     /* seek */
-    cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
-                          " 'arguments': { 'handle': %" PRId64 ", "
-                          " 'offset': %d, 'whence': '%s' } }",
-                          id, 6, "set");
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-seek',"
+                 " 'arguments': { 'handle': %" PRId64 ", "
+                 " 'offset': %d, 'whence': %s } }",
+                 id, 6, "set");
     qmp_assert_no_error(ret);
     val = qdict_get_qdict(ret, "return");
     count = qdict_get_int(val, "position");
@@ -506,13 +498,12 @@ static void test_qga_file_ops(gconstpointer fix)
     g_assert_cmpint(count, ==, 6);
     g_assert(!eof);
     QDECREF(ret);
-    g_free(cmd);

     /* partial read */
-    cmd = g_strdup_printf("{'execute': 'guest-file-read',"
-                          " 'arguments': { 'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-read',"
+                 " 'arguments': { 'handle': %" PRId64 "} }",
+                 id);
     val = qdict_get_qdict(ret, "return");
     count = qdict_get_int(val, "count");
     eof = qdict_get_bool(val, "eof");
@@ -525,15 +516,13 @@ static void test_qga_file_ops(gconstpointer fix)
     g_free(dec);

     QDECREF(ret);
-    g_free(cmd);

     /* close */
-    cmd = g_strdup_printf("{'execute': 'guest-file-close',"
-                          " 'arguments': {'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-close',"
+                 " 'arguments': {'handle': %" PRId64 "} }",
+                 id);
     QDECREF(ret);
-    g_free(cmd);
 }

 static void test_qga_file_write_read(gconstpointer fix)
@@ -541,7 +530,7 @@ static void test_qga_file_write_read(gconstpointer fix)
     const TestFixture *fixture = fix;
     const unsigned char helloworld[] = "Hello World!\n";
     const char *b64;
-    gchar *cmd, *enc;
+    gchar *enc;
     QDict *ret, *val;
     int64_t id, eof;
     gsize count;
@@ -556,10 +545,10 @@ static void test_qga_file_write_read(gconstpointer fix)

     enc = g_base64_encode(helloworld, sizeof(helloworld));
     /* write */
-    cmd = g_strdup_printf("{'execute': 'guest-file-write',"
-                          " 'arguments': { 'handle': %" PRId64 ","
-                          " 'buf-b64': '%s' } }", id, enc);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-write',"
+                 " 'arguments': { 'handle': %" PRId64 ","
+                 " 'buf-b64': %s } }", id, enc);
     g_assert_nonnull(ret);
     qmp_assert_no_error(ret);

@@ -569,13 +558,12 @@ static void test_qga_file_write_read(gconstpointer fix)
     g_assert_cmpint(count, ==, sizeof(helloworld));
     g_assert_cmpint(eof, ==, 0);
     QDECREF(ret);
-    g_free(cmd);

     /* read (check implicit flush) */
-    cmd = g_strdup_printf("{'execute': 'guest-file-read',"
-                          " 'arguments': { 'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-read',"
+                 " 'arguments': { 'handle': %" PRId64 "} }",
+                 id);
     val = qdict_get_qdict(ret, "return");
     count = qdict_get_int(val, "count");
     eof = qdict_get_bool(val, "eof");
@@ -584,14 +572,13 @@ static void test_qga_file_write_read(gconstpointer fix)
     g_assert(eof);
     g_assert_cmpstr(b64, ==, "");
     QDECREF(ret);
-    g_free(cmd);

     /* seek to 0 */
-    cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
-                          " 'arguments': { 'handle': %" PRId64 ", "
-                          " 'offset': %d, 'whence': '%s' } }",
-                          id, 0, "set");
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-seek',"
+                 " 'arguments': { 'handle': %" PRId64 ", "
+                 " 'offset': %d, 'whence': %s } }",
+                 id, 0, "set");
     qmp_assert_no_error(ret);
     val = qdict_get_qdict(ret, "return");
     count = qdict_get_int(val, "position");
@@ -599,13 +586,12 @@ static void test_qga_file_write_read(gconstpointer fix)
     g_assert_cmpint(count, ==, 0);
     g_assert(!eof);
     QDECREF(ret);
-    g_free(cmd);

     /* read */
-    cmd = g_strdup_printf("{'execute': 'guest-file-read',"
-                          " 'arguments': { 'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-read',"
+                 " 'arguments': { 'handle': %" PRId64 "} }",
+                 id);
     val = qdict_get_qdict(ret, "return");
     count = qdict_get_int(val, "count");
     eof = qdict_get_bool(val, "eof");
@@ -614,16 +600,14 @@ static void test_qga_file_write_read(gconstpointer fix)
     g_assert(eof);
     g_assert_cmpstr(b64, ==, enc);
     QDECREF(ret);
-    g_free(cmd);
     g_free(enc);

     /* close */
-    cmd = g_strdup_printf("{'execute': 'guest-file-close',"
-                          " 'arguments': {'handle': %" PRId64 "} }",
-                          id);
-    ret = qmp_fd(fixture->fd, cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-file-close',"
+                 " 'arguments': {'handle': %" PRId64 "} }",
+                 id);
     QDECREF(ret);
-    g_free(cmd);
 }

 static void test_qga_get_time(gconstpointer fix)
@@ -647,7 +631,6 @@ static void test_qga_set_time(gconstpointer fix)
     const TestFixture *fixture = fix;
     QDict *ret;
     int64_t current, time;
-    gchar *cmd;

     /* get current time */
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-time'}");
@@ -673,11 +656,10 @@ static void test_qga_set_time(gconstpointer fix)
     QDECREF(ret);

     /* set back current time */
-    cmd = g_strdup_printf("{'execute': 'guest-set-time',"
-                          " 'arguments': { 'time': %" PRId64 " } }",
-                          current + time * 1000);
-    ret = qmp_fd(fixture->fd, cmd);
-    g_free(cmd);
+    ret = qmp_fd(fixture->fd,
+                 "{'execute': 'guest-set-time',"
+                 " 'arguments': { 'time': %" PRId64 " } }",
+                 current + time * 1000);
     g_assert_nonnull(ret);
     qmp_assert_no_error(ret);
     QDECREF(ret);
@@ -864,7 +846,6 @@ static void test_qga_guest_exec(gconstpointer fix)
     int64_t pid, now, exitcode;
     gsize len;
     bool exited;
-    char *cmd;

     /* exec 'echo foo bar' */
     ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
@@ -879,10 +860,10 @@ static void test_qga_guest_exec(gconstpointer fix)

     /* wait for completion */
     now = g_get_monotonic_time();
-    cmd = g_strdup_printf("{'execute': 'guest-exec-status',"
-                          " 'arguments': { 'pid': %" PRId64 " } }", pid);
     do {
-        ret = qmp_fd(fixture->fd, cmd);
+        ret = qmp_fd(fixture->fd,
+                     "{'execute': 'guest-exec-status',"
+                     " 'arguments': { 'pid': %" PRId64 " } }", pid);
         g_assert_nonnull(ret);
         val = qdict_get_qdict(ret, "return");
         exited = qdict_get_bool(val, "exited");
@@ -892,7 +873,6 @@ static void test_qga_guest_exec(gconstpointer fix)
     } while (!exited &&
              g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
     g_assert(exited);
-    g_free(cmd);

     /* check stdout */
     exitcode = qdict_get_int(val, "exitcode");
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index d4da09f147..f2a2b6cad9 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -662,11 +662,7 @@ static void test_migrate(void)
     g_assert(qdict_haskey(rsp, "return"));
     QDECREF(rsp);

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

-- 
2.13.3

[Qemu-devel] [PATCH v3 06/12] tests/libqos/usb: Clean up string interpolation into QMP input

[Eric Blake]

From: Markus Armbruster <armbru@redhat.com>

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

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

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1500645206-13559-6-git-send-email-armbru@redhat.com>
[fix commit message typo]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqos/usb.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
index 0cdfaecda7..f88d4a6a3a 100644
--- a/tests/libqos/usb.c
+++ b/tests/libqos/usb.c
@@ -40,18 +40,20 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
 void usb_test_hotplug(const char *hcd_id, const int port,
                       void (*port_check)(void))
 {
+    char id[32];
+    char *bus;
     QDict *response;
-    char  *cmd;

-    cmd = g_strdup_printf("{'execute': 'device_add',"
-                          " 'arguments': {"
-                          "   'driver': 'usb-tablet',"
-                          "   'port': '%d',"
-                          "   'bus': '%s.0',"
-                          "   'id': 'usbdev%d'"
-                          "}}", port, hcd_id, port);
-    response = qmp(cmd);
-    g_free(cmd);
+    sprintf(id, "usbdev%d", port);
+    bus = g_strdup_printf("%s.0", hcd_id);
+    response = qmp("{'execute': 'device_add',"
+                   " 'arguments': {"
+                   "   'driver': 'usb-tablet',"
+                   "   'port': %s,"
+                   "   'bus': %s,"
+                   "   'id': %s"
+                   " }}", id + 6, bus, id);
+    g_free(bus);
     g_assert(response);
     g_assert(!qdict_haskey(response, "error"));
     QDECREF(response);
@@ -60,12 +62,8 @@ void usb_test_hotplug(const char *hcd_id, const int port,
         port_check();
     }

-    cmd = g_strdup_printf("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'usbdev%d'"
-                           "}}", port);
-    response = qmp(cmd);
-    g_free(cmd);
+    response = qmp("{'execute': 'device_del', 'arguments': { 'id': %s }}",
+                   id);
     g_assert(response);
     g_assert(qdict_haskey(response, "event"));
     g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-- 
2.13.3

[Qemu-devel] [PATCH v3 07/12] qtest: Add a new helper qmp_cmd() and friends

[Eric Blake]

When interpolating a QMP command from the testsuite, we have
a very common pattern that the command to be executed is a
string constant, while the arguments may need to be crafted
from qobject_from_jsonf() or even by hand.  Make it easier to
craft the arguments without having to save all the interpolation
to the final qmp() call, by adding new helper functions; the
function takes QObject instead of QDict in order to reduce the
number of conversions between the two.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqtest.h | 31 +++++++++++++++++++++++++++++++
 tests/libqtest.c | 25 +++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index 82e89b4d4f..26930fed4d 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -68,6 +68,17 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
 QDict *qtest_qmp(QTestState *s, const char *fmt, ...);

 /**
+ * qtest_qmp_cmd:
+ * @s: #QTestState instance to operate on.
+ * @cmd: Command name to send
+ * @args: Arguments to transfer to the command, or NULL.
+ *
+ * Sends a QMP message to QEMU and returns the response. Calling this will
+ * reduce the reference count of @args.
+ */
+QDict *qtest_qmp_cmd(QTestState *s, const char *cmd, QObject *args);
+
+/**
  * qtest_async_qmp:
  * @s: #QTestState instance to operate on.
  * @fmt...: QMP message to send to qemu; formats arguments through
@@ -550,6 +561,16 @@ static inline void qtest_end(void)
 QDict *qmp(const char *fmt, ...);

 /**
+ * qmp_cmd:
+ * @cmd: Command name to send
+ * @args: Arguments to transfer to the command, or NULL.
+ *
+ * Sends a QMP message to QEMU and returns the response. Calling this will
+ * reduce the reference count of @args.
+ */
+QDict *qmp_cmd(const char *cmd, QObject *args);
+
+/**
  * qmp_async:
  * @fmt...: QMP message to send to qemu; formats arguments through
  * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
@@ -568,6 +589,16 @@ void qmp_async(const char *fmt, ...);
 void qmp_discard_response(const char *fmt, ...);

 /**
+ * qmp_cmd_discard_response:
+ * @cmd: Command name to send
+ * @args: Arguments to transfer to the command, or NULL.
+ *
+ * Sends a QMP message to QEMU and consumes the response. Calling this will
+ * reduce the reference count of @args.
+ */
+void qmp_cmd_discard_response(const char *cmd, QObject *args);
+
+/**
  * qmp_receive:
  *
  * Reads a QMP message from QEMU and returns the response.
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 4a5492a603..1b57cedca1 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -540,6 +540,17 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...)
     return response;
 }

+QDict *qtest_qmp_cmd(QTestState *s, const char *cmd, QObject *args)
+{
+    QDict *dict = qdict_new();
+
+    qdict_put_str(dict, "execute", cmd);
+    if (args) {
+        qdict_put_obj(dict, "arguments", args);
+    }
+    return qtest_qmp(s, "%p", QOBJECT(dict));
+}
+
 void qtest_async_qmp(QTestState *s, const char *fmt, ...)
 {
     va_list ap;
@@ -925,6 +936,11 @@ QDict *qmp(const char *fmt, ...)
     return response;
 }

+QDict *qmp_cmd(const char *cmd, QObject *args)
+{
+    return qtest_qmp_cmd(global_qtest, cmd, args);
+}
+
 void qmp_async(const char *fmt, ...)
 {
     va_list ap;
@@ -942,6 +958,15 @@ void qmp_discard_response(const char *fmt, ...)
     qtest_qmpv_discard_response(global_qtest, fmt, ap);
     va_end(ap);
 }
+
+void qmp_cmd_discard_response(const char *cmd, QObject *args)
+{
+    QDict *response;
+
+    response = qmp_cmd(cmd, args);
+    QDECREF(response);
+}
+
 char *hmp(const char *fmt, ...)
 {
     va_list ap;
-- 
2.13.3

[Qemu-devel] [PATCH v3 08/12] qtests: convert tests to use qmp_cmd

[Eric Blake]

Now that we have the qmp_cmd() helper, we can further simplify
some of the tests by using it.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/device-introspect-test.c |  3 +--
 tests/ide-test.c               |  2 +-
 tests/libqos/libqos.c          |  5 +++--
 tests/libqos/pci-pc.c          |  4 ++--
 tests/libqos/usb.c             | 18 +++++++++---------
 tests/pc-cpu-test.c            | 10 +++++-----
 tests/postcopy-test.c          |  9 +++++----
 tests/vhost-user-test.c        | 12 ++++++------
 8 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
index f7162c023f..fc6d559e14 100644
--- a/tests/device-introspect-test.c
+++ b/tests/device-introspect-test.c
@@ -36,8 +36,7 @@ static QList *qom_list_types(const char *implements, bool abstract)
     if (implements) {
         qdict_put_str(args, "implements", implements);
     }
-    resp = qmp("{'execute': 'qom-list-types',"
-               " 'arguments': %p }", args);
+    resp = qmp_cmd("qom-list-types", QOBJECT(args));
     g_assert(qdict_haskey(resp, "return"));
     ret = qdict_get_qlist(resp, "return");
     QINCREF(ret);
diff --git a/tests/ide-test.c b/tests/ide-test.c
index ea2657d3d1..75dc472e6a 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -651,7 +651,7 @@ static void test_retry_flush(const char *machine)
     qmp_eventwait("STOP");

     /* Complete the command */
-    qmp_discard_response("{'execute':'cont' }");
+    qmp_cmd_discard_response("cont", NULL);

     /* Check registers */
     data = qpci_io_readb(dev, ide_bar, reg_device);
diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
index 42c5315423..18844617ae 100644
--- a/tests/libqos/libqos.c
+++ b/tests/libqos/libqos.c
@@ -4,6 +4,7 @@
 #include "libqtest.h"
 #include "libqos/libqos.h"
 #include "libqos/pci.h"
+#include "qapi/qmp/qjson.h"

 /*** Test Setup & Teardown ***/

@@ -86,7 +87,7 @@ void set_context(QOSState *s)

 static QDict *qmp_execute(const char *command)
 {
-    return qmp("{ 'execute': %s }", command);
+    return qmp_cmd(command, NULL);
 }

 void migrate(QOSState *from, QOSState *to, const char *uri)
@@ -106,7 +107,7 @@ void migrate(QOSState *from, QOSState *to, const char *uri)
     QDECREF(rsp);

     /* Issue the migrate command. */
-    rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
+    rsp = qmp_cmd("migrate", qobject_from_jsonf("{ 'uri': %s }", uri));
     g_assert(qdict_haskey(rsp, "return"));
     QDECREF(rsp);

diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
index d40aa9dffd..8494671290 100644
--- a/tests/libqos/pci-pc.c
+++ b/tests/libqos/pci-pc.c
@@ -17,6 +17,7 @@
 #include "hw/pci/pci_regs.h"

 #include "qemu-common.h"
+#include "qapi/qmp/qjson.h"


 #define ACPI_PCIHP_ADDR         0xae00
@@ -160,8 +161,7 @@ void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
 {
     QDict *response;

-    response = qmp("{'execute': 'device_del', 'arguments': {'id': %s}}",
-                   id);
+    response = qmp_cmd("device_del", qobject_from_jsonf("{'id': %s}", id));
     g_assert(response);
     g_assert(!qdict_haskey(response, "error"));
     QDECREF(response);
diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
index f88d4a6a3a..a96f5ebd6a 100644
--- a/tests/libqos/usb.c
+++ b/tests/libqos/usb.c
@@ -15,6 +15,7 @@
 #include "libqtest.h"
 #include "hw/usb/uhci-regs.h"
 #include "libqos/usb.h"
+#include "qapi/qmp/qjson.h"

 void qusb_pci_init_one(QPCIBus *pcibus, struct qhc *hc, uint32_t devfn, int bar)
 {
@@ -46,13 +47,13 @@ void usb_test_hotplug(const char *hcd_id, const int port,

     sprintf(id, "usbdev%d", port);
     bus = g_strdup_printf("%s.0", hcd_id);
-    response = qmp("{'execute': 'device_add',"
-                   " 'arguments': {"
-                   "   'driver': 'usb-tablet',"
-                   "   'port': %s,"
-                   "   'bus': %s,"
-                   "   'id': %s"
-                   " }}", id + 6, bus, id);
+    response = qmp_cmd("device_add",
+                       qobject_from_jsonf("{"
+                                          "   'driver': 'usb-tablet',"
+                                          "   'port': %s,"
+                                          "   'bus': %s,"
+                                          "   'id': %s"
+                                          "}", id + 6, bus, id));
     g_free(bus);
     g_assert(response);
     g_assert(!qdict_haskey(response, "error"));
@@ -62,8 +63,7 @@ void usb_test_hotplug(const char *hcd_id, const int port,
         port_check();
     }

-    response = qmp("{'execute': 'device_del', 'arguments': { 'id': %s }}",
-                   id);
+    response = qmp_cmd("device_del", qobject_from_jsonf("{ 'id': %s }", id));
     g_assert(response);
     g_assert(qdict_haskey(response, "event"));
     g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
diff --git a/tests/pc-cpu-test.c b/tests/pc-cpu-test.c
index c4211a4e85..9eb4c16965 100644
--- a/tests/pc-cpu-test.c
+++ b/tests/pc-cpu-test.c
@@ -12,6 +12,7 @@
 #include "qemu-common.h"
 #include "libqtest.h"
 #include "qapi/qmp/types.h"
+#include "qapi/qmp/qjson.h"

 struct PCTestData {
     char *machine;
@@ -37,8 +38,7 @@ static void test_pc_with_cpu_add(gconstpointer data)
     qtest_start(args);

     for (i = s->sockets * s->cores * s->threads; i < s->maxcpus; i++) {
-        response = qmp("{ 'execute': 'cpu-add',"
-                       "  'arguments': { 'id': %d } }", i);
+        response = qmp_cmd("cpu-add", qobject_from_jsonf("{'id':%u}", i));
         g_assert(response);
         g_assert(!qdict_haskey(response, "error"));
         QDECREF(response);
@@ -60,9 +60,9 @@ static void test_pc_without_cpu_add(gconstpointer data)
                            s->sockets, s->cores, s->threads, s->maxcpus);
     qtest_start(args);

-    response = qmp("{ 'execute': 'cpu-add',"
-                   "  'arguments': { 'id': %d } }",
-                   s->sockets * s->cores * s->threads);
+    response = qmp_cmd("cpu-add",
+                       qobject_from_jsonf("{'id':%u}",
+                                          s->sockets * s->cores * s->threads));
     g_assert(response);
     g_assert(qdict_haskey(response, "error"));
     QDECREF(response);
diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index e2ead875a8..f6e7340a6a 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -19,6 +19,7 @@
 #include "chardev/char.h"
 #include "sysemu/sysemu.h"
 #include "hw/nvram/chrp_nvram.h"
+#include "qapi/qmp/qjson.h"

 #define MIN_NVRAM_SIZE 8192 /* from spapr_nvram.c */

@@ -252,7 +253,7 @@ static uint64_t get_migration_pass(void)
     QDict *rsp, *rsp_return, *rsp_ram;
     uint64_t result;

-    rsp = return_or_event(qmp("{ 'execute': 'query-migrate' }"));
+    rsp = return_or_event(qmp_cmd("query-migrate", NULL));
     rsp_return = qdict_get_qdict(rsp, "return");
     if (!qdict_haskey(rsp_return, "ram")) {
         /* Still in setup */
@@ -273,7 +274,7 @@ static void wait_for_migration_complete(void)
     do {
         const char *status;

-        rsp = return_or_event(qmp("{ 'execute': 'query-migrate' }"));
+        rsp = return_or_event(qmp_cmd("query-migrate", NULL));
         rsp_return = qdict_get_qdict(rsp, "return");
         status = qdict_get_str(rsp_return, "status");
         completed = strcmp(status, "completed") == 0;
@@ -445,13 +446,13 @@ static void test_migrate(void)
     /* Wait for the first serial output from the source */
     wait_for_serial("src_serial");

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

     wait_for_migration_pass();

-    rsp = return_or_event(qmp("{ 'execute': 'migrate-start-postcopy' }"));
+    rsp = return_or_event(qmp_cmd("migrate-start-postcopy", NULL));
     g_assert(qdict_haskey(rsp, "return"));
     QDECREF(rsp);

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index f2a2b6cad9..88e1e20f99 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -9,6 +9,10 @@
  */

 #include "qemu/osdep.h"
+#include <linux/vhost.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_net.h>
+#include <sys/vfs.h>

 #include "libqtest.h"
 #include "qapi/error.h"
@@ -22,15 +26,11 @@
 #include "libqos/pci-pc.h"
 #include "libqos/virtio-pci.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qjson.h"

 #include "libqos/malloc-pc.h"
 #include "hw/virtio/virtio-net.h"

-#include <linux/vhost.h>
-#include <linux/virtio_ids.h>
-#include <linux/virtio_net.h>
-#include <sys/vfs.h>
-
 /* GLIB version compatibility flags */
 #if !GLIB_CHECK_VERSION(2, 26, 0)
 #define G_TIME_SPAN_SECOND              (G_GINT64_CONSTANT(1000000))
@@ -662,7 +662,7 @@ static void test_migrate(void)
     g_assert(qdict_haskey(rsp, "return"));
     QDECREF(rsp);

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

-- 
2.13.3

[Qemu-devel] [PATCH v3 09/12] tests/libqos/pci: Clean up string interpolation into QMP input

[Eric Blake]

From: Markus Armbruster <armbru@redhat.com>

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

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

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1500645206-13559-7-git-send-email-armbru@redhat.com>
[use qmp_cmd for a slightly smaller diff]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqos/pci.h      |  2 +-
 tests/ivshmem-test.c    | 10 +++++-----
 tests/libqos/pci.c      | 32 +++++++++++++++++---------------
 tests/virtio-blk-test.c |  5 ++++-
 4 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
index ed480614ff..c981061703 100644
--- a/tests/libqos/pci.h
+++ b/tests/libqos/pci.h
@@ -109,6 +109,6 @@ void qpci_iounmap(QPCIDevice *dev, QPCIBar addr);
 QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr);

 void qpci_plug_device_test(const char *driver, const char *id,
-                           uint8_t slot, const char *opts);
+                           uint8_t slot, QDict *extra_args);
 void qpci_unplug_acpi_device_test(const char *id, uint8_t slot);
 #endif
diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
index 37763425ee..38044bb01c 100644
--- a/tests/ivshmem-test.c
+++ b/tests/ivshmem-test.c
@@ -14,6 +14,7 @@
 #include "libqos/libqos-pc.h"
 #include "libqos/libqos-spapr.h"
 #include "libqtest.h"
+#include "qapi/qmp/qjson.h"
 #include "qemu-common.h"

 #define TMPSHMSIZE (1 << 20)
@@ -419,19 +420,18 @@ static void test_ivshmem_server_irq(void)
 static void test_ivshmem_hotplug(void)
 {
     const char *arch = qtest_get_arch();
-    gchar *opts;
+    QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
+                                             tmpshm);

     qtest_start("");

-    opts = g_strdup_printf("'shm': '%s', 'size': '1M'", tmpshm);
-
-    qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts);
+    qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP,
+                          qobject_to_qdict(extra_args));
     if (strcmp(arch, "ppc64") != 0) {
         qpci_unplug_acpi_device_test("iv1", PCI_SLOT_HP);
     }

     qtest_end();
-    g_free(opts);
 }

 static void test_ivshmem_memdev(void)
diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
index 2dcdeade2a..2754412340 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -14,6 +14,7 @@
 #include "libqos/pci.h"

 #include "hw/pci/pci_regs.h"
+#include "qapi/qmp/qjson.h"
 #include "qemu/host-utils.h"

 void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
@@ -392,22 +393,23 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
 }

 void qpci_plug_device_test(const char *driver, const char *id,
-                           uint8_t slot, const char *opts)
+                           uint8_t slot, QDict *extra_args)
 {
-    QDict *response;
-    char *cmd;
-
-    cmd = g_strdup_printf("{'execute': 'device_add',"
-                          " 'arguments': {"
-                          "   'driver': '%s',"
-                          "   'addr': '%d',"
-                          "   %s%s"
-                          "   'id': '%s'"
-                          "}}", driver, slot,
-                          opts ? opts : "", opts ? "," : "",
-                          id);
-    response = qmp(cmd);
-    g_free(cmd);
+    char addr[8];
+    QDict *args, *response;
+
+    sprintf(addr, "%d", slot);
+    args = qobject_to_qdict(
+        qobject_from_jsonf("{ 'driver': %s, 'addr': %s, 'id': %s}",
+                           driver, addr, id));
+
+    if (extra_args) {
+        qdict_join(args, extra_args, true);
+        QDECREF(extra_args);
+    }
+
+    response = qmp_cmd("device_add", QOBJECT(args));
+
     g_assert(response);
     g_assert(!qdict_haskey(response, "error"));
     QDECREF(response);
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 0576cb16ba..64a48f40b2 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -16,6 +16,7 @@
 #include "libqos/virtio-pci.h"
 #include "libqos/virtio-mmio.h"
 #include "libqos/malloc-generic.h"
+#include "qapi/qmp/qjson.h"
 #include "qemu/bswap.h"
 #include "standard-headers/linux/virtio_ids.h"
 #include "standard-headers/linux/virtio_config.h"
@@ -658,12 +659,13 @@ static void pci_hotplug(void)
     QVirtioPCIDevice *dev;
     QOSState *qs;
     const char *arch = qtest_get_arch();
+    QObject *extra_args = qobject_from_jsonf("{ 'drive': 'drive1' }");

     qs = pci_test_start();

     /* plug secondary disk */
     qpci_plug_device_test("virtio-blk-pci", "drv1", PCI_SLOT_HP,
-                          "'drive': 'drive1'");
+                          qobject_to_qdict(extra_args));

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

-- 
2.13.3

[Qemu-devel] [PATCH v3 10/12] tests: Clean up wait for event

[Eric Blake]

From: Markus Armbruster <armbru@redhat.com>

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

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

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1500645206-13559-8-git-send-email-armbru@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/boot-order-test.c   |  2 +-
 tests/libqos/pci-pc.c     |  6 +-----
 tests/tco-test.c          |  3 +--
 tests/usb-hcd-uhci-test.c |  6 +-----
 tests/usb-hcd-xhci-test.c | 12 ++----------
 tests/wdt_ib700-test.c    | 35 ++++++++++-------------------------
 6 files changed, 16 insertions(+), 48 deletions(-)

diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index fc1e7941f7..9d516830dd 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -43,7 +43,7 @@ static void test_a_boot_order(const char *machine,
      * system_reset only requests reset.  We get a RESET event after
      * the actual reset completes.  Need to wait for that.
      */
-    qmp_discard_response("");   /* HACK: wait for event */
+    qmp_eventwait("RESET");
     actual = read_boot_order();
     g_assert_cmphex(actual, ==, expected_reboot);
     qtest_quit(global_qtest);
diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
index 8494671290..99ce39cff1 100644
--- a/tests/libqos/pci-pc.c
+++ b/tests/libqos/pci-pc.c
@@ -168,9 +168,5 @@ void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)

     outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot);

-    response = qmp("");
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qmp_eventwait("DEVICE_DELETED");
 }
diff --git a/tests/tco-test.c b/tests/tco-test.c
index c4c264eb3d..f2ed6ed91c 100644
--- a/tests/tco-test.c
+++ b/tests/tco-test.c
@@ -237,9 +237,8 @@ static void test_tco_max_timeout(void)

 static QDict *get_watchdog_action(void)
 {
-    QDict *ev = qmp("");
+    QDict *ev = qmp_eventwait_ref("WATCHDOG");
     QDict *data;
-    g_assert(!strcmp(qdict_get_str(ev, "event"), "WATCHDOG"));

     data = qdict_get_qdict(ev, "data");
     QINCREF(data);
diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index 5b500fedb0..0fb7f8d223 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -68,11 +68,7 @@ static void test_usb_storage_hotplug(void)
     g_assert(!qdict_haskey(response, "error"));
     QDECREF(response);

-    response = qmp("");
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qmp_eventwait("DEVICE_DELETED");
 }

 int main(int argc, char **argv)
diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c
index 031764da6d..c05a339894 100644
--- a/tests/usb-hcd-xhci-test.c
+++ b/tests/usb-hcd-xhci-test.c
@@ -57,11 +57,7 @@ static void test_usb_uas_hotplug(void)
     g_assert(!qdict_haskey(response, "error"));
     QDECREF(response);

-    response = qmp("");
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
-
+    qmp_eventwait("DEVICE_DELETED");

     response = qmp("{'execute': 'device_del',"
                            " 'arguments': {"
@@ -71,11 +67,7 @@ static void test_usb_uas_hotplug(void)
     g_assert(!qdict_haskey(response, "error"));
     QDECREF(response);

-    response = qmp("");
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qmp_eventwait("DEVICE_DELETED");
 }

 int main(int argc, char **argv)
diff --git a/tests/wdt_ib700-test.c b/tests/wdt_ib700-test.c
index 49f4f0c221..4fc8eeae86 100644
--- a/tests/wdt_ib700-test.c
+++ b/tests/wdt_ib700-test.c
@@ -18,26 +18,10 @@ static void qmp_check_no_event(void)
     QDECREF(resp);
 }

-static QDict *qmp_get_event(const char *name)
-{
-    QDict *event = qmp("");
-    QDict *data;
-    g_assert(qdict_haskey(event, "event"));
-    g_assert(!strcmp(qdict_get_str(event, "event"), name));
-
-    if (qdict_haskey(event, "data")) {
-        data = qdict_get_qdict(event, "data");
-        QINCREF(data);
-    } else {
-        data = NULL;
-    }
-
-    QDECREF(event);
-    return data;
-}
-
 static QDict *ib700_program_and_wait(QTestState *s)
 {
+    QDict *event, *data;
+
     clock_step(NANOSECONDS_PER_SECOND * 40);
     qmp_check_no_event();

@@ -61,7 +45,11 @@ static QDict *ib700_program_and_wait(QTestState *s)
     clock_step(3 * NANOSECONDS_PER_SECOND);
     qmp_check_no_event();
     clock_step(2 * NANOSECONDS_PER_SECOND);
-    return qmp_get_event("WATCHDOG");
+    event = qmp_eventwait_ref("WATCHDOG");
+    data = qdict_get_qdict(event, "data");
+    QINCREF(data);
+    QDECREF(event);
+    return data;
 }


@@ -73,8 +61,7 @@ static void ib700_pause(void)
     d = ib700_program_and_wait(s);
     g_assert(!strcmp(qdict_get_str(d, "action"), "pause"));
     QDECREF(d);
-    d = qmp_get_event("STOP");
-    QDECREF(d);
+    qmp_eventwait("STOP");
     qtest_end();
 }

@@ -86,8 +73,7 @@ static void ib700_reset(void)
     d = ib700_program_and_wait(s);
     g_assert(!strcmp(qdict_get_str(d, "action"), "reset"));
     QDECREF(d);
-    d = qmp_get_event("RESET");
-    QDECREF(d);
+    qmp_eventwait("RESET");
     qtest_end();
 }

@@ -99,8 +85,7 @@ static void ib700_shutdown(void)
     d = ib700_program_and_wait(s);
     g_assert(!strcmp(qdict_get_str(d, "action"), "reset"));
     QDECREF(d);
-    d = qmp_get_event("SHUTDOWN");
-    QDECREF(d);
+    qmp_eventwait("SHUTDOWN");
     qtest_end();
 }

-- 
2.13.3

[Qemu-devel] [PATCH v3 11/12] tests/libqtest: Clean up how we read the QMP greeting

[Eric Blake]

From: Markus Armbruster <armbru@redhat.com>

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

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

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1500645206-13559-9-git-send-email-armbru@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqtest.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 1b57cedca1..b0fcbf2872 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -230,9 +230,11 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
 QTestState *qtest_init(const char *extra_args)
 {
     QTestState *s = qtest_init_without_qmp_handshake(extra_args);
+    QDict *greeting;

     /* Read the QMP greeting and then do the handshake */
-    qtest_qmp_discard_response(s, "");
+    greeting = qtest_qmp_receive(s);
+    QDECREF(greeting);
     qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");

     return s;
-- 
2.13.3

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

[Eric Blake]

From: Markus Armbruster <armbru@redhat.com>

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

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1500645206-13559-10-git-send-email-armbru@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqtest.h | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index 26930fed4d..7e1bd474aa 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -55,7 +55,8 @@ void qtest_quit(QTestState *s);
  *
  * Sends a QMP message to QEMU and consumes the response.
  */
-void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
+void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);

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

 /**
  * qtest_qmp_cmd:
@@ -86,7 +88,8 @@ QDict *qtest_qmp_cmd(QTestState *s, const char *cmd, QObject *args);
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
  */
-void qtest_async_qmp(QTestState *s, const char *fmt, ...);
+void qtest_async_qmp(QTestState *s, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);

 /**
  * qtest_qmpv_discard_response:
@@ -97,7 +100,8 @@ void qtest_async_qmp(QTestState *s, const char *fmt, ...);
  *
  * Sends a QMP message to QEMU and consumes the response.
  */
-void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap);
+void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap)
+    GCC_FMT_ATTR(2, 0);

 /**
  * qtest_qmpv:
@@ -108,7 +112,8 @@ void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap);
  *
  * Sends a QMP message to QEMU and returns the response.
  */
-QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
+QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
+    GCC_FMT_ATTR(2, 0);

 /**
  * qtest_async_qmpv:
@@ -119,7 +124,8 @@ QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
  */
-void qtest_async_qmpv(QTestState *s, const char *fmt, va_list ap);
+void qtest_async_qmpv(QTestState *s, const char *fmt, va_list ap)
+    GCC_FMT_ATTR(2, 0);

 /**
  * qtest_receive:
@@ -558,7 +564,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_cmd:
@@ -577,7 +583,7 @@ QDict *qmp_cmd(const char *cmd, QObject *args);
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
  */
-void qmp_async(const char *fmt, ...);
+void qmp_async(const char *fmt, ...) GCC_FMT_ATTR(1, 2);

 /**
  * qmp_discard_response:
@@ -586,7 +592,7 @@ void qmp_async(const char *fmt, ...);
  *
  * Sends a QMP message to QEMU and consumes the response.
  */
-void qmp_discard_response(const char *fmt, ...);
+void qmp_discard_response(const char *fmt, ...) GCC_FMT_ATTR(1, 2);

 /**
  * qmp_cmd_discard_response:
@@ -955,10 +961,10 @@ static inline int64_t clock_set(int64_t val)
 }

 QDict *qmp_fd_receive(int fd);
-void qmp_fd_sendv(int fd, const char *fmt, va_list ap);
-void qmp_fd_send(int fd, const char *fmt, ...);
-QDict *qmp_fdv(int fd, const char *fmt, va_list ap);
-QDict *qmp_fd(int fd, const char *fmt, ...);
+void qmp_fd_sendv(int fd, const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
+void qmp_fd_send(int fd, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
+QDict *qmp_fdv(int fd, const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
+QDict *qmp_fd(int fd, const char *fmt, ...) GCC_FMT_ATTR(2, 3);

 /**
  * qtest_cb_for_every_machine:
-- 
2.13.3

Re: [Qemu-devel] [PATCH v3 04/12] tests: Pass literal format strings directly to qmp_FOO()

[John Snow]

On 07/25/2017 05:15 PM, Eric Blake wrote:
> From: Markus Armbruster <armbru@redhat.com>
> 
> 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>
> Message-Id: <1500645206-13559-4-git-send-email-armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v3 03/12] qtest: Document calling conventions

[Stefan Hajnoczi]

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

On Tue, Jul 25, 2017 at 04:15:14PM -0500, Eric Blake wrote:
> We have two flavors of vararg usage in qtest; make it clear that
> qmp() has different semantics than hmp(), and let the compiler
> enforce that hmp() is used correctly. However, qmp() (and friends)
> only accept a subset of printf flags look-alikes (namely, those
> that our JSON parser understands), and what is worse, qmp("true")
> (the JSON keyword 'true') is different from qmp("%s", "true")
> (the JSON string '"true"'), and we have some intermediate cleanup
> patches to do before we can mark those as printf-like.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: restore lost attributes, add comments on va_list forms, tweak
> commit message to mention upcoming qmp cleanups
> v2: several comment tweaks, explain why qmp() can't be marked
> ---
>  tests/libqtest.h | 40 +++++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 15 deletions(-)

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

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

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

[Stefan Hajnoczi]

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

On Tue, Jul 25, 2017 at 04:15:16PM -0500, Eric Blake wrote:
> From: Markus Armbruster <armbru@redhat.com>
> 
> When you build QMP input manually like this
> 
>     cmd = g_strdup_printf("{ 'execute': 'migrate',"
>                           "'arguments': { 'uri': '%s' } }",
>                           uri);
>     rsp = qmp(cmd);
>     g_free(cmd);
> 
> you're responsible for escaping the interpolated values for JSON.  Not
> done here, and therefore works only for sufficiently nice @uri.  For
> instance, if @uri contained a single "'", qobject_from_jsonv() would
> fail, qmp_fd_sendv() would misinterpret the failure as empty input and
> do nothing, and the test would hang waiting for a response that never
> comes.
> 
> Leaving interpolation into JSON to qmp() is more robust:
> 
>     rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
> 
> It's also more concise.
> 
> Clean up the simple cases where we interpolate exactly a JSON value.
> 
> Bonus: gets rid of non-literal format strings.  A step towards
> compile-time format string checking without triggering
> -Wformat-nonliteral.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Message-Id: <1500645206-13559-5-git-send-email-armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/libqos/libqos.c   |  16 +----
>  tests/libqos/pci-pc.c   |   9 +--
>  tests/postcopy-test.c   |   8 +--
>  tests/test-qga.c        | 160 +++++++++++++++++++++---------------------------
>  tests/vhost-user-test.c |   6 +-
>  5 files changed, 77 insertions(+), 122 deletions(-)

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

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

Re: [Qemu-devel] [PATCH v3 06/12] tests/libqos/usb: Clean up string interpolation into QMP input

[Stefan Hajnoczi]

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

On Tue, Jul 25, 2017 at 04:15:17PM -0500, Eric Blake wrote:
> From: Markus Armbruster <armbru@redhat.com>
> 
> Leaving interpolation into JSON to qmp() is more robust than building
> QMP input manually, as explained in the previous commit.
> 
> The case in usb_test_hotplug() is slightly more complicated: it
> interpolates *into* JSON values.  Clean it up by building the values
> separately, so we can again leave interpolation to qmp().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Message-Id: <1500645206-13559-6-git-send-email-armbru@redhat.com>
> [fix commit message typo]
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/libqos/usb.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)

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

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

Re: [Qemu-devel] [PATCH v3 07/12] qtest: Add a new helper qmp_cmd() and friends

[Stefan Hajnoczi]

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

On Tue, Jul 25, 2017 at 04:15:18PM -0500, Eric Blake wrote:
> +QDict *qtest_qmp_cmd(QTestState *s, const char *cmd, QObject *args)
> +{
> +    QDict *dict = qdict_new();
> +
> +    qdict_put_str(dict, "execute", cmd);
> +    if (args) {
> +        qdict_put_obj(dict, "arguments", args);
> +    }
> +    return qtest_qmp(s, "%p", QOBJECT(dict));
> +}

qtest_qmp(s, "%p", QOBJECT(dict)) takes ownership of dict?

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

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

Re: [Qemu-devel] [Qemu-block] [PATCH v3 08/12] qtests: convert tests to use qmp_cmd

[Stefan Hajnoczi]

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

On Tue, Jul 25, 2017 at 04:15:19PM -0500, Eric Blake wrote:
> Now that we have the qmp_cmd() helper, we can further simplify
> some of the tests by using it.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/device-introspect-test.c |  3 +--
>  tests/ide-test.c               |  2 +-
>  tests/libqos/libqos.c          |  5 +++--
>  tests/libqos/pci-pc.c          |  4 ++--
>  tests/libqos/usb.c             | 18 +++++++++---------
>  tests/pc-cpu-test.c            | 10 +++++-----
>  tests/postcopy-test.c          |  9 +++++----
>  tests/vhost-user-test.c        | 12 ++++++------
>  8 files changed, 32 insertions(+), 31 deletions(-)

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

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

Re: [Qemu-devel] [PATCH v3 09/12] tests/libqos/pci: Clean up string interpolation into QMP input

[Stefan Hajnoczi]

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

On Tue, Jul 25, 2017 at 04:15:20PM -0500, Eric Blake wrote:
> @@ -419,19 +420,18 @@ static void test_ivshmem_server_irq(void)
>  static void test_ivshmem_hotplug(void)
>  {
>      const char *arch = qtest_get_arch();
> -    gchar *opts;
> +    QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
> +                                             tmpshm);

Is there a difference between:

  qobject_from_jsonf("{ 'shm': '%s' }", tmpshm);

and:

  qobject_from_jsonf("{ 'shm': %s }", tmpshm);

?

Below you use %s instead of '%s'.

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

Re: [Qemu-devel] [PATCH v3 07/12] qtest: Add a new helper qmp_cmd() and friends

[Eric Blake]

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

On 07/28/2017 07:57 AM, Stefan Hajnoczi wrote:
> On Tue, Jul 25, 2017 at 04:15:18PM -0500, Eric Blake wrote:
>> +QDict *qtest_qmp_cmd(QTestState *s, const char *cmd, QObject *args)
>> +{
>> +    QDict *dict = qdict_new();
>> +
>> +    qdict_put_str(dict, "execute", cmd);
>> +    if (args) {
>> +        qdict_put_obj(dict, "arguments", args);
>> +    }
>> +    return qtest_qmp(s, "%p", QOBJECT(dict));
>> +}
> 
> qtest_qmp(s, "%p", QOBJECT(dict)) takes ownership of dict?

Hmm, I documented that for qtest_qmp_cmd(), but you have a good
question.  I need to double-check that it is true for
qobject_from_jsonf("%p", obj), but my recollection is that yes, wrapping
one object inside the other means freeing the outer object also reclaims
the inner (that is, qobject_from_jsonf wasn't increasing refcount).  At
any rate, I'll probably have to submit a followup patch (either to fix
the leak if I'm wrong, or to improve the documentation about %p
reference management if I'm right).

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

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


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

Re: [Qemu-devel] [PATCH v3 07/12] qtest: Add a new helper qmp_cmd() and friends

[Eric Blake]

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

On 07/28/2017 08:06 AM, Eric Blake wrote:
> On 07/28/2017 07:57 AM, Stefan Hajnoczi wrote:
>> On Tue, Jul 25, 2017 at 04:15:18PM -0500, Eric Blake wrote:
>>> +QDict *qtest_qmp_cmd(QTestState *s, const char *cmd, QObject *args)
>>> +{
>>> +    QDict *dict = qdict_new();
>>> +
>>> +    qdict_put_str(dict, "execute", cmd);
>>> +    if (args) {
>>> +        qdict_put_obj(dict, "arguments", args);
>>> +    }
>>> +    return qtest_qmp(s, "%p", QOBJECT(dict));
>>> +}
>>
>> qtest_qmp(s, "%p", QOBJECT(dict)) takes ownership of dict?
> 
> Hmm, I documented that for qtest_qmp_cmd(), but you have a good
> question.  I need to double-check that it is true for
> qobject_from_jsonf("%p", obj), but my recollection is that yes, wrapping
> one object inside the other means freeing the outer object also reclaims
> the inner (that is, qobject_from_jsonf wasn't increasing refcount).  At
> any rate, I'll probably have to submit a followup patch (either to fix
> the leak if I'm wrong, or to improve the documentation about %p
> reference management if I'm right).

$ valgrind --leak-check=full ./tests/check-qjson
...
==10596== LEAK SUMMARY:
==10596==    definitely lost: 0 bytes in 0 blocks
...
$ grep -C5 %p tests/check-qjson.c
            {}}));

    embedded_obj = qobject_from_json("[32, 42]", &error_abort);
    g_assert(embedded_obj != NULL);

    obj = qobject_from_jsonf("[%d, 2, %p]", 1, embedded_obj);
    g_assert(compare_litqobj_to_qobj(&decoded, obj) == 1);

    qobject_decref(obj);
}

So given the clean bill of health from valgrind, we definitely DO turn
over responsibility for freeing on object to its new wrapper, once it is
passed through %p.  Documentation could be improved to make that clear.

Eww, what happens if qobject_from_jsonf() can fail halfway through? If
you mix %p with other stuff, how do you know on failure whether the
reference was taken or not yet reached?  Maybe the testsuite needs an
enhancement.

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


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

Re: [Qemu-devel] [PATCH v3 09/12] tests/libqos/pci: Clean up string interpolation into QMP input

[Eric Blake]

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

On 07/28/2017 08:05 AM, Stefan Hajnoczi wrote:
> On Tue, Jul 25, 2017 at 04:15:20PM -0500, Eric Blake wrote:
>> @@ -419,19 +420,18 @@ static void test_ivshmem_server_irq(void)
>>  static void test_ivshmem_hotplug(void)
>>  {
>>      const char *arch = qtest_get_arch();
>> -    gchar *opts;
>> +    QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
>> +                                             tmpshm);
> 
> Is there a difference between:
> 
>   qobject_from_jsonf("{ 'shm': '%s' }", tmpshm);
> 
> and:
> 
>   qobject_from_jsonf("{ 'shm': %s }", tmpshm);

Yes, and it's important.

sprintf("{ 'shm': '%s' }", tmpshm);

is the same as

qobject_from_jsonf("{ 'shm': %s }" tmpshm);

Passing '%s' through qobject_from_jsonf() is generally wrong (it would
produce ''...'' instead of the intended '...').

Looks like something to fix on the next round.

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


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

Re: [Qemu-devel] [PATCH v3 09/12] tests/libqos/pci: Clean up string interpolation into QMP input

[Eric Blake]

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

On 07/28/2017 11:35 AM, Eric Blake wrote:
>>> +    QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
>>> +                                             tmpshm);
>>

> Passing '%s' through qobject_from_jsonf() is generally wrong (it would
> produce ''...'' instead of the intended '...').
> 
> Looks like something to fix on the next round.
> 

What's scary is that the testsuite didn't start failing.  That's not
good; we'll want to figure out why the bug didn't impact the test.

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


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

Re: [Qemu-devel] [PATCH v3 09/12] tests/libqos/pci: Clean up string interpolation into QMP input

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Tue, Jul 25, 2017 at 04:15:20PM -0500, Eric Blake wrote:
>> @@ -419,19 +420,18 @@ static void test_ivshmem_server_irq(void)
>>  static void test_ivshmem_hotplug(void)
>>  {
>>      const char *arch = qtest_get_arch();
>> -    gchar *opts;
>> +    QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
>> +                                             tmpshm);
>
> Is there a difference between:
>
>   qobject_from_jsonf("{ 'shm': '%s' }", tmpshm);
>
> and:
>
>   qobject_from_jsonf("{ 'shm': %s }", tmpshm);
>
> ?
>
> Below you use %s instead of '%s'.

Yes.  %s interpolates a JSON string, enclosed in quotes, funny
characters quoted.  Thus, the former is wrong.  I screwed up the
conversion from g_strdup_printf().  Good catch!

Re: [Qemu-devel] [PATCH v3 01/12] qobject: Accept "%"PRId64 in qobject_from_jsonf()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Commit 1792d7d0 was written because PRId64 expands to non-portable
> crap for some libc, and we had testsuite failures on Mac OS as a
> result.  This in turn makes it difficult to rely on the obvious
> conversions of 64-bit values into JSON, requiring things such as
> casting int64_t to long long so we can use a reliable %lld and
> still keep -Wformat happy.  So now it's time to fix that.
>
> We are already lexing %I64d (hello mingw); now expand the lexer
> to parse %qd (hello Mac OS). In the process, we can drop one
> state: IN_ESCAPE_I64 is redundant with IN_ESCAPE_LL, and reused
> again by %qd, so rename it to IN_ESCAPE_64.  And fix a comment
> that mistakenly omitted %u as a supported escape.
>
> Next, tweak the parser to accept the exact spelling of PRId64
> regardless of what it expands to (note that there are are now dead
> branches in the 'if' chain for some platforms, like glibc; but
> there are other platforms where all branches are necessary).  We
> are at least safe in that we are parsing the correct size 32-bit or
> a 64-bit quantity on whatever branch we end up in.  This also means
> that we no longer parse everything that the lexer will consume (no
> more %I64d on glibc), but that is intentional.  And of course,
> update the testsuite for coverage of the feature.
>
> Finally, update configure to barf on any libc that uses yet
> another form of PRId64 that we have not yet coded for, so that we
> can once again update json-lexer.c to cater to those quirks (my
> guess? we might see %jd next) (on the bright side, json-parser.c
> should no longer need updates).  Yes, the only way I could find
> to quickly learn what spelling is preferred when cross-compiling
> is to grep a compiled binary; C does not make it easy to turn a
> string constant into an integer constant, let alone make
> preprocessor decisions; and even parsing '$CC -E' output is
> fragile, since 64-bit glibc pre-processes as "l" "d" rather than
> "ld", and newer gcc splits macro expansions across multiple lines.
> I'm assuming that 'strings' is portable enough during configure;
> if that assumption fails in some constrained docker environment,
> an easy hack is to add this to configure:
>   strings () { tr -d '\0' < "$1"; }
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v3: incorporate review comments from Markus
> ---
>  configure             | 24 ++++++++++++++++++++++++
>  qobject/json-lexer.c  | 21 +++++++++------------
>  qobject/json-parser.c | 10 ++++++----
>  tests/check-qjson.c   |  7 +++++++
>  4 files changed, 46 insertions(+), 16 deletions(-)
>
> diff --git a/configure b/configure
> index 987f59ba88..5b0eddec3c 100755
> --- a/configure
> +++ b/configure
> @@ -3222,6 +3222,30 @@ for i in $glib_modules; do
>      fi
>  done
>
> +# Sanity check that we can parse PRId64 and friends in json-lexer.c
> +# (Sadly, the "easiest" way to do this is to grep the compiled binary,
> +# since we can't control whether the preprocessor would output "ld" vs.
> +# "l" "d", nor even portably ensure it fits output on the same line as
> +# a witness marker.)
> +cat > $TMPC <<EOF
> +#include <inttypes.h>
> +int main(void) {
> +    static const char findme[] = "\nUnLiKeLyToClAsH_" PRId64 "\n";
> +    return !findme[0];
> +}
> +EOF
> +if ! compile_prog "$CFLAGS" "$LIBS" ; then

Would compile_object suffice?

> +    error_exit "can't determine value of PRId64"
> +fi
> +nl='
> +'
> +case $(strings $TMPE | grep ^UnLiKeLyToClAsH) in
> +    '' | *"$nl"* ) error_exit "can't determine value of PRId64" ;;
> +    *_ld | *_lld | *_I64d | *_qd) ;;
> +    *) error_exit "unexepected value of PRId64, please add %$(strings $TMPE |
> +       sed -n s/^UnLiKeLyToClAsH_//p) support to json-lexer.c" ;;
> +esac
> +
>  # Sanity check that the current size_t matches the
>  # size that glib thinks it should be. This catches
>  # problems on multi-arch where people try to build
> diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
> index 980ba159d6..9a0fc58444 100644
> --- a/qobject/json-lexer.c
> +++ b/qobject/json-lexer.c
> @@ -29,9 +29,11 @@
>   *
>   * '([^\\']|\\[\"'\\/bfnrt]|\\u[0-9a-fA-F]{4})*'
>   *
> - * Extension for vararg handling in JSON construction:
> + * Extension for vararg handling in JSON construction [this lexer
> + * actually accepts multiple forms of PRId64, but parse_escape() later
> + * filters to only parse the current platform's spelling]:

I'd stick to (parenthesis) instead of [square brackets] here.

>   *
> - * %((l|ll|I64)?d|[ipsf])
> + * %(PRI[du]64|(l|ll)?[ud]|[ipsf])
>   *
>   */
>
[...]

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v3 03/12] qtest: Document calling conventions

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> We have two flavors of vararg usage in qtest; make it clear that
> qmp() has different semantics than hmp(), and let the compiler
> enforce that hmp() is used correctly. However, qmp() (and friends)
> only accept a subset of printf flags look-alikes (namely, those
> that our JSON parser understands), and what is worse, qmp("true")
> (the JSON keyword 'true') is different from qmp("%s", "true")
> (the JSON string '"true"'), and we have some intermediate cleanup
> patches to do before we can mark those as printf-like.

It's not "worse", it's just different :)

Suggest:

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

Note that my text omits most of the details that aren't directly
relevant to the patch.

>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v3: restore lost attributes, add comments on va_list forms, tweak
> commit message to mention upcoming qmp cleanups
> v2: several comment tweaks, explain why qmp() can't be marked
> ---
>  tests/libqtest.h | 40 +++++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 38bc1e9953..82e89b4d4f 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -50,7 +50,8 @@ void qtest_quit(QTestState *s);
>  /**
>   * qtest_qmp_discard_response:
>   * @s: #QTestState instance to operate on.
> - * @fmt...: QMP message to send to qemu
> + * @fmt...: QMP message to send to qemu; formats arguments through
> + * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
>   *
>   * Sends a QMP message to QEMU and consumes the response.
>   */
> @@ -59,7 +60,8 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
>  /**
>   * qtest_qmp:
>   * @s: #QTestState instance to operate on.
> - * @fmt...: QMP message to send to qemu
> + * @fmt...: QMP message to send to qemu; formats arguments through
> + * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
>   *
>   * Sends a QMP message to QEMU and returns the response.
>   */
> @@ -68,7 +70,8 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
>  /**
>   * qtest_async_qmp:
>   * @s: #QTestState instance to operate on.
> - * @fmt...: QMP message to send to qemu
> + * @fmt...: QMP message to send to qemu; formats arguments through
> + * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
>   *
>   * Sends a QMP message to QEMU and leaves the response in the stream.
>   */
> @@ -77,7 +80,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 '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
>   * @ap: QMP message arguments
>   *
>   * Sends a QMP message to QEMU and consumes the response.
> @@ -87,7 +91,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 '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
>   * @ap: QMP message arguments
>   *
>   * Sends a QMP message to QEMU and returns the response.
> @@ -97,7 +102,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 '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
>   * @ap: QMP message arguments
>   *
>   * Sends a QMP message to QEMU and leaves the response in the stream.
> @@ -134,19 +140,19 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
>  /**
>   * qtest_hmp:
>   * @s: #QTestState instance to operate on.
> - * @fmt...: HMP command to send to QEMU
> + * @fmt...: HMP command to send to QEMU, formats arguments like vsprintf().

Like sprintf().

>   *
>   * Send HMP command to QEMU via QMP's human-monitor-command.
>   * QMP events are discarded.
>   *
>   * Returns: the command's output.  The caller should g_free() it.
>   */
> -char *qtest_hmp(QTestState *s, const char *fmt, ...);
> +char *qtest_hmp(QTestState *s, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
>
>  /**
>   * qtest_hmpv:
>   * @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.
> @@ -154,7 +160,8 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...);
>   *
>   * Returns: the command's output.  The caller should g_free() it.
>   */
> -char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap);
> +char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap)
> +    GCC_FMT_ATTR(2, 0);
>
>  /**
>   * qtest_get_irq:
> @@ -535,7 +542,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 '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
>   *
>   * Sends a QMP message to QEMU and returns the response.
>   */
> @@ -543,7 +551,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 '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
>   *
>   * Sends a QMP message to QEMU and leaves the response in the stream.
>   */
> @@ -551,7 +560,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 '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
>   *
>   * Sends a QMP message to QEMU and consumes the response.
>   */
> @@ -592,13 +602,13 @@ static inline QDict *qmp_eventwait_ref(const char *event)
>
>  /**
>   * hmp:
> - * @fmt...: HMP command to send to QEMU
> + * @fmt...: HMP command to send to QEMU, formats arguments like vsprintf().
>   *
>   * Send HMP command to QEMU via QMP's human-monitor-command.
>   *
>   * Returns: the command's output.  The caller should g_free() it.
>   */
> -char *hmp(const char *fmt, ...);
> +char *hmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>
>  /**
>   * get_irq:

With the comment fixed, and preferably with an improved commit message:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v3 07/12] qtest: Add a new helper qmp_cmd() and friends

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> When interpolating a QMP command from the testsuite, we have
> a very common pattern that the command to be executed is a
> string constant, while the arguments may need to be crafted
> from qobject_from_jsonf() or even by hand.  Make it easier to
> craft the arguments without having to save all the interpolation
> to the final qmp() call, by adding new helper functions; the
> function takes QObject instead of QDict in order to reduce the
> number of conversions between the two.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/libqtest.h | 31 +++++++++++++++++++++++++++++++
>  tests/libqtest.c | 25 +++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
>
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 82e89b4d4f..26930fed4d 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -68,6 +68,17 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
>  QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
>
>  /**
> + * qtest_qmp_cmd:
> + * @s: #QTestState instance to operate on.
> + * @cmd: Command name to send
> + * @args: Arguments to transfer to the command, or NULL.

Do we really need "or NULL"?

> + *
> + * Sends a QMP message to QEMU and returns the response. Calling this will
> + * reduce the reference count of @args.

Suggest "decrement".  Same for the wrappers below.

> + */
> +QDict *qtest_qmp_cmd(QTestState *s, const char *cmd, QObject *args);
> +
> +/**
>   * qtest_async_qmp:
>   * @s: #QTestState instance to operate on.
>   * @fmt...: QMP message to send to qemu; formats arguments through
> @@ -550,6 +561,16 @@ static inline void qtest_end(void)
>  QDict *qmp(const char *fmt, ...);
>
>  /**
> + * qmp_cmd:
> + * @cmd: Command name to send
> + * @args: Arguments to transfer to the command, or NULL.
> + *
> + * Sends a QMP message to QEMU and returns the response. Calling this will
> + * reduce the reference count of @args.
> + */
> +QDict *qmp_cmd(const char *cmd, QObject *args);
> +
> +/**
>   * qmp_async:
>   * @fmt...: QMP message to send to qemu; formats arguments through
>   * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf])').
> @@ -568,6 +589,16 @@ void qmp_async(const char *fmt, ...);
>  void qmp_discard_response(const char *fmt, ...);
>
>  /**
> + * qmp_cmd_discard_response:
> + * @cmd: Command name to send
> + * @args: Arguments to transfer to the command, or NULL.
> + *
> + * Sends a QMP message to QEMU and consumes the response. Calling this will
> + * reduce the reference count of @args.
> + */
> +void qmp_cmd_discard_response(const char *cmd, QObject *args);
> +
> +/**
>   * qmp_receive:
>   *
>   * Reads a QMP message from QEMU and returns the response.
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 4a5492a603..1b57cedca1 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -540,6 +540,17 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...)
>      return response;
>  }
>
> +QDict *qtest_qmp_cmd(QTestState *s, const char *cmd, QObject *args)
> +{
> +    QDict *dict = qdict_new();
> +
> +    qdict_put_str(dict, "execute", cmd);
> +    if (args) {
> +        qdict_put_obj(dict, "arguments", args);
> +    }
> +    return qtest_qmp(s, "%p", QOBJECT(dict));

This function should be a one-liner:

       return qtest_qmp(s, "{'execute': %s, 'arguments': %p }", cmd, args);

A bit more complicated if we need to support null @args.

The less we clown around with qdict, the happier I am.  There's no
manual qdict building anywhere else in this file.

> +}
> +
>  void qtest_async_qmp(QTestState *s, const char *fmt, ...)
>  {
>      va_list ap;
> @@ -925,6 +936,11 @@ QDict *qmp(const char *fmt, ...)
>      return response;
>  }
>
> +QDict *qmp_cmd(const char *cmd, QObject *args)
> +{
> +    return qtest_qmp_cmd(global_qtest, cmd, args);
> +}
> +
>  void qmp_async(const char *fmt, ...)
>  {
>      va_list ap;
> @@ -942,6 +958,15 @@ void qmp_discard_response(const char *fmt, ...)
>      qtest_qmpv_discard_response(global_qtest, fmt, ap);
>      va_end(ap);
>  }
> +
> +void qmp_cmd_discard_response(const char *cmd, QObject *args)
> +{
> +    QDict *response;
> +
> +    response = qmp_cmd(cmd, args);
> +    QDECREF(response);
> +}
> +
>  char *hmp(const char *fmt, ...)
>  {
>      va_list ap;

Re: [Qemu-devel] [PATCH v3 07/12] qtest: Add a new helper qmp_cmd() and friends

[Eric Blake]

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

On 07/28/2017 01:45 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> When interpolating a QMP command from the testsuite, we have
>> a very common pattern that the command to be executed is a
>> string constant, while the arguments may need to be crafted
>> from qobject_from_jsonf() or even by hand.  Make it easier to
>> craft the arguments without having to save all the interpolation
>> to the final qmp() call, by adding new helper functions; the
>> function takes QObject instead of QDict in order to reduce the
>> number of conversions between the two.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  tests/libqtest.h | 31 +++++++++++++++++++++++++++++++
>>  tests/libqtest.c | 25 +++++++++++++++++++++++++
>>  2 files changed, 56 insertions(+)
>>
>> diff --git a/tests/libqtest.h b/tests/libqtest.h
>> index 82e89b4d4f..26930fed4d 100644
>> --- a/tests/libqtest.h
>> +++ b/tests/libqtest.h
>> @@ -68,6 +68,17 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
>>  QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
>>
>>  /**
>> + * qtest_qmp_cmd:
>> + * @s: #QTestState instance to operate on.
>> + * @cmd: Command name to send
>> + * @args: Arguments to transfer to the command, or NULL.
> 
> Do we really need "or NULL"?

It allows me to do:

qmp_cmd("cont", NULL)

instead of

qmp("{'execute':'cont'}")

> 
>> + *
>> + * Sends a QMP message to QEMU and returns the response. Calling this will
>> + * reduce the reference count of @args.
> 
> Suggest "decrement".  Same for the wrappers below.

Okay.


>> +QDict *qtest_qmp_cmd(QTestState *s, const char *cmd, QObject *args)
>> +{
>> +    QDict *dict = qdict_new();
>> +
>> +    qdict_put_str(dict, "execute", cmd);
>> +    if (args) {
>> +        qdict_put_obj(dict, "arguments", args);
>> +    }
>> +    return qtest_qmp(s, "%p", QOBJECT(dict));
> 
> This function should be a one-liner:
> 
>        return qtest_qmp(s, "{'execute': %s, 'arguments': %p }", cmd, args);
> 
> A bit more complicated if we need to support null @args.

Well, I did use NULL args.  But it looks like this is leftovers from my
original attempt to ban qobject_from_jsonf() altogether (which we have
now swung in the opposite direction of "let's keep it and make it safer
with compiler checks"), so you are indeed right that not building a
manual QDict would be a bit nicer.

{
    if (!args) {
        args = qdict_new();
    }
    return qtest_qmp(s, "{'execute': %s, 'arguments': %p }", cmd, args);
}

would work, except that it slightly changes what is passed over the
wire, albeit with no semantic change (use of 'arguments':{} is optional)

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


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

Re: [Qemu-devel] [PATCH v3 08/12] qtests: convert tests to use qmp_cmd

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Now that we have the qmp_cmd() helper, we can further simplify
> some of the tests by using it.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/device-introspect-test.c |  3 +--
>  tests/ide-test.c               |  2 +-
>  tests/libqos/libqos.c          |  5 +++--
>  tests/libqos/pci-pc.c          |  4 ++--
>  tests/libqos/usb.c             | 18 +++++++++---------
>  tests/pc-cpu-test.c            | 10 +++++-----
>  tests/postcopy-test.c          |  9 +++++----
>  tests/vhost-user-test.c        | 12 ++++++------
>  8 files changed, 32 insertions(+), 31 deletions(-)
>
> diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
> index f7162c023f..fc6d559e14 100644
> --- a/tests/device-introspect-test.c
> +++ b/tests/device-introspect-test.c
> @@ -36,8 +36,7 @@ static QList *qom_list_types(const char *implements, bool abstract)
>      if (implements) {
>          qdict_put_str(args, "implements", implements);
>      }
> -    resp = qmp("{'execute': 'qom-list-types',"
> -               " 'arguments': %p }", args);
> +    resp = qmp_cmd("qom-list-types", QOBJECT(args));

This one's a clear win.

>      g_assert(qdict_haskey(resp, "return"));
>      ret = qdict_get_qlist(resp, "return");
>      QINCREF(ret);
> diff --git a/tests/ide-test.c b/tests/ide-test.c
> index ea2657d3d1..75dc472e6a 100644
> --- a/tests/ide-test.c
> +++ b/tests/ide-test.c
> @@ -651,7 +651,7 @@ static void test_retry_flush(const char *machine)
>      qmp_eventwait("STOP");
>
>      /* Complete the command */
> -    qmp_discard_response("{'execute':'cont' }");
> +    qmp_cmd_discard_response("cont", NULL);

This one isn't.

>
>      /* Check registers */
>      data = qpci_io_readb(dev, ide_bar, reg_device);
> diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
> index 42c5315423..18844617ae 100644
> --- a/tests/libqos/libqos.c
> +++ b/tests/libqos/libqos.c
> @@ -4,6 +4,7 @@
>  #include "libqtest.h"
>  #include "libqos/libqos.h"
>  #include "libqos/pci.h"
> +#include "qapi/qmp/qjson.h"
>
>  /*** Test Setup & Teardown ***/
>
> @@ -86,7 +87,7 @@ void set_context(QOSState *s)
>
>  static QDict *qmp_execute(const char *command)
>  {
> -    return qmp("{ 'execute': %s }", command);
> +    return qmp_cmd(command, NULL);

Marginal.

>  }
>
>  void migrate(QOSState *from, QOSState *to, const char *uri)
> @@ -106,7 +107,7 @@ void migrate(QOSState *from, QOSState *to, const char *uri)
>      QDECREF(rsp);
>
>      /* Issue the migrate command. */
> -    rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
> +    rsp = qmp_cmd("migrate", qobject_from_jsonf("{ 'uri': %s }", uri));

Not a simplification.  The opposite, I'm afraid.

I could become one if qmp_cmd() worked like this:

       rsp = qmp_cmd("migrate", "{ 'uri': %s }", uri));

Even better if qmp_cmd("cont", "") just works.  Hmm, unles gcc whines
about the empty format string.

>      g_assert(qdict_haskey(rsp, "return"));
>      QDECREF(rsp);
>
[More of the same snipped...]

Re: [Qemu-devel] [PATCH v3 01/12] qobject: Accept "%"PRId64 in qobject_from_jsonf()

[Eric Blake]

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

On 07/28/2017 01:13 PM, Markus Armbruster wrote:

>> Finally, update configure to barf on any libc that uses yet
>> another form of PRId64 that we have not yet coded for, so that we
>> can once again update json-lexer.c to cater to those quirks (my
>> guess? we might see %jd next) (on the bright side, json-parser.c
>> should no longer need updates).  Yes, the only way I could find
>> to quickly learn what spelling is preferred when cross-compiling
>> is to grep a compiled binary; C does not make it easy to turn a
>> string constant into an integer constant, let alone make
>> preprocessor decisions; and even parsing '$CC -E' output is
>> fragile, since 64-bit glibc pre-processes as "l" "d" rather than
>> "ld", and newer gcc splits macro expansions across multiple lines.
>> I'm assuming that 'strings' is portable enough during configure;
>> if that assumption fails in some constrained docker environment,
>> an easy hack is to add this to configure:
>>   strings () { tr -d '\0' < "$1"; }
>>

>> +# Sanity check that we can parse PRId64 and friends in json-lexer.c
>> +# (Sadly, the "easiest" way to do this is to grep the compiled binary,
>> +# since we can't control whether the preprocessor would output "ld" vs.
>> +# "l" "d", nor even portably ensure it fits output on the same line as
>> +# a witness marker.)
>> +cat > $TMPC <<EOF
>> +#include <inttypes.h>
>> +int main(void) {
>> +    static const char findme[] = "\nUnLiKeLyToClAsH_" PRId64 "\n";
>> +    return !findme[0];
>> +}
>> +EOF
>> +if ! compile_prog "$CFLAGS" "$LIBS" ; then
> 
> Would compile_object suffice?
> 

Probably.  It's one less step, although I'd have to make sure that
compile_object has a sane counterpart to compile_prog's $TMPE,...

>> +    error_exit "can't determine value of PRId64"
>> +fi
>> +nl='
>> +'
>> +case $(strings $TMPE | grep ^UnLiKeLyToClAsH) in

since we have to grep something, after all.


>> +++ b/qobject/json-lexer.c
>> @@ -29,9 +29,11 @@
>>   *
>>   * '([^\\']|\\[\"'\\/bfnrt]|\\u[0-9a-fA-F]{4})*'
>>   *
>> - * Extension for vararg handling in JSON construction:
>> + * Extension for vararg handling in JSON construction [this lexer
>> + * actually accepts multiple forms of PRId64, but parse_escape() later
>> + * filters to only parse the current platform's spelling]:
> 
> I'd stick to (parenthesis) instead of [square brackets] here.

Sure.

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


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

Re: [Qemu-devel] [PATCH v3 03/12] qtest: Document calling conventions

[Eric Blake]

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

On 07/28/2017 01:32 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> We have two flavors of vararg usage in qtest; make it clear that
>> qmp() has different semantics than hmp(), and let the compiler
>> enforce that hmp() is used correctly. However, qmp() (and friends)
>> only accept a subset of printf flags look-alikes (namely, those
>> that our JSON parser understands), and what is worse, qmp("true")
>> (the JSON keyword 'true') is different from qmp("%s", "true")
>> (the JSON string '"true"'), and we have some intermediate cleanup
>> patches to do before we can mark those as printf-like.
> 
> It's not "worse", it's just different :)
> 
> Suggest:
> 
>   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 will 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.

"Mikey likes it" (no idea if that pop culture reference from my
childhood has broader range than the US)

>> @@ -134,19 +140,19 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
>>  /**
>>   * qtest_hmp:
>>   * @s: #QTestState instance to operate on.
>> - * @fmt...: HMP command to send to QEMU
>> + * @fmt...: HMP command to send to QEMU, formats arguments like vsprintf().
> 
> Like sprintf().

Hmm, you asked me to use vsprintf on the last one.  Oh, I finally see -
you're trying to get me to match: vsprintf if it is 'va_list', sprintf
if it is '...'.  Yeah, that makes sense.


> With the comment fixed, and preferably with an improved commit message:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks for the reviews and suggestions.

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


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

Re: [Qemu-devel] [PATCH v3 08/12] qtests: convert tests to use qmp_cmd

[Eric Blake]

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

On 07/28/2017 01:53 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Now that we have the qmp_cmd() helper, we can further simplify
>> some of the tests by using it.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---

>>      }
>> -    resp = qmp("{'execute': 'qom-list-types',"
>> -               " 'arguments': %p }", args);
>> +    resp = qmp_cmd("qom-list-types", QOBJECT(args));
> 
> This one's a clear win.

Well, it'd be even NICER if I could pass QDict instead of QObject around.


>> +++ b/tests/ide-test.c
>> @@ -651,7 +651,7 @@ static void test_retry_flush(const char *machine)
>>      qmp_eventwait("STOP");
>>
>>      /* Complete the command */
>> -    qmp_discard_response("{'execute':'cont' }");
>> +    qmp_cmd_discard_response("cont", NULL);
> 
> This one isn't.

Fair enough.

>> @@ -86,7 +87,7 @@ void set_context(QOSState *s)
>>
>>  static QDict *qmp_execute(const char *command)
>>  {
>> -    return qmp("{ 'execute': %s }", command);
>> +    return qmp_cmd(command, NULL);
> 
> Marginal.
> 
>>  }
>>
>>  void migrate(QOSState *from, QOSState *to, const char *uri)
>> @@ -106,7 +107,7 @@ void migrate(QOSState *from, QOSState *to, const char *uri)
>>      QDECREF(rsp);
>>
>>      /* Issue the migrate command. */
>> -    rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
>> +    rsp = qmp_cmd("migrate", qobject_from_jsonf("{ 'uri': %s }", uri));
> 
> Not a simplification.  The opposite, I'm afraid.
> 
> I could become one if qmp_cmd() worked like this:
> 
>        rsp = qmp_cmd("migrate", "{ 'uri': %s }", uri));

Oh, nice. But it makes qmp_cmd become varargs, at which point...

> 
> Even better if qmp_cmd("cont", "") just works.  Hmm, unles gcc whines
> about the empty format string.

yeah, we'd have to figure out a way to shut up gcc when no arguments are
wanted.  Here's a compromise that does the best for all three categories
you pointed out above:

qmp_cmd("cont");
qmp_cmd_args("migrate", "{ 'uri': %s  }", uri);
qmp_cmd_dict("qom-list-types", args);

Sounds like I have a plan of attack!  Also, since I'm somewhat churning
on the stuff you did in a previous patch, should we reorder any of this
series (add the helper first, then a single fix the callers that benefit
from the helpers; instead of fixing callers twice in three patches)?

> 
>>      g_assert(qdict_haskey(rsp, "return"));
>>      QDECREF(rsp);
>>
> [More of the same snipped...]
> 

And, as I said in the cover letter, there's probably a lot more we could
touch in the testsuite if we like the new pattern.

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


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

Re: [Qemu-devel] [PATCH v3 08/12] qtests: convert tests to use qmp_cmd

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/28/2017 01:53 PM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> Now that we have the qmp_cmd() helper, we can further simplify
>>> some of the tests by using it.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
>
>>>      }
>>> -    resp = qmp("{'execute': 'qom-list-types',"
>>> -               " 'arguments': %p }", args);
>>> +    resp = qmp_cmd("qom-list-types", QOBJECT(args));
>> 
>> This one's a clear win.
>
> Well, it'd be even NICER if I could pass QDict instead of QObject around.
>
>
>>> +++ b/tests/ide-test.c
>>> @@ -651,7 +651,7 @@ static void test_retry_flush(const char *machine)
>>>      qmp_eventwait("STOP");
>>>
>>>      /* Complete the command */
>>> -    qmp_discard_response("{'execute':'cont' }");
>>> +    qmp_cmd_discard_response("cont", NULL);
>> 
>> This one isn't.
>
> Fair enough.
>
>>> @@ -86,7 +87,7 @@ void set_context(QOSState *s)
>>>
>>>  static QDict *qmp_execute(const char *command)
>>>  {
>>> -    return qmp("{ 'execute': %s }", command);
>>> +    return qmp_cmd(command, NULL);
>> 
>> Marginal.
>> 
>>>  }
>>>
>>>  void migrate(QOSState *from, QOSState *to, const char *uri)
>>> @@ -106,7 +107,7 @@ void migrate(QOSState *from, QOSState *to, const char *uri)
>>>      QDECREF(rsp);
>>>
>>>      /* Issue the migrate command. */
>>> -    rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
>>> +    rsp = qmp_cmd("migrate", qobject_from_jsonf("{ 'uri': %s }", uri));
>> 
>> Not a simplification.  The opposite, I'm afraid.
>> 
>> I could become one if qmp_cmd() worked like this:
>> 
>>        rsp = qmp_cmd("migrate", "{ 'uri': %s }", uri));
>
> Oh, nice. But it makes qmp_cmd become varargs, at which point...
>
>> 
>> Even better if qmp_cmd("cont", "") just works.  Hmm, unles gcc whines
>> about the empty format string.
>
> yeah, we'd have to figure out a way to shut up gcc when no arguments are
> wanted.  Here's a compromise that does the best for all three categories
> you pointed out above:
>
> qmp_cmd("cont");
> qmp_cmd_args("migrate", "{ 'uri': %s  }", uri);
> qmp_cmd_dict("qom-list-types", args);
>
> Sounds like I have a plan of attack!  Also, since I'm somewhat churning
> on the stuff you did in a previous patch, should we reorder any of this
> series (add the helper first, then a single fix the callers that benefit
> from the helpers; instead of fixing callers twice in three patches)?

If easier review and cleaner history can justify the effort to rework.
Your call.

>>>      g_assert(qdict_haskey(rsp, "return"));
>>>      QDECREF(rsp);
>>>
>> [More of the same snipped...]
>> 
>
> And, as I said in the cover letter, there's probably a lot more we could
> touch in the testsuite if we like the new pattern.

One step at a time.

Re: [Qemu-devel] [PATCH v3 09/12] tests/libqos/pci: Clean up string interpolation into QMP input

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/28/2017 11:35 AM, Eric Blake wrote:
>>>> +    QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
>>>> +                                             tmpshm);
>>>
>
>> Passing '%s' through qobject_from_jsonf() is generally wrong (it would
>> produce ''...'' instead of the intended '...').
>> 
>> Looks like something to fix on the next round.
>> 
>
> What's scary is that the testsuite didn't start failing.  That's not
> good; we'll want to figure out why the bug didn't impact the test.

Good idea.

Re: [Qemu-devel] [PATCH v3 07/12] qtest: Add a new helper qmp_cmd() and friends

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/28/2017 08:06 AM, Eric Blake wrote:
>> On 07/28/2017 07:57 AM, Stefan Hajnoczi wrote:
>>> On Tue, Jul 25, 2017 at 04:15:18PM -0500, Eric Blake wrote:
>>>> +QDict *qtest_qmp_cmd(QTestState *s, const char *cmd, QObject *args)
>>>> +{
>>>> +    QDict *dict = qdict_new();
>>>> +
>>>> +    qdict_put_str(dict, "execute", cmd);
>>>> +    if (args) {
>>>> +        qdict_put_obj(dict, "arguments", args);
>>>> +    }
>>>> +    return qtest_qmp(s, "%p", QOBJECT(dict));
>>>> +}
>>>
>>> qtest_qmp(s, "%p", QOBJECT(dict)) takes ownership of dict?
>> 
>> Hmm, I documented that for qtest_qmp_cmd(), but you have a good
>> question.  I need to double-check that it is true for
>> qobject_from_jsonf("%p", obj), but my recollection is that yes, wrapping
>> one object inside the other means freeing the outer object also reclaims
>> the inner (that is, qobject_from_jsonf wasn't increasing refcount).  At
>> any rate, I'll probably have to submit a followup patch (either to fix
>> the leak if I'm wrong, or to improve the documentation about %p
>> reference management if I'm right).
>
> $ valgrind --leak-check=full ./tests/check-qjson
> ...
> ==10596== LEAK SUMMARY:
> ==10596==    definitely lost: 0 bytes in 0 blocks
> ...
> $ grep -C5 %p tests/check-qjson.c
>             {}}));
>
>     embedded_obj = qobject_from_json("[32, 42]", &error_abort);
>     g_assert(embedded_obj != NULL);
>
>     obj = qobject_from_jsonf("[%d, 2, %p]", 1, embedded_obj);
>     g_assert(compare_litqobj_to_qobj(&decoded, obj) == 1);
>
>     qobject_decref(obj);
> }
>
> So given the clean bill of health from valgrind, we definitely DO turn
> over responsibility for freeing on object to its new wrapper, once it is
> passed through %p.  Documentation could be improved to make that clear.
>
> Eww, what happens if qobject_from_jsonf() can fail halfway through? If
> you mix %p with other stuff, how do you know on failure whether the
> reference was taken or not yet reached?  Maybe the testsuite needs an
> enhancement.

What we actually need is qobject_from_jsonf() behaving sanely failure!
Either it takes over all references (just like on success) or none.

It delegates the part of its job that's relevant here to json-parser.c.
Looks like parse_escape() takes over the reference greedily.  If parsing
fails, it's released along with all the refernces to objects the parser
built.

One way to do "none": delay taking over the reference until parsing
succeeded.  Increment it in parse_escape(), decrement it in
json_parser_parse_err().  Either hold %p arguments in JSONParserContext,
so json_parser_parse_err() can find them easily, or iterate over @tokens
and @ap again.

One way to do "all": on error, have json_parser_parse_err() iterate over
remaining @tokens and @ap to consume references.

Re: [Qemu-devel] [PATCH v3 03/12] qtest: Document calling conventions

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/28/2017 01:32 PM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> We have two flavors of vararg usage in qtest; make it clear that
>>> qmp() has different semantics than hmp(), and let the compiler
>>> enforce that hmp() is used correctly. However, qmp() (and friends)
>>> only accept a subset of printf flags look-alikes (namely, those
>>> that our JSON parser understands), and what is worse, qmp("true")
>>> (the JSON keyword 'true') is different from qmp("%s", "true")
>>> (the JSON string '"true"'), and we have some intermediate cleanup
>>> patches to do before we can mark those as printf-like.
>> 
>> It's not "worse", it's just different :)
>> 
>> Suggest:
>> 
>>   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 will 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.
>
> "Mikey likes it" (no idea if that pop culture reference from my
> childhood has broader range than the US)

'fraid I'm out of range :)

>>> @@ -134,19 +140,19 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
>>>  /**
>>>   * qtest_hmp:
>>>   * @s: #QTestState instance to operate on.
>>> - * @fmt...: HMP command to send to QEMU
>>> + * @fmt...: HMP command to send to QEMU, formats arguments like vsprintf().
>> 
>> Like sprintf().
>
> Hmm, you asked me to use vsprintf on the last one.  Oh, I finally see -
> you're trying to get me to match: vsprintf if it is 'va_list', sprintf
> if it is '...'.  Yeah, that makes sense.

Correct.  I should've explained that from the start.

>> With the comment fixed, and preferably with an improved commit message:
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Thanks for the reviews and suggestions.

You're welcome!

Re: [Qemu-devel] [PATCH v3 03/12] qtest: Document calling conventions

[Eric Blake]

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

On 07/31/2017 03:20 AM, Markus Armbruster wrote:
>>> It's not "worse", it's just different :)
>>>
>>> Suggest:
<snip>

>>
>> "Mikey likes it" (no idea if that pop culture reference from my
>> childhood has broader range than the US)
> 
> 'fraid I'm out of range :)

It's not fair of me to leave you hanging.
https://en.wikipedia.org/wiki/Little_Mikey

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


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

Re: [Qemu-devel] [PATCH v3 07/12] qtest: Add a new helper qmp_cmd() and friends

[Eric Blake]

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

On 07/31/2017 03:16 AM, Markus Armbruster wrote:

>>>> qtest_qmp(s, "%p", QOBJECT(dict)) takes ownership of dict?
>>>

>> So given the clean bill of health from valgrind, we definitely DO turn
>> over responsibility for freeing on object to its new wrapper, once it is
>> passed through %p.  Documentation could be improved to make that clear.
>>
>> Eww, what happens if qobject_from_jsonf() can fail halfway through? If
>> you mix %p with other stuff, how do you know on failure whether the
>> reference was taken or not yet reached?  Maybe the testsuite needs an
>> enhancement.
> 
> What we actually need is qobject_from_jsonf() behaving sanely failure!

Agreed.  Looks like my v4 series will have another up-front patch to
clean things up.

> Either it takes over all references (just like on success) or none.

Taking over references is probably more convenient (we usually succeed,
where it really is easier to just free the new owner recursively, so
always reducing a reference even on failure is something that can easily
be reasoned about - and if the caller wants to hang on, they can
increase refcount).

> 
> It delegates the part of its job that's relevant here to json-parser.c.
> Looks like parse_escape() takes over the reference greedily.  If parsing
> fails, it's released along with all the refernces to objects the parser
> built.
> 
> One way to do "none": delay taking over the reference until parsing
> succeeded.  Increment it in parse_escape(), decrement it in
> json_parser_parse_err().  Either hold %p arguments in JSONParserContext,
> so json_parser_parse_err() can find them easily, or iterate over @tokens
> and @ap again.
> 
> One way to do "all": on error, have json_parser_parse_err() iterate over
> remaining @tokens and @ap to consume references.

I'm leaning towards "all".  And of course, the very first thing is a
patch to the testsuite with two %p separated by something that causes a
mid-string parse error (but at the same time does not trip up -Wformat).

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


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

Re: [Qemu-devel] [PATCH v3 09/12] tests/libqos/pci: Clean up string interpolation into QMP input

[Eric Blake]

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

On 07/31/2017 02:29 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 07/28/2017 11:35 AM, Eric Blake wrote:
>>>>> +    QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
>>>>> +                                             tmpshm);
>>>>
>>
>>> Passing '%s' through qobject_from_jsonf() is generally wrong (it would
>>> produce ''...'' instead of the intended '...').
>>>
>>> Looks like something to fix on the next round.
>>>
>>
>> What's scary is that the testsuite didn't start failing.  That's not
>> good; we'll want to figure out why the bug didn't impact the test.
> 
> Good idea.

The intended result: the created QDict would include
"shm":"/qtest-11387-3641365368" (or comparable string).  The actual
result: the created QDict includes "shm":"%s" - a literal 2-character
string, because the lexer does not do interpolation inside strings.  And
apparently, naming your ivshmem shared memory a literal "%s" (rather
than a name beginning with "/") is not a semantic error, so the test passes.

In other words, qobject_from_jsonf() does NOT do % interpolation of
anything embedded inside '', and basically blindly ignores the tmpshm
vararg.  It would be neat if we could make qobject_from_jsonf() detect a
mismatch in varargs, even though we don't (can't) require a NULL
sentinel (we're limited to fitting in with -Wformat paradigms already).
So while we can't flag it at compile time, I do think we can flag it at
runtime: if, in qobject_from_jsonf() (but NOT in qobject_from_json()),
we reject any JSON string from the lexer that contains "%" but not "%%",
we will have caught all the places where gcc paired a % sequence with a
vararg parameter even though our lexer did not make the same pairing.
If we WANT a literal % in a string (rare, probably only in the
testsuite), we write it as %% and then qobject_from_jsonf() interpolates
it.  Then, since bare % is NOT valid JSON, we don't have to enhance the
lexer to recognize anything new; our changes are limited to
documentation and the vararg parser.  It still means we only get a
runtime, rather than a compile-time, detection of misuse of % passed to
the format argument, but it should be an easy enough proof of concept
that such a runtime failure would have been enough to make ivshmem-test
fail and flag the error during 'make check' rather than escaping review.

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


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

Re: [Qemu-devel] [PATCH v3 07/12] qtest: Add a new helper qmp_cmd() and friends

[Eric Blake]

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

On 07/31/2017 07:34 AM, Eric Blake wrote:
> On 07/31/2017 03:16 AM, Markus Armbruster wrote:
> 
>>>>> qtest_qmp(s, "%p", QOBJECT(dict)) takes ownership of dict?
>>>>
> 
>>> So given the clean bill of health from valgrind, we definitely DO turn
>>> over responsibility for freeing on object to its new wrapper, once it is
>>> passed through %p.  Documentation could be improved to make that clear.
>>>
>>> Eww, what happens if qobject_from_jsonf() can fail halfway through?

Hmm. What if we assert that qobject_from_jsonf() can never fail halfway
through?  Given my research on the other thread, gcc -Wformat will NOT
flag "['%s', %p]",str,obj as a mismatch, although our current code will
try to associate %p with str and probably die horribly when
dereferencing char* as QObject* (and if it does NOT die, we don't even
know that 'obj' was passed as a parameter).  Since the primary usage of
qobject_from_jsonf() is the testsuite, an assertion failure (forcing all
clients to use the interface correctly) is probably simpler than even
trying to have to worry about cleanup after partial failure (regardless
of whether we like the 'none' or 'all' approach).


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


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

Re: [Qemu-devel] [PATCH v3 09/12] tests/libqos/pci: Clean up string interpolation into QMP input

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/31/2017 02:29 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 07/28/2017 11:35 AM, Eric Blake wrote:
>>>>>> +    QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
>>>>>> +                                             tmpshm);
>>>>>
>>>
>>>> Passing '%s' through qobject_from_jsonf() is generally wrong (it would
>>>> produce ''...'' instead of the intended '...').
>>>>
>>>> Looks like something to fix on the next round.
>>>>
>>>
>>> What's scary is that the testsuite didn't start failing.  That's not
>>> good; we'll want to figure out why the bug didn't impact the test.
>> 
>> Good idea.
>
> The intended result: the created QDict would include
> "shm":"/qtest-11387-3641365368" (or comparable string).  The actual
> result: the created QDict includes "shm":"%s" - a literal 2-character
> string, because the lexer does not do interpolation inside strings.  And
> apparently, naming your ivshmem shared memory a literal "%s" (rather
> than a name beginning with "/") is not a semantic error, so the test passes.
>
> In other words, qobject_from_jsonf() does NOT do % interpolation of
> anything embedded inside '', and basically blindly ignores the tmpshm
> vararg.

Actually, it recognizes conversion specifications in lexer state
IN_START.  The only other states where it accepts them are IN_DQ_STRING
and IN_SQ_STRING.  It chokes on '%' in all other states.  Therefore,
conversion specifications can be be silently ignored only in JSON
strings.

>          It would be neat if we could make qobject_from_jsonf() detect a
> mismatch in varargs, even though we don't (can't) require a NULL
> sentinel (we're limited to fitting in with -Wformat paradigms already).
> So while we can't flag it at compile time, I do think we can flag it at
> runtime: if, in qobject_from_jsonf() (but NOT in qobject_from_json()),
> we reject any JSON string from the lexer that contains "%" but not "%%",
> we will have caught all the places where gcc paired a % sequence with a
> vararg parameter even though our lexer did not make the same pairing.
> If we WANT a literal % in a string (rare, probably only in the
> testsuite), we write it as %% and then qobject_from_jsonf() interpolates
> it.

If we make it do that.

Note that the JSON parser then recognizing one subset of printf
conversion specifiers as values (%s, %ld, %lld, ...) and another subset
within strings (just %%).  Not exactly elegant, but it works.

We could let it accept more (all?) conversion specifiers within strings,
but that would bring back the temptation to code JSON injection flaws.

>      Then, since bare % is NOT valid JSON, we don't have to enhance the
> lexer to recognize anything new; our changes are limited to
> documentation and the vararg parser.  It still means we only get a
> runtime, rather than a compile-time, detection of misuse of % passed to
> the format argument, but it should be an easy enough proof of concept
> that such a runtime failure would have been enough to make ivshmem-test
> fail and flag the error during 'make check' rather than escaping review.

Re: [Qemu-devel] [PATCH v3 07/12] qtest: Add a new helper qmp_cmd() and friends

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/31/2017 07:34 AM, Eric Blake wrote:
>> On 07/31/2017 03:16 AM, Markus Armbruster wrote:
>> 
>>>>>> qtest_qmp(s, "%p", QOBJECT(dict)) takes ownership of dict?
>>>>>
>> 
>>>> So given the clean bill of health from valgrind, we definitely DO turn
>>>> over responsibility for freeing on object to its new wrapper, once it is
>>>> passed through %p.  Documentation could be improved to make that clear.
>>>>
>>>> Eww, what happens if qobject_from_jsonf() can fail halfway through?
>
> Hmm. What if we assert that qobject_from_jsonf() can never fail halfway
> through?  Given my research on the other thread, gcc -Wformat will NOT
> flag "['%s', %p]",str,obj as a mismatch, although our current code will
> try to associate %p with str and probably die horribly when
> dereferencing char* as QObject* (and if it does NOT die, we don't even
> know that 'obj' was passed as a parameter).  Since the primary usage of
> qobject_from_jsonf() is the testsuite, an assertion failure (forcing all
> clients to use the interface correctly) is probably simpler than even
> trying to have to worry about cleanup after partial failure (regardless
> of whether we like the 'none' or 'all' approach).

I don't buy the "primarily for the testsuite argument".  It's for
whatever finds it useful.  Safer than formatting the JSON text (no JSON
injection accidents), much easier to read than building a QObject to
pass to the JSON output visitor.

I'd be willing to buy a "passing syntactically incorrect JSON to
qobject_from_jsonf() is a programming error" argument.  Assertion
failure is the appropriate way to deal with programming errors.  Needs
to be spelled out in function contracts, of course, starting with
json_parser_parse_err(), which (scandalously!) has none.

Re: [Qemu-devel] [PATCH v3 07/12] qtest: Add a new helper qmp_cmd() and friends

[Eric Blake]

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

On 08/01/2017 12:48 AM, Markus Armbruster wrote:

>> Hmm. What if we assert that qobject_from_jsonf() can never fail halfway
>> through?


> I don't buy the "primarily for the testsuite argument".  It's for
> whatever finds it useful.  Safer than formatting the JSON text (no JSON
> injection accidents), much easier to read than building a QObject to
> pass to the JSON output visitor.
> 
> I'd be willing to buy a "passing syntactically incorrect JSON to
> qobject_from_jsonf() is a programming error" argument.  Assertion
> failure is the appropriate way to deal with programming errors.  Needs
> to be spelled out in function contracts, of course, starting with
> json_parser_parse_err(), which (scandalously!) has none.

In fact, qobject_from_jsonf() _does_ assert if it is handed ill-formed
input.  Only qobject_from_jsonv() does not.  That will be fixed in my
next series, along with some documentation comments.

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


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