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(-)
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 <thuth@redhat.com>
---
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(-)
diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
index 2dcdead..aada753 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -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 : "");
}
diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
index 0cdfaec..f8d0190 100644
--- a/tests/libqos/usb.c
+++ b/tests/libqos/usb.c
@@ -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);
}
diff --git a/tests/libqtest.c b/tests/libqtest.c
index b9a1f18..4339d97 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -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")) {
+ /* 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);
+ }
+ 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;
+ 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 */
+ 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);
+}
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 3ae5709..9c1006f 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -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
diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index 5b500fe..7465bfd 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -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)
diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c
index 031764d..ae89362 100644
--- a/tests/usb-hcd-xhci-test.c
+++ b/tests/usb-hcd-xhci-test.c
@@ -11,7 +11,6 @@
#include "libqtest.h"
#include "libqos/usb.h"
-
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)
diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index 87a3b6e..6e7ba6f 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -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);
}
diff --git a/tests/virtio-serial-test.c b/tests/virtio-serial-test.c
index b14d943..97f8f52 100644
--- a/tests/virtio-serial-test.c
+++ b/tests/virtio-serial-test.c
@@ -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)
--
1.8.3.1
On Tue, Aug 15, 2017 at 11:22:44AM +0200, Thomas Huth wrote: > 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 <thuth@redhat.com> > --- > 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(-) > > diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c > index 2dcdead..aada753 100644 > --- a/tests/libqos/pci.c > +++ b/tests/libqos/pci.c > @@ -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 : ""); > } > diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c > index 0cdfaec..f8d0190 100644 > --- a/tests/libqos/usb.c > +++ b/tests/libqos/usb.c > @@ -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); > } > diff --git a/tests/libqtest.c b/tests/libqtest.c > index b9a1f18..4339d97 100644 > --- a/tests/libqtest.c > +++ b/tests/libqtest.c > @@ -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")) { > + /* 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? :-) > + 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; > + 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? > + 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); > +} > diff --git a/tests/libqtest.h b/tests/libqtest.h > index 3ae5709..9c1006f 100644 > --- a/tests/libqtest.h > +++ b/tests/libqtest.h > @@ -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 > diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c > index 5b500fe..7465bfd 100644 > --- a/tests/usb-hcd-uhci-test.c > +++ b/tests/usb-hcd-uhci-test.c > @@ -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) > diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c > index 031764d..ae89362 100644 > --- a/tests/usb-hcd-xhci-test.c > +++ b/tests/usb-hcd-xhci-test.c > @@ -11,7 +11,6 @@ > #include "libqtest.h" > #include "libqos/usb.h" > > - Nit: useless change Other than these, it looks quite good to me. Thanks, > 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) > diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c > index 87a3b6e..6e7ba6f 100644 > --- a/tests/virtio-scsi-test.c > +++ b/tests/virtio-scsi-test.c > @@ -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); > } > > diff --git a/tests/virtio-serial-test.c b/tests/virtio-serial-test.c > index b14d943..97f8f52 100644 > --- a/tests/virtio-serial-test.c > +++ b/tests/virtio-serial-test.c > @@ -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) > -- > 1.8.3.1 > > -- Peter Xu
On 13.09.2017 12:41, Peter Xu wrote: > On Tue, Aug 15, 2017 at 11:22:44AM +0200, Thomas Huth wrote: >> 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 <thuth@redhat.com> >> --- >> 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(-) >> >> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c >> index 2dcdead..aada753 100644 >> --- a/tests/libqos/pci.c >> +++ b/tests/libqos/pci.c >> @@ -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 : ""); >> } >> diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c >> index 0cdfaec..f8d0190 100644 >> --- a/tests/libqos/usb.c >> +++ b/tests/libqos/usb.c >> @@ -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); >> } >> diff --git a/tests/libqtest.c b/tests/libqtest.c >> index b9a1f18..4339d97 100644 >> --- a/tests/libqtest.c >> +++ b/tests/libqtest.c >> @@ -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")) { >> + /* 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? >> + 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; >> + 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? Yes, I'll update my patch accordingly... >> + 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); >> +} >> diff --git a/tests/libqtest.h b/tests/libqtest.h >> index 3ae5709..9c1006f 100644 >> --- a/tests/libqtest.h >> +++ b/tests/libqtest.h >> @@ -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 >> diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c >> index 5b500fe..7465bfd 100644 >> --- a/tests/usb-hcd-uhci-test.c >> +++ b/tests/usb-hcd-uhci-test.c >> @@ -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) >> diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c >> index 031764d..ae89362 100644 >> --- a/tests/usb-hcd-xhci-test.c >> +++ b/tests/usb-hcd-xhci-test.c >> @@ -11,7 +11,6 @@ >> #include "libqtest.h" >> #include "libqos/usb.h" >> >> - > > Nit: useless change > > Other than these, it looks quite good to me. Thanks, Thanks for the review! Thomas
On Wed, Sep 13, 2017 at 12:56:20PM +0200, Thomas Huth wrote: [...] > >> + > >> +/** > >> + * 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")) { > >> + /* 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? I think we should not, at least there is no such DEVICE_NEW message defined. Maybe that's the thing you mentioned? (say, an old test didn't flush the read pipe so the message from the old one leaked into a new test) Thanks, -- Peter Xu
© 2016 - 2024 Red Hat, Inc.