[PATCH v7 00/13] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread

[PATCH v7 00/13] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread

[Maxim Levitsky]

This is the next version of the patches.

In this version I implemented Paulo's suggestion of fixing the qtests
to cope with out of order events.

This resulted in small but nice refactoring.

Besides that, the only other change from V6 is that I dropped Paulo's
fix to qtest_qmp_device_del since it is fixed now by my qtest patches.

The iotest 67 still fails with this, something that Paulo
is looking to fix before merging this.

Best regards,
	Maxim Levitsky

Maxim Levitsky (10):
  qtest: rename qtest_qmp_receive to qtest_qmp_receive_dict
  qtest: Reintroduce qtest_qmp_receive
  qtest: switch users back to qtest_qmp_receive
  scsi/scsi_bus: switch search direction in scsi_device_find
  device_core: use drain_call_rcu in in qmp_device_add
  device-core: use RCU for list of children of a bus
  device-core: use atomic_set on .realized property
  scsi/scsi_bus: Add scsi_device_get
  virtio-scsi: use scsi_device_get
  scsi/scsi_bus: fix races in REPORT LUNS

Paolo Bonzini (3):
  qdev: add "check if address free" callback for buses
  scsi: switch to bus->check_address
  scsi/scsi-bus: scsi_device_find: don't return unrealized devices

 hw/core/bus.c                   |  28 ++--
 hw/core/qdev.c                  |  73 ++++++---
 hw/net/virtio-net.c             |   2 +-
 hw/scsi/scsi-bus.c              | 262 ++++++++++++++++++++------------
 hw/scsi/virtio-scsi.c           |  27 ++--
 hw/sd/core.c                    |   3 +-
 include/hw/qdev-core.h          |  24 ++-
 include/hw/scsi/scsi.h          |   1 +
 qdev-monitor.c                  |  12 ++
 tests/qtest/drive_del-test.c    |   9 +-
 tests/qtest/libqos/libqtest.h   |  34 +++--
 tests/qtest/libqtest.c          | 110 +++++++-------
 tests/qtest/migration-helpers.c |  25 ++-
 tests/qtest/pvpanic-test.c      |   4 +-
 tests/qtest/qmp-test.c          |  18 +--
 tests/qtest/tpm-util.c          |   8 +-
 16 files changed, 411 insertions(+), 229 deletions(-)

-- 
2.26.2

[PATCH v7 01/13] qtest: rename qtest_qmp_receive to qtest_qmp_receive_dict

[Maxim Levitsky]

In the next patch a new version of qtest_qmp_receive will be
reintroduced that will buffer received qmp events for later
consumption in qtest_qmp_eventwait_ref

No functional change intended.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 tests/qtest/ahci-test.c        |  4 ++--
 tests/qtest/device-plug-test.c |  2 +-
 tests/qtest/drive_del-test.c   |  2 +-
 tests/qtest/libqos/libqtest.h  |  4 ++--
 tests/qtest/libqtest.c         | 16 ++++++++--------
 tests/qtest/pvpanic-test.c     |  2 +-
 tests/qtest/qmp-test.c         | 18 +++++++++---------
 7 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index 5e1954852e..d42ebaeb4c 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -1590,7 +1590,7 @@ static void test_atapi_tray(void)
     qtest_qmp_send(ahci->parent->qts, "{'execute': 'blockdev-open-tray', "
                     "'arguments': {'id': 'cd0'}}");
     atapi_wait_tray(ahci, true);
-    rsp = qtest_qmp_receive(ahci->parent->qts);
+    rsp = qtest_qmp_receive_dict(ahci->parent->qts);
     qobject_unref(rsp);
 
     qmp_discard_response(ahci->parent->qts,
@@ -1620,7 +1620,7 @@ static void test_atapi_tray(void)
     qtest_qmp_send(ahci->parent->qts, "{'execute': 'blockdev-close-tray', "
                    "'arguments': {'id': 'cd0'}}");
     atapi_wait_tray(ahci, false);
-    rsp = qtest_qmp_receive(ahci->parent->qts);
+    rsp = qtest_qmp_receive_dict(ahci->parent->qts);
     qobject_unref(rsp);
 
     /* Now, to convince ATAPI we understand the media has changed... */
diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c
index 9214892741..a2247856be 100644
--- a/tests/qtest/device-plug-test.c
+++ b/tests/qtest/device-plug-test.c
@@ -23,7 +23,7 @@ static void device_del_start(QTestState *qtest, const char *id)
 
 static void device_del_finish(QTestState *qtest)
 {
-    QDict *resp = qtest_qmp_receive(qtest);
+    QDict *resp = qtest_qmp_receive_dict(qtest);
 
     g_assert(qdict_haskey(resp, "return"));
     qobject_unref(resp);
diff --git a/tests/qtest/drive_del-test.c b/tests/qtest/drive_del-test.c
index 2d765865ce..ba0cd77445 100644
--- a/tests/qtest/drive_del-test.c
+++ b/tests/qtest/drive_del-test.c
@@ -41,7 +41,7 @@ static void device_del(QTestState *qts)
     /* Complication: ignore DEVICE_DELETED event */
     qmp_discard_response(qts, "{'execute': 'device_del',"
                          " 'arguments': { 'id': 'dev0' } }");
-    response = qtest_qmp_receive(qts);
+    response = qtest_qmp_receive_dict(qts);
     g_assert(response);
     g_assert(qdict_haskey(response, "return"));
     qobject_unref(response);
diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h
index a6ee1654f2..a41135fc92 100644
--- a/tests/qtest/libqos/libqtest.h
+++ b/tests/qtest/libqos/libqtest.h
@@ -191,12 +191,12 @@ void qtest_qmp_vsend(QTestState *s, const char *fmt, va_list ap)
     GCC_FMT_ATTR(2, 0);
 
 /**
- * qtest_receive:
+ * qtest_qmp_receive_dict:
  * @s: #QTestState instance to operate on.
  *
  * Reads a QMP message from QEMU and returns the response.
  */
-QDict *qtest_qmp_receive(QTestState *s);
+QDict *qtest_qmp_receive_dict(QTestState *s);
 
 /**
  * qtest_qmp_eventwait:
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 58f58e1ece..dadc465825 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -322,7 +322,7 @@ QTestState *qtest_init(const char *extra_args)
     QDict *greeting;
 
     /* Read the QMP greeting and then do the handshake */
-    greeting = qtest_qmp_receive(s);
+    greeting = qtest_qmp_receive_dict(s);
     qobject_unref(greeting);
     qobject_unref(qtest_qmp(s, "{ 'execute': 'qmp_capabilities' }"));
 
@@ -603,7 +603,7 @@ QDict *qmp_fd_receive(int fd)
     return qmp.response;
 }
 
-QDict *qtest_qmp_receive(QTestState *s)
+QDict *qtest_qmp_receive_dict(QTestState *s)
 {
     return qmp_fd_receive(s->qmp_fd);
 }
@@ -678,7 +678,7 @@ QDict *qtest_vqmp_fds(QTestState *s, int *fds, size_t fds_num,
     qtest_qmp_vsend_fds(s, fds, fds_num, fmt, ap);
 
     /* Receive reply */
-    return qtest_qmp_receive(s);
+    return qtest_qmp_receive_dict(s);
 }
 
 QDict *qtest_vqmp(QTestState *s, const char *fmt, va_list ap)
@@ -686,7 +686,7 @@ QDict *qtest_vqmp(QTestState *s, const char *fmt, va_list ap)
     qtest_qmp_vsend(s, fmt, ap);
 
     /* Receive reply */
-    return qtest_qmp_receive(s);
+    return qtest_qmp_receive_dict(s);
 }
 
 QDict *qmp_fd(int fd, const char *fmt, ...)
@@ -776,7 +776,7 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event)
     QDict *response;
 
     for (;;) {
-        response = qtest_qmp_receive(s);
+        response = qtest_qmp_receive_dict(s);
         if ((qdict_haskey(response, "event")) &&
             (strcmp(qdict_get_str(response, "event"), event) == 0)) {
             return response;
@@ -807,7 +807,7 @@ char *qtest_vhmp(QTestState *s, const char *fmt, va_list ap)
     while (ret == NULL && qdict_get_try_str(resp, "event")) {
         /* Ignore asynchronous QMP events */
         qobject_unref(resp);
-        resp = qtest_qmp_receive(s);
+        resp = qtest_qmp_receive_dict(s);
         ret = g_strdup(qdict_get_try_str(resp, "return"));
     }
     g_assert(ret);
@@ -1255,7 +1255,7 @@ QDict *qtest_qmp_receive_success(QTestState *s,
     const char *event;
 
     for (;;) {
-        response = qtest_qmp_receive(s);
+        response = qtest_qmp_receive_dict(s);
         g_assert(!qdict_haskey(response, "error"));
         ret = qdict_get_qdict(response, "return");
         if (ret) {
@@ -1345,7 +1345,7 @@ void qtest_qmp_device_del(QTestState *qts, const char *id)
     rsp = qtest_qmp_receive_success(qts, device_deleted_cb, &got_event);
     qobject_unref(rsp);
     if (!got_event) {
-        rsp = qtest_qmp_receive(qts);
+        rsp = qtest_qmp_receive_dict(qts);
         g_assert_cmpstr(qdict_get_try_str(rsp, "event"),
                         ==, "DEVICE_DELETED");
         qobject_unref(rsp);
diff --git a/tests/qtest/pvpanic-test.c b/tests/qtest/pvpanic-test.c
index e57639481e..b0b20ad340 100644
--- a/tests/qtest/pvpanic-test.c
+++ b/tests/qtest/pvpanic-test.c
@@ -24,7 +24,7 @@ static void test_panic(void)
 
     qtest_outb(qts, 0x505, 0x1);
 
-    response = qtest_qmp_receive(qts);
+    response = qtest_qmp_receive_dict(qts);
     g_assert(qdict_haskey(response, "event"));
     g_assert_cmpstr(qdict_get_str(response, "event"), ==, "GUEST_PANICKED");
     g_assert(qdict_haskey(response, "data"));
diff --git a/tests/qtest/qmp-test.c b/tests/qtest/qmp-test.c
index e1032c5a21..eb1cd8abb8 100644
--- a/tests/qtest/qmp-test.c
+++ b/tests/qtest/qmp-test.c
@@ -47,37 +47,37 @@ static void test_malformed(QTestState *qts)
 
     /* syntax error */
     qtest_qmp_send_raw(qts, "{]\n");
-    resp = qtest_qmp_receive(qts);
+    resp = qtest_qmp_receive_dict(qts);
     qmp_expect_error_and_unref(resp, "GenericError");
     assert_recovered(qts);
 
     /* lexical error: impossible byte outside string */
     qtest_qmp_send_raw(qts, "{\xFF");
-    resp = qtest_qmp_receive(qts);
+    resp = qtest_qmp_receive_dict(qts);
     qmp_expect_error_and_unref(resp, "GenericError");
     assert_recovered(qts);
 
     /* lexical error: funny control character outside string */
     qtest_qmp_send_raw(qts, "{\x01");
-    resp = qtest_qmp_receive(qts);
+    resp = qtest_qmp_receive_dict(qts);
     qmp_expect_error_and_unref(resp, "GenericError");
     assert_recovered(qts);
 
     /* lexical error: impossible byte in string */
     qtest_qmp_send_raw(qts, "{'bad \xFF");
-    resp = qtest_qmp_receive(qts);
+    resp = qtest_qmp_receive_dict(qts);
     qmp_expect_error_and_unref(resp, "GenericError");
     assert_recovered(qts);
 
     /* lexical error: control character in string */
     qtest_qmp_send_raw(qts, "{'execute': 'nonexistent', 'id':'\n");
-    resp = qtest_qmp_receive(qts);
+    resp = qtest_qmp_receive_dict(qts);
     qmp_expect_error_and_unref(resp, "GenericError");
     assert_recovered(qts);
 
     /* lexical error: interpolation */
     qtest_qmp_send_raw(qts, "%%p");
-    resp = qtest_qmp_receive(qts);
+    resp = qtest_qmp_receive_dict(qts);
     qmp_expect_error_and_unref(resp, "GenericError");
     assert_recovered(qts);
 
@@ -111,7 +111,7 @@ static void test_qmp_protocol(void)
     qts = qtest_init_without_qmp_handshake(common_args);
 
     /* Test greeting */
-    resp = qtest_qmp_receive(qts);
+    resp = qtest_qmp_receive_dict(qts);
     q = qdict_get_qdict(resp, "QMP");
     g_assert(q);
     test_version(qdict_get(q, "version"));
@@ -205,7 +205,7 @@ static void send_oob_cmd_that_fails(QTestState *s, const char *id)
 
 static void recv_cmd_id(QTestState *s, const char *id)
 {
-    QDict *resp = qtest_qmp_receive(s);
+    QDict *resp = qtest_qmp_receive_dict(s);
 
     g_assert_cmpstr(qdict_get_try_str(resp, "id"), ==, id);
     qobject_unref(resp);
@@ -222,7 +222,7 @@ static void test_qmp_oob(void)
     qts = qtest_init_without_qmp_handshake(common_args);
 
     /* Check the greeting message. */
-    resp = qtest_qmp_receive(qts);
+    resp = qtest_qmp_receive_dict(qts);
     q = qdict_get_qdict(resp, "QMP");
     g_assert(q);
     capabilities = qdict_get_qlist(q, "capabilities");
-- 
2.26.2

[PATCH v7 02/13] qtest: Reintroduce qtest_qmp_receive

[Maxim Levitsky]

The new qtest_qmp_receive buffers all the received qmp events, allowing
qtest_qmp_eventwait_ref to return them.

This is intended to solve the race in regard to ordering of qmp events
vs qmp responses, as soon as the callers start using the new interface.

In addition to that, define qtest_qmp_event_ref a function which only scans
the buffer that qtest_qmp_receive stores the events to.

This is intended for callers that are only interested in events that were
received during the last call to the qtest_qmp_receive.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 tests/qtest/libqos/libqtest.h | 23 ++++++++++++++++
 tests/qtest/libqtest.c        | 49 ++++++++++++++++++++++++++++++++++-
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h
index a41135fc92..19429a536d 100644
--- a/tests/qtest/libqos/libqtest.h
+++ b/tests/qtest/libqos/libqtest.h
@@ -198,6 +198,16 @@ void qtest_qmp_vsend(QTestState *s, const char *fmt, va_list ap)
  */
 QDict *qtest_qmp_receive_dict(QTestState *s);
 
+/**
+ * qtest_qmp_receive:
+ * @s: #QTestState instance to operate on.
+ *
+ * Reads a QMP message from QEMU and returns the response.
+ * Buffers all the events received meanwhile, until a
+ * call to qtest_qmp_eventwait
+ */
+QDict *qtest_qmp_receive(QTestState *s);
+
 /**
  * qtest_qmp_eventwait:
  * @s: #QTestState instance to operate on.
@@ -217,6 +227,19 @@ void qtest_qmp_eventwait(QTestState *s, const char *event);
  */
 QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
 
+/**
+ * qtest_qmp_event_ref:
+ * @s: #QTestState instance to operate on.
+ * @s: #event event to return.
+ *
+ * Removes non-matching events from the buffer that was set by
+ * qtest_qmp_receive, until an event bearing the given name is found,
+ * and returns it.
+ * If no event matches, clears the buffer and returns NULL.
+ *
+ */
+QDict *qtest_qmp_event_ref(QTestState *s, const char *event);
+
 /**
  * qtest_qmp_receive_success:
  * @s: #QTestState instance to operate on
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index dadc465825..d4c49a52ff 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -63,6 +63,7 @@ struct QTestState
     bool irq_level[MAX_IRQ];
     GString *rx;
     QTestTransportOps ops;
+    GList *pending_events;
 };
 
 static GHookList abrt_hooks;
@@ -279,6 +280,7 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
 
     g_test_message("starting QEMU: %s", command);
 
+    s->pending_events = NULL;
     s->wstatus = 0;
     s->expected_status = 0;
     s->qemu_pid = fork();
@@ -386,6 +388,13 @@ void qtest_quit(QTestState *s)
     close(s->fd);
     close(s->qmp_fd);
     g_string_free(s->rx, true);
+
+    for (GList *it = s->pending_events; it != NULL; it = it->next) {
+        qobject_unref((QDict *)it->data);
+    }
+
+    g_list_free(s->pending_events);
+
     g_free(s);
 }
 
@@ -603,6 +612,19 @@ QDict *qmp_fd_receive(int fd)
     return qmp.response;
 }
 
+QDict *qtest_qmp_receive(QTestState *s)
+{
+    while (true) {
+        QDict *response = qtest_qmp_receive_dict(s);
+
+        if (!qdict_get_try_str(response, "event")) {
+            return response;
+        }
+        /* Stash the event for a later consumption */
+        s->pending_events = g_list_prepend(s->pending_events, response);
+    }
+}
+
 QDict *qtest_qmp_receive_dict(QTestState *s)
 {
     return qmp_fd_receive(s->qmp_fd);
@@ -771,10 +793,34 @@ void qtest_qmp_send_raw(QTestState *s, const char *fmt, ...)
     va_end(ap);
 }
 
-QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event)
+QDict *qtest_qmp_event_ref(QTestState *s, const char *event)
 {
+    GList *next = NULL;
     QDict *response;
 
+    for (GList *it = s->pending_events; it != NULL; it = next) {
+
+        next = it->next;
+        response = (QDict *)it->data;
+
+        s->pending_events = g_list_remove_link(s->pending_events, it);
+
+        if (!strcmp(qdict_get_str(response, "event"), event)) {
+            return response;
+        }
+        qobject_unref(response);
+    }
+    return NULL;
+}
+
+QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event)
+{
+    QDict *response = qtest_qmp_event_ref(s, event);
+
+    if (response) {
+        return response;
+    }
+
     for (;;) {
         response = qtest_qmp_receive_dict(s);
         if ((qdict_haskey(response, "event")) &&
@@ -1403,6 +1449,7 @@ QTestState *qtest_inproc_init(QTestState **s, bool log, const char* arch,
 {
     QTestState *qts;
     qts = g_new0(QTestState, 1);
+    qts->pending_events = NULL;
     *s = qts; /* Expose qts early on, since the query endianness relies on it */
     qts->wstatus = 0;
     for (int i = 0; i < MAX_IRQ; i++) {
-- 
2.26.2

[PATCH v7 03/13] qtest: switch users back to qtest_qmp_receive

[Maxim Levitsky]

The only remaining users of qtest_qmp_receive_dict are tests
that fuzz the QMP protocol.

Tested with 'make check-qtest'.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 tests/qtest/ahci-test.c         |  4 +-
 tests/qtest/device-plug-test.c  |  2 +-
 tests/qtest/drive_del-test.c    |  9 ++---
 tests/qtest/libqos/libqtest.h   | 17 ---------
 tests/qtest/libqtest.c          | 65 ++++-----------------------------
 tests/qtest/migration-helpers.c | 25 ++++++++++---
 tests/qtest/pvpanic-test.c      |  4 +-
 tests/qtest/tpm-util.c          |  8 +++-
 8 files changed, 41 insertions(+), 93 deletions(-)

diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index d42ebaeb4c..5e1954852e 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -1590,7 +1590,7 @@ static void test_atapi_tray(void)
     qtest_qmp_send(ahci->parent->qts, "{'execute': 'blockdev-open-tray', "
                     "'arguments': {'id': 'cd0'}}");
     atapi_wait_tray(ahci, true);
-    rsp = qtest_qmp_receive_dict(ahci->parent->qts);
+    rsp = qtest_qmp_receive(ahci->parent->qts);
     qobject_unref(rsp);
 
     qmp_discard_response(ahci->parent->qts,
@@ -1620,7 +1620,7 @@ static void test_atapi_tray(void)
     qtest_qmp_send(ahci->parent->qts, "{'execute': 'blockdev-close-tray', "
                    "'arguments': {'id': 'cd0'}}");
     atapi_wait_tray(ahci, false);
-    rsp = qtest_qmp_receive_dict(ahci->parent->qts);
+    rsp = qtest_qmp_receive(ahci->parent->qts);
     qobject_unref(rsp);
 
     /* Now, to convince ATAPI we understand the media has changed... */
diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c
index a2247856be..9214892741 100644
--- a/tests/qtest/device-plug-test.c
+++ b/tests/qtest/device-plug-test.c
@@ -23,7 +23,7 @@ static void device_del_start(QTestState *qtest, const char *id)
 
 static void device_del_finish(QTestState *qtest)
 {
-    QDict *resp = qtest_qmp_receive_dict(qtest);
+    QDict *resp = qtest_qmp_receive(qtest);
 
     g_assert(qdict_haskey(resp, "return"));
     qobject_unref(resp);
diff --git a/tests/qtest/drive_del-test.c b/tests/qtest/drive_del-test.c
index ba0cd77445..9d20a1ed8b 100644
--- a/tests/qtest/drive_del-test.c
+++ b/tests/qtest/drive_del-test.c
@@ -15,9 +15,6 @@
 #include "libqos/virtio.h"
 #include "qapi/qmp/qdict.h"
 
-/* TODO actually test the results and get rid of this */
-#define qmp_discard_response(q, ...) qobject_unref(qtest_qmp(q, __VA_ARGS__))
-
 static void drive_add(QTestState *qts)
 {
     char *resp = qtest_hmp(qts, "drive_add 0 if=none,id=drive0");
@@ -38,13 +35,13 @@ static void device_del(QTestState *qts)
 {
     QDict *response;
 
-    /* Complication: ignore DEVICE_DELETED event */
-    qmp_discard_response(qts, "{'execute': 'device_del',"
+    response = qtest_qmp(qts, "{'execute': 'device_del',"
                          " 'arguments': { 'id': 'dev0' } }");
-    response = qtest_qmp_receive_dict(qts);
     g_assert(response);
     g_assert(qdict_haskey(response, "return"));
     qobject_unref(response);
+
+    qtest_qmp_eventwait(qts, "DEVICE_DELETED");
 }
 
 static void test_drive_without_dev(void)
diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h
index 19429a536d..a91e9e02e9 100644
--- a/tests/qtest/libqos/libqtest.h
+++ b/tests/qtest/libqos/libqtest.h
@@ -240,23 +240,6 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
  */
 QDict *qtest_qmp_event_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/qtest/libqtest.c b/tests/qtest/libqtest.c
index d4c49a52ff..08929f5ff6 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -324,7 +324,7 @@ QTestState *qtest_init(const char *extra_args)
     QDict *greeting;
 
     /* Read the QMP greeting and then do the handshake */
-    greeting = qtest_qmp_receive_dict(s);
+    greeting = qtest_qmp_receive(s);
     qobject_unref(greeting);
     qobject_unref(qtest_qmp(s, "{ 'execute': 'qmp_capabilities' }"));
 
@@ -700,7 +700,7 @@ QDict *qtest_vqmp_fds(QTestState *s, int *fds, size_t fds_num,
     qtest_qmp_vsend_fds(s, fds, fds_num, fmt, ap);
 
     /* Receive reply */
-    return qtest_qmp_receive_dict(s);
+    return qtest_qmp_receive(s);
 }
 
 QDict *qtest_vqmp(QTestState *s, const char *fmt, va_list ap)
@@ -708,7 +708,7 @@ QDict *qtest_vqmp(QTestState *s, const char *fmt, va_list ap)
     qtest_qmp_vsend(s, fmt, ap);
 
     /* Receive reply */
-    return qtest_qmp_receive_dict(s);
+    return qtest_qmp_receive(s);
 }
 
 QDict *qmp_fd(int fd, const char *fmt, ...)
@@ -850,12 +850,6 @@ char *qtest_vhmp(QTestState *s, const char *fmt, va_list ap)
                      " 'arguments': {'command-line': %s}}",
                      cmd);
     ret = g_strdup(qdict_get_try_str(resp, "return"));
-    while (ret == NULL && qdict_get_try_str(resp, "event")) {
-        /* Ignore asynchronous QMP events */
-        qobject_unref(resp);
-        resp = qtest_qmp_receive_dict(s);
-        ret = g_strdup(qdict_get_try_str(resp, "return"));
-    }
     g_assert(ret);
     qobject_unref(resp);
     g_free(cmd);
@@ -1291,35 +1285,6 @@ 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_dict(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 commands.
  */
@@ -1355,13 +1320,6 @@ void qtest_qmp_device_add(QTestState *qts, const char *driver, const char *id,
     qobject_unref(args);
 }
 
-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.
@@ -1378,24 +1336,17 @@ static void device_deleted_cb(void *opaque, const char *name, QDict *data)
  * and this one:
  *
  * {"return": {}}
- *
- * But the order of arrival may vary - so we've got to detect both.
  */
 void qtest_qmp_device_del(QTestState *qts, const char *id)
 {
-    bool got_event = false;
     QDict *rsp;
 
-    qtest_qmp_send(qts, "{'execute': 'device_del', 'arguments': {'id': %s}}",
-                   id);
-    rsp = qtest_qmp_receive_success(qts, device_deleted_cb, &got_event);
+    rsp = qtest_qmp(qts, "{'execute': 'device_del', 'arguments': {'id': %s}}",
+                    id);
+
+    g_assert(qdict_haskey(rsp, "return"));
     qobject_unref(rsp);
-    if (!got_event) {
-        rsp = qtest_qmp_receive_dict(qts);
-        g_assert_cmpstr(qdict_get_try_str(rsp, "event"),
-                        ==, "DEVICE_DELETED");
-        qobject_unref(rsp);
-    }
+    qtest_qmp_eventwait(qts, "DEVICE_DELETED");
 }
 
 bool qmp_rsp_is_err(QDict *rsp)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 516093b39a..b799dbafb7 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -17,10 +17,12 @@
 
 bool got_stop;
 
-static void stop_cb(void *opaque, const char *name, QDict *data)
+static void check_stop_event(QTestState *who)
 {
-    if (!strcmp(name, "STOP")) {
+    QDict *event = qtest_qmp_event_ref(who, "STOP");
+    if (event) {
         got_stop = true;
+        qobject_unref(event);
     }
 }
 
@@ -30,12 +32,19 @@ static void stop_cb(void *opaque, const char *name, QDict *data)
 QDict *wait_command_fd(QTestState *who, int fd, const char *command, ...)
 {
     va_list ap;
+    QDict *resp;
 
     va_start(ap, command);
     qtest_qmp_vsend_fds(who, &fd, 1, command, ap);
     va_end(ap);
 
-    return qtest_qmp_receive_success(who, stop_cb, NULL);
+    resp = qtest_qmp_receive(who);
+    check_stop_event(who);
+
+    g_assert(!qdict_haskey(resp, "error"));
+    g_assert(qdict_haskey(resp, "return"));
+
+    return qdict_get_qdict(resp, "return");
 }
 
 /*
@@ -44,12 +53,18 @@ QDict *wait_command_fd(QTestState *who, int fd, const char *command, ...)
 QDict *wait_command(QTestState *who, const char *command, ...)
 {
     va_list ap;
+    QDict *resp;
 
     va_start(ap, command);
-    qtest_qmp_vsend(who, command, ap);
+    resp = qtest_vqmp(who, command, ap);
     va_end(ap);
 
-    return qtest_qmp_receive_success(who, stop_cb, NULL);
+    check_stop_event(who);
+
+    g_assert(!qdict_haskey(resp, "error"));
+    g_assert(qdict_haskey(resp, "return"));
+
+    return qdict_get_qdict(resp, "return");
 }
 
 /*
diff --git a/tests/qtest/pvpanic-test.c b/tests/qtest/pvpanic-test.c
index b0b20ad340..0657de797f 100644
--- a/tests/qtest/pvpanic-test.c
+++ b/tests/qtest/pvpanic-test.c
@@ -24,9 +24,7 @@ static void test_panic(void)
 
     qtest_outb(qts, 0x505, 0x1);
 
-    response = qtest_qmp_receive_dict(qts);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert_cmpstr(qdict_get_str(response, "event"), ==, "GUEST_PANICKED");
+    response = qtest_qmp_eventwait_ref(qts, "GUEST_PANICKED");
     g_assert(qdict_haskey(response, "data"));
     data = qdict_get_qdict(response, "data");
     g_assert(qdict_haskey(data, "action"));
diff --git a/tests/qtest/tpm-util.c b/tests/qtest/tpm-util.c
index 3ed6c8548a..5a33a6ef0f 100644
--- a/tests/qtest/tpm-util.c
+++ b/tests/qtest/tpm-util.c
@@ -237,12 +237,16 @@ void tpm_util_migrate(QTestState *who, const char *uri)
 void tpm_util_wait_for_migration_complete(QTestState *who)
 {
     while (true) {
+        QDict *rsp;
         QDict *rsp_return;
         bool completed;
         const char *status;
 
-        qtest_qmp_send(who, "{ 'execute': 'query-migrate' }");
-        rsp_return = qtest_qmp_receive_success(who, NULL, NULL);
+        rsp = qtest_qmp(who, "{ 'execute': 'query-migrate' }");
+        g_assert(qdict_haskey(rsp, "return"));
+        rsp_return = qdict_get_qdict(rsp, "return");
+
+        g_assert(!qdict_haskey(rsp_return, "error"));
         status = qdict_get_str(rsp_return, "status");
         completed = strcmp(status, "completed") == 0;
         g_assert_cmpstr(status, !=,  "failed");
-- 
2.26.2

[PATCH v7 04/13] qdev: add "check if address free" callback for buses

[Maxim Levitsky]

From: Paolo Bonzini <pbonzini@redhat.com>

Check if an address is free on the bus before plugging in the
device.  This makes it possible to do the check without any
side effects, and to detect the problem early without having
to do it in the realize callback.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/qdev.c         | 17 +++++++++++++++--
 hw/net/virtio-net.c    |  2 +-
 hw/sd/core.c           |  3 ++-
 include/hw/qdev-core.h | 13 ++++++++++++-
 4 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 96772a15bd..74db78df36 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -94,13 +94,23 @@ static void bus_add_child(BusState *bus, DeviceState *child)
                              0);
 }
 
-void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
+static bool bus_check_address(BusState *bus, DeviceState *child, Error **errp)
+{
+    BusClass *bc = BUS_GET_CLASS(bus);
+    return !bc->check_address || bc->check_address(bus, child, errp);
+}
+
+bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp)
 {
     BusState *old_parent_bus = dev->parent_bus;
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
 
     assert(dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type));
 
+    if (!bus_check_address(bus, dev, errp)) {
+        return false;
+    }
+
     if (old_parent_bus) {
         trace_qdev_update_parent_bus(dev, object_get_typename(OBJECT(dev)),
             old_parent_bus, object_get_typename(OBJECT(old_parent_bus)),
@@ -126,6 +136,7 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
         object_unref(OBJECT(old_parent_bus));
         object_unref(OBJECT(dev));
     }
+    return true;
 }
 
 DeviceState *qdev_new(const char *name)
@@ -371,7 +382,9 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
     assert(!dev->realized && !dev->parent_bus);
 
     if (bus) {
-        qdev_set_parent_bus(dev, bus);
+        if (!qdev_set_parent_bus(dev, bus, errp)) {
+            return false;
+        }
     } else {
         assert(!DEVICE_GET_CLASS(dev)->bus_type);
     }
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 7bf27b9db7..268cecc498 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3142,7 +3142,7 @@ static bool failover_replug_primary(VirtIONet *n, Error **errp)
         error_setg(errp, "virtio_net: couldn't find primary bus");
         return false;
     }
-    qdev_set_parent_bus(n->primary_dev, n->primary_bus);
+    qdev_set_parent_bus(n->primary_dev, n->primary_bus, &error_abort);
     n->primary_should_be_hidden = false;
     if (!qemu_opt_set_bool(n->primary_device_opts,
                            "partially_hotplugged", true, errp)) {
diff --git a/hw/sd/core.c b/hw/sd/core.c
index 957d116f1a..08c93b5903 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -23,6 +23,7 @@
 #include "hw/qdev-core.h"
 #include "hw/sd/sd.h"
 #include "qemu/module.h"
+#include "qapi/error.h"
 #include "trace.h"
 
 static inline const char *sdbus_name(SDBus *sdbus)
@@ -240,7 +241,7 @@ void sdbus_reparent_card(SDBus *from, SDBus *to)
     readonly = sc->get_readonly(card);
 
     sdbus_set_inserted(from, false);
-    qdev_set_parent_bus(DEVICE(card), &to->qbus);
+    qdev_set_parent_bus(DEVICE(card), &to->qbus, &error_abort);
     sdbus_set_inserted(to, true);
     sdbus_set_readonly(to, readonly);
 }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 72064f4dd4..14d476c587 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -210,13 +210,24 @@ struct BusClass {
     /* FIXME first arg should be BusState */
     void (*print_dev)(Monitor *mon, DeviceState *dev, int indent);
     char *(*get_dev_path)(DeviceState *dev);
+
     /*
      * This callback is used to create Open Firmware device path in accordance
      * with OF spec http://forthworks.com/standards/of1275.pdf. Individual bus
      * bindings can be found at http://playground.sun.com/1275/bindings/.
      */
     char *(*get_fw_dev_path)(DeviceState *dev);
+
     void (*reset)(BusState *bus);
+
+    /*
+     * Return whether the device can be added to @bus,
+     * based on the address that was set (via device properties)
+     * before realize.  If not, on return @errp contains the
+     * human-readable error message.
+     */
+    bool (*check_address)(BusState *bus, DeviceState *dev, Error **errp);
+
     BusRealize realize;
     BusUnrealize unrealize;
 
@@ -788,7 +799,7 @@ const char *qdev_fw_name(DeviceState *dev);
 Object *qdev_get_machine(void);
 
 /* FIXME: make this a link<> */
-void qdev_set_parent_bus(DeviceState *dev, BusState *bus);
+bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
 
 extern bool qdev_hotplug;
 extern bool qdev_hot_removed;
-- 
2.26.2

[PATCH v7 05/13] scsi: switch to bus->check_address

[Maxim Levitsky]

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-bus.c | 122 ++++++++++++++++++++++++++++-----------------
 1 file changed, 75 insertions(+), 47 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 3284a5d1fb..94921c04b1 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -22,33 +22,6 @@ static void scsi_req_dequeue(SCSIRequest *req);
 static uint8_t *scsi_target_alloc_buf(SCSIRequest *req, size_t len);
 static void scsi_target_free_buf(SCSIRequest *req);
 
-static Property scsi_props[] = {
-    DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),
-    DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1),
-    DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void scsi_bus_class_init(ObjectClass *klass, void *data)
-{
-    BusClass *k = BUS_CLASS(klass);
-    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
-
-    k->get_dev_path = scsibus_get_dev_path;
-    k->get_fw_dev_path = scsibus_get_fw_dev_path;
-    hc->unplug = qdev_simple_device_unplug_cb;
-}
-
-static const TypeInfo scsi_bus_info = {
-    .name = TYPE_SCSI_BUS,
-    .parent = TYPE_BUS,
-    .instance_size = sizeof(SCSIBus),
-    .class_init = scsi_bus_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { TYPE_HOTPLUG_HANDLER },
-        { }
-    }
-};
 static int next_scsi_bus;
 
 static void scsi_device_realize(SCSIDevice *s, Error **errp)
@@ -160,35 +133,68 @@ static void scsi_dma_restart_cb(void *opaque, int running, RunState state)
     }
 }
 
-static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
+static bool scsi_bus_is_address_free(SCSIBus *bus,
+				     int channel, int target, int lun,
+				     SCSIDevice **p_dev)
+{
+    SCSIDevice *d = scsi_device_find(bus, channel, target, lun);
+    if (d && d->lun == lun) {
+        if (p_dev) {
+            *p_dev = d;
+        }
+        return false;
+    }
+    if (p_dev) {
+        *p_dev = NULL;
+    }
+    return true;
+}
+
+static bool scsi_bus_check_address(BusState *qbus, DeviceState *qdev, Error **errp)
 {
     SCSIDevice *dev = SCSI_DEVICE(qdev);
-    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
-    SCSIDevice *d;
-    Error *local_err = NULL;
+    SCSIBus *bus = SCSI_BUS(qbus);
 
     if (dev->channel > bus->info->max_channel) {
         error_setg(errp, "bad scsi channel id: %d", dev->channel);
-        return;
+        return false;
     }
     if (dev->id != -1 && dev->id > bus->info->max_target) {
         error_setg(errp, "bad scsi device id: %d", dev->id);
-        return;
+        return false;
     }
     if (dev->lun != -1 && dev->lun > bus->info->max_lun) {
         error_setg(errp, "bad scsi device lun: %d", dev->lun);
-        return;
+        return false;
+    }
+
+    if (dev->id != -1 && dev->lun != -1) {
+        SCSIDevice *d;
+        if (!scsi_bus_is_address_free(bus, dev->channel, dev->id, dev->lun, &d)) {
+            error_setg(errp, "lun already used by '%s'", d->qdev.id);
+            return false;
+        }
     }
 
+    return true;
+}
+
+static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
+{
+    SCSIDevice *dev = SCSI_DEVICE(qdev);
+    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
+    bool is_free;
+    Error *local_err = NULL;
+
     if (dev->id == -1) {
         int id = -1;
         if (dev->lun == -1) {
             dev->lun = 0;
         }
         do {
-            d = scsi_device_find(bus, dev->channel, ++id, dev->lun);
-        } while (d && d->lun == dev->lun && id < bus->info->max_target);
-        if (d && d->lun == dev->lun) {
+            is_free = scsi_bus_is_address_free(bus, dev->channel, ++id, dev->lun, NULL);
+        } while (!is_free && id < bus->info->max_target);
+        if (!is_free) {
             error_setg(errp, "no free target");
             return;
         }
@@ -196,20 +202,13 @@ static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
     } else if (dev->lun == -1) {
         int lun = -1;
         do {
-            d = scsi_device_find(bus, dev->channel, dev->id, ++lun);
-        } while (d && d->lun == lun && lun < bus->info->max_lun);
-        if (d && d->lun == lun) {
+            is_free = scsi_bus_is_address_free(bus, dev->channel, dev->id, ++lun, NULL);
+        } while (!is_free && lun < bus->info->max_lun);
+        if (!is_free) {
             error_setg(errp, "no free lun");
             return;
         }
         dev->lun = lun;
-    } else {
-        d = scsi_device_find(bus, dev->channel, dev->id, dev->lun);
-        assert(d);
-        if (d->lun == dev->lun && dev != d) {
-            error_setg(errp, "lun already used by '%s'", d->qdev.id);
-            return;
-        }
     }
 
     QTAILQ_INIT(&dev->requests);
@@ -1709,6 +1708,13 @@ const VMStateDescription vmstate_scsi_device = {
     }
 };
 
+static Property scsi_props[] = {
+    DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),
+    DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1),
+    DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void scsi_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
@@ -1739,6 +1745,28 @@ static const TypeInfo scsi_device_type_info = {
     .instance_init = scsi_dev_instance_init,
 };
 
+static void scsi_bus_class_init(ObjectClass *klass, void *data)
+{
+    BusClass *k = BUS_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
+
+    k->get_dev_path = scsibus_get_dev_path;
+    k->get_fw_dev_path = scsibus_get_fw_dev_path;
+    k->check_address = scsi_bus_check_address;
+    hc->unplug = qdev_simple_device_unplug_cb;
+}
+
+static const TypeInfo scsi_bus_info = {
+    .name = TYPE_SCSI_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = sizeof(SCSIBus),
+    .class_init = scsi_bus_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
+};
+
 static void scsi_register_types(void)
 {
     type_register_static(&scsi_bus_info);
-- 
2.26.2

[PATCH v7 06/13] scsi/scsi_bus: switch search direction in scsi_device_find

[Maxim Levitsky]

This change will allow us to convert the bus children list to RCU,
while not changing the logic of this function

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-2-mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-bus.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 94921c04b1..69d7c3f90c 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1571,7 +1571,7 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
     BusChild *kid;
     SCSIDevice *target_dev = NULL;
 
-    QTAILQ_FOREACH_REVERSE(kid, &bus->qbus.children, sibling) {
+    QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
         DeviceState *qdev = kid->child;
         SCSIDevice *dev = SCSI_DEVICE(qdev);
 
@@ -1579,7 +1579,15 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
             if (dev->lun == lun) {
                 return dev;
             }
-            target_dev = dev;
+
+            /*
+             * If we don't find exact match (channel/bus/lun),
+             * we will return the first device which matches channel/bus
+             */
+
+            if (!target_dev) {
+                target_dev = dev;
+            }
         }
     }
     return target_dev;
-- 
2.26.2

[PATCH v7 07/13] device_core: use drain_call_rcu in in qmp_device_add

[Maxim Levitsky]

Soon, a device removal might only happen on RCU callback execution.
This is okay for device-del which provides a DEVICE_DELETED event,
but not for the failure case of device-add.  To avoid changing
monitor semantics, just drain all pending RCU callbacks on error.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-4-mlevitsk@redhat.com>
[Don't use it in qmp_device_del. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qdev-monitor.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index e9b7228480..bcfb90a08f 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -803,6 +803,18 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
         return;
     }
     dev = qdev_device_add(opts, errp);
+
+    /*
+     * Drain all pending RCU callbacks. This is done because
+     * some bus related operations can delay a device removal
+     * (in this case this can happen if device is added and then
+     * removed due to a configuration error)
+     * to a RCU callback, but user might expect that this interface
+     * will finish its job completely once qmp command returns result
+     * to the user
+     */
+    drain_call_rcu();
+
     if (!dev) {
         qemu_opts_del(opts);
         return;
-- 
2.26.2

[PATCH v7 08/13] device-core: use RCU for list of children of a bus

[Maxim Levitsky]

This fixes the race between device emulation code that tries to find
a child device to dispatch the request to (e.g a scsi disk),
and hotplug of a new device to that bus.

Note that this doesn't convert all the readers of the list
but only these that might go over that list without BQL held.

This is a very small first step to make this code thread safe.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-5-mlevitsk@redhat.com>
[Use RCU_READ_LOCK_GUARD in more places, adjust testcase now that
 the delay in DEVICE_DELETED due to RCU is more consistent. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/bus.c          | 28 +++++++++++++++++-----------
 hw/core/qdev.c         | 37 +++++++++++++++++++++++--------------
 hw/scsi/scsi-bus.c     | 12 +++++++++---
 hw/scsi/virtio-scsi.c  |  6 +++++-
 include/hw/qdev-core.h |  9 +++++++++
 5 files changed, 63 insertions(+), 29 deletions(-)

diff --git a/hw/core/bus.c b/hw/core/bus.c
index 6b987b6946..a0483859ae 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -49,12 +49,14 @@ int qbus_walk_children(BusState *bus,
         }
     }
 
-    QTAILQ_FOREACH(kid, &bus->children, sibling) {
-        err = qdev_walk_children(kid->child,
-                                 pre_devfn, pre_busfn,
-                                 post_devfn, post_busfn, opaque);
-        if (err < 0) {
-            return err;
+    WITH_RCU_READ_LOCK_GUARD() {
+        QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
+            err = qdev_walk_children(kid->child,
+                                     pre_devfn, pre_busfn,
+                                     post_devfn, post_busfn, opaque);
+            if (err < 0) {
+                return err;
+            }
         }
     }
 
@@ -90,8 +92,10 @@ static void bus_reset_child_foreach(Object *obj, ResettableChildCallback cb,
     BusState *bus = BUS(obj);
     BusChild *kid;
 
-    QTAILQ_FOREACH(kid, &bus->children, sibling) {
-        cb(OBJECT(kid->child), opaque, type);
+    WITH_RCU_READ_LOCK_GUARD() {
+        QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
+            cb(OBJECT(kid->child), opaque, type);
+        }
     }
 }
 
@@ -194,9 +198,11 @@ static void bus_set_realized(Object *obj, bool value, Error **errp)
 
         /* TODO: recursive realization */
     } else if (!value && bus->realized) {
-        QTAILQ_FOREACH(kid, &bus->children, sibling) {
-            DeviceState *dev = kid->child;
-            qdev_unrealize(dev);
+        WITH_RCU_READ_LOCK_GUARD() {
+            QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
+                DeviceState *dev = kid->child;
+                qdev_unrealize(dev);
+            }
         }
         if (bc->unrealize) {
             bc->unrealize(bus);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 74db78df36..59e5e710b7 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -51,6 +51,12 @@ const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
     return dc->vmsd;
 }
 
+static void bus_free_bus_child(BusChild *kid)
+{
+    object_unref(OBJECT(kid->child));
+    g_free(kid);
+}
+
 static void bus_remove_child(BusState *bus, DeviceState *child)
 {
     BusChild *kid;
@@ -60,15 +66,16 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
             char name[32];
 
             snprintf(name, sizeof(name), "child[%d]", kid->index);
-            QTAILQ_REMOVE(&bus->children, kid, sibling);
+            QTAILQ_REMOVE_RCU(&bus->children, kid, sibling);
 
             bus->num_children--;
 
             /* This gives back ownership of kid->child back to us.  */
             object_property_del(OBJECT(bus), name);
-            object_unref(OBJECT(kid->child));
-            g_free(kid);
-            return;
+
+            /* free the bus kid, when it is safe to do so*/
+            call_rcu(kid, bus_free_bus_child, rcu);
+            break;
         }
     }
 }
@@ -83,7 +90,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
     kid->child = child;
     object_ref(OBJECT(kid->child));
 
-    QTAILQ_INSERT_HEAD(&bus->children, kid, sibling);
+    QTAILQ_INSERT_HEAD_RCU(&bus->children, kid, sibling);
 
     /* This transfers ownership of kid->child to the property.  */
     snprintf(name, sizeof(name), "child[%d]", kid->index);
@@ -672,17 +679,19 @@ DeviceState *qdev_find_recursive(BusState *bus, const char *id)
     DeviceState *ret;
     BusState *child;
 
-    QTAILQ_FOREACH(kid, &bus->children, sibling) {
-        DeviceState *dev = kid->child;
+    WITH_RCU_READ_LOCK_GUARD() {
+        QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
+            DeviceState *dev = kid->child;
 
-        if (dev->id && strcmp(dev->id, id) == 0) {
-            return dev;
-        }
+            if (dev->id && strcmp(dev->id, id) == 0) {
+                return dev;
+            }
 
-        QLIST_FOREACH(child, &dev->child_bus, sibling) {
-            ret = qdev_find_recursive(child, id);
-            if (ret) {
-                return ret;
+            QLIST_FOREACH(child, &dev->child_bus, sibling) {
+                ret = qdev_find_recursive(child, id);
+                if (ret) {
+                    return ret;
+                }
             }
         }
     }
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 69d7c3f90c..4ab9811cd8 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -399,7 +399,10 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
     id = r->req.dev->id;
     found_lun0 = false;
     n = 0;
-    QTAILQ_FOREACH(kid, &r->req.bus->qbus.children, sibling) {
+
+    RCU_READ_LOCK_GUARD();
+
+    QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
         DeviceState *qdev = kid->child;
         SCSIDevice *dev = SCSI_DEVICE(qdev);
 
@@ -420,7 +423,7 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
     memset(r->buf, 0, len);
     stl_be_p(&r->buf[0], n);
     i = found_lun0 ? 8 : 16;
-    QTAILQ_FOREACH(kid, &r->req.bus->qbus.children, sibling) {
+    QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
         DeviceState *qdev = kid->child;
         SCSIDevice *dev = SCSI_DEVICE(qdev);
 
@@ -429,6 +432,7 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
             i += 8;
         }
     }
+
     assert(i == n + 8);
     r->len = len;
     return true;
@@ -1571,7 +1575,8 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
     BusChild *kid;
     SCSIDevice *target_dev = NULL;
 
-    QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
+    RCU_READ_LOCK_GUARD();
+    QTAILQ_FOREACH_RCU(kid, &bus->qbus.children, sibling) {
         DeviceState *qdev = kid->child;
         SCSIDevice *dev = SCSI_DEVICE(qdev);
 
@@ -1590,6 +1595,7 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
             }
         }
     }
+
     return target_dev;
 }
 
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 3a71ea7097..971afbb217 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -367,12 +367,16 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
     case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
         target = req->req.tmf.lun[1];
         s->resetting++;
-        QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
+
+        rcu_read_lock();
+        QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) {
              d = SCSI_DEVICE(kid->child);
              if (d->channel == 0 && d->id == target) {
                 qdev_reset_all(&d->qdev);
              }
         }
+        rcu_read_unlock();
+
         s->resetting--;
         break;
 
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 14d476c587..2c6307e3ed 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -3,6 +3,8 @@
 
 #include "qemu/queue.h"
 #include "qemu/bitmap.h"
+#include "qemu/rcu.h"
+#include "qemu/rcu_queue.h"
 #include "qom/object.h"
 #include "hw/hotplug.h"
 #include "hw/resettable.h"
@@ -238,6 +240,7 @@ struct BusClass {
 };
 
 typedef struct BusChild {
+    struct rcu_head rcu;
     DeviceState *child;
     int index;
     QTAILQ_ENTRY(BusChild) sibling;
@@ -258,6 +261,12 @@ struct BusState {
     int max_index;
     bool realized;
     int num_children;
+
+    /*
+     * children is a RCU QTAILQ, thus readers must use RCU to access it,
+     * and writers must hold the big qemu lock
+     */
+
     QTAILQ_HEAD(, BusChild) children;
     QLIST_ENTRY(BusState) sibling;
     ResettableState reset;
-- 
2.26.2

[PATCH v7 09/13] device-core: use atomic_set on .realized property

[Maxim Levitsky]

Some code might race with placement of new devices on a bus.
We currently first place a (unrealized) device on the bus
and then realize it.

As a workaround, users that scan the child device list, can
check the realized property to see if it is safe to access such a device.
Use an atomic write here too to aid with this.

A separate discussion is what to do with devices that are unrealized:
It looks like for this case we only call the hotplug handler's unplug
callback and its up to it to unrealize the device.
An atomic operation doesn't cause harm for this code path though.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-6-mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/qdev.c         | 19 ++++++++++++++++++-
 include/hw/qdev-core.h |  2 ++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 59e5e710b7..fc4daa36fa 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -946,7 +946,25 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             }
        }
 
+       qatomic_store_release(&dev->realized, value);
+
     } else if (!value && dev->realized) {
+
+        /*
+         * Change the value so that any concurrent users are aware
+         * that the device is going to be unrealized
+         *
+         * TODO: change .realized property to enum that states
+         * each phase of the device realization/unrealization
+         */
+
+        qatomic_set(&dev->realized, value);
+        /*
+         * Ensure that concurrent users see this update prior to
+         * any other changes done by unrealize.
+         */
+        smp_wmb();
+
         QLIST_FOREACH(bus, &dev->child_bus, sibling) {
             qbus_unrealize(bus);
         }
@@ -961,7 +979,6 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
     }
 
     assert(local_err == NULL);
-    dev->realized = value;
     return;
 
 child_realize_fail:
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 2c6307e3ed..868973319e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -163,6 +163,8 @@ struct NamedClockList {
 /**
  * DeviceState:
  * @realized: Indicates whether the device has been fully constructed.
+ *            When accessed outsize big qemu lock, must be accessed with
+ *            atomic_load_acquire()
  * @reset: ResettableState for the device; handled by Resettable interface.
  *
  * This structure should not be accessed directly.  We declare it here
-- 
2.26.2

[PATCH v7 10/13] scsi/scsi-bus: scsi_device_find: don't return unrealized devices

[Maxim Levitsky]

From: Paolo Bonzini <pbonzini@redhat.com>

The device core first places a device on the bus and then realizes it.
Make scsi_device_find avoid returing such devices to avoid
races in drivers that use an iothread (currently virtio-scsi)

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1812399

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-7-mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-bus.c | 83 +++++++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 30 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 4ab9811cd8..7599113efe 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -24,6 +24,55 @@ static void scsi_target_free_buf(SCSIRequest *req);
 
 static int next_scsi_bus;
 
+static SCSIDevice *do_scsi_device_find(SCSIBus *bus,
+                                       int channel, int id, int lun,
+                                       bool include_unrealized)
+{
+    BusChild *kid;
+    SCSIDevice *retval = NULL;
+
+    QTAILQ_FOREACH_RCU(kid, &bus->qbus.children, sibling) {
+        DeviceState *qdev = kid->child;
+        SCSIDevice *dev = SCSI_DEVICE(qdev);
+
+        if (dev->channel == channel && dev->id == id) {
+            if (dev->lun == lun) {
+                retval = dev;
+                break;
+            }
+
+            /*
+             * If we don't find exact match (channel/bus/lun),
+             * we will return the first device which matches channel/bus
+             */
+
+            if (!retval) {
+                retval = dev;
+            }
+        }
+    }
+
+    /*
+     * This function might run on the IO thread and we might race against
+     * main thread hot-plugging the device.
+     * We assume that as soon as .realized is set to true we can let
+     * the user access the device.
+     */
+
+    if (retval && !include_unrealized &&
+        !qatomic_load_acquire(&retval->qdev.realized)) {
+        retval = NULL;
+    }
+
+    return retval;
+}
+
+SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
+{
+    RCU_READ_LOCK_GUARD();
+    return do_scsi_device_find(bus, channel, id, lun, false);
+}
+
 static void scsi_device_realize(SCSIDevice *s, Error **errp)
 {
     SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
@@ -137,7 +186,10 @@ static bool scsi_bus_is_address_free(SCSIBus *bus,
 				     int channel, int target, int lun,
 				     SCSIDevice **p_dev)
 {
-    SCSIDevice *d = scsi_device_find(bus, channel, target, lun);
+    SCSIDevice *d;
+
+    RCU_READ_LOCK_GUARD();
+    d = do_scsi_device_find(bus, channel, target, lun, true);
     if (d && d->lun == lun) {
         if (p_dev) {
             *p_dev = d;
@@ -1570,35 +1622,6 @@ static char *scsibus_get_fw_dev_path(DeviceState *dev)
                            qdev_fw_name(dev), d->id, d->lun);
 }
 
-SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
-{
-    BusChild *kid;
-    SCSIDevice *target_dev = NULL;
-
-    RCU_READ_LOCK_GUARD();
-    QTAILQ_FOREACH_RCU(kid, &bus->qbus.children, sibling) {
-        DeviceState *qdev = kid->child;
-        SCSIDevice *dev = SCSI_DEVICE(qdev);
-
-        if (dev->channel == channel && dev->id == id) {
-            if (dev->lun == lun) {
-                return dev;
-            }
-
-            /*
-             * If we don't find exact match (channel/bus/lun),
-             * we will return the first device which matches channel/bus
-             */
-
-            if (!target_dev) {
-                target_dev = dev;
-            }
-        }
-    }
-
-    return target_dev;
-}
-
 /* SCSI request list.  For simplicity, pv points to the whole device */
 
 static int put_scsi_requests(QEMUFile *f, void *pv, size_t size,
-- 
2.26.2

[PATCH v7 11/13] scsi/scsi_bus: Add scsi_device_get

[Maxim Levitsky]

Add scsi_device_get which finds the scsi device
and takes a reference to it.

Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Message-Id: <20200913160259.32145-8-mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-bus.c     | 11 +++++++++++
 include/hw/scsi/scsi.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 7599113efe..eda8cb7e70 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -73,6 +73,17 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
     return do_scsi_device_find(bus, channel, id, lun, false);
 }
 
+SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int id, int lun)
+{
+    SCSIDevice *d;
+    RCU_READ_LOCK_GUARD();
+    d = do_scsi_device_find(bus, channel, id, lun, false);
+    if (d) {
+        object_ref(d);
+    }
+    return d;
+}
+
 static void scsi_device_realize(SCSIDevice *s, Error **errp)
 {
     SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 7a55cdbd74..09fa5c9d2a 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -190,6 +190,7 @@ int scsi_device_get_sense(SCSIDevice *dev, uint8_t *buf, int len, bool fixed);
 int scsi_SG_IO_FROM_DEV(BlockBackend *blk, uint8_t *cmd, uint8_t cmd_size,
                         uint8_t *buf, uint8_t buf_size);
 SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int target, int lun);
+SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int target, int lun);
 
 /* scsi-generic.c. */
 extern const SCSIReqOps scsi_generic_req_ops;
-- 
2.26.2

[PATCH v7 12/13] virtio-scsi: use scsi_device_get

[Maxim Levitsky]

This will help us to avoid the scsi device disappearing
after we took a reference to it.

It doesn't by itself forbid case when we try to access
an unrealized device

Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-9-mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/virtio-scsi.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 971afbb217..3db9a8aae9 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -33,7 +33,7 @@ static inline int virtio_scsi_get_lun(uint8_t *lun)
     return ((lun[2] << 8) | lun[3]) & 0x3FFF;
 }
 
-static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
+static inline SCSIDevice *virtio_scsi_device_get(VirtIOSCSI *s, uint8_t *lun)
 {
     if (lun[0] != 1) {
         return NULL;
@@ -41,7 +41,7 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
     if (lun[2] != 0 && !(lun[2] >= 0x40 && lun[2] < 0x80)) {
         return NULL;
     }
-    return scsi_device_find(&s->bus, 0, lun[1], virtio_scsi_get_lun(lun));
+    return scsi_device_get(&s->bus, 0, lun[1], virtio_scsi_get_lun(lun));
 }
 
 void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
@@ -256,7 +256,7 @@ static inline void virtio_scsi_ctx_check(VirtIOSCSI *s, SCSIDevice *d)
  *  case of async cancellation. */
 static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
 {
-    SCSIDevice *d = virtio_scsi_device_find(s, req->req.tmf.lun);
+    SCSIDevice *d = virtio_scsi_device_get(s, req->req.tmf.lun);
     SCSIRequest *r, *next;
     BusChild *kid;
     int target;
@@ -370,10 +370,10 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
 
         rcu_read_lock();
         QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) {
-             d = SCSI_DEVICE(kid->child);
-             if (d->channel == 0 && d->id == target) {
-                qdev_reset_all(&d->qdev);
-             }
+            SCSIDevice *d1 = SCSI_DEVICE(kid->child);
+            if (d1->channel == 0 && d1->id == target) {
+                qdev_reset_all(&d1->qdev);
+            }
         }
         rcu_read_unlock();
 
@@ -386,14 +386,17 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
         break;
     }
 
+    object_unref(OBJECT(d));
     return ret;
 
 incorrect_lun:
     req->resp.tmf.response = VIRTIO_SCSI_S_INCORRECT_LUN;
+    object_unref(OBJECT(d));
     return ret;
 
 fail:
     req->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET;
+    object_unref(OBJECT(d));
     return ret;
 }
 
@@ -564,7 +567,7 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
         }
     }
 
-    d = virtio_scsi_device_find(s, req->req.cmd.lun);
+    d = virtio_scsi_device_get(s, req->req.cmd.lun);
     if (!d) {
         req->resp.cmd.response = VIRTIO_SCSI_S_BAD_TARGET;
         virtio_scsi_complete_cmd_req(req);
@@ -580,10 +583,12 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
             req->sreq->cmd.xfer > req->qsgl.size)) {
         req->resp.cmd.response = VIRTIO_SCSI_S_OVERRUN;
         virtio_scsi_complete_cmd_req(req);
+        object_unref(OBJECT(d));
         return -ENOBUFS;
     }
     scsi_req_ref(req->sreq);
     blk_io_plug(d->conf.blk);
+    object_unref(OBJECT(d));
     return 0;
 }
 
-- 
2.26.2

[PATCH v7 13/13] scsi/scsi_bus: fix races in REPORT LUNS

[Maxim Levitsky]

Currently scsi_target_emulate_report_luns iterates over the child device list
twice, and there is no guarantee that this list is the same in both iterations.

The reason for iterating twice is that the first iteration calculates
how much memory to allocate.  However if we use a dynamic array we can
avoid iterating twice, and therefore we avoid this race.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1866707

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-10-mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-bus.c | 68 ++++++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 35 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index eda8cb7e70..b901e701f0 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -438,19 +438,23 @@ struct SCSITargetReq {
 static void store_lun(uint8_t *outbuf, int lun)
 {
     if (lun < 256) {
+        /* Simple logical unit addressing method*/
+        outbuf[0] = 0;
         outbuf[1] = lun;
-        return;
+    } else {
+        /* Flat space addressing method */
+        outbuf[0] = 0x40 | (lun >> 8);
+        outbuf[1] = (lun & 255);
     }
-    outbuf[1] = (lun & 255);
-    outbuf[0] = (lun >> 8) | 0x40;
 }
 
 static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
 {
     BusChild *kid;
-    int i, len, n;
     int channel, id;
-    bool found_lun0;
+    uint8_t tmp[8] = {0};
+    int len = 0;
+    GByteArray *buf;
 
     if (r->req.cmd.xfer < 16) {
         return false;
@@ -458,46 +462,40 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
     if (r->req.cmd.buf[2] > 2) {
         return false;
     }
+
+    /* reserve space for 63 LUNs*/
+    buf = g_byte_array_sized_new(512);
+
     channel = r->req.dev->channel;
     id = r->req.dev->id;
-    found_lun0 = false;
-    n = 0;
 
-    RCU_READ_LOCK_GUARD();
+    /* add size (will be updated later to correct value */
+    g_byte_array_append(buf, tmp, 8);
+    len += 8;
 
-    QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
-        DeviceState *qdev = kid->child;
-        SCSIDevice *dev = SCSI_DEVICE(qdev);
+    /* add LUN0 */
+    g_byte_array_append(buf, tmp, 8);
+    len += 8;
 
-        if (dev->channel == channel && dev->id == id) {
-            if (dev->lun == 0) {
-                found_lun0 = true;
+    WITH_RCU_READ_LOCK_GUARD() {
+        QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
+            DeviceState *qdev = kid->child;
+            SCSIDevice *dev = SCSI_DEVICE(qdev);
+
+            if (dev->channel == channel && dev->id == id && dev->lun != 0) {
+                store_lun(tmp, dev->lun);
+                g_byte_array_append(buf, tmp, 8);
+                len += 8;
             }
-            n += 8;
         }
     }
-    if (!found_lun0) {
-        n += 8;
-    }
-
-    scsi_target_alloc_buf(&r->req, n + 8);
-
-    len = MIN(n + 8, r->req.cmd.xfer & ~7);
-    memset(r->buf, 0, len);
-    stl_be_p(&r->buf[0], n);
-    i = found_lun0 ? 8 : 16;
-    QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
-        DeviceState *qdev = kid->child;
-        SCSIDevice *dev = SCSI_DEVICE(qdev);
 
-        if (dev->channel == channel && dev->id == id) {
-            store_lun(&r->buf[i], dev->lun);
-            i += 8;
-        }
-    }
+    r->buf_len = len;
+    r->buf = g_byte_array_free(buf, FALSE);
+    r->len = MIN(len, r->req.cmd.xfer & ~7);
 
-    assert(i == n + 8);
-    r->len = len;
+    /* store the LUN list length */
+    stl_be_p(&r->buf[0], len - 8);
     return true;
 }
 
-- 
2.26.2

Re: [PATCH v7 03/13] qtest: switch users back to qtest_qmp_receive

[Paolo Bonzini]

On 06/10/20 14:38, Maxim Levitsky wrote:
> The only remaining users of qtest_qmp_receive_dict are tests
> that fuzz the QMP protocol.
> 
> Tested with 'make check-qtest'.

Probably the qtest_qmp_receive_success conversion should have been in a
separate patch.  But it's a nice side effect that I hadn't anticipated!

Paolo

> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  tests/qtest/ahci-test.c         |  4 +-
>  tests/qtest/device-plug-test.c  |  2 +-
>  tests/qtest/drive_del-test.c    |  9 ++---
>  tests/qtest/libqos/libqtest.h   | 17 ---------
>  tests/qtest/libqtest.c          | 65 ++++-----------------------------
>  tests/qtest/migration-helpers.c | 25 ++++++++++---
>  tests/qtest/pvpanic-test.c      |  4 +-
>  tests/qtest/tpm-util.c          |  8 +++-
>  8 files changed, 41 insertions(+), 93 deletions(-)
> 
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index d42ebaeb4c..5e1954852e 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -1590,7 +1590,7 @@ static void test_atapi_tray(void)
>      qtest_qmp_send(ahci->parent->qts, "{'execute': 'blockdev-open-tray', "
>                      "'arguments': {'id': 'cd0'}}");
>      atapi_wait_tray(ahci, true);
> -    rsp = qtest_qmp_receive_dict(ahci->parent->qts);
> +    rsp = qtest_qmp_receive(ahci->parent->qts);
>      qobject_unref(rsp);
>  
>      qmp_discard_response(ahci->parent->qts,
> @@ -1620,7 +1620,7 @@ static void test_atapi_tray(void)
>      qtest_qmp_send(ahci->parent->qts, "{'execute': 'blockdev-close-tray', "
>                     "'arguments': {'id': 'cd0'}}");
>      atapi_wait_tray(ahci, false);
> -    rsp = qtest_qmp_receive_dict(ahci->parent->qts);
> +    rsp = qtest_qmp_receive(ahci->parent->qts);
>      qobject_unref(rsp);
>  
>      /* Now, to convince ATAPI we understand the media has changed... */
> diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c
> index a2247856be..9214892741 100644
> --- a/tests/qtest/device-plug-test.c
> +++ b/tests/qtest/device-plug-test.c
> @@ -23,7 +23,7 @@ static void device_del_start(QTestState *qtest, const char *id)
>  
>  static void device_del_finish(QTestState *qtest)
>  {
> -    QDict *resp = qtest_qmp_receive_dict(qtest);
> +    QDict *resp = qtest_qmp_receive(qtest);
>  
>      g_assert(qdict_haskey(resp, "return"));
>      qobject_unref(resp);
> diff --git a/tests/qtest/drive_del-test.c b/tests/qtest/drive_del-test.c
> index ba0cd77445..9d20a1ed8b 100644
> --- a/tests/qtest/drive_del-test.c
> +++ b/tests/qtest/drive_del-test.c
> @@ -15,9 +15,6 @@
>  #include "libqos/virtio.h"
>  #include "qapi/qmp/qdict.h"
>  
> -/* TODO actually test the results and get rid of this */
> -#define qmp_discard_response(q, ...) qobject_unref(qtest_qmp(q, __VA_ARGS__))
> -
>  static void drive_add(QTestState *qts)
>  {
>      char *resp = qtest_hmp(qts, "drive_add 0 if=none,id=drive0");
> @@ -38,13 +35,13 @@ static void device_del(QTestState *qts)
>  {
>      QDict *response;
>  
> -    /* Complication: ignore DEVICE_DELETED event */
> -    qmp_discard_response(qts, "{'execute': 'device_del',"
> +    response = qtest_qmp(qts, "{'execute': 'device_del',"
>                           " 'arguments': { 'id': 'dev0' } }");
> -    response = qtest_qmp_receive_dict(qts);
>      g_assert(response);
>      g_assert(qdict_haskey(response, "return"));
>      qobject_unref(response);
> +
> +    qtest_qmp_eventwait(qts, "DEVICE_DELETED");
>  }
>  
>  static void test_drive_without_dev(void)
> diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h
> index 19429a536d..a91e9e02e9 100644
> --- a/tests/qtest/libqos/libqtest.h
> +++ b/tests/qtest/libqos/libqtest.h
> @@ -240,23 +240,6 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
>   */
>  QDict *qtest_qmp_event_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/qtest/libqtest.c b/tests/qtest/libqtest.c
> index d4c49a52ff..08929f5ff6 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -324,7 +324,7 @@ QTestState *qtest_init(const char *extra_args)
>      QDict *greeting;
>  
>      /* Read the QMP greeting and then do the handshake */
> -    greeting = qtest_qmp_receive_dict(s);
> +    greeting = qtest_qmp_receive(s);
>      qobject_unref(greeting);
>      qobject_unref(qtest_qmp(s, "{ 'execute': 'qmp_capabilities' }"));
>  
> @@ -700,7 +700,7 @@ QDict *qtest_vqmp_fds(QTestState *s, int *fds, size_t fds_num,
>      qtest_qmp_vsend_fds(s, fds, fds_num, fmt, ap);
>  
>      /* Receive reply */
> -    return qtest_qmp_receive_dict(s);
> +    return qtest_qmp_receive(s);
>  }
>  
>  QDict *qtest_vqmp(QTestState *s, const char *fmt, va_list ap)
> @@ -708,7 +708,7 @@ QDict *qtest_vqmp(QTestState *s, const char *fmt, va_list ap)
>      qtest_qmp_vsend(s, fmt, ap);
>  
>      /* Receive reply */
> -    return qtest_qmp_receive_dict(s);
> +    return qtest_qmp_receive(s);
>  }
>  
>  QDict *qmp_fd(int fd, const char *fmt, ...)
> @@ -850,12 +850,6 @@ char *qtest_vhmp(QTestState *s, const char *fmt, va_list ap)
>                       " 'arguments': {'command-line': %s}}",
>                       cmd);
>      ret = g_strdup(qdict_get_try_str(resp, "return"));
> -    while (ret == NULL && qdict_get_try_str(resp, "event")) {
> -        /* Ignore asynchronous QMP events */
> -        qobject_unref(resp);
> -        resp = qtest_qmp_receive_dict(s);
> -        ret = g_strdup(qdict_get_try_str(resp, "return"));
> -    }
>      g_assert(ret);
>      qobject_unref(resp);
>      g_free(cmd);
> @@ -1291,35 +1285,6 @@ 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_dict(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 commands.
>   */
> @@ -1355,13 +1320,6 @@ void qtest_qmp_device_add(QTestState *qts, const char *driver, const char *id,
>      qobject_unref(args);
>  }
>  
> -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.
> @@ -1378,24 +1336,17 @@ static void device_deleted_cb(void *opaque, const char *name, QDict *data)
>   * and this one:
>   *
>   * {"return": {}}
> - *
> - * But the order of arrival may vary - so we've got to detect both.
>   */
>  void qtest_qmp_device_del(QTestState *qts, const char *id)
>  {
> -    bool got_event = false;
>      QDict *rsp;
>  
> -    qtest_qmp_send(qts, "{'execute': 'device_del', 'arguments': {'id': %s}}",
> -                   id);
> -    rsp = qtest_qmp_receive_success(qts, device_deleted_cb, &got_event);
> +    rsp = qtest_qmp(qts, "{'execute': 'device_del', 'arguments': {'id': %s}}",
> +                    id);
> +
> +    g_assert(qdict_haskey(rsp, "return"));
>      qobject_unref(rsp);
> -    if (!got_event) {
> -        rsp = qtest_qmp_receive_dict(qts);
> -        g_assert_cmpstr(qdict_get_try_str(rsp, "event"),
> -                        ==, "DEVICE_DELETED");
> -        qobject_unref(rsp);
> -    }
> +    qtest_qmp_eventwait(qts, "DEVICE_DELETED");
>  }
>  
>  bool qmp_rsp_is_err(QDict *rsp)
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index 516093b39a..b799dbafb7 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -17,10 +17,12 @@
>  
>  bool got_stop;
>  
> -static void stop_cb(void *opaque, const char *name, QDict *data)
> +static void check_stop_event(QTestState *who)
>  {
> -    if (!strcmp(name, "STOP")) {
> +    QDict *event = qtest_qmp_event_ref(who, "STOP");
> +    if (event) {
>          got_stop = true;
> +        qobject_unref(event);
>      }
>  }
>  
> @@ -30,12 +32,19 @@ static void stop_cb(void *opaque, const char *name, QDict *data)
>  QDict *wait_command_fd(QTestState *who, int fd, const char *command, ...)
>  {
>      va_list ap;
> +    QDict *resp;
>  
>      va_start(ap, command);
>      qtest_qmp_vsend_fds(who, &fd, 1, command, ap);
>      va_end(ap);
>  
> -    return qtest_qmp_receive_success(who, stop_cb, NULL);
> +    resp = qtest_qmp_receive(who);
> +    check_stop_event(who);
> +
> +    g_assert(!qdict_haskey(resp, "error"));
> +    g_assert(qdict_haskey(resp, "return"));
> +
> +    return qdict_get_qdict(resp, "return");
>  }
>  
>  /*
> @@ -44,12 +53,18 @@ QDict *wait_command_fd(QTestState *who, int fd, const char *command, ...)
>  QDict *wait_command(QTestState *who, const char *command, ...)
>  {
>      va_list ap;
> +    QDict *resp;
>  
>      va_start(ap, command);
> -    qtest_qmp_vsend(who, command, ap);
> +    resp = qtest_vqmp(who, command, ap);
>      va_end(ap);
>  
> -    return qtest_qmp_receive_success(who, stop_cb, NULL);
> +    check_stop_event(who);
> +
> +    g_assert(!qdict_haskey(resp, "error"));
> +    g_assert(qdict_haskey(resp, "return"));
> +
> +    return qdict_get_qdict(resp, "return");
>  }
>  
>  /*
> diff --git a/tests/qtest/pvpanic-test.c b/tests/qtest/pvpanic-test.c
> index b0b20ad340..0657de797f 100644
> --- a/tests/qtest/pvpanic-test.c
> +++ b/tests/qtest/pvpanic-test.c
> @@ -24,9 +24,7 @@ static void test_panic(void)
>  
>      qtest_outb(qts, 0x505, 0x1);
>  
> -    response = qtest_qmp_receive_dict(qts);
> -    g_assert(qdict_haskey(response, "event"));
> -    g_assert_cmpstr(qdict_get_str(response, "event"), ==, "GUEST_PANICKED");
> +    response = qtest_qmp_eventwait_ref(qts, "GUEST_PANICKED");
>      g_assert(qdict_haskey(response, "data"));
>      data = qdict_get_qdict(response, "data");
>      g_assert(qdict_haskey(data, "action"));
> diff --git a/tests/qtest/tpm-util.c b/tests/qtest/tpm-util.c
> index 3ed6c8548a..5a33a6ef0f 100644
> --- a/tests/qtest/tpm-util.c
> +++ b/tests/qtest/tpm-util.c
> @@ -237,12 +237,16 @@ void tpm_util_migrate(QTestState *who, const char *uri)
>  void tpm_util_wait_for_migration_complete(QTestState *who)
>  {
>      while (true) {
> +        QDict *rsp;
>          QDict *rsp_return;
>          bool completed;
>          const char *status;
>  
> -        qtest_qmp_send(who, "{ 'execute': 'query-migrate' }");
> -        rsp_return = qtest_qmp_receive_success(who, NULL, NULL);
> +        rsp = qtest_qmp(who, "{ 'execute': 'query-migrate' }");
> +        g_assert(qdict_haskey(rsp, "return"));
> +        rsp_return = qdict_get_qdict(rsp, "return");
> +
> +        g_assert(!qdict_haskey(rsp_return, "error"));
>          status = qdict_get_str(rsp_return, "status");
>          completed = strcmp(status, "completed") == 0;
>          g_assert_cmpstr(status, !=,  "failed");
> 

Re: [PATCH v7 00/13] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread

[no-reply]

Patchew URL: https://patchew.org/QEMU/20201006123904.610658-1-mlevitsk@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

N/A. Internal error while reading log file



The full log is available at
http://patchew.org/logs/20201006123904.610658-1-mlevitsk@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v7 03/13] qtest: switch users back to qtest_qmp_receive

[Maxim Levitsky]

On Tue, 2020-10-06 at 14:56 +0200, Paolo Bonzini wrote:
> On 06/10/20 14:38, Maxim Levitsky wrote:
> > The only remaining users of qtest_qmp_receive_dict are tests
> > that fuzz the QMP protocol.
> > 
> > Tested with 'make check-qtest'.
> 
> Probably the qtest_qmp_receive_success conversion should have been in a
> separate patch.  But it's a nice side effect that I hadn't anticipated!

I was afraid that a partial conversion will break things, since I had changed
the qtest_vqmp_fds, qtest_vqmp and qtest_qmp, so there are quite a lot of 
places where this is called implicitly. Now looking again at this,
it is probably possible to separate this chinage. If I need to send another
version I'll do this.

Best regards,
	Maxim Levitsky

> 
> Paolo
> 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  tests/qtest/ahci-test.c         |  4 +-
> >  tests/qtest/device-plug-test.c  |  2 +-
> >  tests/qtest/drive_del-test.c    |  9 ++---
> >  tests/qtest/libqos/libqtest.h   | 17 ---------
> >  tests/qtest/libqtest.c          | 65 ++++-----------------------------
> >  tests/qtest/migration-helpers.c | 25 ++++++++++---
> >  tests/qtest/pvpanic-test.c      |  4 +-
> >  tests/qtest/tpm-util.c          |  8 +++-
> >  8 files changed, 41 insertions(+), 93 deletions(-)
> > 
> > diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> > index d42ebaeb4c..5e1954852e 100644
> > --- a/tests/qtest/ahci-test.c
> > +++ b/tests/qtest/ahci-test.c
> > @@ -1590,7 +1590,7 @@ static void test_atapi_tray(void)
> >      qtest_qmp_send(ahci->parent->qts, "{'execute': 'blockdev-open-tray', "
> >                      "'arguments': {'id': 'cd0'}}");
> >      atapi_wait_tray(ahci, true);
> > -    rsp = qtest_qmp_receive_dict(ahci->parent->qts);
> > +    rsp = qtest_qmp_receive(ahci->parent->qts);
> >      qobject_unref(rsp);
> >  
> >      qmp_discard_response(ahci->parent->qts,
> > @@ -1620,7 +1620,7 @@ static void test_atapi_tray(void)
> >      qtest_qmp_send(ahci->parent->qts, "{'execute': 'blockdev-close-tray', "
> >                     "'arguments': {'id': 'cd0'}}");
> >      atapi_wait_tray(ahci, false);
> > -    rsp = qtest_qmp_receive_dict(ahci->parent->qts);
> > +    rsp = qtest_qmp_receive(ahci->parent->qts);
> >      qobject_unref(rsp);
> >  
> >      /* Now, to convince ATAPI we understand the media has changed... */
> > diff --git a/tests/qtest/device-plug-test.c b/tests/qtest/device-plug-test.c
> > index a2247856be..9214892741 100644
> > --- a/tests/qtest/device-plug-test.c
> > +++ b/tests/qtest/device-plug-test.c
> > @@ -23,7 +23,7 @@ static void device_del_start(QTestState *qtest, const char *id)
> >  
> >  static void device_del_finish(QTestState *qtest)
> >  {
> > -    QDict *resp = qtest_qmp_receive_dict(qtest);
> > +    QDict *resp = qtest_qmp_receive(qtest);
> >  
> >      g_assert(qdict_haskey(resp, "return"));
> >      qobject_unref(resp);
> > diff --git a/tests/qtest/drive_del-test.c b/tests/qtest/drive_del-test.c
> > index ba0cd77445..9d20a1ed8b 100644
> > --- a/tests/qtest/drive_del-test.c
> > +++ b/tests/qtest/drive_del-test.c
> > @@ -15,9 +15,6 @@
> >  #include "libqos/virtio.h"
> >  #include "qapi/qmp/qdict.h"
> >  
> > -/* TODO actually test the results and get rid of this */
> > -#define qmp_discard_response(q, ...) qobject_unref(qtest_qmp(q, __VA_ARGS__))
> > -
> >  static void drive_add(QTestState *qts)
> >  {
> >      char *resp = qtest_hmp(qts, "drive_add 0 if=none,id=drive0");
> > @@ -38,13 +35,13 @@ static void device_del(QTestState *qts)
> >  {
> >      QDict *response;
> >  
> > -    /* Complication: ignore DEVICE_DELETED event */
> > -    qmp_discard_response(qts, "{'execute': 'device_del',"
> > +    response = qtest_qmp(qts, "{'execute': 'device_del',"
> >                           " 'arguments': { 'id': 'dev0' } }");
> > -    response = qtest_qmp_receive_dict(qts);
> >      g_assert(response);
> >      g_assert(qdict_haskey(response, "return"));
> >      qobject_unref(response);
> > +
> > +    qtest_qmp_eventwait(qts, "DEVICE_DELETED");
> >  }
> >  
> >  static void test_drive_without_dev(void)
> > diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h
> > index 19429a536d..a91e9e02e9 100644
> > --- a/tests/qtest/libqos/libqtest.h
> > +++ b/tests/qtest/libqos/libqtest.h
> > @@ -240,23 +240,6 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
> >   */
> >  QDict *qtest_qmp_event_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/qtest/libqtest.c b/tests/qtest/libqtest.c
> > index d4c49a52ff..08929f5ff6 100644
> > --- a/tests/qtest/libqtest.c
> > +++ b/tests/qtest/libqtest.c
> > @@ -324,7 +324,7 @@ QTestState *qtest_init(const char *extra_args)
> >      QDict *greeting;
> >  
> >      /* Read the QMP greeting and then do the handshake */
> > -    greeting = qtest_qmp_receive_dict(s);
> > +    greeting = qtest_qmp_receive(s);
> >      qobject_unref(greeting);
> >      qobject_unref(qtest_qmp(s, "{ 'execute': 'qmp_capabilities' }"));
> >  
> > @@ -700,7 +700,7 @@ QDict *qtest_vqmp_fds(QTestState *s, int *fds, size_t fds_num,
> >      qtest_qmp_vsend_fds(s, fds, fds_num, fmt, ap);
> >  
> >      /* Receive reply */
> > -    return qtest_qmp_receive_dict(s);
> > +    return qtest_qmp_receive(s);
> >  }
> >  
> >  QDict *qtest_vqmp(QTestState *s, const char *fmt, va_list ap)
> > @@ -708,7 +708,7 @@ QDict *qtest_vqmp(QTestState *s, const char *fmt, va_list ap)
> >      qtest_qmp_vsend(s, fmt, ap);
> >  
> >      /* Receive reply */
> > -    return qtest_qmp_receive_dict(s);
> > +    return qtest_qmp_receive(s);
> >  }
> >  
> >  QDict *qmp_fd(int fd, const char *fmt, ...)
> > @@ -850,12 +850,6 @@ char *qtest_vhmp(QTestState *s, const char *fmt, va_list ap)
> >                       " 'arguments': {'command-line': %s}}",
> >                       cmd);
> >      ret = g_strdup(qdict_get_try_str(resp, "return"));
> > -    while (ret == NULL && qdict_get_try_str(resp, "event")) {
> > -        /* Ignore asynchronous QMP events */
> > -        qobject_unref(resp);
> > -        resp = qtest_qmp_receive_dict(s);
> > -        ret = g_strdup(qdict_get_try_str(resp, "return"));
> > -    }
> >      g_assert(ret);
> >      qobject_unref(resp);
> >      g_free(cmd);
> > @@ -1291,35 +1285,6 @@ 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_dict(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 commands.
> >   */
> > @@ -1355,13 +1320,6 @@ void qtest_qmp_device_add(QTestState *qts, const char *driver, const char *id,
> >      qobject_unref(args);
> >  }
> >  
> > -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.
> > @@ -1378,24 +1336,17 @@ static void device_deleted_cb(void *opaque, const char *name, QDict *data)
> >   * and this one:
> >   *
> >   * {"return": {}}
> > - *
> > - * But the order of arrival may vary - so we've got to detect both.
> >   */
> >  void qtest_qmp_device_del(QTestState *qts, const char *id)
> >  {
> > -    bool got_event = false;
> >      QDict *rsp;
> >  
> > -    qtest_qmp_send(qts, "{'execute': 'device_del', 'arguments': {'id': %s}}",
> > -                   id);
> > -    rsp = qtest_qmp_receive_success(qts, device_deleted_cb, &got_event);
> > +    rsp = qtest_qmp(qts, "{'execute': 'device_del', 'arguments': {'id': %s}}",
> > +                    id);
> > +
> > +    g_assert(qdict_haskey(rsp, "return"));
> >      qobject_unref(rsp);
> > -    if (!got_event) {
> > -        rsp = qtest_qmp_receive_dict(qts);
> > -        g_assert_cmpstr(qdict_get_try_str(rsp, "event"),
> > -                        ==, "DEVICE_DELETED");
> > -        qobject_unref(rsp);
> > -    }
> > +    qtest_qmp_eventwait(qts, "DEVICE_DELETED");
> >  }
> >  
> >  bool qmp_rsp_is_err(QDict *rsp)
> > diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> > index 516093b39a..b799dbafb7 100644
> > --- a/tests/qtest/migration-helpers.c
> > +++ b/tests/qtest/migration-helpers.c
> > @@ -17,10 +17,12 @@
> >  
> >  bool got_stop;
> >  
> > -static void stop_cb(void *opaque, const char *name, QDict *data)
> > +static void check_stop_event(QTestState *who)
> >  {
> > -    if (!strcmp(name, "STOP")) {
> > +    QDict *event = qtest_qmp_event_ref(who, "STOP");
> > +    if (event) {
> >          got_stop = true;
> > +        qobject_unref(event);
> >      }
> >  }
> >  
> > @@ -30,12 +32,19 @@ static void stop_cb(void *opaque, const char *name, QDict *data)
> >  QDict *wait_command_fd(QTestState *who, int fd, const char *command, ...)
> >  {
> >      va_list ap;
> > +    QDict *resp;
> >  
> >      va_start(ap, command);
> >      qtest_qmp_vsend_fds(who, &fd, 1, command, ap);
> >      va_end(ap);
> >  
> > -    return qtest_qmp_receive_success(who, stop_cb, NULL);
> > +    resp = qtest_qmp_receive(who);
> > +    check_stop_event(who);
> > +
> > +    g_assert(!qdict_haskey(resp, "error"));
> > +    g_assert(qdict_haskey(resp, "return"));
> > +
> > +    return qdict_get_qdict(resp, "return");
> >  }
> >  
> >  /*
> > @@ -44,12 +53,18 @@ QDict *wait_command_fd(QTestState *who, int fd, const char *command, ...)
> >  QDict *wait_command(QTestState *who, const char *command, ...)
> >  {
> >      va_list ap;
> > +    QDict *resp;
> >  
> >      va_start(ap, command);
> > -    qtest_qmp_vsend(who, command, ap);
> > +    resp = qtest_vqmp(who, command, ap);
> >      va_end(ap);
> >  
> > -    return qtest_qmp_receive_success(who, stop_cb, NULL);
> > +    check_stop_event(who);
> > +
> > +    g_assert(!qdict_haskey(resp, "error"));
> > +    g_assert(qdict_haskey(resp, "return"));
> > +
> > +    return qdict_get_qdict(resp, "return");
> >  }
> >  
> >  /*
> > diff --git a/tests/qtest/pvpanic-test.c b/tests/qtest/pvpanic-test.c
> > index b0b20ad340..0657de797f 100644
> > --- a/tests/qtest/pvpanic-test.c
> > +++ b/tests/qtest/pvpanic-test.c
> > @@ -24,9 +24,7 @@ static void test_panic(void)
> >  
> >      qtest_outb(qts, 0x505, 0x1);
> >  
> > -    response = qtest_qmp_receive_dict(qts);
> > -    g_assert(qdict_haskey(response, "event"));
> > -    g_assert_cmpstr(qdict_get_str(response, "event"), ==, "GUEST_PANICKED");
> > +    response = qtest_qmp_eventwait_ref(qts, "GUEST_PANICKED");
> >      g_assert(qdict_haskey(response, "data"));
> >      data = qdict_get_qdict(response, "data");
> >      g_assert(qdict_haskey(data, "action"));
> > diff --git a/tests/qtest/tpm-util.c b/tests/qtest/tpm-util.c
> > index 3ed6c8548a..5a33a6ef0f 100644
> > --- a/tests/qtest/tpm-util.c
> > +++ b/tests/qtest/tpm-util.c
> > @@ -237,12 +237,16 @@ void tpm_util_migrate(QTestState *who, const char *uri)
> >  void tpm_util_wait_for_migration_complete(QTestState *who)
> >  {
> >      while (true) {
> > +        QDict *rsp;
> >          QDict *rsp_return;
> >          bool completed;
> >          const char *status;
> >  
> > -        qtest_qmp_send(who, "{ 'execute': 'query-migrate' }");
> > -        rsp_return = qtest_qmp_receive_success(who, NULL, NULL);
> > +        rsp = qtest_qmp(who, "{ 'execute': 'query-migrate' }");
> > +        g_assert(qdict_haskey(rsp, "return"));
> > +        rsp_return = qdict_get_qdict(rsp, "return");
> > +
> > +        g_assert(!qdict_haskey(rsp_return, "error"));
> >          status = qdict_get_str(rsp_return, "status");
> >          completed = strcmp(status, "completed") == 0;
> >          g_assert_cmpstr(status, !=,  "failed");
> > 

Re: [PATCH v7 01/13] qtest: rename qtest_qmp_receive to qtest_qmp_receive_dict

[Thomas Huth]

On 06/10/2020 14.38, Maxim Levitsky wrote:
> In the next patch a new version of qtest_qmp_receive will be
> reintroduced that will buffer received qmp events for later
> consumption in qtest_qmp_eventwait_ref
> 
> No functional change intended.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  tests/qtest/ahci-test.c        |  4 ++--
>  tests/qtest/device-plug-test.c |  2 +-
>  tests/qtest/drive_del-test.c   |  2 +-
>  tests/qtest/libqos/libqtest.h  |  4 ++--
>  tests/qtest/libqtest.c         | 16 ++++++++--------
>  tests/qtest/pvpanic-test.c     |  2 +-
>  tests/qtest/qmp-test.c         | 18 +++++++++---------
>  7 files changed, 24 insertions(+), 24 deletions(-)


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

Re: [PATCH v7 02/13] qtest: Reintroduce qtest_qmp_receive

[Thomas Huth]

On 06/10/2020 14.38, Maxim Levitsky wrote:
> The new qtest_qmp_receive buffers all the received qmp events, allowing
> qtest_qmp_eventwait_ref to return them.
> 
> This is intended to solve the race in regard to ordering of qmp events
> vs qmp responses, as soon as the callers start using the new interface.
> 
> In addition to that, define qtest_qmp_event_ref a function which only scans
> the buffer that qtest_qmp_receive stores the events to.
> 
> This is intended for callers that are only interested in events that were
> received during the last call to the qtest_qmp_receive.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  tests/qtest/libqos/libqtest.h | 23 ++++++++++++++++
>  tests/qtest/libqtest.c        | 49 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h
> index a41135fc92..19429a536d 100644
> --- a/tests/qtest/libqos/libqtest.h
> +++ b/tests/qtest/libqos/libqtest.h
> @@ -198,6 +198,16 @@ void qtest_qmp_vsend(QTestState *s, const char *fmt, va_list ap)
>   */
>  QDict *qtest_qmp_receive_dict(QTestState *s);
>  
> +/**
> + * qtest_qmp_receive:
> + * @s: #QTestState instance to operate on.
> + *
> + * Reads a QMP message from QEMU and returns the response.
> + * Buffers all the events received meanwhile, until a
> + * call to qtest_qmp_eventwait
> + */
> +QDict *qtest_qmp_receive(QTestState *s);

Re-introducing qtest_qmp_receive() with different behavior than before will
likely make backports of other later patches a pain, and might also break
other patches that use this function but are not merged yet. Could you
please use a different name for this function instead? Maye
qtest_qmp_receive_buffered() or something like that?

 Thomas

Re: [PATCH v7 02/13] qtest: Reintroduce qtest_qmp_receive

[Paolo Bonzini]

On 12/10/20 13:14, Thomas Huth wrote:
>> +/**
>> + * qtest_qmp_receive:
>> + * @s: #QTestState instance to operate on.
>> + *
>> + * Reads a QMP message from QEMU and returns the response.
>> + * Buffers all the events received meanwhile, until a
>> + * call to qtest_qmp_eventwait
>> + */
>> +QDict *qtest_qmp_receive(QTestState *s);
> Re-introducing qtest_qmp_receive() with different behavior than before will
> likely make backports of other later patches a pain, and might also break
> other patches that use this function but are not merged yet. Could you
> please use a different name for this function instead? Maye
> qtest_qmp_receive_buffered() or something like that?

We chose to use the same name because the new version generally is the
one you want and, except for the handling of events, is exactly the same
as before.  In other words, I'm treating the new semantics more as a
bugfix than a feature.

The only trap that backports of later patches could fall into is if they
want to look at events, but it would be caught easily because the test
would fail.

Paolo

Re: [PATCH v7 02/13] qtest: Reintroduce qtest_qmp_receive

[Thomas Huth]

On 12/10/2020 15.47, Paolo Bonzini wrote:
> On 12/10/20 13:14, Thomas Huth wrote:
>>> +/**
>>> + * qtest_qmp_receive:
>>> + * @s: #QTestState instance to operate on.
>>> + *
>>> + * Reads a QMP message from QEMU and returns the response.
>>> + * Buffers all the events received meanwhile, until a
>>> + * call to qtest_qmp_eventwait
>>> + */
>>> +QDict *qtest_qmp_receive(QTestState *s);
>> Re-introducing qtest_qmp_receive() with different behavior than before will
>> likely make backports of other later patches a pain, and might also break
>> other patches that use this function but are not merged yet. Could you
>> please use a different name for this function instead? Maye
>> qtest_qmp_receive_buffered() or something like that?
> 
> We chose to use the same name because the new version generally is the
> one you want and, except for the handling of events, is exactly the same
> as before.  In other words, I'm treating the new semantics more as a
> bugfix than a feature.
> 
> The only trap that backports of later patches could fall into is if they
> want to look at events, but it would be caught easily because the test
> would fail.

Ok, thanks for the explanation! ... but I think it might be good to have
this information in the patch description, though.

 Thomas

Re: [PATCH v7 02/13] qtest: Reintroduce qtest_qmp_receive

[Paolo Bonzini]

On 12/10/20 15:49, Thomas Huth wrote:
>> We chose to use the same name because the new version generally is the
>> one you want and, except for the handling of events, is exactly the same
>> as before.  In other words, I'm treating the new semantics more as a
>> bugfix than a feature.
>>
>> The only trap that backports of later patches could fall into is if they
>> want to look at events, but it would be caught easily because the test
>> would fail.
> Ok, thanks for the explanation! ... but I think it might be good to have
> this information in the patch description, though.

Ok, I'll add a couple words.

Paolo