* [Qemu-devel] [PATCH 1/7] libqtest: Clean up unused QTestState member sigact_old
2015-09-18 12:00 [Qemu-devel] [PATCH 0/7] Fix device introspection regressions Markus Armbruster
@ 2015-09-18 12:00 ` Markus Armbruster
2015-09-18 15:36 ` Eric Blake
2015-09-18 12:00 ` [Qemu-devel] [PATCH 2/7] libqtest: New hmp() & friends Markus Armbruster
` (5 subsequent siblings)
6 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2015-09-18 12:00 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber, stefanha, ehabkost
Unused since commit d766825.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/libqtest.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index e5188e0..8dede56 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -46,7 +46,6 @@ struct QTestState
bool irq_level[MAX_IRQ];
GString *rx;
pid_t qemu_pid; /* our child QEMU process */
- struct sigaction sigact_old; /* restored on exit */
};
static GList *qtest_instances;
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] libqtest: Clean up unused QTestState member sigact_old
2015-09-18 12:00 ` [Qemu-devel] [PATCH 1/7] libqtest: Clean up unused QTestState member sigact_old Markus Armbruster
@ 2015-09-18 15:36 ` Eric Blake
0 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-09-18 15:36 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: afaerber, stefanha, ehabkost
[-- Attachment #1: Type: text/plain, Size: 769 bytes --]
On 09/18/2015 06:00 AM, Markus Armbruster wrote:
> Unused since commit d766825.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> tests/libqtest.c | 1 -
> 1 file changed, 1 deletion(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index e5188e0..8dede56 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -46,7 +46,6 @@ struct QTestState
> bool irq_level[MAX_IRQ];
> GString *rx;
> pid_t qemu_pid; /* our child QEMU process */
> - struct sigaction sigact_old; /* restored on exit */
> };
>
> static GList *qtest_instances;
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH 2/7] libqtest: New hmp() & friends
2015-09-18 12:00 [Qemu-devel] [PATCH 0/7] Fix device introspection regressions Markus Armbruster
2015-09-18 12:00 ` [Qemu-devel] [PATCH 1/7] libqtest: Clean up unused QTestState member sigact_old Markus Armbruster
@ 2015-09-18 12:00 ` Markus Armbruster
2015-09-18 15:47 ` Eric Blake
2015-09-18 12:00 ` [Qemu-devel] [PATCH 3/7] device-introspect-test: New, covering device introspection Markus Armbruster
` (4 subsequent siblings)
6 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2015-09-18 12:00 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber, stefanha, ehabkost
New convenience function hmp() to facilitate use of
human-monitor-command in tests. Use it to simplify its existing uses.
To blend into existing libqtest code, also add qtest_hmpv() and
qtest_hmp(). That, and the egregiously verbose GTK-Doc comment format
make this patch look bigger than it is.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/drive_del-test.c | 22 ++++++----------------
tests/ide-test.c | 8 ++------
tests/libqtest.c | 35 +++++++++++++++++++++++++++++++++++
tests/libqtest.h | 33 +++++++++++++++++++++++++++++++++
4 files changed, 76 insertions(+), 22 deletions(-)
diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index 8951f6f..3390946 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -16,28 +16,18 @@
static void drive_add(void)
{
- QDict *response;
+ char *resp = hmp("drive_add 0 if=none,id=drive0");
- response = qmp("{'execute': 'human-monitor-command',"
- " 'arguments': {"
- " 'command-line': 'drive_add 0 if=none,id=drive0'"
- "}}");
- g_assert(response);
- g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "OK\r\n");
- QDECREF(response);
+ g_assert_cmpstr(resp, ==, "OK\r\n");
+ g_free(resp);
}
static void drive_del(void)
{
- QDict *response;
+ char *resp = hmp("drive_del drive0");
- response = qmp("{'execute': 'human-monitor-command',"
- " 'arguments': {"
- " 'command-line': 'drive_del drive0'"
- "}}");
- g_assert(response);
- g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "");
- QDECREF(response);
+ g_assert_cmpstr(resp, ==, "");
+ g_free(resp);
}
static void device_del(void)
diff --git a/tests/ide-test.c b/tests/ide-test.c
index 4a07e3a..ef0a473 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -488,9 +488,7 @@ static void test_flush(void)
tmp_path);
/* Delay the completion of the flush request until we explicitly do it */
- qmp_discard_response("{'execute':'human-monitor-command', 'arguments': {"
- " 'command-line':"
- " 'qemu-io ide0-hd0 \"break flush_to_os A\"'} }");
+ g_free(hmp("qemu-io ide0-hd0 \"break flush_to_os A\""));
/* FLUSH CACHE command on device 0*/
outb(IDE_BASE + reg_device, 0);
@@ -502,9 +500,7 @@ static void test_flush(void)
assert_bit_clear(data, DF | ERR | DRQ);
/* Complete the command */
- qmp_discard_response("{'execute':'human-monitor-command', 'arguments': {"
- " 'command-line':"
- " 'qemu-io ide0-hd0 \"resume A\"'} }");
+ g_free(hmp("qemu-io ide0-hd0 \"resume A\""));
/* Check registers */
data = inb(IDE_BASE + reg_device);
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 8dede56..f98bde9 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -483,6 +483,33 @@ void qtest_qmp_eventwait(QTestState *s, const char *event)
}
}
+char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap)
+{
+ char *cmd;
+ QDict *resp;
+ char *ret;
+
+ cmd = g_strdup_vprintf(fmt, ap);
+ resp = qtest_qmp(s, "{'execute': 'human-monitor-command',"
+ " 'arguments': {'command-line': %s}}",
+ cmd);
+ ret = g_strdup(qdict_get_try_str(resp, "return"));
+ g_assert(ret);
+ QDECREF(resp);
+ g_free(cmd);
+ return ret;
+}
+
+char *qtest_hmp(QTestState *s, const char *fmt, ...)
+{
+ va_list ap;
+ char *ret;
+
+ va_start(ap, fmt);
+ ret = qtest_hmpv(s, fmt, ap);
+ va_end(ap);
+ return ret;
+}
const char *qtest_get_arch(void)
{
@@ -774,6 +801,14 @@ void qmp_discard_response(const char *fmt, ...)
qtest_qmpv_discard_response(global_qtest, fmt, ap);
va_end(ap);
}
+char *hmp(const char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+ return qtest_hmpv(global_qtest, fmt, ap);
+ va_end(ap);
+}
bool qtest_big_endian(void)
{
diff --git a/tests/libqtest.h b/tests/libqtest.h
index ec42031..b270f7b 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -120,6 +120,29 @@ QDict *qtest_qmp_receive(QTestState *s);
void qtest_qmp_eventwait(QTestState *s, const char *event);
/**
+ * qtest_hmpv:
+ * @s: #QTestState instance to operate on.
+ * @fmt...: HMP command to send to QEMU
+ *
+ * Send HMP command to QEMU via QMP's human-monitor-command.
+ *
+ * Returns: the command's output.
+ */
+char *qtest_hmp(QTestState *s, const char *fmt, ...);
+
+/**
+ * qtest_hmpv:
+ * @s: #QTestState instance to operate on.
+ * @fmt: HMP command to send to QEMU
+ * @ap: HMP command arguments
+ *
+ * Send HMP command to QEMU via QMP's human-monitor-command.
+ *
+ * Returns: the command's output.
+ */
+char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap);
+
+/**
* qtest_get_irq:
* @s: #QTestState instance to operate on.
* @num: Interrupt to observe.
@@ -499,6 +522,16 @@ static inline void qmp_eventwait(const char *event)
}
/**
+ * hmp:
+ * @fmt...: HMP command to send to QEMU
+ *
+ * Send HMP command to QEMU via QMP's human-monitor-command.
+ *
+ * Returns: the command's output.
+ */
+char *hmp(const char *fmt, ...);
+
+/**
* get_irq:
* @num: Interrupt to observe.
*
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] libqtest: New hmp() & friends
2015-09-18 12:00 ` [Qemu-devel] [PATCH 2/7] libqtest: New hmp() & friends Markus Armbruster
@ 2015-09-18 15:47 ` Eric Blake
2015-09-21 5:59 ` Markus Armbruster
0 siblings, 1 reply; 38+ messages in thread
From: Eric Blake @ 2015-09-18 15:47 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: afaerber, stefanha, ehabkost
[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]
On 09/18/2015 06:00 AM, Markus Armbruster wrote:
> New convenience function hmp() to facilitate use of
> human-monitor-command in tests. Use it to simplify its existing uses.
>
> To blend into existing libqtest code, also add qtest_hmpv() and
> qtest_hmp(). That, and the egregiously verbose GTK-Doc comment format
> make this patch look bigger than it is.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> tests/drive_del-test.c | 22 ++++++----------------
> tests/ide-test.c | 8 ++------
> tests/libqtest.c | 35 +++++++++++++++++++++++++++++++++++
> tests/libqtest.h | 33 +++++++++++++++++++++++++++++++++
> 4 files changed, 76 insertions(+), 22 deletions(-)
>
> @@ -774,6 +801,14 @@ void qmp_discard_response(const char *fmt, ...)
> qtest_qmpv_discard_response(global_qtest, fmt, ap);
> va_end(ap);
> }
> +char *hmp(const char *fmt, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, fmt);
> + return qtest_hmpv(global_qtest, fmt, ap);
> + va_end(ap);
Umm, that isn't quite what you meant :)
With the dead code after return fixed,
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] libqtest: New hmp() & friends
2015-09-18 15:47 ` Eric Blake
@ 2015-09-21 5:59 ` Markus Armbruster
0 siblings, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2015-09-21 5:59 UTC (permalink / raw)
To: Eric Blake; +Cc: ehabkost, qemu-devel, stefanha, afaerber
Eric Blake <eblake@redhat.com> writes:
> On 09/18/2015 06:00 AM, Markus Armbruster wrote:
>> New convenience function hmp() to facilitate use of
>> human-monitor-command in tests. Use it to simplify its existing uses.
>>
>> To blend into existing libqtest code, also add qtest_hmpv() and
>> qtest_hmp(). That, and the egregiously verbose GTK-Doc comment format
>> make this patch look bigger than it is.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> tests/drive_del-test.c | 22 ++++++----------------
>> tests/ide-test.c | 8 ++------
>> tests/libqtest.c | 35 +++++++++++++++++++++++++++++++++++
>> tests/libqtest.h | 33 +++++++++++++++++++++++++++++++++
>> 4 files changed, 76 insertions(+), 22 deletions(-)
>>
>
>
>> @@ -774,6 +801,14 @@ void qmp_discard_response(const char *fmt, ...)
>> qtest_qmpv_discard_response(global_qtest, fmt, ap);
>> va_end(ap);
>> }
>> +char *hmp(const char *fmt, ...)
>> +{
>> + va_list ap;
>> +
>> + va_start(ap, fmt);
>> + return qtest_hmpv(global_qtest, fmt, ap);
>> + va_end(ap);
>
> Umm, that isn't quite what you meant :)
D'oh!
> With the dead code after return fixed,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH 3/7] device-introspect-test: New, covering device introspection
2015-09-18 12:00 [Qemu-devel] [PATCH 0/7] Fix device introspection regressions Markus Armbruster
2015-09-18 12:00 ` [Qemu-devel] [PATCH 1/7] libqtest: Clean up unused QTestState member sigact_old Markus Armbruster
2015-09-18 12:00 ` [Qemu-devel] [PATCH 2/7] libqtest: New hmp() & friends Markus Armbruster
@ 2015-09-18 12:00 ` Markus Armbruster
2015-09-18 15:55 ` Eric Blake
2015-09-18 12:00 ` [Qemu-devel] [PATCH 4/7] qmp: Fix device-list-properties not to crash for abstract device Markus Armbruster
` (3 subsequent siblings)
6 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2015-09-18 12:00 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber, stefanha, ehabkost
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
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();
+}
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] device-introspect-test: New, covering device introspection
2015-09-18 12:00 ` [Qemu-devel] [PATCH 3/7] device-introspect-test: New, covering device introspection Markus Armbruster
@ 2015-09-18 15:55 ` Eric Blake
2015-09-21 6:05 ` Markus Armbruster
0 siblings, 1 reply; 38+ messages in thread
From: Eric Blake @ 2015-09-18 15:55 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: afaerber, stefanha, ehabkost
[-- Attachment #1: Type: text/plain, Size: 2897 bytes --]
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 +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] device-introspect-test: New, covering device introspection
2015-09-18 15:55 ` Eric Blake
@ 2015-09-21 6:05 ` Markus Armbruster
0 siblings, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2015-09-21 6:05 UTC (permalink / raw)
To: Eric Blake; +Cc: ehabkost, qemu-devel, stefanha, afaerber
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?
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH 4/7] qmp: Fix device-list-properties not to crash for abstract device
2015-09-18 12:00 [Qemu-devel] [PATCH 0/7] Fix device introspection regressions Markus Armbruster
` (2 preceding siblings ...)
2015-09-18 12:00 ` [Qemu-devel] [PATCH 3/7] device-introspect-test: New, covering device introspection Markus Armbruster
@ 2015-09-18 12:00 ` Markus Armbruster
2015-09-18 15:58 ` Eric Blake
2015-09-18 12:00 ` [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices Markus Armbruster
` (2 subsequent siblings)
6 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2015-09-18 12:00 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-stable, afaerber, stefanha, ehabkost
Broken in commit f4eb32b "qmp: show QOM properties in
device-list-properties", v2.1.
Cc: qemu-stable@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qmp.c | 6 ++++++
tests/device-introspect-test.c | 11 ++++-------
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/qmp.c b/qmp.c
index 9623c80..6f370d5 100644
--- a/qmp.c
+++ b/qmp.c
@@ -520,6 +520,12 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
return NULL;
}
+ if (object_class_is_abstract(klass)) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "name",
+ "non-abstract device type");
+ return NULL;
+ }
+
obj = object_new(typename);
QTAILQ_FOREACH(prop, &obj->properties, node) {
diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
index bcaaf2a..3e40877 100644
--- a/tests/device-introspect-test.c
+++ b/tests/device-introspect-test.c
@@ -45,13 +45,10 @@ 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);
- }
+ resp = qmp("{'execute': 'device-list-properties',"
+ " 'arguments': {'typename': %s}}",
+ type);
+ QDECREF(resp);
help = hmp("device_add \"%s,help\"", type);
g_free(help);
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] qmp: Fix device-list-properties not to crash for abstract device
2015-09-18 12:00 ` [Qemu-devel] [PATCH 4/7] qmp: Fix device-list-properties not to crash for abstract device Markus Armbruster
@ 2015-09-18 15:58 ` Eric Blake
0 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-09-18 15:58 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: ehabkost, qemu-stable, stefanha, afaerber
[-- Attachment #1: Type: text/plain, Size: 1159 bytes --]
On 09/18/2015 06:00 AM, Markus Armbruster wrote:
> Broken in commit f4eb32b "qmp: show QOM properties in
> device-list-properties", v2.1.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> qmp.c | 6 ++++++
> tests/device-introspect-test.c | 11 ++++-------
> 2 files changed, 10 insertions(+), 7 deletions(-)
>
> +++ b/tests/device-introspect-test.c
> @@ -45,13 +45,10 @@ 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")) {
Ah, this answers part of my confusion on 3/7. I was looking for so
condition on 'abstract', but the skip was based on a condition on
'device' (which happens to be abstract). Don't know if a different
wording on that comment would help; maybe:
FIXME device-list-properties crashes for device, because it is abstract;
skip
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-18 12:00 [Qemu-devel] [PATCH 0/7] Fix device introspection regressions Markus Armbruster
` (3 preceding siblings ...)
2015-09-18 12:00 ` [Qemu-devel] [PATCH 4/7] qmp: Fix device-list-properties not to crash for abstract device Markus Armbruster
@ 2015-09-18 12:00 ` Markus Armbruster
2015-09-18 12:38 ` Christian Borntraeger
` (3 more replies)
2015-09-18 12:00 ` [Qemu-devel] [PATCH 6/7] Revert "qdev: Use qdev_get_device_class() for -device <type>, help" Markus Armbruster
2015-09-18 12:00 ` [Qemu-devel] [PATCH 7/7] tests: Simplify how qom-test is run Markus Armbruster
6 siblings, 4 replies; 38+ messages in thread
From: Markus Armbruster @ 2015-09-18 12:00 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, ehabkost, Peter Crosthwaite, qemu-stable,
Alexander Graf, Alistair Francis, Christian Borntraeger, qemu-ppc,
Antony Pavlov, stefanha, Cornelia Huck, Paolo Bonzini, afaerber,
Li Guang, Richard Henderson
Several devices don't survive object_unref(object_new(T)): they crash
or hang during cleanup, or they leave dangling pointers behind.
This breaks at least device-list-properties, because
qmp_device_list_properties() needs to create a device to find its
properties. Broken in commit f4eb32b "qmp: show QOM properties in
device-list-properties", v2.1. Example reproducer:
$ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
{ "execute": "qmp_capabilities" }
{"return": {}}
{ "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
Aborted (core dumped)
[Exit 134 (SIGABRT)]
Unfortunately, I can't fix the problems in these devices right now.
Instead, add DeviceClass member cannot_even_create_with_object_new_yet
to mark them:
* Crash or hang during cleanup (didn't debug them, so I can't say
why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
"s390-sclp-event-facility", "sclp"
* Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
"fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
* Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
"host-powerpc64-cpu", "host-embedded-powerpc-cpu",
"host-powerpc-cpu"
Make qmp_device_list_properties() fail cleanly when the device is so
marked. This improves device-list-properties from "crashes or hangs"
to "fails". Not a complete fix, just a better-than-nothing
work-around. In the above reproducer, device-list-properties now
fails with "Can't list properties of device 'pxa2xx-pcmcia'".
This also protects -device FOO,help, which uses the same machinery
since commit ef52358 "qdev-monitor: include QOM properties in -device
FOO, help output", v2.2. Example reproducer:
$ qemu-system-* -machine none -device pxa2xx-pcmcia,help
Before:
qemu-system-aarch64: .../memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
After:
Can't list properties of device 'pxa2xx-pcmcia'
Cc: "Andreas Färber" <afaerber@suse.de>
Cc: Alexander Graf <agraf@suse.de>
Cc: Alistair Francis <alistair.francis@xilinx.com>
Cc: Antony Pavlov <antonynpavlov@gmail.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Li Guang <lig.fnst@cn.fujitsu.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Richard Henderson <rth@twiddle.net>
Cc: qemu-ppc@nongnu.org
Cc: qemu-stable@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/arm/allwinner-a10.c | 2 ++
hw/arm/digic.c | 2 ++
hw/arm/fsl-imx25.c | 2 ++
hw/arm/fsl-imx31.c | 2 ++
hw/arm/xlnx-zynqmp.c | 2 ++
hw/pci-host/versatile.c | 11 +++++++++++
hw/pcmcia/pxa2xx.c | 9 +++++++++
hw/s390x/event-facility.c | 3 +++
hw/s390x/sclp.c | 3 +++
include/hw/qdev-core.h | 13 +++++++++++++
qmp.c | 5 +++++
qom/cpu.c | 2 ++
target-i386/cpu.c | 2 ++
target-ppc/kvm.c | 4 ++++
tests/device-introspect-test.c | 29 -----------------------------
15 files changed, 62 insertions(+), 29 deletions(-)
diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
index ff249af..7692090 100644
--- a/hw/arm/allwinner-a10.c
+++ b/hw/arm/allwinner-a10.c
@@ -103,6 +103,8 @@ static void aw_a10_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = aw_a10_realize;
+ /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static const TypeInfo aw_a10_type_info = {
diff --git a/hw/arm/digic.c b/hw/arm/digic.c
index ec8c330..3decef4 100644
--- a/hw/arm/digic.c
+++ b/hw/arm/digic.c
@@ -97,6 +97,8 @@ static void digic_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = digic_realize;
+ /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static const TypeInfo digic_type_info = {
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 86fde42..13c06b2 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -284,6 +284,8 @@ static void fsl_imx25_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = fsl_imx25_realize;
+ /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static const TypeInfo fsl_imx25_type_info = {
diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
index 8e1ed48..7cb8fd4 100644
--- a/hw/arm/fsl-imx31.c
+++ b/hw/arm/fsl-imx31.c
@@ -258,6 +258,8 @@ static void fsl_imx31_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = fsl_imx31_realize;
+ /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static const TypeInfo fsl_imx31_type_info = {
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 2185542..d558b10 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -271,6 +271,8 @@ static void xlnx_zynqmp_class_init(ObjectClass *oc, void *data)
dc->props = xlnx_zynqmp_props;
dc->realize = xlnx_zynqmp_realize;
+ /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static const TypeInfo xlnx_zynqmp_type_info = {
diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index 6d23553..f28a115 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -500,6 +500,8 @@ static void pci_vpb_class_init(ObjectClass *klass, void *data)
dc->reset = pci_vpb_reset;
dc->vmsd = &pci_vpb_vmstate;
dc->props = pci_vpb_properties;
+ /* Reason: object_unref() hangs */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static const TypeInfo pci_vpb_info = {
@@ -521,10 +523,19 @@ static void pci_realview_init(Object *obj)
s->mem_win_size[2] = 0x08000000;
}
+static void pci_realview_class_init(ObjectClass *class, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(class);
+
+ /* Reason: object_unref() hangs */
+ dc->cannot_even_create_with_object_new_yet = true;
+}
+
static const TypeInfo pci_realview_info = {
.name = "realview_pci",
.parent = TYPE_VERSATILE_PCI,
.instance_init = pci_realview_init,
+ .class_init = pci_realview_class_init,
};
static void versatile_pci_register_types(void)
diff --git a/hw/pcmcia/pxa2xx.c b/hw/pcmcia/pxa2xx.c
index a7e1877..c050c41 100644
--- a/hw/pcmcia/pxa2xx.c
+++ b/hw/pcmcia/pxa2xx.c
@@ -249,11 +249,20 @@ void pxa2xx_pcmcia_set_irq_cb(void *opaque, qemu_irq irq, qemu_irq cd_irq)
s->cd_irq = cd_irq;
}
+static void pxa2xx_pcmcia_class_init(ObjectClass *class, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(class);
+
+ /* Reason: object_unref() crashes */
+ dc->cannot_even_create_with_object_new_yet = true;
+}
+
static const TypeInfo pxa2xx_pcmcia_type_info = {
.name = TYPE_PXA2XX_PCMCIA,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(PXA2xxPCMCIAState),
.instance_init = pxa2xx_pcmcia_initfn,
+ .class_init = pxa2xx_pcmcia_class_init,
};
static void pxa2xx_pcmcia_register_types(void)
diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index ef2a051..8fa361d 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -381,6 +381,9 @@ static void init_event_facility_class(ObjectClass *klass, void *data)
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
k->command_handler = command_handler;
k->event_pending = event_pending;
+
+ /* Reason: object_unref() hangs */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static const TypeInfo sclp_event_facility_info = {
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index fd277e1..b2b46c9 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -562,6 +562,9 @@ static void sclp_class_init(ObjectClass *oc, void *data)
sc->read_cpu_info = sclp_read_cpu_info;
sc->execute = sclp_execute;
sc->service_interrupt = service_interrupt;
+
+ /* Reason: object_unref() hangs */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static TypeInfo sclp_info = {
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 038b54d..bc30cca 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -114,6 +114,19 @@ typedef struct DeviceClass {
* TODO remove once we're there
*/
bool cannot_instantiate_with_device_add_yet;
+ /*
+ * Does this device model survive object_unref(object_new(TNAME))?
+ * All device models should, and this flag shouldn't exist. Some
+ * devices crash in object_new(), some crash or hang in
+ * object_unref(). Makes introspecting properties with
+ * qmp_device_list_properties() dangerous. Bad, because it's used
+ * by -device FOO,help. This flag serves to protect that code.
+ * It should never be set without a comment explaining why it is
+ * set.
+ * TODO remove once we're there
+ */
+ bool cannot_even_create_with_object_new_yet;
+
bool hotpluggable;
/* callbacks */
diff --git a/qmp.c b/qmp.c
index 6f370d5..257f09f 100644
--- a/qmp.c
+++ b/qmp.c
@@ -526,6 +526,11 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
return NULL;
}
+ if (DEVICE_CLASS(klass)->cannot_even_create_with_object_new_yet) {
+ error_setg(errp, "Can't list properties of device '%s'", typename);
+ return NULL;
+ }
+
obj = object_new(typename);
QTAILQ_FOREACH(prop, &obj->properties, node) {
diff --git a/qom/cpu.c b/qom/cpu.c
index fb80d13..5ff9ea7 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -361,6 +361,8 @@ static void cpu_class_init(ObjectClass *klass, void *data)
* IRQs, adding reset handlers, halting non-first CPUs, ...
*/
dc->cannot_instantiate_with_device_add_yet = true;
+ /* Reason: use after free: cpu_exec_init() saves CPUState in cpus */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static const TypeInfo cpu_type_info = {
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7c52714..32e7b84 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1449,6 +1449,8 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void *data)
*/
dc->props = host_x86_cpu_properties;
+ /* Reason: host_x86_cpu_initfn() dies when !kvm_enabled() */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static void host_x86_cpu_initfn(Object *obj)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 110436d..9943bba 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2188,6 +2188,7 @@ static void kvmppc_host_cpu_initfn(Object *obj)
static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
{
+ DeviceClass *dc = DEVICE_CLASS(oc);
PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
uint32_t vmx = kvmppc_get_vmx();
uint32_t dfp = kvmppc_get_dfp();
@@ -2214,6 +2215,9 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
if (icache_size != -1) {
pcc->l1_icache_size = icache_size;
}
+
+ /* Reason: kvmppc_host_cpu_initfn() dies when !kvm_enabled() */
+ dc->cannot_even_create_with_object_new_yet = true;
}
bool kvmppc_has_cap_epr(void)
diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
index 3e40877..ca82f0c 100644
--- a/tests/device-introspect-test.c
+++ b/tests/device-introspect-test.c
@@ -84,32 +84,6 @@ static void test_device_intro_abstract(void)
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;
@@ -123,9 +97,6 @@ static void test_device_intro_concrete(void)
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);
}
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-18 12:00 ` [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices Markus Armbruster
@ 2015-09-18 12:38 ` Christian Borntraeger
2015-09-21 8:30 ` David Hildenbrand
2015-09-18 16:09 ` Eric Blake
` (2 subsequent siblings)
3 siblings, 1 reply; 38+ messages in thread
From: Christian Borntraeger @ 2015-09-18 12:38 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel, David Hildenbrand
Cc: Peter Maydell, ehabkost, Peter Crosthwaite, qemu-stable,
Alexander Graf, Alistair Francis, qemu-ppc, Antony Pavlov,
stefanha, Cornelia Huck, Paolo Bonzini, afaerber, Li Guang,
Richard Henderson
Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
> Several devices don't survive object_unref(object_new(T)): they crash
> or hang during cleanup, or they leave dangling pointers behind.
>
> This breaks at least device-list-properties, because
> qmp_device_list_properties() needs to create a device to find its
> properties. Broken in commit f4eb32b "qmp: show QOM properties in
> device-list-properties", v2.1. Example reproducer:
>
> $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
> { "execute": "qmp_capabilities" }
> {"return": {}}
> { "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
> qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
> Aborted (core dumped)
> [Exit 134 (SIGABRT)]
>
> Unfortunately, I can't fix the problems in these devices right now.
> Instead, add DeviceClass member cannot_even_create_with_object_new_yet
> to mark them:
>
> * Crash or hang during cleanup (didn't debug them, so I can't say
> why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
> "s390-sclp-event-facility", "sclp"
Ack for the sclp things. Theses devices are created by the machine and
sclp creates the event-facility, so not having a way to query properties
for these devices is better than a hang.
David, can you have a look on why these devices fail as outlined?
>
> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
> "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
>
> * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
> "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
> "host-powerpc-cpu"
>
> Make qmp_device_list_properties() fail cleanly when the device is so
> marked. This improves device-list-properties from "crashes or hangs"
> to "fails". Not a complete fix, just a better-than-nothing
> work-around. In the above reproducer, device-list-properties now
> fails with "Can't list properties of device 'pxa2xx-pcmcia'".
>
> This also protects -device FOO,help, which uses the same machinery
> since commit ef52358 "qdev-monitor: include QOM properties in -device
> FOO, help output", v2.2. Example reproducer:
>
> $ qemu-system-* -machine none -device pxa2xx-pcmcia,help
>
> Before:
>
> qemu-system-aarch64: .../memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
>
> After:
>
> Can't list properties of device 'pxa2xx-pcmcia'
>
> Cc: "Andreas Färber" <afaerber@suse.de>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Alistair Francis <alistair.francis@xilinx.com>
> Cc: Antony Pavlov <antonynpavlov@gmail.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Li Guang <lig.fnst@cn.fujitsu.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: qemu-ppc@nongnu.org
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/arm/allwinner-a10.c | 2 ++
> hw/arm/digic.c | 2 ++
> hw/arm/fsl-imx25.c | 2 ++
> hw/arm/fsl-imx31.c | 2 ++
> hw/arm/xlnx-zynqmp.c | 2 ++
> hw/pci-host/versatile.c | 11 +++++++++++
> hw/pcmcia/pxa2xx.c | 9 +++++++++
> hw/s390x/event-facility.c | 3 +++
> hw/s390x/sclp.c | 3 +++
> include/hw/qdev-core.h | 13 +++++++++++++
> qmp.c | 5 +++++
> qom/cpu.c | 2 ++
> target-i386/cpu.c | 2 ++
> target-ppc/kvm.c | 4 ++++
> tests/device-introspect-test.c | 29 -----------------------------
> 15 files changed, 62 insertions(+), 29 deletions(-)
>
> diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
> index ff249af..7692090 100644
> --- a/hw/arm/allwinner-a10.c
> +++ b/hw/arm/allwinner-a10.c
> @@ -103,6 +103,8 @@ static void aw_a10_class_init(ObjectClass *oc, void *data)
> DeviceClass *dc = DEVICE_CLASS(oc);
>
> dc->realize = aw_a10_realize;
> + /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
> + dc->cannot_even_create_with_object_new_yet = true;
> }
>
> static const TypeInfo aw_a10_type_info = {
> diff --git a/hw/arm/digic.c b/hw/arm/digic.c
> index ec8c330..3decef4 100644
> --- a/hw/arm/digic.c
> +++ b/hw/arm/digic.c
> @@ -97,6 +97,8 @@ static void digic_class_init(ObjectClass *oc, void *data)
> DeviceClass *dc = DEVICE_CLASS(oc);
>
> dc->realize = digic_realize;
> + /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
> + dc->cannot_even_create_with_object_new_yet = true;
> }
>
> static const TypeInfo digic_type_info = {
> diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
> index 86fde42..13c06b2 100644
> --- a/hw/arm/fsl-imx25.c
> +++ b/hw/arm/fsl-imx25.c
> @@ -284,6 +284,8 @@ static void fsl_imx25_class_init(ObjectClass *oc, void *data)
> DeviceClass *dc = DEVICE_CLASS(oc);
>
> dc->realize = fsl_imx25_realize;
> + /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
> + dc->cannot_even_create_with_object_new_yet = true;
> }
>
> static const TypeInfo fsl_imx25_type_info = {
> diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
> index 8e1ed48..7cb8fd4 100644
> --- a/hw/arm/fsl-imx31.c
> +++ b/hw/arm/fsl-imx31.c
> @@ -258,6 +258,8 @@ static void fsl_imx31_class_init(ObjectClass *oc, void *data)
> DeviceClass *dc = DEVICE_CLASS(oc);
>
> dc->realize = fsl_imx31_realize;
> + /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
> + dc->cannot_even_create_with_object_new_yet = true;
> }
>
> static const TypeInfo fsl_imx31_type_info = {
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 2185542..d558b10 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -271,6 +271,8 @@ static void xlnx_zynqmp_class_init(ObjectClass *oc, void *data)
>
> dc->props = xlnx_zynqmp_props;
> dc->realize = xlnx_zynqmp_realize;
> + /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
> + dc->cannot_even_create_with_object_new_yet = true;
> }
>
> static const TypeInfo xlnx_zynqmp_type_info = {
> diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
> index 6d23553..f28a115 100644
> --- a/hw/pci-host/versatile.c
> +++ b/hw/pci-host/versatile.c
> @@ -500,6 +500,8 @@ static void pci_vpb_class_init(ObjectClass *klass, void *data)
> dc->reset = pci_vpb_reset;
> dc->vmsd = &pci_vpb_vmstate;
> dc->props = pci_vpb_properties;
> + /* Reason: object_unref() hangs */
> + dc->cannot_even_create_with_object_new_yet = true;
> }
>
> static const TypeInfo pci_vpb_info = {
> @@ -521,10 +523,19 @@ static void pci_realview_init(Object *obj)
> s->mem_win_size[2] = 0x08000000;
> }
>
> +static void pci_realview_class_init(ObjectClass *class, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(class);
> +
> + /* Reason: object_unref() hangs */
> + dc->cannot_even_create_with_object_new_yet = true;
> +}
> +
> static const TypeInfo pci_realview_info = {
> .name = "realview_pci",
> .parent = TYPE_VERSATILE_PCI,
> .instance_init = pci_realview_init,
> + .class_init = pci_realview_class_init,
> };
>
> static void versatile_pci_register_types(void)
> diff --git a/hw/pcmcia/pxa2xx.c b/hw/pcmcia/pxa2xx.c
> index a7e1877..c050c41 100644
> --- a/hw/pcmcia/pxa2xx.c
> +++ b/hw/pcmcia/pxa2xx.c
> @@ -249,11 +249,20 @@ void pxa2xx_pcmcia_set_irq_cb(void *opaque, qemu_irq irq, qemu_irq cd_irq)
> s->cd_irq = cd_irq;
> }
>
> +static void pxa2xx_pcmcia_class_init(ObjectClass *class, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(class);
> +
> + /* Reason: object_unref() crashes */
> + dc->cannot_even_create_with_object_new_yet = true;
> +}
> +
> static const TypeInfo pxa2xx_pcmcia_type_info = {
> .name = TYPE_PXA2XX_PCMCIA,
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(PXA2xxPCMCIAState),
> .instance_init = pxa2xx_pcmcia_initfn,
> + .class_init = pxa2xx_pcmcia_class_init,
> };
>
> static void pxa2xx_pcmcia_register_types(void)
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index ef2a051..8fa361d 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -381,6 +381,9 @@ static void init_event_facility_class(ObjectClass *klass, void *data)
> set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> k->command_handler = command_handler;
> k->event_pending = event_pending;
> +
> + /* Reason: object_unref() hangs */
> + dc->cannot_even_create_with_object_new_yet = true;
> }
>
> static const TypeInfo sclp_event_facility_info = {
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index fd277e1..b2b46c9 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -562,6 +562,9 @@ static void sclp_class_init(ObjectClass *oc, void *data)
> sc->read_cpu_info = sclp_read_cpu_info;
> sc->execute = sclp_execute;
> sc->service_interrupt = service_interrupt;
> +
> + /* Reason: object_unref() hangs */
> + dc->cannot_even_create_with_object_new_yet = true;
> }
>
> static TypeInfo sclp_info = {
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 038b54d..bc30cca 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -114,6 +114,19 @@ typedef struct DeviceClass {
> * TODO remove once we're there
> */
> bool cannot_instantiate_with_device_add_yet;
> + /*
> + * Does this device model survive object_unref(object_new(TNAME))?
> + * All device models should, and this flag shouldn't exist. Some
> + * devices crash in object_new(), some crash or hang in
> + * object_unref(). Makes introspecting properties with
> + * qmp_device_list_properties() dangerous. Bad, because it's used
> + * by -device FOO,help. This flag serves to protect that code.
> + * It should never be set without a comment explaining why it is
> + * set.
> + * TODO remove once we're there
> + */
> + bool cannot_even_create_with_object_new_yet;
> +
> bool hotpluggable;
>
> /* callbacks */
> diff --git a/qmp.c b/qmp.c
> index 6f370d5..257f09f 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -526,6 +526,11 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
> return NULL;
> }
>
> + if (DEVICE_CLASS(klass)->cannot_even_create_with_object_new_yet) {
> + error_setg(errp, "Can't list properties of device '%s'", typename);
> + return NULL;
> + }
> +
> obj = object_new(typename);
>
> QTAILQ_FOREACH(prop, &obj->properties, node) {
> diff --git a/qom/cpu.c b/qom/cpu.c
> index fb80d13..5ff9ea7 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -361,6 +361,8 @@ static void cpu_class_init(ObjectClass *klass, void *data)
> * IRQs, adding reset handlers, halting non-first CPUs, ...
> */
> dc->cannot_instantiate_with_device_add_yet = true;
> + /* Reason: use after free: cpu_exec_init() saves CPUState in cpus */
> + dc->cannot_even_create_with_object_new_yet = true;
> }
>
> static const TypeInfo cpu_type_info = {
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7c52714..32e7b84 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1449,6 +1449,8 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void *data)
> */
>
> dc->props = host_x86_cpu_properties;
> + /* Reason: host_x86_cpu_initfn() dies when !kvm_enabled() */
> + dc->cannot_even_create_with_object_new_yet = true;
> }
>
> static void host_x86_cpu_initfn(Object *obj)
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 110436d..9943bba 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -2188,6 +2188,7 @@ static void kvmppc_host_cpu_initfn(Object *obj)
>
> static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
> {
> + DeviceClass *dc = DEVICE_CLASS(oc);
> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> uint32_t vmx = kvmppc_get_vmx();
> uint32_t dfp = kvmppc_get_dfp();
> @@ -2214,6 +2215,9 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
> if (icache_size != -1) {
> pcc->l1_icache_size = icache_size;
> }
> +
> + /* Reason: kvmppc_host_cpu_initfn() dies when !kvm_enabled() */
> + dc->cannot_even_create_with_object_new_yet = true;
> }
>
> bool kvmppc_has_cap_epr(void)
> diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
> index 3e40877..ca82f0c 100644
> --- a/tests/device-introspect-test.c
> +++ b/tests/device-introspect-test.c
> @@ -84,32 +84,6 @@ static void test_device_intro_abstract(void)
> 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;
> @@ -123,9 +97,6 @@ static void test_device_intro_concrete(void)
> 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);
> }
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-18 12:38 ` Christian Borntraeger
@ 2015-09-21 8:30 ` David Hildenbrand
2015-09-21 15:38 ` Eduardo Habkost
0 siblings, 1 reply; 38+ messages in thread
From: David Hildenbrand @ 2015-09-21 8:30 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Peter Maydell, ehabkost, Peter Crosthwaite, qemu-devel,
qemu-stable, Alexander Graf, Markus Armbruster, qemu-ppc,
Antony Pavlov, stefanha, Cornelia Huck, Paolo Bonzini,
Alistair Francis, afaerber, Li Guang, Richard Henderson
> Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
> > Several devices don't survive object_unref(object_new(T)): they crash
> > or hang during cleanup, or they leave dangling pointers behind.
> >
> > This breaks at least device-list-properties, because
> > qmp_device_list_properties() needs to create a device to find its
> > properties. Broken in commit f4eb32b "qmp: show QOM properties in
> > device-list-properties", v2.1. Example reproducer:
> >
> > $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
> > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
> > { "execute": "qmp_capabilities" }
> > {"return": {}}
> > { "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
> > qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
> > Aborted (core dumped)
> > [Exit 134 (SIGABRT)]
> >
> > Unfortunately, I can't fix the problems in these devices right now.
> > Instead, add DeviceClass member cannot_even_create_with_object_new_yet
> > to mark them:
> >
> > * Crash or hang during cleanup (didn't debug them, so I can't say
> > why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
> > "s390-sclp-event-facility", "sclp"
>
> Ack for the sclp things. Theses devices are created by the machine and
> sclp creates the event-facility, so not having a way to query properties
> for these devices is better than a hang.
>
> David, can you have a look on why these devices fail as outlined?
>
The problem was that the quiesce and cpu hotplug sclp event devices had no
parent (onoly a parent bus). So when the bus (inside the event facility) was
removed, it looped forever trying remove/unparent it's "bus childs" (which had
no parent).
sclp (parent=machine)
-> even-facility (parent=sclp)
-> bus (parent=event-facility)
-> quiesce (parent=null)
-> cpu hotplug (parent=null)
Maybe that helps others struggling with the same symptoms.
Just a quick comment on the introspection:
I don't think it is a good idea to expose properties that way. Temporarily
creating devices for the sake of querying property names sounds very wrong to
me, because certain devices require certain knowledge about how and when they
can be created.
Feels a little like hacking an interface to a java program, which allows to
create any object of a special kind dynamically (constructor arguments?),
reading some member variable (names) via reflections and then throwing that
object away. Which sounds very wrong to me.
Wonder if it wouldn't make more sense to query only the static properties of a
device. I mean if the dynamic properties are dynamic by definition (and can
change during runtime). So looking up the static properties via the typename
feels more KIS-style to me. Of course, the relevant properties would have to be
defined statically then, which shouldn't be a problem.
Dynamic properties that are actually created could depend on almost
everything in the system - why fake something that we don't know.
David
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-21 8:30 ` David Hildenbrand
@ 2015-09-21 15:38 ` Eduardo Habkost
2015-09-22 8:02 ` David Hildenbrand
2015-09-22 8:07 ` Markus Armbruster
0 siblings, 2 replies; 38+ messages in thread
From: Eduardo Habkost @ 2015-09-21 15:38 UTC (permalink / raw)
To: David Hildenbrand
Cc: Peter Maydell, Peter Crosthwaite, qemu-devel, qemu-stable,
Alexander Graf, Markus Armbruster, Christian Borntraeger,
qemu-ppc, Antony Pavlov, stefanha, Cornelia Huck, Paolo Bonzini,
Alistair Francis, afaerber, Li Guang, Richard Henderson
On Mon, Sep 21, 2015 at 10:30:50AM +0200, David Hildenbrand wrote:
> > Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
> > > Several devices don't survive object_unref(object_new(T)): they crash
> > > or hang during cleanup, or they leave dangling pointers behind.
> > >
> > > This breaks at least device-list-properties, because
> > > qmp_device_list_properties() needs to create a device to find its
> > > properties. Broken in commit f4eb32b "qmp: show QOM properties in
> > > device-list-properties", v2.1. Example reproducer:
> > >
> > > $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
> > > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
> > > { "execute": "qmp_capabilities" }
> > > {"return": {}}
> > > { "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
> > > qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
> > > Aborted (core dumped)
> > > [Exit 134 (SIGABRT)]
> > >
> > > Unfortunately, I can't fix the problems in these devices right now.
> > > Instead, add DeviceClass member cannot_even_create_with_object_new_yet
> > > to mark them:
> > >
> > > * Crash or hang during cleanup (didn't debug them, so I can't say
> > > why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
> > > "s390-sclp-event-facility", "sclp"
> >
> > Ack for the sclp things. Theses devices are created by the machine and
> > sclp creates the event-facility, so not having a way to query properties
> > for these devices is better than a hang.
> >
> > David, can you have a look on why these devices fail as outlined?
> >
>
> The problem was that the quiesce and cpu hotplug sclp event devices had no
> parent (onoly a parent bus). So when the bus (inside the event facility) was
> removed, it looped forever trying remove/unparent it's "bus childs" (which had
> no parent).
>
> sclp (parent=machine)
> -> even-facility (parent=sclp)
> -> bus (parent=event-facility)
> -> quiesce (parent=null)
> -> cpu hotplug (parent=null)
>
> Maybe that helps others struggling with the same symptoms.
>
>
> Just a quick comment on the introspection:
>
> I don't think it is a good idea to expose properties that way. Temporarily
> creating devices for the sake of querying property names sounds very wrong to
> me, because certain devices require certain knowledge about how and when they
> can be created.
No knowledge should be required for object_new(). Classes' instance_init
functions should have no side-effects outside the object itself.
>
> Feels a little like hacking an interface to a java program, which allows to
> create any object of a special kind dynamically (constructor arguments?),
> reading some member variable (names) via reflections and then throwing that
> object away. Which sounds very wrong to me.
I wouldn't call it wrong. Confusing, maybe. We use a mix of class-based
and prototype-based approaches to inheritance and property
introspection.
>
> Wonder if it wouldn't make more sense to query only the static properties of a
> device. I mean if the dynamic properties are dynamic by definition (and can
> change during runtime). So looking up the static properties via the typename
> feels more KIS-style to me. Of course, the relevant properties would have to be
> defined statically then, which shouldn't be a problem.
It depends on your definition of "shouldn't be a problem". :)
The static property system is more limited than the QOM dynamic property
system, today. We already depend on introspection of dynamic properties
registered by instance_init functions, so we would need to move
everything to a better static property system if we want to stop doing
object_new() during class introspection without regressions.
>
> Dynamic properties that are actually created could depend on almost
> everything in the system - why fake something that we don't know.
Properties registered on instance_init shouldn't depend on anything else
on the system. Properties registered later in the object lifetime (e.g.
on realize) can.
--
Eduardo
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-21 15:38 ` Eduardo Habkost
@ 2015-09-22 8:02 ` David Hildenbrand
2015-09-22 8:07 ` Markus Armbruster
1 sibling, 0 replies; 38+ messages in thread
From: David Hildenbrand @ 2015-09-22 8:02 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Peter Maydell, Peter Crosthwaite, qemu-devel, qemu-stable,
Alexander Graf, Markus Armbruster, Christian Borntraeger,
qemu-ppc, Antony Pavlov, stefanha, Cornelia Huck, Paolo Bonzini,
Alistair Francis, afaerber, Li Guang, Richard Henderson
> No knowledge should be required for object_new(). Classes' instance_init
> functions should have no side-effects outside the object itself.
While this should theoretically be true, I can guarantee to you that this is
not the case for all devices :) (especially as there are too many unwritten
laws related to that whole qmp/qom thing flying around).
E.g. we can save ourselves from the creation of other devices (init + realize)
by setting a device to !hotpluggable in the device class. This code simply
bypasses such checks and assumes that QEMUs internal structure can deal with
that. We saw that this doesn't work this far.
Other interfaces (object_add) seem to rely on TYPE_USER_CREATABLE, so we can't
trigger "everything" from outside QEMU.
One can argue that we simply have to fix the devices - nevertheless I think this
is the wrong approach to the problem.
>
> >
> > Feels a little like hacking an interface to a java program, which allows to
> > create any object of a special kind dynamically (constructor arguments?),
> > reading some member variable (names) via reflections and then throwing that
> > object away. Which sounds very wrong to me.
>
> I wouldn't call it wrong. Confusing, maybe. We use a mix of class-based
> and prototype-based approaches to inheritance and property
> introspection.
>
> >
> > Wonder if it wouldn't make more sense to query only the static properties of a
> > device. I mean if the dynamic properties are dynamic by definition (and can
> > change during runtime). So looking up the static properties via the typename
> > feels more KIS-style to me. Of course, the relevant properties would have to be
> > defined statically then, which shouldn't be a problem.
>
> It depends on your definition of "shouldn't be a problem". :)
Well, it might require some internal rework of that property thingy, hehe ;)
>
> The static property system is more limited than the QOM dynamic property
> system, today. We already depend on introspection of dynamic properties
> registered by instance_init functions, so we would need to move
> everything to a better static property system if we want to stop doing
> object_new() during class introspection without regressions.
>
> >
> > Dynamic properties that are actually created could depend on almost
> > everything in the system - why fake something that we don't know.
>
> Properties registered on instance_init shouldn't depend on anything else
> on the system. Properties registered later in the object lifetime (e.g.
> on realize) can.
>
Okay, so if the properties in instance_init should depend on nothing else in
the system, they are always the same for one QEMU version + device.
So maybe it would make sense to define all these "fixed properties" on a device
class level (e.g. via dc->props) and fill them with life afterwards (if we can't
completely specify them at that point). Of course this would need some
rework/careful thinking (e.g. concept of uninitialized properties).
Looking at the errors we get with that approach tells me that it will easily
break again in the future (we need tests for all devices - do we already have
it?) and makes me wonder if a definition on a device class level wouldn't be the
right thing to do - a clean interface description that is valid for each device
instance.
David
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-21 15:38 ` Eduardo Habkost
2015-09-22 8:02 ` David Hildenbrand
@ 2015-09-22 8:07 ` Markus Armbruster
1 sibling, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2015-09-22 8:07 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Alexander Graf, Peter Crosthwaite, Peter Maydell, qemu-stable,
qemu-devel, David Hildenbrand, qemu-ppc, Antony Pavlov, stefanha,
afaerber, Cornelia Huck, Paolo Bonzini, Alistair Francis,
Christian Borntraeger, Li Guang, Richard Henderson
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Mon, Sep 21, 2015 at 10:30:50AM +0200, David Hildenbrand wrote:
>> > Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
>> > > Several devices don't survive object_unref(object_new(T)): they crash
>> > > or hang during cleanup, or they leave dangling pointers behind.
>> > >
>> > > This breaks at least device-list-properties, because
>> > > qmp_device_list_properties() needs to create a device to find its
>> > > properties. Broken in commit f4eb32b "qmp: show QOM properties in
>> > > device-list-properties", v2.1. Example reproducer:
>> > >
>> > > $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
>> > > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
>> > > { "execute": "qmp_capabilities" }
>> > > {"return": {}}
>> > > { "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
>> > > qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
>> > > Aborted (core dumped)
>> > > [Exit 134 (SIGABRT)]
>> > >
>> > > Unfortunately, I can't fix the problems in these devices right now.
>> > > Instead, add DeviceClass member cannot_even_create_with_object_new_yet
>> > > to mark them:
>> > >
>> > > * Crash or hang during cleanup (didn't debug them, so I can't say
>> > > why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>> > > "s390-sclp-event-facility", "sclp"
>> >
>> > Ack for the sclp things. Theses devices are created by the machine and
>> > sclp creates the event-facility, so not having a way to query properties
>> > for these devices is better than a hang.
>> >
>> > David, can you have a look on why these devices fail as outlined?
>> >
>>
>> The problem was that the quiesce and cpu hotplug sclp event devices had no
>> parent (onoly a parent bus). So when the bus (inside the event facility) was
>> removed, it looped forever trying remove/unparent it's "bus childs" (which had
>> no parent).
>>
>> sclp (parent=machine)
>> -> even-facility (parent=sclp)
>> -> bus (parent=event-facility)
>> -> quiesce (parent=null)
>> -> cpu hotplug (parent=null)
>>
>> Maybe that helps others struggling with the same symptoms.
>>
>>
>> Just a quick comment on the introspection:
>>
>> I don't think it is a good idea to expose properties that way. Temporarily
>> creating devices for the sake of querying property names sounds very wrong to
>> me, because certain devices require certain knowledge about how and when they
>> can be created.
I sympathize. However, QOM is what it is. Its design assumption
include:
Properties are dynamically added. To introspect them, you need to
create the object.
and:
> No knowledge should be required for object_new(). Classes' instance_init
> functions should have no side-effects outside the object itself.
I'd tighten this to "object_unref(object_new(...)) works and has no
visible side effect".
See also review of [PATCH RFC 1/7] qom: allow properties to be
registered against classes
Message-ID: <55E72154.7070404@suse.de>
http://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00517.html
>> Feels a little like hacking an interface to a java program, which allows to
>> create any object of a special kind dynamically (constructor arguments?),
>> reading some member variable (names) via reflections and then throwing that
>> object away. Which sounds very wrong to me.
>
> I wouldn't call it wrong. Confusing, maybe. We use a mix of class-based
> and prototype-based approaches to inheritance and property
> introspection.
I'm no friend of this aspect of QOM. But QOM experts assure us it is
necessary (see thread quoted above).
>>
>> Wonder if it wouldn't make more sense to query only the static properties of a
>> device. I mean if the dynamic properties are dynamic by definition (and can
>> change during runtime). So looking up the static properties via the typename
>> feels more KIS-style to me. Of course, the relevant properties would
>> have to be
>> defined statically then, which shouldn't be a problem.
>
> It depends on your definition of "shouldn't be a problem". :)
>
> The static property system is more limited than the QOM dynamic property
> system, today. We already depend on introspection of dynamic properties
> registered by instance_init functions, so we would need to move
> everything to a better static property system if we want to stop doing
> object_new() during class introspection without regressions.
If Dan's patch "qom: allow properties to be registered against classes"
goes in, we can certainly consider the idea to limit
device-list-properties to properties registered against classes.
Even then, the assumption "object_unref(object_new(...)) works and has
no visible side effect" remains until proven unnecessary, because
fundamental design assumptions should not be discarded lightly.
Until then, side effects in instance_init() methods are simply bugs.
This patch protects device-list-properties from known ones.
>> Dynamic properties that are actually created could depend on almost
>> everything in the system - why fake something that we don't know.
>
> Properties registered on instance_init shouldn't depend on anything else
> on the system. Properties registered later in the object lifetime (e.g.
> on realize) can.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-18 12:00 ` [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices Markus Armbruster
2015-09-18 12:38 ` Christian Borntraeger
@ 2015-09-18 16:09 ` Eric Blake
2015-09-21 6:08 ` Markus Armbruster
2015-09-18 16:36 ` Eduardo Habkost
2015-09-18 18:42 ` Thomas Huth
3 siblings, 1 reply; 38+ messages in thread
From: Eric Blake @ 2015-09-18 16:09 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Peter Maydell, ehabkost, Peter Crosthwaite, qemu-stable,
Alistair Francis, Christian Borntraeger, Alexander Graf, qemu-ppc,
Antony Pavlov, stefanha, Cornelia Huck, Paolo Bonzini, afaerber,
Li Guang, Richard Henderson
[-- Attachment #1: Type: text/plain, Size: 2934 bytes --]
On 09/18/2015 06:00 AM, Markus Armbruster wrote:
> Several devices don't survive object_unref(object_new(T)): they crash
> or hang during cleanup, or they leave dangling pointers behind.
>
> Unfortunately, I can't fix the problems in these devices right now.
> Instead, add DeviceClass member cannot_even_create_with_object_new_yet
> to mark them:
(intentionally) Ugly because it is a workaround, but then again, giving
the attribute an ugly name will help call attention to the specific
device owners to fix the mess. I can live with that.
> This also protects -device FOO,help, which uses the same machinery
> since commit ef52358 "qdev-monitor: include QOM properties in -device
> FOO, help output", v2.2. Example reproducer:
>
> $ qemu-system-* -machine none -device pxa2xx-pcmcia,help
>
> Before:
>
> qemu-system-aarch64: .../memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
>
> After:
>
> Can't list properties of device 'pxa2xx-pcmcia'
Not ideal, but much better than a crash, so it gets my vote for
inclusion as an incremental improvement.
> +++ b/include/hw/qdev-core.h
> @@ -114,6 +114,19 @@ typedef struct DeviceClass {
> * TODO remove once we're there
> */
> bool cannot_instantiate_with_device_add_yet;
> + /*
> + * Does this device model survive object_unref(object_new(TNAME))?
> + * All device models should, and this flag shouldn't exist. Some
> + * devices crash in object_new(), some crash or hang in
> + * object_unref(). Makes introspecting properties with
> + * qmp_device_list_properties() dangerous. Bad, because it's used
> + * by -device FOO,help. This flag serves to protect that code.
> + * It should never be set without a comment explaining why it is
> + * set.
> + * TODO remove once we're there
> + */
> + bool cannot_even_create_with_object_new_yet;
And a sufficiently verbose explanation for why the code is so ugly.
> @@ -123,9 +97,6 @@ static void test_device_intro_concrete(void)
> type = qdict_get_try_str(qobject_to_qdict(qlist_entry_obj(entry)),
> "name");
> g_assert(type);
> - if (blacklisted(type)) {
> - continue; /* FIXME broken device, skip */
> - }
The devices are still broken, but the testsuite no longer flags them as
broken because it no longer crashes or hangs, and you intentionally
wrote the test to treat any output (including a graceful error message)
as successful use of the probe. The ugly attribute is now our only
documentation of the problems, but that is still something sufficient to
hopefully get it fixed.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-18 16:09 ` Eric Blake
@ 2015-09-21 6:08 ` Markus Armbruster
0 siblings, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2015-09-21 6:08 UTC (permalink / raw)
To: Eric Blake
Cc: Peter Maydell, ehabkost, Peter Crosthwaite, qemu-devel,
qemu-stable, Alistair Francis, Christian Borntraeger,
Alexander Graf, qemu-ppc, Antony Pavlov, stefanha, Cornelia Huck,
Paolo Bonzini, afaerber, Li Guang, Richard Henderson
Eric Blake <eblake@redhat.com> writes:
> On 09/18/2015 06:00 AM, Markus Armbruster wrote:
>> Several devices don't survive object_unref(object_new(T)): they crash
>> or hang during cleanup, or they leave dangling pointers behind.
>>
>
>> Unfortunately, I can't fix the problems in these devices right now.
>> Instead, add DeviceClass member cannot_even_create_with_object_new_yet
>> to mark them:
>
> (intentionally) Ugly because it is a workaround, but then again, giving
> the attribute an ugly name will help call attention to the specific
> device owners to fix the mess. I can live with that.
Named in Anthony's memory (see commit efec3dd) ;-P
>> This also protects -device FOO,help, which uses the same machinery
>> since commit ef52358 "qdev-monitor: include QOM properties in -device
>> FOO, help output", v2.2. Example reproducer:
>>
>> $ qemu-system-* -machine none -device pxa2xx-pcmcia,help
>>
>> Before:
>>
>> qemu-system-aarch64: .../memory.c:1307: memory_region_finalize:
>> Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
>>
>> After:
>>
>> Can't list properties of device 'pxa2xx-pcmcia'
>
> Not ideal, but much better than a crash, so it gets my vote for
> inclusion as an incremental improvement.
>
>
>> +++ b/include/hw/qdev-core.h
>> @@ -114,6 +114,19 @@ typedef struct DeviceClass {
>> * TODO remove once we're there
>> */
>> bool cannot_instantiate_with_device_add_yet;
>> + /*
>> + * Does this device model survive object_unref(object_new(TNAME))?
>> + * All device models should, and this flag shouldn't exist. Some
>> + * devices crash in object_new(), some crash or hang in
>> + * object_unref(). Makes introspecting properties with
>> + * qmp_device_list_properties() dangerous. Bad, because it's used
>> + * by -device FOO,help. This flag serves to protect that code.
>> + * It should never be set without a comment explaining why it is
>> + * set.
>> + * TODO remove once we're there
>> + */
>> + bool cannot_even_create_with_object_new_yet;
>
> And a sufficiently verbose explanation for why the code is so ugly.
>
>> @@ -123,9 +97,6 @@ static void test_device_intro_concrete(void)
>> type = qdict_get_try_str(qobject_to_qdict(qlist_entry_obj(entry)),
>> "name");
>> g_assert(type);
>> - if (blacklisted(type)) {
>> - continue; /* FIXME broken device, skip */
>> - }
>
> The devices are still broken, but the testsuite no longer flags them as
> broken because it no longer crashes or hangs, and you intentionally
> wrote the test to treat any output (including a graceful error message)
> as successful use of the probe. The ugly attribute is now our only
> documentation of the problems, but that is still something sufficient to
> hopefully get it fixed.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-18 12:00 ` [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices Markus Armbruster
2015-09-18 12:38 ` Christian Borntraeger
2015-09-18 16:09 ` Eric Blake
@ 2015-09-18 16:36 ` Eduardo Habkost
2015-09-21 6:09 ` Markus Armbruster
2015-09-18 18:42 ` Thomas Huth
3 siblings, 1 reply; 38+ messages in thread
From: Eduardo Habkost @ 2015-09-18 16:36 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Maydell, Peter Crosthwaite, qemu-devel, qemu-stable,
Alexander Graf, Alistair Francis, Christian Borntraeger, qemu-ppc,
Antony Pavlov, stefanha, Cornelia Huck, Paolo Bonzini, afaerber,
Li Guang, Richard Henderson
On Fri, Sep 18, 2015 at 02:00:38PM +0200, Markus Armbruster wrote:
> Several devices don't survive object_unref(object_new(T)): they crash
> or hang during cleanup, or they leave dangling pointers behind.
>
> This breaks at least device-list-properties, because
> qmp_device_list_properties() needs to create a device to find its
> properties. Broken in commit f4eb32b "qmp: show QOM properties in
> device-list-properties", v2.1. Example reproducer:
>
> $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
> { "execute": "qmp_capabilities" }
> {"return": {}}
> { "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
> qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
> Aborted (core dumped)
> [Exit 134 (SIGABRT)]
>
> Unfortunately, I can't fix the problems in these devices right now.
> Instead, add DeviceClass member cannot_even_create_with_object_new_yet
> to mark them:
>
> * Crash or hang during cleanup (didn't debug them, so I can't say
> why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
> "s390-sclp-event-facility", "sclp"
>
> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
> "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
That's isn't true for all CPU classes, only the ones that (incorrectly)
call cpu_exec_init() on instance_init instead of realize. I believe at
least TYPE_POWERPC_CPU is safe already.
--
Eduardo
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-18 16:36 ` Eduardo Habkost
@ 2015-09-21 6:09 ` Markus Armbruster
2015-09-21 15:13 ` Eduardo Habkost
0 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2015-09-21 6:09 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Peter Maydell, Peter Crosthwaite, qemu-devel, qemu-stable,
Alistair Francis, Christian Borntraeger, Alexander Graf, qemu-ppc,
Antony Pavlov, stefanha, Cornelia Huck, Paolo Bonzini, afaerber,
Li Guang, Richard Henderson
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Fri, Sep 18, 2015 at 02:00:38PM +0200, Markus Armbruster wrote:
>> Several devices don't survive object_unref(object_new(T)): they crash
>> or hang during cleanup, or they leave dangling pointers behind.
>>
>> This breaks at least device-list-properties, because
>> qmp_device_list_properties() needs to create a device to find its
>> properties. Broken in commit f4eb32b "qmp: show QOM properties in
>> device-list-properties", v2.1. Example reproducer:
>>
>> $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
>> { "execute": "qmp_capabilities" }
>> {"return": {}}
>> { "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
>> qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
>> Aborted (core dumped)
>> [Exit 134 (SIGABRT)]
>>
>> Unfortunately, I can't fix the problems in these devices right now.
>> Instead, add DeviceClass member cannot_even_create_with_object_new_yet
>> to mark them:
>>
>> * Crash or hang during cleanup (didn't debug them, so I can't say
>> why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>> "s390-sclp-event-facility", "sclp"
>>
>> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
>> "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
>
> That's isn't true for all CPU classes, only the ones that (incorrectly)
> call cpu_exec_init() on instance_init instead of realize. I believe at
> least TYPE_POWERPC_CPU is safe already.
Okay, I'll try to mark only the ones that actually screw up.
Thanks!
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-21 6:09 ` Markus Armbruster
@ 2015-09-21 15:13 ` Eduardo Habkost
0 siblings, 0 replies; 38+ messages in thread
From: Eduardo Habkost @ 2015-09-21 15:13 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Maydell, Peter Crosthwaite, qemu-devel, qemu-stable,
Alistair Francis, Christian Borntraeger, Alexander Graf, qemu-ppc,
Antony Pavlov, stefanha, Cornelia Huck, Paolo Bonzini, afaerber,
Li Guang, Richard Henderson
On Mon, Sep 21, 2015 at 08:09:48AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > On Fri, Sep 18, 2015 at 02:00:38PM +0200, Markus Armbruster wrote:
> >> Several devices don't survive object_unref(object_new(T)): they crash
> >> or hang during cleanup, or they leave dangling pointers behind.
> >>
> >> This breaks at least device-list-properties, because
> >> qmp_device_list_properties() needs to create a device to find its
> >> properties. Broken in commit f4eb32b "qmp: show QOM properties in
> >> device-list-properties", v2.1. Example reproducer:
> >>
> >> $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
> >> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
> >> { "execute": "qmp_capabilities" }
> >> {"return": {}}
> >> { "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
> >> qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
> >> Aborted (core dumped)
> >> [Exit 134 (SIGABRT)]
> >>
> >> Unfortunately, I can't fix the problems in these devices right now.
> >> Instead, add DeviceClass member cannot_even_create_with_object_new_yet
> >> to mark them:
> >>
> >> * Crash or hang during cleanup (didn't debug them, so I can't say
> >> why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
> >> "s390-sclp-event-facility", "sclp"
> >>
> >> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
> >> "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
> >
> > That's isn't true for all CPU classes, only the ones that (incorrectly)
> > call cpu_exec_init() on instance_init instead of realize. I believe at
> > least TYPE_POWERPC_CPU is safe already.
>
> Okay, I'll try to mark only the ones that actually screw up.
Most of them screw up, today. If you prefer to simply set it to true on
TYPE_CPU and then explicitly override it to false only on the few
subclasses that are already fixed, I think it would be OK.
--
Eduardo
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-18 12:00 ` [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices Markus Armbruster
` (2 preceding siblings ...)
2015-09-18 16:36 ` Eduardo Habkost
@ 2015-09-18 18:42 ` Thomas Huth
2015-09-18 19:32 ` Eduardo Habkost
3 siblings, 1 reply; 38+ messages in thread
From: Thomas Huth @ 2015-09-18 18:42 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Peter Maydell, ehabkost, Peter Crosthwaite, qemu-stable,
Alistair Francis, Christian Borntraeger, Alexander Graf, qemu-ppc,
Antony Pavlov, stefanha, Cornelia Huck, Paolo Bonzini, afaerber,
Li Guang, Richard Henderson
On 18/09/15 14:00, Markus Armbruster wrote:
> Several devices don't survive object_unref(object_new(T)): they crash
> or hang during cleanup, or they leave dangling pointers behind.
>
> This breaks at least device-list-properties, because
> qmp_device_list_properties() needs to create a device to find its
> properties. Broken in commit f4eb32b "qmp: show QOM properties in
> device-list-properties", v2.1. Example reproducer:
>
> $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
> { "execute": "qmp_capabilities" }
> {"return": {}}
> { "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
> qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
> Aborted (core dumped)
> [Exit 134 (SIGABRT)]
>
> Unfortunately, I can't fix the problems in these devices right now.
> Instead, add DeviceClass member cannot_even_create_with_object_new_yet
> to mark them:
>
> * Crash or hang during cleanup (didn't debug them, so I can't say
> why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
> "s390-sclp-event-facility", "sclp"
>
> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
> "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
>
> * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
> "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
> "host-powerpc-cpu"
I just had a look at the powerpc code - you're likely talking about
the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
target-ppc/kvm.c ? That should be fine, I think, because
kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
enabled.
Thomas
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-18 18:42 ` Thomas Huth
@ 2015-09-18 19:32 ` Eduardo Habkost
2015-09-21 6:14 ` Markus Armbruster
0 siblings, 1 reply; 38+ messages in thread
From: Eduardo Habkost @ 2015-09-18 19:32 UTC (permalink / raw)
To: Thomas Huth
Cc: Peter Maydell, Peter Crosthwaite, qemu-devel, qemu-stable,
Markus Armbruster, Christian Borntraeger, Alexander Graf,
qemu-ppc, Antony Pavlov, stefanha, Cornelia Huck, Paolo Bonzini,
Alistair Francis, afaerber, Li Guang, Richard Henderson
On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
> On 18/09/15 14:00, Markus Armbruster wrote:
> > Several devices don't survive object_unref(object_new(T)): they crash
> > or hang during cleanup, or they leave dangling pointers behind.
> >
> > This breaks at least device-list-properties, because
> > qmp_device_list_properties() needs to create a device to find its
> > properties. Broken in commit f4eb32b "qmp: show QOM properties in
> > device-list-properties", v2.1. Example reproducer:
> >
> > $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
> > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
> > { "execute": "qmp_capabilities" }
> > {"return": {}}
> > { "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
> > qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
> > Aborted (core dumped)
> > [Exit 134 (SIGABRT)]
> >
> > Unfortunately, I can't fix the problems in these devices right now.
> > Instead, add DeviceClass member cannot_even_create_with_object_new_yet
> > to mark them:
> >
> > * Crash or hang during cleanup (didn't debug them, so I can't say
> > why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
> > "s390-sclp-event-facility", "sclp"
> >
> > * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
> > "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
> >
> > * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
> > "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
> > "host-powerpc-cpu"
>
> I just had a look at the powerpc code - you're likely talking about
> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
> target-ppc/kvm.c ? That should be fine, I think, because
> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
> enabled.
It's true that currently the assert() will never trigger, but we will
have to eventually move class registration to type_init if we want to
make a generic query-cpu-definitions implementation work properly
without depending on global machine accel configuration.
It won't hurt to set cannot_even_create_with_object_new_yet properly to
reflect that the class isn't ready yet.
--
Eduardo
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-18 19:32 ` Eduardo Habkost
@ 2015-09-21 6:14 ` Markus Armbruster
2015-09-21 15:20 ` Eduardo Habkost
2015-09-21 15:48 ` Thomas Huth
0 siblings, 2 replies; 38+ messages in thread
From: Markus Armbruster @ 2015-09-21 6:14 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Peter Maydell, Thomas Huth, Peter Crosthwaite, qemu-stable,
qemu-devel, Christian Borntraeger, Alexander Graf, qemu-ppc,
Antony Pavlov, stefanha, Cornelia Huck, Paolo Bonzini,
Alistair Francis, afaerber, Li Guang, Richard Henderson
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
>> On 18/09/15 14:00, Markus Armbruster wrote:
>> > Several devices don't survive object_unref(object_new(T)): they crash
>> > or hang during cleanup, or they leave dangling pointers behind.
>> >
>> > This breaks at least device-list-properties, because
>> > qmp_device_list_properties() needs to create a device to find its
>> > properties. Broken in commit f4eb32b "qmp: show QOM properties in
>> > device-list-properties", v2.1. Example reproducer:
>> >
>> > $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
>> > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
>> > { "execute": "qmp_capabilities" }
>> > {"return": {}}
>> > { "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
>> > qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
>> > Aborted (core dumped)
>> > [Exit 134 (SIGABRT)]
>> >
>> > Unfortunately, I can't fix the problems in these devices right now.
>> > Instead, add DeviceClass member cannot_even_create_with_object_new_yet
>> > to mark them:
>> >
>> > * Crash or hang during cleanup (didn't debug them, so I can't say
>> > why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>> > "s390-sclp-event-facility", "sclp"
>> >
>> > * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
>> > "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
>> >
>> > * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
>> > "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
>> > "host-powerpc-cpu"
>>
>> I just had a look at the powerpc code - you're likely talking about
>> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
>> target-ppc/kvm.c ? That should be fine, I think, because
>> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
>> enabled.
Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
and off, where C is the appropriate host CPU.
> It's true that currently the assert() will never trigger, but we will
> have to eventually move class registration to type_init if we want to
> make a generic query-cpu-definitions implementation work properly
> without depending on global machine accel configuration.
Good point. These CPUs need a TODO marker.
> It won't hurt to set cannot_even_create_with_object_new_yet properly to
> reflect that the class isn't ready yet.
Marking cannot_even_create_with_object_new_yet() is the obvious TODO
marker. It has an unnecessary side effect: we disable introspection for
these CPUs needlessly. Tolerable?
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-21 6:14 ` Markus Armbruster
@ 2015-09-21 15:20 ` Eduardo Habkost
2015-09-21 15:48 ` Thomas Huth
1 sibling, 0 replies; 38+ messages in thread
From: Eduardo Habkost @ 2015-09-21 15:20 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Maydell, Thomas Huth, Peter Crosthwaite, qemu-stable,
qemu-devel, Christian Borntraeger, Alexander Graf, qemu-ppc,
Antony Pavlov, stefanha, Cornelia Huck, Paolo Bonzini,
Alistair Francis, afaerber, Li Guang, Richard Henderson
On Mon, Sep 21, 2015 at 08:14:56AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
> >> On 18/09/15 14:00, Markus Armbruster wrote:
> >> > Several devices don't survive object_unref(object_new(T)): they crash
> >> > or hang during cleanup, or they leave dangling pointers behind.
> >> >
> >> > This breaks at least device-list-properties, because
> >> > qmp_device_list_properties() needs to create a device to find its
> >> > properties. Broken in commit f4eb32b "qmp: show QOM properties in
> >> > device-list-properties", v2.1. Example reproducer:
> >> >
> >> > $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
> >> > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
> >> > { "execute": "qmp_capabilities" }
> >> > {"return": {}}
> >> > { "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
> >> > qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
> >> > Aborted (core dumped)
> >> > [Exit 134 (SIGABRT)]
> >> >
> >> > Unfortunately, I can't fix the problems in these devices right now.
> >> > Instead, add DeviceClass member cannot_even_create_with_object_new_yet
> >> > to mark them:
> >> >
> >> > * Crash or hang during cleanup (didn't debug them, so I can't say
> >> > why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
> >> > "s390-sclp-event-facility", "sclp"
> >> >
> >> > * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
> >> > "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
> >> >
> >> > * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
> >> > "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
> >> > "host-powerpc-cpu"
> >>
> >> I just had a look at the powerpc code - you're likely talking about
> >> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
> >> target-ppc/kvm.c ? That should be fine, I think, because
> >> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
> >> enabled.
>
> Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
> and off, where C is the appropriate host CPU.
>
> > It's true that currently the assert() will never trigger, but we will
> > have to eventually move class registration to type_init if we want to
> > make a generic query-cpu-definitions implementation work properly
> > without depending on global machine accel configuration.
>
> Good point. These CPUs need a TODO marker.
>
> > It won't hurt to set cannot_even_create_with_object_new_yet properly to
> > reflect that the class isn't ready yet.
>
> Marking cannot_even_create_with_object_new_yet() is the obvious TODO
> marker. It has an unnecessary side effect: we disable introspection for
> these CPUs needlessly. Tolerable?
To me, it is. We already disable introspection of these CPUs needlessly
in all cases except when KVM is explicitly enabled in the command-line.
--
Eduardo
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-21 6:14 ` Markus Armbruster
2015-09-21 15:20 ` Eduardo Habkost
@ 2015-09-21 15:48 ` Thomas Huth
2015-09-21 16:39 ` Markus Armbruster
1 sibling, 1 reply; 38+ messages in thread
From: Thomas Huth @ 2015-09-21 15:48 UTC (permalink / raw)
To: Markus Armbruster, Eduardo Habkost
Cc: Peter Maydell, Peter Crosthwaite, qemu-devel, qemu-stable,
Alexander Graf, Christian Borntraeger, qemu-ppc, Antony Pavlov,
stefanha, Cornelia Huck, Paolo Bonzini, Alistair Francis,
afaerber, Li Guang, Richard Henderson
On 21/09/15 08:14, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
>> On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
>>> On 18/09/15 14:00, Markus Armbruster wrote:
>>>> Several devices don't survive object_unref(object_new(T)): they crash
>>>> or hang during cleanup, or they leave dangling pointers behind.
>>>>
>>>> This breaks at least device-list-properties, because
>>>> qmp_device_list_properties() needs to create a device to find its
>>>> properties. Broken in commit f4eb32b "qmp: show QOM properties in
>>>> device-list-properties", v2.1. Example reproducer:
>>>>
>>>> $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
>>>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
>>>> { "execute": "qmp_capabilities" }
>>>> {"return": {}}
>>>> { "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
>>>> qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
>>>> Aborted (core dumped)
>>>> [Exit 134 (SIGABRT)]
>>>>
>>>> Unfortunately, I can't fix the problems in these devices right now.
>>>> Instead, add DeviceClass member cannot_even_create_with_object_new_yet
>>>> to mark them:
>>>>
>>>> * Crash or hang during cleanup (didn't debug them, so I can't say
>>>> why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>>>> "s390-sclp-event-facility", "sclp"
>>>>
>>>> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
>>>> "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
>>>>
>>>> * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
>>>> "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
>>>> "host-powerpc-cpu"
>>>
>>> I just had a look at the powerpc code - you're likely talking about
>>> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
>>> target-ppc/kvm.c ? That should be fine, I think, because
>>> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
>>> enabled.
>
> Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
> and off, where C is the appropriate host CPU.
In both cases (KVM on and off), I simply get:
# qemu-system-ppc64 -machine accel=tcg -device host-powerpc64-cpu,help
'host-powerpc64-cpu' is not a valid device model name
# qemu-system-ppc64 -machine accel=kvm -device host-powerpc64-cpu,help
'host-powerpc64-cpu' is not a valid device model name
No crash/abort here.
Thomas
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-21 15:48 ` Thomas Huth
@ 2015-09-21 16:39 ` Markus Armbruster
2015-09-21 17:22 ` Thomas Huth
0 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2015-09-21 16:39 UTC (permalink / raw)
To: Thomas Huth
Cc: Peter Maydell, Eduardo Habkost, Peter Crosthwaite, qemu-devel,
qemu-stable, Christian Borntraeger, Alexander Graf, qemu-ppc,
Antony Pavlov, stefanha, Cornelia Huck, Paolo Bonzini,
Alistair Francis, afaerber, Li Guang, Richard Henderson
Thomas Huth <thuth@redhat.com> writes:
> On 21/09/15 08:14, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>
>>> On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
>>>> On 18/09/15 14:00, Markus Armbruster wrote:
>>>>> Several devices don't survive object_unref(object_new(T)): they crash
>>>>> or hang during cleanup, or they leave dangling pointers behind.
>>>>>
>>>>> This breaks at least device-list-properties, because
>>>>> qmp_device_list_properties() needs to create a device to find its
>>>>> properties. Broken in commit f4eb32b "qmp: show QOM properties in
>>>>> device-list-properties", v2.1. Example reproducer:
>>>>>
>>>>> $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
>>>>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
>>>>> { "execute": "qmp_capabilities" }
>>>>> {"return": {}}
>>>>> { "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
>>>>> qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
>>>>> Aborted (core dumped)
>>>>> [Exit 134 (SIGABRT)]
>>>>>
>>>>> Unfortunately, I can't fix the problems in these devices right now.
>>>>> Instead, add DeviceClass member cannot_even_create_with_object_new_yet
>>>>> to mark them:
>>>>>
>>>>> * Crash or hang during cleanup (didn't debug them, so I can't say
>>>>> why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>>>>> "s390-sclp-event-facility", "sclp"
>>>>>
>>>>> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
>>>>> "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
>>>>>
>>>>> * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
>>>>> "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
>>>>> "host-powerpc-cpu"
>>>>
>>>> I just had a look at the powerpc code - you're likely talking about
>>>> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
>>>> target-ppc/kvm.c ? That should be fine, I think, because
>>>> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
>>>> enabled.
>>
>> Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
>> and off, where C is the appropriate host CPU.
>
> In both cases (KVM on and off), I simply get:
>
> # qemu-system-ppc64 -machine accel=tcg -device host-powerpc64-cpu,help
> 'host-powerpc64-cpu' is not a valid device model name
> # qemu-system-ppc64 -machine accel=kvm -device host-powerpc64-cpu,help
> 'host-powerpc64-cpu' is not a valid device model name
>
> No crash/abort here.
No introspection, either. Can you try the same with QMP command
device-list-properties?
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-21 16:39 ` Markus Armbruster
@ 2015-09-21 17:22 ` Thomas Huth
2015-09-21 18:19 ` Eduardo Habkost
0 siblings, 1 reply; 38+ messages in thread
From: Thomas Huth @ 2015-09-21 17:22 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Maydell, Eduardo Habkost, Peter Crosthwaite, qemu-devel,
qemu-stable, Christian Borntraeger, Alexander Graf, qemu-ppc,
Antony Pavlov, stefanha, Cornelia Huck, Paolo Bonzini,
Alistair Francis, afaerber, Li Guang, Richard Henderson
On 21/09/15 18:39, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
>
>> On 21/09/15 08:14, Markus Armbruster wrote:
>>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>>
>>>> On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
>>>>> On 18/09/15 14:00, Markus Armbruster wrote:
>>>>>> Several devices don't survive object_unref(object_new(T)): they crash
>>>>>> or hang during cleanup, or they leave dangling pointers behind.
>>>>>>
>>>>>> This breaks at least device-list-properties, because
>>>>>> qmp_device_list_properties() needs to create a device to find its
>>>>>> properties. Broken in commit f4eb32b "qmp: show QOM properties in
>>>>>> device-list-properties", v2.1. Example reproducer:
>>>>>>
>>>>>> $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
>>>>>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
>>>>>> { "execute": "qmp_capabilities" }
>>>>>> {"return": {}}
>>>>>> { "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
>>>>>> qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
>>>>>> Aborted (core dumped)
>>>>>> [Exit 134 (SIGABRT)]
>>>>>>
>>>>>> Unfortunately, I can't fix the problems in these devices right now.
>>>>>> Instead, add DeviceClass member cannot_even_create_with_object_new_yet
>>>>>> to mark them:
>>>>>>
>>>>>> * Crash or hang during cleanup (didn't debug them, so I can't say
>>>>>> why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>>>>>> "s390-sclp-event-facility", "sclp"
>>>>>>
>>>>>> * Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
>>>>>> "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
>>>>>>
>>>>>> * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
>>>>>> "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
>>>>>> "host-powerpc-cpu"
>>>>>
>>>>> I just had a look at the powerpc code - you're likely talking about
>>>>> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
>>>>> target-ppc/kvm.c ? That should be fine, I think, because
>>>>> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
>>>>> enabled.
>>>
>>> Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
>>> and off, where C is the appropriate host CPU.
>>
>> In both cases (KVM on and off), I simply get:
>>
>> # qemu-system-ppc64 -machine accel=tcg -device host-powerpc64-cpu,help
>> 'host-powerpc64-cpu' is not a valid device model name
>> # qemu-system-ppc64 -machine accel=kvm -device host-powerpc64-cpu,help
>> 'host-powerpc64-cpu' is not a valid device model name
>>
>> No crash/abort here.
>
> No introspection, either. Can you try the same with QMP command
> device-list-properties?
# ppc64-softmmu/qemu-system-ppc64 -nodefaults -S -display none -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
{ "execute": "qmp_capabilities" }
{"return": {}}
{ "execute": "device-list-properties", "arguments": { "typename": "host-powerpc64-cpu" } }
{"error": {"class": "DeviceNotFound", "desc": "Device 'host-powerpc64-cpu' not found"}}
{ "execute": "quit" }
{"return": {}}
{"timestamp": {"seconds": 1442856053, "microseconds": 732079}, "event": "SHUTDOWN"}
# ppc64-softmmu/qemu-system-ppc64 -machine accel=kvm -nodefaults -S -display none -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
{ "execute": "qmp_capabilities" }
{"return": {}}
{ "execute": "device-list-properties", "arguments": { "typename": "host-powerpc64-cpu" } }
{"return": [{"name": "compat", "description": "compatibility mode, power6/power7/power8", "type": "str"}]}
{ "execute": "quit" }
{"return": {}}
{"timestamp": {"seconds": 1442856113, "microseconds": 913588}, "event": "SHUTDOWN"}
Thomas
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices
2015-09-21 17:22 ` Thomas Huth
@ 2015-09-21 18:19 ` Eduardo Habkost
0 siblings, 0 replies; 38+ messages in thread
From: Eduardo Habkost @ 2015-09-21 18:19 UTC (permalink / raw)
To: Thomas Huth
Cc: Peter Maydell, Peter Crosthwaite, Markus Armbruster, qemu-devel,
qemu-stable, Christian Borntraeger, Alexander Graf, qemu-ppc,
Antony Pavlov, stefanha, Cornelia Huck, Paolo Bonzini,
Alistair Francis, afaerber, Li Guang, Richard Henderson
On Mon, Sep 21, 2015 at 07:22:51PM +0200, Thomas Huth wrote:
> On 21/09/15 18:39, Markus Armbruster wrote:
> > Thomas Huth <thuth@redhat.com> writes:
> >> On 21/09/15 08:14, Markus Armbruster wrote:
> >>> Eduardo Habkost <ehabkost@redhat.com> writes:
> >>>> On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
> >>>>> On 18/09/15 14:00, Markus Armbruster wrote:
[...]
> >>>>>> * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
> >>>>>> "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
> >>>>>> "host-powerpc-cpu"
> >>>>>
> >>>>> I just had a look at the powerpc code - you're likely talking about
> >>>>> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
> >>>>> target-ppc/kvm.c ? That should be fine, I think, because
> >>>>> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
> >>>>> enabled.
> >>>
> >>> Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
> >>> and off, where C is the appropriate host CPU.
> >>
> >> In both cases (KVM on and off), I simply get:
> >>
> >> # qemu-system-ppc64 -machine accel=tcg -device host-powerpc64-cpu,help
> >> 'host-powerpc64-cpu' is not a valid device model name
> >> # qemu-system-ppc64 -machine accel=kvm -device host-powerpc64-cpu,help
> >> 'host-powerpc64-cpu' is not a valid device model name
> >>
> >> No crash/abort here.
> >
> > No introspection, either. Can you try the same with QMP command
> > device-list-properties?
>
> # ppc64-softmmu/qemu-system-ppc64 -nodefaults -S -display none -qmp stdio
[...]
> { "execute": "device-list-properties", "arguments": { "typename": "host-powerpc64-cpu" } }
> {"error": {"class": "DeviceNotFound", "desc": "Device 'host-powerpc64-cpu' not found"}}
>
> # ppc64-softmmu/qemu-system-ppc64 -machine accel=kvm -nodefaults -S -display none -qmp stdio
[...]
> { "execute": "device-list-properties", "arguments": { "typename": "host-powerpc64-cpu" } }
> {"return": [{"name": "compat", "description": "compatibility mode, power6/power7/power8", "type": "str"}]}
So, introspection is already broken, but inconsistently broken (because
it depends on machine arguments). Setting
cannot_even_create_with_object_new_yet will make it more consistently
broken.
--
Eduardo
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH 6/7] Revert "qdev: Use qdev_get_device_class() for -device <type>, help"
2015-09-18 12:00 [Qemu-devel] [PATCH 0/7] Fix device introspection regressions Markus Armbruster
` (4 preceding siblings ...)
2015-09-18 12:00 ` [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices Markus Armbruster
@ 2015-09-18 12:00 ` Markus Armbruster
2015-09-18 16:13 ` Eric Blake
2015-09-18 12:00 ` [Qemu-devel] [PATCH 7/7] tests: Simplify how qom-test is run Markus Armbruster
6 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2015-09-18 12:00 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-stable, afaerber, stefanha, ehabkost
This reverts commit 31bed5509dfcbdfc293154ce81086a4dbd7a80b6.
The reverted commit changed qdev_device_help() to reject abstract
devices and devices that have cannot_instantiate_with_device_add_yet
set, to fix crash bugs like -device x86_64-cpu,help.
Rejecting abstract devices makes sense: they're purely internal, and
the implementation of the help feature can't cope with them.
Rejecting non-pluggable devices makes less sense: even though you
can't use them with -device, the help may still be useful elsewhere,
for instance with -global. This is a regression: -device FOO,help
used to help even for FOO that aren't pluggable.
The previous two commits fixed the crash bug at a lower layer, so
reverting this one is now safe. Fixes the -device FOO,help
regression, except for the broken devices marked
cannot_even_create_with_object_new_yet. For those, the error message
is improved.
Example of a device where the regression is fixed:
$ qemu-system-x86_64 -device PIIX4_PM,help
PIIX4_PM.command_serr_enable=bool (on/off)
PIIX4_PM.multifunction=bool (on/off)
PIIX4_PM.rombar=uint32
PIIX4_PM.romfile=str
PIIX4_PM.addr=int32 (Slot and optional function number, example: 06.0 or 06)
PIIX4_PM.memory-hotplug-support=bool
PIIX4_PM.acpi-pci-hotplug-with-bridge-support=bool
PIIX4_PM.s4_val=uint8
PIIX4_PM.disable_s4=uint8
PIIX4_PM.disable_s3=uint8
PIIX4_PM.smb_io_base=uint32
Example of a device where it isn't fixed:
$ qemu-system-x86_64 -device host-x86_64-cpu,help
Can't list properties of device 'host-x86_64-cpu'
Both failed with "Parameter 'driver' expects pluggable device type"
before.
Cc: qemu-stable@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qdev-monitor.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index f9e2d62..0e14747 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -237,9 +237,12 @@ int qdev_device_help(QemuOpts *opts)
return 0;
}
- qdev_get_device_class(&driver, &local_err);
- if (local_err) {
- goto error;
+ if (!object_class_by_name(driver)) {
+ const char *typename = find_typename_by_alias(driver);
+
+ if (typename) {
+ driver = typename;
+ }
}
prop_list = qmp_device_list_properties(driver, &local_err);
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] Revert "qdev: Use qdev_get_device_class() for -device <type>, help"
2015-09-18 12:00 ` [Qemu-devel] [PATCH 6/7] Revert "qdev: Use qdev_get_device_class() for -device <type>, help" Markus Armbruster
@ 2015-09-18 16:13 ` Eric Blake
0 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-09-18 16:13 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: ehabkost, qemu-stable, stefanha, afaerber
[-- Attachment #1: Type: text/plain, Size: 2173 bytes --]
On 09/18/2015 06:00 AM, Markus Armbruster wrote:
> This reverts commit 31bed5509dfcbdfc293154ce81086a4dbd7a80b6.
>
> The reverted commit changed qdev_device_help() to reject abstract
> devices and devices that have cannot_instantiate_with_device_add_yet
> set, to fix crash bugs like -device x86_64-cpu,help.
>
> Rejecting abstract devices makes sense: they're purely internal, and
> the implementation of the help feature can't cope with them.
>
> Rejecting non-pluggable devices makes less sense: even though you
> can't use them with -device, the help may still be useful elsewhere,
> for instance with -global. This is a regression: -device FOO,help
> used to help even for FOO that aren't pluggable.
>
> The previous two commits fixed the crash bug at a lower layer, so
> reverting this one is now safe. Fixes the -device FOO,help
> regression, except for the broken devices marked
> cannot_even_create_with_object_new_yet. For those, the error message
> is improved.
>
> Example of a device where the regression is fixed:
>
> $ qemu-system-x86_64 -device PIIX4_PM,help
> PIIX4_PM.command_serr_enable=bool (on/off)
> PIIX4_PM.multifunction=bool (on/off)
> PIIX4_PM.rombar=uint32
> PIIX4_PM.romfile=str
> PIIX4_PM.addr=int32 (Slot and optional function number, example: 06.0 or 06)
> PIIX4_PM.memory-hotplug-support=bool
> PIIX4_PM.acpi-pci-hotplug-with-bridge-support=bool
> PIIX4_PM.s4_val=uint8
> PIIX4_PM.disable_s4=uint8
> PIIX4_PM.disable_s3=uint8
> PIIX4_PM.smb_io_base=uint32
>
> Example of a device where it isn't fixed:
>
> $ qemu-system-x86_64 -device host-x86_64-cpu,help
> Can't list properties of device 'host-x86_64-cpu'
>
> Both failed with "Parameter 'driver' expects pluggable device type"
> before.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> qdev-monitor.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH 7/7] tests: Simplify how qom-test is run
2015-09-18 12:00 [Qemu-devel] [PATCH 0/7] Fix device introspection regressions Markus Armbruster
` (5 preceding siblings ...)
2015-09-18 12:00 ` [Qemu-devel] [PATCH 6/7] Revert "qdev: Use qdev_get_device_class() for -device <type>, help" Markus Armbruster
@ 2015-09-18 12:00 ` Markus Armbruster
2015-09-18 12:53 ` Andreas Färber
6 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2015-09-18 12:00 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber, stefanha, ehabkost
Add it to check-qtest-generic-y instead of check-qtest-$(target)-y for
every target.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/Makefile | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/tests/Makefile b/tests/Makefile
index 4559045..28c5f93 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -219,10 +219,7 @@ gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
-# qom-test works for all sysemu architectures:
-$(foreach target,$(SYSEMU_TARGET_LIST), \
- $(if $(findstring tests/qom-test$(EXESUF), $(check-qtest-$(target)-y)),, \
- $(eval check-qtest-$(target)-y += tests/qom-test$(EXESUF))))
+check-qtest-generic-y += tests/qom-test$(EXESUF)
check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
comments.json empty.json enum-empty.json enum-missing-data.json \
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH 7/7] tests: Simplify how qom-test is run
2015-09-18 12:00 ` [Qemu-devel] [PATCH 7/7] tests: Simplify how qom-test is run Markus Armbruster
@ 2015-09-18 12:53 ` Andreas Färber
2015-09-18 14:24 ` Markus Armbruster
0 siblings, 1 reply; 38+ messages in thread
From: Andreas Färber @ 2015-09-18 12:53 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: ehabkost, stefanha
Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
> Add it to check-qtest-generic-y instead of check-qtest-$(target)-y for
> every target.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> tests/Makefile | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/tests/Makefile b/tests/Makefile
> index 4559045..28c5f93 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -219,10 +219,7 @@ gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
> check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
> check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>
> -# qom-test works for all sysemu architectures:
> -$(foreach target,$(SYSEMU_TARGET_LIST), \
> - $(if $(findstring tests/qom-test$(EXESUF), $(check-qtest-$(target)-y)),, \
> - $(eval check-qtest-$(target)-y += tests/qom-test$(EXESUF))))
> +check-qtest-generic-y += tests/qom-test$(EXESUF)
Does this -generic- have the same filtering code to avoid running the
tests twice for x86_64, aarch64, ppc64, etc.? Please don't regress.
Regards,
Andreas
>
> check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
> comments.json empty.json enum-empty.json enum-missing-data.json \
>
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] tests: Simplify how qom-test is run
2015-09-18 12:53 ` Andreas Färber
@ 2015-09-18 14:24 ` Markus Armbruster
2015-09-18 15:28 ` Andreas Färber
0 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2015-09-18 14:24 UTC (permalink / raw)
To: Andreas Färber; +Cc: qemu-devel, stefanha, ehabkost
Andreas Färber <afaerber@suse.de> writes:
> Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
>> Add it to check-qtest-generic-y instead of check-qtest-$(target)-y for
>> every target.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> tests/Makefile | 5 +----
>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/tests/Makefile b/tests/Makefile
>> index 4559045..28c5f93 100644
>> --- a/tests/Makefile
>> +++ b/tests/Makefile
>> @@ -219,10 +219,7 @@ gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
>> check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>> check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>>
>> -# qom-test works for all sysemu architectures:
>> -$(foreach target,$(SYSEMU_TARGET_LIST), \
>> - $(if $(findstring tests/qom-test$(EXESUF),
>> $(check-qtest-$(target)-y)),, \
>> - $(eval check-qtest-$(target)-y += tests/qom-test$(EXESUF))))
>> +check-qtest-generic-y += tests/qom-test$(EXESUF)
>
> Does this -generic- have the same filtering code to avoid running the
> tests twice for x86_64, aarch64, ppc64, etc.? Please don't regress.
I'm dense today. Can you explain the filtering code to me?
[...]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] tests: Simplify how qom-test is run
2015-09-18 14:24 ` Markus Armbruster
@ 2015-09-18 15:28 ` Andreas Färber
2015-09-21 6:15 ` Markus Armbruster
2015-09-23 13:57 ` Markus Armbruster
0 siblings, 2 replies; 38+ messages in thread
From: Andreas Färber @ 2015-09-18 15:28 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, stefanha, ehabkost
Am 18.09.2015 um 16:24 schrieb Markus Armbruster:
> Andreas Färber <afaerber@suse.de> writes:
>> Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
>>> Add it to check-qtest-generic-y instead of check-qtest-$(target)-y for
>>> every target.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> tests/Makefile | 5 +----
>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/tests/Makefile b/tests/Makefile
>>> index 4559045..28c5f93 100644
>>> --- a/tests/Makefile
>>> +++ b/tests/Makefile
>>> @@ -219,10 +219,7 @@ gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
>>> check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>>> check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>>>
>>> -# qom-test works for all sysemu architectures:
>>> -$(foreach target,$(SYSEMU_TARGET_LIST), \
>>> - $(if $(findstring tests/qom-test$(EXESUF),
>>> $(check-qtest-$(target)-y)),, \
>>> - $(eval check-qtest-$(target)-y += tests/qom-test$(EXESUF))))
>>> +check-qtest-generic-y += tests/qom-test$(EXESUF)
>>
>> Does this -generic- have the same filtering code to avoid running the
>> tests twice for x86_64, aarch64, ppc64, etc.? Please don't regress.
>
> I'm dense today. Can you explain the filtering code to me?
For practical purpose,s x86_64 adds all tests from i386, that included
qom-test then. If we now add it for x86_64 too, it got executed twice,
which the above $(if ...) fixes by not adding it for x86_64 if it's
already in. Just checking whether -generic- has equivalent filtering or
other code somewhere else?
BR,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] tests: Simplify how qom-test is run
2015-09-18 15:28 ` Andreas Färber
@ 2015-09-21 6:15 ` Markus Armbruster
2015-09-23 13:57 ` Markus Armbruster
1 sibling, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2015-09-21 6:15 UTC (permalink / raw)
To: Andreas Färber; +Cc: qemu-devel, stefanha, ehabkost
Andreas Färber <afaerber@suse.de> writes:
> Am 18.09.2015 um 16:24 schrieb Markus Armbruster:
>> Andreas Färber <afaerber@suse.de> writes:
>>> Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
>>>> Add it to check-qtest-generic-y instead of check-qtest-$(target)-y for
>>>> every target.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>> tests/Makefile | 5 +----
>>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>>
>>>> diff --git a/tests/Makefile b/tests/Makefile
>>>> index 4559045..28c5f93 100644
>>>> --- a/tests/Makefile
>>>> +++ b/tests/Makefile
>>>> @@ -219,10 +219,7 @@ gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
>>>> check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>>>> check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>>>>
>>>> -# qom-test works for all sysemu architectures:
>>>> -$(foreach target,$(SYSEMU_TARGET_LIST), \
>>>> - $(if $(findstring tests/qom-test$(EXESUF),
>>>> $(check-qtest-$(target)-y)),, \
>>>> - $(eval check-qtest-$(target)-y += tests/qom-test$(EXESUF))))
>>>> +check-qtest-generic-y += tests/qom-test$(EXESUF)
>>>
>>> Does this -generic- have the same filtering code to avoid running the
>>> tests twice for x86_64, aarch64, ppc64, etc.? Please don't regress.
>>
>> I'm dense today. Can you explain the filtering code to me?
>
> For practical purpose,s x86_64 adds all tests from i386, that included
> qom-test then. If we now add it for x86_64 too, it got executed twice,
> which the above $(if ...) fixes by not adding it for x86_64 if it's
> already in. Just checking whether -generic- has equivalent filtering or
> other code somewhere else?
I'll double-check. Thanks!
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] tests: Simplify how qom-test is run
2015-09-18 15:28 ` Andreas Färber
2015-09-21 6:15 ` Markus Armbruster
@ 2015-09-23 13:57 ` Markus Armbruster
1 sibling, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2015-09-23 13:57 UTC (permalink / raw)
To: Andreas Färber; +Cc: qemu-devel, stefanha, ehabkost
Andreas Färber <afaerber@suse.de> writes:
> Am 18.09.2015 um 16:24 schrieb Markus Armbruster:
>> Andreas Färber <afaerber@suse.de> writes:
>>> Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
>>>> Add it to check-qtest-generic-y instead of check-qtest-$(target)-y for
>>>> every target.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>> tests/Makefile | 5 +----
>>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>>
>>>> diff --git a/tests/Makefile b/tests/Makefile
>>>> index 4559045..28c5f93 100644
>>>> --- a/tests/Makefile
>>>> +++ b/tests/Makefile
>>>> @@ -219,10 +219,7 @@ gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
>>>> check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>>>> check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>>>>
>>>> -# qom-test works for all sysemu architectures:
>>>> -$(foreach target,$(SYSEMU_TARGET_LIST), \
>>>> - $(if $(findstring tests/qom-test$(EXESUF),
>>>> $(check-qtest-$(target)-y)),, \
>>>> - $(eval check-qtest-$(target)-y += tests/qom-test$(EXESUF))))
>>>> +check-qtest-generic-y += tests/qom-test$(EXESUF)
>>>
>>> Does this -generic- have the same filtering code to avoid running the
>>> tests twice for x86_64, aarch64, ppc64, etc.? Please don't regress.
>>
>> I'm dense today. Can you explain the filtering code to me?
>
> For practical purpose,s x86_64 adds all tests from i386, that included
> qom-test then. If we now add it for x86_64 too, it got executed twice,
> which the above $(if ...) fixes by not adding it for x86_64 if it's
> already in. Just checking whether -generic- has equivalent filtering or
> other code somewhere else?
The code in master works only sometimes. Here's the explanation copied
from my revised patch's commit message:
We want to run qom-test for every architecture, without having to
manually add it to every architecture's list of tests. Commit 3687d53
accomplished this by adding it to every architecture's list
automatically.
However, some architectures inherit their tests from others, like this:
check-qtest-x86_64-y = $(check-qtest-i386-y)
check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
For such architectures, we ended up running the (slow!) test twice.
Commit 2b8419c attempted to avoid this by adding the test only when
it's not already present. Works only as long as we consider adding
the test to the architectures on the left hand side *after* the ones
on the right hand side: x86_64 after i386, microblazeel after
microblaze, xtensaeb after xtensa.
Turns out we consider them in $(SYSEMU_TARGET_LIST) order. Defined as
SYSEMU_TARGET_LIST := $(subst -softmmu.mak,,$(notdir \
$(wildcard $(SRC_PATH)/default-configs/*-softmmu.mak)))
On my machine, this results in the oder xtensa, x86_64, microblazeel,
microblaze, i386. Consequently, qom-test runs twice for microblazeel
and x86_64.
After my patch v2 (to be sent soon), it runs exactly once per target.
^ permalink raw reply [flat|nested] 38+ messages in thread