| Message ID | 1442577640-11612-4-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/18/2015 06:00 AM, Markus Armbruster wrote: > The test doesn't check the output makes any sense, only that QEMU Reads slightly better as: s/check/check that/ > survives. Useful since we've had an astounding number of crash bugs > around there. > > In fact, we have a bunch of them right now: several devices crash or > hang, and all CPUs leave a dangling pointer behind. Blacklist them in > the test. The next commits will fix. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > tests/Makefile | 11 ++- > tests/device-introspect-test.c | 149 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 157 insertions(+), 3 deletions(-) > create mode 100644 tests/device-introspect-test.c > > @@ -478,8 +483,8 @@ $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y) > $(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \ > QTEST_QEMU_IMG=qemu-img$(EXESUF) \ > MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \ > - gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y),"GTESTER $@") > - $(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \ > + gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-generic-y) $(check-qtest-$*-y),"GTESTER $@") Long line; worth adding a \ line-wrap? > +++ b/tests/device-introspect-test.c > @@ -0,0 +1,149 @@ > +/* > + * Covers QMP device-list-properties and HMP device_add help. We > + * currently don't check their output makes sense, only that QEMU again, might be better as: s/check/check that/ > +static void test_one_device(const char *type) > +{ > + QDict *resp; > + char *help; > + > + /* FIXME device-list-properties crashes for abstract device, skip */ > + if (strcmp(type, "device")) { > + resp = qmp("{'execute': 'device-list-properties'," > + " 'arguments': {'typename': %s}}", > + type); > + QDECREF(resp); > + } > + > + help = hmp("device_add \"%s,help\"", type); > + g_free(help); The comment mentions a skip, and the commit message header mentioned a blacklist, but I'm not seeing a skip here. Am I missing something? > + > +static bool blacklisted(const char *type) > +{ > + static const char *blacklist[] = { > + /* crash in object_unref(): */ > + "pxa2xx-pcmcia", > + /* hang in object_unref(): */ > + "realview_pci", "versatile_pci", "s390-sclp-event-facility", "sclp", > + /* create a CPU, thus use after free (see below): */ > + "allwinner-a10", "digic", "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", > + }; Okay, here's the blacklist the commit message mentioned, but maybe that means the earlier comment is stale?
Eric Blake <eblake@redhat.com> writes: > On 09/18/2015 06:00 AM, Markus Armbruster wrote: >> The test doesn't check the output makes any sense, only that QEMU > > Reads slightly better as: > s/check/check that/ Okay. >> survives. Useful since we've had an astounding number of crash bugs >> around there. >> >> In fact, we have a bunch of them right now: several devices crash or >> hang, and all CPUs leave a dangling pointer behind. Blacklist them in >> the test. The next commits will fix. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> tests/Makefile | 11 ++- >> tests/device-introspect-test.c | 149 +++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 157 insertions(+), 3 deletions(-) >> create mode 100644 tests/device-introspect-test.c >> > >> @@ -478,8 +483,8 @@ $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y) >> $(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \ >> QTEST_QEMU_IMG=qemu-img$(EXESUF) \ >> MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \ >> - gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y),"GTESTER $@") >> - $(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \ >> + gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-generic-y) $(check-qtest-$*-y),"GTESTER $@") > > Long line; worth adding a \ line-wrap? I'll look into it. >> +++ b/tests/device-introspect-test.c >> @@ -0,0 +1,149 @@ > >> +/* >> + * Covers QMP device-list-properties and HMP device_add help. We >> + * currently don't check their output makes sense, only that QEMU > > again, might be better as: > s/check/check that/ Okay. >> +static void test_one_device(const char *type) >> +{ >> + QDict *resp; >> + char *help; >> + >> + /* FIXME device-list-properties crashes for abstract device, skip */ >> + if (strcmp(type, "device")) { >> + resp = qmp("{'execute': 'device-list-properties'," >> + " 'arguments': {'typename': %s}}", >> + type); >> + QDECREF(resp); >> + } >> + >> + help = hmp("device_add \"%s,help\"", type); >> + g_free(help); > > The comment mentions a skip, and the commit message header mentioned a > blacklist, but I'm not seeing a skip here. Am I missing something? The conditional skips part of the test when type is "device", because that part crashes. >> + >> +static bool blacklisted(const char *type) >> +{ >> + static const char *blacklist[] = { >> + /* crash in object_unref(): */ >> + "pxa2xx-pcmcia", >> + /* hang in object_unref(): */ >> + "realview_pci", "versatile_pci", "s390-sclp-event-facility", "sclp", >> + /* create a CPU, thus use after free (see below): */ >> + "allwinner-a10", "digic", "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", >> + }; > > Okay, here's the blacklist the commit message mentioned, but maybe that > means the earlier comment is stale? The commit message is slightly inaccurate: unlike the other devices, "device" isn't blacklisted completely. Good enough?
diff --git a/tests/Makefile b/tests/Makefile index 7c6025a..4559045 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -86,6 +86,9 @@ check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh # All QTests for now are POSIX-only, but the dependencies are # really in libqtest, not in the testcases themselves. +check-qtest-generic-y += tests/device-introspect-test$(EXESUF) +gcov-files-generic-y += qdev-monitor.c qmp.c + gcov-files-ipack-y += hw/ipack/ipack.c check-qtest-ipack-y += tests/ipoctal232-test$(EXESUF) gcov-files-ipack-y += hw/char/ipoctal232.c @@ -375,6 +378,7 @@ libqos-imx-obj-y = $(libqos-obj-y) tests/libqos/i2c-imx.o libqos-usb-obj-y = $(libqos-pc-obj-y) tests/libqos/usb.o libqos-virtio-obj-y = $(libqos-pc-obj-y) tests/libqos/virtio.o tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o tests/libqos/malloc-generic.o +tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o tests/rtc-test$(EXESUF): tests/rtc-test.o tests/m48t59-test$(EXESUF): tests/m48t59-test.o tests/endianness-test$(EXESUF): tests/endianness-test.o @@ -440,7 +444,8 @@ CFLAGS += $(TEST_CFLAGS) TARGETS=$(patsubst %-softmmu,%, $(filter %-softmmu,$(TARGET_DIRS))) ifeq ($(CONFIG_POSIX),y) QTEST_TARGETS=$(foreach TARGET,$(TARGETS), $(if $(check-qtest-$(TARGET)-y), $(TARGET),)) -check-qtest-y=$(foreach TARGET,$(TARGETS), $(check-qtest-$(TARGET)-y)) +check-qtest-y=$(check-qtest-generic-y) +check-qtest-y+=$(foreach TARGET,$(TARGETS), $(check-qtest-$(TARGET)-y)) endif qtest-obj-y = tests/libqtest.o $(test-util-obj-y) @@ -478,8 +483,8 @@ $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y) $(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \ QTEST_QEMU_IMG=qemu-img$(EXESUF) \ MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \ - gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y),"GTESTER $@") - $(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \ + gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-generic-y) $(check-qtest-$*-y),"GTESTER $@") + $(if $(CONFIG_GCOV),@for f in $(gcov-files-generic-y) $(gcov-files-$*-y); do \ echo Gcov report for $$f:;\ $(GCOV) $(GCOV_OPTIONS) $$f -o `dirname $$f`; \ done,) diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c new file mode 100644 index 0000000..bcaaf2a --- /dev/null +++ b/tests/device-introspect-test.c @@ -0,0 +1,149 @@ +/* + * Device introspection test cases + * + * Copyright (c) 2015 Red Hat Inc. + * + * Authors: + * Markus Armbruster <armbru@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. + */ + +/* + * Covers QMP device-list-properties and HMP device_add help. We + * currently don't check their output makes sense, only that QEMU + * survives. Useful since we've had an astounding number of crash + * bugs around here. + */ + +#include <glib.h> +#include <stdarg.h> +#include "qemu-common.h" +#include "qapi/qmp/qstring.h" +#include "libqtest.h" + +const char common_args[] = "-nodefaults -machine none"; + +static QList *device_type_list(bool abstract) +{ + QDict *resp; + QList *ret; + + resp = qmp("{'execute': 'qom-list-types'," + " 'arguments': {'implements': 'device', 'abstract': %i}}", + abstract); + g_assert(qdict_haskey(resp, "return")); + ret = qdict_get_qlist(resp, "return"); + QINCREF(ret); + QDECREF(resp); + return ret; +} + +static void test_one_device(const char *type) +{ + QDict *resp; + char *help; + + /* FIXME device-list-properties crashes for abstract device, skip */ + if (strcmp(type, "device")) { + resp = qmp("{'execute': 'device-list-properties'," + " 'arguments': {'typename': %s}}", + type); + QDECREF(resp); + } + + help = hmp("device_add \"%s,help\"", type); + g_free(help); +} + +static void test_device_intro_list(void) +{ + QList *types; + char *help; + + qtest_start(common_args); + + types = device_type_list(true); + QDECREF(types); + + help = hmp("device_add help"); + g_free(help); + + qtest_end(); +} + +static void test_device_intro_none(void) +{ + qtest_start(common_args); + test_one_device("nonexistent"); + qtest_end(); +} + +static void test_device_intro_abstract(void) +{ + qtest_start(common_args); + test_one_device("device"); + qtest_end(); +} + +static bool blacklisted(const char *type) +{ + static const char *blacklist[] = { + /* crash in object_unref(): */ + "pxa2xx-pcmcia", + /* hang in object_unref(): */ + "realview_pci", "versatile_pci", "s390-sclp-event-facility", "sclp", + /* create a CPU, thus use after free (see below): */ + "allwinner-a10", "digic", "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", + }; + size_t len = strlen(type); + int i; + + if (len >= 4 && !strcmp(type + len - 4, "-cpu")) { + /* use after free: cpu_exec_init() saves CPUState in cpus */ + return true; + } + + for (i = 0; i < ARRAY_SIZE(blacklist); i++) { + if (!strcmp(blacklist[i], type)) { + return true; + } + } + return false; +} + +static void test_device_intro_concrete(void) +{ + QList *types; + QListEntry *entry; + const char *type; + + qtest_start(common_args); + types = device_type_list(false); + + QLIST_FOREACH_ENTRY(types, entry) { + type = qdict_get_try_str(qobject_to_qdict(qlist_entry_obj(entry)), + "name"); + g_assert(type); + if (blacklisted(type)) { + continue; /* FIXME broken device, skip */ + } + test_one_device(type); + } + + QDECREF(types); + qtest_end(); +} + +int main(int argc, char **argv) +{ + g_test_init(&argc, &argv, NULL); + + qtest_add_func("device/introspect/list", test_device_intro_list); + qtest_add_func("device/introspect/none", test_device_intro_none); + qtest_add_func("device/introspect/abstract", test_device_intro_abstract); + qtest_add_func("device/introspect/concrete", test_device_intro_concrete); + + return g_test_run(); +}
The test doesn't check the output makes any sense, only that QEMU survives. Useful since we've had an astounding number of crash bugs around there. In fact, we have a bunch of them right now: several devices crash or hang, and all CPUs leave a dangling pointer behind. Blacklist them in the test. The next commits will fix. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- tests/Makefile | 11 ++- tests/device-introspect-test.c | 149 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+), 3 deletions(-) create mode 100644 tests/device-introspect-test.c