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

Markus Armbruster posted 23 patches 7 years, 3 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v2 15/23] tests: New helper qtest_qmp_receive_success()
Posted by Markus Armbruster 7 years, 3 months ago
Commit b21373d0713 copied wait_command() from tests/migration-test.c
to tests/tpm-util.c.  Replace both copies by new libqtest helper
qtest_qmp_receive_success().  Also use it to simplify
qtest_qmp_device_del().

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

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

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


Re: [Qemu-devel] [PATCH v2 15/23] tests: New helper qtest_qmp_receive_success()
Posted by Eric Blake 7 years, 3 months ago
On 07/27/2018 10:13 AM, Markus Armbruster wrote:
> Commit b21373d0713 copied wait_command() from tests/migration-test.c
> to tests/tpm-util.c.  Replace both copies by new libqtest helper
> qtest_qmp_receive_success().  Also use it to simplify
> qtest_qmp_device_del().
> 
> Bonus: gets rid of a non-literal format string.  A step towards
> compile-time format string checking without triggering
> -Wformat-nonliteral.

Where? [1]

> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---

> +++ b/tests/libqtest.c
> @@ -1029,6 +1029,35 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine))
>       qobject_unref(response);
>   }
>   
> +QDict *qtest_qmp_receive_success(QTestState *s,
> +                                 void (*event_cb)(void *opaque,
> +                                                  const char *event,
> +                                                  QDict *data),
> +                                 void *opaque)
> +{

I like the new signature!

> +++ b/tests/migration-test.c

>   /*
>    * Events can get in the way of responses we are actually waiting for.
>    */
>   static QDict *wait_command(QTestState *who, const char *command)
>   {

[1] This is still taking a format non-literal command...

> -    const char *event_string;
> -    QDict *response, *ret;
> -
> -    response = qtest_qmp(who, command);
...
> +    qtest_qmp_send(who, command);

and is passing it on through.

> +    return qtest_qmp_receive_success(who, stop_cb, NULL);
>   }
>   

> +++ b/tests/tpm-util.c

> -/*
> - * Events can get in the way of responses we are actually waiting for.
> - */
> -static QDict *tpm_util_wait_command(QTestState *who, const char *command)
> -{

Maybe you were counting this instance,

>   void tpm_util_wait_for_migration_complete(QTestState *who)
>   {
>       while (true) {
> -        QDict *rsp, *rsp_return;
> +        QDict *rsp_return;
>           bool completed;
>           const char *status;
>   
> -        rsp = tpm_util_wait_command(who, "{ 'execute': 'query-migrate' }");
> -        rsp_return = qdict_get_qdict(rsp, "return");
> +        qtest_qmp_send(who, "{ 'execute': 'query-migrate' }");

where you did indeed drop a format non-literal?

But on the assumption that there's more cleanups later in the series, 
this is indeed incrementally better, so

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

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

Re: [Qemu-devel] [PATCH v2 15/23] tests: New helper qtest_qmp_receive_success()
Posted by Markus Armbruster 7 years, 3 months ago
Eric Blake <eblake@redhat.com> writes:

> On 07/27/2018 10:13 AM, Markus Armbruster wrote:
>> Commit b21373d0713 copied wait_command() from tests/migration-test.c
>> to tests/tpm-util.c.  Replace both copies by new libqtest helper
>> qtest_qmp_receive_success().  Also use it to simplify
>> qtest_qmp_device_del().
>>
>> Bonus: gets rid of a non-literal format string.  A step towards
>> compile-time format string checking without triggering
>> -Wformat-nonliteral.
>
> Where? [1]
>
>>
>> Cc: Thomas Huth <thuth@redhat.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>
>> +++ b/tests/libqtest.c
>> @@ -1029,6 +1029,35 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine))
>>       qobject_unref(response);
>>   }
>>   +QDict *qtest_qmp_receive_success(QTestState *s,
>> +                                 void (*event_cb)(void *opaque,
>> +                                                  const char *event,
>> +                                                  QDict *data),
>> +                                 void *opaque)
>> +{
>
> I like the new signature!
>
>> +++ b/tests/migration-test.c
>
>>   /*
>>    * Events can get in the way of responses we are actually waiting for.
>>    */
>>   static QDict *wait_command(QTestState *who, const char *command)
>>   {
>
> [1] This is still taking a format non-literal command...
>
>> -    const char *event_string;
>> -    QDict *response, *ret;
>> -
>> -    response = qtest_qmp(who, command);
> ...
>> +    qtest_qmp_send(who, command);
>
> and is passing it on through.

No worse than before.  Will be fixed in the next two patches.

>> +    return qtest_qmp_receive_success(who, stop_cb, NULL);
>>   }
>>   
>
>> +++ b/tests/tpm-util.c
>
>> -/*
>> - * Events can get in the way of responses we are actually waiting for.
>> - */
>> -static QDict *tpm_util_wait_command(QTestState *who, const char *command)
>> -{
>
> Maybe you were counting this instance,
>

    -    const char *event_string;
    -    QDict *response;
    -
    -    response = qtest_qmp(who, command);

/work/armbru/qemu/tests/tpm-util.c: In function ‘tpm_util_wait_command’:
/work/armbru/qemu/tests/tpm-util.c:258:5: warning: format not a string literal and no format arguments [-Wformat-security]
     response = qtest_qmp(who, command);
     ^~~~~~~~

    -
    -    while (qdict_haskey(response, "event")) {
    -        /* OK, it was an event */
    -        event_string = qdict_get_str(response, "event");
    -        if (!strcmp(event_string, "STOP")) {
    -            got_stop = true;
    -        }
    -        qobject_unref(response);
    -        response = qtest_qmp_receive(who);
    -    }
    -    return response;
    -}
>>   void tpm_util_wait_for_migration_complete(QTestState *who)
>>   {
>>       while (true) {
>> -        QDict *rsp, *rsp_return;
>> +        QDict *rsp_return;
>>           bool completed;
>>           const char *status;
>>   -        rsp = tpm_util_wait_command(who, "{ 'execute':
>> 'query-migrate' }");
>> -        rsp_return = qdict_get_qdict(rsp, "return");
>> +        qtest_qmp_send(who, "{ 'execute': 'query-migrate' }");
>
> where you did indeed drop a format non-literal?

> But on the assumption that there's more cleanups later in the series,
> this is indeed incrementally better, so
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!