* [Qemu-devel] [RFC PATCH for 2.11] tests: Introduce generic device hot-plug/hot-unplug functions
@ 2017-08-15 9:22 Thomas Huth
2017-09-13 10:41 ` Peter Xu
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Huth @ 2017-08-15 9:22 UTC (permalink / raw)
To: qemu-devel
Cc: Gerd Hoffmann, Paolo Bonzini, Amit Shah, Markus Armbruster,
Eduardo Habkost
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [RFC PATCH for 2.11] tests: Introduce generic device hot-plug/hot-unplug functions
2017-08-15 9:22 [Qemu-devel] [RFC PATCH for 2.11] tests: Introduce generic device hot-plug/hot-unplug functions Thomas Huth
@ 2017-09-13 10:41 ` Peter Xu
2017-09-13 10:56 ` Thomas Huth
0 siblings, 1 reply; 4+ messages in thread
From: Peter Xu @ 2017-09-13 10:41 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Gerd Hoffmann,
Markus Armbruster, Amit Shah
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [RFC PATCH for 2.11] tests: Introduce generic device hot-plug/hot-unplug functions
2017-09-13 10:41 ` Peter Xu
@ 2017-09-13 10:56 ` Thomas Huth
2017-09-13 12:22 ` Peter Xu
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Huth @ 2017-09-13 10:56 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Gerd Hoffmann,
Markus Armbruster, Amit Shah
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [RFC PATCH for 2.11] tests: Introduce generic device hot-plug/hot-unplug functions
2017-09-13 10:56 ` Thomas Huth
@ 2017-09-13 12:22 ` Peter Xu
0 siblings, 0 replies; 4+ messages in thread
From: Peter Xu @ 2017-09-13 12:22 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Gerd Hoffmann,
Markus Armbruster, Amit Shah
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-09-13 12:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-15 9:22 [Qemu-devel] [RFC PATCH for 2.11] tests: Introduce generic device hot-plug/hot-unplug functions Thomas Huth
2017-09-13 10:41 ` Peter Xu
2017-09-13 10:56 ` Thomas Huth
2017-09-13 12:22 ` Peter Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).