[Qemu-devel] [PATCH 0/4] qtest: fix "device_del" out-of-order events

[Qemu-devel] [PATCH 0/4] qtest: fix "device_del" out-of-order events

[Peter Xu]

It starts from a "make check" failure on one of my private tree. The
problem is that when we do "device_del" we normally looking for two
things: one response (which is mostly empty), and a REMOVE event.  The
tricky point is the event can either be there before/after the empty
response.  So I added qmp_device_del() to make sure the order does not
matter, then use it where proper.

Since I'm at it, I also added the sister helper qmp_device_add(), it
helps to remove LOCs.

I still don't 100% sure why my private tree can trigger this error,
while the master cannot. Anyway, I think this is something we should
have, no matter what.

Please review.  Thanks.

Peter Xu (4):
  libqtest: add qmp_device_del()
  tests: use qmp_device_del() where proper
  libqtest: add qmp_device_add()
  tests: use qmp_device_add() where proper

 tests/libqos/pci.c        | 15 +++--------
 tests/libqos/usb.c        | 28 +++++----------------
 tests/libqtest.c          | 63 +++++++++++++++++++++++++++++++++++++++++++++++
 tests/libqtest.h          | 17 +++++++++++++
 tests/usb-hcd-uhci-test.c | 28 +++------------------
 tests/usb-hcd-xhci-test.c | 52 ++++----------------------------------
 6 files changed, 97 insertions(+), 106 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/4] libqtest: add qmp_device_del()

[Peter Xu]

Device deletion is tricky since we'll get both a response and an event,
while the order of arrival may vary.  Provide a helper to handle this
complexity.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/libqtest.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/libqtest.h |  8 ++++++++
 2 files changed, 56 insertions(+)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index b9a1f18..a34d8c4 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -925,6 +925,54 @@ QDict *qmp(const char *fmt, ...)
     return response;
 }
 
+void qmp_device_del(const char *id)
+{
+    QDict *response1, *response2, *event = NULL;
+    char *cmd;
+
+    /*
+     * device deletion will get one response and one event. E.g.:
+     *
+     * {'execute': 'device_del','arguments': { 'id': 'scsi-hd'}}
+     *
+     * will get this one:
+     *
+     * {"timestamp": {"seconds": 1505289667, "microseconds": 569862},
+     *  "event": "DEVICE_DELETED", "data": {"device": "scsi-hd",
+     *  "path": "/machine/peripheral/scsi-hd"}}
+     *
+     * and this one:
+     *
+     * {"return": {}}
+     *
+     * But the order of arrival may vary.  Detect both.
+     */
+
+    cmd = g_strdup_printf("{'execute': 'device_del',"
+                          " 'arguments': {"
+                          "   'id': '%s'"
+                          "}}", id);
+    response1 = qmp(cmd);
+    g_free(cmd);
+    g_assert(response1);
+    g_assert(!qdict_haskey(response1, "error"));
+
+    response2 = qmp("");
+    g_assert(response2);
+    g_assert(!qdict_haskey(response2, "error"));
+
+    if (qdict_haskey(response1, "event")) {
+        event = response1;
+    } else if (qdict_haskey(response2, "event")) {
+        event = response2;
+    }
+    g_assert(event);
+    g_assert(!strcmp(qdict_get_str(event, "event"), "DEVICE_DELETED"));
+
+    QDECREF(response1);
+    QDECREF(response2);
+}
+
 void qmp_async(const char *fmt, ...)
 {
     va_list ap;
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 3ae5709..0d48e4b 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -920,6 +920,14 @@ QDict *qmp_fdv(int fd, const char *fmt, va_list ap);
 QDict *qmp_fd(int fd, const char *fmt, ...);
 
 /**
+ * qmp_device_del:
+ * @id: The device ID to be deleted
+ *
+ * Delete the device with ID @id from QMP interface.
+ */
+void qmp_device_del(const char *id);
+
+/**
  * qtest_cb_for_every_machine:
  * @cb: Pointer to the callback function
  *
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/4] libqtest: add qmp_device_del()

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Device deletion is tricky since we'll get both a response and an event,
> while the order of arrival may vary.  Provide a helper to handle this
> complexity.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  tests/libqtest.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/libqtest.h |  8 ++++++++
>  2 files changed, 56 insertions(+)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index b9a1f18..a34d8c4 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -925,6 +925,54 @@ QDict *qmp(const char *fmt, ...)
>      return response;
>  }
>  
> +void qmp_device_del(const char *id)
> +{
> +    QDict *response1, *response2, *event = NULL;
> +    char *cmd;
> +
> +    /*
> +     * device deletion will get one response and one event. E.g.:
> +     *
> +     * {'execute': 'device_del','arguments': { 'id': 'scsi-hd'}}
> +     *
> +     * will get this one:
> +     *
> +     * {"timestamp": {"seconds": 1505289667, "microseconds": 569862},
> +     *  "event": "DEVICE_DELETED", "data": {"device": "scsi-hd",
> +     *  "path": "/machine/peripheral/scsi-hd"}}
> +     *
> +     * and this one:
> +     *
> +     * {"return": {}}
> +     *
> +     * But the order of arrival may vary.  Detect both.
> +     */
> +
> +    cmd = g_strdup_printf("{'execute': 'device_del',"
> +                          " 'arguments': {"
> +                          "   'id': '%s'"
> +                          "}}", id);
> +    response1 = qmp(cmd);
> +    g_free(cmd);
> +    g_assert(response1);
> +    g_assert(!qdict_haskey(response1, "error"));
> +
> +    response2 = qmp("");
> +    g_assert(response2);
> +    g_assert(!qdict_haskey(response2, "error"));

qmp_receive() would be cleaner than qmp("").

> +
> +    if (qdict_haskey(response1, "event")) {
> +        event = response1;
> +    } else if (qdict_haskey(response2, "event")) {
> +        event = response2;
> +    }
> +    g_assert(event);
> +    g_assert(!strcmp(qdict_get_str(event, "event"), "DEVICE_DELETED"));
> +
> +    QDECREF(response1);
> +    QDECREF(response2);
> +}
> +

Uh, this looks similar to existing qtest_qmp_device_del().  Do we need both?

>  void qmp_async(const char *fmt, ...)
>  {
>      va_list ap;
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 3ae5709..0d48e4b 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -920,6 +920,14 @@ QDict *qmp_fdv(int fd, const char *fmt, va_list ap);
>  QDict *qmp_fd(int fd, const char *fmt, ...);
>  
>  /**
> + * qmp_device_del:
> + * @id: The device ID to be deleted
> + *
> + * Delete the device with ID @id from QMP interface.
> + */
> +void qmp_device_del(const char *id);
> +
> +/**
>   * qtest_cb_for_every_machine:
>   * @cb: Pointer to the callback function
>   *

Re: [Qemu-devel] [PATCH 1/4] libqtest: add qmp_device_del()

[Eric Blake]

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

On 10/02/2017 12:25 PM, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
>> Device deletion is tricky since we'll get both a response and an event,
>> while the order of arrival may vary.  Provide a helper to handle this
>> complexity.
>>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>>  tests/libqtest.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/libqtest.h |  8 ++++++++
>>  2 files changed, 56 insertions(+)
>>
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index b9a1f18..a34d8c4 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -925,6 +925,54 @@ QDict *qmp(const char *fmt, ...)
>>      return response;
>>  }
>>  
>> +void qmp_device_del(const char *id)

>> +
>> +    response2 = qmp("");
>> +    g_assert(response2);
>> +    g_assert(!qdict_haskey(response2, "error"));
> 
> qmp_receive() would be cleaner than qmp("").
> 
>> +
>> +    if (qdict_haskey(response1, "event")) {
>> +        event = response1;
>> +    } else if (qdict_haskey(response2, "event")) {
>> +        event = response2;
>> +    }
>> +    g_assert(event);
>> +    g_assert(!strcmp(qdict_get_str(event, "event"), "DEVICE_DELETED"));
>> +
>> +    QDECREF(response1);
>> +    QDECREF(response2);
>> +}
>> +
> 
> Uh, this looks similar to existing qtest_qmp_device_del().  Do we need both?

In fact, if I'm reading correctly, this is an early version, which was
retitled into commit acd80015 adding qtest_qmp_device_del().

At any rate, when I post my respin of my libqtest cleanups, I have the
qmp("") slated to be fixed.

-- 
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 2/4] tests: use qmp_device_del() where proper

[Peter Xu]

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/libqos/usb.c        | 11 ++---------
 tests/usb-hcd-uhci-test.c | 14 +-------------
 tests/usb-hcd-xhci-test.c | 29 ++---------------------------
 3 files changed, 5 insertions(+), 49 deletions(-)

diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
index 0cdfaec..973dfdd 100644
--- a/tests/libqos/usb.c
+++ b/tests/libqos/usb.c
@@ -60,14 +60,7 @@ void usb_test_hotplug(const char *hcd_id, const int port,
         port_check();
     }
 
-    cmd = g_strdup_printf("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'usbdev%d'"
-                           "}}", port);
-    response = qmp(cmd);
+    cmd = g_strdup_printf("usbdev%d", port);
+    qmp_device_del(cmd);
     g_free(cmd);
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
 }
diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index 5b500fe..7471275 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -60,19 +60,7 @@ static void test_usb_storage_hotplug(void)
     g_assert(!qdict_haskey(response, "error"));
     QDECREF(response);
 
-    response = qmp("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'usbdev0'"
-                           "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("");
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qmp_device_del("usbdev0");
 }
 
 int main(int argc, char **argv)
diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c
index 031764d..156f8a3 100644
--- a/tests/usb-hcd-xhci-test.c
+++ b/tests/usb-hcd-xhci-test.c
@@ -49,33 +49,8 @@ static void test_usb_uas_hotplug(void)
         added disk is visible after BUS rescan
     */
 
-    response = qmp("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'scsi-hd'"
-                           "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("");
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
-
-
-    response = qmp("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'uas'"
-                           "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("");
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qmp_device_del("scsi-hd");
+    qmp_device_del("uas");
 }
 
 int main(int argc, char **argv)
-- 
2.7.4

[Qemu-devel] [PATCH 3/4] libqtest: add qmp_device_add()

[Peter Xu]

Since we have qmp_device_del(), pair them up.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/libqtest.c | 15 +++++++++++++++
 tests/libqtest.h |  9 +++++++++
 2 files changed, 24 insertions(+)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index a34d8c4..c7da962 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -925,6 +925,21 @@ QDict *qmp(const char *fmt, ...)
     return response;
 }
 
+void qmp_device_add(const char *args)
+{
+    QDict *response;
+    char *cmd;
+
+    cmd = g_strdup_printf("{'execute': 'device_add',"
+                          " 'arguments': { %s }"
+                          "}", args);
+    response = qmp(cmd);
+    g_free(cmd);
+    g_assert(response);
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+}
+
 void qmp_device_del(const char *id)
 {
     QDict *response1, *response2, *event = NULL;
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 0d48e4b..ecd02ac 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -920,6 +920,15 @@ QDict *qmp_fdv(int fd, const char *fmt, va_list ap);
 QDict *qmp_fd(int fd, const char *fmt, ...);
 
 /**
+ * qmp_device_add:
+ * @args: Parameters for the new device, like:
+ *        "'driver': 'XXX', 'id': 'XXX', 'addr': 'XXX'"
+ *
+ * Create a new device with parameter @args provided.
+ */
+void qmp_device_add(const char *args);
+
+/**
  * qmp_device_del:
  * @id: The device ID to be deleted
  *
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/4] libqtest: add qmp_device_add()

[Thomas Huth]

On 13.09.2017 11:36, Peter Xu wrote:
> Since we have qmp_device_del(), pair them up.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  tests/libqtest.c | 15 +++++++++++++++
>  tests/libqtest.h |  9 +++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index a34d8c4..c7da962 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -925,6 +925,21 @@ QDict *qmp(const char *fmt, ...)
>      return response;
>  }
>  
> +void qmp_device_add(const char *args)

I think it would be nicer to have a function with variable args here, so
that the callers do not have to do the g_strdup_printf() dance all over
the place. See e.g. my qtest_hot_plug_device() function in my patch
here: http://patchwork.ozlabs.org/patch/801487/

If you fix that, I don't mind if we finally go with my patch or with
yours... If we decide to go with my patch, it would be nice to get some
Reviewed-bys for it. I think I could then send a PULL request for it,
since I've got some other test related patches in my queue and we do not
have a real maintainer for the tests directory...

 Thomas

[Qemu-devel] [PATCH 4/4] tests: use qmp_device_add() where proper

[Peter Xu]

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/libqos/pci.c        | 15 +++------------
 tests/libqos/usb.c        | 17 ++++-------------
 tests/usb-hcd-uhci-test.c | 14 ++------------
 tests/usb-hcd-xhci-test.c | 23 +++--------------------
 4 files changed, 12 insertions(+), 57 deletions(-)

diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
index 2dcdead..7724fcb 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -394,21 +394,12 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
 void qpci_plug_device_test(const char *driver, const char *id,
                            uint8_t slot, const char *opts)
 {
-    QDict *response;
     char *cmd;
 
-    cmd = g_strdup_printf("{'execute': 'device_add',"
-                          " 'arguments': {"
-                          "   'driver': '%s',"
-                          "   'addr': '%d',"
-                          "   %s%s"
-                          "   'id': '%s'"
-                          "}}", driver, slot,
+    cmd = g_strdup_printf("'driver': '%s', 'addr': '%d',"
+                          "%s%s 'id': '%s'", driver, slot,
                           opts ? opts : "", opts ? "," : "",
                           id);
-    response = qmp(cmd);
+    qmp_device_add(cmd);
     g_free(cmd);
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
 }
diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
index 973dfdd..4b23373 100644
--- a/tests/libqos/usb.c
+++ b/tests/libqos/usb.c
@@ -40,21 +40,12 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
 void usb_test_hotplug(const char *hcd_id, const int port,
                       void (*port_check)(void))
 {
-    QDict *response;
     char  *cmd;
 
-    cmd = g_strdup_printf("{'execute': 'device_add',"
-                          " 'arguments': {"
-                          "   'driver': 'usb-tablet',"
-                          "   'port': '%d',"
-                          "   'bus': '%s.0',"
-                          "   'id': 'usbdev%d'"
-                          "}}", port, hcd_id, port);
-    response = qmp(cmd);
-    g_free(cmd);
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
+    cmd = g_strdup_printf("'driver': 'usb-tablet', 'port': '%d',"
+                          "'bus': '%s.0', 'id': 'usbdev%d'",
+                          port, hcd_id, port);
+    qmp_device_add(cmd);
 
     if (port_check) {
         port_check();
diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index 7471275..5f26207 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -48,18 +48,8 @@ static void test_uhci_hotplug(void)
 
 static void test_usb_storage_hotplug(void)
 {
-    QDict *response;
-
-    response = qmp("{'execute': 'device_add',"
-                   " 'arguments': {"
-                   "   'driver': 'usb-storage',"
-                   "   'drive': 'drive0',"
-                   "   'id': 'usbdev0'"
-                   "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
+    qmp_device_add("'driver': 'usb-storage',"
+                   "'drive': 'drive0', 'id': 'usbdev0'");
     qmp_device_del("usbdev0");
 }
 
diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c
index 156f8a3..732b414 100644
--- a/tests/usb-hcd-xhci-test.c
+++ b/tests/usb-hcd-xhci-test.c
@@ -23,26 +23,9 @@ static void test_xhci_hotplug(void)
 
 static void test_usb_uas_hotplug(void)
 {
-    QDict *response;
-
-    response = qmp("{'execute': 'device_add',"
-                   " 'arguments': {"
-                   "   'driver': 'usb-uas',"
-                   "   'id': 'uas'"
-                   "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("{'execute': 'device_add',"
-                   " 'arguments': {"
-                   "   'driver': 'scsi-hd',"
-                   "   'drive': 'drive0',"
-                   "   'id': 'scsi-hd'"
-                   "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
+    qmp_device_add("'driver': 'usb-uas', 'id': 'uas'");
+    qmp_device_add("'driver': 'scsi-hd', 'drive': 'drive0',"
+                   "'id': 'scsi-hd'");
 
     /* TODO:
         UAS HBA driver in libqos, to check that
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/4] qtest: fix "device_del" out-of-order events

[Thomas Huth]

On 13.09.2017 11:36, Peter Xu wrote:
> It starts from a "make check" failure on one of my private tree. The
> problem is that when we do "device_del" we normally looking for two
> things: one response (which is mostly empty), and a REMOVE event.  The
> tricky point is the event can either be there before/after the empty
> response.  So I added qmp_device_del() to make sure the order does not
> matter, then use it where proper.
> 
> Since I'm at it, I also added the sister helper qmp_device_add(), it
> helps to remove LOCs.

I've had a similar idea a couple of weeks ago, see here:

http://patchwork.ozlabs.org/patch/801487/

> I still don't 100% sure why my private tree can trigger this error,
> while the master cannot. Anyway, I think this is something we should
> have, no matter what.

Did you maybe touch the USB tests in your private tree?
As far as I know, some test currently use QMP in a bad way, for example
usb_test_hotplug() only checks for the DEVICE_DELETED at the end, but
forgets to read back the final return value. That return value is then
presented to the next part of the code that uses QMP instead ... it
currently only works more or less by accident, but as soon as you try to
add new code inbetween, it certainly will fail.
==> We really got to clean this up (either with my patch or your patch
series).

 Thomas

Re: [Qemu-devel] [PATCH 0/4] qtest: fix "device_del" out-of-order events

[Peter Xu]

On Wed, Sep 13, 2017 at 11:53:16AM +0200, Thomas Huth wrote:
> On 13.09.2017 11:36, Peter Xu wrote:
> > It starts from a "make check" failure on one of my private tree. The
> > problem is that when we do "device_del" we normally looking for two
> > things: one response (which is mostly empty), and a REMOVE event.  The
> > tricky point is the event can either be there before/after the empty
> > response.  So I added qmp_device_del() to make sure the order does not
> > matter, then use it where proper.
> > 
> > Since I'm at it, I also added the sister helper qmp_device_add(), it
> > helps to remove LOCs.
> 
> I've had a similar idea a couple of weeks ago, see here:
> 
> http://patchwork.ozlabs.org/patch/801487/

Good to know!

> 
> > I still don't 100% sure why my private tree can trigger this error,
> > while the master cannot. Anyway, I think this is something we should
> > have, no matter what.
> 
> Did you maybe touch the USB tests in your private tree?

No.  But I changed some internals of QMP there, I guess that's the
reason that caused the misorder to happen more easily.

> As far as I know, some test currently use QMP in a bad way, for example
> usb_test_hotplug() only checks for the DEVICE_DELETED at the end, but
> forgets to read back the final return value. That return value is then
> presented to the next part of the code that uses QMP instead ... it
> currently only works more or less by accident, but as soon as you try to
> add new code inbetween, it certainly will fail.
> ==> We really got to clean this up (either with my patch or your patch
> series).

Agree.

I think your patch is nicer on the interface (as you have mentioned in
the other reply), I can try to review it later.

However it seems that your patch didn't really solve the problem I
encountered (mis-ordered message arrivals).  It would be good if you
want to solve it together, or I can draft patch upon yours.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/4] qtest: fix "device_del" out-of-order events

[Thomas Huth]

On 13.09.2017 12:28, Peter Xu wrote:
> On Wed, Sep 13, 2017 at 11:53:16AM +0200, Thomas Huth wrote:
>> On 13.09.2017 11:36, Peter Xu wrote:
>>> It starts from a "make check" failure on one of my private tree. The
>>> problem is that when we do "device_del" we normally looking for two
>>> things: one response (which is mostly empty), and a REMOVE event.  The
>>> tricky point is the event can either be there before/after the empty
>>> response.  So I added qmp_device_del() to make sure the order does not
>>> matter, then use it where proper.
>>>
>>> Since I'm at it, I also added the sister helper qmp_device_add(), it
>>> helps to remove LOCs.
>>
>> I've had a similar idea a couple of weeks ago, see here:
>>
>> http://patchwork.ozlabs.org/patch/801487/
> 
> Good to know!
> 
>>
>>> I still don't 100% sure why my private tree can trigger this error,
>>> while the master cannot. Anyway, I think this is something we should
>>> have, no matter what.
>>
>> Did you maybe touch the USB tests in your private tree?
> 
> No.  But I changed some internals of QMP there, I guess that's the
> reason that caused the misorder to happen more easily.
> 
>> As far as I know, some test currently use QMP in a bad way, for example
>> usb_test_hotplug() only checks for the DEVICE_DELETED at the end, but
>> forgets to read back the final return value. That return value is then
>> presented to the next part of the code that uses QMP instead ... it
>> currently only works more or less by accident, but as soon as you try to
>> add new code inbetween, it certainly will fail.
>> ==> We really got to clean this up (either with my patch or your patch
>> series).
> 
> Agree.
> 
> I think your patch is nicer on the interface (as you have mentioned in
> the other reply), I can try to review it later.
> 
> However it seems that your patch didn't really solve the problem I
> encountered (mis-ordered message arrivals).  It would be good if you
> want to solve it together, or I can draft patch upon yours.

True, I'll try to respin my patch, including the ideas from your patch...

 Thomas

Re: [Qemu-devel] [PATCH 0/4] qtest: fix "device_del" out-of-order events

[Peter Xu]

On Wed, Sep 13, 2017 at 12:35:17PM +0200, Thomas Huth wrote:

[...]

> >> As far as I know, some test currently use QMP in a bad way, for example
> >> usb_test_hotplug() only checks for the DEVICE_DELETED at the end, but
> >> forgets to read back the final return value. That return value is then
> >> presented to the next part of the code that uses QMP instead ... it
> >> currently only works more or less by accident, but as soon as you try to
> >> add new code inbetween, it certainly will fail.
> >> ==> We really got to clean this up (either with my patch or your patch
> >> series).
> > 
> > Agree.
> > 
> > I think your patch is nicer on the interface (as you have mentioned in
> > the other reply), I can try to review it later.
> > 
> > However it seems that your patch didn't really solve the problem I
> > encountered (mis-ordered message arrivals).  It would be good if you
> > want to solve it together, or I can draft patch upon yours.
> 
> True, I'll try to respin my patch, including the ideas from your patch...

That would be great.  I've left some comments there already.  Thanks!

-- 
Peter Xu