tests: Introduce generic device hot-plug/hot-unplug functions

A lot of tests provide code for adding and removing a device via the device_add and device_del QMP commands. Maintaining this code in so many places is cumbersome and error-prone (some of the code parts check the responses in an incorrect way, for example), so let's provide some proper generic qtest functions for adding and removing a device instead.

Signed-off-by Thomas Huth <[email protected]>

---
I'm planning to use qtest_hot_plug_device() in a bigger patch series that I'm currently preparing. But since this clean-up patch is valid on its own already, I'd like to get some feedback (or even a Reviewed-by) for this patch here already if possible...

 tests/libqos/pci.c         | 19 ++-------------
 tests/libqos/usb.c         | 30 +++++------------------
 tests/libqtest.c           | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/libqtest.h           | 19 +++++++++++++++
 tests/usb-hcd-uhci-test.c  | 26 ++------------------
 tests/usb-hcd-xhci-test.c  | 52 ++++------------------------------------
 tests/virtio-scsi-test.c   | 24 ++-----------------
 tests/virtio-serial-test.c | 25 +++----------------
 8 files changed, 98 insertions(+), 157 deletions(-)

tests/libqos/pci.c +2 -17

index 2dcdead..aada753 100644
@@ -394,21 +394,6 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
 void qpci_plug_device_test(const char *driver, const char *id,
                            uint8_t slot, const char *opts)
 {
-    QDict *response;
-    char *cmd;
-
-    cmd = g_strdup_printf("{'execute': 'device_add',"
-                          " 'arguments': {"
-                          "   'driver': '%s',"
-                          "   'addr': '%d',"
-                          "   %s%s"
-                          "   'id': '%s'"
-                          "}}", driver, slot,
-                          opts ? opts : "", opts ? "," : "",
-                          id);
-    response = qmp(cmd);
-    g_free(cmd);
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
+    qtest_hot_plug_device(driver, id, "'addr': '%d'%s%s", slot,
+                          opts ? ", " : "", opts ? opts : "");
 }

tests/libqos/usb.c +6 -24

index 0cdfaec..f8d0190 100644
@@ -40,34 +40,16 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
 void usb_test_hotplug(const char *hcd_id, const int port,
                       void (*port_check)(void))
 {
-    QDict *response;
-    char  *cmd;
+    char  *id = g_strdup_printf("usbdev%d", port);
 
-    cmd = g_strdup_printf("{'execute': 'device_add',"
-                          " 'arguments': {"
-                          "   'driver': 'usb-tablet',"
-                          "   'port': '%d',"
-                          "   'bus': '%s.0',"
-                          "   'id': 'usbdev%d'"
-                          "}}", port, hcd_id, port);
-    response = qmp(cmd);
-    g_free(cmd);
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
+    qtest_hot_plug_device("usb-tablet", id, "'port': '%d', 'bus': '%s.0'",
+                          port, hcd_id);
 
     if (port_check) {
         port_check();
     }
 
-    cmd = g_strdup_printf("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'usbdev%d'"
-                           "}}", port);
-    response = qmp(cmd);
-    g_free(cmd);
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qtest_hot_unplug_device(id);
+
+    g_free(id);
 }

tests/libqtest.c +22 -0

index b9a1f18..4339d97 100644
@@ -987,3 +987,63 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine))
     qtest_end();
     QDECREF(response);
 }
+
+/**
+ * Generic hot-plugging test via the device_add QMP command
+ */
+void qtest_hot_plug_device(const char *driver, const char *id,
+                           const char *fmt, ...)
+{
+    QDict *response;
+    char *cmd, *opts = NULL;
+    va_list va;
+
+    if (fmt) {
+        va_start(va, fmt);
+        opts = g_strdup_vprintf(fmt, va);
+        va_end(va);
+    }
+
+    cmd = g_strdup_printf("{'execute': 'device_add',"
+                          " 'arguments': { 'driver': '%s', 'id': '%s'%s%s }}",
+                          driver, id, opts ? ", " : "", opts ? opts : "");
+    g_free(opts);
+

tests/libqtest.c +10 -0

+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+}
+
+/**
+ * Generic hot-unplugging test via the device_del QMP command
+ */
+void qtest_hot_unplug_device(const char *id)
+{
+    QDict *response;

tests/libqtest.c +8 -0

+        g_assert_cmpstr(qdict_get_str(response, "event"), ==, "DEVICE_DELETED");
+        QDECREF(response);
+        response = qmp("");
+        g_assert(response);
+    }
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+}

tests/libqtest.h +19 -0

index 3ae5709..9c1006f 100644
@@ -927,4 +927,23 @@ QDict *qmp_fd(int fd, const char *fmt, ...);
  */
 void qtest_cb_for_every_machine(void (*cb)(const char *machine));
 
+/**
+ * qtest_hot_plug_device:
+ * @driver: Name of the device that should be added
+ * @id: Identification string
+ * @fmt: printf-like format string for further options to device_add
+ *
+ * Generic hot-plugging test via the device_add QMP command.
+ */
+void qtest_hot_plug_device(const char *driver, const char *id,
+                           const char *fmt, ...) GCC_FMT_ATTR(3, 4);
+
+/**
+ * qtest_hot_unplug_device:
+ * @id: Identification string
+ *
+ * Generic hot-unplugging test via the device_del QMP command.
+ */
+void qtest_hot_unplug_device(const char *id);
+
 #endif

tests/usb-hcd-uhci-test.c +2 -24

index 5b500fe..7465bfd 100644
@@ -48,31 +48,9 @@ static void test_uhci_hotplug(void)
 
 static void test_usb_storage_hotplug(void)
 {
-    QDict *response;
+    qtest_hot_plug_device("usb-storage", "usbdev0", "'drive': 'drive0'");
 
-    response = qmp("{'execute': 'device_add',"
-                   " 'arguments': {"
-                   "   'driver': 'usb-storage',"
-                   "   'drive': 'drive0',"
-                   "   'id': 'usbdev0'"
-                   "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'usbdev0'"
-                           "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("");
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qtest_hot_unplug_device("usbdev0");
 }
 
 int main(int argc, char **argv)

tests/usb-hcd-xhci-test.c +0 -0

index 031764d..ae89362 100644

tests/usb-hcd-xhci-test.c +4 -47

 static void test_xhci_init(void)
 {
 }

@@ -23,59 +22,16 @@ static void test_xhci_hotplug(void)
 
 static void test_usb_uas_hotplug(void)
 {
-    QDict *response;
-
-    response = qmp("{'execute': 'device_add',"
-                   " 'arguments': {"
-                   "   'driver': 'usb-uas',"
-                   "   'id': 'uas'"
-                   "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("{'execute': 'device_add',"
-                   " 'arguments': {"
-                   "   'driver': 'scsi-hd',"
-                   "   'drive': 'drive0',"
-                   "   'id': 'scsi-hd'"
-                   "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
+    qtest_hot_plug_device("usb-uas", "uas", NULL);
+    qtest_hot_plug_device("scsi-hd", "scsihd", "'drive': 'drive0'");
 
     /* TODO:
         UAS HBA driver in libqos, to check that
         added disk is visible after BUS rescan
     */
 
-    response = qmp("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'scsi-hd'"
-                           "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("");
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
-
-
-    response = qmp("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'uas'"
-                           "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("");
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qtest_hot_unplug_device("scsihd");
+    qtest_hot_unplug_device("uas");
 }
 
 int main(int argc, char **argv)

tests/virtio-scsi-test.c +2 -22

index 87a3b6e..6e7ba6f 100644
@@ -192,32 +192,12 @@ static void pci_nop(void)
 
 static void hotplug(void)
 {
-    QDict *response;
     QOSState *qs;
 
     qs = qvirtio_scsi_start(
             "-drive id=drv1,if=none,file=null-co://,format=raw");
-    response = qmp("{\"execute\": \"device_add\","
-                   " \"arguments\": {"
-                   "   \"driver\": \"scsi-hd\","
-                   "   \"id\": \"scsi-hd\","
-                   "   \"drive\": \"drv1\""
-                   "}}");
-
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("{\"execute\": \"device_del\","
-                   " \"arguments\": {"
-                   "   \"id\": \"scsi-hd\""
-                   "}}");
-
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qtest_hot_plug_device("scsi-hd", "scsihd", "'drive': 'drv1'");
+    qtest_hot_unplug_device("scsihd");
     qvirtio_scsi_stop(qs);
 }
 

tests/virtio-serial-test.c +3 -22

index b14d943..97f8f52 100644
@@ -17,28 +17,9 @@ static void pci_nop(void)
 
 static void hotplug(void)
 {
-    QDict *response;
-
-    response = qmp("{\"execute\": \"device_add\","
-                   " \"arguments\": {"
-                   "   \"driver\": \"virtserialport\","
-                   "   \"id\": \"hp-port\""
-                   "}}");
-
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("{\"execute\": \"device_del\","
-                   " \"arguments\": {"
-                   "   \"id\": \"hp-port\""
-                   "}}");
-
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qtest_hot_plug_device("virtserialport", "hp-port", NULL);
+
+    qtest_hot_unplug_device("hp-port");
 }
 
 int main(int argc, char **argv)

Signature · 1 line

1.8.3.1

Quoted 131 lines · Thomas Huth

A lot of tests provide code for adding and removing a device via the device_add and device_del QMP commands. Maintaining this code in so many places is cumbersome and error-prone (some of the code parts check the responses in an incorrect way, for example), so let's provide some proper generic qtest functions for adding and removing a device instead.

Signed-off-by Thomas Huth <[email protected]>

---
I'm planning to use qtest_hot_plug_device() in a bigger patch series that I'm currently preparing. But since this clean-up patch is valid on its own already, I'd like to get some feedback (or even a Reviewed-by) for this patch here already if possible...

 tests/libqos/pci.c         | 19 ++-------------
 tests/libqos/usb.c         | 30 +++++------------------
 tests/libqtest.c           | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/libqtest.h           | 19 +++++++++++++++
 tests/usb-hcd-uhci-test.c  | 26 ++------------------
 tests/usb-hcd-xhci-test.c  | 52 ++++------------------------------------
 tests/virtio-scsi-test.c   | 24 ++-----------------
 tests/virtio-serial-test.c | 25 +++----------------
 8 files changed, 98 insertions(+), 157 deletions(-)

tests/libqos/pci.c +2 -17

index 2dcdead..aada753 100644
@@ -394,21 +394,6 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
 void qpci_plug_device_test(const char *driver, const char *id,
                            uint8_t slot, const char *opts)
 {
-    QDict *response;
-    char *cmd;
-
-    cmd = g_strdup_printf("{'execute': 'device_add',"
-                          " 'arguments': {"
-                          "   'driver': '%s',"
-                          "   'addr': '%d',"
-                          "   %s%s"
-                          "   'id': '%s'"
-                          "}}", driver, slot,
-                          opts ? opts : "", opts ? "," : "",
-                          id);
-    response = qmp(cmd);
-    g_free(cmd);
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
+    qtest_hot_plug_device(driver, id, "'addr': '%d'%s%s", slot,
+                          opts ? ", " : "", opts ? opts : "");
 }

tests/libqos/usb.c +6 -24

index 0cdfaec..f8d0190 100644
@@ -40,34 +40,16 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
 void usb_test_hotplug(const char *hcd_id, const int port,
                       void (*port_check)(void))
 {
-    QDict *response;
-    char  *cmd;
+    char  *id = g_strdup_printf("usbdev%d", port);
 
-    cmd = g_strdup_printf("{'execute': 'device_add',"
-                          " 'arguments': {"
-                          "   'driver': 'usb-tablet',"
-                          "   'port': '%d',"
-                          "   'bus': '%s.0',"
-                          "   'id': 'usbdev%d'"
-                          "}}", port, hcd_id, port);
-    response = qmp(cmd);
-    g_free(cmd);
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
+    qtest_hot_plug_device("usb-tablet", id, "'port': '%d', 'bus': '%s.0'",
+                          port, hcd_id);
 
     if (port_check) {
         port_check();
     }
 
-    cmd = g_strdup_printf("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'usbdev%d'"
-                           "}}", port);
-    response = qmp(cmd);
-    g_free(cmd);
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qtest_hot_unplug_device(id);
+
+    g_free(id);
 }

tests/libqtest.c +26 -0

index b9a1f18..4339d97 100644
@@ -987,3 +987,63 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine))
     qtest_end();
     QDECREF(response);
 }
+
+/**
+ * Generic hot-plugging test via the device_add QMP command
+ */
+void qtest_hot_plug_device(const char *driver, const char *id,
+                           const char *fmt, ...)
+{
+    QDict *response;
+    char *cmd, *opts = NULL;
+    va_list va;
+
+    if (fmt) {
+        va_start(va, fmt);
+        opts = g_strdup_vprintf(fmt, va);
+        va_end(va);
+    }
+
+    cmd = g_strdup_printf("{'execute': 'device_add',"
+                          " 'arguments': { 'driver': '%s', 'id': '%s'%s%s }}",
+                          driver, id, opts ? ", " : "", opts ? opts : "");
+    g_free(opts);
+
+    response = qmp(cmd);
+    g_free(cmd);
+    g_assert(response);
+    while (qdict_haskey(response, "event")) {

tests/libqtest.c void qtest_cb_for_every_machine(void (*cb)(const char *machine)) · line 1021

Quoted 6 lines

tests/libqtest.c +6 -0

+        /* We can get DEVICE_DELETED events in case something went wrong */
+        g_assert_cmpstr(qdict_get_str(response, "event"), !=, "DEVICE_DELETED");
+        QDECREF(response);
+        response = qmp("");
+        g_assert(response);
+    }

I guess we don't need this block for hot plug? :-)

Quoted 10 lines

tests/libqtest.c +10 -0

+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+}
+
+/**
+ * Generic hot-unplugging test via the device_del QMP command
+ */
+void qtest_hot_unplug_device(const char *id)
+{
+    QDict *response;

tests/libqtest.c void qtest_cb_for_every_machine(void (*cb)(const char *machine)) · line 1041

Quoted 10 lines

tests/libqtest.c +10 -0

+    char *cmd;
+
+    cmd = g_strdup_printf("{'execute': 'device_del',"
+                          " 'arguments': { 'id': '%s' }}", id);
+
+    response = qmp(cmd);
+    g_free(cmd);
+    g_assert(response);
+    while (qdict_haskey(response, "event")) {
+        /* We should get DEVICE_DELETED event first */

So here is the mis-order thing: could the "return: {}" arrive earlier? IHMO it can. If so, we'll leave the event in the buffer without reading it.

If you see my patch, I do two reads (assuming we will only get exactly one "return: {}" and a DELETED event), and take care of the ordering. Would that be better?

Quoted 82 lines

tests/libqtest.c +8 -0

+        g_assert_cmpstr(qdict_get_str(response, "event"), ==, "DEVICE_DELETED");
+        QDECREF(response);
+        response = qmp("");
+        g_assert(response);
+    }
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+}

tests/libqtest.h +19 -0

index 3ae5709..9c1006f 100644
@@ -927,4 +927,23 @@ QDict *qmp_fd(int fd, const char *fmt, ...);
  */
 void qtest_cb_for_every_machine(void (*cb)(const char *machine));
 
+/**
+ * qtest_hot_plug_device:
+ * @driver: Name of the device that should be added
+ * @id: Identification string
+ * @fmt: printf-like format string for further options to device_add
+ *
+ * Generic hot-plugging test via the device_add QMP command.
+ */
+void qtest_hot_plug_device(const char *driver, const char *id,
+                           const char *fmt, ...) GCC_FMT_ATTR(3, 4);
+
+/**
+ * qtest_hot_unplug_device:
+ * @id: Identification string
+ *
+ * Generic hot-unplugging test via the device_del QMP command.
+ */
+void qtest_hot_unplug_device(const char *id);
+
 #endif

tests/usb-hcd-uhci-test.c +2 -24

index 5b500fe..7465bfd 100644
@@ -48,31 +48,9 @@ static void test_uhci_hotplug(void)
 
 static void test_usb_storage_hotplug(void)
 {
-    QDict *response;
+    qtest_hot_plug_device("usb-storage", "usbdev0", "'drive': 'drive0'");
 
-    response = qmp("{'execute': 'device_add',"
-                   " 'arguments': {"
-                   "   'driver': 'usb-storage',"
-                   "   'drive': 'drive0',"
-                   "   'id': 'usbdev0'"
-                   "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("{'execute': 'device_del',"
-                           " 'arguments': {"
-                           "   'id': 'usbdev0'"
-                           "}}");
-    g_assert(response);
-    g_assert(!qdict_haskey(response, "error"));
-    QDECREF(response);
-
-    response = qmp("");
-    g_assert(response);
-    g_assert(qdict_haskey(response, "event"));
-    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
-    QDECREF(response);
+    qtest_hot_unplug_device("usbdev0");
 }
 
 int main(int argc, char **argv)

tests/usb-hcd-xhci-test.c +0 -0

index 031764d..ae89362 100644
@@ -11,7 +11,6 @@
 #include "libqtest.h"
 #include "libqos/usb.h"
 

tests/usb-hcd-xhci-test.c line 14

Quoted 1 line

tests/usb-hcd-xhci-test.c +0 -1

-

Nit: useless change

Other than these, it looks quite good to me. Thanks,

Quoted 26 lines

Quoted 26 lines

tests/libqtest.c +26 -0

+
+/**
+ * Generic hot-plugging test via the device_add QMP command
+ */
+void qtest_hot_plug_device(const char *driver, const char *id,
+                           const char *fmt, ...)
+{
+    QDict *response;
+    char *cmd, *opts = NULL;
+    va_list va;
+
+    if (fmt) {
+        va_start(va, fmt);
+        opts = g_strdup_vprintf(fmt, va);
+        va_end(va);
+    }
+
+    cmd = g_strdup_printf("{'execute': 'device_add',"
+                          " 'arguments': { 'driver': '%s', 'id': '%s'%s%s }}",
+                          driver, id, opts ? ", " : "", opts ? opts : "");
+    g_free(opts);
+
+    response = qmp(cmd);
+    g_free(cmd);
+    g_assert(response);
+    while (qdict_haskey(response, "event")) {

Quoted 8 lines

tests/libqtest.c void qtest_cb_for_every_machine(void (*cb)(const char *machine)) · line 1021

Quoted 6 lines

tests/libqtest.c +6 -0

+        /* We can get DEVICE_DELETED events in case something went wrong */
+        g_assert_cmpstr(qdict_get_str(response, "event"), !=, "DEVICE_DELETED");
+        QDECREF(response);
+        response = qmp("");
+        g_assert(response);
+    }

I guess we don't need this block for hot plug? :-)

I'm pretty sure I've seen this happening somewhere ... but maybe it was just a fall-out of the mis-behaving usb test...
I'll replace it with a g_assert(!qdict_haskey(response, "event")) instead, since we should not get any events in this case, should we?