* [Qemu-devel] [PATCH v2 0/3] Add a tester for HMP commands
@ 2017-03-30 7:50 Thomas Huth
2017-03-30 7:50 ` [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response " Thomas Huth
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Thomas Huth @ 2017-03-30 7:50 UTC (permalink / raw)
To: Dr. David Alan Gilbert, qemu-devel; +Cc: Markus Armbruster
We currently do not test HMP commands automatically yet, so if they
break, we do not notice this until somebody runs into the problem
(like the "info qtree" problem that we recently had on qemu-system-ppc64).
So let's add a simple tester that runs some HMP commands to check if they
can crash or abort QEMU.
Note: Three boards (isapc, puv3 and tricore_testboard) are currently
still blacklisted in the third patch due to crashes/aborts in HMP
commands there. I've added the problems to our BiteSizeTasks wiki page,
so I hope they will get fixed by GSoC students or somebody else soon.
Once the problems are fixed, the blacklisting of these boards can be
removed in the tester, too.
v2:
- Blacklist the boards "xenpv" and "xenfv" since they can not be used
without Xen (i.e. in plain TCG mode).
Thomas Huth (3):
libqtest: Ignore QMP events when parsing the response for HMP commands
libqtest: Add a generic function to run a callback function for every
machine
tests: Add a tester for HMP commands
tests/Makefile.include | 2 +
tests/libqtest.c | 36 +++++++++++
tests/libqtest.h | 8 +++
tests/pc-cpu-test.c | 95 +++++++++++------------------
tests/qom-test.c | 36 ++---------
tests/test-hmp.c | 161 +++++++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 249 insertions(+), 89 deletions(-)
create mode 100644 tests/test-hmp.c
--
1.8.3.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
2017-03-30 7:50 [Qemu-devel] [PATCH v2 0/3] Add a tester for HMP commands Thomas Huth
@ 2017-03-30 7:50 ` Thomas Huth
2017-04-03 19:09 ` John Snow
2017-03-30 7:50 ` [Qemu-devel] [PATCH v2 2/3] libqtest: Add a generic function to run a callback function for every machine Thomas Huth
2017-03-30 7:50 ` [Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands Thomas Huth
2 siblings, 1 reply; 16+ messages in thread
From: Thomas Huth @ 2017-03-30 7:50 UTC (permalink / raw)
To: Dr. David Alan Gilbert, qemu-devel; +Cc: Markus Armbruster
When running certain HMP commands (like "device_del") via QMP, we
can sometimes get a QMP event in the response first, so that the
"g_assert(ret)" statement in qtest_hmp() triggers and the test
fails. Fix this by ignoring such QMP events while looking for the
real return value from QMP.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
tests/libqtest.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index a5c3d2b..c9b2d76 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -580,6 +580,12 @@ char *qtest_hmpv(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 */
+ QDECREF(resp);
+ resp = qtest_qmp_receive(s);
+ ret = g_strdup(qdict_get_try_str(resp, "return"));
+ }
g_assert(ret);
QDECREF(resp);
g_free(cmd);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v2 2/3] libqtest: Add a generic function to run a callback function for every machine
2017-03-30 7:50 [Qemu-devel] [PATCH v2 0/3] Add a tester for HMP commands Thomas Huth
2017-03-30 7:50 ` [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response " Thomas Huth
@ 2017-03-30 7:50 ` Thomas Huth
2017-04-03 19:14 ` Philippe Mathieu-Daudé
2017-03-30 7:50 ` [Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands Thomas Huth
2 siblings, 1 reply; 16+ messages in thread
From: Thomas Huth @ 2017-03-30 7:50 UTC (permalink / raw)
To: Dr. David Alan Gilbert, qemu-devel; +Cc: Markus Armbruster
Some tests need to run single tests for every available machine of the
current QEMU binary. To avoid code duplication, let's extract this
code that deals with 'query-machines' into a separate function.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
tests/libqtest.c | 30 +++++++++++++++++
tests/libqtest.h | 8 +++++
tests/pc-cpu-test.c | 95 ++++++++++++++++++++---------------------------------
tests/qom-test.c | 36 ++++----------------
4 files changed, 80 insertions(+), 89 deletions(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index c9b2d76..d8b8066 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -938,3 +938,33 @@ bool qtest_big_endian(QTestState *s)
{
return s->big_endian;
}
+
+void qtest_cb_for_every_machine(void (*cb)(const char *machine))
+{
+ QDict *response, *minfo;
+ QList *list;
+ const QListEntry *p;
+ QObject *qobj;
+ QString *qstr;
+ const char *mname;
+
+ qtest_start("-machine none");
+ response = qmp("{ 'execute': 'query-machines' }");
+ g_assert(response);
+ list = qdict_get_qlist(response, "return");
+ g_assert(list);
+
+ for (p = qlist_first(list); p; p = qlist_next(p)) {
+ minfo = qobject_to_qdict(qlist_entry_obj(p));
+ g_assert(minfo);
+ qobj = qdict_get(minfo, "name");
+ g_assert(qobj);
+ qstr = qobject_to_qstring(qobj);
+ g_assert(qstr);
+ mname = qstring_get_str(qstr);
+ cb(mname);
+ }
+
+ qtest_end();
+ QDECREF(response);
+}
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 2c9962d..43ffadd 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -917,4 +917,12 @@ void qmp_fd_send(int fd, const char *fmt, ...);
QDict *qmp_fdv(int fd, const char *fmt, va_list ap);
QDict *qmp_fd(int fd, const char *fmt, ...);
+/**
+ * qtest_cb_for_every_machine:
+ * @cb: Pointer to the callback function
+ *
+ * Call a callback function for every name of all available machines.
+ */
+void qtest_cb_for_every_machine(void (*cb)(const char *machine));
+
#endif
diff --git a/tests/pc-cpu-test.c b/tests/pc-cpu-test.c
index c3a2633..c4211a4 100644
--- a/tests/pc-cpu-test.c
+++ b/tests/pc-cpu-test.c
@@ -79,69 +79,46 @@ static void test_data_free(gpointer data)
g_free(pc);
}
-static void add_pc_test_cases(void)
+static void add_pc_test_case(const char *mname)
{
- QDict *response, *minfo;
- QList *list;
- const QListEntry *p;
- QObject *qobj;
- QString *qstr;
- const char *mname;
char *path;
PCTestData *data;
- qtest_start("-machine none");
- response = qmp("{ 'execute': 'query-machines' }");
- g_assert(response);
- list = qdict_get_qlist(response, "return");
- g_assert(list);
-
- for (p = qlist_first(list); p; p = qlist_next(p)) {
- minfo = qobject_to_qdict(qlist_entry_obj(p));
- g_assert(minfo);
- qobj = qdict_get(minfo, "name");
- g_assert(qobj);
- qstr = qobject_to_qstring(qobj);
- g_assert(qstr);
- mname = qstring_get_str(qstr);
- if (!g_str_has_prefix(mname, "pc-")) {
- continue;
- }
- data = g_malloc(sizeof(PCTestData));
- data->machine = g_strdup(mname);
- data->cpu_model = "Haswell"; /* 1.3+ theoretically */
- data->sockets = 1;
- data->cores = 3;
- data->threads = 2;
- data->maxcpus = data->sockets * data->cores * data->threads * 2;
- if (g_str_has_suffix(mname, "-1.4") ||
- (strcmp(mname, "pc-1.3") == 0) ||
- (strcmp(mname, "pc-1.2") == 0) ||
- (strcmp(mname, "pc-1.1") == 0) ||
- (strcmp(mname, "pc-1.0") == 0) ||
- (strcmp(mname, "pc-0.15") == 0) ||
- (strcmp(mname, "pc-0.14") == 0) ||
- (strcmp(mname, "pc-0.13") == 0) ||
- (strcmp(mname, "pc-0.12") == 0) ||
- (strcmp(mname, "pc-0.11") == 0) ||
- (strcmp(mname, "pc-0.10") == 0)) {
- path = g_strdup_printf("cpu/%s/init/%ux%ux%u&maxcpus=%u",
- mname, data->sockets, data->cores,
- data->threads, data->maxcpus);
- qtest_add_data_func_full(path, data, test_pc_without_cpu_add,
- test_data_free);
- g_free(path);
- } else {
- path = g_strdup_printf("cpu/%s/add/%ux%ux%u&maxcpus=%u",
- mname, data->sockets, data->cores,
- data->threads, data->maxcpus);
- qtest_add_data_func_full(path, data, test_pc_with_cpu_add,
- test_data_free);
- g_free(path);
- }
+ if (!g_str_has_prefix(mname, "pc-")) {
+ return;
+ }
+ data = g_malloc(sizeof(PCTestData));
+ data->machine = g_strdup(mname);
+ data->cpu_model = "Haswell"; /* 1.3+ theoretically */
+ data->sockets = 1;
+ data->cores = 3;
+ data->threads = 2;
+ data->maxcpus = data->sockets * data->cores * data->threads * 2;
+ if (g_str_has_suffix(mname, "-1.4") ||
+ (strcmp(mname, "pc-1.3") == 0) ||
+ (strcmp(mname, "pc-1.2") == 0) ||
+ (strcmp(mname, "pc-1.1") == 0) ||
+ (strcmp(mname, "pc-1.0") == 0) ||
+ (strcmp(mname, "pc-0.15") == 0) ||
+ (strcmp(mname, "pc-0.14") == 0) ||
+ (strcmp(mname, "pc-0.13") == 0) ||
+ (strcmp(mname, "pc-0.12") == 0) ||
+ (strcmp(mname, "pc-0.11") == 0) ||
+ (strcmp(mname, "pc-0.10") == 0)) {
+ path = g_strdup_printf("cpu/%s/init/%ux%ux%u&maxcpus=%u",
+ mname, data->sockets, data->cores,
+ data->threads, data->maxcpus);
+ qtest_add_data_func_full(path, data, test_pc_without_cpu_add,
+ test_data_free);
+ g_free(path);
+ } else {
+ path = g_strdup_printf("cpu/%s/add/%ux%ux%u&maxcpus=%u",
+ mname, data->sockets, data->cores,
+ data->threads, data->maxcpus);
+ qtest_add_data_func_full(path, data, test_pc_with_cpu_add,
+ test_data_free);
+ g_free(path);
}
- QDECREF(response);
- qtest_end();
}
int main(int argc, char **argv)
@@ -151,7 +128,7 @@ int main(int argc, char **argv)
g_test_init(&argc, &argv, NULL);
if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
- add_pc_test_cases();
+ qtest_cb_for_every_machine(add_pc_test_case);
}
return g_test_run();
diff --git a/tests/qom-test.c b/tests/qom-test.c
index d48f890..ab0595d 100644
--- a/tests/qom-test.c
+++ b/tests/qom-test.c
@@ -107,46 +107,22 @@ static void test_machine(gconstpointer data)
g_free((void *)machine);
}
-static void add_machine_test_cases(void)
+static void add_machine_test_case(const char *mname)
{
const char *arch = qtest_get_arch();
- QDict *response, *minfo;
- QList *list;
- const QListEntry *p;
- QObject *qobj;
- QString *qstr;
- const char *mname;
- qtest_start("-machine none");
- response = qmp("{ 'execute': 'query-machines' }");
- g_assert(response);
- list = qdict_get_qlist(response, "return");
- g_assert(list);
-
- for (p = qlist_first(list); p; p = qlist_next(p)) {
- minfo = qobject_to_qdict(qlist_entry_obj(p));
- g_assert(minfo);
- qobj = qdict_get(minfo, "name");
- g_assert(qobj);
- qstr = qobject_to_qstring(qobj);
- g_assert(qstr);
- mname = qstring_get_str(qstr);
- if (!is_blacklisted(arch, mname)) {
- char *path = g_strdup_printf("qom/%s", mname);
- qtest_add_data_func(path, g_strdup(mname), test_machine);
- g_free(path);
- }
+ if (!is_blacklisted(arch, mname)) {
+ char *path = g_strdup_printf("qom/%s", mname);
+ qtest_add_data_func(path, g_strdup(mname), test_machine);
+ g_free(path);
}
-
- qtest_end();
- QDECREF(response);
}
int main(int argc, char **argv)
{
g_test_init(&argc, &argv, NULL);
- add_machine_test_cases();
+ qtest_cb_for_every_machine(add_machine_test_case);
return g_test_run();
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands
2017-03-30 7:50 [Qemu-devel] [PATCH v2 0/3] Add a tester for HMP commands Thomas Huth
2017-03-30 7:50 ` [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response " Thomas Huth
2017-03-30 7:50 ` [Qemu-devel] [PATCH v2 2/3] libqtest: Add a generic function to run a callback function for every machine Thomas Huth
@ 2017-03-30 7:50 ` Thomas Huth
2017-04-24 11:23 ` Dr. David Alan Gilbert
` (2 more replies)
2 siblings, 3 replies; 16+ messages in thread
From: Thomas Huth @ 2017-03-30 7:50 UTC (permalink / raw)
To: Dr. David Alan Gilbert, qemu-devel; +Cc: Markus Armbruster
HMP commands do not get any automatic testing yet, so on certain
QEMU machines, some HMP commands were causing crashes in the past.
Thus we should test HMP commands in our test suite, too, to avoid
that such problems creep in again in the future.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
tests/Makefile.include | 2 +
tests/test-hmp.c | 161 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 163 insertions(+)
create mode 100644 tests/test-hmp.c
diff --git a/tests/Makefile.include b/tests/Makefile.include
index f3de81f..0e754ac 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -331,6 +331,7 @@ check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
check-qtest-generic-y += tests/qom-test$(EXESUF)
+check-qtest-generic-y += tests/test-hmp$(EXESUF)
qapi-schema += alternate-any.json
qapi-schema += alternate-array.json
@@ -720,6 +721,7 @@ tests/tpci200-test$(EXESUF): tests/tpci200-test.o
tests/display-vga-test$(EXESUF): tests/display-vga-test.o
tests/ipoctal232-test$(EXESUF): tests/ipoctal232-test.o
tests/qom-test$(EXESUF): tests/qom-test.o
+tests/test-hmp$(EXESUF): tests/test-hmp.o
tests/drive_del-test$(EXESUF): tests/drive_del-test.o $(libqos-pc-obj-y)
tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o $(libqos-pc-obj-y)
tests/nvme-test$(EXESUF): tests/nvme-test.o
diff --git a/tests/test-hmp.c b/tests/test-hmp.c
new file mode 100644
index 0000000..339ed41
--- /dev/null
+++ b/tests/test-hmp.c
@@ -0,0 +1,161 @@
+/*
+ * Test HMP commands.
+ *
+ * Copyright (c) 2017 Red Hat Inc.
+ *
+ * Author:
+ * Thomas Huth <thuth@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2
+ * or later. See the COPYING file in the top-level directory.
+ *
+ * This test calls some HMP commands for all machines that the current
+ * QEMU binary provides, to check whether they terminate successfully
+ * (i.e. do not crash QEMU).
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+
+static int verbose;
+
+static const char *hmp_cmds[] = {
+ "boot_set ndc",
+ "chardev-add null,id=testchardev1",
+ "chardev-remove testchardev1",
+ "commit all",
+ "cpu-add 1",
+ "cpu 0",
+ "device_add ?",
+ "device_add usb-mouse,id=mouse1",
+ "mouse_button 7",
+ "mouse_move 10 10",
+ "mouse_button 0",
+ "device_del mouse1",
+ "dump-guest-memory /dev/null 0 4096",
+ "gdbserver",
+ "host_net_add user id=net0",
+ "hostfwd_add tcp::43210-:43210",
+ "hostfwd_remove tcp::43210-:43210",
+ "host_net_remove 0 net0",
+ "i /w 0",
+ "log all",
+ "log none",
+ "memsave 0 4096 \"/dev/null\"",
+ "migrate_set_cache_size 1",
+ "migrate_set_downtime 1",
+ "migrate_set_speed 1",
+ "netdev_add user,id=net1",
+ "set_link net1 off",
+ "set_link net1 on",
+ "netdev_del net1",
+ "nmi",
+ "o /w 0 0x1234",
+ "object_add memory-backend-ram,id=mem1,size=256M",
+ "object_del mem1",
+ "pmemsave 0 4096 \"/dev/null\"",
+ "p $pc + 8",
+ "qom-list /",
+ "qom-set /machine initrd test",
+ "screendump /dev/null",
+ "sendkey x",
+ "singlestep on",
+ "wavcapture /dev/null",
+ "stopcapture 0",
+ "sum 0 512",
+ "x /8i 0x100",
+ "xp /16x 0",
+ NULL
+};
+
+/* Run through the list of pre-defined commands */
+static void test_commands(void)
+{
+ char *response;
+ int i;
+
+ for (i = 0; hmp_cmds[i] != NULL; i++) {
+ if (verbose) {
+ fprintf(stderr, "\t%s\n", hmp_cmds[i]);
+ }
+ response = hmp(hmp_cmds[i]);
+ g_free(response);
+ }
+
+}
+
+/* Run through all info commands and call them blindly (without arguments) */
+static void test_info_commands(void)
+{
+ char *resp, *info, *info_buf, *endp;
+
+ info_buf = info = hmp("help info");
+
+ while (*info) {
+ /* Extract the info command, ignore parameters and description */
+ g_assert(strncmp(info, "info ", 5) == 0);
+ endp = strchr(&info[5], ' ');
+ g_assert(endp != NULL);
+ *endp = '\0';
+ /* Now run the info command */
+ if (verbose) {
+ fprintf(stderr, "\t%s\n", info);
+ }
+ resp = hmp(info);
+ g_free(resp);
+ /* And move forward to the next line */
+ info = strchr(endp + 1, '\n');
+ if (!info) {
+ break;
+ }
+ info += 1;
+ }
+
+ g_free(info_buf);
+}
+
+static void test_machine(gconstpointer data)
+{
+ const char *machine = data;
+ char *args;
+
+ args = g_strdup_printf("-S -M %s", machine);
+ qtest_start(args);
+
+ test_info_commands();
+ test_commands();
+
+ qtest_end();
+ g_free(args);
+ g_free((void *)data);
+}
+
+static void add_machine_test_case(const char *mname)
+{
+ char *path = g_strdup_printf("hmp/%s", mname);
+
+ /* Ignore blacklisted machines that have known problems */
+ if (!strcmp("isapc", mname) == 0 || !strcmp("puv3", mname)
+ || !strcmp("tricore_testboard", mname)
+ || !strcmp("xenfv", mname) == 0 || !strcmp("xenpv", mname)) {
+ return;
+ }
+
+ qtest_add_data_func(path, g_strdup(mname), test_machine);
+ g_free(path);
+}
+
+int main(int argc, char **argv)
+{
+ char *v_env = getenv("V");
+
+ if (v_env && *v_env >= '2') {
+ verbose = true;
+ }
+
+ g_test_init(&argc, &argv, NULL);
+
+ qtest_cb_for_every_machine(add_machine_test_case);
+
+ return g_test_run();
+}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
2017-03-30 7:50 ` [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response " Thomas Huth
@ 2017-04-03 19:09 ` John Snow
2017-04-04 7:31 ` Thomas Huth
0 siblings, 1 reply; 16+ messages in thread
From: John Snow @ 2017-04-03 19:09 UTC (permalink / raw)
To: Thomas Huth, Dr. David Alan Gilbert, qemu-devel; +Cc: Markus Armbruster
On 03/30/2017 03:50 AM, Thomas Huth wrote:
> When running certain HMP commands (like "device_del") via QMP, we
> can sometimes get a QMP event in the response first, so that the
> "g_assert(ret)" statement in qtest_hmp() triggers and the test
> fails. Fix this by ignoring such QMP events while looking for the
> real return value from QMP.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> tests/libqtest.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index a5c3d2b..c9b2d76 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -580,6 +580,12 @@ char *qtest_hmpv(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 */
> + QDECREF(resp);
> + resp = qtest_qmp_receive(s);
> + ret = g_strdup(qdict_get_try_str(resp, "return"));
> + }
> g_assert(ret);
> QDECREF(resp);
> g_free(cmd);
>
You've probably been asked this, but can you just shove the QMP response
you don't want into the event queue for consumption by other calls?
--js
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/3] libqtest: Add a generic function to run a callback function for every machine
2017-03-30 7:50 ` [Qemu-devel] [PATCH v2 2/3] libqtest: Add a generic function to run a callback function for every machine Thomas Huth
@ 2017-04-03 19:14 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-04-03 19:14 UTC (permalink / raw)
To: Thomas Huth, Dr. David Alan Gilbert, qemu-devel; +Cc: Markus Armbruster
On 03/30/2017 04:50 AM, Thomas Huth wrote:
> Some tests need to run single tests for every available machine of the
> current QEMU binary. To avoid code duplication, let's extract this
> code that deals with 'query-machines' into a separate function.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> tests/libqtest.c | 30 +++++++++++++++++
> tests/libqtest.h | 8 +++++
> tests/pc-cpu-test.c | 95 ++++++++++++++++++++---------------------------------
> tests/qom-test.c | 36 ++++----------------
> 4 files changed, 80 insertions(+), 89 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index c9b2d76..d8b8066 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -938,3 +938,33 @@ bool qtest_big_endian(QTestState *s)
> {
> return s->big_endian;
> }
> +
> +void qtest_cb_for_every_machine(void (*cb)(const char *machine))
> +{
> + QDict *response, *minfo;
> + QList *list;
> + const QListEntry *p;
> + QObject *qobj;
> + QString *qstr;
> + const char *mname;
> +
> + qtest_start("-machine none");
> + response = qmp("{ 'execute': 'query-machines' }");
> + g_assert(response);
> + list = qdict_get_qlist(response, "return");
> + g_assert(list);
> +
> + for (p = qlist_first(list); p; p = qlist_next(p)) {
> + minfo = qobject_to_qdict(qlist_entry_obj(p));
> + g_assert(minfo);
> + qobj = qdict_get(minfo, "name");
> + g_assert(qobj);
> + qstr = qobject_to_qstring(qobj);
> + g_assert(qstr);
> + mname = qstring_get_str(qstr);
> + cb(mname);
> + }
> +
> + qtest_end();
> + QDECREF(response);
> +}
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 2c9962d..43ffadd 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -917,4 +917,12 @@ void qmp_fd_send(int fd, const char *fmt, ...);
> QDict *qmp_fdv(int fd, const char *fmt, va_list ap);
> QDict *qmp_fd(int fd, const char *fmt, ...);
>
> +/**
> + * qtest_cb_for_every_machine:
> + * @cb: Pointer to the callback function
> + *
> + * Call a callback function for every name of all available machines.
> + */
> +void qtest_cb_for_every_machine(void (*cb)(const char *machine));
> +
> #endif
> diff --git a/tests/pc-cpu-test.c b/tests/pc-cpu-test.c
> index c3a2633..c4211a4 100644
> --- a/tests/pc-cpu-test.c
> +++ b/tests/pc-cpu-test.c
> @@ -79,69 +79,46 @@ static void test_data_free(gpointer data)
> g_free(pc);
> }
>
> -static void add_pc_test_cases(void)
> +static void add_pc_test_case(const char *mname)
> {
> - QDict *response, *minfo;
> - QList *list;
> - const QListEntry *p;
> - QObject *qobj;
> - QString *qstr;
> - const char *mname;
> char *path;
> PCTestData *data;
>
> - qtest_start("-machine none");
> - response = qmp("{ 'execute': 'query-machines' }");
> - g_assert(response);
> - list = qdict_get_qlist(response, "return");
> - g_assert(list);
> -
> - for (p = qlist_first(list); p; p = qlist_next(p)) {
> - minfo = qobject_to_qdict(qlist_entry_obj(p));
> - g_assert(minfo);
> - qobj = qdict_get(minfo, "name");
> - g_assert(qobj);
> - qstr = qobject_to_qstring(qobj);
> - g_assert(qstr);
> - mname = qstring_get_str(qstr);
> - if (!g_str_has_prefix(mname, "pc-")) {
> - continue;
> - }
> - data = g_malloc(sizeof(PCTestData));
> - data->machine = g_strdup(mname);
> - data->cpu_model = "Haswell"; /* 1.3+ theoretically */
> - data->sockets = 1;
> - data->cores = 3;
> - data->threads = 2;
> - data->maxcpus = data->sockets * data->cores * data->threads * 2;
> - if (g_str_has_suffix(mname, "-1.4") ||
> - (strcmp(mname, "pc-1.3") == 0) ||
> - (strcmp(mname, "pc-1.2") == 0) ||
> - (strcmp(mname, "pc-1.1") == 0) ||
> - (strcmp(mname, "pc-1.0") == 0) ||
> - (strcmp(mname, "pc-0.15") == 0) ||
> - (strcmp(mname, "pc-0.14") == 0) ||
> - (strcmp(mname, "pc-0.13") == 0) ||
> - (strcmp(mname, "pc-0.12") == 0) ||
> - (strcmp(mname, "pc-0.11") == 0) ||
> - (strcmp(mname, "pc-0.10") == 0)) {
> - path = g_strdup_printf("cpu/%s/init/%ux%ux%u&maxcpus=%u",
> - mname, data->sockets, data->cores,
> - data->threads, data->maxcpus);
> - qtest_add_data_func_full(path, data, test_pc_without_cpu_add,
> - test_data_free);
> - g_free(path);
> - } else {
> - path = g_strdup_printf("cpu/%s/add/%ux%ux%u&maxcpus=%u",
> - mname, data->sockets, data->cores,
> - data->threads, data->maxcpus);
> - qtest_add_data_func_full(path, data, test_pc_with_cpu_add,
> - test_data_free);
> - g_free(path);
> - }
> + if (!g_str_has_prefix(mname, "pc-")) {
> + return;
> + }
> + data = g_malloc(sizeof(PCTestData));
> + data->machine = g_strdup(mname);
> + data->cpu_model = "Haswell"; /* 1.3+ theoretically */
> + data->sockets = 1;
> + data->cores = 3;
> + data->threads = 2;
> + data->maxcpus = data->sockets * data->cores * data->threads * 2;
> + if (g_str_has_suffix(mname, "-1.4") ||
> + (strcmp(mname, "pc-1.3") == 0) ||
> + (strcmp(mname, "pc-1.2") == 0) ||
> + (strcmp(mname, "pc-1.1") == 0) ||
> + (strcmp(mname, "pc-1.0") == 0) ||
> + (strcmp(mname, "pc-0.15") == 0) ||
> + (strcmp(mname, "pc-0.14") == 0) ||
> + (strcmp(mname, "pc-0.13") == 0) ||
> + (strcmp(mname, "pc-0.12") == 0) ||
> + (strcmp(mname, "pc-0.11") == 0) ||
> + (strcmp(mname, "pc-0.10") == 0)) {
> + path = g_strdup_printf("cpu/%s/init/%ux%ux%u&maxcpus=%u",
> + mname, data->sockets, data->cores,
> + data->threads, data->maxcpus);
> + qtest_add_data_func_full(path, data, test_pc_without_cpu_add,
> + test_data_free);
> + g_free(path);
> + } else {
> + path = g_strdup_printf("cpu/%s/add/%ux%ux%u&maxcpus=%u",
> + mname, data->sockets, data->cores,
> + data->threads, data->maxcpus);
> + qtest_add_data_func_full(path, data, test_pc_with_cpu_add,
> + test_data_free);
> + g_free(path);
> }
> - QDECREF(response);
> - qtest_end();
> }
>
> int main(int argc, char **argv)
> @@ -151,7 +128,7 @@ int main(int argc, char **argv)
> g_test_init(&argc, &argv, NULL);
>
> if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> - add_pc_test_cases();
> + qtest_cb_for_every_machine(add_pc_test_case);
> }
>
> return g_test_run();
> diff --git a/tests/qom-test.c b/tests/qom-test.c
> index d48f890..ab0595d 100644
> --- a/tests/qom-test.c
> +++ b/tests/qom-test.c
> @@ -107,46 +107,22 @@ static void test_machine(gconstpointer data)
> g_free((void *)machine);
> }
>
> -static void add_machine_test_cases(void)
> +static void add_machine_test_case(const char *mname)
> {
> const char *arch = qtest_get_arch();
> - QDict *response, *minfo;
> - QList *list;
> - const QListEntry *p;
> - QObject *qobj;
> - QString *qstr;
> - const char *mname;
>
> - qtest_start("-machine none");
> - response = qmp("{ 'execute': 'query-machines' }");
> - g_assert(response);
> - list = qdict_get_qlist(response, "return");
> - g_assert(list);
> -
> - for (p = qlist_first(list); p; p = qlist_next(p)) {
> - minfo = qobject_to_qdict(qlist_entry_obj(p));
> - g_assert(minfo);
> - qobj = qdict_get(minfo, "name");
> - g_assert(qobj);
> - qstr = qobject_to_qstring(qobj);
> - g_assert(qstr);
> - mname = qstring_get_str(qstr);
> - if (!is_blacklisted(arch, mname)) {
> - char *path = g_strdup_printf("qom/%s", mname);
> - qtest_add_data_func(path, g_strdup(mname), test_machine);
> - g_free(path);
> - }
> + if (!is_blacklisted(arch, mname)) {
> + char *path = g_strdup_printf("qom/%s", mname);
> + qtest_add_data_func(path, g_strdup(mname), test_machine);
> + g_free(path);
> }
> -
> - qtest_end();
> - QDECREF(response);
> }
>
> int main(int argc, char **argv)
> {
> g_test_init(&argc, &argv, NULL);
>
> - add_machine_test_cases();
> + qtest_cb_for_every_machine(add_machine_test_case);
>
> return g_test_run();
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
2017-04-03 19:09 ` John Snow
@ 2017-04-04 7:31 ` Thomas Huth
2017-04-04 14:33 ` John Snow
0 siblings, 1 reply; 16+ messages in thread
From: Thomas Huth @ 2017-04-04 7:31 UTC (permalink / raw)
To: John Snow, Dr. David Alan Gilbert, qemu-devel; +Cc: Markus Armbruster
On 03.04.2017 21:09, John Snow wrote:
>
>
> On 03/30/2017 03:50 AM, Thomas Huth wrote:
>> When running certain HMP commands (like "device_del") via QMP, we
>> can sometimes get a QMP event in the response first, so that the
>> "g_assert(ret)" statement in qtest_hmp() triggers and the test
>> fails. Fix this by ignoring such QMP events while looking for the
>> real return value from QMP.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> tests/libqtest.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index a5c3d2b..c9b2d76 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -580,6 +580,12 @@ char *qtest_hmpv(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 */
>> + QDECREF(resp);
>> + resp = qtest_qmp_receive(s);
>> + ret = g_strdup(qdict_get_try_str(resp, "return"));
>> + }
>> g_assert(ret);
>> QDECREF(resp);
>> g_free(cmd);
>>
>
> You've probably been asked this, but can you just shove the QMP response
> you don't want into the event queue for consumption by other calls?
Well, this is the qtest_hmpv() function, so I assume that the caller
just wants to execute a HMP command and does not really care about QMP
events. If you care about QMP events, you should use the qmp functions
instead.
Thomas
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
2017-04-04 7:31 ` Thomas Huth
@ 2017-04-04 14:33 ` John Snow
2017-04-07 17:58 ` Dr. David Alan Gilbert
2017-04-11 9:12 ` Markus Armbruster
0 siblings, 2 replies; 16+ messages in thread
From: John Snow @ 2017-04-04 14:33 UTC (permalink / raw)
To: Thomas Huth, Dr. David Alan Gilbert, qemu-devel; +Cc: Markus Armbruster
On 04/04/2017 03:31 AM, Thomas Huth wrote:
> On 03.04.2017 21:09, John Snow wrote:
>>
>>
>> On 03/30/2017 03:50 AM, Thomas Huth wrote:
>>> When running certain HMP commands (like "device_del") via QMP, we
>>> can sometimes get a QMP event in the response first, so that the
>>> "g_assert(ret)" statement in qtest_hmp() triggers and the test
>>> fails. Fix this by ignoring such QMP events while looking for the
>>> real return value from QMP.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>> tests/libqtest.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>>> index a5c3d2b..c9b2d76 100644
>>> --- a/tests/libqtest.c
>>> +++ b/tests/libqtest.c
>>> @@ -580,6 +580,12 @@ char *qtest_hmpv(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 */
>>> + QDECREF(resp);
>>> + resp = qtest_qmp_receive(s);
>>> + ret = g_strdup(qdict_get_try_str(resp, "return"));
>>> + }
>>> g_assert(ret);
>>> QDECREF(resp);
>>> g_free(cmd);
>>>
>>
>> You've probably been asked this, but can you just shove the QMP response
>> you don't want into the event queue for consumption by other calls?
>
> Well, this is the qtest_hmpv() function, so I assume that the caller
> just wants to execute a HMP command and does not really care about QMP
> events. If you care about QMP events, you should use the qmp functions
> instead.
>
> Thomas
>
I don't think it's obvious that using HMP functions should cause the QMP
stream to become faulty, though.
If someone uses an HMP function and then tries to wait on a QMP event to
confirm that some key condition has occurred (pausing or resuming, for
instance) it would not be immediately apparent from the user's POV that
this function just eats replies because it was convenient to do so.
I guess the event queue only exists in python though, so it's not as
trivial as I was thinking it would be...
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
2017-04-04 14:33 ` John Snow
@ 2017-04-07 17:58 ` Dr. David Alan Gilbert
2017-04-11 9:12 ` Markus Armbruster
1 sibling, 0 replies; 16+ messages in thread
From: Dr. David Alan Gilbert @ 2017-04-07 17:58 UTC (permalink / raw)
To: John Snow; +Cc: Thomas Huth, qemu-devel, Markus Armbruster
* John Snow (jsnow@redhat.com) wrote:
>
>
> On 04/04/2017 03:31 AM, Thomas Huth wrote:
> > On 03.04.2017 21:09, John Snow wrote:
> >>
> >>
> >> On 03/30/2017 03:50 AM, Thomas Huth wrote:
> >>> When running certain HMP commands (like "device_del") via QMP, we
> >>> can sometimes get a QMP event in the response first, so that the
> >>> "g_assert(ret)" statement in qtest_hmp() triggers and the test
> >>> fails. Fix this by ignoring such QMP events while looking for the
> >>> real return value from QMP.
> >>>
> >>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >>> ---
> >>> tests/libqtest.c | 6 ++++++
> >>> 1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/tests/libqtest.c b/tests/libqtest.c
> >>> index a5c3d2b..c9b2d76 100644
> >>> --- a/tests/libqtest.c
> >>> +++ b/tests/libqtest.c
> >>> @@ -580,6 +580,12 @@ char *qtest_hmpv(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 */
> >>> + QDECREF(resp);
> >>> + resp = qtest_qmp_receive(s);
> >>> + ret = g_strdup(qdict_get_try_str(resp, "return"));
> >>> + }
> >>> g_assert(ret);
> >>> QDECREF(resp);
> >>> g_free(cmd);
> >>>
> >>
> >> You've probably been asked this, but can you just shove the QMP response
> >> you don't want into the event queue for consumption by other calls?
> >
> > Well, this is the qtest_hmpv() function, so I assume that the caller
> > just wants to execute a HMP command and does not really care about QMP
> > events. If you care about QMP events, you should use the qmp functions
> > instead.
> >
> > Thomas
> >
>
> I don't think it's obvious that using HMP functions should cause the QMP
> stream to become faulty, though.
>
> If someone uses an HMP function and then tries to wait on a QMP event to
> confirm that some key condition has occurred (pausing or resuming, for
> instance) it would not be immediately apparent from the user's POV that
> this function just eats replies because it was convenient to do so.
>
> I guess the event queue only exists in python though, so it's not as
> trivial as I was thinking it would be...
I think it's OK to discard the QMP events - it feels rare to mix and match
in a test; if you care about QMP events you'll probably be basing the
test around QMP rather than HMP.
Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
2017-04-04 14:33 ` John Snow
2017-04-07 17:58 ` Dr. David Alan Gilbert
@ 2017-04-11 9:12 ` Markus Armbruster
2017-04-24 11:31 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 16+ messages in thread
From: Markus Armbruster @ 2017-04-11 9:12 UTC (permalink / raw)
To: John Snow; +Cc: Thomas Huth, Dr. David Alan Gilbert, qemu-devel
John Snow <jsnow@redhat.com> writes:
> On 04/04/2017 03:31 AM, Thomas Huth wrote:
>> On 03.04.2017 21:09, John Snow wrote:
>>>
>>>
>>> On 03/30/2017 03:50 AM, Thomas Huth wrote:
>>>> When running certain HMP commands (like "device_del") via QMP, we
>>>> can sometimes get a QMP event in the response first, so that the
>>>> "g_assert(ret)" statement in qtest_hmp() triggers and the test
>>>> fails. Fix this by ignoring such QMP events while looking for the
>>>> real return value from QMP.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>> tests/libqtest.c | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>>>> index a5c3d2b..c9b2d76 100644
>>>> --- a/tests/libqtest.c
>>>> +++ b/tests/libqtest.c
>>>> @@ -580,6 +580,12 @@ char *qtest_hmpv(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 */
>>>> + QDECREF(resp);
>>>> + resp = qtest_qmp_receive(s);
>>>> + ret = g_strdup(qdict_get_try_str(resp, "return"));
>>>> + }
>>>> g_assert(ret);
>>>> QDECREF(resp);
>>>> g_free(cmd);
>>>>
>>>
>>> You've probably been asked this, but can you just shove the QMP response
>>> you don't want into the event queue for consumption by other calls?
>>
>> Well, this is the qtest_hmpv() function, so I assume that the caller
>> just wants to execute a HMP command and does not really care about QMP
>> events. If you care about QMP events, you should use the qmp functions
>> instead.
>>
>> Thomas
>>
>
> I don't think it's obvious that using HMP functions should cause the QMP
> stream to become faulty, though.
qtest_hmpv() is a helper function. It tries to be convenient for the
common case. In particular, it receives, checks and consumes the QMP
reply. Before the patch, it screws up when QMP events arrive before the
reply. The patch fixes it by also consuming the events. Makes sense.
The non-obviousness should be addressed in the function comment.
> If someone uses an HMP function and then tries to wait on a QMP event to
> confirm that some key condition has occurred (pausing or resuming, for
> instance) it would not be immediately apparent from the user's POV that
> this function just eats replies because it was convenient to do so.
Code that needs to see events can't use this helper function. It needs
to use more primitive functions instead.
> I guess the event queue only exists in python though, so it's not as
> trivial as I was thinking it would be...
More sophisticated handling of QMP events in libqtest is out of scope
for this patch. I'm not passing judgement on whether it would be
useful :)
With the function comment updated to mention QMP events get consumed:
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands
2017-03-30 7:50 ` [Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands Thomas Huth
@ 2017-04-24 11:23 ` Dr. David Alan Gilbert
2017-04-28 1:56 ` Eric Blake
2017-06-27 2:55 ` Eric Blake
2 siblings, 0 replies; 16+ messages in thread
From: Dr. David Alan Gilbert @ 2017-04-24 11:23 UTC (permalink / raw)
To: Thomas Huth; +Cc: qemu-devel, Markus Armbruster
* Thomas Huth (thuth@redhat.com) wrote:
> HMP commands do not get any automatic testing yet, so on certain
> QEMU machines, some HMP commands were causing crashes in the past.
> Thus we should test HMP commands in our test suite, too, to avoid
> that such problems creep in again in the future.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> tests/Makefile.include | 2 +
> tests/test-hmp.c | 161 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 163 insertions(+)
> create mode 100644 tests/test-hmp.c
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index f3de81f..0e754ac 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -331,6 +331,7 @@ check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
> check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
>
> check-qtest-generic-y += tests/qom-test$(EXESUF)
> +check-qtest-generic-y += tests/test-hmp$(EXESUF)
>
> qapi-schema += alternate-any.json
> qapi-schema += alternate-array.json
> @@ -720,6 +721,7 @@ tests/tpci200-test$(EXESUF): tests/tpci200-test.o
> tests/display-vga-test$(EXESUF): tests/display-vga-test.o
> tests/ipoctal232-test$(EXESUF): tests/ipoctal232-test.o
> tests/qom-test$(EXESUF): tests/qom-test.o
> +tests/test-hmp$(EXESUF): tests/test-hmp.o
> tests/drive_del-test$(EXESUF): tests/drive_del-test.o $(libqos-pc-obj-y)
> tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o $(libqos-pc-obj-y)
> tests/nvme-test$(EXESUF): tests/nvme-test.o
> diff --git a/tests/test-hmp.c b/tests/test-hmp.c
> new file mode 100644
> index 0000000..339ed41
> --- /dev/null
> +++ b/tests/test-hmp.c
> @@ -0,0 +1,161 @@
> +/*
> + * Test HMP commands.
> + *
> + * Copyright (c) 2017 Red Hat Inc.
> + *
> + * Author:
> + * Thomas Huth <thuth@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2
> + * or later. See the COPYING file in the top-level directory.
> + *
> + * This test calls some HMP commands for all machines that the current
> + * QEMU binary provides, to check whether they terminate successfully
> + * (i.e. do not crash QEMU).
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +
> +static int verbose;
> +
> +static const char *hmp_cmds[] = {
> + "boot_set ndc",
> + "chardev-add null,id=testchardev1",
> + "chardev-remove testchardev1",
> + "commit all",
> + "cpu-add 1",
> + "cpu 0",
> + "device_add ?",
> + "device_add usb-mouse,id=mouse1",
> + "mouse_button 7",
> + "mouse_move 10 10",
> + "mouse_button 0",
> + "device_del mouse1",
> + "dump-guest-memory /dev/null 0 4096",
> + "gdbserver",
> + "host_net_add user id=net0",
> + "hostfwd_add tcp::43210-:43210",
> + "hostfwd_remove tcp::43210-:43210",
> + "host_net_remove 0 net0",
> + "i /w 0",
> + "log all",
> + "log none",
> + "memsave 0 4096 \"/dev/null\"",
> + "migrate_set_cache_size 1",
> + "migrate_set_downtime 1",
> + "migrate_set_speed 1",
> + "netdev_add user,id=net1",
> + "set_link net1 off",
> + "set_link net1 on",
> + "netdev_del net1",
> + "nmi",
> + "o /w 0 0x1234",
> + "object_add memory-backend-ram,id=mem1,size=256M",
> + "object_del mem1",
> + "pmemsave 0 4096 \"/dev/null\"",
> + "p $pc + 8",
> + "qom-list /",
> + "qom-set /machine initrd test",
> + "screendump /dev/null",
> + "sendkey x",
> + "singlestep on",
> + "wavcapture /dev/null",
> + "stopcapture 0",
> + "sum 0 512",
> + "x /8i 0x100",
> + "xp /16x 0",
> + NULL
> +};
> +
> +/* Run through the list of pre-defined commands */
> +static void test_commands(void)
> +{
> + char *response;
> + int i;
> +
> + for (i = 0; hmp_cmds[i] != NULL; i++) {
> + if (verbose) {
> + fprintf(stderr, "\t%s\n", hmp_cmds[i]);
> + }
> + response = hmp(hmp_cmds[i]);
> + g_free(response);
> + }
> +
> +}
> +
> +/* Run through all info commands and call them blindly (without arguments) */
> +static void test_info_commands(void)
> +{
> + char *resp, *info, *info_buf, *endp;
> +
> + info_buf = info = hmp("help info");
> +
> + while (*info) {
> + /* Extract the info command, ignore parameters and description */
> + g_assert(strncmp(info, "info ", 5) == 0);
> + endp = strchr(&info[5], ' ');
> + g_assert(endp != NULL);
> + *endp = '\0';
> + /* Now run the info command */
> + if (verbose) {
> + fprintf(stderr, "\t%s\n", info);
> + }
> + resp = hmp(info);
> + g_free(resp);
> + /* And move forward to the next line */
> + info = strchr(endp + 1, '\n');
> + if (!info) {
> + break;
> + }
> + info += 1;
> + }
> +
> + g_free(info_buf);
> +}
> +
> +static void test_machine(gconstpointer data)
> +{
> + const char *machine = data;
> + char *args;
> +
> + args = g_strdup_printf("-S -M %s", machine);
> + qtest_start(args);
> +
> + test_info_commands();
> + test_commands();
> +
> + qtest_end();
> + g_free(args);
> + g_free((void *)data);
> +}
> +
> +static void add_machine_test_case(const char *mname)
> +{
> + char *path = g_strdup_printf("hmp/%s", mname);
> +
> + /* Ignore blacklisted machines that have known problems */
> + if (!strcmp("isapc", mname) == 0 || !strcmp("puv3", mname)
> + || !strcmp("tricore_testboard", mname)
> + || !strcmp("xenfv", mname) == 0 || !strcmp("xenpv", mname)) {
You've got a leak of 'path' there.
However, it's only a test and it's small, so we can fix it up later.
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
and series queued.
Dave
> + return;
> + }
> +
> + qtest_add_data_func(path, g_strdup(mname), test_machine);
> + g_free(path);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + char *v_env = getenv("V");
> +
> + if (v_env && *v_env >= '2') {
> + verbose = true;
> + }
> +
> + g_test_init(&argc, &argv, NULL);
> +
> + qtest_cb_for_every_machine(add_machine_test_case);
> +
> + return g_test_run();
> +}
> --
> 1.8.3.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
2017-04-11 9:12 ` Markus Armbruster
@ 2017-04-24 11:31 ` Dr. David Alan Gilbert
2017-04-24 12:23 ` Markus Armbruster
0 siblings, 1 reply; 16+ messages in thread
From: Dr. David Alan Gilbert @ 2017-04-24 11:31 UTC (permalink / raw)
To: Markus Armbruster; +Cc: John Snow, Thomas Huth, qemu-devel
* Markus Armbruster (armbru@redhat.com) wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > On 04/04/2017 03:31 AM, Thomas Huth wrote:
> >> On 03.04.2017 21:09, John Snow wrote:
> >>>
> >>>
> >>> On 03/30/2017 03:50 AM, Thomas Huth wrote:
> >>>> When running certain HMP commands (like "device_del") via QMP, we
> >>>> can sometimes get a QMP event in the response first, so that the
> >>>> "g_assert(ret)" statement in qtest_hmp() triggers and the test
> >>>> fails. Fix this by ignoring such QMP events while looking for the
> >>>> real return value from QMP.
> >>>>
> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >>>> ---
> >>>> tests/libqtest.c | 6 ++++++
> >>>> 1 file changed, 6 insertions(+)
> >>>>
> >>>> diff --git a/tests/libqtest.c b/tests/libqtest.c
> >>>> index a5c3d2b..c9b2d76 100644
> >>>> --- a/tests/libqtest.c
> >>>> +++ b/tests/libqtest.c
> >>>> @@ -580,6 +580,12 @@ char *qtest_hmpv(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 */
> >>>> + QDECREF(resp);
> >>>> + resp = qtest_qmp_receive(s);
> >>>> + ret = g_strdup(qdict_get_try_str(resp, "return"));
> >>>> + }
> >>>> g_assert(ret);
> >>>> QDECREF(resp);
> >>>> g_free(cmd);
> >>>>
> >>>
> >>> You've probably been asked this, but can you just shove the QMP response
> >>> you don't want into the event queue for consumption by other calls?
> >>
> >> Well, this is the qtest_hmpv() function, so I assume that the caller
> >> just wants to execute a HMP command and does not really care about QMP
> >> events. If you care about QMP events, you should use the qmp functions
> >> instead.
> >>
> >> Thomas
> >>
> >
> > I don't think it's obvious that using HMP functions should cause the QMP
> > stream to become faulty, though.
>
> qtest_hmpv() is a helper function. It tries to be convenient for the
> common case. In particular, it receives, checks and consumes the QMP
> reply. Before the patch, it screws up when QMP events arrive before the
> reply. The patch fixes it by also consuming the events. Makes sense.
>
> The non-obviousness should be addressed in the function comment.
>
> > If someone uses an HMP function and then tries to wait on a QMP event to
> > confirm that some key condition has occurred (pausing or resuming, for
> > instance) it would not be immediately apparent from the user's POV that
> > this function just eats replies because it was convenient to do so.
>
> Code that needs to see events can't use this helper function. It needs
> to use more primitive functions instead.
>
> > I guess the event queue only exists in python though, so it's not as
> > trivial as I was thinking it would be...
>
> More sophisticated handling of QMP events in libqtest is out of scope
> for this patch. I'm not passing judgement on whether it would be
> useful :)
>
> With the function comment updated to mention QMP events get consumed:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
OK, I've added that comment to qtest_hmp and qtest_hmpv when I've
just queued it.
(and removed an extra 'v')
/**
- * qtest_hmpv:
+ * qtest_hmp:
* @s: #QTestState instance to operate on.
* @fmt...: HMP command to send to QEMU
*
* Send HMP command to QEMU via QMP's human-monitor-command.
+ * QMP events are discarded.
*
* Returns: the command's output. The caller should g_free() it.
*/
@@ -149,6 +150,7 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...);
* @ap: HMP command arguments
*
* Send HMP command to QEMU via QMP's human-monitor-command.
+ * QMP events are discarded.
*
* Returns: the command's output. The caller should g_free() it.
*/
Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response for HMP commands
2017-04-24 11:31 ` Dr. David Alan Gilbert
@ 2017-04-24 12:23 ` Markus Armbruster
0 siblings, 0 replies; 16+ messages in thread
From: Markus Armbruster @ 2017-04-24 12:23 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Thomas Huth, John Snow, qemu-devel
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>> > On 04/04/2017 03:31 AM, Thomas Huth wrote:
>> >> On 03.04.2017 21:09, John Snow wrote:
>> >>>
>> >>>
>> >>> On 03/30/2017 03:50 AM, Thomas Huth wrote:
>> >>>> When running certain HMP commands (like "device_del") via QMP, we
>> >>>> can sometimes get a QMP event in the response first, so that the
>> >>>> "g_assert(ret)" statement in qtest_hmp() triggers and the test
>> >>>> fails. Fix this by ignoring such QMP events while looking for the
>> >>>> real return value from QMP.
>> >>>>
>> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> >>>> ---
>> >>>> tests/libqtest.c | 6 ++++++
>> >>>> 1 file changed, 6 insertions(+)
>> >>>>
>> >>>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> >>>> index a5c3d2b..c9b2d76 100644
>> >>>> --- a/tests/libqtest.c
>> >>>> +++ b/tests/libqtest.c
>> >>>> @@ -580,6 +580,12 @@ char *qtest_hmpv(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 */
>> >>>> + QDECREF(resp);
>> >>>> + resp = qtest_qmp_receive(s);
>> >>>> + ret = g_strdup(qdict_get_try_str(resp, "return"));
>> >>>> + }
>> >>>> g_assert(ret);
>> >>>> QDECREF(resp);
>> >>>> g_free(cmd);
>> >>>>
>> >>>
>> >>> You've probably been asked this, but can you just shove the QMP response
>> >>> you don't want into the event queue for consumption by other calls?
>> >>
>> >> Well, this is the qtest_hmpv() function, so I assume that the caller
>> >> just wants to execute a HMP command and does not really care about QMP
>> >> events. If you care about QMP events, you should use the qmp functions
>> >> instead.
>> >>
>> >> Thomas
>> >>
>> >
>> > I don't think it's obvious that using HMP functions should cause the QMP
>> > stream to become faulty, though.
>>
>> qtest_hmpv() is a helper function. It tries to be convenient for the
>> common case. In particular, it receives, checks and consumes the QMP
>> reply. Before the patch, it screws up when QMP events arrive before the
>> reply. The patch fixes it by also consuming the events. Makes sense.
>>
>> The non-obviousness should be addressed in the function comment.
>>
>> > If someone uses an HMP function and then tries to wait on a QMP event to
>> > confirm that some key condition has occurred (pausing or resuming, for
>> > instance) it would not be immediately apparent from the user's POV that
>> > this function just eats replies because it was convenient to do so.
>>
>> Code that needs to see events can't use this helper function. It needs
>> to use more primitive functions instead.
>>
>> > I guess the event queue only exists in python though, so it's not as
>> > trivial as I was thinking it would be...
>>
>> More sophisticated handling of QMP events in libqtest is out of scope
>> for this patch. I'm not passing judgement on whether it would be
>> useful :)
>>
>> With the function comment updated to mention QMP events get consumed:
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> OK, I've added that comment to qtest_hmp and qtest_hmpv when I've
> just queued it.
> (and removed an extra 'v')
>
> /**
> - * qtest_hmpv:
> + * qtest_hmp:
> * @s: #QTestState instance to operate on.
> * @fmt...: HMP command to send to QEMU
> *
> * Send HMP command to QEMU via QMP's human-monitor-command.
> + * QMP events are discarded.
> *
> * Returns: the command's output. The caller should g_free() it.
> */
> @@ -149,6 +150,7 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...);
> * @ap: HMP command arguments
> *
> * Send HMP command to QEMU via QMP's human-monitor-command.
> + * QMP events are discarded.
> *
> * Returns: the command's output. The caller should g_free() it.
> */
>
> Dave
Works for me, thanks!
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands
2017-03-30 7:50 ` [Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands Thomas Huth
2017-04-24 11:23 ` Dr. David Alan Gilbert
@ 2017-04-28 1:56 ` Eric Blake
2017-06-27 2:55 ` Eric Blake
2 siblings, 0 replies; 16+ messages in thread
From: Eric Blake @ 2017-04-28 1:56 UTC (permalink / raw)
To: Thomas Huth, Dr. David Alan Gilbert, qemu-devel; +Cc: Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 799 bytes --]
On 03/30/2017 02:50 AM, Thomas Huth wrote:
> HMP commands do not get any automatic testing yet, so on certain
> QEMU machines, some HMP commands were causing crashes in the past.
> Thus we should test HMP commands in our test suite, too, to avoid
> that such problems creep in again in the future.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> tests/Makefile.include | 2 +
> tests/test-hmp.c | 161 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 163 insertions(+)
> create mode 100644 tests/test-hmp.c
We should also update tests/.gitignore to make sure tests/test-hmp
doesn't get accidentally committed.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands
2017-03-30 7:50 ` [Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands Thomas Huth
2017-04-24 11:23 ` Dr. David Alan Gilbert
2017-04-28 1:56 ` Eric Blake
@ 2017-06-27 2:55 ` Eric Blake
2017-06-29 11:48 ` Dr. David Alan Gilbert
2 siblings, 1 reply; 16+ messages in thread
From: Eric Blake @ 2017-06-27 2:55 UTC (permalink / raw)
To: Thomas Huth, Dr. David Alan Gilbert, qemu-devel; +Cc: Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]
On 03/30/2017 02:50 AM, Thomas Huth wrote:
> HMP commands do not get any automatic testing yet, so on certain
> QEMU machines, some HMP commands were causing crashes in the past.
> Thus we should test HMP commands in our test suite, too, to avoid
> that such problems creep in again in the future.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> +static const char *hmp_cmds[] = {
> + "boot_set ndc",
> +/* Run through the list of pre-defined commands */
> +static void test_commands(void)
> +{
> + char *response;
> + int i;
> +
> + for (i = 0; hmp_cmds[i] != NULL; i++) {
> + if (verbose) {
> + fprintf(stderr, "\t%s\n", hmp_cmds[i]);
> + }
> + response = hmp(hmp_cmds[i]);
I failed to notice this sooner, but hmp() is passing its first arg as a
format string through a printf family. If hmp_cmds[i] ever gets
modified to include something with a %, it will misbehave. Better is to
use hmp("%s", variable).
I've patched it locally as part of rebasing my work on avoiding dynamic
JSON format strings, if no one beats me to a fix (my series also adds
the gcc format attribute tag, so that the compiler catches any further
mismatches in hmp() format vs. arguments).
> + while (*info) {
> + /* Extract the info command, ignore parameters and description */
> + g_assert(strncmp(info, "info ", 5) == 0);
> + endp = strchr(&info[5], ' ');
> + g_assert(endp != NULL);
> + *endp = '\0';
> + /* Now run the info command */
> + if (verbose) {
> + fprintf(stderr, "\t%s\n", info);
> + }
> + resp = hmp(info);
Another instance.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands
2017-06-27 2:55 ` Eric Blake
@ 2017-06-29 11:48 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 16+ messages in thread
From: Dr. David Alan Gilbert @ 2017-06-29 11:48 UTC (permalink / raw)
To: Eric Blake; +Cc: Thomas Huth, qemu-devel, Markus Armbruster
* Eric Blake (eblake@redhat.com) wrote:
> On 03/30/2017 02:50 AM, Thomas Huth wrote:
> > HMP commands do not get any automatic testing yet, so on certain
> > QEMU machines, some HMP commands were causing crashes in the past.
> > Thus we should test HMP commands in our test suite, too, to avoid
> > that such problems creep in again in the future.
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
>
> > +static const char *hmp_cmds[] = {
> > + "boot_set ndc",
>
> > +/* Run through the list of pre-defined commands */
> > +static void test_commands(void)
> > +{
> > + char *response;
> > + int i;
> > +
> > + for (i = 0; hmp_cmds[i] != NULL; i++) {
> > + if (verbose) {
> > + fprintf(stderr, "\t%s\n", hmp_cmds[i]);
> > + }
> > + response = hmp(hmp_cmds[i]);
>
> I failed to notice this sooner, but hmp() is passing its first arg as a
> format string through a printf family. If hmp_cmds[i] ever gets
> modified to include something with a %, it will misbehave. Better is to
> use hmp("%s", variable).
>
> I've patched it locally as part of rebasing my work on avoiding dynamic
> JSON format strings, if no one beats me to a fix (my series also adds
> the gcc format attribute tag, so that the compiler catches any further
> mismatches in hmp() format vs. arguments).
Ah yes, good spot. Please include that fix.
Dave
> > + while (*info) {
> > + /* Extract the info command, ignore parameters and description */
> > + g_assert(strncmp(info, "info ", 5) == 0);
> > + endp = strchr(&info[5], ' ');
> > + g_assert(endp != NULL);
> > + *endp = '\0';
> > + /* Now run the info command */
> > + if (verbose) {
> > + fprintf(stderr, "\t%s\n", info);
> > + }
> > + resp = hmp(info);
>
> Another instance.
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3266
> Virtualization: qemu.org | libvirt.org
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-06-29 11:48 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-30 7:50 [Qemu-devel] [PATCH v2 0/3] Add a tester for HMP commands Thomas Huth
2017-03-30 7:50 ` [Qemu-devel] [PATCH v2 1/3] libqtest: Ignore QMP events when parsing the response " Thomas Huth
2017-04-03 19:09 ` John Snow
2017-04-04 7:31 ` Thomas Huth
2017-04-04 14:33 ` John Snow
2017-04-07 17:58 ` Dr. David Alan Gilbert
2017-04-11 9:12 ` Markus Armbruster
2017-04-24 11:31 ` Dr. David Alan Gilbert
2017-04-24 12:23 ` Markus Armbruster
2017-03-30 7:50 ` [Qemu-devel] [PATCH v2 2/3] libqtest: Add a generic function to run a callback function for every machine Thomas Huth
2017-04-03 19:14 ` Philippe Mathieu-Daudé
2017-03-30 7:50 ` [Qemu-devel] [PATCH v2 3/3] tests: Add a tester for HMP commands Thomas Huth
2017-04-24 11:23 ` Dr. David Alan Gilbert
2017-04-28 1:56 ` Eric Blake
2017-06-27 2:55 ` Eric Blake
2017-06-29 11:48 ` Dr. David Alan Gilbert
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.