[Qemu-devel] [PATCH v5 00/13] Preliminary libqtest cleanups

[Qemu-devel] [PATCH v5 00/13] Preliminary libqtest cleanups

[Eric Blake]

Markus gave some good advice on my 'v4: Clean up around qmp() and hmp()'
series [1].  Among other things, we agreed that if I'm going to get rid
of the qtest_* layer of function wrappers, I should do it all the way
(rather than just on the qmp() functions), and up front.  So that's what
this series does - focus on the stuff that should be easier to commit
up front, while I still play around with the hairier stuff related to
improving the qmp() interfaces.

I'm naming this v5, even though most of it is new content (patch 1 was
posted independently [2], and patch 13 was 7/22 of v4).  The overall
diffstat is rather fun.

[1] https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg00595.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg00385.html

Eric Blake (13):
  test-qga: Kill broken and dead QGA_TEST_SIDE_EFFECTING code
  qtest: Don't perform side effects inside assertion
  libqtest: Remove dead qtest_instances variable
  libqtest: Let socket_send() compute length
  libqtest: Use qemu_strtoul()
  libqtest: Topologically sort functions
  libqtest: Inline qtest_query_target_endianness()
  tests: Rely more on global_qtest
  libqtest: Shorten a couple more qtest_* functions
  libqtest: Drop qtest_init() and qtest_qmp_discard_response()
  libqtest: Drop many static inline qtest_ wrappers
  libqtest: Use global_qtest in qtest_sendf() and qtest_rsp()
  numa-test: Use hmp()

 tests/libqos/virtio.h    |   2 +-
 tests/libqtest.h         | 642 +++++++++--------------------------------------
 tests/libqtest.c         | 496 ++++++++++++++++++------------------
 qtest.c                  |  80 ++++--
 tests/fdc-test.c         |   2 +-
 tests/ide-test.c         |  10 +-
 tests/ipmi-bt-test.c     |   2 +-
 tests/ipmi-kcs-test.c    |   2 +-
 tests/libqos/libqos-pc.c |   2 +-
 tests/libqos/rtas.c      |   3 +-
 tests/numa-test.c        |  21 +-
 tests/postcopy-test.c    |  16 +-
 tests/qmp-test.c         |   2 +-
 tests/rtc-test.c         |   9 +-
 tests/tco-test.c         |   5 +-
 tests/test-qga.c         |  90 -------
 tests/vhost-user-test.c  |   3 +-
 tests/wdt_ib700-test.c   |  30 ++-
 18 files changed, 467 insertions(+), 950 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH v5 01/13] test-qga: Kill broken and dead QGA_TEST_SIDE_EFFECTING code

[Eric Blake]

Back when the test was introduced, in commit 62c39b307, the
test was set up to run qemu-ga directly on the host performing
the test, and defaults to limiting itself to safe commands.  At
the time, it was envisioned that setting QGA_TEST_SIDE_EFFECTING
in the environment could cover a few more commands, while noting
the potential danger of those side effects running in the host.

But this has NEVER been tested: if you enable the environment
variable, the test WILL fail.  One obvious reason: if you are not
running as root, you'll probably get a permission failure when
trying to freeze the file systems, or when changing system time.
Less obvious: if you run the test as root (wow, you're brave), you
could end up hanging if the test tries to log things to a
temporarily frozen filesystem.  But the cutest reason of all: if
you get past the above hurdles, the test uses invalid JSON in
test_qga_fstrim() (missing '' around the dictionary key 'minimum'),
and will thus fail an assertion in qmp_fd().

Rather than leave this untested time-bomb in place, rip it out.
Hopefully, as originally envisioned, we can find an opportunity
to test an actual sandboxed guest where the guest-agent has
full permissions and will not unduly affect the host running
the test - if so, 'git revert' can be used if desired, for
salvaging any useful parts of this attempt.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/test-qga.c | 90 --------------------------------------------------------
 1 file changed, 90 deletions(-)

diff --git a/tests/test-qga.c b/tests/test-qga.c
index 06783e7585..fd6bc7690f 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -642,65 +642,6 @@ static void test_qga_get_time(gconstpointer fix)
     QDECREF(ret);
 }

-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'}");
-    g_assert_nonnull(ret);
-    qmp_assert_no_error(ret);
-    current = qdict_get_int(ret, "return");
-    g_assert_cmpint(current, >, 0);
-    QDECREF(ret);
-
-    /* set some old time */
-    ret = qmp_fd(fixture->fd, "{'execute': 'guest-set-time',"
-                 " 'arguments': { 'time': 1000 } }");
-    g_assert_nonnull(ret);
-    qmp_assert_no_error(ret);
-    QDECREF(ret);
-
-    /* check old time */
-    ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-time'}");
-    g_assert_nonnull(ret);
-    qmp_assert_no_error(ret);
-    time = qdict_get_int(ret, "return");
-    g_assert_cmpint(time / 1000, <, G_USEC_PER_SEC * 10);
-    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);
-    g_assert_nonnull(ret);
-    qmp_assert_no_error(ret);
-    QDECREF(ret);
-}
-
-static void test_qga_fstrim(gconstpointer fix)
-{
-    const TestFixture *fixture = fix;
-    QDict *ret;
-    QList *list;
-    const QListEntry *entry;
-
-    ret = qmp_fd(fixture->fd, "{'execute': 'guest-fstrim',"
-                 " arguments: { minimum: 4194304 } }");
-    g_assert_nonnull(ret);
-    qmp_assert_no_error(ret);
-    list = qdict_get_qlist(ret, "return");
-    entry = qlist_first(list);
-    g_assert(qdict_haskey(qobject_to_qdict(entry->value), "paths"));
-
-    QDECREF(ret);
-}
-
 static void test_qga_blacklist(gconstpointer data)
 {
     TestFixture fix;
@@ -831,30 +772,6 @@ static void test_qga_fsfreeze_status(gconstpointer fix)
     QDECREF(ret);
 }

-static void test_qga_fsfreeze_and_thaw(gconstpointer fix)
-{
-    const TestFixture *fixture = fix;
-    QDict *ret;
-    const gchar *status;
-
-    ret = qmp_fd(fixture->fd, "{'execute': 'guest-fsfreeze-freeze'}");
-    g_assert_nonnull(ret);
-    qmp_assert_no_error(ret);
-    QDECREF(ret);
-
-    ret = qmp_fd(fixture->fd, "{'execute': 'guest-fsfreeze-status'}");
-    g_assert_nonnull(ret);
-    qmp_assert_no_error(ret);
-    status = qdict_get_try_str(ret, "return");
-    g_assert_cmpstr(status, ==, "frozen");
-    QDECREF(ret);
-
-    ret = qmp_fd(fixture->fd, "{'execute': 'guest-fsfreeze-thaw'}");
-    g_assert_nonnull(ret);
-    qmp_assert_no_error(ret);
-    QDECREF(ret);
-}
-
 static void test_qga_guest_exec(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
@@ -1029,13 +946,6 @@ int main(int argc, char **argv)
     g_test_add_data_func("/qga/guest-get-osinfo", &fix,
                          test_qga_guest_get_osinfo);

-    if (g_getenv("QGA_TEST_SIDE_EFFECTING")) {
-        g_test_add_data_func("/qga/fsfreeze-and-thaw", &fix,
-                             test_qga_fsfreeze_and_thaw);
-        g_test_add_data_func("/qga/set-time", &fix, test_qga_set_time);
-        g_test_add_data_func("/qga/fstrim", &fix, test_qga_fstrim);
-    }
-
     ret = g_test_run();

     fixture_tear_down(&fix, NULL);
-- 
2.13.5

[Qemu-devel] [PATCH v5 02/13] qtest: Don't perform side effects inside assertion

[Eric Blake]

Assertions should be separate from the side effects, since in
theory, g_assert() can be disabled (in practice, we can't really
ever do that).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qtest.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 57 insertions(+), 23 deletions(-)

diff --git a/qtest.c b/qtest.c
index 88a09e9afc..cbbfb71114 100644
--- a/qtest.c
+++ b/qtest.c
@@ -332,10 +332,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
                strcmp(words[0], "outl") == 0) {
         unsigned long addr;
         unsigned long value;
+        int ret;

         g_assert(words[1] && words[2]);
-        g_assert(qemu_strtoul(words[1], NULL, 0, &addr) == 0);
-        g_assert(qemu_strtoul(words[2], NULL, 0, &value) == 0);
+        ret = qemu_strtoul(words[1], NULL, 0, &addr);
+        g_assert(ret == 0);
+        ret = qemu_strtoul(words[2], NULL, 0, &value);
+        g_assert(ret == 0);
         g_assert(addr <= 0xffff);

         if (words[0][3] == 'b') {
@@ -352,9 +355,11 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         strcmp(words[0], "inl") == 0) {
         unsigned long addr;
         uint32_t value = -1U;
+        int ret;

         g_assert(words[1]);
-        g_assert(qemu_strtoul(words[1], NULL, 0, &addr) == 0);
+        ret = qemu_strtoul(words[1], NULL, 0, &addr);
+        g_assert(ret == 0);
         g_assert(addr <= 0xffff);

         if (words[0][2] == 'b') {
@@ -372,10 +377,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
                strcmp(words[0], "writeq") == 0) {
         uint64_t addr;
         uint64_t value;
+        int ret;

         g_assert(words[1] && words[2]);
-        g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
-        g_assert(qemu_strtou64(words[2], NULL, 0, &value) == 0);
+        ret = qemu_strtou64(words[1], NULL, 0, &addr);
+        g_assert(ret == 0);
+        ret = qemu_strtou64(words[2], NULL, 0, &value);
+        g_assert(ret == 0);

         if (words[0][5] == 'b') {
             uint8_t data = value;
@@ -401,9 +409,11 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
                strcmp(words[0], "readq") == 0) {
         uint64_t addr;
         uint64_t value = UINT64_C(-1);
+        int ret;

         g_assert(words[1]);
-        g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
+        ret = qemu_strtou64(words[1], NULL, 0, &addr);
+        g_assert(ret == 0);

         if (words[0][4] == 'b') {
             uint8_t data;
@@ -427,10 +437,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         uint64_t addr, len, i;
         uint8_t *data;
         char *enc;
+        int ret;

         g_assert(words[1] && words[2]);
-        g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
-        g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
+        ret = qemu_strtou64(words[1], NULL, 0, &addr);
+        g_assert(ret == 0);
+        ret = qemu_strtou64(words[2], NULL, 0, &len);
+        g_assert(ret == 0);
         /* We'd send garbage to libqtest if len is 0 */
         g_assert(len);

@@ -451,10 +464,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         uint64_t addr, len;
         uint8_t *data;
         gchar *b64_data;
+        int ret;

         g_assert(words[1] && words[2]);
-        g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
-        g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
+        ret = qemu_strtou64(words[1], NULL, 0, &addr);
+        g_assert(ret == 0);
+        ret = qemu_strtou64(words[2], NULL, 0, &len);
+        g_assert(ret == 0);

         data = g_malloc(len);
         cpu_physical_memory_read(addr, data, len);
@@ -468,10 +484,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         uint64_t addr, len, i;
         uint8_t *data;
         size_t data_len;
+        int ret;

         g_assert(words[1] && words[2] && words[3]);
-        g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
-        g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
+        ret = qemu_strtou64(words[1], NULL, 0, &addr);
+        g_assert(ret == 0);
+        ret = qemu_strtou64(words[2], NULL, 0, &len);
+        g_assert(ret == 0);

         data_len = strlen(words[3]);
         if (data_len < 3) {
@@ -497,11 +516,15 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         uint64_t addr, len;
         uint8_t *data;
         unsigned long pattern;
+        int ret;

         g_assert(words[1] && words[2] && words[3]);
-        g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
-        g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
-        g_assert(qemu_strtoul(words[3], NULL, 0, &pattern) == 0);
+        ret = qemu_strtou64(words[1], NULL, 0, &addr);
+        g_assert(ret == 0);
+        ret = qemu_strtou64(words[2], NULL, 0, &len);
+        g_assert(ret == 0);
+        ret = qemu_strtoul(words[3], NULL, 0, &pattern);
+        g_assert(ret == 0);

         if (len) {
             data = g_malloc(len);
@@ -517,10 +540,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         uint8_t *data;
         size_t data_len;
         gsize out_len;
+        int ret;

         g_assert(words[1] && words[2] && words[3]);
-        g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
-        g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
+        ret = qemu_strtou64(words[1], NULL, 0, &addr);
+        g_assert(ret == 0);
+        ret = qemu_strtou64(words[2], NULL, 0, &len);
+        g_assert(ret == 0);

         data_len = strlen(words[3]);
         if (data_len < 3) {
@@ -551,11 +577,16 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
     } else if (strcmp(words[0], "rtas") == 0) {
         uint64_t res, args, ret;
         unsigned long nargs, nret;
+        int rc;

-        g_assert(qemu_strtoul(words[2], NULL, 0, &nargs) == 0);
-        g_assert(qemu_strtou64(words[3], NULL, 0, &args) == 0);
-        g_assert(qemu_strtoul(words[4], NULL, 0, &nret) == 0);
-        g_assert(qemu_strtou64(words[5], NULL, 0, &ret) == 0);
+        rc = qemu_strtoul(words[2], NULL, 0, &nargs);
+        g_assert(rc == 0);
+        rc = qemu_strtou64(words[3], NULL, 0, &args);
+        g_assert(rc == 0);
+        rc = qemu_strtoul(words[4], NULL, 0, &nret);
+        g_assert(rc == 0);
+        rc = qemu_strtou64(words[5], NULL, 0, &ret);
+        g_assert(rc == 0);
         res = qtest_rtas_call(words[1], nargs, args, nret, ret);

         qtest_send_prefix(chr);
@@ -565,7 +596,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         int64_t ns;

         if (words[1]) {
-            g_assert(qemu_strtoi64(words[1], NULL, 0, &ns) == 0);
+            int ret = qemu_strtoi64(words[1], NULL, 0, &ns);
+            g_assert(ret == 0);
         } else {
             ns = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
         }
@@ -575,9 +607,11 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
                     (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
     } else if (qtest_enabled() && strcmp(words[0], "clock_set") == 0) {
         int64_t ns;
+        int ret;

         g_assert(words[1]);
-        g_assert(qemu_strtoi64(words[1], NULL, 0, &ns) == 0);
+        ret = qemu_strtoi64(words[1], NULL, 0, &ns);
+        g_assert(ret == 0);
         qtest_clock_warp(ns);
         qtest_send_prefix(chr);
         qtest_sendf(chr, "OK %"PRIi64"\n",
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 02/13] qtest: Don't perform side effects inside assertion

[Philippe Mathieu-Daudé]

Hi Eric,

On 08/18/2017 06:15 PM, Eric Blake wrote:
> Assertions should be separate from the side effects, since in
> theory, g_assert() can be disabled (in practice, we can't really
> ever do that).

What about the suggestion on your "Hacks for building on gcc 7 / Fedora 
26" series about avoid building without assertions?

The obvious win is a more readable code.

http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html

> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   qtest.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++-------------------
>   1 file changed, 57 insertions(+), 23 deletions(-)
> 
> diff --git a/qtest.c b/qtest.c
> index 88a09e9afc..cbbfb71114 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -332,10 +332,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>                  strcmp(words[0], "outl") == 0) {
>           unsigned long addr;
>           unsigned long value;
> +        int ret;
> 
>           g_assert(words[1] && words[2]);
> -        g_assert(qemu_strtoul(words[1], NULL, 0, &addr) == 0);
> -        g_assert(qemu_strtoul(words[2], NULL, 0, &value) == 0);
> +        ret = qemu_strtoul(words[1], NULL, 0, &addr);
> +        g_assert(ret == 0);
> +        ret = qemu_strtoul(words[2], NULL, 0, &value);
> +        g_assert(ret == 0);
>           g_assert(addr <= 0xffff);
> 
>           if (words[0][3] == 'b') {
> @@ -352,9 +355,11 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>           strcmp(words[0], "inl") == 0) {
>           unsigned long addr;
>           uint32_t value = -1U;
> +        int ret;
> 
>           g_assert(words[1]);
> -        g_assert(qemu_strtoul(words[1], NULL, 0, &addr) == 0);
> +        ret = qemu_strtoul(words[1], NULL, 0, &addr);
> +        g_assert(ret == 0);
>           g_assert(addr <= 0xffff);
> 
>           if (words[0][2] == 'b') {
> @@ -372,10 +377,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>                  strcmp(words[0], "writeq") == 0) {
>           uint64_t addr;
>           uint64_t value;
> +        int ret;
> 
>           g_assert(words[1] && words[2]);
> -        g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
> -        g_assert(qemu_strtou64(words[2], NULL, 0, &value) == 0);
> +        ret = qemu_strtou64(words[1], NULL, 0, &addr);
> +        g_assert(ret == 0);
> +        ret = qemu_strtou64(words[2], NULL, 0, &value);
> +        g_assert(ret == 0);
> 
>           if (words[0][5] == 'b') {
>               uint8_t data = value;
> @@ -401,9 +409,11 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>                  strcmp(words[0], "readq") == 0) {
>           uint64_t addr;
>           uint64_t value = UINT64_C(-1);
> +        int ret;
> 
>           g_assert(words[1]);
> -        g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
> +        ret = qemu_strtou64(words[1], NULL, 0, &addr);
> +        g_assert(ret == 0);
> 
>           if (words[0][4] == 'b') {
>               uint8_t data;
> @@ -427,10 +437,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>           uint64_t addr, len, i;
>           uint8_t *data;
>           char *enc;
> +        int ret;
> 
>           g_assert(words[1] && words[2]);
> -        g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
> -        g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
> +        ret = qemu_strtou64(words[1], NULL, 0, &addr);
> +        g_assert(ret == 0);
> +        ret = qemu_strtou64(words[2], NULL, 0, &len);
> +        g_assert(ret == 0);
>           /* We'd send garbage to libqtest if len is 0 */
>           g_assert(len);
> 
> @@ -451,10 +464,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>           uint64_t addr, len;
>           uint8_t *data;
>           gchar *b64_data;
> +        int ret;
> 
>           g_assert(words[1] && words[2]);
> -        g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
> -        g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
> +        ret = qemu_strtou64(words[1], NULL, 0, &addr);
> +        g_assert(ret == 0);
> +        ret = qemu_strtou64(words[2], NULL, 0, &len);
> +        g_assert(ret == 0);
> 
>           data = g_malloc(len);
>           cpu_physical_memory_read(addr, data, len);
> @@ -468,10 +484,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>           uint64_t addr, len, i;
>           uint8_t *data;
>           size_t data_len;
> +        int ret;
> 
>           g_assert(words[1] && words[2] && words[3]);
> -        g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
> -        g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
> +        ret = qemu_strtou64(words[1], NULL, 0, &addr);
> +        g_assert(ret == 0);
> +        ret = qemu_strtou64(words[2], NULL, 0, &len);
> +        g_assert(ret == 0);
> 
>           data_len = strlen(words[3]);
>           if (data_len < 3) {
> @@ -497,11 +516,15 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>           uint64_t addr, len;
>           uint8_t *data;
>           unsigned long pattern;
> +        int ret;
> 
>           g_assert(words[1] && words[2] && words[3]);
> -        g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
> -        g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
> -        g_assert(qemu_strtoul(words[3], NULL, 0, &pattern) == 0);
> +        ret = qemu_strtou64(words[1], NULL, 0, &addr);
> +        g_assert(ret == 0);
> +        ret = qemu_strtou64(words[2], NULL, 0, &len);
> +        g_assert(ret == 0);
> +        ret = qemu_strtoul(words[3], NULL, 0, &pattern);
> +        g_assert(ret == 0);
> 
>           if (len) {
>               data = g_malloc(len);
> @@ -517,10 +540,13 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>           uint8_t *data;
>           size_t data_len;
>           gsize out_len;
> +        int ret;
> 
>           g_assert(words[1] && words[2] && words[3]);
> -        g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
> -        g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
> +        ret = qemu_strtou64(words[1], NULL, 0, &addr);
> +        g_assert(ret == 0);
> +        ret = qemu_strtou64(words[2], NULL, 0, &len);
> +        g_assert(ret == 0);
> 
>           data_len = strlen(words[3]);
>           if (data_len < 3) {
> @@ -551,11 +577,16 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>       } else if (strcmp(words[0], "rtas") == 0) {
>           uint64_t res, args, ret;
>           unsigned long nargs, nret;
> +        int rc;
> 
> -        g_assert(qemu_strtoul(words[2], NULL, 0, &nargs) == 0);
> -        g_assert(qemu_strtou64(words[3], NULL, 0, &args) == 0);
> -        g_assert(qemu_strtoul(words[4], NULL, 0, &nret) == 0);
> -        g_assert(qemu_strtou64(words[5], NULL, 0, &ret) == 0);
> +        rc = qemu_strtoul(words[2], NULL, 0, &nargs);
> +        g_assert(rc == 0);
> +        rc = qemu_strtou64(words[3], NULL, 0, &args);
> +        g_assert(rc == 0);
> +        rc = qemu_strtoul(words[4], NULL, 0, &nret);
> +        g_assert(rc == 0);
> +        rc = qemu_strtou64(words[5], NULL, 0, &ret);
> +        g_assert(rc == 0);
>           res = qtest_rtas_call(words[1], nargs, args, nret, ret);
> 
>           qtest_send_prefix(chr);
> @@ -565,7 +596,8 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>           int64_t ns;
> 
>           if (words[1]) {
> -            g_assert(qemu_strtoi64(words[1], NULL, 0, &ns) == 0);
> +            int ret = qemu_strtoi64(words[1], NULL, 0, &ns);
> +            g_assert(ret == 0);
>           } else {
>               ns = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
>           }
> @@ -575,9 +607,11 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>                       (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
>       } else if (qtest_enabled() && strcmp(words[0], "clock_set") == 0) {
>           int64_t ns;
> +        int ret;
> 
>           g_assert(words[1]);
> -        g_assert(qemu_strtoi64(words[1], NULL, 0, &ns) == 0);
> +        ret = qemu_strtoi64(words[1], NULL, 0, &ns);
> +        g_assert(ret == 0);
>           qtest_clock_warp(ns);
>           qtest_send_prefix(chr);
>           qtest_sendf(chr, "OK %"PRIi64"\n",
> 

Re: [Qemu-devel] [PATCH v5 02/13] qtest: Don't perform side effects inside assertion

[Eric Blake]

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

On 08/18/2017 04:33 PM, Philippe Mathieu-Daudé wrote:
> Hi Eric,
> 
> On 08/18/2017 06:15 PM, Eric Blake wrote:
>> Assertions should be separate from the side effects, since in
>> theory, g_assert() can be disabled (in practice, we can't really
>> ever do that).
> 
> What about the suggestion on your "Hacks for building on gcc 7 / Fedora
> 26" series about avoid building without assertions?

NDEBUG doesn't affect g_assert() (only assert(), but that wasn't in use
here) - I have to double-check glib documentation to see whether
g_assert() can be crippled in a manner similar to how I know assert()
can be crippled.  Ideally, we have a form that always performs side
effects exactly once, whether or not abort-on-error is enabled (assert()
does not have that, but I don't know whether glib does).

> 
> The obvious win is a more readable code.
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html

Indeed, that's a patch proposal that I still haven't written, but it can
be done independently of this series.

Still, even if we guarantee that assertions will never be crippled for
qemu, it is bad practice to code side-effects in an assert(), because it
makes the code harder to copy-and-paste into projects that DO work with
assertions disabled, and it raises red flags in reviewers' minds of
whether it was intentional.

[And truth be told, this particular patch is not really about libqtest,
so much as something that horrified me when I was grepping for
qemu_strtoul() in order to fix the checkpatch warning I got on
libqtest's raw use of strtol() in patch 6, and which necessitated me to
write patch 5 - even though the name of the file in question is 'qtest.c']

-- 
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 v5 02/13] qtest: Don't perform side effects inside assertion

[Philippe Mathieu-Daudé]

On 08/18/2017 06:39 PM, Eric Blake wrote:
> On 08/18/2017 04:33 PM, Philippe Mathieu-Daudé wrote:
>> Hi Eric,
>>
>> On 08/18/2017 06:15 PM, Eric Blake wrote:
>>> Assertions should be separate from the side effects, since in
>>> theory, g_assert() can be disabled (in practice, we can't really
>>> ever do that).
>>
>> What about the suggestion on your "Hacks for building on gcc 7 / Fedora
>> 26" series about avoid building without assertions?
> 
> NDEBUG doesn't affect g_assert() (only assert(), but that wasn't in use
> here) - I have to double-check glib documentation to see whether
> g_assert() can be crippled in a manner similar to how I know assert()
> can be crippled.  Ideally, we have a form that always performs side
> effects exactly once, whether or not abort-on-error is enabled (assert()
> does not have that, but I don't know whether glib does).

Yes it does:

https://developer.gnome.org/glib/stable/glib-Testing.html#g-assert

"The macro can be turned off in final releases of code by defining 
G_DISABLE_ASSERT when compiling the application, so code must not depend 
on any side effects from expr ."

> 
>>
>> The obvious win is a more readable code.
>>
>> http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html
> 
> Indeed, that's a patch proposal that I still haven't written, but it can
> be done independently of this series.
> 
> Still, even if we guarantee that assertions will never be crippled for
> qemu, it is bad practice to code side-effects in an assert(), because it
> makes the code harder to copy-and-paste into projects that DO work with
> assertions disabled, and it raises red flags in reviewers' minds of
> whether it was intentional.

This is a good point.

> [And truth be told, this particular patch is not really about libqtest,
> so much as something that horrified me when I was grepping for
> qemu_strtoul() in order to fix the checkpatch warning I got on
> libqtest's raw use of strtol() in patch 6, and which necessitated me to
> write patch 5 - even though the name of the file in question is 'qtest.c']
> 

Re: [Qemu-devel] [PATCH v5 02/13] qtest: Don't perform side effects inside assertion

[Eric Blake]

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

On 08/18/2017 04:39 PM, Eric Blake wrote:
> On 08/18/2017 04:33 PM, Philippe Mathieu-Daudé wrote:
>> Hi Eric,
>>
>> On 08/18/2017 06:15 PM, Eric Blake wrote:
>>> Assertions should be separate from the side effects, since in
>>> theory, g_assert() can be disabled (in practice, we can't really
>>> ever do that).
>>
>> What about the suggestion on your "Hacks for building on gcc 7 / Fedora
>> 26" series about avoid building without assertions?
> 
> NDEBUG doesn't affect g_assert() (only assert(), but that wasn't in use
> here) - I have to double-check glib documentation to see whether
> g_assert() can be crippled in a manner similar to how I know assert()
> can be crippled.  Ideally, we have a form that always performs side
> effects exactly once, whether or not abort-on-error is enabled (assert()
> does not have that, but I don't know whether glib does).

Per https://developer.gnome.org/glib/stable/glib-Testing.html#g-assert,
G_DISABLE_ASSERT has the same compile-time effect on g_assert() as
NDEBUG on assert().  Even worse, g_assert_not_reached() has the same
problem.

Then there is the runtime switch g_test_set_nonfatal_assertions() which
affects the REST of the g_assert_*() family (such as g_assert_cmpint(),
g_assert_true(), g_assert_null()) - but NOT g_assert() proper.  And I
recall Markus has posted in the past about complaining that
g_assert_cmpint() should not be used outside of tests/.

-- 
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 v5 02/13] qtest: Don't perform side effects inside assertion

[Eric Blake]

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

On 08/18/2017 04:58 PM, Eric Blake wrote:
>> NDEBUG doesn't affect g_assert() (only assert(), but that wasn't in use
>> here) - I have to double-check glib documentation to see whether
>> g_assert() can be crippled in a manner similar to how I know assert()
>> can be crippled.  Ideally, we have a form that always performs side
>> effects exactly once, whether or not abort-on-error is enabled (assert()
>> does not have that, but I don't know whether glib does).
> 
> Per https://developer.gnome.org/glib/stable/glib-Testing.html#g-assert,
> G_DISABLE_ASSERT has the same compile-time effect on g_assert() as
> NDEBUG on assert().  Even worse, g_assert_not_reached() has the same
> problem.

Oh, and I failed to mention:

g_assert() has the same problem as assert() - when disabled at
compile-time, 'expr' is not evaluated.  If you want legible code (by
including side-effects within your macro that has optional
abort-on-error behavior), then you need some OTHER macro that always
performs the evaluation.  But if you create such a macro, don't use
'assert' in its name.  And I still fail to see why anyone would actually
want to disable abort-on-error behavior - once an assertion has gone
wrong, continuing execution is liable to hit follow-on problems that are
much harder to diagnose than it would have been just quitting at the
assertion failure.

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

[Qemu-devel] [PATCH v5 03/13] libqtest: Remove dead qtest_instances variable

[Eric Blake]

Prior to commit 063c23d9, we were tracking a list of parallel
qtest objects, in order to safely clean up a SIGABRT handler
only after the last connection quits.  But when we switched to
more of glib's infrastructure, the list became dead code that
is never assigned to.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqtest.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index b9a1f180e1..3f956f09fc 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -42,7 +42,6 @@ struct QTestState
 };

 static GHookList abrt_hooks;
-static GList *qtest_instances;
 static struct sigaction sigact_old;

 #define g_assert_no_errno(ret) do { \
@@ -240,13 +239,10 @@ QTestState *qtest_init(const char *extra_args)

 void qtest_quit(QTestState *s)
 {
-    qtest_instances = g_list_remove(qtest_instances, s);
     g_hook_destroy_link(&abrt_hooks, g_hook_find_data(&abrt_hooks, TRUE, s));

     /* Uninstall SIGABRT handler on last instance */
-    if (!qtest_instances) {
-        cleanup_sigabrt_handler();
-    }
+    cleanup_sigabrt_handler();

     kill_qemu(s);
     close(s->fd);
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 03/13] libqtest: Remove dead qtest_instances variable

[Philippe Mathieu-Daudé]

On 08/18/2017 06:15 PM, Eric Blake wrote:
> Prior to commit 063c23d9, we were tracking a list of parallel
> qtest objects, in order to safely clean up a SIGABRT handler
> only after the last connection quits.  But when we switched to
> more of glib's infrastructure, the list became dead code that
> is never assigned to.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

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

> ---
>   tests/libqtest.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index b9a1f180e1..3f956f09fc 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -42,7 +42,6 @@ struct QTestState
>   };
> 
>   static GHookList abrt_hooks;
> -static GList *qtest_instances;
>   static struct sigaction sigact_old;
> 
>   #define g_assert_no_errno(ret) do { \
> @@ -240,13 +239,10 @@ QTestState *qtest_init(const char *extra_args)
> 
>   void qtest_quit(QTestState *s)
>   {
> -    qtest_instances = g_list_remove(qtest_instances, s);
>       g_hook_destroy_link(&abrt_hooks, g_hook_find_data(&abrt_hooks, TRUE, s));
> 
>       /* Uninstall SIGABRT handler on last instance */
> -    if (!qtest_instances) {
> -        cleanup_sigabrt_handler();
> -    }
> +    cleanup_sigabrt_handler();
> 
>       kill_qemu(s);
>       close(s->fd);
> 

[Qemu-devel] [PATCH v5 04/13] libqtest: Let socket_send() compute length

[Eric Blake]

Rather than make multiple callers call strlen(), it's easier if
socket_send() itself can compute a length via strlen() if none
was provided (caller passes -1).  Callers that can get at the
length more efficiently are left that way.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqtest.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 3f956f09fc..a6ce21d7f9 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -251,10 +251,13 @@ void qtest_quit(QTestState *s)
     g_free(s);
 }

-static void socket_send(int fd, const char *buf, size_t size)
+static void socket_send(int fd, const char *buf, ssize_t size)
 {
     size_t offset;

+    if (size < 0) {
+        size = strlen(buf);
+    }
     offset = 0;
     while (offset < size) {
         ssize_t len;
@@ -274,9 +277,8 @@ static void socket_send(int fd, const char *buf, size_t size)
 static void socket_sendf(int fd, const char *fmt, va_list ap)
 {
     gchar *str = g_strdup_vprintf(fmt, ap);
-    size_t size = strlen(str);

-    socket_send(fd, str, size);
+    socket_send(fd, str, -1);
     g_free(str);
 }

@@ -858,7 +860,7 @@ void qtest_bufwrite(QTestState *s, uint64_t addr, const void *data, size_t size)

     bdata = g_base64_encode(data, size);
     qtest_sendf(s, "b64write 0x%" PRIx64 " 0x%zx ", addr, size);
-    socket_send(s->fd, bdata, strlen(bdata));
+    socket_send(s->fd, bdata, -1);
     socket_send(s->fd, "\n", 1);
     qtest_rsp(s, 0);
     g_free(bdata);
-- 
2.13.5

[Qemu-devel] [PATCH v5 05/13] libqtest: Use qemu_strtoul()

[Eric Blake]

This will keep checkpatch happy when the next patch does code motion.
Fix the include order to match HACKING when adding the needed header.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqtest.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index a6ce21d7f9..438a22678d 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -15,12 +15,13 @@
  *
  */
 #include "qemu/osdep.h"
-#include "libqtest.h"

 #include <sys/socket.h>
 #include <sys/wait.h>
 #include <sys/un.h>

+#include "libqtest.h"
+#include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/json-streamer.h"
@@ -333,12 +334,14 @@ redo:
     g_string_free(line, TRUE);

     if (strcmp(words[0], "IRQ") == 0) {
-        int irq;
+        long irq;
+        int ret;

         g_assert(words[1] != NULL);
         g_assert(words[2] != NULL);

-        irq = strtoul(words[2], NULL, 0);
+        ret = qemu_strtol(words[2], NULL, 0, &irq);
+        g_assert(!ret);
         g_assert_cmpint(irq, >=, 0);
         g_assert_cmpint(irq, <, MAX_IRQ);

@@ -701,11 +704,13 @@ void qtest_outl(QTestState *s, uint16_t addr, uint32_t value)
 static uint32_t qtest_in(QTestState *s, const char *cmd, uint16_t addr)
 {
     gchar **args;
-    uint32_t value;
+    int ret;
+    unsigned long value;

     qtest_sendf(s, "%s 0x%x\n", cmd, addr);
     args = qtest_rsp(s, 2);
-    value = strtoul(args[1], NULL, 0);
+    ret = qemu_strtoul(args[1], NULL, 0, &value);
+    g_assert(!ret && value <= UINT32_MAX);
     g_strfreev(args);

     return value;
@@ -756,11 +761,13 @@ void qtest_writeq(QTestState *s, uint64_t addr, uint64_t value)
 static uint64_t qtest_read(QTestState *s, const char *cmd, uint64_t addr)
 {
     gchar **args;
+    int ret;
     uint64_t value;

     qtest_sendf(s, "%s 0x%" PRIx64 "\n", cmd, addr);
     args = qtest_rsp(s, 2);
-    value = strtoull(args[1], NULL, 0);
+    ret = qemu_strtou64(args[1], NULL, 0, &value);
+    g_assert(!ret);
     g_strfreev(args);

     return value;
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 05/13] libqtest: Use qemu_strtoul()

[Philippe Mathieu-Daudé]

On 08/18/2017 06:15 PM, Eric Blake wrote:
> This will keep checkpatch happy when the next patch does code motion.
> Fix the include order to match HACKING when adding the needed header.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

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

> ---
>   tests/libqtest.c | 19 +++++++++++++------
>   1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index a6ce21d7f9..438a22678d 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -15,12 +15,13 @@
>    *
>    */
>   #include "qemu/osdep.h"
> -#include "libqtest.h"
> 
>   #include <sys/socket.h>
>   #include <sys/wait.h>
>   #include <sys/un.h>
> 
> +#include "libqtest.h"
> +#include "qemu/cutils.h"
>   #include "qapi/error.h"
>   #include "qapi/qmp/json-parser.h"
>   #include "qapi/qmp/json-streamer.h"
> @@ -333,12 +334,14 @@ redo:
>       g_string_free(line, TRUE);
> 
>       if (strcmp(words[0], "IRQ") == 0) {
> -        int irq;
> +        long irq;
> +        int ret;
> 
>           g_assert(words[1] != NULL);
>           g_assert(words[2] != NULL);
> 
> -        irq = strtoul(words[2], NULL, 0);
> +        ret = qemu_strtol(words[2], NULL, 0, &irq);
> +        g_assert(!ret);
>           g_assert_cmpint(irq, >=, 0);
>           g_assert_cmpint(irq, <, MAX_IRQ);
> 
> @@ -701,11 +704,13 @@ void qtest_outl(QTestState *s, uint16_t addr, uint32_t value)
>   static uint32_t qtest_in(QTestState *s, const char *cmd, uint16_t addr)
>   {
>       gchar **args;
> -    uint32_t value;
> +    int ret;
> +    unsigned long value;
> 
>       qtest_sendf(s, "%s 0x%x\n", cmd, addr);
>       args = qtest_rsp(s, 2);
> -    value = strtoul(args[1], NULL, 0);
> +    ret = qemu_strtoul(args[1], NULL, 0, &value);
> +    g_assert(!ret && value <= UINT32_MAX);
>       g_strfreev(args);
> 
>       return value;
> @@ -756,11 +761,13 @@ void qtest_writeq(QTestState *s, uint64_t addr, uint64_t value)
>   static uint64_t qtest_read(QTestState *s, const char *cmd, uint64_t addr)
>   {
>       gchar **args;
> +    int ret;
>       uint64_t value;
> 
>       qtest_sendf(s, "%s 0x%" PRIx64 "\n", cmd, addr);
>       args = qtest_rsp(s, 2);
> -    value = strtoull(args[1], NULL, 0);
> +    ret = qemu_strtou64(args[1], NULL, 0, &value);
> +    g_assert(!ret);
>       g_strfreev(args);
> 
>       return value;
> 

[Qemu-devel] [PATCH v5 06/13] libqtest: Topologically sort functions

[Eric Blake]

Put static functions prior to public headers, in part so that
improvements to qtest_start() can benefit from the static
helpers without needing forward references.  Code motion, with
no semantic change.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqtest.c | 263 +++++++++++++++++++++++++++----------------------------
 1 file changed, 131 insertions(+), 132 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 438a22678d..5d16351e24 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -49,7 +49,6 @@ static struct sigaction sigact_old;
     g_assert_cmpint(ret, !=, -1); \
 } while (0)

-static int qtest_query_target_endianness(QTestState *s);

 static int init_socket(const char *socket_path)
 {
@@ -128,6 +127,137 @@ static void setup_sigabrt_handler(void)
     sigaction(SIGABRT, &sigact, &sigact_old);
 }

+static void socket_send(int fd, const char *buf, ssize_t size)
+{
+    size_t offset;
+
+    if (size < 0) {
+        size = strlen(buf);
+    }
+    offset = 0;
+    while (offset < size) {
+        ssize_t len;
+
+        len = write(fd, buf + offset, size - offset);
+        if (len == -1 && errno == EINTR) {
+            continue;
+        }
+
+        g_assert_no_errno(len);
+        g_assert_cmpint(len, >, 0);
+
+        offset += len;
+    }
+}
+
+static void socket_sendf(int fd, const char *fmt, va_list ap)
+{
+    gchar *str = g_strdup_vprintf(fmt, ap);
+
+    socket_send(fd, str, -1);
+    g_free(str);
+}
+
+static void GCC_FMT_ATTR(2, 3) qtest_sendf(QTestState *s, const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    socket_sendf(s->fd, fmt, ap);
+    va_end(ap);
+}
+
+static GString *qtest_recv_line(QTestState *s)
+{
+    GString *line;
+    size_t offset;
+    char *eol;
+
+    while ((eol = strchr(s->rx->str, '\n')) == NULL) {
+        ssize_t len;
+        char buffer[1024];
+
+        len = read(s->fd, buffer, sizeof(buffer));
+        if (len == -1 && errno == EINTR) {
+            continue;
+        }
+
+        if (len == -1 || len == 0) {
+            fprintf(stderr, "Broken pipe\n");
+            exit(1);
+        }
+
+        g_string_append_len(s->rx, buffer, len);
+    }
+
+    offset = eol - s->rx->str;
+    line = g_string_new_len(s->rx->str, offset);
+    g_string_erase(s->rx, 0, offset + 1);
+
+    return line;
+}
+
+static gchar **qtest_rsp(QTestState *s, int expected_args)
+{
+    GString *line;
+    gchar **words;
+    int i;
+
+redo:
+    line = qtest_recv_line(s);
+    words = g_strsplit(line->str, " ", 0);
+    g_string_free(line, TRUE);
+
+    if (strcmp(words[0], "IRQ") == 0) {
+        long irq;
+        int ret;
+
+        g_assert(words[1] != NULL);
+        g_assert(words[2] != NULL);
+
+        ret = qemu_strtol(words[2], NULL, 0, &irq);
+        g_assert(!ret);
+        g_assert_cmpint(irq, >=, 0);
+        g_assert_cmpint(irq, <, MAX_IRQ);
+
+        if (strcmp(words[1], "raise") == 0) {
+            s->irq_level[irq] = true;
+        } else {
+            s->irq_level[irq] = false;
+        }
+
+        g_strfreev(words);
+        goto redo;
+    }
+
+    g_assert(words[0] != NULL);
+    g_assert_cmpstr(words[0], ==, "OK");
+
+    if (expected_args) {
+        for (i = 0; i < expected_args; i++) {
+            g_assert(words[i] != NULL);
+        }
+    } else {
+        g_strfreev(words);
+    }
+
+    return words;
+}
+
+static int qtest_query_target_endianness(QTestState *s)
+{
+    gchar **args;
+    int big_endian;
+
+    qtest_sendf(s, "endianness\n");
+    args = qtest_rsp(s, 1);
+    g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
+    big_endian = strcmp(args[1], "big") == 0;
+    g_strfreev(args);
+
+    return big_endian;
+}
+
 static void cleanup_sigabrt_handler(void)
 {
     sigaction(SIGABRT, &sigact_old, NULL);
@@ -252,137 +382,6 @@ void qtest_quit(QTestState *s)
     g_free(s);
 }

-static void socket_send(int fd, const char *buf, ssize_t size)
-{
-    size_t offset;
-
-    if (size < 0) {
-        size = strlen(buf);
-    }
-    offset = 0;
-    while (offset < size) {
-        ssize_t len;
-
-        len = write(fd, buf + offset, size - offset);
-        if (len == -1 && errno == EINTR) {
-            continue;
-        }
-
-        g_assert_no_errno(len);
-        g_assert_cmpint(len, >, 0);
-
-        offset += len;
-    }
-}
-
-static void socket_sendf(int fd, const char *fmt, va_list ap)
-{
-    gchar *str = g_strdup_vprintf(fmt, ap);
-
-    socket_send(fd, str, -1);
-    g_free(str);
-}
-
-static void GCC_FMT_ATTR(2, 3) qtest_sendf(QTestState *s, const char *fmt, ...)
-{
-    va_list ap;
-
-    va_start(ap, fmt);
-    socket_sendf(s->fd, fmt, ap);
-    va_end(ap);
-}
-
-static GString *qtest_recv_line(QTestState *s)
-{
-    GString *line;
-    size_t offset;
-    char *eol;
-
-    while ((eol = strchr(s->rx->str, '\n')) == NULL) {
-        ssize_t len;
-        char buffer[1024];
-
-        len = read(s->fd, buffer, sizeof(buffer));
-        if (len == -1 && errno == EINTR) {
-            continue;
-        }
-
-        if (len == -1 || len == 0) {
-            fprintf(stderr, "Broken pipe\n");
-            exit(1);
-        }
-
-        g_string_append_len(s->rx, buffer, len);
-    }
-
-    offset = eol - s->rx->str;
-    line = g_string_new_len(s->rx->str, offset);
-    g_string_erase(s->rx, 0, offset + 1);
-
-    return line;
-}
-
-static gchar **qtest_rsp(QTestState *s, int expected_args)
-{
-    GString *line;
-    gchar **words;
-    int i;
-
-redo:
-    line = qtest_recv_line(s);
-    words = g_strsplit(line->str, " ", 0);
-    g_string_free(line, TRUE);
-
-    if (strcmp(words[0], "IRQ") == 0) {
-        long irq;
-        int ret;
-
-        g_assert(words[1] != NULL);
-        g_assert(words[2] != NULL);
-
-        ret = qemu_strtol(words[2], NULL, 0, &irq);
-        g_assert(!ret);
-        g_assert_cmpint(irq, >=, 0);
-        g_assert_cmpint(irq, <, MAX_IRQ);
-
-        if (strcmp(words[1], "raise") == 0) {
-            s->irq_level[irq] = true;
-        } else {
-            s->irq_level[irq] = false;
-        }
-
-        g_strfreev(words);
-        goto redo;
-    }
-
-    g_assert(words[0] != NULL);
-    g_assert_cmpstr(words[0], ==, "OK");
-
-    if (expected_args) {
-        for (i = 0; i < expected_args; i++) {
-            g_assert(words[i] != NULL);
-        }
-    } else {
-        g_strfreev(words);
-    }
-
-    return words;
-}
-
-static int qtest_query_target_endianness(QTestState *s)
-{
-    gchar **args;
-    int big_endian;
-
-    qtest_sendf(s, "endianness\n");
-    args = qtest_rsp(s, 1);
-    g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
-    big_endian = strcmp(args[1], "big") == 0;
-    g_strfreev(args);
-
-    return big_endian;
-}
-
 typedef struct {
     JSONMessageParser parser;
     QDict *response;
-- 
2.13.5

[Qemu-devel] [PATCH v5 07/13] libqtest: Inline qtest_query_target_endianness()

[Eric Blake]

There was only one caller; it's easier to inline things.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqtest.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 5d16351e24..b6dd26e54a 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -244,20 +244,6 @@ redo:
     return words;
 }

-static int qtest_query_target_endianness(QTestState *s)
-{
-    gchar **args;
-    int big_endian;
-
-    qtest_sendf(s, "endianness\n");
-    args = qtest_rsp(s, 1);
-    g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
-    big_endian = strcmp(args[1], "big") == 0;
-    g_strfreev(args);
-
-    return big_endian;
-}
-
 static void cleanup_sigabrt_handler(void)
 {
     sigaction(SIGABRT, &sigact_old, NULL);
@@ -288,6 +274,7 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
     gchar *qmp_socket_path;
     gchar *command;
     const char *qemu_binary;
+    gchar **args;

     qemu_binary = getenv("QTEST_QEMU_BINARY");
     if (!qemu_binary) {
@@ -351,8 +338,11 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
     }

     /* ask endianness of the target */
-
-    s->big_endian = qtest_query_target_endianness(s);
+    qtest_sendf(s, "endianness\n");
+    args = qtest_rsp(s, 1);
+    g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
+    s->big_endian = strcmp(args[1], "big") == 0;
+    g_strfreev(args);

     return s;
 }
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 07/13] libqtest: Inline qtest_query_target_endianness()

[Philippe Mathieu-Daudé]

On 08/18/2017 06:15 PM, Eric Blake wrote:
> There was only one caller; it's easier to inline things.

What's the benefit of this change? Having a separate function ease code 
reading. You might add the 'inline' qualifier but the compiler is smart 
enough to inline it.

Instead of this change I'd inline it and remove the return value, having 
a void function initializing s->big_endian. Matter of taste I think :)

> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   tests/libqtest.c | 22 ++++++----------------
>   1 file changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 5d16351e24..b6dd26e54a 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -244,20 +244,6 @@ redo:
>       return words;
>   }
> 
> -static int qtest_query_target_endianness(QTestState *s)
> -{
> -    gchar **args;
> -    int big_endian;
> -
> -    qtest_sendf(s, "endianness\n");
> -    args = qtest_rsp(s, 1);
> -    g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
> -    big_endian = strcmp(args[1], "big") == 0;
> -    g_strfreev(args);
> -
> -    return big_endian;
> -}
> -
>   static void cleanup_sigabrt_handler(void)
>   {
>       sigaction(SIGABRT, &sigact_old, NULL);
> @@ -288,6 +274,7 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
>       gchar *qmp_socket_path;
>       gchar *command;
>       const char *qemu_binary;
> +    gchar **args;
> 
>       qemu_binary = getenv("QTEST_QEMU_BINARY");
>       if (!qemu_binary) {
> @@ -351,8 +338,11 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
>       }
> 
>       /* ask endianness of the target */
> -
> -    s->big_endian = qtest_query_target_endianness(s);
> +    qtest_sendf(s, "endianness\n");
> +    args = qtest_rsp(s, 1);
> +    g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
> +    s->big_endian = strcmp(args[1], "big") == 0;
> +    g_strfreev(args);
> 
>       return s;
>   }
> 

Re: [Qemu-devel] [PATCH v5 07/13] libqtest: Inline qtest_query_target_endianness()

[Eric Blake]

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

On 08/18/2017 04:46 PM, Philippe Mathieu-Daudé wrote:
> On 08/18/2017 06:15 PM, Eric Blake wrote:
>> There was only one caller; it's easier to inline things.
> 
> What's the benefit of this change? Having a separate function ease code
> reading. You might add the 'inline' qualifier but the compiler is smart
> enough to inline it.
> 
> Instead of this change I'd inline it and remove the return value, having
> a void function initializing s->big_endian. Matter of taste I think :)
> 
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   tests/libqtest.c | 22 ++++++----------------
>>   1 file changed, 6 insertions(+), 16 deletions(-)

I think the diffstat shows that inlining is a win.  My other reason that
it is a win:

>> @@ -351,8 +338,11 @@ QTestState
>> *qtest_init_without_qmp_handshake(const char *extra_args)
>>       }
>>
>>       /* ask endianness of the target */
>> -
>> -    s->big_endian = qtest_query_target_endianness(s);
>> +    qtest_sendf(s, "endianness\n");
>> +    args = qtest_rsp(s, 1);

The old code has to pass an arbitrary QTestState *s down to a helper
function.  But I'm trying to get rid of as many functions as possible
that depend on arbitrary QTestState *s, particularly when callers end up
passing 'global_qtest' for 's'.  Inlining it makes it more obvious that
the changes in 10/13 are safely operating on global_qtest, which in turn
makes it possible for 12/13 to drop QTestState *s parameters from
qtest_sendf() and qtest_rsp().  If the function were not inlined, those
later cleanups would not be as trivial.  (Or put another way - I rebased
this patch to be earlier in the series precisely because of what I ran
into while writing those patches)

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

[Qemu-devel] [PATCH v5 08/13] tests: Rely more on global_qtest

[Eric Blake]

libqtest provides two layers of functions: qtest_*() that operate
on an explicit object, and a plain version that operates on the
'global_qtest' object.  However, very few tests care about the
distinction, and even the tests that manipulate multiple qtest
connections at once are just fine reassigning global_qtest around
the blocks of code that will then operate on the updated global,
rather than calling the verbose form.  Before the next few patches
get rid of the qtest_* layer, we first need to update the remaining
few spots that were using the long form where we can instead rely
on the short form.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/fdc-test.c         |  2 +-
 tests/ide-test.c         | 10 +++++-----
 tests/ipmi-bt-test.c     |  2 +-
 tests/ipmi-kcs-test.c    |  2 +-
 tests/libqos/libqos-pc.c |  2 +-
 tests/postcopy-test.c    | 14 +++++++-------
 tests/rtc-test.c         |  9 +++------
 tests/tco-test.c         |  5 ++---
 tests/wdt_ib700-test.c   | 30 +++++++++++++++++-------------
 9 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index 325712e0f2..0b68d9aec4 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -565,7 +565,7 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);

     qtest_start("-device floppy,id=floppy0");
-    qtest_irq_intercept_in(global_qtest, "ioapic");
+    irq_intercept_in("ioapic");
     qtest_add_func("/fdc/cmos", test_cmos);
     qtest_add_func("/fdc/no_media_on_start", test_no_media_on_start);
     qtest_add_func("/fdc/read_without_media", test_read_without_media);
diff --git a/tests/ide-test.c b/tests/ide-test.c
index aa9de065fc..505a800b44 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -462,7 +462,7 @@ static void test_bmdma_setup(void)
         "-drive file=%s,if=ide,serial=%s,cache=writeback,format=raw "
         "-global ide-hd.ver=%s",
         tmp_path, "testdisk", "version");
-    qtest_irq_intercept_in(global_qtest, "ioapic");
+    irq_intercept_in("ioapic");
 }

 static void test_bmdma_teardown(void)
@@ -584,7 +584,7 @@ static void test_flush(void)

     dev = get_pci_device(&bmdma_bar, &ide_bar);

-    qtest_irq_intercept_in(global_qtest, "ioapic");
+    irq_intercept_in("ioapic");

     /* Dirty media so that CMD_FLUSH_CACHE will actually go to disk */
     make_dirty(0);
@@ -635,7 +635,7 @@ static void test_retry_flush(const char *machine)

     dev = get_pci_device(&bmdma_bar, &ide_bar);

-    qtest_irq_intercept_in(global_qtest, "ioapic");
+    irq_intercept_in("ioapic");

     /* Dirty media so that CMD_FLUSH_CACHE will actually go to disk */
     make_dirty(0);
@@ -826,7 +826,7 @@ static void cdrom_pio_impl(int nblocks)
     ide_test_start("-drive if=none,file=%s,media=cdrom,format=raw,id=sr0,index=0 "
                    "-device ide-cd,drive=sr0,bus=ide.0", tmp_path);
     dev = get_pci_device(&bmdma_bar, &ide_bar);
-    qtest_irq_intercept_in(global_qtest, "ioapic");
+    irq_intercept_in("ioapic");

     /* PACKET command on device 0 */
     qpci_io_writeb(dev, ide_bar, reg_device, 0);
@@ -909,7 +909,7 @@ static void test_cdrom_dma(void)

     ide_test_start("-drive if=none,file=%s,media=cdrom,format=raw,id=sr0,index=0 "
                    "-device ide-cd,drive=sr0,bus=ide.0", tmp_path);
-    qtest_irq_intercept_in(global_qtest, "ioapic");
+    irq_intercept_in("ioapic");

     guest_buf = guest_alloc(guest_malloc, len);
     prdt[0].addr = cpu_to_le32(guest_buf);
diff --git a/tests/ipmi-bt-test.c b/tests/ipmi-bt-test.c
index 7e21a9bbcb..891f5bfb13 100644
--- a/tests/ipmi-bt-test.c
+++ b/tests/ipmi-bt-test.c
@@ -421,7 +421,7 @@ int main(int argc, char **argv)
           " -device isa-ipmi-bt,bmc=bmc0", emu_port);
     qtest_start(cmdline);
     g_free(cmdline);
-    qtest_irq_intercept_in(global_qtest, "ioapic");
+    irq_intercept_in("ioapic");
     qtest_add_func("/ipmi/extern/connect", test_connect);
     qtest_add_func("/ipmi/extern/bt_base", test_bt_base);
     qtest_add_func("/ipmi/extern/bt_enable_irq", test_enable_irq);
diff --git a/tests/ipmi-kcs-test.c b/tests/ipmi-kcs-test.c
index 178ffc1797..53127d2884 100644
--- a/tests/ipmi-kcs-test.c
+++ b/tests/ipmi-kcs-test.c
@@ -280,7 +280,7 @@ int main(int argc, char **argv)
                               " -device isa-ipmi-kcs,bmc=bmc0");
     qtest_start(cmdline);
     g_free(cmdline);
-    qtest_irq_intercept_in(global_qtest, "ioapic");
+    irq_intercept_in("ioapic");
     qtest_add_func("/ipmi/local/kcs_base", test_kcs_base);
     qtest_add_func("/ipmi/local/kcs_abort", test_kcs_abort);
     qtest_add_func("/ipmi/local/kcs_enable_irq", test_enable_irq);
diff --git a/tests/libqos/libqos-pc.c b/tests/libqos/libqos-pc.c
index b554758802..6a2ff6608b 100644
--- a/tests/libqos/libqos-pc.c
+++ b/tests/libqos/libqos-pc.c
@@ -25,7 +25,7 @@ QOSState *qtest_pc_boot(const char *cmdline_fmt, ...)
     qs = qtest_vboot(&qos_ops, cmdline_fmt, ap);
     va_end(ap);

-    qtest_irq_intercept_in(global_qtest, "ioapic");
+    irq_intercept_in("ioapic");

     return qs;
 }
diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index 8142f2ab90..9c4e37473d 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -236,7 +236,7 @@ static QDict *return_or_event(QDict *response)
         got_stop = true;
     }
     QDECREF(response);
-    return return_or_event(qtest_qmp_receive(global_qtest));
+    return return_or_event(qmp_receive());
 }


@@ -318,13 +318,13 @@ static void check_guests_ram(void)
     bool hit_edge = false;
     bool bad = false;

-    qtest_memread(global_qtest, start_address, &first_byte, 1);
+    memread(start_address, &first_byte, 1);
     last_byte = first_byte;

     for (address = start_address + 4096; address < end_address; address += 4096)
     {
         uint8_t b;
-        qtest_memread(global_qtest, address, &b, 1);
+        memread(address, &b, 1);
         if (b != last_byte) {
             if (((b + 1) % 256) == last_byte && !hit_edge) {
                 /* This is OK, the guest stopped at the point of
@@ -474,19 +474,19 @@ static void test_migrate(void)

     global_qtest = to;

-    qtest_memread(to, start_address, &dest_byte_a, 1);
+    memread(start_address, &dest_byte_a, 1);

     /* Destination still running, wait for a byte to change */
     do {
-        qtest_memread(to, start_address, &dest_byte_b, 1);
+        memread(start_address, &dest_byte_b, 1);
         usleep(10 * 1000);
     } while (dest_byte_a == dest_byte_b);

     qmp_discard_response("{ 'execute' : 'stop'}");
     /* With it stopped, check nothing changes */
-    qtest_memread(to, start_address, &dest_byte_c, 1);
+    memread(start_address, &dest_byte_c, 1);
     sleep(1);
-    qtest_memread(to, start_address, &dest_byte_d, 1);
+    memread(start_address, &dest_byte_d, 1);
     g_assert_cmpint(dest_byte_c, ==, dest_byte_d);

     check_guests_ram();
diff --git a/tests/rtc-test.c b/tests/rtc-test.c
index d7a96cbd79..bdd234d316 100644
--- a/tests/rtc-test.c
+++ b/tests/rtc-test.c
@@ -685,13 +685,12 @@ static void periodic_timer(void)

 int main(int argc, char **argv)
 {
-    QTestState *s = NULL;
     int ret;

     g_test_init(&argc, &argv, NULL);

-    s = qtest_start("-rtc clock=vm");
-    qtest_irq_intercept_in(s, "ioapic");
+    qtest_start("-rtc clock=vm");
+    irq_intercept_in("ioapic");

     qtest_add_func("/rtc/check-time/bcd", bcd_check_time);
     qtest_add_func("/rtc/check-time/dec", dec_check_time);
@@ -711,9 +710,7 @@ int main(int argc, char **argv)

     ret = g_test_run();

-    if (s) {
-        qtest_quit(s);
-    }
+    qtest_end();

     return ret;
 }
diff --git a/tests/tco-test.c b/tests/tco-test.c
index c4c264eb3d..5b87bc16b9 100644
--- a/tests/tco-test.c
+++ b/tests/tco-test.c
@@ -54,14 +54,13 @@ static void test_end(TestData *d)

 static void test_init(TestData *d)
 {
-    QTestState *qs;
     char *s;

     s = g_strdup_printf("-machine q35 %s %s",
                         d->noreboot ? "" : "-global ICH9-LPC.noreboot=false",
                         !d->args ? "" : d->args);
-    qs = qtest_start(s);
-    qtest_irq_intercept_in(qs, "ioapic");
+    qtest_start(s);
+    irq_intercept_in("ioapic");
     g_free(s);

     d->bus = qpci_init_pc(NULL);
diff --git a/tests/wdt_ib700-test.c b/tests/wdt_ib700-test.c
index 49f4f0c221..59ba184a82 100644
--- a/tests/wdt_ib700-test.c
+++ b/tests/wdt_ib700-test.c
@@ -36,7 +36,7 @@ static QDict *qmp_get_event(const char *name)
     return data;
 }

-static QDict *ib700_program_and_wait(QTestState *s)
+static QDict *ib700_program_and_wait(void)
 {
     clock_step(NANOSECONDS_PER_SECOND * 40);
     qmp_check_no_event();
@@ -68,9 +68,10 @@ static QDict *ib700_program_and_wait(QTestState *s)
 static void ib700_pause(void)
 {
     QDict *d;
-    QTestState *s = qtest_start("-watchdog-action pause -device ib700");
-    qtest_irq_intercept_in(s, "ioapic");
-    d = ib700_program_and_wait(s);
+
+    qtest_start("-watchdog-action pause -device ib700");
+    irq_intercept_in("ioapic");
+    d = ib700_program_and_wait();
     g_assert(!strcmp(qdict_get_str(d, "action"), "pause"));
     QDECREF(d);
     d = qmp_get_event("STOP");
@@ -81,9 +82,10 @@ static void ib700_pause(void)
 static void ib700_reset(void)
 {
     QDict *d;
-    QTestState *s = qtest_start("-watchdog-action reset -device ib700");
-    qtest_irq_intercept_in(s, "ioapic");
-    d = ib700_program_and_wait(s);
+
+    qtest_start("-watchdog-action reset -device ib700");
+    irq_intercept_in("ioapic");
+    d = ib700_program_and_wait();
     g_assert(!strcmp(qdict_get_str(d, "action"), "reset"));
     QDECREF(d);
     d = qmp_get_event("RESET");
@@ -94,9 +96,10 @@ static void ib700_reset(void)
 static void ib700_shutdown(void)
 {
     QDict *d;
-    QTestState *s = qtest_start("-watchdog-action reset -no-reboot -device ib700");
-    qtest_irq_intercept_in(s, "ioapic");
-    d = ib700_program_and_wait(s);
+
+    qtest_start("-watchdog-action reset -no-reboot -device ib700");
+    irq_intercept_in("ioapic");
+    d = ib700_program_and_wait();
     g_assert(!strcmp(qdict_get_str(d, "action"), "reset"));
     QDECREF(d);
     d = qmp_get_event("SHUTDOWN");
@@ -107,9 +110,10 @@ static void ib700_shutdown(void)
 static void ib700_none(void)
 {
     QDict *d;
-    QTestState *s = qtest_start("-watchdog-action none -device ib700");
-    qtest_irq_intercept_in(s, "ioapic");
-    d = ib700_program_and_wait(s);
+
+    qtest_start("-watchdog-action none -device ib700");
+    irq_intercept_in("ioapic");
+    d = ib700_program_and_wait();
     g_assert(!strcmp(qdict_get_str(d, "action"), "none"));
     QDECREF(d);
     qtest_end();
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 08/13] tests: Rely more on global_qtest

[John Snow]

On 08/18/2017 05:15 PM, Eric Blake wrote:
> libqtest provides two layers of functions: qtest_*() that operate
> on an explicit object, and a plain version that operates on the
> 'global_qtest' object.  However, very few tests care about the
> distinction, and even the tests that manipulate multiple qtest
> connections at once are just fine reassigning global_qtest around
> the blocks of code that will then operate on the updated global,
> rather than calling the verbose form.  Before the next few patches
> get rid of the qtest_* layer, we first need to update the remaining
> few spots that were using the long form where we can instead rely
> on the short form.
> 

Not a big fan of globals and implicit state, but I do at least agree
that we don't need two sets of functions.

(You just happen to be killing the set I like.)

eh, to-may-to to-mah-to.

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

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

Re: [Qemu-devel] [Qemu-block] [PATCH v5 08/13] tests: Rely more on global_qtest

[Paolo Bonzini]

On 18/08/2017 23:33, John Snow wrote:
>> Before the next few patches
>> get rid of the qtest_* layer, we first need to update the remaining
>> few spots that were using the long form where we can instead rely
>> on the short form.
>>
> Not a big fan of globals and implicit state, but I do at least agree
> that we don't need two sets of functions.

I agree with using the short form where possible, but I disagree on
removing the long forms.  Rather, global_qtest in my opinion should have
been static (though I'm not proposing that you do this); inlining the
wrappers is not needed for performance.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v5 08/13] tests: Rely more on global_qtest

[Eric Blake]

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

On 08/19/2017 11:34 AM, Paolo Bonzini wrote:
> On 18/08/2017 23:33, John Snow wrote:
>>> Before the next few patches
>>> get rid of the qtest_* layer, we first need to update the remaining
>>> few spots that were using the long form where we can instead rely
>>> on the short form.
>>>
>> Not a big fan of globals and implicit state, but I do at least agree
>> that we don't need two sets of functions.
> 
> I agree with using the short form where possible, but I disagree on
> removing the long forms.  Rather, global_qtest in my opinion should have
> been static (though I'm not proposing that you do this); inlining the
> wrappers is not needed for performance.

But no one outside of libqtest.c is using the long form.  Maintaining a
long form that isn't used is counter-productive, if the short form is
good enough for everything we need.

-- 
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] [Qemu-block] [PATCH v5 08/13] tests: Rely more on global_qtest

[Paolo Bonzini]

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

On 23/08/2017 21:26, Eric Blake wrote:
>> I agree with using the short form where possible, but I disagree on
>> removing the long forms.  Rather, global_qtest in my opinion should have
>> been static (though I'm not proposing that you do this); inlining the
>> wrappers is not needed for performance.
> But no one outside of libqtest.c is using the long form.  Maintaining a
> long form that isn't used is counter-productive, if the short form is
> good enough for everything we need.

Well, whatever is assigning to global_qtest should be using the long form.

Paolo


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

Re: [Qemu-devel] [Qemu-block] [PATCH v5 08/13] tests: Rely more on global_qtest

[Eric Blake]

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

On 08/23/2017 03:02 PM, Paolo Bonzini wrote:
> On 23/08/2017 21:26, Eric Blake wrote:
>>> I agree with using the short form where possible, but I disagree on
>>> removing the long forms.  Rather, global_qtest in my opinion should have
>>> been static (though I'm not proposing that you do this); inlining the
>>> wrappers is not needed for performance.
>> But no one outside of libqtest.c is using the long form.  Maintaining a
>> long form that isn't used is counter-productive, if the short form is
>> good enough for everything we need.
> 
> Well, whatever is assigning to global_qtest should be using the long form.

Question - what about going the other way, and switching ALL callers to
always use the explicit form?  I'd really like to maintain only one
form, but if we think maintaining the explicit form is better, then I'd
rather see:

s = qtest_init(...);
qmp(s, "{'execute':'foo'}");

than:

qtest_init(...); // implicitly sets global_test
qmp("{'execute':'foo'}"); // implicitly uses global_test

and not the existing:

qtest_init(...);
qtest_qmp(global_test, "{'execute':'foo'}");

Then, in combination with my other series to make qmp() more friendly,
it would look like:

s = qtest_init(...);
qmp(s, "foo");

Or, put another way, I'd rather have the short name irq_intercept_in()
than the long name qtest_irq_intercept_in(); but as long as we maintain
only ONE set of names, then I'm okay having that one set always be
explicit about passing QTestState *s as a parameter and completely
getting rid of the global_test variable (rather than making it static).

But whatever we decide, we'll want to stick to it, and it may require a
respin - so let's get a consensus before I do any more churn on this series.

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH v5 08/13] tests: Rely more on global_qtest

[Paolo Bonzini]

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

On 23/08/2017 23:30, Eric Blake wrote:
>> Well, whatever is assigning to global_qtest should be using the long form.
> Question - what about going the other way, and switching ALL callers to
> always use the explicit form?  I'd really like to maintain only one
> form, but if we think maintaining the explicit form is better, then I'd
> rather see:
> 
> s = qtest_init(...);
> qmp(s, "{'execute':'foo'}");

I think the short form is not problematic per se, but when e.g.
migration is involved, then the tests are abusing it...

Paolo


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

Re: [Qemu-devel] [Qemu-block] [PATCH v5 08/13] tests: Rely more on global_qtest

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 23/08/2017 23:30, Eric Blake wrote:
>>> Well, whatever is assigning to global_qtest should be using the long form.
>> Question - what about going the other way, and switching ALL callers to
>> always use the explicit form?  I'd really like to maintain only one
>> form, but if we think maintaining the explicit form is better, then I'd
>> rather see:
>> 
>> s = qtest_init(...);
>> qmp(s, "{'execute':'foo'}");
>
> I think the short form is not problematic per se, but when e.g.
> migration is involved, then the tests are abusing it...

The long forms avoid global state.  Global state exits through the front
door; designer congratulates himself for this virtous deed.

Trouble is the long forms are, well, long, so designer creates short
ones.  Global state jumps right back through the open window.  Almost
everything uses the short forms, and with good reason.

Ritual avoidance of global state without really avoiding it has a
name in German programming jargon: "Muesli".

While undisciplined use of global state is the root of many problems, I
fail to see a problem here.  For me,

    do this with QTestState A
    do that with QTestState A

    do something with QTestState B

    do more with QTestState A

is no better than

    with QTestState A
        do this
        do that

    with QTestState B
        do something

    with QTestState A
        do more

In a language less primitive than C, I'd write it exactly that way, and
nobody would complain.  In old, primitive C, I have to write

    global_qtest = A;
    do this
    do that

    global_qtest = B;
    do something

    global_qtest = A;
    do more

Why's that so horrible to justify busywork on wrappers?

Creating and updating the short form wrappers has grown on me.  If I
enjoyed busywork, I would've become Vice President of Nothing in
Particular or something.

Pretty-please?  ;)

Re: [Qemu-devel] [Qemu-block] [PATCH v5 08/13] tests: Rely more on global_qtest

[Paolo Bonzini]

On 24/08/2017 09:42, Markus Armbruster wrote:
> 
> In a language less primitive than C, I'd write it exactly that way, and
> nobody would complain.  In old, primitive C, I have to write
> 
>     global_qtest = A;
>     do this
>     do that
> 
>     global_qtest = B;
>     do something
> 
>     global_qtest = A;
>     do more
> 
> Why's that so horrible to justify busywork on wrappers?

Because cut-and-paste is a thing. :(

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v5 08/13] tests: Rely more on global_qtest

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 24/08/2017 09:42, Markus Armbruster wrote:
>> 
>> In a language less primitive than C, I'd write it exactly that way, and
>> nobody would complain.  In old, primitive C, I have to write
>> 
>>     global_qtest = A;
>>     do this
>>     do that
>> 
>>     global_qtest = B;
>>     do something
>> 
>>     global_qtest = A;
>>     do more
>> 
>> Why's that so horrible to justify busywork on wrappers?
>
> Because cut-and-paste is a thing. :(

Cut-and-paste cuts both ways (pardon the pun):

    initialize with QTestState A
    frobnicate with QTestState A
    glomnify with QTestState A
    frobnicate with QTestState A
    initialize with QTestState B
    boingboing with QTestState B
    finalize with QTestState A
    finalize with QTestState A

Uh, forgot to frobnicate after boingboing, let me fix that real quick!

    initialize with QTestState A
    frobnicate with QTestState A
    glomnify with QTestState A
    frobnicate with QTestState A
    initialize with QTestState B
    boingboing with QTestState B
    frobnicate with QTestState A
    finalize with QTestState A
    finalize with QTestState A

Spot the pasto.

This hasty paste would simply work with global_qtest.  I'm not claiming
there are cases that are just the opposite.  I'm just challenging your
apparent claim that the long forms help with avoiding or catching
pastos.  Can you explain how they help more than they hurt?

A bit of global state isn't automatically bad.  *Shared* global state is
what gets us into trouble.  Can you point to examples where global_qtest
is shared in ways that aren't 100% trivial?

Re: [Qemu-devel] [Qemu-block] [PATCH v5 08/13] tests: Rely more on global_qtest

[Paolo Bonzini]

On 24/08/2017 12:09, Markus Armbruster wrote:
> Cut-and-paste cuts both ways (pardon the pun):
> 
>     initialize with QTestState A
>     frobnicate with QTestState A
>     glomnify with QTestState A
>     frobnicate with QTestState A
>     initialize with QTestState B
>     boingboing with QTestState B
>     finalize with QTestState A
>     finalize with QTestState A
> 
> Uh, forgot to frobnicate after boingboing, let me fix that real quick!
> 
>     initialize with QTestState A
>     frobnicate with QTestState A
>     glomnify with QTestState A
>     frobnicate with QTestState A
>     initialize with QTestState B
>     boingboing with QTestState B
>     frobnicate with QTestState A
>     finalize with QTestState A
>     finalize with QTestState A
> 
> Spot the pasto.
>
> This hasty paste would simply work with global_qtest.  I'm not claiming
> there are cases that are just the opposite.  I'm just challenging your
> apparent claim that the long forms help with avoiding or catching
> pastos.  Can you explain how they help more than they hurt?

A pasto without global_qtest is local.

But cut-and-paste that involves _assigning_ global_qtest, even if it
doesn't have cut-and-paste mistakes, may create a mess of functions
knowing^Wbelieving they know what global_qtest is.

Assigning global_qtest means the short-form functions have effectively
dynamic binding.  In a perfect world, global_qtest would be static and
the create-and-assign-global would assert(!global_qtest).  Then >1 VM ->
don't use the short forms.

Paolo

> A bit of global state isn't automatically bad.  *Shared* global state is
> what gets us into trouble.  Can you point to examples where global_qtest
> is shared in ways that aren't 100% trivial?

Re: [Qemu-devel] [Qemu-block] [PATCH v5 08/13] tests: Rely more on global_qtest

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 24/08/2017 12:09, Markus Armbruster wrote:
>> Cut-and-paste cuts both ways (pardon the pun):
>> 
>>     initialize with QTestState A
>>     frobnicate with QTestState A
>>     glomnify with QTestState A
>>     frobnicate with QTestState A
>>     initialize with QTestState B
>>     boingboing with QTestState B
>>     finalize with QTestState A
>>     finalize with QTestState A
>> 
>> Uh, forgot to frobnicate after boingboing, let me fix that real quick!
>> 
>>     initialize with QTestState A
>>     frobnicate with QTestState A
>>     glomnify with QTestState A
>>     frobnicate with QTestState A
>>     initialize with QTestState B
>>     boingboing with QTestState B
>>     frobnicate with QTestState A
>>     finalize with QTestState A
>>     finalize with QTestState A
>> 
>> Spot the pasto.
>>
>> This hasty paste would simply work with global_qtest.  I'm not claiming
>> there are cases that are just the opposite.  I'm just challenging your
>> apparent claim that the long forms help with avoiding or catching
>> pastos.  Can you explain how they help more than they hurt?
>
> A pasto without global_qtest is local.
>
> But cut-and-paste that involves _assigning_ global_qtest, even if it
> doesn't have cut-and-paste mistakes, may create a mess of functions
> knowing^Wbelieving they know what global_qtest is.
>
> Assigning global_qtest means the short-form functions have effectively
> dynamic binding.  In a perfect world, global_qtest would be static and
> the create-and-assign-global would assert(!global_qtest).  Then >1 VM ->
> don't use the short forms.

No consensus, status quo wins by default.

Since I find work of maintaining that status quo quite aggravating, I'll
do my best to dump it one someone with different sensibilities.

[Qemu-devel] [PATCH v5 09/13] libqtest: Shorten a couple more qtest_* functions

[Eric Blake]

qtest_rtas_call() and qtest_big_endian() did not have a short
version with an implied global_qtest; but changing these two
functions fits with the theme of the previous patch.

It doesn't hurt that we are now no longer ambiguous with the
qtest_rtas_call() of include/hw/ppc/spapr_rtos.h.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqos/virtio.h |  2 +-
 tests/libqtest.h      | 18 ++++++++----------
 tests/libqtest.c      | 13 ++++++-------
 tests/libqos/rtas.c   |  3 +--
 4 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
index 8fbcd1869c..7bca1b418a 100644
--- a/tests/libqos/virtio.h
+++ b/tests/libqos/virtio.h
@@ -93,7 +93,7 @@ struct QVirtioBus {
 static inline bool qvirtio_is_big_endian(QVirtioDevice *d)
 {
     /* FIXME: virtio 1.0 is always little-endian */
-    return qtest_big_endian(global_qtest);
+    return big_endian();
 }

 static inline uint32_t qvring_size(uint32_t num, uint32_t align)
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 3ae570927a..28945b3f7f 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -338,19 +338,17 @@ uint64_t qtest_readq(QTestState *s, uint64_t addr);
 void qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size);

 /**
- * qtest_rtas_call:
- * @s: #QTestState instance to operate on.
+ * rtas_call:
  * @name: name of the command to call.
  * @nargs: Number of args.
  * @args: Guest address to read args from.
  * @nret: Number of return value.
  * @ret: Guest address to write return values to.
  *
- * Call an RTAS function
+ * Call an RTAS function, using #global_qtest
  */
-uint64_t qtest_rtas_call(QTestState *s, const char *name,
-                         uint32_t nargs, uint64_t args,
-                         uint32_t nret, uint64_t ret);
+uint64_t rtas_call(const char *name, uint32_t nargs, uint64_t args,
+                   uint32_t nret, uint64_t ret);

 /**
  * qtest_bufread:
@@ -430,12 +428,12 @@ int64_t qtest_clock_step(QTestState *s, int64_t step);
 int64_t qtest_clock_set(QTestState *s, int64_t val);

 /**
- * qtest_big_endian:
- * @s: QTestState instance to operate on.
+ * big_endian:
  *
- * Returns: True if the architecture under test has a big endian configuration.
+ * Returns: True if the architecture under test, via #global_qtest,
+ * has a big endian configuration.
  */
-bool qtest_big_endian(QTestState *s);
+bool big_endian(void);

 /**
  * qtest_get_arch:
diff --git a/tests/libqtest.c b/tests/libqtest.c
index b6dd26e54a..261d86df5a 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -816,13 +816,12 @@ void qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size)
     g_strfreev(args);
 }

-uint64_t qtest_rtas_call(QTestState *s, const char *name,
-                         uint32_t nargs, uint64_t args,
-                         uint32_t nret, uint64_t ret)
+uint64_t rtas_call(const char *name, uint32_t nargs, uint64_t args,
+                   uint32_t nret, uint64_t ret)
 {
-    qtest_sendf(s, "rtas %s %u 0x%"PRIx64" %u 0x%"PRIx64"\n",
+    qtest_sendf(global_qtest, "rtas %s %u 0x%"PRIx64" %u 0x%"PRIx64"\n",
                 name, nargs, args, nret, ret);
-    qtest_rsp(s, 0);
+    qtest_rsp(global_qtest, 0);
     return 0;
 }

@@ -947,9 +946,9 @@ char *hmp(const char *fmt, ...)
     return ret;
 }

-bool qtest_big_endian(QTestState *s)
+bool big_endian(void)
 {
-    return s->big_endian;
+    return global_qtest->big_endian;
 }

 void qtest_cb_for_every_machine(void (*cb)(const char *machine))
diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
index 0269803ce0..e7ba7ab18f 100644
--- a/tests/libqos/rtas.c
+++ b/tests/libqos/rtas.c
@@ -37,8 +37,7 @@ static uint64_t qrtas_call(QGuestAllocator *alloc, const char *name,
     target_ret = guest_alloc(alloc, nret * sizeof(uint32_t));

     qrtas_copy_args(target_args, nargs, args);
-    res = qtest_rtas_call(global_qtest, name,
-                          nargs, target_args, nret, target_ret);
+    res = rtas_call(name, nargs, target_args, nret, target_ret);
     qrtas_copy_ret(target_ret, nret, ret);

     guest_free(alloc, target_ret);
-- 
2.13.5

[Qemu-devel] [PATCH v5 10/13] libqtest: Drop qtest_init() and qtest_qmp_discard_response()

[Eric Blake]

Most of our tests were using qtest_start() to initialize the
connection and then set global_qtest; the few tests that were
using qtest_init() instead are still setting global_qtest shortly
afterwards.  So it makes more sense to just always set
global_qtest, and have all callers go through a single entry
point.

Furthermore, we already have another qtest_init() in
include/sysemu/qtest.h, with a different signature - which makes
it hard to trace which version is being called based on what was
being included.

So, morph qtest_init() into qtest_start(), and hoist the setting
of global_qtest earlier during initialization; which in turn lets
the initialization sequence also benefit from the global variable.
For now, having a separate qtest_end() and qtest_quit() still
makes some tests easier, but hoisting the declaration so the two
are side-by-side makes the overall lifecycle management of
global_qtest easier to see.

While reworking things, prefer 0-initialization, and simplify the
initial handshake work; rendering qtest_qmp_discard_response()
unused.  As a bonus: it removes one spot passing an empty string
through varargs, so we are one step closer to using compile-time
format checking on qmp() without tripping over -Wformat-zero-length.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqtest.h        | 61 ++++++++++++-------------------------------------
 tests/libqtest.c        | 45 +++++++++++++-----------------------
 tests/postcopy-test.c   |  2 +-
 tests/qmp-test.c        |  2 +-
 tests/vhost-user-test.c |  3 ++-
 5 files changed, 35 insertions(+), 78 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index 28945b3f7f..dca62fd8da 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -24,20 +24,23 @@ typedef struct QTestState QTestState;
 extern QTestState *global_qtest;

 /**
- * qtest_init:
+ * qtest_start:
  * @extra_args: other arguments to pass to QEMU.
  *
+ * Start QEMU, and complete the QMP handshake. Sets #global_qtest, which
+ * is returned for convenience.
+ *
  * Returns: #QTestState instance.
  */
-QTestState *qtest_init(const char *extra_args);
+QTestState *qtest_start(const char *extra_args);

 /**
- * qtest_init_without_qmp_handshake:
+ * qtest_start_without_qmp_handshake:
  * @extra_args: other arguments to pass to QEMU.
  *
- * Returns: #QTestState instance.
+ * Starts the connection, but does no handshakes; sets #global_qtest.
  */
-QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
+void qtest_start_without_qmp_handshake(const char *extra_args);

 /**
  * qtest_quit:
@@ -48,13 +51,15 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
 void qtest_quit(QTestState *s);

 /**
- * qtest_qmp_discard_response:
- * @s: #QTestState instance to operate on.
- * @fmt...: QMP message to send to qemu
+ * qtest_end:
  *
- * Sends a QMP message to QEMU and consumes the response.
+ * Shut down the current #global_qtest QEMU process.
  */
-void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
+static inline void qtest_end(void)
+{
+    qtest_quit(global_qtest);
+    global_qtest = NULL;
+}

 /**
  * qtest_qmp:
@@ -75,16 +80,6 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
 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
- * @ap: QMP message arguments
- *
- * Sends a QMP message to QEMU and consumes the response.
- */
-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
@@ -506,32 +501,6 @@ void qtest_add_data_func_full(const char *str, void *data,
 void qtest_add_abrt_handler(GHookFunc fn, const void *data);

 /**
- * qtest_start:
- * @args: other arguments to pass to QEMU
- *
- * Start QEMU and assign the resulting #QTestState to a global variable.
- * The global variable is used by "shortcut" functions documented below.
- *
- * Returns: #QTestState instance.
- */
-static inline QTestState *qtest_start(const char *args)
-{
-    global_qtest = qtest_init(args);
-    return global_qtest;
-}
-
-/**
- * qtest_end:
- *
- * Shut down the QEMU process started by qtest_start().
- */
-static inline void qtest_end(void)
-{
-    qtest_quit(global_qtest);
-    global_qtest = NULL;
-}
-
-/**
  * qmp:
  * @fmt...: QMP message to send to qemu
  *
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 261d86df5a..c4e41261ab 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -266,7 +266,7 @@ void qtest_add_abrt_handler(GHookFunc fn, const void *data)
     g_hook_prepend(&abrt_hooks, hook);
 }

-QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
+void qtest_start_without_qmp_handshake(const char *extra_args)
 {
     QTestState *s;
     int sock, qmpsock, i;
@@ -282,7 +282,7 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
         exit(1);
     }

-    s = g_malloc(sizeof(*s));
+    global_qtest = s = g_malloc0(sizeof(*s));

     socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
     qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
@@ -338,24 +338,26 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
     }

     /* ask endianness of the target */
-    qtest_sendf(s, "endianness\n");
-    args = qtest_rsp(s, 1);
+    qtest_sendf(global_qtest, "endianness\n");
+    args = qtest_rsp(global_qtest, 1);
     g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
     s->big_endian = strcmp(args[1], "big") == 0;
     g_strfreev(args);
-
-    return s;
 }

-QTestState *qtest_init(const char *extra_args)
+QTestState *qtest_start(const char *extra_args)
 {
-    QTestState *s = qtest_init_without_qmp_handshake(extra_args);
+    QDict *rsp;
+
+    qtest_start_without_qmp_handshake(extra_args);

     /* Read the QMP greeting and then do the handshake */
-    qtest_qmp_discard_response(s, "");
-    qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
+    rsp = qmp_fd_receive(global_qtest->qmp_fd);
+    QDECREF(rsp);
+    rsp = qmp("{ 'execute': 'qmp_capabilities' }");
+    QDECREF(rsp);

-    return s;
+    return global_qtest;
 }

 void qtest_quit(QTestState *s)
@@ -539,23 +541,6 @@ void qtest_async_qmp(QTestState *s, const char *fmt, ...)
     va_end(ap);
 }

-void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap)
-{
-    QDict *response = qtest_qmpv(s, fmt, ap);
-    QDECREF(response);
-}
-
-void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...)
-{
-    va_list ap;
-    QDict *response;
-
-    va_start(ap, fmt);
-    response = qtest_qmpv(s, fmt, ap);
-    va_end(ap);
-    QDECREF(response);
-}
-
 QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event)
 {
     QDict *response;
@@ -930,10 +915,12 @@ void qmp_async(const char *fmt, ...)
 void qmp_discard_response(const char *fmt, ...)
 {
     va_list ap;
+    QDict *response;

     va_start(ap, fmt);
-    qtest_qmpv_discard_response(global_qtest, fmt, ap);
+    response = qtest_qmpv(global_qtest, fmt, ap);
     va_end(ap);
+    QDECREF(response);
 }
 char *hmp(const char *fmt, ...)
 {
diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index 9c4e37473d..91419ee3cb 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -404,7 +404,7 @@ static void test_migrate(void)
     from = qtest_start(cmd_src);
     g_free(cmd_src);

-    to = qtest_init(cmd_dst);
+    to = qtest_start(cmd_dst);
     g_free(cmd_dst);

     global_qtest = from;
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 5d0260b2be..3f1176f7f8 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -75,7 +75,7 @@ static void test_qmp_protocol(void)
     QDict *resp, *q, *ret;
     QList *capabilities;

-    global_qtest = qtest_init_without_qmp_handshake(common_args);
+    qtest_start_without_qmp_handshake(common_args);

     /* Test greeting */
     resp = qmp_receive();
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index d4da09f147..0518474b3a 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -646,7 +646,8 @@ static void test_migrate(void)
     g_assert_cmpint(size, ==, (2 * 1024 * 1024) / (VHOST_LOG_PAGE * 8));

     cmd = GET_QEMU_CMDE(dest, 2, "", " -incoming %s", uri);
-    to = qtest_init(cmd);
+    to = qtest_start(cmd);
+    global_qtest = from;
     g_free(cmd);

     source = g_source_new(&test_migrate_source_funcs,
-- 
2.13.5

[Qemu-devel] [PATCH v5 11/13] libqtest: Drop many static inline qtest_ wrappers

[Eric Blake]

None of the tests were calling the long qtest_*() form, except via
the static inline short forms.  Remove a layer of indirection by
only supporting the short form, and using global_qtest directly in
the .c file.  (Yes, this flies in the face of thread-safety, by
relying on a global instead of passing all state through parameters,
but ease of writing/maintaining tests trumps ivory-tower design, and
our tests aren't really multi-threaded.)

The list of affected functions (by their short name):

qmp_receive
qmp_eventwait
qmp_eventwait_ref
get_irq
irq_intercept_in
irq_intercept_out
outb
outw
outl
inb
inw
inl
writeb
writew
writel
writeq
readb
readw
readl
readq
memread
bufread
memwrite
bufwrite
qmemset
clock_step_next
clock_step
clock_set

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqtest.h | 563 ++++++++++---------------------------------------------
 tests/libqtest.c | 173 ++++++++---------
 2 files changed, 190 insertions(+), 546 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index dca62fd8da..431e546193 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -21,6 +21,15 @@

 typedef struct QTestState QTestState;

+/**
+ * global_qtest:
+ * The current test object.
+ *
+ * Many functions in this file implicitly operate on the current
+ * object; tests that need to alternate between two parallel
+ * connections can do so by switching which test state is current
+ * before issuing commands.
+ */
 extern QTestState *global_qtest;

 /**
@@ -46,7 +55,8 @@ void qtest_start_without_qmp_handshake(const char *extra_args);
  * qtest_quit:
  * @s: #QTestState instance to operate on.
  *
- * Shut down the QEMU process associated to @s.
+ * Shut down the QEMU process associated to @s.  See also qtest_end()
+ * for clearing #global_qtest.
  */
 void qtest_quit(QTestState *s);

@@ -100,31 +110,31 @@ QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
 void qtest_async_qmpv(QTestState *s, const char *fmt, va_list ap);

 /**
- * qtest_receive:
- * @s: #QTestState instance to operate on.
+ * qmp_receive:
  *
- * Reads a QMP message from QEMU and returns the response.
+ * Reads a QMP message from QEMU, using #global_qtest, and returns the
+ * response.
  */
-QDict *qtest_qmp_receive(QTestState *s);
+QDict *qmp_receive(void);

 /**
- * qtest_qmp_eventwait:
- * @s: #QTestState instance to operate on.
+ * qmp_eventwait:
  * @s: #event event to wait for.
  *
- * Continuously polls for QMP responses until it receives the desired event.
+ * Continuously polls for QMP responses, using #global_qtest, until it
+ * receives the desired event.
  */
-void qtest_qmp_eventwait(QTestState *s, const char *event);
+void qmp_eventwait(const char *event);

 /**
- * qtest_qmp_eventwait_ref:
- * @s: #QTestState instance to operate on.
+ * qmp_eventwait_ref:
  * @s: #event event to wait for.
  *
- * Continuously polls for QMP responses until it receives the desired event.
- * Returns a copy of the event for further investigation.
+ * Continuously polls for QMP responses, using #global_qtest, until it
+ * receives the desired event.  Returns a copy of the event for
+ * further investigation.
  */
-QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
+QDict *qmp_eventwait_ref(const char *event);

 /**
  * qtest_hmp:
@@ -152,185 +162,167 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...);
 char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap);

 /**
- * qtest_get_irq:
- * @s: #QTestState instance to operate on.
+ * get_irq:
  * @num: Interrupt to observe.
  *
- * Returns: The level of the @num interrupt.
+ * Returns: The level of the @num interrupt, using #global_qtest.
  */
-bool qtest_get_irq(QTestState *s, int num);
+bool get_irq(int num);

 /**
- * qtest_irq_intercept_in:
- * @s: #QTestState instance to operate on.
+ * irq_intercept_in:
  * @string: QOM path of a device.
  *
  * Associate qtest irqs with the GPIO-in pins of the device
- * whose path is specified by @string.
+ * whose path is specified by @string, using #global_qtest.
  */
-void qtest_irq_intercept_in(QTestState *s, const char *string);
+void irq_intercept_in(const char *string);

 /**
- * qtest_irq_intercept_out:
- * @s: #QTestState instance to operate on.
+ * irq_intercept_out:
  * @string: QOM path of a device.
  *
  * Associate qtest irqs with the GPIO-out pins of the device
- * whose path is specified by @string.
+ * whose path is specified by @string, using #global_qtest.
  */
-void qtest_irq_intercept_out(QTestState *s, const char *string);
+void irq_intercept_out(const char *string);

 /**
- * qtest_outb:
- * @s: #QTestState instance to operate on.
+ * outb:
  * @addr: I/O port to write to.
  * @value: Value being written.
  *
- * Write an 8-bit value to an I/O port.
+ * Write an 8-bit value to an I/O port, using #global_qtest.
  */
-void qtest_outb(QTestState *s, uint16_t addr, uint8_t value);
+void outb(uint16_t addr, uint8_t value);

 /**
- * qtest_outw:
- * @s: #QTestState instance to operate on.
+ * outw:
  * @addr: I/O port to write to.
  * @value: Value being written.
  *
- * Write a 16-bit value to an I/O port.
+ * Write a 16-bit value to an I/O port, using #global_qtest.
  */
-void qtest_outw(QTestState *s, uint16_t addr, uint16_t value);
+void outw(uint16_t addr, uint16_t value);

 /**
- * qtest_outl:
- * @s: #QTestState instance to operate on.
+ * outl:
  * @addr: I/O port to write to.
  * @value: Value being written.
  *
- * Write a 32-bit value to an I/O port.
+ * Write a 32-bit value to an I/O port, using #global_qtest.
  */
-void qtest_outl(QTestState *s, uint16_t addr, uint32_t value);
+void outl(uint16_t addr, uint32_t value);

 /**
- * qtest_inb:
- * @s: #QTestState instance to operate on.
+ * inb:
  * @addr: I/O port to read from.
  *
- * Returns an 8-bit value from an I/O port.
+ * Returns an 8-bit value from an I/O port, using #global_qtest.
  */
-uint8_t qtest_inb(QTestState *s, uint16_t addr);
+uint8_t inb(uint16_t addr);

 /**
- * qtest_inw:
- * @s: #QTestState instance to operate on.
+ * inw:
  * @addr: I/O port to read from.
  *
- * Returns a 16-bit value from an I/O port.
+ * Returns a 16-bit value from an I/O port, using #global_qtest.
  */
-uint16_t qtest_inw(QTestState *s, uint16_t addr);
+uint16_t inw(uint16_t addr);

 /**
- * qtest_inl:
- * @s: #QTestState instance to operate on.
+ * inl:
  * @addr: I/O port to read from.
  *
- * Returns a 32-bit value from an I/O port.
+ * Returns a 32-bit value from an I/O port, using #global_qtest.
  */
-uint32_t qtest_inl(QTestState *s, uint16_t addr);
+uint32_t inl(uint16_t addr);

 /**
- * qtest_writeb:
- * @s: #QTestState instance to operate on.
+ * writeb:
  * @addr: Guest address to write to.
  * @value: Value being written.
  *
- * Writes an 8-bit value to memory.
+ * Writes an 8-bit value to memory, using #global_qtest.
  */
-void qtest_writeb(QTestState *s, uint64_t addr, uint8_t value);
+void writeb(uint64_t addr, uint8_t value);

 /**
- * qtest_writew:
- * @s: #QTestState instance to operate on.
+ * writew:
  * @addr: Guest address to write to.
  * @value: Value being written.
  *
- * Writes a 16-bit value to memory.
+ * Writes a 16-bit value to memory, using #global_qtest.
  */
-void qtest_writew(QTestState *s, uint64_t addr, uint16_t value);
+void writew(uint64_t addr, uint16_t value);

 /**
- * qtest_writel:
- * @s: #QTestState instance to operate on.
+ * writel:
  * @addr: Guest address to write to.
  * @value: Value being written.
  *
- * Writes a 32-bit value to memory.
+ * Writes a 32-bit value to memory, using #global_qtest.
  */
-void qtest_writel(QTestState *s, uint64_t addr, uint32_t value);
+void writel(uint64_t addr, uint32_t value);

 /**
- * qtest_writeq:
- * @s: #QTestState instance to operate on.
+ * writeq:
  * @addr: Guest address to write to.
  * @value: Value being written.
  *
- * Writes a 64-bit value to memory.
+ * Writes a 64-bit value to memory, using #global_qtest.
  */
-void qtest_writeq(QTestState *s, uint64_t addr, uint64_t value);
+void writeq(uint64_t addr, uint64_t value);

 /**
- * qtest_readb:
- * @s: #QTestState instance to operate on.
+ * readb:
  * @addr: Guest address to read from.
  *
- * Reads an 8-bit value from memory.
+ * Reads an 8-bit value from memory, using #global_qtest.
  *
  * Returns: Value read.
  */
-uint8_t qtest_readb(QTestState *s, uint64_t addr);
+uint8_t readb(uint64_t addr);

 /**
- * qtest_readw:
- * @s: #QTestState instance to operate on.
+ * readw:
  * @addr: Guest address to read from.
  *
- * Reads a 16-bit value from memory.
+ * Reads a 16-bit value from memory, using #global_qtest.
  *
  * Returns: Value read.
  */
-uint16_t qtest_readw(QTestState *s, uint64_t addr);
+uint16_t readw(uint64_t addr);

 /**
- * qtest_readl:
- * @s: #QTestState instance to operate on.
+ * readl:
  * @addr: Guest address to read from.
  *
- * Reads a 32-bit value from memory.
+ * Reads a 32-bit value from memory, using #global_qtest.
  *
  * Returns: Value read.
  */
-uint32_t qtest_readl(QTestState *s, uint64_t addr);
+uint32_t readl(uint64_t addr);

 /**
- * qtest_readq:
- * @s: #QTestState instance to operate on.
+ * readq:
  * @addr: Guest address to read from.
  *
- * Reads a 64-bit value from memory.
+ * Reads a 64-bit value from memory, using #global_qtest.
  *
  * Returns: Value read.
  */
-uint64_t qtest_readq(QTestState *s, uint64_t addr);
+uint64_t readq(uint64_t addr);

 /**
- * qtest_memread:
- * @s: #QTestState instance to operate on.
+ * memread:
  * @addr: Guest address to read from.
  * @data: Pointer to where memory contents will be stored.
  * @size: Number of bytes to read.
  *
- * Read guest memory into a buffer.
+ * Read guest memory into a buffer, using #global_qtest.
  */
-void qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size);
+void memread(uint64_t addr, void *data, size_t size);

 /**
  * rtas_call:
@@ -346,81 +338,76 @@ uint64_t rtas_call(const char *name, uint32_t nargs, uint64_t args,
                    uint32_t nret, uint64_t ret);

 /**
- * qtest_bufread:
- * @s: #QTestState instance to operate on.
+ * bufread:
  * @addr: Guest address to read from.
  * @data: Pointer to where memory contents will be stored.
  * @size: Number of bytes to read.
  *
- * Read guest memory into a buffer and receive using a base64 encoding.
+ * Read guest memory into a buffer and receive using a base64
+ * encoding, using #global_qtest.
  */
-void qtest_bufread(QTestState *s, uint64_t addr, void *data, size_t size);
+void bufread(uint64_t addr, void *data, size_t size);

 /**
- * qtest_memwrite:
- * @s: #QTestState instance to operate on.
+ * memwrite:
  * @addr: Guest address to write to.
  * @data: Pointer to the bytes that will be written to guest memory.
  * @size: Number of bytes to write.
  *
- * Write a buffer to guest memory.
+ * Write a buffer to guest memory, using #global_qtest.
  */
-void qtest_memwrite(QTestState *s, uint64_t addr, const void *data, size_t size);
+void memwrite(uint64_t addr, const void *data, size_t size);

 /**
- * qtest_bufwrite:
- * @s: #QTestState instance to operate on.
+ * bufwrite:
  * @addr: Guest address to write to.
  * @data: Pointer to the bytes that will be written to guest memory.
  * @size: Number of bytes to write.
  *
- * Write a buffer to guest memory and transmit using a base64 encoding.
+ * Write a buffer to guest memory and transmit using a base64
+ * encoding, using #global_qtest.
  */
-void qtest_bufwrite(QTestState *s, uint64_t addr,
-                    const void *data, size_t size);
+void bufwrite(uint64_t addr, const void *data, size_t size);

 /**
- * qtest_memset:
- * @s: #QTestState instance to operate on.
+ * qmemset:
  * @addr: Guest address to write to.
  * @patt: Byte pattern to fill the guest memory region with.
  * @size: Number of bytes to write.
  *
- * Write a pattern to guest memory.
+ * Write a pattern to guest memory, using #global_qtest.
  */
-void qtest_memset(QTestState *s, uint64_t addr, uint8_t patt, size_t size);
+void qmemset(uint64_t addr, uint8_t patt, size_t size);

 /**
- * qtest_clock_step_next:
- * @s: #QTestState instance to operate on.
+ * clock_step_next:
  *
- * Advance the QEMU_CLOCK_VIRTUAL to the next deadline.
+ * Advance the QEMU_CLOCK_VIRTUAL to the next deadline, using #global_qtest.
  *
  * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds.
  */
-int64_t qtest_clock_step_next(QTestState *s);
+int64_t clock_step_next(void);

 /**
- * qtest_clock_step:
- * @s: QTestState instance to operate on.
+ * clock_step:
  * @step: Number of nanoseconds to advance the clock by.
  *
- * Advance the QEMU_CLOCK_VIRTUAL by @step nanoseconds.
+ * Advance the QEMU_CLOCK_VIRTUAL by @step nanoseconds, using #global_qtest.
  *
  * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds.
  */
-int64_t qtest_clock_step(QTestState *s, int64_t step);
+int64_t clock_step(int64_t step);

 /**
- * qtest_clock_set:
- * @s: QTestState instance to operate on.
+ * clock_set:
  * @val: Nanoseconds value to advance the clock to.
  *
- * Advance the QEMU_CLOCK_VIRTUAL to @val nanoseconds since the VM was launched.
+ * Advance the QEMU_CLOCK_VIRTUAL to @val nanoseconds since the VM was
+ * launched, using #global_qtest.
  *
  * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds.
  */
-int64_t qtest_clock_set(QTestState *s, int64_t val);
+int64_t clock_set(int64_t val);

 /**
  * big_endian:
@@ -525,39 +512,6 @@ void qmp_async(const char *fmt, ...);
 void qmp_discard_response(const char *fmt, ...);

 /**
- * qmp_receive:
- *
- * Reads a QMP message from QEMU and returns the response.
- */
-static inline QDict *qmp_receive(void)
-{
-    return qtest_qmp_receive(global_qtest);
-}
-
-/**
- * qmp_eventwait:
- * @s: #event event to wait for.
- *
- * Continuously polls for QMP responses until it receives the desired event.
- */
-static inline void qmp_eventwait(const char *event)
-{
-    return qtest_qmp_eventwait(global_qtest, event);
-}
-
-/**
- * qmp_eventwait_ref:
- * @s: #event event to wait for.
- *
- * Continuously polls for QMP responses until it receives the desired event.
- * Returns a copy of the event for further investigation.
- */
-static inline QDict *qmp_eventwait_ref(const char *event)
-{
-    return qtest_qmp_eventwait_ref(global_qtest, event);
-}
-
-/**
  * hmp:
  * @fmt...: HMP command to send to QEMU
  *
@@ -567,319 +521,6 @@ static inline QDict *qmp_eventwait_ref(const char *event)
  */
 char *hmp(const char *fmt, ...);

-/**
- * get_irq:
- * @num: Interrupt to observe.
- *
- * Returns: The level of the @num interrupt.
- */
-static inline bool get_irq(int num)
-{
-    return qtest_get_irq(global_qtest, num);
-}
-
-/**
- * irq_intercept_in:
- * @string: QOM path of a device.
- *
- * Associate qtest irqs with the GPIO-in pins of the device
- * whose path is specified by @string.
- */
-static inline void irq_intercept_in(const char *string)
-{
-    qtest_irq_intercept_in(global_qtest, string);
-}
-
-/**
- * qtest_irq_intercept_out:
- * @string: QOM path of a device.
- *
- * Associate qtest irqs with the GPIO-out pins of the device
- * whose path is specified by @string.
- */
-static inline void irq_intercept_out(const char *string)
-{
-    qtest_irq_intercept_out(global_qtest, string);
-}
-
-/**
- * outb:
- * @addr: I/O port to write to.
- * @value: Value being written.
- *
- * Write an 8-bit value to an I/O port.
- */
-static inline void outb(uint16_t addr, uint8_t value)
-{
-    qtest_outb(global_qtest, addr, value);
-}
-
-/**
- * outw:
- * @addr: I/O port to write to.
- * @value: Value being written.
- *
- * Write a 16-bit value to an I/O port.
- */
-static inline void outw(uint16_t addr, uint16_t value)
-{
-    qtest_outw(global_qtest, addr, value);
-}
-
-/**
- * outl:
- * @addr: I/O port to write to.
- * @value: Value being written.
- *
- * Write a 32-bit value to an I/O port.
- */
-static inline void outl(uint16_t addr, uint32_t value)
-{
-    qtest_outl(global_qtest, addr, value);
-}
-
-/**
- * inb:
- * @addr: I/O port to read from.
- *
- * Reads an 8-bit value from an I/O port.
- *
- * Returns: Value read.
- */
-static inline uint8_t inb(uint16_t addr)
-{
-    return qtest_inb(global_qtest, addr);
-}
-
-/**
- * inw:
- * @addr: I/O port to read from.
- *
- * Reads a 16-bit value from an I/O port.
- *
- * Returns: Value read.
- */
-static inline uint16_t inw(uint16_t addr)
-{
-    return qtest_inw(global_qtest, addr);
-}
-
-/**
- * inl:
- * @addr: I/O port to read from.
- *
- * Reads a 32-bit value from an I/O port.
- *
- * Returns: Value read.
- */
-static inline uint32_t inl(uint16_t addr)
-{
-    return qtest_inl(global_qtest, addr);
-}
-
-/**
- * writeb:
- * @addr: Guest address to write to.
- * @value: Value being written.
- *
- * Writes an 8-bit value to guest memory.
- */
-static inline void writeb(uint64_t addr, uint8_t value)
-{
-    qtest_writeb(global_qtest, addr, value);
-}
-
-/**
- * writew:
- * @addr: Guest address to write to.
- * @value: Value being written.
- *
- * Writes a 16-bit value to guest memory.
- */
-static inline void writew(uint64_t addr, uint16_t value)
-{
-    qtest_writew(global_qtest, addr, value);
-}
-
-/**
- * writel:
- * @addr: Guest address to write to.
- * @value: Value being written.
- *
- * Writes a 32-bit value to guest memory.
- */
-static inline void writel(uint64_t addr, uint32_t value)
-{
-    qtest_writel(global_qtest, addr, value);
-}
-
-/**
- * writeq:
- * @addr: Guest address to write to.
- * @value: Value being written.
- *
- * Writes a 64-bit value to guest memory.
- */
-static inline void writeq(uint64_t addr, uint64_t value)
-{
-    qtest_writeq(global_qtest, addr, value);
-}
-
-/**
- * readb:
- * @addr: Guest address to read from.
- *
- * Reads an 8-bit value from guest memory.
- *
- * Returns: Value read.
- */
-static inline uint8_t readb(uint64_t addr)
-{
-    return qtest_readb(global_qtest, addr);
-}
-
-/**
- * readw:
- * @addr: Guest address to read from.
- *
- * Reads a 16-bit value from guest memory.
- *
- * Returns: Value read.
- */
-static inline uint16_t readw(uint64_t addr)
-{
-    return qtest_readw(global_qtest, addr);
-}
-
-/**
- * readl:
- * @addr: Guest address to read from.
- *
- * Reads a 32-bit value from guest memory.
- *
- * Returns: Value read.
- */
-static inline uint32_t readl(uint64_t addr)
-{
-    return qtest_readl(global_qtest, addr);
-}
-
-/**
- * readq:
- * @addr: Guest address to read from.
- *
- * Reads a 64-bit value from guest memory.
- *
- * Returns: Value read.
- */
-static inline uint64_t readq(uint64_t addr)
-{
-    return qtest_readq(global_qtest, addr);
-}
-
-/**
- * memread:
- * @addr: Guest address to read from.
- * @data: Pointer to where memory contents will be stored.
- * @size: Number of bytes to read.
- *
- * Read guest memory into a buffer.
- */
-static inline void memread(uint64_t addr, void *data, size_t size)
-{
-    qtest_memread(global_qtest, addr, data, size);
-}
-
-/**
- * bufread:
- * @addr: Guest address to read from.
- * @data: Pointer to where memory contents will be stored.
- * @size: Number of bytes to read.
- *
- * Read guest memory into a buffer, receive using a base64 encoding.
- */
-static inline void bufread(uint64_t addr, void *data, size_t size)
-{
-    qtest_bufread(global_qtest, addr, data, size);
-}
-
-/**
- * memwrite:
- * @addr: Guest address to write to.
- * @data: Pointer to the bytes that will be written to guest memory.
- * @size: Number of bytes to write.
- *
- * Write a buffer to guest memory.
- */
-static inline void memwrite(uint64_t addr, const void *data, size_t size)
-{
-    qtest_memwrite(global_qtest, addr, data, size);
-}
-
-/**
- * bufwrite:
- * @addr: Guest address to write to.
- * @data: Pointer to the bytes that will be written to guest memory.
- * @size: Number of bytes to write.
- *
- * Write a buffer to guest memory, transmit using a base64 encoding.
- */
-static inline void bufwrite(uint64_t addr, const void *data, size_t size)
-{
-    qtest_bufwrite(global_qtest, addr, data, size);
-}
-
-/**
- * qmemset:
- * @addr: Guest address to write to.
- * @patt: Byte pattern to fill the guest memory region with.
- * @size: Number of bytes to write.
- *
- * Write a pattern to guest memory.
- */
-static inline void qmemset(uint64_t addr, uint8_t patt, size_t size)
-{
-    qtest_memset(global_qtest, addr, patt, size);
-}
-
-/**
- * clock_step_next:
- *
- * Advance the QEMU_CLOCK_VIRTUAL to the next deadline.
- *
- * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds.
- */
-static inline int64_t clock_step_next(void)
-{
-    return qtest_clock_step_next(global_qtest);
-}
-
-/**
- * clock_step:
- * @step: Number of nanoseconds to advance the clock by.
- *
- * Advance the QEMU_CLOCK_VIRTUAL by @step nanoseconds.
- *
- * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds.
- */
-static inline int64_t clock_step(int64_t step)
-{
-    return qtest_clock_step(global_qtest, step);
-}
-
-/**
- * clock_set:
- * @val: Nanoseconds value to advance the clock to.
- *
- * Advance the QEMU_CLOCK_VIRTUAL to @val nanoseconds since the VM was launched.
- *
- * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds.
- */
-static inline int64_t clock_set(int64_t val)
-{
-    return qtest_clock_set(global_qtest, 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, ...);
diff --git a/tests/libqtest.c b/tests/libqtest.c
index c4e41261ab..2998b173f0 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -426,9 +426,9 @@ QDict *qmp_fd_receive(int fd)
     return qmp.response;
 }

-QDict *qtest_qmp_receive(QTestState *s)
+QDict *qmp_receive(void)
 {
-    return qmp_fd_receive(s->qmp_fd);
+    return qmp_fd_receive(global_qtest->qmp_fd);
 }

 /**
@@ -498,7 +498,7 @@ QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
     qtest_async_qmpv(s, fmt, ap);

     /* Receive reply */
-    return qtest_qmp_receive(s);
+    return qmp_fd_receive(s->qmp_fd);
 }

 QDict *qmp_fd(int fd, const char *fmt, ...)
@@ -541,12 +541,12 @@ void qtest_async_qmp(QTestState *s, const char *fmt, ...)
     va_end(ap);
 }

-QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event)
+QDict *qmp_eventwait_ref(const char *event)
 {
     QDict *response;

     for (;;) {
-        response = qtest_qmp_receive(s);
+        response = qmp_receive();
         if ((qdict_haskey(response, "event")) &&
             (strcmp(qdict_get_str(response, "event"), event) == 0)) {
             return response;
@@ -555,11 +555,11 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event)
     }
 }

-void qtest_qmp_eventwait(QTestState *s, const char *event)
+void qmp_eventwait(const char *event)
 {
     QDict *response;

-    response = qtest_qmp_eventwait_ref(s, event);
+    response = qmp_eventwait_ref(event);
     QDECREF(response);
 }

@@ -577,7 +577,7 @@ char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap)
     while (ret == NULL && qdict_get_try_str(resp, "event")) {
         /* Ignore asynchronous QMP events */
         QDECREF(resp);
-        resp = qtest_qmp_receive(s);
+        resp = qmp_fd_receive(s->qmp_fd);
         ret = g_strdup(qdict_get_try_str(resp, "return"));
     }
     g_assert(ret);
@@ -606,83 +606,83 @@ const char *qtest_get_arch(void)
     return end + strlen("/qemu-system-");
 }

-bool qtest_get_irq(QTestState *s, int num)
+bool get_irq(int num)
 {
     /* dummy operation in order to make sure irq is up to date */
-    qtest_inb(s, 0);
+    inb(0);

-    return s->irq_level[num];
+    return global_qtest->irq_level[num];
 }

-static int64_t qtest_clock_rsp(QTestState *s)
+static int64_t qtest_clock_rsp(void)
 {
     gchar **words;
     int64_t clock;
-    words = qtest_rsp(s, 2);
+    words = qtest_rsp(global_qtest, 2);
     clock = g_ascii_strtoll(words[1], NULL, 0);
     g_strfreev(words);
     return clock;
 }

-int64_t qtest_clock_step_next(QTestState *s)
+int64_t clock_step_next(void)
 {
-    qtest_sendf(s, "clock_step\n");
-    return qtest_clock_rsp(s);
+    qtest_sendf(global_qtest, "clock_step\n");
+    return qtest_clock_rsp();
 }

-int64_t qtest_clock_step(QTestState *s, int64_t step)
+int64_t clock_step(int64_t step)
 {
-    qtest_sendf(s, "clock_step %"PRIi64"\n", step);
-    return qtest_clock_rsp(s);
+    qtest_sendf(global_qtest, "clock_step %"PRIi64"\n", step);
+    return qtest_clock_rsp();
 }

-int64_t qtest_clock_set(QTestState *s, int64_t val)
+int64_t clock_set(int64_t val)
 {
-    qtest_sendf(s, "clock_set %"PRIi64"\n", val);
-    return qtest_clock_rsp(s);
+    qtest_sendf(global_qtest, "clock_set %"PRIi64"\n", val);
+    return qtest_clock_rsp();
 }

-void qtest_irq_intercept_out(QTestState *s, const char *qom_path)
+void irq_intercept_out(const char *qom_path)
 {
-    qtest_sendf(s, "irq_intercept_out %s\n", qom_path);
-    qtest_rsp(s, 0);
+    qtest_sendf(global_qtest, "irq_intercept_out %s\n", qom_path);
+    qtest_rsp(global_qtest, 0);
 }

-void qtest_irq_intercept_in(QTestState *s, const char *qom_path)
+void irq_intercept_in(const char *qom_path)
 {
-    qtest_sendf(s, "irq_intercept_in %s\n", qom_path);
-    qtest_rsp(s, 0);
+    qtest_sendf(global_qtest, "irq_intercept_in %s\n", qom_path);
+    qtest_rsp(global_qtest, 0);
 }

-static void qtest_out(QTestState *s, const char *cmd, uint16_t addr, uint32_t value)
+static void out(const char *cmd, uint16_t addr, uint32_t value)
 {
-    qtest_sendf(s, "%s 0x%x 0x%x\n", cmd, addr, value);
-    qtest_rsp(s, 0);
+    qtest_sendf(global_qtest, "%s 0x%x 0x%x\n", cmd, addr, value);
+    qtest_rsp(global_qtest, 0);
 }

-void qtest_outb(QTestState *s, uint16_t addr, uint8_t value)
+void outb(uint16_t addr, uint8_t value)
 {
-    qtest_out(s, "outb", addr, value);
+    out("outb", addr, value);
 }

-void qtest_outw(QTestState *s, uint16_t addr, uint16_t value)
+void outw(uint16_t addr, uint16_t value)
 {
-    qtest_out(s, "outw", addr, value);
+    out("outw", addr, value);
 }

-void qtest_outl(QTestState *s, uint16_t addr, uint32_t value)
+void outl(uint16_t addr, uint32_t value)
 {
-    qtest_out(s, "outl", addr, value);
+    out("outl", addr, value);
 }

-static uint32_t qtest_in(QTestState *s, const char *cmd, uint16_t addr)
+static uint32_t in(const char *cmd, uint16_t addr)
 {
     gchar **args;
     int ret;
     unsigned long value;

-    qtest_sendf(s, "%s 0x%x\n", cmd, addr);
-    args = qtest_rsp(s, 2);
+    qtest_sendf(global_qtest, "%s 0x%x\n", cmd, addr);
+    args = qtest_rsp(global_qtest, 2);
     ret = qemu_strtoul(args[1], NULL, 0, &value);
     g_assert(!ret && value <= UINT32_MAX);
     g_strfreev(args);
@@ -690,56 +690,57 @@ static uint32_t qtest_in(QTestState *s, const char *cmd, uint16_t addr)
     return value;
 }

-uint8_t qtest_inb(QTestState *s, uint16_t addr)
+uint8_t inb(uint16_t addr)
 {
-    return qtest_in(s, "inb", addr);
+    return in("inb", addr);
 }

-uint16_t qtest_inw(QTestState *s, uint16_t addr)
+uint16_t inw(uint16_t addr)
 {
-    return qtest_in(s, "inw", addr);
+    return in("inw", addr);
 }

-uint32_t qtest_inl(QTestState *s, uint16_t addr)
+uint32_t inl(uint16_t addr)
 {
-    return qtest_in(s, "inl", addr);
+    return in("inl", addr);
 }

-static void qtest_write(QTestState *s, const char *cmd, uint64_t addr,
+static void qtest_write(const char *cmd, uint64_t addr,
                         uint64_t value)
 {
-    qtest_sendf(s, "%s 0x%" PRIx64 " 0x%" PRIx64 "\n", cmd, addr, value);
-    qtest_rsp(s, 0);
+    qtest_sendf(global_qtest, "%s 0x%" PRIx64 " 0x%" PRIx64 "\n", cmd, addr,
+                value);
+    qtest_rsp(global_qtest, 0);
 }

-void qtest_writeb(QTestState *s, uint64_t addr, uint8_t value)
+void writeb(uint64_t addr, uint8_t value)
 {
-    qtest_write(s, "writeb", addr, value);
+    qtest_write("writeb", addr, value);
 }

-void qtest_writew(QTestState *s, uint64_t addr, uint16_t value)
+void writew(uint64_t addr, uint16_t value)
 {
-    qtest_write(s, "writew", addr, value);
+    qtest_write("writew", addr, value);
 }

-void qtest_writel(QTestState *s, uint64_t addr, uint32_t value)
+void writel(uint64_t addr, uint32_t value)
 {
-    qtest_write(s, "writel", addr, value);
+    qtest_write("writel", addr, value);
 }

-void qtest_writeq(QTestState *s, uint64_t addr, uint64_t value)
+void writeq(uint64_t addr, uint64_t value)
 {
-    qtest_write(s, "writeq", addr, value);
+    qtest_write("writeq", addr, value);
 }

-static uint64_t qtest_read(QTestState *s, const char *cmd, uint64_t addr)
+static uint64_t qtest_read(const char *cmd, uint64_t addr)
 {
     gchar **args;
     int ret;
     uint64_t value;

-    qtest_sendf(s, "%s 0x%" PRIx64 "\n", cmd, addr);
-    args = qtest_rsp(s, 2);
+    qtest_sendf(global_qtest, "%s 0x%" PRIx64 "\n", cmd, addr);
+    args = qtest_rsp(global_qtest, 2);
     ret = qemu_strtou64(args[1], NULL, 0, &value);
     g_assert(!ret);
     g_strfreev(args);
@@ -747,24 +748,24 @@ static uint64_t qtest_read(QTestState *s, const char *cmd, uint64_t addr)
     return value;
 }

-uint8_t qtest_readb(QTestState *s, uint64_t addr)
+uint8_t readb(uint64_t addr)
 {
-    return qtest_read(s, "readb", addr);
+    return qtest_read("readb", addr);
 }

-uint16_t qtest_readw(QTestState *s, uint64_t addr)
+uint16_t readw(uint64_t addr)
 {
-    return qtest_read(s, "readw", addr);
+    return qtest_read("readw", addr);
 }

-uint32_t qtest_readl(QTestState *s, uint64_t addr)
+uint32_t readl(uint64_t addr)
 {
-    return qtest_read(s, "readl", addr);
+    return qtest_read("readl", addr);
 }

-uint64_t qtest_readq(QTestState *s, uint64_t addr)
+uint64_t readq(uint64_t addr)
 {
-    return qtest_read(s, "readq", addr);
+    return qtest_read("readq", addr);
 }

 static int hex2nib(char ch)
@@ -780,7 +781,7 @@ static int hex2nib(char ch)
     }
 }

-void qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size)
+void memread(uint64_t addr, void *data, size_t size)
 {
     uint8_t *ptr = data;
     gchar **args;
@@ -790,8 +791,8 @@ void qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size)
         return;
     }

-    qtest_sendf(s, "read 0x%" PRIx64 " 0x%zx\n", addr, size);
-    args = qtest_rsp(s, 2);
+    qtest_sendf(global_qtest, "read 0x%" PRIx64 " 0x%zx\n", addr, size);
+    args = qtest_rsp(global_qtest, 2);

     for (i = 0; i < size; i++) {
         ptr[i] = hex2nib(args[1][2 + (i * 2)]) << 4;
@@ -834,25 +835,25 @@ void qtest_add_data_func(const char *str, const void *data,
     g_free(path);
 }

-void qtest_bufwrite(QTestState *s, uint64_t addr, const void *data, size_t size)
+void bufwrite(uint64_t addr, const void *data, size_t size)
 {
     gchar *bdata;

     bdata = g_base64_encode(data, size);
-    qtest_sendf(s, "b64write 0x%" PRIx64 " 0x%zx ", addr, size);
-    socket_send(s->fd, bdata, -1);
-    socket_send(s->fd, "\n", 1);
-    qtest_rsp(s, 0);
+    qtest_sendf(global_qtest, "b64write 0x%" PRIx64 " 0x%zx ", addr, size);
+    socket_send(global_qtest->fd, bdata, -1);
+    socket_send(global_qtest->fd, "\n", 1);
+    qtest_rsp(global_qtest, 0);
     g_free(bdata);
 }

-void qtest_bufread(QTestState *s, uint64_t addr, void *data, size_t size)
+void bufread(uint64_t addr, void *data, size_t size)
 {
     gchar **args;
     size_t len;

-    qtest_sendf(s, "b64read 0x%" PRIx64 " 0x%zx\n", addr, size);
-    args = qtest_rsp(s, 2);
+    qtest_sendf(global_qtest, "b64read 0x%" PRIx64 " 0x%zx\n", addr, size);
+    args = qtest_rsp(global_qtest, 2);

     g_base64_decode_inplace(args[1], &len);
     if (size != len) {
@@ -865,7 +866,7 @@ void qtest_bufread(QTestState *s, uint64_t addr, void *data, size_t size)
     g_strfreev(args);
 }

-void qtest_memwrite(QTestState *s, uint64_t addr, const void *data, size_t size)
+void memwrite(uint64_t addr, const void *data, size_t size)
 {
     const uint8_t *ptr = data;
     size_t i;
@@ -881,15 +882,17 @@ void qtest_memwrite(QTestState *s, uint64_t addr, const void *data, size_t size)
         sprintf(&enc[i * 2], "%02x", ptr[i]);
     }

-    qtest_sendf(s, "write 0x%" PRIx64 " 0x%zx 0x%s\n", addr, size, enc);
-    qtest_rsp(s, 0);
+    qtest_sendf(global_qtest, "write 0x%" PRIx64 " 0x%zx 0x%s\n", addr, size,
+                enc);
+    qtest_rsp(global_qtest, 0);
     g_free(enc);
 }

-void qtest_memset(QTestState *s, uint64_t addr, uint8_t pattern, size_t size)
+void qmemset(uint64_t addr, uint8_t pattern, size_t size)
 {
-    qtest_sendf(s, "memset 0x%" PRIx64 " 0x%zx 0x%02x\n", addr, size, pattern);
-    qtest_rsp(s, 0);
+    qtest_sendf(global_qtest, "memset 0x%" PRIx64 " 0x%zx 0x%02x\n", addr, size,
+                pattern);
+    qtest_rsp(global_qtest, 0);
 }

 QDict *qmp(const char *fmt, ...)
-- 
2.13.5

[Qemu-devel] [PATCH v5 12/13] libqtest: Use global_qtest in qtest_sendf() and qtest_rsp()

[Eric Blake]

All callers are now passing global_qtest, just remove the parameter
instead.  While at it, improve the naming of the va_list variant
of socket_sendf() to be more like the printf/vprintf naming scheme.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqtest.c | 77 +++++++++++++++++++++++++++-----------------------------
 1 file changed, 37 insertions(+), 40 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 2998b173f0..0ec8668c88 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -150,7 +150,7 @@ static void socket_send(int fd, const char *buf, ssize_t size)
     }
 }

-static void socket_sendf(int fd, const char *fmt, va_list ap)
+static void socket_vsendf(int fd, const char *fmt, va_list ap)
 {
     gchar *str = g_strdup_vprintf(fmt, ap);

@@ -158,12 +158,12 @@ static void socket_sendf(int fd, const char *fmt, va_list ap)
     g_free(str);
 }

-static void GCC_FMT_ATTR(2, 3) qtest_sendf(QTestState *s, const char *fmt, ...)
+static void GCC_FMT_ATTR(1, 2) qtest_sendf(const char *fmt, ...)
 {
     va_list ap;

     va_start(ap, fmt);
-    socket_sendf(s->fd, fmt, ap);
+    socket_vsendf(global_qtest->fd, fmt, ap);
     va_end(ap);
 }

@@ -197,14 +197,14 @@ static GString *qtest_recv_line(QTestState *s)
     return line;
 }

-static gchar **qtest_rsp(QTestState *s, int expected_args)
+static gchar **qtest_rsp(int expected_args)
 {
     GString *line;
     gchar **words;
     int i;

 redo:
-    line = qtest_recv_line(s);
+    line = qtest_recv_line(global_qtest);
     words = g_strsplit(line->str, " ", 0);
     g_string_free(line, TRUE);

@@ -221,9 +221,9 @@ redo:
         g_assert_cmpint(irq, <, MAX_IRQ);

         if (strcmp(words[1], "raise") == 0) {
-            s->irq_level[irq] = true;
+            global_qtest->irq_level[irq] = true;
         } else {
-            s->irq_level[irq] = false;
+            global_qtest->irq_level[irq] = false;
         }

         g_strfreev(words);
@@ -338,8 +338,8 @@ void qtest_start_without_qmp_handshake(const char *extra_args)
     }

     /* ask endianness of the target */
-    qtest_sendf(global_qtest, "endianness\n");
-    args = qtest_rsp(global_qtest, 1);
+    qtest_sendf("endianness\n");
+    args = qtest_rsp(1);
     g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
     s->big_endian = strcmp(args[1], "big") == 0;
     g_strfreev(args);
@@ -618,7 +618,7 @@ static int64_t qtest_clock_rsp(void)
 {
     gchar **words;
     int64_t clock;
-    words = qtest_rsp(global_qtest, 2);
+    words = qtest_rsp(2);
     clock = g_ascii_strtoll(words[1], NULL, 0);
     g_strfreev(words);
     return clock;
@@ -626,38 +626,38 @@ static int64_t qtest_clock_rsp(void)

 int64_t clock_step_next(void)
 {
-    qtest_sendf(global_qtest, "clock_step\n");
+    qtest_sendf("clock_step\n");
     return qtest_clock_rsp();
 }

 int64_t clock_step(int64_t step)
 {
-    qtest_sendf(global_qtest, "clock_step %"PRIi64"\n", step);
+    qtest_sendf("clock_step %"PRIi64"\n", step);
     return qtest_clock_rsp();
 }

 int64_t clock_set(int64_t val)
 {
-    qtest_sendf(global_qtest, "clock_set %"PRIi64"\n", val);
+    qtest_sendf("clock_set %"PRIi64"\n", val);
     return qtest_clock_rsp();
 }

 void irq_intercept_out(const char *qom_path)
 {
-    qtest_sendf(global_qtest, "irq_intercept_out %s\n", qom_path);
-    qtest_rsp(global_qtest, 0);
+    qtest_sendf("irq_intercept_out %s\n", qom_path);
+    qtest_rsp(0);
 }

 void irq_intercept_in(const char *qom_path)
 {
-    qtest_sendf(global_qtest, "irq_intercept_in %s\n", qom_path);
-    qtest_rsp(global_qtest, 0);
+    qtest_sendf("irq_intercept_in %s\n", qom_path);
+    qtest_rsp(0);
 }

 static void out(const char *cmd, uint16_t addr, uint32_t value)
 {
-    qtest_sendf(global_qtest, "%s 0x%x 0x%x\n", cmd, addr, value);
-    qtest_rsp(global_qtest, 0);
+    qtest_sendf("%s 0x%x 0x%x\n", cmd, addr, value);
+    qtest_rsp(0);
 }

 void outb(uint16_t addr, uint8_t value)
@@ -681,8 +681,8 @@ static uint32_t in(const char *cmd, uint16_t addr)
     int ret;
     unsigned long value;

-    qtest_sendf(global_qtest, "%s 0x%x\n", cmd, addr);
-    args = qtest_rsp(global_qtest, 2);
+    qtest_sendf("%s 0x%x\n", cmd, addr);
+    args = qtest_rsp(2);
     ret = qemu_strtoul(args[1], NULL, 0, &value);
     g_assert(!ret && value <= UINT32_MAX);
     g_strfreev(args);
@@ -708,9 +708,8 @@ uint32_t inl(uint16_t addr)
 static void qtest_write(const char *cmd, uint64_t addr,
                         uint64_t value)
 {
-    qtest_sendf(global_qtest, "%s 0x%" PRIx64 " 0x%" PRIx64 "\n", cmd, addr,
-                value);
-    qtest_rsp(global_qtest, 0);
+    qtest_sendf("%s 0x%" PRIx64 " 0x%" PRIx64 "\n", cmd, addr, value);
+    qtest_rsp(0);
 }

 void writeb(uint64_t addr, uint8_t value)
@@ -739,8 +738,8 @@ static uint64_t qtest_read(const char *cmd, uint64_t addr)
     int ret;
     uint64_t value;

-    qtest_sendf(global_qtest, "%s 0x%" PRIx64 "\n", cmd, addr);
-    args = qtest_rsp(global_qtest, 2);
+    qtest_sendf("%s 0x%" PRIx64 "\n", cmd, addr);
+    args = qtest_rsp(2);
     ret = qemu_strtou64(args[1], NULL, 0, &value);
     g_assert(!ret);
     g_strfreev(args);
@@ -791,8 +790,8 @@ void memread(uint64_t addr, void *data, size_t size)
         return;
     }

-    qtest_sendf(global_qtest, "read 0x%" PRIx64 " 0x%zx\n", addr, size);
-    args = qtest_rsp(global_qtest, 2);
+    qtest_sendf("read 0x%" PRIx64 " 0x%zx\n", addr, size);
+    args = qtest_rsp(2);

     for (i = 0; i < size; i++) {
         ptr[i] = hex2nib(args[1][2 + (i * 2)]) << 4;
@@ -805,9 +804,9 @@ void memread(uint64_t addr, void *data, size_t size)
 uint64_t rtas_call(const char *name, uint32_t nargs, uint64_t args,
                    uint32_t nret, uint64_t ret)
 {
-    qtest_sendf(global_qtest, "rtas %s %u 0x%"PRIx64" %u 0x%"PRIx64"\n",
+    qtest_sendf("rtas %s %u 0x%"PRIx64" %u 0x%"PRIx64"\n",
                 name, nargs, args, nret, ret);
-    qtest_rsp(global_qtest, 0);
+    qtest_rsp(0);
     return 0;
 }

@@ -840,10 +839,10 @@ void bufwrite(uint64_t addr, const void *data, size_t size)
     gchar *bdata;

     bdata = g_base64_encode(data, size);
-    qtest_sendf(global_qtest, "b64write 0x%" PRIx64 " 0x%zx ", addr, size);
+    qtest_sendf("b64write 0x%" PRIx64 " 0x%zx ", addr, size);
     socket_send(global_qtest->fd, bdata, -1);
     socket_send(global_qtest->fd, "\n", 1);
-    qtest_rsp(global_qtest, 0);
+    qtest_rsp(0);
     g_free(bdata);
 }

@@ -852,8 +851,8 @@ void bufread(uint64_t addr, void *data, size_t size)
     gchar **args;
     size_t len;

-    qtest_sendf(global_qtest, "b64read 0x%" PRIx64 " 0x%zx\n", addr, size);
-    args = qtest_rsp(global_qtest, 2);
+    qtest_sendf("b64read 0x%" PRIx64 " 0x%zx\n", addr, size);
+    args = qtest_rsp(2);

     g_base64_decode_inplace(args[1], &len);
     if (size != len) {
@@ -882,17 +881,15 @@ void memwrite(uint64_t addr, const void *data, size_t size)
         sprintf(&enc[i * 2], "%02x", ptr[i]);
     }

-    qtest_sendf(global_qtest, "write 0x%" PRIx64 " 0x%zx 0x%s\n", addr, size,
-                enc);
-    qtest_rsp(global_qtest, 0);
+    qtest_sendf("write 0x%" PRIx64 " 0x%zx 0x%s\n", addr, size, enc);
+    qtest_rsp(0);
     g_free(enc);
 }

 void qmemset(uint64_t addr, uint8_t pattern, size_t size)
 {
-    qtest_sendf(global_qtest, "memset 0x%" PRIx64 " 0x%zx 0x%02x\n", addr, size,
-                pattern);
-    qtest_rsp(global_qtest, 0);
+    qtest_sendf("memset 0x%" PRIx64 " 0x%zx 0x%02x\n", addr, size, pattern);
+    qtest_rsp(0);
 }

 QDict *qmp(const char *fmt, ...)
-- 
2.13.5

[Qemu-devel] [PATCH v5 13/13] numa-test: Use hmp()

[Eric Blake]

Don't open-code something that has a convenient helper available.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 tests/numa-test.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/tests/numa-test.c b/tests/numa-test.c
index 3f636840b1..e1b6152244 100644
--- a/tests/numa-test.c
+++ b/tests/numa-test.c
@@ -17,21 +17,6 @@ static char *make_cli(const char *generic_cli, const char *test_cli)
     return g_strdup_printf("%s %s", generic_cli ? generic_cli : "", test_cli);
 }

-static char *hmp_info_numa(void)
-{
-    QDict *resp;
-    char *s;
-
-    resp = qmp("{ 'execute': 'human-monitor-command', 'arguments': "
-                      "{ 'command-line': 'info numa '} }");
-    g_assert(resp);
-    g_assert(qdict_haskey(resp, "return"));
-    s = g_strdup(qdict_get_str(resp, "return"));
-    g_assert(s);
-    QDECREF(resp);
-    return s;
-}
-
 static void test_mon_explicit(const void *data)
 {
     char *s;
@@ -42,7 +27,7 @@ static void test_mon_explicit(const void *data)
                    "-numa node,nodeid=1,cpus=4-7 ");
     qtest_start(cli);

-    s = hmp_info_numa();
+    s = hmp("info numa");
     g_assert(strstr(s, "node 0 cpus: 0 1 2 3"));
     g_assert(strstr(s, "node 1 cpus: 4 5 6 7"));
     g_free(s);
@@ -59,7 +44,7 @@ static void test_mon_default(const void *data)
     cli = make_cli(data, "-smp 8 -numa node -numa node");
     qtest_start(cli);

-    s = hmp_info_numa();
+    s = hmp("info numa");
     g_assert(strstr(s, "node 0 cpus: 0 2 4 6"));
     g_assert(strstr(s, "node 1 cpus: 1 3 5 7"));
     g_free(s);
@@ -78,7 +63,7 @@ static void test_mon_partial(const void *data)
                    "-numa node,nodeid=1,cpus=4-5 ");
     qtest_start(cli);

-    s = hmp_info_numa();
+    s = hmp("info numa");
     g_assert(strstr(s, "node 0 cpus: 0 1 2 3 6 7"));
     g_assert(strstr(s, "node 1 cpus: 4 5"));
     g_free(s);
-- 
2.13.5

Re: [Qemu-devel] [PATCH v5 13/13] numa-test: Use hmp()

[Philippe Mathieu-Daudé]

On 08/18/2017 06:15 PM, Eric Blake wrote:
> Don't open-code something that has a convenient helper available.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

> ---
>   tests/numa-test.c | 21 +++------------------
>   1 file changed, 3 insertions(+), 18 deletions(-)
> 
> diff --git a/tests/numa-test.c b/tests/numa-test.c
> index 3f636840b1..e1b6152244 100644
> --- a/tests/numa-test.c
> +++ b/tests/numa-test.c
> @@ -17,21 +17,6 @@ static char *make_cli(const char *generic_cli, const char *test_cli)
>       return g_strdup_printf("%s %s", generic_cli ? generic_cli : "", test_cli);
>   }
> 
> -static char *hmp_info_numa(void)
> -{
> -    QDict *resp;
> -    char *s;
> -
> -    resp = qmp("{ 'execute': 'human-monitor-command', 'arguments': "
> -                      "{ 'command-line': 'info numa '} }");
> -    g_assert(resp);
> -    g_assert(qdict_haskey(resp, "return"));
> -    s = g_strdup(qdict_get_str(resp, "return"));
> -    g_assert(s);
> -    QDECREF(resp);
> -    return s;
> -}
> -
>   static void test_mon_explicit(const void *data)
>   {
>       char *s;
> @@ -42,7 +27,7 @@ static void test_mon_explicit(const void *data)
>                      "-numa node,nodeid=1,cpus=4-7 ");
>       qtest_start(cli);
> 
> -    s = hmp_info_numa();
> +    s = hmp("info numa");
>       g_assert(strstr(s, "node 0 cpus: 0 1 2 3"));
>       g_assert(strstr(s, "node 1 cpus: 4 5 6 7"));
>       g_free(s);
> @@ -59,7 +44,7 @@ static void test_mon_default(const void *data)
>       cli = make_cli(data, "-smp 8 -numa node -numa node");
>       qtest_start(cli);
> 
> -    s = hmp_info_numa();
> +    s = hmp("info numa");
>       g_assert(strstr(s, "node 0 cpus: 0 2 4 6"));
>       g_assert(strstr(s, "node 1 cpus: 1 3 5 7"));
>       g_free(s);
> @@ -78,7 +63,7 @@ static void test_mon_partial(const void *data)
>                      "-numa node,nodeid=1,cpus=4-5 ");
>       qtest_start(cli);
> 
> -    s = hmp_info_numa();
> +    s = hmp("info numa");
>       g_assert(strstr(s, "node 0 cpus: 0 1 2 3 6 7"));
>       g_assert(strstr(s, "node 1 cpus: 4 5"));
>       g_free(s);
>