* [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work
@ 2017-03-03 12:32 Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 01/28] qga: Fix crash on non-dictionary QMP argument Markus Armbruster
` (28 more replies)
0 siblings, 29 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
v4:
* PATCH 05+06: New
* PATCH 07: Messing with the list of commands breaks when multiple
monitors race against each other, use two separate lists instead
(R-by dropped)
v3:
* PATCH 12+18+19: Commit message tidied up [Eric]
* PATCH 17: Spurious whitespace change dropped [Eric]
* PATCH 22: Missing visit_end_list() fixed [Eric]
* PATCH 23: Misplaced visit_end_list() fixed, unwanted test change
dropped [Eric]
v2:
* PATCH 19-20+22+24-25: New
* PATCH 03: Update MAINTAINERS new file
* PATCH 21+23: New test_visitor_in_fail_list_nested(), string visitor
patch tweaked, comments polished
* PATCH 23: full_name() bugs fixed
Markus Armbruster (28):
qga: Fix crash on non-dictionary QMP argument
libqtest: Work around a "QMP wants a newline" bug
qmp-test: New, covering basic QMP protocol
qmp: Dumb down how we run QMP command registration
qapi: Support multiple command registries per program
qapi-introspect: Mangle --prefix argument properly for C
qmp: Clean up how we enforce capability negotiation
qmp: Drop duplicated QMP command object checks
qmp: Eliminate silly QERR_QMP_* macros
qmp: Improve QMP dispatch error messages
qapi: Improve a QObject input visitor error message
qapi: Clean up after commit 3d344c2
qapi: Make QObject input visitor set *list reliably
qapi: Improve qobject input visitor error reporting
qapi: Drop string input visitor method optional()
qapi: Make string input and opts visitor require non-null input
qom: Make object_property_set_qobject()'s input visitor strict
test-qobject-input-visitor: Use strict visitor
qapi: Drop unused non-strict qobject input visitor
tests-qobject-input-strict: Merge into test-qobject-input-visitor
test-string-input-visitor: Tear down existing test automatically
test-string-input-visitor: Improve list coverage
tests: Cover partial input visit of list
test-qobject-input-visitor: Cover missing nested struct member
qapi: Make input visitors detect unvisited list tails
tests: Cover input visit beyond end of list
qapi: Fix object input visit beyond end of list
qapi: Improve qobject visitor documentation
MAINTAINERS | 1 +
block/nbd.c | 2 +-
block/nfs.c | 2 +-
block/ssh.c | 2 +-
docs/qapi-code-gen.txt | 2 +-
hw/ppc/spapr_drc.c | 5 +
include/monitor/monitor.h | 1 +
include/qapi/qmp/dispatch.h | 22 +-
include/qapi/qmp/qerror.h | 9 -
include/qapi/qobject-input-visitor.h | 40 +++-
include/qapi/qobject-output-visitor.h | 35 +++-
include/qapi/visitor-impl.h | 7 +-
include/qapi/visitor.h | 19 +-
include/qemu/module.h | 2 -
monitor.c | 178 ++++++----------
qapi/opts-visitor.c | 12 ++
qapi/qapi-visit-core.c | 8 +
qapi/qmp-dispatch.c | 39 +++-
qapi/qmp-registry.c | 37 ++--
qapi/qobject-input-visitor.c | 219 ++++++++++++-------
qapi/string-input-visitor.c | 97 +++++----
qapi/trace-events | 1 +
qga/commands.c | 2 +-
qga/guest-agent-core.h | 2 +
qga/main.c | 19 +-
qmp.c | 2 +-
qom/qom-qobject.c | 4 +-
scripts/qapi-commands.py | 19 +-
scripts/qapi-introspect.py | 2 +-
scripts/qapi-visit.py | 3 +
target/s390x/cpu_models.c | 2 +-
tests/Makefile.include | 9 +-
tests/check-qnull.c | 2 +-
tests/libqtest.c | 29 ++-
tests/libqtest.h | 8 +
tests/qmp-test.c | 139 +++++++++++++
tests/test-opts-visitor.c | 80 +++++++
tests/test-qga.c | 2 +-
tests/test-qmp-commands.c | 14 +-
tests/test-qobject-input-strict.c | 381 ----------------------------------
tests/test-qobject-input-visitor.c | 260 ++++++++++++++++++++++-
tests/test-string-input-visitor.c | 142 ++++++++++---
tests/test-visitor-serialization.c | 2 +-
trace-events | 1 -
vl.c | 2 +-
45 files changed, 1093 insertions(+), 773 deletions(-)
create mode 100644 tests/qmp-test.c
delete mode 100644 tests/test-qobject-input-strict.c
--
2.7.4
^ permalink raw reply [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 01/28] qga: Fix crash on non-dictionary QMP argument
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 02/28] libqtest: Work around a "QMP wants a newline" bug Markus Armbruster
` (27 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, Michael Roth
The value of key 'arguments' must be a JSON object. qemu-ga neglects
to check, and crashes. To reproduce, send
{ 'execute': 'guest-sync', 'arguments': [] }
to qemu-ga.
do_qmp_dispatch() uses qdict_get_qdict() to get the arguments. When
not a JSON object, this gets a null pointer, which flows through the
generated marshalling function to qobject_input_visitor_new(), where
it fails the assertion. qmp_dispatch_check_obj() needs to catch this
error.
QEMU isn't affected, because it runs qmp_check_input_obj() first,
which basically duplicates qmp_dispatch_check_obj()'s checks, plus the
missing one.
Fix by copying the missing one from qmp_check_input_obj() to
qmp_dispatch_check_obj().
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qapi/qmp-dispatch.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 48bec20..621922f 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -47,7 +47,13 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
return NULL;
}
has_exec_key = true;
- } else if (strcmp(arg_name, "arguments")) {
+ } else if (!strcmp(arg_name, "arguments")) {
+ if (qobject_type(arg_obj) != QTYPE_QDICT) {
+ error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT_MEMBER,
+ "arguments", "object");
+ return NULL;
+ }
+ } else {
error_setg(errp, QERR_QMP_EXTRA_MEMBER, arg_name);
return NULL;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 02/28] libqtest: Work around a "QMP wants a newline" bug
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 01/28] qga: Fix crash on non-dictionary QMP argument Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 03/28] qmp-test: New, covering basic QMP protocol Markus Armbruster
` (26 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
The next commit is going to add a test that calls qmp("null").
Curiously, this hangs. Here's why.
qmp_fd_sendv() doesn't send newlines. Not even when @fmt contains
some. At first glance, the QMP parser seems to be fine with that.
However, it turns out that it fails to react to input until it sees
either a newline, an object or an array. To reproduce, feed to a QMP
monitor like this:
$ echo -n 'null' | socat UNIX:/work/armbru/images/test-qmp STDIO
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 8, "major": 2}, "package": " (v2.8.0-1195-gf84141e-dirty)"}, "capabilities": []}}
No output after the greeting.
Add a newline:
$ echo 'null' | socat UNIX:/work/armbru/images/test-qmp STDIO
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 8, "major": 2}, "package": " (v2.8.0-1195-gf84141e-dirty)"}, "capabilities": []}}
{"error": {"class": "GenericError", "desc": "Expected 'object' in QMP input"}}
Correct output for input 'null'.
Add an object instead:
$ echo -n 'null { "execute": "qmp_capabilities" }' | socat UNIX:qmp-socket STDIO
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 8, "major": 2}, "package": " (v2.8.0-1195-gf84141e-dirty)"}, "capabilities": []}}
{"error": {"class": "GenericError", "desc": "Expected 'object' in QMP input"}}
{"return": {}}
Also correct output.
Work around this QMP bug by having qmp_fd_sendv() append a newline.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
tests/libqtest.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 3a0e0d6..951a3b4 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -442,14 +442,20 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
if (qobj) {
int log = getenv("QTEST_LOG") != NULL;
QString *qstr = qobject_to_json(qobj);
- const char *str = qstring_get_str(qstr);
- size_t size = qstring_get_length(qstr);
+ const char *str;
+
+ /*
+ * BUG: QMP doesn't react to input until it sees a newline, an
+ * object, or an array. Work-around: give it a newline.
+ */
+ qstring_append_chr(qstr, '\n');
+ str = qstring_get_str(qstr);
if (log) {
fprintf(stderr, "%s", str);
}
/* Send QMP request */
- socket_send(fd, str, size);
+ socket_send(fd, str, qstring_get_length(qstr));
QDECREF(qstr);
qobject_decref(qobj);
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 03/28] qmp-test: New, covering basic QMP protocol
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 01/28] qga: Fix crash on non-dictionary QMP argument Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 02/28] libqtest: Work around a "QMP wants a newline" bug Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 04/28] qmp: Dumb down how we run QMP command registration Markus Armbruster
` (25 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
MAINTAINERS | 1 +
tests/Makefile.include | 5 +-
tests/libqtest.c | 17 ++++--
tests/libqtest.h | 8 +++
tests/qmp-test.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 164 insertions(+), 6 deletions(-)
create mode 100644 tests/qmp-test.c
diff --git a/MAINTAINERS b/MAINTAINERS
index be79f68..d0ded4b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1393,6 +1393,7 @@ F: qmp.c
F: monitor.c
F: docs/*qmp-*
F: scripts/qmp/
+F: tests/qmp-test.c
T: git git://repo.or.cz/qemu/armbru.git qapi-next
Register API
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3310c17..4f52239 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -133,7 +133,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)
+check-qtest-generic-y = tests/qmp-test$(EXESUF)
+gcov-files-generic-y = monitor.c qapi/qmp-dispatch.c
+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
@@ -653,6 +655,7 @@ libqos-imx-obj-y = $(libqos-obj-y) tests/libqos/i2c-imx.o
libqos-usb-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) tests/libqos/usb.o
libqos-virtio-obj-y = $(libqos-spapr-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/qmp-test$(EXESUF): tests/qmp-test.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
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 951a3b4..ca6b641 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -149,7 +149,7 @@ void qtest_add_abrt_handler(GHookFunc fn, const void *data)
g_hook_prepend(&abrt_hooks, hook);
}
-QTestState *qtest_init(const char *extra_args)
+QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
{
QTestState *s;
int sock, qmpsock, i;
@@ -204,10 +204,6 @@ QTestState *qtest_init(const char *extra_args)
s->irq_level[i] = false;
}
- /* Read the QMP greeting and then do the handshake */
- qtest_qmp_discard_response(s, "");
- qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
-
if (getenv("QTEST_STOP")) {
kill(s->qemu_pid, SIGSTOP);
}
@@ -219,6 +215,17 @@ QTestState *qtest_init(const char *extra_args)
return s;
}
+QTestState *qtest_init(const char *extra_args)
+{
+ QTestState *s = qtest_init_without_qmp_handshake(extra_args);
+
+ /* Read the QMP greeting and then do the handshake */
+ qtest_qmp_discard_response(s, "");
+ qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
+
+ return s;
+}
+
void qtest_quit(QTestState *s)
{
qtest_instances = g_list_remove(qtest_instances, s);
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 90f182e..2c9962d 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -32,6 +32,14 @@ extern QTestState *global_qtest;
QTestState *qtest_init(const char *extra_args);
/**
+ * qtest_init_without_qmp_handshake:
+ * @extra_args: other arguments to pass to QEMU.
+ *
+ * Returns: #QTestState instance.
+ */
+QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
+
+/**
* qtest_quit:
* @s: #QTestState instance to operate on.
*
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
new file mode 100644
index 0000000..405e49e
--- /dev/null
+++ b/tests/qmp-test.c
@@ -0,0 +1,139 @@
+/*
+ * QMP protocol test cases
+ *
+ * Copyright (c) 2017 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.
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "qapi-visit.h"
+#include "qapi/error.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/visitor.h"
+
+const char common_args[] = "-nodefaults -machine none";
+
+static const char *get_error_class(QDict *resp)
+{
+ QDict *error = qdict_get_qdict(resp, "error");
+ const char *desc = qdict_get_try_str(error, "desc");
+
+ g_assert(desc);
+ return error ? qdict_get_try_str(error, "class") : NULL;
+}
+
+static void test_version(QObject *version)
+{
+ Visitor *v;
+ VersionInfo *vinfo;
+
+ g_assert(version);
+ v = qobject_input_visitor_new(version, true);
+ visit_type_VersionInfo(v, "version", &vinfo, &error_abort);
+ qapi_free_VersionInfo(vinfo);
+ visit_free(v);
+}
+
+static void test_malformed(void)
+{
+ QDict *resp;
+
+ /* Not even a dictionary */
+ resp = qmp("null");
+ g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
+ QDECREF(resp);
+
+ /* No "execute" key */
+ resp = qmp("{}");
+ g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
+ QDECREF(resp);
+
+ /* "execute" isn't a string */
+ resp = qmp("{ 'execute': true }");
+ g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
+ QDECREF(resp);
+
+ /* "arguments" isn't a dictionary */
+ resp = qmp("{ 'execute': 'no-such-cmd', 'arguments': [] }");
+ g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
+ QDECREF(resp);
+
+ /* extra key */
+ resp = qmp("{ 'execute': 'no-such-cmd', 'extra': true }");
+ g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
+ QDECREF(resp);
+}
+
+static void test_qmp_protocol(void)
+{
+ QDict *resp, *q, *ret;
+ QList *capabilities;
+
+ global_qtest = qtest_init_without_qmp_handshake(common_args);
+
+ /* Test greeting */
+ resp = qmp_receive();
+ q = qdict_get_qdict(resp, "QMP");
+ g_assert(q);
+ test_version(qdict_get(q, "version"));
+ capabilities = qdict_get_qlist(q, "capabilities");
+ g_assert(capabilities && qlist_empty(capabilities));
+ QDECREF(resp);
+
+ /* Test valid command before handshake */
+ resp = qmp("{ 'execute': 'query-version' }");
+ g_assert_cmpstr(get_error_class(resp), ==, "CommandNotFound");
+ QDECREF(resp);
+
+ /* Test malformed commands before handshake */
+ test_malformed();
+
+ /* Test handshake */
+ resp = qmp("{ 'execute': 'qmp_capabilities' }");
+ ret = qdict_get_qdict(resp, "return");
+ g_assert(ret && !qdict_size(ret));
+ QDECREF(resp);
+
+ /* Test repeated handshake */
+ resp = qmp("{ 'execute': 'qmp_capabilities' }");
+ g_assert_cmpstr(get_error_class(resp), ==, "CommandNotFound");
+ QDECREF(resp);
+
+ /* Test valid command */
+ resp = qmp("{ 'execute': 'query-version' }");
+ test_version(qdict_get(resp, "return"));
+ QDECREF(resp);
+
+ /* Test malformed commands */
+ test_malformed();
+
+ /* Test 'id' */
+ resp = qmp("{ 'execute': 'query-name', 'id': 'cookie#1' }");
+ ret = qdict_get_qdict(resp, "return");
+ g_assert(ret);
+ g_assert_cmpstr(qdict_get_try_str(resp, "id"), ==, "cookie#1");
+ QDECREF(resp);
+
+ /* Test command failure with 'id' */
+ resp = qmp("{ 'execute': 'human-monitor-command', 'id': 2 }");
+ g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
+ g_assert_cmpint(qdict_get_int(resp, "id"), ==, 2);
+ QDECREF(resp);
+
+ qtest_end();
+}
+
+int main(int argc, char *argv[])
+{
+ g_test_init(&argc, &argv, NULL);
+
+ qtest_add_func("qmp/protocol", test_qmp_protocol);
+
+ return g_test_run();
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 04/28] qmp: Dumb down how we run QMP command registration
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (2 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 03/28] qmp-test: New, covering basic QMP protocol Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 05/28] qapi: Support multiple command registries per program Markus Armbruster
` (24 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
The way we get QMP commands registered is high tech:
* qapi-commands.py generates qmp_init_marshal() that does the actual work
* it also generates the magic to register it as a MODULE_INIT_QAPI
function, so it runs when someone calls
module_call_init(MODULE_INIT_QAPI)
* main() calls module_call_init()
QEMU needs to register a few non-qapified commands. Same high tech
works: monitor.c has its own qmp_init_marshal() along with the magic
to make it run in module_call_init(MODULE_INIT_QAPI).
QEMU also needs to unregister commands that are not wanted in this
build's configuration (commit 5032a16). Simple enough:
qmp_unregister_commands_hack(). The difficulty is to make it run
after the generated qmp_init_marshal(). We can't simply run it in
monitor.c's qmp_init_marshal(), because the order in which the
registered functions run is indeterminate. So qmp_init_marshal()
registers qmp_unregister_commands_hack() separately. Since
registering *appends* to the list of registered functions, this will
make it run after all the functions that have been registered already.
I suspect it takes a long and expensive computer science education to
not find this silly.
Dumb it down as follows:
* Drop MODULE_INIT_QAPI entirely
* Give the generated qmp_init_marshal() external linkage.
* Call it instead of module_call_init(MODULE_INIT_QAPI)
* Except in QEMU proper, call new monitor_init_qmp_commands() that in
turn calls the generated qmp_init_marshal(), registers the
additional commands and unregisters the unwanted ones.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
include/monitor/monitor.h | 1 +
include/qemu/module.h | 2 --
monitor.c | 9 ++++-----
qga/main.c | 2 +-
scripts/qapi-commands.py | 5 ++---
tests/test-qmp-commands.c | 2 +-
vl.c | 2 +-
7 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index e64b944..d2b3aaf 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -16,6 +16,7 @@ extern Monitor *cur_mon;
bool monitor_cur_is_qmp(void);
+void monitor_init_qmp_commands(void);
void monitor_init(Chardev *chr, int flags);
void monitor_cleanup(void);
diff --git a/include/qemu/module.h b/include/qemu/module.h
index 877cca7..56dd218 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -42,7 +42,6 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void) \
typedef enum {
MODULE_INIT_BLOCK,
MODULE_INIT_OPTS,
- MODULE_INIT_QAPI,
MODULE_INIT_QOM,
MODULE_INIT_TRACE,
MODULE_INIT_MAX
@@ -50,7 +49,6 @@ typedef enum {
#define block_init(function) module_init(function, MODULE_INIT_BLOCK)
#define opts_init(function) module_init(function, MODULE_INIT_OPTS)
-#define qapi_init(function) module_init(function, MODULE_INIT_QAPI)
#define type_init(function) module_init(function, MODULE_INIT_QOM)
#define trace_init(function) module_init(function, MODULE_INIT_TRACE)
diff --git a/monitor.c b/monitor.c
index b68944d..53f5f5a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -997,8 +997,10 @@ static void qmp_unregister_commands_hack(void)
#endif
}
-static void qmp_init_marshal(void)
+void monitor_init_qmp_commands(void)
{
+ qmp_init_marshal();
+
qmp_register_command("query-qmp-schema", qmp_query_qmp_schema,
QCO_NO_OPTIONS);
qmp_register_command("device_add", qmp_device_add,
@@ -1006,12 +1008,9 @@ static void qmp_init_marshal(void)
qmp_register_command("netdev_add", qmp_netdev_add,
QCO_NO_OPTIONS);
- /* call it after the rest of qapi_init() */
- register_module_init(qmp_unregister_commands_hack, MODULE_INIT_QAPI);
+ qmp_unregister_commands_hack();
}
-qapi_init(qmp_init_marshal);
-
/* set the current CPU defined by the user */
int monitor_set_cpu(int cpu_index)
{
diff --git a/qga/main.c b/qga/main.c
index 538e4ee..6f8c614 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1321,7 +1321,7 @@ int main(int argc, char **argv)
config->log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
- module_call_init(MODULE_INIT_QAPI);
+ qmp_init_marshal();
init_dfl_pathnames();
config_load(config);
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 09e8467..a75946f 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -208,14 +208,12 @@ def gen_register_command(name, success_response):
def gen_registry(registry):
ret = mcgen('''
-static void qmp_init_marshal(void)
+void qmp_init_marshal(void)
{
''')
ret += registry
ret += mcgen('''
}
-
-qapi_init(qmp_init_marshal);
''')
return ret
@@ -308,6 +306,7 @@ fdecl.write(mcgen('''
#include "qapi/qmp/qdict.h"
#include "qapi/error.h"
+void qmp_init_marshal(void);
''',
prefix=prefix))
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index ff94481..c4e20d1 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -273,7 +273,7 @@ int main(int argc, char **argv)
g_test_add_func("/0.15/dealloc_types", test_dealloc_types);
g_test_add_func("/0.15/dealloc_partial", test_dealloc_partial);
- module_call_init(MODULE_INIT_QAPI);
+ qmp_init_marshal();
g_test_run();
return 0;
diff --git a/vl.c b/vl.c
index e10a27b..c6020b9 100644
--- a/vl.c
+++ b/vl.c
@@ -2987,7 +2987,7 @@ int main(int argc, char **argv, char **envp)
qemu_init_exec_dir(argv[0]);
module_call_init(MODULE_INIT_QOM);
- module_call_init(MODULE_INIT_QAPI);
+ monitor_init_qmp_commands();
qemu_add_opts(&qemu_drive_opts);
qemu_add_drive_opts(&qemu_legacy_drive_opts);
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 05/28] qapi: Support multiple command registries per program
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (3 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 04/28] qmp: Dumb down how we run QMP command registration Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 18:24 ` Eric Blake
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 06/28] qapi-introspect: Mangle --prefix argument properly for C Markus Armbruster
` (23 subsequent siblings)
28 siblings, 1 reply; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
The command registry encapsulates a single command list. Give the
functions using it a parameter instead. Define suitable command lists
in monitor, guest agent and test-qmp-commands.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/qapi/qmp/dispatch.h | 22 ++++++++++++++--------
monitor.c | 31 +++++++++++++++++--------------
qapi/qmp-dispatch.c | 17 +++++++++++++----
qapi/qmp-registry.c | 37 ++++++++++++++++++-------------------
qga/commands.c | 2 +-
qga/guest-agent-core.h | 2 ++
qga/main.c | 19 ++++++++++---------
scripts/qapi-commands.py | 16 ++++++++++------
tests/test-qmp-commands.c | 12 +++++++-----
9 files changed, 92 insertions(+), 66 deletions(-)
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 57651ea..20578dc 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -34,18 +34,24 @@ typedef struct QmpCommand
bool enabled;
} QmpCommand;
-void qmp_register_command(const char *name, QmpCommandFunc *fn,
- QmpCommandOptions options);
-void qmp_unregister_command(const char *name);
-QmpCommand *qmp_find_command(const char *name);
-QObject *qmp_dispatch(QObject *request);
-void qmp_disable_command(const char *name);
-void qmp_enable_command(const char *name);
+typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
+
+void qmp_register_command(QmpCommandList *cmds, const char *name,
+ QmpCommandFunc *fn, QmpCommandOptions options);
+void qmp_unregister_command(QmpCommandList *cmds, const char *name);
+QmpCommand *qmp_find_command(QmpCommandList *cmds, const char *name);
+QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request);
+void qmp_disable_command(QmpCommandList *cmds, const char *name);
+void qmp_enable_command(QmpCommandList *cmds, const char *name);
+
bool qmp_command_is_enabled(const QmpCommand *cmd);
const char *qmp_command_name(const QmpCommand *cmd);
bool qmp_has_success_response(const QmpCommand *cmd);
QObject *qmp_build_error_object(Error *err);
+
typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
-void qmp_for_each_command(qmp_cmd_callback_fn fn, void *opaque);
+
+void qmp_for_each_command(QmpCommandList *cmds, qmp_cmd_callback_fn fn,
+ void *opaque);
#endif
diff --git a/monitor.c b/monitor.c
index 53f5f5a..c7f7602 100644
--- a/monitor.c
+++ b/monitor.c
@@ -221,6 +221,8 @@ static int mon_refcount;
static mon_cmd_t mon_cmds[];
static mon_cmd_t info_cmds[];
+QmpCommandList qmp_commands;
+
Monitor *cur_mon;
static QEMUClockType event_clock_type = QEMU_CLOCK_REALTIME;
@@ -919,7 +921,7 @@ CommandInfoList *qmp_query_commands(Error **errp)
{
CommandInfoList *list = NULL;
- qmp_for_each_command(query_commands_cb, &list);
+ qmp_for_each_command(&qmp_commands, query_commands_cb, &list);
return list;
}
@@ -973,39 +975,40 @@ static void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
static void qmp_unregister_commands_hack(void)
{
#ifndef CONFIG_SPICE
- qmp_unregister_command("query-spice");
+ qmp_unregister_command(&qmp_commands, "query-spice");
#endif
#ifndef TARGET_I386
- qmp_unregister_command("rtc-reset-reinjection");
+ qmp_unregister_command(&qmp_commands, "rtc-reset-reinjection");
#endif
#ifndef TARGET_S390X
- qmp_unregister_command("dump-skeys");
+ qmp_unregister_command(&qmp_commands, "dump-skeys");
#endif
#ifndef TARGET_ARM
- qmp_unregister_command("query-gic-capabilities");
+ qmp_unregister_command(&qmp_commands, "query-gic-capabilities");
#endif
#if !defined(TARGET_S390X) && !defined(TARGET_I386)
- qmp_unregister_command("query-cpu-model-expansion");
+ qmp_unregister_command(&qmp_commands, "query-cpu-model-expansion");
#endif
#if !defined(TARGET_S390X)
- qmp_unregister_command("query-cpu-model-baseline");
- qmp_unregister_command("query-cpu-model-comparison");
+ qmp_unregister_command(&qmp_commands, "query-cpu-model-baseline");
+ qmp_unregister_command(&qmp_commands, "query-cpu-model-comparison");
#endif
#if !defined(TARGET_PPC) && !defined(TARGET_ARM) && !defined(TARGET_I386) \
&& !defined(TARGET_S390X)
- qmp_unregister_command("query-cpu-definitions");
+ qmp_unregister_command(&qmp_commands, "query-cpu-definitions");
#endif
}
void monitor_init_qmp_commands(void)
{
- qmp_init_marshal();
+ qmp_init_marshal(&qmp_commands);
- qmp_register_command("query-qmp-schema", qmp_query_qmp_schema,
+ qmp_register_command(&qmp_commands, "query-qmp-schema",
+ qmp_query_qmp_schema,
QCO_NO_OPTIONS);
- qmp_register_command("device_add", qmp_device_add,
+ qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
QCO_NO_OPTIONS);
- qmp_register_command("netdev_add", qmp_netdev_add,
+ qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_add,
QCO_NO_OPTIONS);
qmp_unregister_commands_hack();
@@ -3787,7 +3790,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
goto err_out;
}
- rsp = qmp_dispatch(req);
+ rsp = qmp_dispatch(&qmp_commands, req);
err_out:
if (err) {
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 621922f..95a0f48 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -67,7 +67,11 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
return dict;
}
-static QObject *do_qmp_dispatch(QObject *request, Error **errp)
+volatile QmpCommand *save_cmd;
+QmpCommand cmd2;
+
+static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
+ Error **errp)
{
Error *local_err = NULL;
const char *command;
@@ -81,7 +85,7 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp)
}
command = qdict_get_str(dict, "execute");
- cmd = qmp_find_command(command);
+ cmd = qmp_find_command(cmds, command);
if (cmd == NULL) {
error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
"The command %s has not been found", command);
@@ -93,6 +97,9 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp)
return NULL;
}
+ assert(!cmd->options & QCO_NO_SUCCESS_RESP);
+ save_cmd = cmd;
+ cmd2 = *cmd;
if (!qdict_haskey(dict, "arguments")) {
args = qdict_new();
} else {
@@ -111,6 +118,8 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp)
QDECREF(args);
+ assert(!cmd->options & QCO_NO_SUCCESS_RESP);
+ assert(ret || local_err);
return ret;
}
@@ -121,13 +130,13 @@ QObject *qmp_build_error_object(Error *err)
error_get_pretty(err));
}
-QObject *qmp_dispatch(QObject *request)
+QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request)
{
Error *err = NULL;
QObject *ret;
QDict *rsp;
- ret = do_qmp_dispatch(request, &err);
+ ret = do_qmp_dispatch(cmds, request, &err);
rsp = qdict_new();
if (err) {
diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
index e805368..5af484c 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -15,11 +15,8 @@
#include "qemu/osdep.h"
#include "qapi/qmp/dispatch.h"
-static QTAILQ_HEAD(QmpCommandList, QmpCommand) qmp_commands =
- QTAILQ_HEAD_INITIALIZER(qmp_commands);
-
-void qmp_register_command(const char *name, QmpCommandFunc *fn,
- QmpCommandOptions options)
+void qmp_register_command(QmpCommandList *cmds, const char *name,
+ QmpCommandFunc *fn, QmpCommandOptions options)
{
QmpCommand *cmd = g_malloc0(sizeof(*cmd));
@@ -27,22 +24,22 @@ void qmp_register_command(const char *name, QmpCommandFunc *fn,
cmd->fn = fn;
cmd->enabled = true;
cmd->options = options;
- QTAILQ_INSERT_TAIL(&qmp_commands, cmd, node);
+ QTAILQ_INSERT_TAIL(cmds, cmd, node);
}
-void qmp_unregister_command(const char *name)
+void qmp_unregister_command(QmpCommandList *cmds, const char *name)
{
- QmpCommand *cmd = qmp_find_command(name);
+ QmpCommand *cmd = qmp_find_command(cmds, name);
- QTAILQ_REMOVE(&qmp_commands, cmd, node);
+ QTAILQ_REMOVE(cmds, cmd, node);
g_free(cmd);
}
-QmpCommand *qmp_find_command(const char *name)
+QmpCommand *qmp_find_command(QmpCommandList *cmds, const char *name)
{
QmpCommand *cmd;
- QTAILQ_FOREACH(cmd, &qmp_commands, node) {
+ QTAILQ_FOREACH(cmd, cmds, node) {
if (strcmp(cmd->name, name) == 0) {
return cmd;
}
@@ -50,11 +47,12 @@ QmpCommand *qmp_find_command(const char *name)
return NULL;
}
-static void qmp_toggle_command(const char *name, bool enabled)
+static void qmp_toggle_command(QmpCommandList *cmds, const char *name,
+ bool enabled)
{
QmpCommand *cmd;
- QTAILQ_FOREACH(cmd, &qmp_commands, node) {
+ QTAILQ_FOREACH(cmd, cmds, node) {
if (strcmp(cmd->name, name) == 0) {
cmd->enabled = enabled;
return;
@@ -62,14 +60,14 @@ static void qmp_toggle_command(const char *name, bool enabled)
}
}
-void qmp_disable_command(const char *name)
+void qmp_disable_command(QmpCommandList *cmds, const char *name)
{
- qmp_toggle_command(name, false);
+ qmp_toggle_command(cmds, name, false);
}
-void qmp_enable_command(const char *name)
+void qmp_enable_command(QmpCommandList *cmds, const char *name)
{
- qmp_toggle_command(name, true);
+ qmp_toggle_command(cmds, name, true);
}
bool qmp_command_is_enabled(const QmpCommand *cmd)
@@ -87,11 +85,12 @@ bool qmp_has_success_response(const QmpCommand *cmd)
return !(cmd->options & QCO_NO_SUCCESS_RESP);
}
-void qmp_for_each_command(qmp_cmd_callback_fn fn, void *opaque)
+void qmp_for_each_command(QmpCommandList *cmds, qmp_cmd_callback_fn fn,
+ void *opaque)
{
QmpCommand *cmd;
- QTAILQ_FOREACH(cmd, &qmp_commands, node) {
+ QTAILQ_FOREACH(cmd, cmds, node) {
fn(cmd, opaque);
}
}
diff --git a/qga/commands.c b/qga/commands.c
index edd3e83..4d92946 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -75,7 +75,7 @@ struct GuestAgentInfo *qmp_guest_info(Error **errp)
GuestAgentInfo *info = g_new0(GuestAgentInfo, 1);
info->version = g_strdup(QEMU_VERSION);
- qmp_for_each_command(qmp_command_info, info);
+ qmp_for_each_command(&ga_commands, qmp_command_info, info);
return info;
}
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index 63e9d39..3e8a4ac 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -18,7 +18,9 @@
typedef struct GAState GAState;
typedef struct GACommandState GACommandState;
+
extern GAState *ga_state;
+extern QmpCommandList ga_commands;
GList *ga_command_blacklist_init(GList *blacklist);
void ga_command_state_init(GAState *s, GACommandState *cs);
diff --git a/qga/main.c b/qga/main.c
index 6f8c614..9435bc7 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -92,6 +92,7 @@ struct GAState {
};
struct GAState *ga_state;
+QmpCommandList ga_commands;
/* commands that are safe to issue while filesystems are frozen */
static const char *ga_freeze_whitelist[] = {
@@ -370,7 +371,7 @@ static void ga_disable_non_whitelisted(QmpCommand *cmd, void *opaque)
}
if (!whitelisted) {
g_debug("disabling command: %s", name);
- qmp_disable_command(name);
+ qmp_disable_command(&ga_commands, name);
}
}
@@ -383,7 +384,7 @@ static void ga_enable_non_blacklisted(QmpCommand *cmd, void *opaque)
if (g_list_find_custom(blacklist, name, ga_strcmp) == NULL &&
!qmp_command_is_enabled(cmd)) {
g_debug("enabling command: %s", name);
- qmp_enable_command(name);
+ qmp_enable_command(&ga_commands, name);
}
}
@@ -420,7 +421,7 @@ void ga_set_frozen(GAState *s)
return;
}
/* disable all non-whitelisted (for frozen state) commands */
- qmp_for_each_command(ga_disable_non_whitelisted, NULL);
+ qmp_for_each_command(&ga_commands, ga_disable_non_whitelisted, NULL);
g_warning("disabling logging due to filesystem freeze");
ga_disable_logging(s);
s->frozen = true;
@@ -456,7 +457,7 @@ void ga_unset_frozen(GAState *s)
}
/* enable all disabled, non-blacklisted commands */
- qmp_for_each_command(ga_enable_non_blacklisted, s->blacklist);
+ qmp_for_each_command(&ga_commands, ga_enable_non_blacklisted, s->blacklist);
s->frozen = false;
if (!ga_delete_file(s->state_filepath_isfrozen)) {
g_warning("unable to delete %s, fsfreeze may not function properly",
@@ -555,7 +556,7 @@ static void process_command(GAState *s, QDict *req)
g_assert(req);
g_debug("processing command");
- rsp = qmp_dispatch(QOBJECT(req));
+ rsp = qmp_dispatch(&ga_commands, QOBJECT(req));
if (rsp) {
ret = send_response(s, rsp);
if (ret < 0) {
@@ -1119,7 +1120,7 @@ static void config_parse(GAConfig *config, int argc, char **argv)
break;
case 'b': {
if (is_help_option(optarg)) {
- qmp_for_each_command(ga_print_cmd, NULL);
+ qmp_for_each_command(&ga_commands, ga_print_cmd, NULL);
exit(EXIT_SUCCESS);
}
config->blacklist = g_list_concat(config->blacklist,
@@ -1247,7 +1248,7 @@ static int run_agent(GAState *s, GAConfig *config)
s->deferred_options.log_filepath = config->log_filepath;
}
ga_disable_logging(s);
- qmp_for_each_command(ga_disable_non_whitelisted, NULL);
+ qmp_for_each_command(&ga_commands, ga_disable_non_whitelisted, NULL);
} else {
if (config->daemonize) {
become_daemon(config->pid_filepath);
@@ -1277,7 +1278,7 @@ static int run_agent(GAState *s, GAConfig *config)
s->blacklist = config->blacklist;
do {
g_debug("disabling command: %s", (char *)l->data);
- qmp_disable_command(l->data);
+ qmp_disable_command(&ga_commands, l->data);
l = g_list_next(l);
} while (l);
}
@@ -1321,7 +1322,7 @@ int main(int argc, char **argv)
config->log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
- qmp_init_marshal();
+ qga_qmp_init_marshal(&ga_commands);
init_dfl_pathnames();
config_load(config);
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index a75946f..2b062ad 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -198,7 +198,8 @@ def gen_register_command(name, success_response):
options = 'QCO_NO_SUCCESS_RESP'
ret = mcgen('''
- qmp_register_command("%(name)s", qmp_marshal_%(c_name)s, %(opts)s);
+ qmp_register_command(cmds, "%(name)s",
+ qmp_marshal_%(c_name)s, %(opts)s);
''',
name=name, c_name=c_name(name),
opts=options)
@@ -208,9 +209,12 @@ def gen_register_command(name, success_response):
def gen_registry(registry):
ret = mcgen('''
-void qmp_init_marshal(void)
+void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds)
{
-''')
+ QTAILQ_INIT(cmds);
+
+''',
+ c_prefix=c_name(prefix, protect=False))
ret += registry
ret += mcgen('''
}
@@ -289,7 +293,6 @@ fdef.write(mcgen('''
#include "qemu-common.h"
#include "qemu/module.h"
#include "qapi/qmp/types.h"
-#include "qapi/qmp/dispatch.h"
#include "qapi/visitor.h"
#include "qapi/qobject-output-visitor.h"
#include "qapi/qobject-input-visitor.h"
@@ -304,11 +307,12 @@ fdef.write(mcgen('''
fdecl.write(mcgen('''
#include "%(prefix)sqapi-types.h"
#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/dispatch.h"
#include "qapi/error.h"
-void qmp_init_marshal(void);
+void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
''',
- prefix=prefix))
+ prefix=prefix, c_prefix=c_name(prefix, protect=False)))
schema = QAPISchema(input_file)
gen = QAPISchemaGenCommandVisitor()
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index c4e20d1..1cf413b 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -8,6 +8,8 @@
#include "tests/test-qapi-types.h"
#include "tests/test-qapi-visit.h"
+static QmpCommandList qmp_commands;
+
void qmp_user_def_cmd(Error **errp)
{
}
@@ -94,7 +96,7 @@ static void test_dispatch_cmd(void)
qdict_put_obj(req, "execute", QOBJECT(qstring_from_str("user_def_cmd")));
- resp = qmp_dispatch(QOBJECT(req));
+ resp = qmp_dispatch(&qmp_commands, QOBJECT(req));
assert(resp != NULL);
assert(!qdict_haskey(qobject_to_qdict(resp), "error"));
@@ -111,7 +113,7 @@ static void test_dispatch_cmd_failure(void)
qdict_put_obj(req, "execute", QOBJECT(qstring_from_str("user_def_cmd2")));
- resp = qmp_dispatch(QOBJECT(req));
+ resp = qmp_dispatch(&qmp_commands, QOBJECT(req));
assert(resp != NULL);
assert(qdict_haskey(qobject_to_qdict(resp), "error"));
@@ -125,7 +127,7 @@ static void test_dispatch_cmd_failure(void)
qdict_put_obj(req, "execute", QOBJECT(qstring_from_str("user_def_cmd")));
- resp = qmp_dispatch(QOBJECT(req));
+ resp = qmp_dispatch(&qmp_commands, QOBJECT(req));
assert(resp != NULL);
assert(qdict_haskey(qobject_to_qdict(resp), "error"));
@@ -139,7 +141,7 @@ static QObject *test_qmp_dispatch(QDict *req)
QDict *resp;
QObject *ret;
- resp_obj = qmp_dispatch(QOBJECT(req));
+ resp_obj = qmp_dispatch(&qmp_commands, QOBJECT(req));
assert(resp_obj);
resp = qobject_to_qdict(resp_obj);
assert(resp && !qdict_haskey(resp, "error"));
@@ -273,7 +275,7 @@ int main(int argc, char **argv)
g_test_add_func("/0.15/dealloc_types", test_dealloc_types);
g_test_add_func("/0.15/dealloc_partial", test_dealloc_partial);
- qmp_init_marshal();
+ test_qmp_init_marshal(&qmp_commands);
g_test_run();
return 0;
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 06/28] qapi-introspect: Mangle --prefix argument properly for C
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (4 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 05/28] qapi: Support multiple command registries per program Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 18:29 ` Eric Blake
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 07/28] qmp: Clean up how we enforce capability negotiation Markus Armbruster
` (22 subsequent siblings)
28 siblings, 1 reply; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
scripts/qapi-introspect.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
index 541644e..fb72c61 100644
--- a/scripts/qapi-introspect.py
+++ b/scripts/qapi-introspect.py
@@ -64,7 +64,7 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
# generate C
# TODO can generate awfully long lines
jsons.extend(self._jsons)
- name = prefix + 'qmp_schema_json'
+ name = c_name(prefix, protect=False) + 'qmp_schema_json'
self.decl = mcgen('''
extern const char %(c_name)s[];
''',
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 07/28] qmp: Clean up how we enforce capability negotiation
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (5 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 06/28] qapi-introspect: Mangle --prefix argument properly for C Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 18:40 ` Eric Blake
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 08/28] qmp: Drop duplicated QMP command object checks Markus Armbruster
` (21 subsequent siblings)
28 siblings, 1 reply; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
To enforce capability negotiation before normal operation,
handle_qmp_command() inspects every command before it's handed off to
qmp_dispatch(). This is a bit of a layering violation, and results in
duplicated code.
Before capability negotiation (!cur_mon->in_command_mode), we fail
commands other than "qmp_capabilities". This is what enforces
capability negotiation.
Afterwards, we fail command "qmp_capabilities".
Clean this up as follows.
The obvious place to fail a command is the command itself, so move the
"afterwards" check to qmp_qmp_capabilities().
We do the "before" check in every other command, but that would be
bothersome. Instead, start with an alternate list of commant that
contains only "qmp_capabilities". Switch to the full list in
qmp_qmp_capabilities().
Additionally, replace the generic human-readable error message for
CommandNotFound by one that reminds the user to run qmp_capabilities.
Without that, we'd regress commit 2d5a834.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
monitor.c | 76 +++++++++++++++++++++++++++++++++++----------------------------
1 file changed, 42 insertions(+), 34 deletions(-)
diff --git a/monitor.c b/monitor.c
index c7f7602..b540096 100644
--- a/monitor.c
+++ b/monitor.c
@@ -165,7 +165,7 @@ typedef struct {
* When command qmp_capabilities succeeds, we go into command
* mode.
*/
- bool in_command_mode; /* are we in command mode? */
+ QmpCommandList *commands;
} MonitorQMP;
/*
@@ -221,7 +221,7 @@ static int mon_refcount;
static mon_cmd_t mon_cmds[];
static mon_cmd_t info_cmds[];
-QmpCommandList qmp_commands;
+QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
Monitor *cur_mon;
@@ -416,7 +416,8 @@ static void monitor_qapi_event_emit(QAPIEvent event, QDict *qdict)
trace_monitor_protocol_event_emit(event, qdict);
QLIST_FOREACH(mon, &mon_list, entry) {
- if (monitor_is_qmp(mon) && mon->qmp.in_command_mode) {
+ if (monitor_is_qmp(mon)
+ && mon->qmp.commands != &qmp_cap_negotiation_commands) {
monitor_json_emitter(mon, QOBJECT(qdict));
}
}
@@ -565,11 +566,6 @@ static void monitor_qapi_event_init(void)
qmp_event_set_func_emit(monitor_qapi_event_queue);
}
-void qmp_qmp_capabilities(Error **errp)
-{
- cur_mon->qmp.in_command_mode = true;
-}
-
static void handle_hmp_command(Monitor *mon, const char *cmdline);
static void monitor_data_init(Monitor *mon)
@@ -921,7 +917,7 @@ CommandInfoList *qmp_query_commands(Error **errp)
{
CommandInfoList *list = NULL;
- qmp_for_each_command(&qmp_commands, query_commands_cb, &list);
+ qmp_for_each_command(cur_mon->qmp.commands, query_commands_cb, &list);
return list;
}
@@ -1001,6 +997,13 @@ static void qmp_unregister_commands_hack(void)
void monitor_init_qmp_commands(void)
{
+ /*
+ * Two command lists:
+ * - qmp_commands contains all QMP commands
+ * - qmp_cap_negotiation_commands contains just
+ * "qmp_capabilities", to enforce capability negotiation
+ */
+
qmp_init_marshal(&qmp_commands);
qmp_register_command(&qmp_commands, "query-qmp-schema",
@@ -1012,6 +1015,22 @@ void monitor_init_qmp_commands(void)
QCO_NO_OPTIONS);
qmp_unregister_commands_hack();
+
+ QTAILQ_INIT(&qmp_cap_negotiation_commands);
+ qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
+ qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS);
+}
+
+void qmp_qmp_capabilities(Error **errp)
+{
+ if (cur_mon->qmp.commands == &qmp_commands) {
+ error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
+ "Capabilities negotiation is already complete, command "
+ "ignored");
+ return;
+ }
+
+ cur_mon->qmp.commands = &qmp_commands;
}
/* set the current CPU defined by the user */
@@ -3681,26 +3700,6 @@ static int monitor_can_read(void *opaque)
return (mon->suspend_cnt == 0) ? 1 : 0;
}
-static bool invalid_qmp_mode(const Monitor *mon, const char *cmd,
- Error **errp)
-{
- bool is_cap = g_str_equal(cmd, "qmp_capabilities");
-
- if (is_cap && mon->qmp.in_command_mode) {
- error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
- "Capabilities negotiation is already complete, command "
- "'%s' ignored", cmd);
- return true;
- }
- if (!is_cap && !mon->qmp.in_command_mode) {
- error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
- "Expecting capabilities negotiation with "
- "'qmp_capabilities' before command '%s'", cmd);
- return true;
- }
- return false;
-}
-
/*
* Input object checking rules
*
@@ -3786,11 +3785,20 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
cmd_name = qdict_get_str(qdict, "execute");
trace_handle_qmp_command(mon, cmd_name);
- if (invalid_qmp_mode(mon, cmd_name, &err)) {
- goto err_out;
- }
+ rsp = qmp_dispatch(cur_mon->qmp.commands, req);
- rsp = qmp_dispatch(&qmp_commands, req);
+ qdict = qdict_get_qdict(qobject_to_qdict(rsp), "error");
+ if (qdict) {
+ if (mon->qmp.commands == &qmp_cap_negotiation_commands
+ && !g_strcmp0(qdict_get_try_str(qdict, "class"),
+ QapiErrorClass_lookup[ERROR_CLASS_COMMAND_NOT_FOUND])) {
+ /* Provide a more useful error message */
+ qdict_del(qdict, "desc");
+ qdict_put(qdict, "desc",
+ qstring_from_str("Expecting capabilities negotiation"
+ " with 'qmp_capabilities'"));
+ }
+ }
err_out:
if (err) {
@@ -3888,7 +3896,7 @@ static void monitor_qmp_event(void *opaque, int event)
switch (event) {
case CHR_EVENT_OPENED:
- mon->qmp.in_command_mode = false;
+ mon->qmp.commands = &qmp_cap_negotiation_commands;
data = get_qmp_greeting();
monitor_json_emitter(mon, data);
qobject_decref(data);
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 08/28] qmp: Drop duplicated QMP command object checks
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (6 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 07/28] qmp: Clean up how we enforce capability negotiation Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 09/28] qmp: Eliminate silly QERR_QMP_* macros Markus Armbruster
` (20 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
qmp_check_input_obj() duplicates qmp_dispatch_check_obj(), except the
latter screws up an error message. handle_qmp_command() runs first
the former, then the latter via qmp_dispatch(), masking the screwup.
qemu-ga also masks the screwup, because it also duplicates checks,
just differently.
qmp_check_input_obj() exists because handle_qmp_command() needs to
examine the command before dispatching it. The previous commit got
rid of this need, except for a tracepoint, and a bit of "id" code that
relies on qdict not being null.
Fix up the error message in qmp_dispatch_check_obj(), drop
qmp_check_input_obj() and the tracepoint. Protect the "id" code with
a conditional.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
monitor.c | 74 +++++------------------------------------------------
qapi/qmp-dispatch.c | 3 +--
trace-events | 1 -
3 files changed, 7 insertions(+), 71 deletions(-)
diff --git a/monitor.c b/monitor.c
index b540096..0a65734 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3700,67 +3700,10 @@ static int monitor_can_read(void *opaque)
return (mon->suspend_cnt == 0) ? 1 : 0;
}
-/*
- * Input object checking rules
- *
- * 1. Input object must be a dict
- * 2. The "execute" key must exist
- * 3. The "execute" key must be a string
- * 4. If the "arguments" key exists, it must be a dict
- * 5. If the "id" key exists, it can be anything (ie. json-value)
- * 6. Any argument not listed above is considered invalid
- */
-static QDict *qmp_check_input_obj(QObject *input_obj, Error **errp)
-{
- const QDictEntry *ent;
- int has_exec_key = 0;
- QDict *input_dict;
-
- input_dict = qobject_to_qdict(input_obj);
- if (!input_dict) {
- error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT, "object");
- return NULL;
- }
-
-
- for (ent = qdict_first(input_dict); ent; ent = qdict_next(input_dict, ent)){
- const char *arg_name = qdict_entry_key(ent);
- const QObject *arg_obj = qdict_entry_value(ent);
-
- if (!strcmp(arg_name, "execute")) {
- if (qobject_type(arg_obj) != QTYPE_QSTRING) {
- error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT_MEMBER,
- "execute", "string");
- return NULL;
- }
- has_exec_key = 1;
- } else if (!strcmp(arg_name, "arguments")) {
- if (qobject_type(arg_obj) != QTYPE_QDICT) {
- error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT_MEMBER,
- "arguments", "object");
- return NULL;
- }
- } else if (!strcmp(arg_name, "id")) {
- /* Any string is acceptable as "id", so nothing to check */
- } else {
- error_setg(errp, QERR_QMP_EXTRA_MEMBER, arg_name);
- return NULL;
- }
- }
-
- if (!has_exec_key) {
- error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT, "execute");
- return NULL;
- }
-
- return input_dict;
-}
-
static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
{
QObject *req, *rsp = NULL, *id = NULL;
QDict *qdict = NULL;
- const char *cmd_name;
Monitor *mon = cur_mon;
Error *err = NULL;
@@ -3773,17 +3716,12 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
goto err_out;
}
- qdict = qmp_check_input_obj(req, &err);
- if (!qdict) {
- goto err_out;
- }
-
- id = qdict_get(qdict, "id");
- qobject_incref(id);
- qdict_del(qdict, "id");
-
- cmd_name = qdict_get_str(qdict, "execute");
- trace_handle_qmp_command(mon, cmd_name);
+ qdict = qobject_to_qdict(req);
+ if (qdict) {
+ id = qdict_get(qdict, "id");
+ qobject_incref(id);
+ qdict_del(qdict, "id");
+ } /* else will fail qmp_dispatch() */
rsp = qmp_dispatch(cur_mon->qmp.commands, req);
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 95a0f48..2ce6dfb 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -30,8 +30,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
dict = qobject_to_qdict(request);
if (!dict) {
- error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT,
- "request is not a dictionary");
+ error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT, "object");
return NULL;
}
diff --git a/trace-events b/trace-events
index 7288557..b07a09b 100644
--- a/trace-events
+++ b/trace-events
@@ -65,7 +65,6 @@ xen_remap_bucket(uint64_t index) "index %#"PRIx64
xen_map_cache_return(void* ptr) "%p"
# monitor.c
-handle_qmp_command(void *mon, const char *cmd_name) "mon %p cmd_name \"%s\""
monitor_protocol_event_handler(uint32_t event, void *qdict) "event=%d data=%p"
monitor_protocol_event_emit(uint32_t event, void *data) "event=%d data=%p"
monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) "event=%d data=%p rate=%" PRId64
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 09/28] qmp: Eliminate silly QERR_QMP_* macros
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (7 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 08/28] qmp: Drop duplicated QMP command object checks Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 10/28] qmp: Improve QMP dispatch error messages Markus Armbruster
` (19 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
The QERR_ macros are leftovers from the days of "rich" error objects.
QERR_QMP_BAD_INPUT_OBJECT, QERR_QMP_BAD_INPUT_OBJECT_MEMBER,
QERR_QMP_EXTRA_MEMBER are used in just one place now, except for one
use that has crept into qobject-input-visitor.c.
Drop these macros, to make the (bad) error messages more visible.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
include/qapi/qmp/qerror.h | 9 ---------
qapi/qmp-dispatch.c | 13 +++++++------
qapi/qobject-input-visitor.c | 3 ++-
3 files changed, 9 insertions(+), 16 deletions(-)
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 6586c9f..c82360f 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -82,15 +82,6 @@
#define QERR_QGA_COMMAND_FAILED \
"Guest agent command failed, error was '%s'"
-#define QERR_QMP_BAD_INPUT_OBJECT \
- "Expected '%s' in QMP input"
-
-#define QERR_QMP_BAD_INPUT_OBJECT_MEMBER \
- "QMP input object member '%s' expects '%s'"
-
-#define QERR_QMP_EXTRA_MEMBER \
- "QMP input object member '%s' is unexpected"
-
#define QERR_SET_PASSWD_FAILED \
"Could not set password"
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 2ce6dfb..23b0528 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -30,7 +30,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
dict = qobject_to_qdict(request);
if (!dict) {
- error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT, "object");
+ error_setg(errp, "Expected '%s' in QMP input", "object");
return NULL;
}
@@ -41,25 +41,26 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
if (!strcmp(arg_name, "execute")) {
if (qobject_type(arg_obj) != QTYPE_QSTRING) {
- error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "execute",
- "string");
+ error_setg(errp, "QMP input object member '%s' expects '%s'",
+ "execute", "string");
return NULL;
}
has_exec_key = true;
} else if (!strcmp(arg_name, "arguments")) {
if (qobject_type(arg_obj) != QTYPE_QDICT) {
- error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT_MEMBER,
+ error_setg(errp, "QMP input object member '%s' expects '%s'",
"arguments", "object");
return NULL;
}
} else {
- error_setg(errp, QERR_QMP_EXTRA_MEMBER, arg_name);
+ error_setg(errp, "QMP input object member '%s' is unexpected",
+ arg_name);
return NULL;
}
}
if (!has_exec_key) {
- error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT, "execute");
+ error_setg(errp, "Expected '%s' in QMP input", "execute");
return NULL;
}
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 0063327..ed6c24c 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -140,7 +140,8 @@ static void qobject_input_check_struct(Visitor *v, Error **errp)
g_hash_table_iter_init(&iter, top_ht);
if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
- error_setg(errp, QERR_QMP_EXTRA_MEMBER, key);
+ error_setg(errp, "QMP input object member '%s' is unexpected",
+ key);
}
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 10/28] qmp: Improve QMP dispatch error messages
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (8 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 09/28] qmp: Eliminate silly QERR_QMP_* macros Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 19:55 ` Philippe Mathieu-Daudé
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 11/28] qapi: Improve a QObject input visitor error message Markus Armbruster
` (18 subsequent siblings)
28 siblings, 1 reply; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qapi/qmp-dispatch.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 23b0528..578c6d8 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -30,7 +30,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
dict = qobject_to_qdict(request);
if (!dict) {
- error_setg(errp, "Expected '%s' in QMP input", "object");
+ error_setg(errp, "QMP input must be a JSON object");
return NULL;
}
@@ -41,15 +41,17 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
if (!strcmp(arg_name, "execute")) {
if (qobject_type(arg_obj) != QTYPE_QSTRING) {
- error_setg(errp, "QMP input object member '%s' expects '%s'",
- "execute", "string");
+ error_setg(errp,
+ "QMP input object member '%s' must be %s",
+ "execute", "a string");
return NULL;
}
has_exec_key = true;
} else if (!strcmp(arg_name, "arguments")) {
if (qobject_type(arg_obj) != QTYPE_QDICT) {
- error_setg(errp, "QMP input object member '%s' expects '%s'",
- "arguments", "object");
+ error_setg(errp,
+ "QMP input object member '%s' must be %s",
+ "arguments", "an object");
return NULL;
}
} else {
@@ -60,7 +62,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
}
if (!has_exec_key) {
- error_setg(errp, "Expected '%s' in QMP input", "execute");
+ error_setg(errp, "QMP input object lacks key 'execute'");
return NULL;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 11/28] qapi: Improve a QObject input visitor error message
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (9 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 10/28] qmp: Improve QMP dispatch error messages Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 12/28] qapi: Clean up after commit 3d344c2 Markus Armbruster
` (17 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
The QObject input visitor has three error message formats:
* Parameter '%s' is missing
* "Invalid parameter type for '%s', expected: %s"
* "QMP input object member '%s' is unexpected"
The '%s' are member names (or "null", but I'll fix that later).
The last error message calls the thing "QMP input object member"
instead of "parameter". Misleading when the visitor is used on
QObjects that don't come from QMP. Change it to "Parameter '%s' is
unexpected".
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qapi/qobject-input-visitor.c | 3 +--
tests/test-qga.c | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index ed6c24c..f3b6713 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -140,8 +140,7 @@ static void qobject_input_check_struct(Visitor *v, Error **errp)
g_hash_table_iter_init(&iter, top_ht);
if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
- error_setg(errp, "QMP input object member '%s' is unexpected",
- key);
+ error_setg(errp, "Parameter '%s' is unexpected", key);
}
}
}
diff --git a/tests/test-qga.c b/tests/test-qga.c
index 868b02a..ae97b57 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -213,7 +213,7 @@ static void test_qga_invalid_args(gconstpointer fix)
desc = qdict_get_try_str(error, "desc");
g_assert_cmpstr(class, ==, "GenericError");
- g_assert_cmpstr(desc, ==, "QMP input object member 'foo' is unexpected");
+ g_assert_cmpstr(desc, ==, "Parameter 'foo' is unexpected");
QDECREF(ret);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 12/28] qapi: Clean up after commit 3d344c2
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (10 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 11/28] qapi: Improve a QObject input visitor error message Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 13/28] qapi: Make QObject input visitor set *list reliably Markus Armbruster
` (16 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
Drop unused QIV_STACK_SIZE and unused qobject_input_start_struct()
parameter errp.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qapi/qobject-input-visitor.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index f3b6713..2c2f883 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -21,8 +21,6 @@
#include "qapi/qmp/types.h"
#include "qapi/qmp/qerror.h"
-#define QIV_STACK_SIZE 1024
-
typedef struct StackObject
{
QObject *obj; /* Object being visited */
@@ -103,8 +101,7 @@ static void qdict_add_key(const char *key, QObject *obj, void *opaque)
}
static const QListEntry *qobject_input_push(QObjectInputVisitor *qiv,
- QObject *obj, void *qapi,
- Error **errp)
+ QObject *obj, void *qapi)
{
GHashTable *h;
StackObject *tos = g_new0(StackObject, 1);
@@ -170,7 +167,6 @@ static void qobject_input_start_struct(Visitor *v, const char *name, void **obj,
{
QObjectInputVisitor *qiv = to_qiv(v);
QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
- Error *err = NULL;
if (obj) {
*obj = NULL;
@@ -184,11 +180,7 @@ static void qobject_input_start_struct(Visitor *v, const char *name, void **obj,
return;
}
- qobject_input_push(qiv, qobj, obj, &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
+ qobject_input_push(qiv, qobj, obj);
if (obj) {
*obj = g_malloc0(size);
@@ -216,7 +208,7 @@ static void qobject_input_start_list(Visitor *v, const char *name,
return;
}
- entry = qobject_input_push(qiv, qobj, list, errp);
+ entry = qobject_input_push(qiv, qobj, list);
if (list) {
if (entry) {
*list = g_malloc0(size);
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 13/28] qapi: Make QObject input visitor set *list reliably
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (11 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 12/28] qapi: Clean up after commit 3d344c2 Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 19:57 ` Philippe Mathieu-Daudé
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 14/28] qapi: Improve qobject input visitor error reporting Markus Armbruster
` (15 subsequent siblings)
28 siblings, 1 reply; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
qobject_input_start_struct() sets *list, except when it fails because
qobject_input_get_object() fails, i.e. the input object doesn't exist.
All the other input visitor start_struct(), start_list(),
start_alternate() always set *obj / *list.
Change qobject_input_start_struct() to match.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qapi/qobject-input-visitor.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 2c2f883..d58696c 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -196,25 +196,21 @@ static void qobject_input_start_list(Visitor *v, const char *name,
QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
const QListEntry *entry;
+ if (list) {
+ *list = NULL;
+ }
if (!qobj) {
return;
}
if (qobject_type(qobj) != QTYPE_QLIST) {
- if (list) {
- *list = NULL;
- }
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"list");
return;
}
entry = qobject_input_push(qiv, qobj, list);
- if (list) {
- if (entry) {
- *list = g_malloc0(size);
- } else {
- *list = NULL;
- }
+ if (entry && list) {
+ *list = g_malloc0(size);
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 14/28] qapi: Improve qobject input visitor error reporting
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (12 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 13/28] qapi: Make QObject input visitor set *list reliably Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 15/28] qapi: Drop string input visitor method optional() Markus Armbruster
` (14 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
Error messages refer to nodes of the QObject being visited by name.
Trouble is the names are sometimes less than helpful:
* The name of the root QObject is whatever @name argument got passed
to the visitor, except NULL gets mapped to "null". We commonly pass
NULL. Not good.
Avoiding errors "at the root" mitigates. For instance,
visit_start_struct() can only fail when the visited object is not a
dictionary, and we commonly ensure it is beforehand.
* The name of a QDict's member is the member key. Good enough only
when this happens to be unique.
* The name of a QList's member is "null". Not good.
Improve error messages by referring to nodes by path instead, as
follows:
* The path of the root QObject is whatever @name argument got passed
to the visitor, except NULL gets mapped to "<anonymous>".
* The path of a root QDict's member is the member key.
* The path of a root QList's member is "[%u]", where %u is the list
index, starting at zero.
* The path of a non-root QDict's member is the path of the QDict
concatenated with "." and the member key.
* The path of a non-root QList's member is the path of the QList
concatenated with "[%u]", where %u is the list index.
For example, the incorrect QMP command
{ "execute": "blockdev-add", "arguments": { "node-name": "foo", "driver": "raw", "file": {"driver": "file" } } }
now fails with
{"error": {"class": "GenericError", "desc": "Parameter 'file.filename' is missing"}}
instead of
{"error": {"class": "GenericError", "desc": "Parameter 'filename' is missing"}}
and
{ "execute": "input-send-event", "arguments": { "device": "bar", "events": [ [] ] } }
now fails with
{"error": {"class": "GenericError", "desc": "Invalid parameter type for 'events[0]', expected: object"}}
instead of
{"error": {"class": "GenericError", "desc": "Invalid parameter type for 'null', expected: QDict"}}
Aside: calling the thing "parameter" is suboptimal for QMP, because
the root object is "arguments" there.
The qobject output visitor doesn't have this problem because it should
not fail. Same for dealloc and clone visitors.
The string visitors don't have this problem because they visit just
one value, whose name needs to be passed to the visitor as @name. The
string output visitor shouldn't fail anyway.
The options visitor uses QemuOpts names. Their name space is flat, so
the use of QDict member keys as names is fine. NULL names used with
roots and lists could conceivably result in bad error messages. Left
for another day.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
include/qapi/visitor.h | 6 ---
qapi/qobject-input-visitor.c | 121 +++++++++++++++++++++++++++++++------------
2 files changed, 87 insertions(+), 40 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 9bb6cba..7c91a50 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -66,12 +66,6 @@
* object, @name is the key associated with the value; and when
* visiting a member of a list, @name is NULL.
*
- * FIXME: Clients must pass NULL for @name when visiting a member of a
- * list, but this leads to poor error messages; it might be nicer to
- * require a non-NULL name such as "key.0" for '{ "key": [ "value" ]
- * }' if an error is encountered on "value" (or to have the visitor
- * core auto-generate the nicer name).
- *
* The visit_type_FOO() functions expect a non-null @obj argument;
* they allocate *@obj during input visits, leave it unchanged on
* output visits, and recursively free any resources during a dealloc
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index d58696c..8015a98 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -21,19 +21,19 @@
#include "qapi/qmp/types.h"
#include "qapi/qmp/qerror.h"
-typedef struct StackObject
-{
- QObject *obj; /* Object being visited */
+typedef struct StackObject {
+ const char *name; /* Name of @obj in its parent, if any */
+ QObject *obj; /* QDict or QList being visited */
void *qapi; /* sanity check that caller uses same pointer */
- GHashTable *h; /* If obj is dict: unvisited keys */
- const QListEntry *entry; /* If obj is list: unvisited tail */
+ GHashTable *h; /* If @obj is QDict: unvisited keys */
+ const QListEntry *entry; /* If @obj is QList: unvisited tail */
+ unsigned index; /* If @obj is QList: list index of @entry */
- QSLIST_ENTRY(StackObject) node;
+ QSLIST_ENTRY(StackObject) node; /* parent */
} StackObject;
-struct QObjectInputVisitor
-{
+struct QObjectInputVisitor {
Visitor visitor;
/* Root of visit at visitor creation. */
@@ -45,6 +45,8 @@ struct QObjectInputVisitor
/* True to reject parse in visit_end_struct() if unvisited keys remain. */
bool strict;
+
+ GString *errname; /* Accumulator for full_name() */
};
static QObjectInputVisitor *to_qiv(Visitor *v)
@@ -52,9 +54,42 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
return container_of(v, QObjectInputVisitor, visitor);
}
-static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
- const char *name,
- bool consume, Error **errp)
+static const char *full_name(QObjectInputVisitor *qiv, const char *name)
+{
+ StackObject *so;
+ char buf[32];
+
+ if (qiv->errname) {
+ g_string_truncate(qiv->errname, 0);
+ } else {
+ qiv->errname = g_string_new("");
+ }
+
+ QSLIST_FOREACH(so , &qiv->stack, node) {
+ if (qobject_type(so->obj) == QTYPE_QDICT) {
+ g_string_prepend(qiv->errname, name);
+ g_string_prepend_c(qiv->errname, '.');
+ } else {
+ snprintf(buf, sizeof(buf), "[%u]", so->index);
+ g_string_prepend(qiv->errname, buf);
+ }
+ name = so->name;
+ }
+
+ if (name) {
+ g_string_prepend(qiv->errname, name);
+ } else if (qiv->errname->str[0] == '.') {
+ g_string_erase(qiv->errname, 0, 1);
+ } else {
+ return "<anonymous>";
+ }
+
+ return qiv->errname->str;
+}
+
+static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
+ const char *name,
+ bool consume)
{
StackObject *tos;
QObject *qobj;
@@ -78,9 +113,6 @@ static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
bool removed = g_hash_table_remove(tos->h, name);
assert(removed);
}
- if (!ret) {
- error_setg(errp, QERR_MISSING_PARAMETER, name);
- }
} else {
assert(qobject_type(qobj) == QTYPE_QLIST);
assert(!name);
@@ -88,12 +120,25 @@ static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
assert(ret);
if (consume) {
tos->entry = qlist_next(tos->entry);
+ tos->index++;
}
}
return ret;
}
+static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
+ const char *name,
+ bool consume, Error **errp)
+{
+ QObject *obj = qobject_input_try_get_object(qiv, name, consume);
+
+ if (!obj) {
+ error_setg(errp, QERR_MISSING_PARAMETER, full_name(qiv, name));
+ }
+ return obj;
+}
+
static void qdict_add_key(const char *key, QObject *obj, void *opaque)
{
GHashTable *h = opaque;
@@ -101,12 +146,14 @@ static void qdict_add_key(const char *key, QObject *obj, void *opaque)
}
static const QListEntry *qobject_input_push(QObjectInputVisitor *qiv,
+ const char *name,
QObject *obj, void *qapi)
{
GHashTable *h;
StackObject *tos = g_new0(StackObject, 1);
assert(obj);
+ tos->name = name;
tos->obj = obj;
tos->qapi = qapi;
@@ -116,6 +163,7 @@ static const QListEntry *qobject_input_push(QObjectInputVisitor *qiv,
tos->h = h;
} else if (qobject_type(obj) == QTYPE_QLIST) {
tos->entry = qlist_first(qobject_to_qlist(obj));
+ tos->index = -1;
}
QSLIST_INSERT_HEAD(&qiv->stack, tos, node);
@@ -137,7 +185,8 @@ static void qobject_input_check_struct(Visitor *v, Error **errp)
g_hash_table_iter_init(&iter, top_ht);
if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
- error_setg(errp, "Parameter '%s' is unexpected", key);
+ error_setg(errp, "Parameter '%s' is unexpected",
+ full_name(qiv, key));
}
}
}
@@ -175,12 +224,12 @@ static void qobject_input_start_struct(Visitor *v, const char *name, void **obj,
return;
}
if (qobject_type(qobj) != QTYPE_QDICT) {
- error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
- "QDict");
+ error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+ full_name(qiv, name), "object");
return;
}
- qobject_input_push(qiv, qobj, obj);
+ qobject_input_push(qiv, name, qobj, obj);
if (obj) {
*obj = g_malloc0(size);
@@ -203,12 +252,12 @@ static void qobject_input_start_list(Visitor *v, const char *name,
return;
}
if (qobject_type(qobj) != QTYPE_QLIST) {
- error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
- "list");
+ error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+ full_name(qiv, name), "array");
return;
}
- entry = qobject_input_push(qiv, qobj, list);
+ entry = qobject_input_push(qiv, name, qobj, list);
if (entry && list) {
*list = g_malloc0(size);
}
@@ -258,8 +307,8 @@ static void qobject_input_type_int64(Visitor *v, const char *name, int64_t *obj,
}
qint = qobject_to_qint(qobj);
if (!qint) {
- error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
- "integer");
+ error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+ full_name(qiv, name), "integer");
return;
}
@@ -279,8 +328,8 @@ static void qobject_input_type_uint64(Visitor *v, const char *name,
}
qint = qobject_to_qint(qobj);
if (!qint) {
- error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
- "integer");
+ error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+ full_name(qiv, name), "integer");
return;
}
@@ -299,8 +348,8 @@ static void qobject_input_type_bool(Visitor *v, const char *name, bool *obj,
}
qbool = qobject_to_qbool(qobj);
if (!qbool) {
- error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
- "boolean");
+ error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+ full_name(qiv, name), "boolean");
return;
}
@@ -320,8 +369,8 @@ static void qobject_input_type_str(Visitor *v, const char *name, char **obj,
}
qstr = qobject_to_qstring(qobj);
if (!qstr) {
- error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
- "string");
+ error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+ full_name(qiv, name), "string");
return;
}
@@ -351,8 +400,8 @@ static void qobject_input_type_number(Visitor *v, const char *name, double *obj,
return;
}
- error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
- "number");
+ error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+ full_name(qiv, name), "number");
}
static void qobject_input_type_any(Visitor *v, const char *name, QObject **obj,
@@ -380,15 +429,15 @@ static void qobject_input_type_null(Visitor *v, const char *name, Error **errp)
}
if (qobject_type(qobj) != QTYPE_QNULL) {
- error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
- "null");
+ error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+ full_name(qiv, name), "null");
}
}
static void qobject_input_optional(Visitor *v, const char *name, bool *present)
{
QObjectInputVisitor *qiv = to_qiv(v);
- QObject *qobj = qobject_input_get_object(qiv, name, false, NULL);
+ QObject *qobj = qobject_input_try_get_object(qiv, name, false);
if (!qobj) {
*present = false;
@@ -401,6 +450,7 @@ static void qobject_input_optional(Visitor *v, const char *name, bool *present)
static void qobject_input_free(Visitor *v)
{
QObjectInputVisitor *qiv = to_qiv(v);
+
while (!QSLIST_EMPTY(&qiv->stack)) {
StackObject *tos = QSLIST_FIRST(&qiv->stack);
@@ -409,6 +459,9 @@ static void qobject_input_free(Visitor *v)
}
qobject_decref(qiv->root);
+ if (qiv->errname) {
+ g_string_free(qiv->errname, TRUE);
+ }
g_free(qiv);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 15/28] qapi: Drop string input visitor method optional()
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (13 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 14/28] qapi: Improve qobject input visitor error reporting Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 16/28] qapi: Make string input and opts visitor require non-null input Markus Armbruster
` (13 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
visit_optional() is to be called only between visit_start_struct() and
visit_end_struct(). Visitors that don't support struct visits,
i.e. don't implement start_struct(), end_struct(), have no use for it.
Clarify documentation.
The string input visitor doesn't support struct visits. Its
parse_optional() is therefore useless. Drop it.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
include/qapi/visitor-impl.h | 4 ++--
qapi/string-input-visitor.c | 13 -------------
2 files changed, 2 insertions(+), 15 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 8bd47ee..962ba1d 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -102,8 +102,8 @@ struct Visitor
/* Must be set to visit explicit null values. */
void (*type_null)(Visitor *v, const char *name, Error **errp);
- /* Must be set for input visitors, optional otherwise. The core
- * takes care of the return type in the public interface. */
+ /* Must be set for input visitors to visit structs, optional otherwise.
+ The core takes care of the return type in the public interface. */
void (*optional)(Visitor *v, const char *name, bool *present);
/* Must be set */
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 8dfa561..1a855c5 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -314,18 +314,6 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
*obj = val;
}
-static void parse_optional(Visitor *v, const char *name, bool *present)
-{
- StringInputVisitor *siv = to_siv(v);
-
- if (!siv->string) {
- *present = false;
- return;
- }
-
- *present = true;
-}
-
static void string_input_free(Visitor *v)
{
StringInputVisitor *siv = to_siv(v);
@@ -351,7 +339,6 @@ Visitor *string_input_visitor_new(const char *str)
v->visitor.start_list = start_list;
v->visitor.next_list = next_list;
v->visitor.end_list = end_list;
- v->visitor.optional = parse_optional;
v->visitor.free = string_input_free;
v->string = str;
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 16/28] qapi: Make string input and opts visitor require non-null input
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (14 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 15/28] qapi: Drop string input visitor method optional() Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-06 17:07 ` Philippe Mathieu-Daudé
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 17/28] qom: Make object_property_set_qobject()'s input visitor strict Markus Armbruster
` (12 subsequent siblings)
28 siblings, 1 reply; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
The string input visitor tries to cope with null input. Null input
isn't used anywhere, and isn't covered by tests. Unsurprisingly, it
doesn't fully work: start_list() crashes because it passes the input
via parse_str() to strtoll() unchecked.
Make string_input_visitor_new() assert its argument isn't null, and
drop the code trying to deal with null input.
The opts visitor crashes when you try to actually visit something with
null input. Make opts_visitor_new() assert its argument isn't null,
mostly for clarity.
qobject_input_visitor_new() already asserts its argument isn't null.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qapi/opts-visitor.c | 1 +
qapi/string-input-visitor.c | 54 ++++++++++++++-------------------------------
2 files changed, 18 insertions(+), 37 deletions(-)
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index a0a7c0e..c50dc4b 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -528,6 +528,7 @@ opts_visitor_new(const QemuOpts *opts)
{
OptsVisitor *ov;
+ assert(opts);
ov = g_malloc0(sizeof *ov);
ov->visitor.type = VISITOR_INPUT;
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 1a855c5..f126cd9 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -182,12 +182,6 @@ static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
{
StringInputVisitor *siv = to_siv(v);
- if (!siv->string) {
- error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
- "integer");
- return;
- }
-
if (parse_str(siv, name, errp) < 0) {
return;
}
@@ -242,13 +236,7 @@ static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
Error *err = NULL;
uint64_t val;
- if (siv->string) {
- parse_option_size(name, siv->string, &val, &err);
- } else {
- error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
- "size");
- return;
- }
+ parse_option_size(name, siv->string, &val, &err);
if (err) {
error_propagate(errp, err);
return;
@@ -262,19 +250,17 @@ static void parse_type_bool(Visitor *v, const char *name, bool *obj,
{
StringInputVisitor *siv = to_siv(v);
- if (siv->string) {
- if (!strcasecmp(siv->string, "on") ||
- !strcasecmp(siv->string, "yes") ||
- !strcasecmp(siv->string, "true")) {
- *obj = true;
- return;
- }
- if (!strcasecmp(siv->string, "off") ||
- !strcasecmp(siv->string, "no") ||
- !strcasecmp(siv->string, "false")) {
- *obj = false;
- return;
- }
+ if (!strcasecmp(siv->string, "on") ||
+ !strcasecmp(siv->string, "yes") ||
+ !strcasecmp(siv->string, "true")) {
+ *obj = true;
+ return;
+ }
+ if (!strcasecmp(siv->string, "off") ||
+ !strcasecmp(siv->string, "no") ||
+ !strcasecmp(siv->string, "false")) {
+ *obj = false;
+ return;
}
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -285,13 +271,8 @@ static void parse_type_str(Visitor *v, const char *name, char **obj,
Error **errp)
{
StringInputVisitor *siv = to_siv(v);
- if (siv->string) {
- *obj = g_strdup(siv->string);
- } else {
- *obj = NULL;
- error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
- "string");
- }
+
+ *obj = g_strdup(siv->string);
}
static void parse_type_number(Visitor *v, const char *name, double *obj,
@@ -302,10 +283,8 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
double val;
errno = 0;
- if (siv->string) {
- val = strtod(siv->string, &endp);
- }
- if (!siv->string || errno || endp == siv->string || *endp) {
+ val = strtod(siv->string, &endp);
+ if (errno || endp == siv->string || *endp) {
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"number");
return;
@@ -327,6 +306,7 @@ Visitor *string_input_visitor_new(const char *str)
{
StringInputVisitor *v;
+ assert(str);
v = g_malloc0(sizeof(*v));
v->visitor.type = VISITOR_INPUT;
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 17/28] qom: Make object_property_set_qobject()'s input visitor strict
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (15 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 16/28] qapi: Make string input and opts visitor require non-null input Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 18/28] test-qobject-input-visitor: Use strict visitor Markus Armbruster
` (11 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
Commit 240f64b made all qobject input visitors created outside tests
strict, except for the one in object_property_set_qobject(). That one
was left behind only because Eric couldn't spare the time to figure
out whether making it strict would break anything, with a TODO
comment. Time to resolve it.
Strict makes a difference only for otherwise successful visits of QAPI
structs or unions. Let's examine what the callers of
object_property_set_qobject() visit:
* object_property_set_str(), object_property_set_bool(),
object_property_set_int() visit a QString, QBool, QInt,
respectively. Strictness can't matter.
* qmp_qom_set visits its @value argument. Comes straight from QMP and
can be anything ('any' in the QAPI schema). Strictness matters when
the property's set() method visits a struct or union QAPI type.
No such methods exist, thus switching to strict can't break
anything.
If we acquire such methods in the future, we'll *want* the visitor
to be strict, so that unexpected members get rejected as they should
be.
Switch to strict.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qom/qom-qobject.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/qom/qom-qobject.c b/qom/qom-qobject.c
index 447e4a0..bbdedda 100644
--- a/qom/qom-qobject.c
+++ b/qom/qom-qobject.c
@@ -22,8 +22,8 @@ void object_property_set_qobject(Object *obj, QObject *value,
const char *name, Error **errp)
{
Visitor *v;
- /* TODO: Should we reject, rather than ignore, excess input? */
- v = qobject_input_visitor_new(value, false);
+
+ v = qobject_input_visitor_new(value, true);
object_property_set(obj, v, name, errp);
visit_free(v);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 18/28] test-qobject-input-visitor: Use strict visitor
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (16 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 17/28] qom: Make object_property_set_qobject()'s input visitor strict Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 19/28] qapi: Drop unused non-strict qobject input visitor Markus Armbruster
` (10 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
The qobject input visitor comes in a strict and a non-strict variant.
This test is the non-strict variant's last user. Turns out it relies
on non-strict only in test_visitor_in_null(), and just out of
laziness. We don't actually test the non-strict behavior.
Clean up test_visitor_in_null(), and switch to the strict variant.
The next commit will drop the non-strict variant.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
tests/test-qobject-input-visitor.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 945404a..125e34c 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -49,7 +49,7 @@ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data,
data->obj = qobject_from_jsonv(json_string, ap);
g_assert(data->obj);
- data->qiv = qobject_input_visitor_new(data->obj, false);
+ data->qiv = qobject_input_visitor_new(data->obj, true);
g_assert(data->qiv);
return data->qiv;
}
@@ -290,14 +290,14 @@ static void test_visitor_in_null(TestInputVisitorData *data,
* when input is not null.
*/
- v = visitor_input_test_init(data, "{ 'a': null, 'b': '' }");
+ v = visitor_input_test_init(data, "{ 'a': null, 'b': '', 'c': null }");
visit_start_struct(v, NULL, NULL, 0, &error_abort);
visit_type_null(v, "a", &error_abort);
- visit_type_str(v, "a", &tmp, &err);
- g_assert(!tmp);
- error_free_or_abort(&err);
visit_type_null(v, "b", &err);
error_free_or_abort(&err);
+ visit_type_str(v, "c", &tmp, &err);
+ g_assert(!tmp);
+ error_free_or_abort(&err);
visit_check_struct(v, &error_abort);
visit_end_struct(v, NULL);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 19/28] qapi: Drop unused non-strict qobject input visitor
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (17 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 18/28] test-qobject-input-visitor: Use strict visitor Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 20/28] tests-qobject-input-strict: Merge into test-qobject-input-visitor Markus Armbruster
` (9 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
The split between tests/test-qobject-input-visitor.c and
tests/test-qobject-input-strict.c now makes less sense than ever. The
next commit will take care of that.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/nbd.c | 2 +-
block/nfs.c | 2 +-
block/ssh.c | 2 +-
docs/qapi-code-gen.txt | 2 +-
include/qapi/qobject-input-visitor.h | 5 +----
qapi/qobject-input-visitor.c | 28 ++++++++++------------------
qmp.c | 2 +-
qom/qom-qobject.c | 2 +-
scripts/qapi-commands.py | 2 +-
target/s390x/cpu_models.c | 2 +-
tests/check-qnull.c | 2 +-
tests/qmp-test.c | 2 +-
tests/test-qmp-commands.c | 2 +-
tests/test-qobject-input-strict.c | 2 +-
tests/test-qobject-input-visitor.c | 2 +-
tests/test-visitor-serialization.c | 2 +-
16 files changed, 25 insertions(+), 36 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index a7f9108..f478f80 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -278,7 +278,7 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, Error **errp)
goto done;
}
- iv = qobject_input_visitor_new(crumpled_addr, true);
+ iv = qobject_input_visitor_new(crumpled_addr);
visit_type_SocketAddress(iv, NULL, &saddr, &local_err);
if (local_err) {
error_propagate(errp, local_err);
diff --git a/block/nfs.c b/block/nfs.c
index 890d5d4..3f43f6e 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -474,7 +474,7 @@ static NFSServer *nfs_config(QDict *options, Error **errp)
goto out;
}
- iv = qobject_input_visitor_new(crumpled_addr, true);
+ iv = qobject_input_visitor_new(crumpled_addr);
visit_type_NFSServer(iv, NULL, &server, &local_error);
if (local_error) {
error_propagate(errp, local_error);
diff --git a/block/ssh.c b/block/ssh.c
index 835932e..278e66f 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -601,7 +601,7 @@ static InetSocketAddress *ssh_config(QDict *options, Error **errp)
goto out;
}
- iv = qobject_input_visitor_new(crumpled_addr, true);
+ iv = qobject_input_visitor_new(crumpled_addr);
visit_type_InetSocketAddress(iv, NULL, &inet, &local_error);
if (local_error) {
error_propagate(errp, local_error);
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 7eb7be1..6746c10 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -1138,7 +1138,7 @@ Example:
Visitor *v;
UserDefOneList *arg1 = NULL;
- v = qobject_input_visitor_new(QOBJECT(args), true);
+ v = qobject_input_visitor_new(QOBJECT(args));
visit_start_struct(v, NULL, NULL, 0, &err);
if (err) {
goto out;
diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h
index cde328d..21db9c4 100644
--- a/include/qapi/qobject-input-visitor.h
+++ b/include/qapi/qobject-input-visitor.h
@@ -21,10 +21,7 @@ typedef struct QObjectInputVisitor QObjectInputVisitor;
/*
* Return a new input visitor that converts a QObject to a QAPI object.
- *
- * Set @strict to reject a parse that doesn't consume all keys of a
- * dictionary; otherwise excess input is ignored.
*/
-Visitor *qobject_input_visitor_new(QObject *obj, bool strict);
+Visitor *qobject_input_visitor_new(QObject *obj);
#endif
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 8015a98..eafcdf4 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -43,9 +43,6 @@ struct QObjectInputVisitor {
* QDict or QList). */
QSLIST_HEAD(, StackObject) stack;
- /* True to reject parse in visit_end_struct() if unvisited keys remain. */
- bool strict;
-
GString *errname; /* Accumulator for full_name() */
};
@@ -157,11 +154,12 @@ static const QListEntry *qobject_input_push(QObjectInputVisitor *qiv,
tos->obj = obj;
tos->qapi = qapi;
- if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
+ if (qobject_type(obj) == QTYPE_QDICT) {
h = g_hash_table_new(g_str_hash, g_str_equal);
qdict_iter(qobject_to_qdict(obj), qdict_add_key, h);
tos->h = h;
- } else if (qobject_type(obj) == QTYPE_QLIST) {
+ } else {
+ assert(qobject_type(obj) == QTYPE_QLIST);
tos->entry = qlist_first(qobject_to_qlist(obj));
tos->index = -1;
}
@@ -175,20 +173,15 @@ static void qobject_input_check_struct(Visitor *v, Error **errp)
{
QObjectInputVisitor *qiv = to_qiv(v);
StackObject *tos = QSLIST_FIRST(&qiv->stack);
+ GHashTableIter iter;
+ const char *key;
assert(tos && !tos->entry);
- if (qiv->strict) {
- GHashTable *const top_ht = tos->h;
- if (top_ht) {
- GHashTableIter iter;
- const char *key;
- g_hash_table_iter_init(&iter, top_ht);
- if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
- error_setg(errp, "Parameter '%s' is unexpected",
- full_name(qiv, key));
- }
- }
+ g_hash_table_iter_init(&iter, tos->h);
+ if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
+ error_setg(errp, "Parameter '%s' is unexpected",
+ full_name(qiv, key));
}
}
@@ -465,7 +458,7 @@ static void qobject_input_free(Visitor *v)
g_free(qiv);
}
-Visitor *qobject_input_visitor_new(QObject *obj, bool strict)
+Visitor *qobject_input_visitor_new(QObject *obj)
{
QObjectInputVisitor *v;
@@ -489,7 +482,6 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool strict)
v->visitor.type_null = qobject_input_type_null;
v->visitor.optional = qobject_input_optional;
v->visitor.free = qobject_input_free;
- v->strict = strict;
v->root = obj;
qobject_incref(obj);
diff --git a/qmp.c b/qmp.c
index dfaabac..fa82b59 100644
--- a/qmp.c
+++ b/qmp.c
@@ -675,7 +675,7 @@ void qmp_object_add(const char *type, const char *id,
pdict = qdict_new();
}
- v = qobject_input_visitor_new(QOBJECT(pdict), true);
+ v = qobject_input_visitor_new(QOBJECT(pdict));
obj = user_creatable_add_type(type, id, pdict, v, errp);
visit_free(v);
if (obj) {
diff --git a/qom/qom-qobject.c b/qom/qom-qobject.c
index bbdedda..4aec20d 100644
--- a/qom/qom-qobject.c
+++ b/qom/qom-qobject.c
@@ -23,7 +23,7 @@ void object_property_set_qobject(Object *obj, QObject *value,
{
Visitor *v;
- v = qobject_input_visitor_new(value, true);
+ v = qobject_input_visitor_new(value);
object_property_set(obj, v, name, errp);
visit_free(v);
}
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 2b062ad..0c05449 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -130,7 +130,7 @@ def gen_marshal(name, arg_type, boxed, ret_type):
push_indent()
ret += mcgen('''
- v = qobject_input_visitor_new(QOBJECT(args), true);
+ v = qobject_input_visitor_new(QOBJECT(args));
visit_start_struct(v, NULL, NULL, 0, &err);
if (err) {
goto out;
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 2a894ee..4ea3a2d 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -346,7 +346,7 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
}
if (qdict) {
- visitor = qobject_input_visitor_new(info->props, true);
+ visitor = qobject_input_visitor_new(info->props);
visit_start_struct(visitor, NULL, NULL, 0, errp);
if (*errp) {
object_unref(obj);
diff --git a/tests/check-qnull.c b/tests/check-qnull.c
index b50bb8a..8dd1c96 100644
--- a/tests/check-qnull.c
+++ b/tests/check-qnull.c
@@ -47,7 +47,7 @@ static void qnull_visit_test(void)
g_assert(qnull_.refcnt == 1);
obj = qnull();
- v = qobject_input_visitor_new(obj, true);
+ v = qobject_input_visitor_new(obj);
qobject_decref(obj);
visit_type_null(v, NULL, &error_abort);
visit_free(v);
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 405e49e..5d0260b 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -34,7 +34,7 @@ static void test_version(QObject *version)
VersionInfo *vinfo;
g_assert(version);
- v = qobject_input_visitor_new(version, true);
+ v = qobject_input_visitor_new(version);
visit_type_VersionInfo(v, "version", &vinfo, &error_abort);
qapi_free_VersionInfo(vinfo);
visit_free(v);
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 1cf413b..0f81a98 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -246,7 +246,7 @@ static void test_dealloc_partial(void)
ud2_dict = qdict_new();
qdict_put_obj(ud2_dict, "string0", QOBJECT(qstring_from_str(text)));
- v = qobject_input_visitor_new(QOBJECT(ud2_dict), true);
+ v = qobject_input_visitor_new(QOBJECT(ud2_dict));
visit_type_UserDefTwo(v, NULL, &ud2, &err);
visit_free(v);
QDECREF(ud2_dict);
diff --git a/tests/test-qobject-input-strict.c b/tests/test-qobject-input-strict.c
index 4087ea3..7d26113 100644
--- a/tests/test-qobject-input-strict.c
+++ b/tests/test-qobject-input-strict.c
@@ -53,7 +53,7 @@ static Visitor *validate_test_init_internal(TestInputVisitorData *data,
data->obj = qobject_from_jsonv(json_string, ap);
g_assert(data->obj);
- data->qiv = qobject_input_visitor_new(data->obj, true);
+ data->qiv = qobject_input_visitor_new(data->obj);
g_assert(data->qiv);
return data->qiv;
}
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 125e34c..658fa2c 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -49,7 +49,7 @@ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data,
data->obj = qobject_from_jsonv(json_string, ap);
g_assert(data->obj);
- data->qiv = qobject_input_visitor_new(data->obj, true);
+ data->qiv = qobject_input_visitor_new(data->obj);
g_assert(data->qiv);
return data->qiv;
}
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index 66b2b1c..c7e64f0 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -1040,7 +1040,7 @@ static void qmp_deserialize(void **native_out, void *datap,
obj = qobject_from_json(qstring_get_str(output_json));
QDECREF(output_json);
- d->qiv = qobject_input_visitor_new(obj, true);
+ d->qiv = qobject_input_visitor_new(obj);
qobject_decref(obj_orig);
qobject_decref(obj);
visit(d->qiv, native_out, errp);
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 20/28] tests-qobject-input-strict: Merge into test-qobject-input-visitor
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (18 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 19/28] qapi: Drop unused non-strict qobject input visitor Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 21/28] test-string-input-visitor: Tear down existing test automatically Markus Armbruster
` (8 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
Much of test-qobject-input-strict.c duplicates
test-qobject-input-strict.c, but with less assertions on expected
output:
* test_validate_struct() duplicates test_visitor_in_struct()
* test_validate_struct_nested() duplicates
test_visitor_in_struct_nested()
* test_validate_list() duplicates the first half of
test_visitor_in_list()
* test_validate_union_native_list() duplicates
test_visitor_in_native_list_int()
* test_validate_union_flat() duplicates test_visitor_in_union_flat()
* test_validate_alternate() duplicates the first part of
test_visitor_in_alternate()
Merge the remaining test cases into test-qobject-input-visitor.c, and
drop the now redundant test-qobject-input-strict.c.
Test case "/visitor/input-strict/fail/list" isn't really about lists,
it's about a bad struct nested in a list. Rename accordingly.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
tests/Makefile.include | 4 +-
tests/test-qobject-input-strict.c | 381 -------------------------------------
tests/test-qobject-input-visitor.c | 187 ++++++++++++++++++
3 files changed, 188 insertions(+), 384 deletions(-)
delete mode 100644 tests/test-qobject-input-strict.c
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 4f52239..8b3e2bd 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -28,7 +28,6 @@ check-unit-y += tests/test-clone-visitor$(EXESUF)
gcov-files-test-clone-visitor-y = qapi/qapi-clone-visitor.c
check-unit-y += tests/test-qobject-input-visitor$(EXESUF)
gcov-files-test-qobject-input-visitor-y = qapi/qobject-input-visitor.c
-check-unit-y += tests/test-qobject-input-strict$(EXESUF)
check-unit-y += tests/test-qmp-commands$(EXESUF)
gcov-files-test-qmp-commands-y = qapi/qmp-dispatch.c
check-unit-y += tests/test-string-input-visitor$(EXESUF)
@@ -489,7 +488,7 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
tests/test-coroutine.o tests/test-string-output-visitor.o \
tests/test-string-input-visitor.o tests/test-qobject-output-visitor.o \
tests/test-clone-visitor.o \
- tests/test-qobject-input-visitor.o tests/test-qobject-input-strict.o \
+ tests/test-qobject-input-visitor.o \
tests/test-qmp-commands.o tests/test-visitor-serialization.o \
tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o \
tests/test-opts-visitor.o tests/test-qmp-event.o \
@@ -598,7 +597,6 @@ tests/test-qmp-event$(EXESUF): tests/test-qmp-event.o $(test-qapi-obj-y)
tests/test-qobject-output-visitor$(EXESUF): tests/test-qobject-output-visitor.o $(test-qapi-obj-y)
tests/test-clone-visitor$(EXESUF): tests/test-clone-visitor.o $(test-qapi-obj-y)
tests/test-qobject-input-visitor$(EXESUF): tests/test-qobject-input-visitor.o $(test-qapi-obj-y)
-tests/test-qobject-input-strict$(EXESUF): tests/test-qobject-input-strict.o $(test-qapi-obj-y)
tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o tests/test-qmp-marshal.o $(test-qapi-obj-y)
tests/test-visitor-serialization$(EXESUF): tests/test-visitor-serialization.o $(test-qapi-obj-y)
tests/test-opts-visitor$(EXESUF): tests/test-opts-visitor.o $(test-qapi-obj-y)
diff --git a/tests/test-qobject-input-strict.c b/tests/test-qobject-input-strict.c
deleted file mode 100644
index 7d26113..0000000
--- a/tests/test-qobject-input-strict.c
+++ /dev/null
@@ -1,381 +0,0 @@
-/*
- * QObject Input Visitor unit-tests (strict mode).
- *
- * Copyright (C) 2011-2012, 2015 Red Hat Inc.
- *
- * Authors:
- * Luiz Capitulino <lcapitulino@redhat.com>
- * Paolo Bonzini <pbonzini@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.
- */
-
-#include "qemu/osdep.h"
-
-#include "qemu-common.h"
-#include "qapi/error.h"
-#include "qapi/qobject-input-visitor.h"
-#include "test-qapi-types.h"
-#include "test-qapi-visit.h"
-#include "qapi/qmp/types.h"
-#include "qapi/qmp/qjson.h"
-#include "test-qmp-introspect.h"
-#include "qmp-introspect.h"
-#include "qapi-visit.h"
-
-typedef struct TestInputVisitorData {
- QObject *obj;
- Visitor *qiv;
-} TestInputVisitorData;
-
-static void validate_teardown(TestInputVisitorData *data,
- const void *unused)
-{
- qobject_decref(data->obj);
- data->obj = NULL;
-
- if (data->qiv) {
- visit_free(data->qiv);
- data->qiv = NULL;
- }
-}
-
-/* The various test_init functions are provided instead of a test setup
- function so that the JSON string used by the tests are kept in the test
- functions (and not in main()). */
-static Visitor *validate_test_init_internal(TestInputVisitorData *data,
- const char *json_string,
- va_list *ap)
-{
- validate_teardown(data, NULL);
-
- data->obj = qobject_from_jsonv(json_string, ap);
- g_assert(data->obj);
-
- data->qiv = qobject_input_visitor_new(data->obj);
- g_assert(data->qiv);
- return data->qiv;
-}
-
-static GCC_FMT_ATTR(2, 3)
-Visitor *validate_test_init(TestInputVisitorData *data,
- const char *json_string, ...)
-{
- Visitor *v;
- va_list ap;
-
- va_start(ap, json_string);
- v = validate_test_init_internal(data, json_string, &ap);
- va_end(ap);
- return v;
-}
-
-/* similar to validate_test_init(), but does not expect a string
- * literal/format json_string argument and so can be used for
- * programatically generated strings (and we can't pass in programatically
- * generated strings via %s format parameters since qobject_from_jsonv()
- * will wrap those in double-quotes and treat the entire object as a
- * string)
- */
-static Visitor *validate_test_init_raw(TestInputVisitorData *data,
- const char *json_string)
-{
- return validate_test_init_internal(data, json_string, NULL);
-}
-
-
-static void test_validate_struct(TestInputVisitorData *data,
- const void *unused)
-{
- TestStruct *p = NULL;
- Visitor *v;
-
- v = validate_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo' }");
-
- visit_type_TestStruct(v, NULL, &p, &error_abort);
- g_free(p->string);
- g_free(p);
-}
-
-static void test_validate_struct_nested(TestInputVisitorData *data,
- const void *unused)
-{
- UserDefTwo *udp = NULL;
- Visitor *v;
-
- v = validate_test_init(data, "{ 'string0': 'string0', "
- "'dict1': { 'string1': 'string1', "
- "'dict2': { 'userdef': { 'integer': 42, "
- "'string': 'string' }, 'string': 'string2'}}}");
-
- visit_type_UserDefTwo(v, NULL, &udp, &error_abort);
- qapi_free_UserDefTwo(udp);
-}
-
-static void test_validate_list(TestInputVisitorData *data,
- const void *unused)
-{
- UserDefOneList *head = NULL;
- Visitor *v;
-
- v = validate_test_init(data, "[ { 'string': 'string0', 'integer': 42 }, { 'string': 'string1', 'integer': 43 }, { 'string': 'string2', 'integer': 44 } ]");
-
- visit_type_UserDefOneList(v, NULL, &head, &error_abort);
- qapi_free_UserDefOneList(head);
-}
-
-static void test_validate_union_native_list(TestInputVisitorData *data,
- const void *unused)
-{
- UserDefNativeListUnion *tmp = NULL;
- Visitor *v;
-
- v = validate_test_init(data, "{ 'type': 'integer', 'data' : [ 1, 2 ] }");
-
- visit_type_UserDefNativeListUnion(v, NULL, &tmp, &error_abort);
- qapi_free_UserDefNativeListUnion(tmp);
-}
-
-static void test_validate_union_flat(TestInputVisitorData *data,
- const void *unused)
-{
- UserDefFlatUnion *tmp = NULL;
- Visitor *v;
-
- v = validate_test_init(data,
- "{ 'enum1': 'value1', "
- "'integer': 41, "
- "'string': 'str', "
- "'boolean': true }");
-
- visit_type_UserDefFlatUnion(v, NULL, &tmp, &error_abort);
- qapi_free_UserDefFlatUnion(tmp);
-}
-
-static void test_validate_alternate(TestInputVisitorData *data,
- const void *unused)
-{
- UserDefAlternate *tmp = NULL;
- Visitor *v;
-
- v = validate_test_init(data, "42");
-
- visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort);
- qapi_free_UserDefAlternate(tmp);
-}
-
-static void test_validate_fail_struct(TestInputVisitorData *data,
- const void *unused)
-{
- TestStruct *p = NULL;
- Error *err = NULL;
- Visitor *v;
-
- v = validate_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo', 'extra': 42 }");
-
- visit_type_TestStruct(v, NULL, &p, &err);
- error_free_or_abort(&err);
- g_assert(!p);
-}
-
-static void test_validate_fail_struct_nested(TestInputVisitorData *data,
- const void *unused)
-{
- UserDefTwo *udp = NULL;
- Error *err = NULL;
- Visitor *v;
-
- v = validate_test_init(data, "{ 'string0': 'string0', 'dict1': { 'string1': 'string1', 'dict2': { 'userdef1': { 'integer': 42, 'string': 'string', 'extra': [42, 23, {'foo':'bar'}] }, 'string2': 'string2'}}}");
-
- visit_type_UserDefTwo(v, NULL, &udp, &err);
- error_free_or_abort(&err);
- g_assert(!udp);
-}
-
-static void test_validate_fail_struct_missing(TestInputVisitorData *data,
- const void *unused)
-{
- Error *err = NULL;
- Visitor *v;
- QObject *any;
- GenericAlternate *alt;
- bool present;
- int en;
- int64_t i64;
- uint32_t u32;
- int8_t i8;
- char *str;
- double dbl;
-
- v = validate_test_init(data, "{}");
- visit_start_struct(v, NULL, NULL, 0, &error_abort);
- visit_start_struct(v, "struct", NULL, 0, &err);
- error_free_or_abort(&err);
- visit_start_list(v, "list", NULL, 0, &err);
- error_free_or_abort(&err);
- visit_start_alternate(v, "alternate", &alt, sizeof(*alt), false, &err);
- error_free_or_abort(&err);
- visit_optional(v, "optional", &present);
- g_assert(!present);
- visit_type_enum(v, "enum", &en, EnumOne_lookup, &err);
- error_free_or_abort(&err);
- visit_type_int(v, "i64", &i64, &err);
- error_free_or_abort(&err);
- visit_type_uint32(v, "u32", &u32, &err);
- error_free_or_abort(&err);
- visit_type_int8(v, "i8", &i8, &err);
- error_free_or_abort(&err);
- visit_type_str(v, "i8", &str, &err);
- error_free_or_abort(&err);
- visit_type_number(v, "dbl", &dbl, &err);
- error_free_or_abort(&err);
- visit_type_any(v, "any", &any, &err);
- error_free_or_abort(&err);
- visit_type_null(v, "null", &err);
- error_free_or_abort(&err);
- visit_end_struct(v, NULL);
-}
-
-static void test_validate_fail_list(TestInputVisitorData *data,
- const void *unused)
-{
- UserDefOneList *head = NULL;
- Error *err = NULL;
- Visitor *v;
-
- v = validate_test_init(data, "[ { 'string': 'string0', 'integer': 42 }, { 'string': 'string1', 'integer': 43 }, { 'string': 'string2', 'integer': 44, 'extra': 'ggg' } ]");
-
- visit_type_UserDefOneList(v, NULL, &head, &err);
- error_free_or_abort(&err);
- g_assert(!head);
-}
-
-static void test_validate_fail_union_native_list(TestInputVisitorData *data,
- const void *unused)
-{
- UserDefNativeListUnion *tmp = NULL;
- Error *err = NULL;
- Visitor *v;
-
- v = validate_test_init(data,
- "{ 'type': 'integer', 'data' : [ 'string' ] }");
-
- visit_type_UserDefNativeListUnion(v, NULL, &tmp, &err);
- error_free_or_abort(&err);
- g_assert(!tmp);
-}
-
-static void test_validate_fail_union_flat(TestInputVisitorData *data,
- const void *unused)
-{
- UserDefFlatUnion *tmp = NULL;
- Error *err = NULL;
- Visitor *v;
-
- v = validate_test_init(data, "{ 'string': 'c', 'integer': 41, 'boolean': true }");
-
- visit_type_UserDefFlatUnion(v, NULL, &tmp, &err);
- error_free_or_abort(&err);
- g_assert(!tmp);
-}
-
-static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data,
- const void *unused)
-{
- UserDefFlatUnion2 *tmp = NULL;
- Error *err = NULL;
- Visitor *v;
-
- /* test situation where discriminator field ('enum1' here) is missing */
- v = validate_test_init(data, "{ 'integer': 42, 'string': 'c', 'string1': 'd', 'string2': 'e' }");
-
- visit_type_UserDefFlatUnion2(v, NULL, &tmp, &err);
- error_free_or_abort(&err);
- g_assert(!tmp);
-}
-
-static void test_validate_fail_alternate(TestInputVisitorData *data,
- const void *unused)
-{
- UserDefAlternate *tmp;
- Visitor *v;
- Error *err = NULL;
-
- v = validate_test_init(data, "3.14");
-
- visit_type_UserDefAlternate(v, NULL, &tmp, &err);
- error_free_or_abort(&err);
- g_assert(!tmp);
-}
-
-static void do_test_validate_qmp_introspect(TestInputVisitorData *data,
- const char *schema_json)
-{
- SchemaInfoList *schema = NULL;
- Visitor *v;
-
- v = validate_test_init_raw(data, schema_json);
-
- visit_type_SchemaInfoList(v, NULL, &schema, &error_abort);
- g_assert(schema);
-
- qapi_free_SchemaInfoList(schema);
-}
-
-static void test_validate_qmp_introspect(TestInputVisitorData *data,
- const void *unused)
-{
- do_test_validate_qmp_introspect(data, test_qmp_schema_json);
- do_test_validate_qmp_introspect(data, qmp_schema_json);
-}
-
-static void validate_test_add(const char *testpath,
- TestInputVisitorData *data,
- void (*test_func)(TestInputVisitorData *data, const void *user_data))
-{
- g_test_add(testpath, TestInputVisitorData, data, NULL, test_func,
- validate_teardown);
-}
-
-int main(int argc, char **argv)
-{
- TestInputVisitorData testdata;
-
- g_test_init(&argc, &argv, NULL);
-
- validate_test_add("/visitor/input-strict/pass/struct",
- &testdata, test_validate_struct);
- validate_test_add("/visitor/input-strict/pass/struct-nested",
- &testdata, test_validate_struct_nested);
- validate_test_add("/visitor/input-strict/pass/list",
- &testdata, test_validate_list);
- validate_test_add("/visitor/input-strict/pass/union-flat",
- &testdata, test_validate_union_flat);
- validate_test_add("/visitor/input-strict/pass/alternate",
- &testdata, test_validate_alternate);
- validate_test_add("/visitor/input-strict/pass/union-native-list",
- &testdata, test_validate_union_native_list);
- validate_test_add("/visitor/input-strict/fail/struct",
- &testdata, test_validate_fail_struct);
- validate_test_add("/visitor/input-strict/fail/struct-nested",
- &testdata, test_validate_fail_struct_nested);
- validate_test_add("/visitor/input-strict/fail/struct-missing",
- &testdata, test_validate_fail_struct_missing);
- validate_test_add("/visitor/input-strict/fail/list",
- &testdata, test_validate_fail_list);
- validate_test_add("/visitor/input-strict/fail/union-flat",
- &testdata, test_validate_fail_union_flat);
- validate_test_add("/visitor/input-strict/fail/union-flat-no-discriminator",
- &testdata, test_validate_fail_union_flat_no_discrim);
- validate_test_add("/visitor/input-strict/fail/alternate",
- &testdata, test_validate_fail_alternate);
- validate_test_add("/visitor/input-strict/fail/union-native-list",
- &testdata, test_validate_fail_union_native_list);
- validate_test_add("/visitor/input-strict/pass/qmp-introspect",
- &testdata, test_validate_qmp_introspect);
-
- g_test_run();
-
- return 0;
-}
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 658fa2c..32c6b3d 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -5,6 +5,7 @@
*
* Authors:
* Luiz Capitulino <lcapitulino@redhat.com>
+ * Paolo Bonzini <pbonzini@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.
@@ -19,6 +20,9 @@
#include "test-qapi-visit.h"
#include "qapi/qmp/types.h"
#include "qapi/qmp/qjson.h"
+#include "test-qmp-introspect.h"
+#include "qmp-introspect.h"
+#include "qapi-visit.h"
typedef struct TestInputVisitorData {
QObject *obj;
@@ -833,6 +837,171 @@ static void test_visitor_in_wrong_type(TestInputVisitorData *data,
error_free_or_abort(&err);
}
+static void test_visitor_in_fail_struct(TestInputVisitorData *data,
+ const void *unused)
+{
+ TestStruct *p = NULL;
+ Error *err = NULL;
+ Visitor *v;
+
+ v = visitor_input_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo', 'extra': 42 }");
+
+ visit_type_TestStruct(v, NULL, &p, &err);
+ error_free_or_abort(&err);
+ g_assert(!p);
+}
+
+static void test_visitor_in_fail_struct_nested(TestInputVisitorData *data,
+ const void *unused)
+{
+ UserDefTwo *udp = NULL;
+ Error *err = NULL;
+ Visitor *v;
+
+ v = visitor_input_test_init(data, "{ 'string0': 'string0', 'dict1': { 'string1': 'string1', 'dict2': { 'userdef1': { 'integer': 42, 'string': 'string', 'extra': [42, 23, {'foo':'bar'}] }, 'string2': 'string2'}}}");
+
+ visit_type_UserDefTwo(v, NULL, &udp, &err);
+ error_free_or_abort(&err);
+ g_assert(!udp);
+}
+
+static void test_visitor_in_fail_struct_in_list(TestInputVisitorData *data,
+ const void *unused)
+{
+ UserDefOneList *head = NULL;
+ Error *err = NULL;
+ Visitor *v;
+
+ v = visitor_input_test_init(data, "[ { 'string': 'string0', 'integer': 42 }, { 'string': 'string1', 'integer': 43 }, { 'string': 'string2', 'integer': 44, 'extra': 'ggg' } ]");
+
+ visit_type_UserDefOneList(v, NULL, &head, &err);
+ error_free_or_abort(&err);
+ g_assert(!head);
+}
+
+static void test_visitor_in_fail_struct_missing(TestInputVisitorData *data,
+ const void *unused)
+{
+ Error *err = NULL;
+ Visitor *v;
+ QObject *any;
+ GenericAlternate *alt;
+ bool present;
+ int en;
+ int64_t i64;
+ uint32_t u32;
+ int8_t i8;
+ char *str;
+ double dbl;
+
+ v = visitor_input_test_init(data, "{}");
+ visit_start_struct(v, NULL, NULL, 0, &error_abort);
+ visit_start_struct(v, "struct", NULL, 0, &err);
+ error_free_or_abort(&err);
+ visit_start_list(v, "list", NULL, 0, &err);
+ error_free_or_abort(&err);
+ visit_start_alternate(v, "alternate", &alt, sizeof(*alt), false, &err);
+ error_free_or_abort(&err);
+ visit_optional(v, "optional", &present);
+ g_assert(!present);
+ visit_type_enum(v, "enum", &en, EnumOne_lookup, &err);
+ error_free_or_abort(&err);
+ visit_type_int(v, "i64", &i64, &err);
+ error_free_or_abort(&err);
+ visit_type_uint32(v, "u32", &u32, &err);
+ error_free_or_abort(&err);
+ visit_type_int8(v, "i8", &i8, &err);
+ error_free_or_abort(&err);
+ visit_type_str(v, "i8", &str, &err);
+ error_free_or_abort(&err);
+ visit_type_number(v, "dbl", &dbl, &err);
+ error_free_or_abort(&err);
+ visit_type_any(v, "any", &any, &err);
+ error_free_or_abort(&err);
+ visit_type_null(v, "null", &err);
+ error_free_or_abort(&err);
+ visit_end_struct(v, NULL);
+}
+
+static void test_visitor_in_fail_union_native_list(TestInputVisitorData *data,
+ const void *unused)
+{
+ UserDefNativeListUnion *tmp = NULL;
+ Error *err = NULL;
+ Visitor *v;
+
+ v = visitor_input_test_init(data,
+ "{ 'type': 'integer', 'data' : [ 'string' ] }");
+
+ visit_type_UserDefNativeListUnion(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+ g_assert(!tmp);
+}
+
+static void test_visitor_in_fail_union_flat(TestInputVisitorData *data,
+ const void *unused)
+{
+ UserDefFlatUnion *tmp = NULL;
+ Error *err = NULL;
+ Visitor *v;
+
+ v = visitor_input_test_init(data, "{ 'string': 'c', 'integer': 41, 'boolean': true }");
+
+ visit_type_UserDefFlatUnion(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+ g_assert(!tmp);
+}
+
+static void test_visitor_in_fail_union_flat_no_discrim(TestInputVisitorData *data,
+ const void *unused)
+{
+ UserDefFlatUnion2 *tmp = NULL;
+ Error *err = NULL;
+ Visitor *v;
+
+ /* test situation where discriminator field ('enum1' here) is missing */
+ v = visitor_input_test_init(data, "{ 'integer': 42, 'string': 'c', 'string1': 'd', 'string2': 'e' }");
+
+ visit_type_UserDefFlatUnion2(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+ g_assert(!tmp);
+}
+
+static void test_visitor_in_fail_alternate(TestInputVisitorData *data,
+ const void *unused)
+{
+ UserDefAlternate *tmp;
+ Visitor *v;
+ Error *err = NULL;
+
+ v = visitor_input_test_init(data, "3.14");
+
+ visit_type_UserDefAlternate(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+ g_assert(!tmp);
+}
+
+static void do_test_visitor_in_qmp_introspect(TestInputVisitorData *data,
+ const char *schema_json)
+{
+ SchemaInfoList *schema = NULL;
+ Visitor *v;
+
+ v = visitor_input_test_init_raw(data, schema_json);
+
+ visit_type_SchemaInfoList(v, NULL, &schema, &error_abort);
+ g_assert(schema);
+
+ qapi_free_SchemaInfoList(schema);
+}
+
+static void test_visitor_in_qmp_introspect(TestInputVisitorData *data,
+ const void *unused)
+{
+ do_test_visitor_in_qmp_introspect(data, test_qmp_schema_json);
+ do_test_visitor_in_qmp_introspect(data, qmp_schema_json);
+}
+
int main(int argc, char **argv)
{
g_test_init(&argc, &argv, NULL);
@@ -893,6 +1062,24 @@ int main(int argc, char **argv)
NULL, test_visitor_in_native_list_string);
input_visitor_test_add("/visitor/input/native_list/number",
NULL, test_visitor_in_native_list_number);
+ input_visitor_test_add("/visitor/input/fail/struct",
+ NULL, test_visitor_in_fail_struct);
+ input_visitor_test_add("/visitor/input/fail/struct-nested",
+ NULL, test_visitor_in_fail_struct_nested);
+ input_visitor_test_add("/visitor/input/fail/struct-in-list",
+ NULL, test_visitor_in_fail_struct_in_list);
+ input_visitor_test_add("/visitor/input/fail/struct-missing",
+ NULL, test_visitor_in_fail_struct_missing);
+ input_visitor_test_add("/visitor/input/fail/union-flat",
+ NULL, test_visitor_in_fail_union_flat);
+ input_visitor_test_add("/visitor/input/fail/union-flat-no-discriminator",
+ NULL, test_visitor_in_fail_union_flat_no_discrim);
+ input_visitor_test_add("/visitor/input/fail/alternate",
+ NULL, test_visitor_in_fail_alternate);
+ input_visitor_test_add("/visitor/input/fail/union-native-list",
+ NULL, test_visitor_in_fail_union_native_list);
+ input_visitor_test_add("/visitor/input/qmp-introspect",
+ NULL, test_visitor_in_qmp_introspect);
g_test_run();
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 21/28] test-string-input-visitor: Tear down existing test automatically
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (19 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 20/28] tests-qobject-input-strict: Merge into test-qobject-input-visitor Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 22/28] test-string-input-visitor: Improve list coverage Markus Armbruster
` (7 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
Call visitor_input_teardown() from visitor_input_test_init(), so you
don't have to call it from the actual tests.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
tests/test-string-input-visitor.c | 19 ++-----------------
1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 7f10e25..a32828c 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -39,6 +39,8 @@ static
Visitor *visitor_input_test_init(TestInputVisitorData *data,
const char *string)
{
+ visitor_input_teardown(data, NULL);
+
data->v = string_input_visitor_new(string);
g_assert(data->v);
return data->v;
@@ -57,8 +59,6 @@ static void test_visitor_in_int(TestInputVisitorData *data,
g_assert(!err);
g_assert_cmpint(res, ==, value);
- visitor_input_teardown(data, unused);
-
v = visitor_input_test_init(data, "not an int");
visit_type_int(v, NULL, &res, &err);
@@ -87,8 +87,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
qapi_free_int16List(res);
- visitor_input_teardown(data, unused);
-
v = visitor_input_test_init(data, "not an int list");
visit_type_int16List(v, NULL, &res, &err);
@@ -108,35 +106,30 @@ static void test_visitor_in_bool(TestInputVisitorData *data,
visit_type_bool(v, NULL, &res, &err);
g_assert(!err);
g_assert_cmpint(res, ==, true);
- visitor_input_teardown(data, unused);
v = visitor_input_test_init(data, "yes");
visit_type_bool(v, NULL, &res, &err);
g_assert(!err);
g_assert_cmpint(res, ==, true);
- visitor_input_teardown(data, unused);
v = visitor_input_test_init(data, "on");
visit_type_bool(v, NULL, &res, &err);
g_assert(!err);
g_assert_cmpint(res, ==, true);
- visitor_input_teardown(data, unused);
v = visitor_input_test_init(data, "false");
visit_type_bool(v, NULL, &res, &err);
g_assert(!err);
g_assert_cmpint(res, ==, false);
- visitor_input_teardown(data, unused);
v = visitor_input_test_init(data, "no");
visit_type_bool(v, NULL, &res, &err);
g_assert(!err);
g_assert_cmpint(res, ==, false);
- visitor_input_teardown(data, unused);
v = visitor_input_test_init(data, "off");
@@ -190,8 +183,6 @@ static void test_visitor_in_enum(TestInputVisitorData *data,
visit_type_EnumOne(v, NULL, &res, &err);
g_assert(!err);
g_assert_cmpint(i, ==, res);
-
- visitor_input_teardown(data, NULL);
}
}
@@ -224,30 +215,24 @@ static void test_visitor_in_fuzz(TestInputVisitorData *data,
v = visitor_input_test_init(data, buf);
visit_type_int(v, NULL, &ires, NULL);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, buf);
visit_type_intList(v, NULL, &ilres, NULL);
qapi_free_intList(ilres);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, buf);
visit_type_bool(v, NULL, &bres, NULL);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, buf);
visit_type_number(v, NULL, &nres, NULL);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, buf);
sres = NULL;
visit_type_str(v, NULL, &sres, NULL);
g_free(sres);
- visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, buf);
visit_type_EnumOne(v, NULL, &eres, NULL);
- visitor_input_teardown(data, NULL);
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 22/28] test-string-input-visitor: Improve list coverage
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (20 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 21/28] test-string-input-visitor: Tear down existing test automatically Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 23/28] tests: Cover partial input visit of list Markus Armbruster
` (6 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
Lists with elements above INT64_MAX don't work (known bug). Empty
lists don't work (weird).
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
tests/test-string-input-visitor.c | 85 +++++++++++++++++++++++++++++++++------
1 file changed, 72 insertions(+), 13 deletions(-)
diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index a32828c..72f8732 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -65,31 +65,90 @@ static void test_visitor_in_int(TestInputVisitorData *data,
error_free_or_abort(&err);
}
+static void check_ilist(Visitor *v, int64_t *expected, size_t n)
+{
+ int64List *res = NULL;
+ int64List *tail;
+ int i;
+
+ visit_type_int64List(v, NULL, &res, &error_abort);
+ tail = res;
+ for (i = 0; i < n; i++) {
+ g_assert(tail);
+ g_assert_cmpint(tail->value, ==, expected[i]);
+ tail = tail->next;
+ }
+ g_assert(!tail);
+
+ qapi_free_int64List(res);
+}
+
+static void check_ulist(Visitor *v, uint64_t *expected, size_t n)
+{
+ uint64List *res = NULL;
+ uint64List *tail;
+ int i;
+
+ /* BUG: unsigned numbers above INT64_MAX don't work */
+ for (i = 0; i < n; i++) {
+ if (expected[i] > INT64_MAX) {
+ Error *err = NULL;
+ visit_type_uint64List(v, NULL, &res, &err);
+ error_free_or_abort(&err);
+ return;
+ }
+ }
+
+ visit_type_uint64List(v, NULL, &res, &error_abort);
+ tail = res;
+ for (i = 0; i < n; i++) {
+ g_assert(tail);
+ g_assert_cmpuint(tail->value, ==, expected[i]);
+ tail = tail->next;
+ }
+ g_assert(!tail);
+
+ qapi_free_uint64List(res);
+}
+
static void test_visitor_in_intList(TestInputVisitorData *data,
const void *unused)
{
- int64_t value[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20};
- int16List *res = NULL, *tmp;
+ /* Note: the visitor *sorts* ranges *unsigned* */
+ int64_t expect1[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 };
+ int64_t expect2[] = { 32767, -32768, -32767 };
+ int64_t expect3[] = { INT64_MAX, INT64_MIN };
+ uint64_t expect4[] = { UINT64_MAX };
Error *err = NULL;
+ int64List *res = NULL;
Visitor *v;
- int i = 0;
+
+ /* Valid lists */
v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
+ check_ilist(v, expect1, ARRAY_SIZE(expect1));
- visit_type_int16List(v, NULL, &res, &error_abort);
- tmp = res;
- while (i < sizeof(value) / sizeof(value[0])) {
- g_assert(tmp);
- g_assert_cmpint(tmp->value, ==, value[i++]);
- tmp = tmp->next;
- }
- g_assert(!tmp);
+ v = visitor_input_test_init(data, "32767,-32768--32767");
+ check_ilist(v, expect2, ARRAY_SIZE(expect2));
- qapi_free_int16List(res);
+ v = visitor_input_test_init(data,
+ "-9223372036854775808,9223372036854775807");
+ check_ilist(v, expect3, ARRAY_SIZE(expect3));
+
+ v = visitor_input_test_init(data, "18446744073709551615");
+ check_ulist(v, expect4, ARRAY_SIZE(expect4));
+
+ /* Empty list is invalid (weird) */
+
+ v = visitor_input_test_init(data, "");
+ visit_type_int64List(v, NULL, &res, &err);
+ error_free_or_abort(&err);
+
+ /* Not a list */
v = visitor_input_test_init(data, "not an int list");
- visit_type_int16List(v, NULL, &res, &err);
+ visit_type_int64List(v, NULL, &res, &err);
error_free_or_abort(&err);
g_assert(!res);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 23/28] tests: Cover partial input visit of list
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (21 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 22/28] test-string-input-visitor: Improve list coverage Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 24/28] test-qobject-input-visitor: Cover missing nested struct member Markus Armbruster
` (5 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
Demonstrates a design flaw: there is no way to for input visitors to
report that a list visit didn't visit the complete input list. The
generated list visits always do, but manual visits needn't.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
tests/test-opts-visitor.c | 41 +++++++++++++++++++++++++++++++++++
tests/test-qobject-input-visitor.c | 44 ++++++++++++++++++++++++++++++++++++++
tests/test-string-input-visitor.c | 22 +++++++++++++++++++
3 files changed, 107 insertions(+)
diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
index 0a9e75f..d0f7646 100644
--- a/tests/test-opts-visitor.c
+++ b/tests/test-opts-visitor.c
@@ -172,6 +172,44 @@ expect_u64_max(OptsVisitorFixture *f, gconstpointer test_data)
/* test cases */
+static void
+test_opts_range_unvisited(void)
+{
+ intList *list = NULL;
+ intList *tail;
+ QemuOpts *opts;
+ Visitor *v;
+
+ opts = qemu_opts_parse(qemu_find_opts("userdef"), "ilist=0-2", false,
+ &error_abort);
+
+ v = opts_visitor_new(opts);
+
+ visit_start_struct(v, NULL, NULL, 0, &error_abort);
+
+ /* Would be simpler if the visitor genuinely supported virtual walks */
+ visit_start_list(v, "ilist", (GenericList **)&list, sizeof(*list),
+ &error_abort);
+ tail = list;
+ visit_type_int(v, NULL, &tail->value, &error_abort);
+ g_assert_cmpint(tail->value, ==, 0);
+ tail = (intList *)visit_next_list(v, (GenericList *)tail, sizeof(*list));
+ g_assert(tail);
+ visit_type_int(v, NULL, &tail->value, &error_abort);
+ g_assert_cmpint(tail->value, ==, 1);
+ tail = (intList *)visit_next_list(v, (GenericList *)tail, sizeof(*list));
+ g_assert(tail);
+ visit_end_list(v, (void **)&list);
+ /* BUG: unvisited tail not reported; actually not reportable by design */
+
+ visit_check_struct(v, &error_abort);
+ visit_end_struct(v, NULL);
+
+ qapi_free_intList(list);
+ visit_free(v);
+ qemu_opts_del(opts);
+}
+
int
main(int argc, char **argv)
{
@@ -263,6 +301,9 @@ main(int argc, char **argv)
add_test("/visitor/opts/i64/range/2big/full", &expect_fail,
"i64=-0x8000000000000000-0x7fffffffffffffff");
+ g_test_add_func("/visitor/opts/range/unvisited",
+ test_opts_range_unvisited);
+
g_test_run();
return 0;
}
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 32c6b3d..10c15c4 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -923,6 +923,46 @@ static void test_visitor_in_fail_struct_missing(TestInputVisitorData *data,
visit_end_struct(v, NULL);
}
+static void test_visitor_in_fail_list(TestInputVisitorData *data,
+ const void *unused)
+{
+ int64_t i64 = -1;
+ Visitor *v;
+
+ /* Unvisited list tail */
+
+ v = visitor_input_test_init(data, "[ 1, 2, 3 ]");
+
+ visit_start_list(v, NULL, NULL, 0, &error_abort);
+ visit_type_int(v, NULL, &i64, &error_abort);
+ g_assert_cmpint(i64, ==, 1);
+ visit_type_int(v, NULL, &i64, &error_abort);
+ g_assert_cmpint(i64, ==, 2);
+ visit_end_list(v, NULL);
+ /* BUG: unvisited tail not reported; actually not reportable by design */
+}
+
+static void test_visitor_in_fail_list_nested(TestInputVisitorData *data,
+ const void *unused)
+{
+ int64_t i64 = -1;
+ Visitor *v;
+
+ /* Unvisited nested list tail */
+
+ v = visitor_input_test_init(data, "[ 0, [ 1, 2, 3 ] ]");
+
+ visit_start_list(v, NULL, NULL, 0, &error_abort);
+ visit_type_int(v, NULL, &i64, &error_abort);
+ g_assert_cmpint(i64, ==, 0);
+ visit_start_list(v, NULL, NULL, 0, &error_abort);
+ visit_type_int(v, NULL, &i64, &error_abort);
+ g_assert_cmpint(i64, ==, 1);
+ visit_end_list(v, NULL);
+ /* BUG: unvisited tail not reported; actually not reportable by design */
+ visit_end_list(v, NULL);
+}
+
static void test_visitor_in_fail_union_native_list(TestInputVisitorData *data,
const void *unused)
{
@@ -1070,6 +1110,10 @@ int main(int argc, char **argv)
NULL, test_visitor_in_fail_struct_in_list);
input_visitor_test_add("/visitor/input/fail/struct-missing",
NULL, test_visitor_in_fail_struct_missing);
+ input_visitor_test_add("/visitor/input/fail/list",
+ NULL, test_visitor_in_fail_list);
+ input_visitor_test_add("/visitor/input/fail/list-nested",
+ NULL, test_visitor_in_fail_list_nested);
input_visitor_test_add("/visitor/input/fail/union-flat",
NULL, test_visitor_in_fail_union_flat);
input_visitor_test_add("/visitor/input/fail/union-flat-no-discriminator",
diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 72f8732..70cee65 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -121,6 +121,7 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
uint64_t expect4[] = { UINT64_MAX };
Error *err = NULL;
int64List *res = NULL;
+ int64List *tail;
Visitor *v;
/* Valid lists */
@@ -151,6 +152,27 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
visit_type_int64List(v, NULL, &res, &err);
error_free_or_abort(&err);
g_assert(!res);
+
+ /* Unvisited list tail */
+
+ v = visitor_input_test_init(data, "0,2-3");
+
+ /* Would be simpler if the visitor genuinely supported virtual walks */
+ visit_start_list(v, NULL, (GenericList **)&res, sizeof(*res),
+ &error_abort);
+ tail = res;
+ visit_type_int64(v, NULL, &tail->value, &error_abort);
+ g_assert_cmpint(tail->value, ==, 0);
+ tail = (int64List *)visit_next_list(v, (GenericList *)tail, sizeof(*res));
+ g_assert(tail);
+ visit_type_int64(v, NULL, &tail->value, &error_abort);
+ g_assert_cmpint(tail->value, ==, 2);
+ tail = (int64List *)visit_next_list(v, (GenericList *)tail, sizeof(*res));
+ g_assert(tail);
+ visit_end_list(v, (void **)&res);
+ /* BUG: unvisited tail not reported; actually not reportable by design */
+
+ qapi_free_int64List(res);
}
static void test_visitor_in_bool(TestInputVisitorData *data,
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 24/28] test-qobject-input-visitor: Cover missing nested struct member
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (22 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 23/28] tests: Cover partial input visit of list Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 18:45 ` Eric Blake
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails Markus Armbruster
` (4 subsequent siblings)
28 siblings, 1 reply; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/test-qobject-input-visitor.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 10c15c4..9f3a826 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -894,7 +894,7 @@ static void test_visitor_in_fail_struct_missing(TestInputVisitorData *data,
char *str;
double dbl;
- v = visitor_input_test_init(data, "{}");
+ v = visitor_input_test_init(data, "{ 'sub': [ {} ] }");
visit_start_struct(v, NULL, NULL, 0, &error_abort);
visit_start_struct(v, "struct", NULL, 0, &err);
error_free_or_abort(&err);
@@ -920,6 +920,12 @@ static void test_visitor_in_fail_struct_missing(TestInputVisitorData *data,
error_free_or_abort(&err);
visit_type_null(v, "null", &err);
error_free_or_abort(&err);
+ visit_start_list(v, "sub", NULL, 0, &error_abort);
+ visit_start_struct(v, NULL, NULL, 0, &error_abort);
+ visit_type_int(v, "i64", &i64, &err);
+ error_free_or_abort(&err);
+ visit_end_struct(v, NULL);
+ visit_end_list(v, NULL);
visit_end_struct(v, NULL);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (23 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 24/28] test-qobject-input-visitor: Cover missing nested struct member Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 19:15 ` Eric Blake
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 26/28] tests: Cover input visit beyond end of list Markus Armbruster
` (3 subsequent siblings)
28 siblings, 1 reply; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
Fix the design flaw demonstrated in the previous commit: new method
check_list() lets input visitors report that unvisited input remains
for a list, exactly like check_struct() lets them report that
unvisited input remains for a struct or union.
Implement the method for the qobject input visitor (straightforward),
and the string input visitor (less so, due to the magic list syntax
there). The opts visitor's list magic is even more impenetrable, and
all I can do there today is a stub with a FIXME comment. No worse
than before.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/ppc/spapr_drc.c | 5 +++++
include/qapi/visitor-impl.h | 3 +++
include/qapi/visitor.h | 13 +++++++++++++
qapi/opts-visitor.c | 11 +++++++++++
qapi/qapi-visit-core.c | 8 ++++++++
qapi/qobject-input-visitor.c | 37 +++++++++++++++++++++++++++++++------
qapi/string-input-visitor.c | 30 ++++++++++++++++++++++++++++++
qapi/trace-events | 1 +
scripts/qapi-visit.py | 3 +++
tests/test-opts-visitor.c | 2 +-
tests/test-qobject-input-visitor.c | 9 +++++++--
tests/test-string-input-visitor.c | 4 +++-
12 files changed, 116 insertions(+), 10 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 2de6377..150f6bf 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -326,7 +326,12 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
return;
}
}
+ visit_check_list(v, &err);
visit_end_list(v, NULL);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
break;
}
default:
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 962ba1d..e87709d 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -61,6 +61,9 @@ struct Visitor
/* Must be set */
GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size);
+ /* Optional; intended for input visitors */
+ void (*check_list)(Visitor *v, Error **errp);
+
/* Must be set */
void (*end_list)(Visitor *v, void **list);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 7c91a50..1a1b620 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -370,6 +370,19 @@ void visit_start_list(Visitor *v, const char *name, GenericList **list,
GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size);
/*
+ * Prepare for completing a list visit.
+ *
+ * @errp obeys typical error usage, and reports failures such as
+ * unvisited list tail remaining in the input stream.
+ *
+ * Should be called prior to visit_end_list() if all other
+ * intermediate visit steps were successful, to allow the visitor one
+ * last chance to report errors. May be skipped on a cleanup path,
+ * where there is no need to check for further errors.
+ */
+void visit_check_list(Visitor *v, Error **errp);
+
+/*
* Complete a list visit started earlier.
*
* @list must match what was passed to the paired visit_start_list().
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index c50dc4b..026d25b 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -273,6 +273,16 @@ opts_next_list(Visitor *v, GenericList *tail, size_t size)
static void
+opts_check_list(Visitor *v, Error **errp)
+{
+ /*
+ * FIXME should set error when unvisited elements remain. Mostly
+ * harmless, as the generated visits always visit all elements.
+ */
+}
+
+
+static void
opts_end_list(Visitor *v, void **obj)
{
OptsVisitor *ov = to_ov(v);
@@ -539,6 +549,7 @@ opts_visitor_new(const QemuOpts *opts)
ov->visitor.start_list = &opts_start_list;
ov->visitor.next_list = &opts_next_list;
+ ov->visitor.check_list = &opts_check_list;
ov->visitor.end_list = &opts_end_list;
ov->visitor.type_int64 = &opts_type_int64;
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index e6e93f0..43a09d1 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -90,6 +90,14 @@ GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size)
return v->next_list(v, tail, size);
}
+void visit_check_list(Visitor *v, Error **errp)
+{
+ trace_visit_check_list(v);
+ if (v->check_list) {
+ v->check_list(v, errp);
+ }
+}
+
void visit_end_list(Visitor *v, void **obj)
{
trace_visit_end_list(v, obj);
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index eafcdf4..34065ba 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -51,7 +51,8 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
return container_of(v, QObjectInputVisitor, visitor);
}
-static const char *full_name(QObjectInputVisitor *qiv, const char *name)
+static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
+ int n)
{
StackObject *so;
char buf[32];
@@ -63,8 +64,10 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name)
}
QSLIST_FOREACH(so , &qiv->stack, node) {
- if (qobject_type(so->obj) == QTYPE_QDICT) {
- g_string_prepend(qiv->errname, name);
+ if (n) {
+ n--;
+ } else if (qobject_type(so->obj) == QTYPE_QDICT) {
+ g_string_prepend(qiv->errname, name ?: "<anonymous>");
g_string_prepend_c(qiv->errname, '.');
} else {
snprintf(buf, sizeof(buf), "[%u]", so->index);
@@ -72,18 +75,24 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name)
}
name = so->name;
}
+ assert(!n);
if (name) {
g_string_prepend(qiv->errname, name);
} else if (qiv->errname->str[0] == '.') {
g_string_erase(qiv->errname, 0, 1);
- } else {
+ } else if (!qiv->errname->str[0]) {
return "<anonymous>";
}
return qiv->errname->str;
}
+static const char *full_name(QObjectInputVisitor *qiv, const char *name)
+{
+ return full_name_nth(qiv, name, 0);
+}
+
static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
const char *name,
bool consume)
@@ -260,15 +269,30 @@ static GenericList *qobject_input_next_list(Visitor *v, GenericList *tail,
size_t size)
{
QObjectInputVisitor *qiv = to_qiv(v);
- StackObject *so = QSLIST_FIRST(&qiv->stack);
+ StackObject *tos = QSLIST_FIRST(&qiv->stack);
- if (!so->entry) {
+ assert(tos && tos->obj && qobject_type(tos->obj) == QTYPE_QLIST);
+
+ if (!tos->entry) {
return NULL;
}
tail->next = g_malloc0(size);
return tail->next;
}
+static void qobject_input_check_list(Visitor *v, Error **errp)
+{
+ QObjectInputVisitor *qiv = to_qiv(v);
+ StackObject *tos = QSLIST_FIRST(&qiv->stack);
+
+ assert(tos && tos->obj && qobject_type(tos->obj) == QTYPE_QLIST);
+
+ if (tos->entry) {
+ error_setg(errp, "Only %u list elements expected in %s",
+ tos->index + 1, full_name_nth(qiv, NULL, 1));
+ }
+}
+
static void qobject_input_start_alternate(Visitor *v, const char *name,
GenericAlternate **obj, size_t size,
@@ -471,6 +495,7 @@ Visitor *qobject_input_visitor_new(QObject *obj)
v->visitor.end_struct = qobject_input_pop;
v->visitor.start_list = qobject_input_start_list;
v->visitor.next_list = qobject_input_next_list;
+ v->visitor.check_list = qobject_input_check_list;
v->visitor.end_list = qobject_input_pop;
v->visitor.start_alternate = qobject_input_start_alternate;
v->visitor.type_int64 = qobject_input_type_int64;
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index f126cd9..806b01ae 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -170,6 +170,35 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
return tail->next;
}
+static void check_list(Visitor *v, Error **errp)
+{
+ const StringInputVisitor *siv = to_siv(v);
+ Range *r;
+ GList *cur_range;
+
+ if (!siv->ranges || !siv->cur_range) {
+ return;
+ }
+
+ r = siv->cur_range->data;
+ if (!r) {
+ return;
+ }
+
+ if (!range_contains(r, siv->cur)) {
+ cur_range = g_list_next(siv->cur_range);
+ if (!cur_range) {
+ return;
+ }
+ r = cur_range->data;
+ if (!r) {
+ return;
+ }
+ }
+
+ error_setg(errp, "Range contains too many values");
+}
+
static void end_list(Visitor *v, void **obj)
{
StringInputVisitor *siv = to_siv(v);
@@ -318,6 +347,7 @@ Visitor *string_input_visitor_new(const char *str)
v->visitor.type_number = parse_type_number;
v->visitor.start_list = start_list;
v->visitor.next_list = next_list;
+ v->visitor.check_list = check_list;
v->visitor.end_list = end_list;
v->visitor.free = string_input_free;
diff --git a/qapi/trace-events b/qapi/trace-events
index 9cbb61b..339cacf 100644
--- a/qapi/trace-events
+++ b/qapi/trace-events
@@ -8,6 +8,7 @@ visit_end_struct(void *v, void *obj) "v=%p obj=%p"
visit_start_list(void *v, const char *name, void *obj, size_t size) "v=%p name=%s obj=%p size=%zu"
visit_next_list(void *v, void *tail, size_t size) "v=%p tail=%p size=%zu"
+visit_check_list(void *v) "v=%p"
visit_end_list(void *v, void *obj) "v=%p obj=%p"
visit_start_alternate(void *v, const char *name, void *obj, size_t size, bool promote_int) "v=%p name=%s obj=%p size=%zu promote_int=%d"
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 96f2491..330b9f3 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -133,6 +133,9 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
}
}
+ if (!err) {
+ visit_check_list(v, &err);
+ }
visit_end_list(v, (void **)obj);
if (err && visit_is_input(v)) {
qapi_free_%(c_name)s(*obj);
diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
index d0f7646..b93fd33 100644
--- a/tests/test-opts-visitor.c
+++ b/tests/test-opts-visitor.c
@@ -199,8 +199,8 @@ test_opts_range_unvisited(void)
g_assert_cmpint(tail->value, ==, 1);
tail = (intList *)visit_next_list(v, (GenericList *)tail, sizeof(*list));
g_assert(tail);
+ visit_check_list(v, &error_abort); /* BUG: unvisited tail not reported */
visit_end_list(v, (void **)&list);
- /* BUG: unvisited tail not reported; actually not reportable by design */
visit_check_struct(v, &error_abort);
visit_end_struct(v, NULL);
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 9f3a826..87d4a77 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -933,6 +933,7 @@ static void test_visitor_in_fail_list(TestInputVisitorData *data,
const void *unused)
{
int64_t i64 = -1;
+ Error *err = NULL;
Visitor *v;
/* Unvisited list tail */
@@ -944,14 +945,16 @@ static void test_visitor_in_fail_list(TestInputVisitorData *data,
g_assert_cmpint(i64, ==, 1);
visit_type_int(v, NULL, &i64, &error_abort);
g_assert_cmpint(i64, ==, 2);
+ visit_check_list(v, &err);
+ error_free_or_abort(&err);
visit_end_list(v, NULL);
- /* BUG: unvisited tail not reported; actually not reportable by design */
}
static void test_visitor_in_fail_list_nested(TestInputVisitorData *data,
const void *unused)
{
int64_t i64 = -1;
+ Error *err = NULL;
Visitor *v;
/* Unvisited nested list tail */
@@ -964,8 +967,10 @@ static void test_visitor_in_fail_list_nested(TestInputVisitorData *data,
visit_start_list(v, NULL, NULL, 0, &error_abort);
visit_type_int(v, NULL, &i64, &error_abort);
g_assert_cmpint(i64, ==, 1);
+ visit_check_list(v, &err);
+ error_free_or_abort(&err);
visit_end_list(v, NULL);
- /* BUG: unvisited tail not reported; actually not reportable by design */
+ visit_check_list(v, &error_abort);
visit_end_list(v, NULL);
}
diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 70cee65..fbe380a 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -169,8 +169,10 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
g_assert_cmpint(tail->value, ==, 2);
tail = (int64List *)visit_next_list(v, (GenericList *)tail, sizeof(*res));
g_assert(tail);
+
+ visit_check_list(v, &err);
+ error_free_or_abort(&err);
visit_end_list(v, (void **)&res);
- /* BUG: unvisited tail not reported; actually not reportable by design */
qapi_free_int64List(res);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 26/28] tests: Cover input visit beyond end of list
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (24 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 27/28] qapi: Fix object " Markus Armbruster
` (2 subsequent siblings)
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
When you try to visit beyond the end of a list, the qobject input
visitor crashes, and the string visitor screws returns garbage. The
generated list visits never go beyond the list end, but manual visits
could.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
tests/test-opts-visitor.c | 39 ++++++++++++++++++++++++++++++++++++++
tests/test-qobject-input-visitor.c | 10 ++++++++++
tests/test-string-input-visitor.c | 16 ++++++++++++++++
3 files changed, 65 insertions(+)
diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
index b93fd33..2238f8e 100644
--- a/tests/test-opts-visitor.c
+++ b/tests/test-opts-visitor.c
@@ -210,6 +210,43 @@ test_opts_range_unvisited(void)
qemu_opts_del(opts);
}
+static void
+test_opts_range_beyond(void)
+{
+ Error *err = NULL;
+ intList *list = NULL;
+ intList *tail;
+ QemuOpts *opts;
+ Visitor *v;
+ int64_t val;
+
+ opts = qemu_opts_parse(qemu_find_opts("userdef"), "ilist=0", false,
+ &error_abort);
+
+ v = opts_visitor_new(opts);
+
+ visit_start_struct(v, NULL, NULL, 0, &error_abort);
+
+ /* Would be simpler if the visitor genuinely supported virtual walks */
+ visit_start_list(v, "ilist", (GenericList **)&list, sizeof(*list),
+ &error_abort);
+ tail = list;
+ visit_type_int(v, NULL, &tail->value, &error_abort);
+ g_assert_cmpint(tail->value, ==, 0);
+ tail = (intList *)visit_next_list(v, (GenericList *)tail, sizeof(*tail));
+ g_assert(!tail);
+ visit_type_int(v, NULL, &val, &err);
+ error_free_or_abort(&err);
+ visit_end_list(v, (void **)&list);
+
+ visit_check_struct(v, &error_abort);
+ visit_end_struct(v, NULL);
+
+ qapi_free_intList(list);
+ visit_free(v);
+ qemu_opts_del(opts);
+}
+
int
main(int argc, char **argv)
{
@@ -303,6 +340,8 @@ main(int argc, char **argv)
g_test_add_func("/visitor/opts/range/unvisited",
test_opts_range_unvisited);
+ g_test_add_func("/visitor/opts/range/beyond",
+ test_opts_range_beyond);
g_test_run();
return 0;
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 87d4a77..8011baa 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -948,6 +948,16 @@ static void test_visitor_in_fail_list(TestInputVisitorData *data,
visit_check_list(v, &err);
error_free_or_abort(&err);
visit_end_list(v, NULL);
+
+ /* Visit beyond end of list */
+ v = visitor_input_test_init(data, "[]");
+
+ visit_start_list(v, NULL, NULL, 0, &error_abort);
+#if 0 /* FIXME crash */
+ visit_type_int(v, NULL, &i64, &err);
+ error_free_or_abort(&err);
+#endif
+ visit_end_list(v, NULL);
}
static void test_visitor_in_fail_list_nested(TestInputVisitorData *data,
diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index fbe380a..6db850b 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -123,6 +123,7 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
int64List *res = NULL;
int64List *tail;
Visitor *v;
+ int64_t val;
/* Valid lists */
@@ -175,6 +176,21 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
visit_end_list(v, (void **)&res);
qapi_free_int64List(res);
+
+ /* Visit beyond end of list */
+ v = visitor_input_test_init(data, "0");
+
+ visit_start_list(v, NULL, (GenericList **)&res, sizeof(*res),
+ &error_abort);
+ tail = res;
+ visit_type_int64(v, NULL, &tail->value, &err);
+ g_assert_cmpint(tail->value, ==, 0);
+ visit_type_int64(v, NULL, &val, &err);
+ g_assert_cmpint(val, ==, 1); /* BUG */
+ visit_check_list(v, &error_abort);
+ visit_end_list(v, (void **)&res);
+
+ qapi_free_int64List(res);
}
static void test_visitor_in_bool(TestInputVisitorData *data,
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 27/28] qapi: Fix object input visit beyond end of list
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (25 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 26/28] tests: Cover input visit beyond end of list Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 28/28] qapi: Improve qobject visitor documentation Markus Armbruster
2017-03-03 12:58 ` [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work no-reply
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qapi/qobject-input-visitor.c | 11 ++++++++---
tests/test-qobject-input-visitor.c | 2 --
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 34065ba..d192727 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -122,10 +122,15 @@ static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
} else {
assert(qobject_type(qobj) == QTYPE_QLIST);
assert(!name);
- ret = qlist_entry_obj(tos->entry);
- assert(ret);
+ if (tos->entry) {
+ ret = qlist_entry_obj(tos->entry);
+ if (consume) {
+ tos->entry = qlist_next(tos->entry);
+ }
+ } else {
+ ret = NULL;
+ }
if (consume) {
- tos->entry = qlist_next(tos->entry);
tos->index++;
}
}
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 8011baa..94305f5 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -953,10 +953,8 @@ static void test_visitor_in_fail_list(TestInputVisitorData *data,
v = visitor_input_test_init(data, "[]");
visit_start_list(v, NULL, NULL, 0, &error_abort);
-#if 0 /* FIXME crash */
visit_type_int(v, NULL, &i64, &err);
error_free_or_abort(&err);
-#endif
visit_end_list(v, NULL);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v4 28/28] qapi: Improve qobject visitor documentation
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (26 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 27/28] qapi: Fix object " Markus Armbruster
@ 2017-03-03 12:32 ` Markus Armbruster
2017-03-03 12:58 ` [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work no-reply
28 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 12:32 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
include/qapi/qobject-input-visitor.h | 37 ++++++++++++++++++++++++++++++++++-
include/qapi/qobject-output-visitor.h | 35 +++++++++++++++++++++++++++++----
2 files changed, 67 insertions(+), 5 deletions(-)
diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h
index 21db9c4..0b7633a 100644
--- a/include/qapi/qobject-input-visitor.h
+++ b/include/qapi/qobject-input-visitor.h
@@ -1,6 +1,7 @@
/*
* Input Visitor
*
+ * Copyright (C) 2017 Red Hat, Inc.
* Copyright IBM, Corp. 2011
*
* Authors:
@@ -20,7 +21,41 @@
typedef struct QObjectInputVisitor QObjectInputVisitor;
/*
- * Return a new input visitor that converts a QObject to a QAPI object.
+ * Create a QObject input visitor for @obj
+ *
+ * A QObject input visitor visit builds a QAPI object from a QObject.
+ * This simultaneously walks the QAPI object being built and the
+ * QObject. The latter walk starts at @obj.
+ *
+ * visit_type_FOO() creates an instance of QAPI type FOO. The visited
+ * QObject must match FOO. QDict matches struct/union types, QList
+ * matches list types, QString matches type 'str' and enumeration
+ * types, QInt matches integer types, QFloat matches type 'number',
+ * QBool matches type 'bool'. Type 'any' is matched by QObject. A
+ * QAPI alternate type is matched when one of its member types is.
+ *
+ * visit_start_struct() ... visit_end_struct() visits a QDict and
+ * creates a QAPI struct/union. Visits in between visit the
+ * dictionary members. visit_optional() is true when the QDict has
+ * this member. visit_check_struct() fails if unvisited members
+ * remain.
+ *
+ * visit_start_list() ... visit_end_list() visits a QList and creates
+ * a QAPI list. Visits in between visit list members, one after the
+ * other. visit_next_list() returns NULL when all QList members have
+ * been visited. visit_check_list() fails if unvisited members
+ * remain.
+ *
+ * visit_start_alternate() ... visit_end_alternate() visits a QObject
+ * and creates a QAPI alternate. The visit in between visits the same
+ * QObject and initializes the alternate member that is in use.
+ *
+ * Error messages refer to parts of @obj in JavaScript/Python syntax.
+ * For example, 'a.b[2]' refers to the second member of the QList
+ * member 'b' of the QDict member 'a' of QDict @obj.
+ *
+ * The caller is responsible for freeing the visitor with
+ * visit_free().
*/
Visitor *qobject_input_visitor_new(QObject *obj);
diff --git a/include/qapi/qobject-output-visitor.h b/include/qapi/qobject-output-visitor.h
index 8241877..9b990c3 100644
--- a/include/qapi/qobject-output-visitor.h
+++ b/include/qapi/qobject-output-visitor.h
@@ -19,11 +19,38 @@
typedef struct QObjectOutputVisitor QObjectOutputVisitor;
-/*
- * Create a new QObject output visitor.
+/**
+ * Create a QObject output visitor for @obj
*
- * If everything else succeeds, pass @result to visit_complete() to
- * collect the result of the visit.
+ * A QObject output visitor visit builds a QObject from QAPI Object.
+ * This simultaneously walks the QAPI object and the QObject being
+ * built. The latter walk starts at @obj.
+ *
+ * visit_type_FOO() creates a QObject for QAPI type FOO. It creates a
+ * QDict for struct/union types, a QList for list types, QString for
+ * type 'str' and enumeration types, QInt for integer types, QFloat
+ * for type 'number', QBool for type 'bool'. For type 'any', it
+ * increments the QObject's reference count. For QAPI alternate
+ * types, it creates the QObject for the member that is in use.
+ *
+ * visit_start_struct() ... visit_end_struct() visits a QAPI
+ * struct/union and creates a QDict. Visits in between visit the
+ * members. visit_optional() is true when the struct/union has this
+ * member. visit_check_struct() does nothing.
+ *
+ * visit_start_list() ... visit_end_list() visits a QAPI list and
+ * creates a QList. Visits in between visit list members, one after
+ * the other. visit_next_list() returns NULL when all QAPI list
+ * members have been visited. visit_check_list() does nothing.
+ *
+ * visit_start_alternate() ... visit_end_alternate() visits a QAPI
+ * alternate. The visit in between creates the QObject for the
+ * alternate member that is in use.
+ *
+ * Errors are not expected to happen.
+ *
+ * The caller is responsible for freeing the visitor with
+ * visit_free().
*/
Visitor *qobject_output_visitor_new(QObject **result);
--
2.7.4
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
` (27 preceding siblings ...)
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 28/28] qapi: Improve qobject visitor documentation Markus Armbruster
@ 2017-03-03 12:58 ` no-reply
28 siblings, 0 replies; 53+ messages in thread
From: no-reply @ 2017-03-03 12:58 UTC (permalink / raw)
To: armbru; +Cc: famz, qemu-devel
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 1488544368-30622-1-git-send-email-armbru@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/1488544368-30622-1-git-send-email-armbru@redhat.com -> patchew/1488544368-30622-1-git-send-email-armbru@redhat.com
Switched to a new branch 'test'
0fcdaa8 qapi: Improve qobject visitor documentation
2302604 qapi: Fix object input visit beyond end of list
ed9dde1 tests: Cover input visit beyond end of list
833e4de qapi: Make input visitors detect unvisited list tails
e307180 test-qobject-input-visitor: Cover missing nested struct member
f75af8b tests: Cover partial input visit of list
0251f6c test-string-input-visitor: Improve list coverage
20cde7c test-string-input-visitor: Tear down existing test automatically
17938fd tests-qobject-input-strict: Merge into test-qobject-input-visitor
d50cfe3 qapi: Drop unused non-strict qobject input visitor
b5bd9d0 test-qobject-input-visitor: Use strict visitor
ce66a1a qom: Make object_property_set_qobject()'s input visitor strict
7e59b7b qapi: Make string input and opts visitor require non-null input
681dfe6 qapi: Drop string input visitor method optional()
5c003fd qapi: Improve qobject input visitor error reporting
f518cf0 qapi: Make QObject input visitor set *list reliably
ec5936a qapi: Clean up after commit 3d344c2
5449a43 qapi: Improve a QObject input visitor error message
4347bc9 qmp: Improve QMP dispatch error messages
b6359af qmp: Eliminate silly QERR_QMP_* macros
c6ba9d0 qmp: Drop duplicated QMP command object checks
7e52b24 qmp: Clean up how we enforce capability negotiation
eb66f8c qapi-introspect: Mangle --prefix argument properly for C
84ddc51 qapi: Support multiple command registries per program
0725a24 qmp: Dumb down how we run QMP command registration
1404809 qmp-test: New, covering basic QMP protocol
5fe3949 libqtest: Work around a "QMP wants a newline" bug
c06623c qga: Fix crash on non-dictionary QMP argument
=== OUTPUT BEGIN ===
Checking PATCH 1/28: qga: Fix crash on non-dictionary QMP argument...
Checking PATCH 2/28: libqtest: Work around a "QMP wants a newline" bug...
Checking PATCH 3/28: qmp-test: New, covering basic QMP protocol...
Checking PATCH 4/28: qmp: Dumb down how we run QMP command registration...
Checking PATCH 5/28: qapi: Support multiple command registries per program...
ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
#143: FILE: qapi/qmp-dispatch.c:70:
+volatile QmpCommand *save_cmd;
ERROR: trailing whitespace
#420: FILE: scripts/qapi-commands.py:201:
+ qmp_register_command(cmds, "%(name)s", $
total: 2 errors, 0 warnings, 435 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 6/28: qapi-introspect: Mangle --prefix argument properly for C...
Checking PATCH 7/28: qmp: Clean up how we enforce capability negotiation...
Checking PATCH 8/28: qmp: Drop duplicated QMP command object checks...
Checking PATCH 9/28: qmp: Eliminate silly QERR_QMP_* macros...
Checking PATCH 10/28: qmp: Improve QMP dispatch error messages...
Checking PATCH 11/28: qapi: Improve a QObject input visitor error message...
Checking PATCH 12/28: qapi: Clean up after commit 3d344c2...
Checking PATCH 13/28: qapi: Make QObject input visitor set *list reliably...
Checking PATCH 14/28: qapi: Improve qobject input visitor error reporting...
Checking PATCH 15/28: qapi: Drop string input visitor method optional()...
Checking PATCH 16/28: qapi: Make string input and opts visitor require non-null input...
Checking PATCH 17/28: qom: Make object_property_set_qobject()'s input visitor strict...
Checking PATCH 18/28: test-qobject-input-visitor: Use strict visitor...
Checking PATCH 19/28: qapi: Drop unused non-strict qobject input visitor...
Checking PATCH 20/28: tests-qobject-input-strict: Merge into test-qobject-input-visitor...
ERROR: line over 90 characters
#486: FILE: tests/test-qobject-input-visitor.c:847:
+ v = visitor_input_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo', 'extra': 42 }");
ERROR: line over 90 characters
#500: FILE: tests/test-qobject-input-visitor.c:861:
+ v = visitor_input_test_init(data, "{ 'string0': 'string0', 'dict1': { 'string1': 'string1', 'dict2': { 'userdef1': { 'integer': 42, 'string': 'string', 'extra': [42, 23, {'foo':'bar'}] }, 'string2': 'string2'}}}");
ERROR: line over 90 characters
#514: FILE: tests/test-qobject-input-visitor.c:875:
+ v = visitor_input_test_init(data, "[ { 'string': 'string0', 'integer': 42 }, { 'string': 'string1', 'integer': 43 }, { 'string': 'string2', 'integer': 44, 'extra': 'ggg' } ]");
ERROR: line over 90 characters
#587: FILE: tests/test-qobject-input-visitor.c:948:
+ v = visitor_input_test_init(data, "{ 'string': 'c', 'integer': 41, 'boolean': true }");
WARNING: line over 80 characters
#594: FILE: tests/test-qobject-input-visitor.c:955:
+static void test_visitor_in_fail_union_flat_no_discrim(TestInputVisitorData *data,
ERROR: line over 90 characters
#602: FILE: tests/test-qobject-input-visitor.c:963:
+ v = visitor_input_test_init(data, "{ 'integer': 42, 'string': 'c', 'string1': 'd', 'string2': 'e' }");
total: 5 errors, 1 warnings, 233 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 21/28: test-string-input-visitor: Tear down existing test automatically...
Checking PATCH 22/28: test-string-input-visitor: Improve list coverage...
Checking PATCH 23/28: tests: Cover partial input visit of list...
Checking PATCH 24/28: test-qobject-input-visitor: Cover missing nested struct member...
Checking PATCH 25/28: qapi: Make input visitors detect unvisited list tails...
Checking PATCH 26/28: tests: Cover input visit beyond end of list...
ERROR: if this code is redundant consider removing it
#85: FILE: tests/test-qobject-input-visitor.c:956:
+#if 0 /* FIXME crash */
total: 1 errors, 0 warnings, 95 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 27/28: qapi: Fix object input visit beyond end of list...
Checking PATCH 28/28: qapi: Improve qobject visitor documentation...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 05/28] qapi: Support multiple command registries per program
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 05/28] qapi: Support multiple command registries per program Markus Armbruster
@ 2017-03-03 18:24 ` Eric Blake
2017-03-03 19:37 ` Markus Armbruster
0 siblings, 1 reply; 53+ messages in thread
From: Eric Blake @ 2017-03-03 18:24 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2593 bytes --]
On 03/03/2017 06:32 AM, Markus Armbruster wrote:
> The command registry encapsulates a single command list. Give the
> functions using it a parameter instead. Define suitable command lists
> in monitor, guest agent and test-qmp-commands.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/qapi/qmp/dispatch.h | 22 ++++++++++++++--------
> monitor.c | 31 +++++++++++++++++--------------
> qapi/qmp-dispatch.c | 17 +++++++++++++----
> qapi/qmp-registry.c | 37 ++++++++++++++++++-------------------
> qga/commands.c | 2 +-
> qga/guest-agent-core.h | 2 ++
> qga/main.c | 19 ++++++++++---------
> scripts/qapi-commands.py | 16 ++++++++++------
> tests/test-qmp-commands.c | 12 +++++++-----
> 9 files changed, 92 insertions(+), 66 deletions(-)
>
> +++ b/qapi/qmp-dispatch.c
> @@ -67,7 +67,11 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
> return dict;
> }
>
> -static QObject *do_qmp_dispatch(QObject *request, Error **errp)
> +volatile QmpCommand *save_cmd;
A comment why volatile is necessary would be nice...
> +QmpCommand cmd2;
> +
> +static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
> + Error **errp)
> {
> Error *local_err = NULL;
> const char *command;
> @@ -81,7 +85,7 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp)
> }
>
> command = qdict_get_str(dict, "execute");
> - cmd = qmp_find_command(command);
> + cmd = qmp_find_command(cmds, command);
> if (cmd == NULL) {
> error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
> "The command %s has not been found", command);
> @@ -93,6 +97,9 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp)
> return NULL;
> }
>
> + assert(!cmd->options & QCO_NO_SUCCESS_RESP);
> + save_cmd = cmd;
> + cmd2 = *cmd;
> if (!qdict_haskey(dict, "arguments")) {
> args = qdict_new();
> } else {
> @@ -111,6 +118,8 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp)
>
> QDECREF(args);
>
> + assert(!cmd->options & QCO_NO_SUCCESS_RESP);
> + assert(ret || local_err);
...or is this leftovers from your debugging?
The rest of the patch looks fine; it is converting a global variable
into a per-instance variable.
--
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] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 06/28] qapi-introspect: Mangle --prefix argument properly for C
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 06/28] qapi-introspect: Mangle --prefix argument properly for C Markus Armbruster
@ 2017-03-03 18:29 ` Eric Blake
2017-03-03 19:41 ` Markus Armbruster
0 siblings, 1 reply; 53+ messages in thread
From: Eric Blake @ 2017-03-03 18:29 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]
On 03/03/2017 06:32 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> scripts/qapi-introspect.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
I'm guessing we haven't seen a use of a prefix that matters yet, but
that an upcoming patch triggered a compilation failure without this fix.
Mentioning that in the commit message wouldn't hurt.
>
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> index 541644e..fb72c61 100644
> --- a/scripts/qapi-introspect.py
> +++ b/scripts/qapi-introspect.py
> @@ -64,7 +64,7 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
> # generate C
> # TODO can generate awfully long lines
> jsons.extend(self._jsons)
> - name = prefix + 'qmp_schema_json'
> + name = c_name(prefix, protect=False) + 'qmp_schema_json'
> self.decl = mcgen('''
> extern const char %(c_name)s[];
> ''',
>
--
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] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 07/28] qmp: Clean up how we enforce capability negotiation
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 07/28] qmp: Clean up how we enforce capability negotiation Markus Armbruster
@ 2017-03-03 18:40 ` Eric Blake
2017-03-03 19:45 ` Markus Armbruster
0 siblings, 1 reply; 53+ messages in thread
From: Eric Blake @ 2017-03-03 18:40 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2260 bytes --]
On 03/03/2017 06:32 AM, Markus Armbruster wrote:
> To enforce capability negotiation before normal operation,
> handle_qmp_command() inspects every command before it's handed off to
> qmp_dispatch(). This is a bit of a layering violation, and results in
> duplicated code.
>
> Before capability negotiation (!cur_mon->in_command_mode), we fail
> commands other than "qmp_capabilities". This is what enforces
> capability negotiation.
>
> Afterwards, we fail command "qmp_capabilities".
>
> Clean this up as follows.
>
> The obvious place to fail a command is the command itself, so move the
> "afterwards" check to qmp_qmp_capabilities().
>
> We do the "before" check in every other command, but that would be
> bothersome. Instead, start with an alternate list of commant that
s/commant/commands/
> contains only "qmp_capabilities". Switch to the full list in
> qmp_qmp_capabilities().
>
> Additionally, replace the generic human-readable error message for
> CommandNotFound by one that reminds the user to run qmp_capabilities.
> Without that, we'd regress commit 2d5a834.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> monitor.c | 76 +++++++++++++++++++++++++++++++++++----------------------------
> 1 file changed, 42 insertions(+), 34 deletions(-)
>
> @@ -3786,11 +3785,20 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> cmd_name = qdict_get_str(qdict, "execute");
> trace_handle_qmp_command(mon, cmd_name);
>
> - if (invalid_qmp_mode(mon, cmd_name, &err)) {
> - goto err_out;
> - }
> + rsp = qmp_dispatch(cur_mon->qmp.commands, req);
>
> - rsp = qmp_dispatch(&qmp_commands, req);
> + qdict = qdict_get_qdict(qobject_to_qdict(rsp), "error");
> + if (qdict) {
> + if (mon->qmp.commands == &qmp_cap_negotiation_commands
> + && !g_strcmp0(qdict_get_try_str(qdict, "class"),
> + QapiErrorClass_lookup[ERROR_CLASS_COMMAND_NOT_FOUND])) {
Could join these two 'if' into one, for less {}, but that's cosmetic.
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] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 24/28] test-qobject-input-visitor: Cover missing nested struct member
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 24/28] test-qobject-input-visitor: Cover missing nested struct member Markus Armbruster
@ 2017-03-03 18:45 ` Eric Blake
0 siblings, 0 replies; 53+ messages in thread
From: Eric Blake @ 2017-03-03 18:45 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 374 bytes --]
On 03/03/2017 06:32 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> tests/test-qobject-input-visitor.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
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] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails Markus Armbruster
@ 2017-03-03 19:15 ` Eric Blake
2017-03-03 19:50 ` Markus Armbruster
0 siblings, 1 reply; 53+ messages in thread
From: Eric Blake @ 2017-03-03 19:15 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3511 bytes --]
On 03/03/2017 06:32 AM, Markus Armbruster wrote:
> Fix the design flaw demonstrated in the previous commit: new method
> check_list() lets input visitors report that unvisited input remains
> for a list, exactly like check_struct() lets them report that
> unvisited input remains for a struct or union.
>
> Implement the method for the qobject input visitor (straightforward),
> and the string input visitor (less so, due to the magic list syntax
> there). The opts visitor's list magic is even more impenetrable, and
> all I can do there today is a stub with a FIXME comment. No worse
> than before.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
Didn't I already review this one?
Ah, there's my R-b:
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg07614.html
But since it disappeared again, I had another look.
> +++ b/qapi/qobject-input-visitor.c
> @@ -51,7 +51,8 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
> return container_of(v, QObjectInputVisitor, visitor);
> }
>
> -static const char *full_name(QObjectInputVisitor *qiv, const char *name)
> +static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
> + int n)
> {
> StackObject *so;
> char buf[32];
> @@ -63,8 +64,10 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name)
> }
>
> QSLIST_FOREACH(so , &qiv->stack, node) {
> - if (qobject_type(so->obj) == QTYPE_QDICT) {
> - g_string_prepend(qiv->errname, name);
> + if (n) {
> + n--;
> + } else if (qobject_type(so->obj) == QTYPE_QDICT) {
> + g_string_prepend(qiv->errname, name ?: "<anonymous>");
> g_string_prepend_c(qiv->errname, '.');
> } else {
> snprintf(buf, sizeof(buf), "[%u]", so->index);
> @@ -72,18 +75,24 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name)
> }
> name = so->name;
> }
> + assert(!n);
If I'm reading this right, your use of n-- in the loop followed by the
post-condition is to assert that QSLIST_FOREACH() iterated n times, but
lets see what callers pass for n:
>
> if (name) {
> g_string_prepend(qiv->errname, name);
> } else if (qiv->errname->str[0] == '.') {
> g_string_erase(qiv->errname, 0, 1);
> - } else {
> + } else if (!qiv->errname->str[0]) {
> return "<anonymous>";
> }
>
> return qiv->errname->str;
> }
>
> +static const char *full_name(QObjectInputVisitor *qiv, const char *name)
> +{
> + return full_name_nth(qiv, name, 0);
> +}
One caller passes 0,
> +static void qobject_input_check_list(Visitor *v, Error **errp)
> +{
> + QObjectInputVisitor *qiv = to_qiv(v);
> + StackObject *tos = QSLIST_FIRST(&qiv->stack);
> +
> + assert(tos && tos->obj && qobject_type(tos->obj) == QTYPE_QLIST);
> +
> + if (tos->entry) {
> + error_setg(errp, "Only %u list elements expected in %s",
> + tos->index + 1, full_name_nth(qiv, NULL, 1));
the other passes 1. No other calls. Did we really need an integer,
where we use n--, or would a bool have done as well?
At any rate, since I've already reviewed it once, you can add R-b, but
we may want a followup to make it less confusing.
--
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] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 05/28] qapi: Support multiple command registries per program
2017-03-03 18:24 ` Eric Blake
@ 2017-03-03 19:37 ` Markus Armbruster
2017-03-03 19:52 ` Eric Blake
0 siblings, 1 reply; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 19:37 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>> The command registry encapsulates a single command list. Give the
>> functions using it a parameter instead. Define suitable command lists
>> in monitor, guest agent and test-qmp-commands.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> include/qapi/qmp/dispatch.h | 22 ++++++++++++++--------
>> monitor.c | 31 +++++++++++++++++--------------
>> qapi/qmp-dispatch.c | 17 +++++++++++++----
>> qapi/qmp-registry.c | 37 ++++++++++++++++++-------------------
>> qga/commands.c | 2 +-
>> qga/guest-agent-core.h | 2 ++
>> qga/main.c | 19 ++++++++++---------
>> scripts/qapi-commands.py | 16 ++++++++++------
>> tests/test-qmp-commands.c | 12 +++++++-----
>> 9 files changed, 92 insertions(+), 66 deletions(-)
>>
>
>> +++ b/qapi/qmp-dispatch.c
>> @@ -67,7 +67,11 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>> return dict;
>> }
>>
>> -static QObject *do_qmp_dispatch(QObject *request, Error **errp)
>> +volatile QmpCommand *save_cmd;
>
> A comment why volatile is necessary would be nice...
Uh...
>> +QmpCommand cmd2;
>> +
>> +static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
>> + Error **errp)
>> {
>> Error *local_err = NULL;
>> const char *command;
>> @@ -81,7 +85,7 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp)
>> }
>>
>> command = qdict_get_str(dict, "execute");
>> - cmd = qmp_find_command(command);
>> + cmd = qmp_find_command(cmds, command);
>> if (cmd == NULL) {
>> error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
>> "The command %s has not been found", command);
>> @@ -93,6 +97,9 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp)
>> return NULL;
>> }
>>
>> + assert(!cmd->options & QCO_NO_SUCCESS_RESP);
>> + save_cmd = cmd;
>> + cmd2 = *cmd;
>> if (!qdict_haskey(dict, "arguments")) {
>> args = qdict_new();
>> } else {
>> @@ -111,6 +118,8 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp)
>>
>> QDECREF(args);
>>
>> + assert(!cmd->options & QCO_NO_SUCCESS_RESP);
>> + assert(ret || local_err);
>
> ...or is this leftovers from your debugging?
Corret. I'll drop it.
> The rest of the patch looks fine; it is converting a global variable
> into a per-instance variable.
Squashing in the obvious garbage collection. May I add your R-by?
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 95a0f48..72827a3 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -67,9 +67,6 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
return dict;
}
-volatile QmpCommand *save_cmd;
-QmpCommand cmd2;
-
static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
Error **errp)
{
@@ -97,9 +94,6 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
return NULL;
}
- assert(!cmd->options & QCO_NO_SUCCESS_RESP);
- save_cmd = cmd;
- cmd2 = *cmd;
if (!qdict_haskey(dict, "arguments")) {
args = qdict_new();
} else {
@@ -118,8 +112,6 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
QDECREF(args);
- assert(!cmd->options & QCO_NO_SUCCESS_RESP);
- assert(ret || local_err);
return ret;
}
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 06/28] qapi-introspect: Mangle --prefix argument properly for C
2017-03-03 18:29 ` Eric Blake
@ 2017-03-03 19:41 ` Markus Armbruster
0 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 19:41 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> scripts/qapi-introspect.py | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> I'm guessing we haven't seen a use of a prefix that matters yet, but
> that an upcoming patch triggered a compilation failure without this fix.
> Mentioning that in the commit message wouldn't hurt.
Done. Thanks!
[...]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 07/28] qmp: Clean up how we enforce capability negotiation
2017-03-03 18:40 ` Eric Blake
@ 2017-03-03 19:45 ` Markus Armbruster
2017-03-03 19:57 ` Eric Blake
0 siblings, 1 reply; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 19:45 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>> To enforce capability negotiation before normal operation,
>> handle_qmp_command() inspects every command before it's handed off to
>> qmp_dispatch(). This is a bit of a layering violation, and results in
>> duplicated code.
>>
>> Before capability negotiation (!cur_mon->in_command_mode), we fail
>> commands other than "qmp_capabilities". This is what enforces
>> capability negotiation.
>>
>> Afterwards, we fail command "qmp_capabilities".
>>
>> Clean this up as follows.
>>
>> The obvious place to fail a command is the command itself, so move the
>> "afterwards" check to qmp_qmp_capabilities().
>>
>> We do the "before" check in every other command, but that would be
>> bothersome. Instead, start with an alternate list of commant that
>
> s/commant/commands/
Fixed.
>> contains only "qmp_capabilities". Switch to the full list in
>> qmp_qmp_capabilities().
>>
>> Additionally, replace the generic human-readable error message for
>> CommandNotFound by one that reminds the user to run qmp_capabilities.
>> Without that, we'd regress commit 2d5a834.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> monitor.c | 76 +++++++++++++++++++++++++++++++++++----------------------------
>> 1 file changed, 42 insertions(+), 34 deletions(-)
>>
>
>> @@ -3786,11 +3785,20 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>> cmd_name = qdict_get_str(qdict, "execute");
>> trace_handle_qmp_command(mon, cmd_name);
>>
>> - if (invalid_qmp_mode(mon, cmd_name, &err)) {
>> - goto err_out;
>> - }
>> + rsp = qmp_dispatch(cur_mon->qmp.commands, req);
>>
>> - rsp = qmp_dispatch(&qmp_commands, req);
>> + qdict = qdict_get_qdict(qobject_to_qdict(rsp), "error");
>> + if (qdict) {
>> + if (mon->qmp.commands == &qmp_cap_negotiation_commands
>> + && !g_strcmp0(qdict_get_try_str(qdict, "class"),
>> + QapiErrorClass_lookup[ERROR_CLASS_COMMAND_NOT_FOUND])) {
>
> Could join these two 'if' into one, for less {}, but that's cosmetic.
Or maybe get reshuffle so that qdict_get_qdict() is called only when
needed:
if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
qdict = qdict_get_qdict(qobject_to_qdict(rsp), "error");
if (qdict
&& !g_strcmp0(qdict_get_try_str(qdict, "class"),
QapiErrorClass_lookup[ERROR_CLASS_COMMAND_NOT_FOUND])) {
/* Provide a more useful error message */
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails
2017-03-03 19:15 ` Eric Blake
@ 2017-03-03 19:50 ` Markus Armbruster
2017-03-03 20:01 ` Eric Blake
0 siblings, 1 reply; 53+ messages in thread
From: Markus Armbruster @ 2017-03-03 19:50 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>> Fix the design flaw demonstrated in the previous commit: new method
>> check_list() lets input visitors report that unvisited input remains
>> for a list, exactly like check_struct() lets them report that
>> unvisited input remains for a struct or union.
>>
>> Implement the method for the qobject input visitor (straightforward),
>> and the string input visitor (less so, due to the magic list syntax
>> there). The opts visitor's list magic is even more impenetrable, and
>> all I can do there today is a stub with a FIXME comment. No worse
>> than before.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
> Didn't I already review this one?
>
> Ah, there's my R-b:
> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg07614.html
Oops! My apologies...
> But since it disappeared again, I had another look.
>
>> +++ b/qapi/qobject-input-visitor.c
>> @@ -51,7 +51,8 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
>> return container_of(v, QObjectInputVisitor, visitor);
>> }
>>
>> -static const char *full_name(QObjectInputVisitor *qiv, const char *name)
>> +static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
>> + int n)
>> {
>> StackObject *so;
>> char buf[32];
>> @@ -63,8 +64,10 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name)
>> }
>>
>> QSLIST_FOREACH(so , &qiv->stack, node) {
>> - if (qobject_type(so->obj) == QTYPE_QDICT) {
>> - g_string_prepend(qiv->errname, name);
>> + if (n) {
>> + n--;
>> + } else if (qobject_type(so->obj) == QTYPE_QDICT) {
>> + g_string_prepend(qiv->errname, name ?: "<anonymous>");
>> g_string_prepend_c(qiv->errname, '.');
>> } else {
>> snprintf(buf, sizeof(buf), "[%u]", so->index);
>> @@ -72,18 +75,24 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name)
>> }
>> name = so->name;
>> }
>> + assert(!n);
>
> If I'm reading this right, your use of n-- in the loop followed by the
> post-condition is to assert that QSLIST_FOREACH() iterated n times, but
> lets see what callers pass for n:
At least @n times.
>>
>> if (name) {
>> g_string_prepend(qiv->errname, name);
>> } else if (qiv->errname->str[0] == '.') {
>> g_string_erase(qiv->errname, 0, 1);
>> - } else {
>> + } else if (!qiv->errname->str[0]) {
>> return "<anonymous>";
>> }
>>
>> return qiv->errname->str;
>> }
>>
>> +static const char *full_name(QObjectInputVisitor *qiv, const char *name)
>> +{
>> + return full_name_nth(qiv, name, 0);
>> +}
>
> One caller passes 0,
>
>
>> +static void qobject_input_check_list(Visitor *v, Error **errp)
>> +{
>> + QObjectInputVisitor *qiv = to_qiv(v);
>> + StackObject *tos = QSLIST_FIRST(&qiv->stack);
>> +
>> + assert(tos && tos->obj && qobject_type(tos->obj) == QTYPE_QLIST);
>> +
>> + if (tos->entry) {
>> + error_setg(errp, "Only %u list elements expected in %s",
>> + tos->index + 1, full_name_nth(qiv, NULL, 1));
>
> the other passes 1. No other calls. Did we really need an integer,
> where we use n--, or would a bool have done as well?
Since I actually use only 0 and 1, a bool would do, but would it make
the code simpler?
> At any rate, since I've already reviewed it once, you can add R-b, but
> we may want a followup to make it less confusing.
Would renaming the function to full_name_but_n() help?
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 05/28] qapi: Support multiple command registries per program
2017-03-03 19:37 ` Markus Armbruster
@ 2017-03-03 19:52 ` Eric Blake
0 siblings, 0 replies; 53+ messages in thread
From: Eric Blake @ 2017-03-03 19:52 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 811 bytes --]
On 03/03/2017 01:37 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>>> The command registry encapsulates a single command list. Give the
>>> functions using it a parameter instead. Define suitable command lists
>>> in monitor, guest agent and test-qmp-commands.
>>>
>> ...or is this leftovers from your debugging?
>
> Corret. I'll drop it.
>
>> The rest of the patch looks fine; it is converting a global variable
>> into a per-instance variable.
>
> Squashing in the obvious garbage collection. May I add your R-by?
>
Yes, with that squashed in,
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] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 10/28] qmp: Improve QMP dispatch error messages
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 10/28] qmp: Improve QMP dispatch error messages Markus Armbruster
@ 2017-03-03 19:55 ` Philippe Mathieu-Daudé
2017-03-05 8:01 ` Markus Armbruster
0 siblings, 1 reply; 53+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-03-03 19:55 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Hi Markus,
On 03/03/2017 09:32 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> qapi/qmp-dispatch.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 23b0528..578c6d8 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -30,7 +30,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>
> dict = qobject_to_qdict(request);
> if (!dict) {
> - error_setg(errp, "Expected '%s' in QMP input", "object");
> + error_setg(errp, "QMP input must be a JSON object");
> return NULL;
> }
>
> @@ -41,15 +41,17 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>
> if (!strcmp(arg_name, "execute")) {
> if (qobject_type(arg_obj) != QTYPE_QSTRING) {
> - error_setg(errp, "QMP input object member '%s' expects '%s'",
> - "execute", "string");
> + error_setg(errp,
> + "QMP input object member '%s' must be %s",
> + "execute", "a string");
let's avoid formatting like the rest of this patch.
> return NULL;
> }
> has_exec_key = true;
> } else if (!strcmp(arg_name, "arguments")) {
> if (qobject_type(arg_obj) != QTYPE_QDICT) {
> - error_setg(errp, "QMP input object member '%s' expects '%s'",
> - "arguments", "object");
> + error_setg(errp,
> + "QMP input object member '%s' must be %s",
> + "arguments", "an object");
same.
> return NULL;
> }
> } else {
> @@ -60,7 +62,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
> }
>
> if (!has_exec_key) {
> - error_setg(errp, "Expected '%s' in QMP input", "execute");
> + error_setg(errp, "QMP input object lacks key 'execute'");
> return NULL;
> }
>
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 13/28] qapi: Make QObject input visitor set *list reliably
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 13/28] qapi: Make QObject input visitor set *list reliably Markus Armbruster
@ 2017-03-03 19:57 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 53+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-03-03 19:57 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
On 03/03/2017 09:32 AM, Markus Armbruster wrote:
> qobject_input_start_struct() sets *list, except when it fails because
> qobject_input_get_object() fails, i.e. the input object doesn't exist.
>
> All the other input visitor start_struct(), start_list(),
> start_alternate() always set *obj / *list.
>
> Change qobject_input_start_struct() to match.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> qapi/qobject-input-visitor.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 2c2f883..d58696c 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -196,25 +196,21 @@ static void qobject_input_start_list(Visitor *v, const char *name,
> QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
> const QListEntry *entry;
>
> + if (list) {
> + *list = NULL;
> + }
> if (!qobj) {
> return;
> }
> if (qobject_type(qobj) != QTYPE_QLIST) {
> - if (list) {
> - *list = NULL;
> - }
> error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> "list");
> return;
> }
>
> entry = qobject_input_push(qiv, qobj, list);
> - if (list) {
> - if (entry) {
> - *list = g_malloc0(size);
> - } else {
> - *list = NULL;
> - }
> + if (entry && list) {
> + *list = g_malloc0(size);
> }
> }
>
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 07/28] qmp: Clean up how we enforce capability negotiation
2017-03-03 19:45 ` Markus Armbruster
@ 2017-03-03 19:57 ` Eric Blake
0 siblings, 0 replies; 53+ messages in thread
From: Eric Blake @ 2017-03-03 19:57 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1138 bytes --]
On 03/03/2017 01:45 PM, Markus Armbruster wrote:
>>> - rsp = qmp_dispatch(&qmp_commands, req);
>>> + qdict = qdict_get_qdict(qobject_to_qdict(rsp), "error");
>>> + if (qdict) {
>>> + if (mon->qmp.commands == &qmp_cap_negotiation_commands
>>> + && !g_strcmp0(qdict_get_try_str(qdict, "class"),
>>> + QapiErrorClass_lookup[ERROR_CLASS_COMMAND_NOT_FOUND])) {
>>
>> Could join these two 'if' into one, for less {}, but that's cosmetic.
>
> Or maybe get reshuffle so that qdict_get_qdict() is called only when
> needed:
>
> if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
> qdict = qdict_get_qdict(qobject_to_qdict(rsp), "error");
> if (qdict
> && !g_strcmp0(qdict_get_try_str(qdict, "class"),
> QapiErrorClass_lookup[ERROR_CLASS_COMMAND_NOT_FOUND])) {
> /* Provide a more useful error message */
Yes, that's even nicer (it's probably in the noise, but
micro-optimizations are fun!)
--
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] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails
2017-03-03 19:50 ` Markus Armbruster
@ 2017-03-03 20:01 ` Eric Blake
2017-03-05 8:06 ` Markus Armbruster
0 siblings, 1 reply; 53+ messages in thread
From: Eric Blake @ 2017-03-03 20:01 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2548 bytes --]
On 03/03/2017 01:50 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>>> Fix the design flaw demonstrated in the previous commit: new method
>>> check_list() lets input visitors report that unvisited input remains
>>> for a list, exactly like check_struct() lets them report that
>>> unvisited input remains for a struct or union.
>>>
>>> Implement the method for the qobject input visitor (straightforward),
>>> and the string input visitor (less so, due to the magic list syntax
>>> there). The opts visitor's list magic is even more impenetrable, and
>>> all I can do there today is a stub with a FIXME comment. No worse
>>> than before.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>
>> Didn't I already review this one?
>>
>> Ah, there's my R-b:
>> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg07614.html
>
>>>
>>> --- a/qapi/qobject-input-visitor.c
>>> +++ b/qapi/qobject-input-visitor.c
>>> @@ -51,7 +51,8 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
>>> return container_of(v, QObjectInputVisitor, visitor);
>>> }
>>>
>>> -static const char *full_name(QObjectInputVisitor *qiv, const char *name)
>>> +static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
>>> + int n)
>>> {
No function comment, so the _nth and int n are guesses on their meaning...
>> If I'm reading this right, your use of n-- in the loop followed by the
>> post-condition is to assert that QSLIST_FOREACH() iterated n times, but
>> lets see what callers pass for n:
>
> At least @n times.
Ah, as in 'use first available result' or 'iterate at least once', based
on our callers, but could also mean 'iterate at least twice' for a
caller that passes 2.
>> the other passes 1. No other calls. Did we really need an integer,
>> where we use n--, or would a bool have done as well?
>
> Since I actually use only 0 and 1, a bool would do, but would it make
> the code simpler?
I don't know that a bool would be any simpler,
>
>> At any rate, since I've already reviewed it once, you can add R-b, but
>> we may want a followup to make it less confusing.
>
> Would renaming the function to full_name_but_n() help?
Or even keep the name unchanged, but add function comments describing
what 'n' means.
--
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] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 10/28] qmp: Improve QMP dispatch error messages
2017-03-03 19:55 ` Philippe Mathieu-Daudé
@ 2017-03-05 8:01 ` Markus Armbruster
2017-03-06 16:10 ` Eric Blake
2017-03-07 14:21 ` Eric Blake
0 siblings, 2 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-05 8:01 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> Hi Markus,
>
> On 03/03/2017 09:32 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>> qapi/qmp-dispatch.c | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
>> index 23b0528..578c6d8 100644
>> --- a/qapi/qmp-dispatch.c
>> +++ b/qapi/qmp-dispatch.c
>> @@ -30,7 +30,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>>
>> dict = qobject_to_qdict(request);
>> if (!dict) {
>> - error_setg(errp, "Expected '%s' in QMP input", "object");
>> + error_setg(errp, "QMP input must be a JSON object");
>> return NULL;
>> }
>>
>> @@ -41,15 +41,17 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>>
>> if (!strcmp(arg_name, "execute")) {
>> if (qobject_type(arg_obj) != QTYPE_QSTRING) {
>> - error_setg(errp, "QMP input object member '%s' expects '%s'",
>> - "execute", "string");
>> + error_setg(errp,
>> + "QMP input object member '%s' must be %s",
>> + "execute", "a string");
>
> let's avoid formatting like the rest of this patch.
>
>> return NULL;
>> }
>> has_exec_key = true;
>> } else if (!strcmp(arg_name, "arguments")) {
>> if (qobject_type(arg_obj) != QTYPE_QDICT) {
>> - error_setg(errp, "QMP input object member '%s' expects '%s'",
>> - "arguments", "object");
>> + error_setg(errp,
>> + "QMP input object member '%s' must be %s",
>> + "arguments", "an object");
>
> same.
>
>> return NULL;
>> }
>> } else {
>> @@ -60,7 +62,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>> }
>>
>> if (!has_exec_key) {
>> - error_setg(errp, "Expected '%s' in QMP input", "execute");
>> + error_setg(errp, "QMP input object lacks key 'execute'");
>> return NULL;
>> }
>>
>>
Since substantial other work depends on this series, it needs to go in
sooner rather than later. I'm therefore *dropping* this patch from the
series. We can then bikeshed^Wpolish to our heart's content without
holding up other work.
That said: what about this?
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index dc50212..5ad36f8 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -30,7 +30,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
dict = qobject_to_qdict(request);
if (!dict) {
- error_setg(errp, "Expected '%s' in QMP input", "object");
+ error_setg(errp, "QMP input must be a JSON object");
return NULL;
}
@@ -41,26 +41,26 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
if (!strcmp(arg_name, "execute")) {
if (qobject_type(arg_obj) != QTYPE_QSTRING) {
- error_setg(errp, "QMP input object member '%s' expects '%s'",
- "execute", "string");
+ error_setg(errp,
+ "QMP input member 'execute' must be a string");
return NULL;
}
has_exec_key = true;
} else if (!strcmp(arg_name, "arguments")) {
if (qobject_type(arg_obj) != QTYPE_QDICT) {
- error_setg(errp, "QMP input object member '%s' expects '%s'",
- "arguments", "object");
+ error_setg(errp,
+ "QMP input member 'arguments' must be an object");
return NULL;
}
} else {
- error_setg(errp, "QMP input object member '%s' is unexpected",
+ error_setg(errp, "QMP input member '%s' is unexpected",
arg_name);
return NULL;
}
}
if (!has_exec_key) {
- error_setg(errp, "Expected '%s' in QMP input", "execute");
+ error_setg(errp, "QMP input lacks member 'execute'");
return NULL;
}
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails
2017-03-03 20:01 ` Eric Blake
@ 2017-03-05 8:06 ` Markus Armbruster
0 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-05 8:06 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 03/03/2017 01:50 PM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>>>> Fix the design flaw demonstrated in the previous commit: new method
>>>> check_list() lets input visitors report that unvisited input remains
>>>> for a list, exactly like check_struct() lets them report that
>>>> unvisited input remains for a struct or union.
>>>>
>>>> Implement the method for the qobject input visitor (straightforward),
>>>> and the string input visitor (less so, due to the magic list syntax
>>>> there). The opts visitor's list magic is even more impenetrable, and
>>>> all I can do there today is a stub with a FIXME comment. No worse
>>>> than before.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>
>>> Didn't I already review this one?
>>>
>>> Ah, there's my R-b:
>>> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg07614.html
>>
>
>>>>
>>>> --- a/qapi/qobject-input-visitor.c
>>>> +++ b/qapi/qobject-input-visitor.c
>>>> @@ -51,7 +51,8 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
>>>> return container_of(v, QObjectInputVisitor, visitor);
>>>> }
>>>>
>>>> -static const char *full_name(QObjectInputVisitor *qiv, const char *name)
>>>> +static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
>>>> + int n)
>>>> {
>
> No function comment, so the _nth and int n are guesses on their meaning...
>
>
>>> If I'm reading this right, your use of n-- in the loop followed by the
>>> post-condition is to assert that QSLIST_FOREACH() iterated n times, but
>>> lets see what callers pass for n:
>>
>> At least @n times.
>
> Ah, as in 'use first available result' or 'iterate at least once', based
> on our callers, but could also mean 'iterate at least twice' for a
> caller that passes 2.
>
>
>>> the other passes 1. No other calls. Did we really need an integer,
>>> where we use n--, or would a bool have done as well?
>>
>> Since I actually use only 0 and 1, a bool would do, but would it make
>> the code simpler?
>
> I don't know that a bool would be any simpler,
>
>>
>>> At any rate, since I've already reviewed it once, you can add R-b, but
>>> we may want a followup to make it less confusing.
>>
>> Would renaming the function to full_name_but_n() help?
>
> Or even keep the name unchanged, but add function comments describing
> what 'n' means.
Makes sense. I'll do it on top to avoid delaying merge of this series
and the other stuff that depends on it.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 10/28] qmp: Improve QMP dispatch error messages
2017-03-05 8:01 ` Markus Armbruster
@ 2017-03-06 16:10 ` Eric Blake
2017-03-07 7:45 ` Markus Armbruster
2017-03-07 14:21 ` Eric Blake
1 sibling, 1 reply; 53+ messages in thread
From: Eric Blake @ 2017-03-06 16:10 UTC (permalink / raw)
To: Markus Armbruster, Philippe Mathieu-Daudé; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1139 bytes --]
On 03/05/2017 02:01 AM, Markus Armbruster wrote:
>>> @@ -41,15 +41,17 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>>>
>>> if (!strcmp(arg_name, "execute")) {
>>> if (qobject_type(arg_obj) != QTYPE_QSTRING) {
>>> - error_setg(errp, "QMP input object member '%s' expects '%s'",
>>> - "execute", "string");
>>> + error_setg(errp,
>>> + "QMP input object member '%s' must be %s",
>>> + "execute", "a string");
>>
>> let's avoid formatting like the rest of this patch.
>>
> Since substantial other work depends on this series, it needs to go in
> sooner rather than later. I'm therefore *dropping* this patch from the
> series. We can then bikeshed^Wpolish to our heart's content without
> holding up other work.
Agreed.
>
> That said: what about this?
Looks better. Do you want R-b now, or when you re-post it as an
official patch?
--
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] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 16/28] qapi: Make string input and opts visitor require non-null input
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 16/28] qapi: Make string input and opts visitor require non-null input Markus Armbruster
@ 2017-03-06 17:07 ` Philippe Mathieu-Daudé
2017-03-07 7:47 ` Markus Armbruster
0 siblings, 1 reply; 53+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-03-06 17:07 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
On 03/03/2017 09:32 AM, Markus Armbruster wrote:
> The string input visitor tries to cope with null input. Null input
> isn't used anywhere, and isn't covered by tests. Unsurprisingly, it
> doesn't fully work: start_list() crashes because it passes the input
> via parse_str() to strtoll() unchecked.
>
> Make string_input_visitor_new() assert its argument isn't null, and
> drop the code trying to deal with null input.
>
> The opts visitor crashes when you try to actually visit something with
> null input. Make opts_visitor_new() assert its argument isn't null,
> mostly for clarity.
>
> qobject_input_visitor_new() already asserts its argument isn't null.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> qapi/opts-visitor.c | 1 +
> qapi/string-input-visitor.c | 54 ++++++++++++++-------------------------------
> 2 files changed, 18 insertions(+), 37 deletions(-)
>
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index a0a7c0e..c50dc4b 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -528,6 +528,7 @@ opts_visitor_new(const QemuOpts *opts)
> {
> OptsVisitor *ov;
>
> + assert(opts);
> ov = g_malloc0(sizeof *ov);
>
> ov->visitor.type = VISITOR_INPUT;
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index 1a855c5..f126cd9 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -182,12 +182,6 @@ static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
> {
> StringInputVisitor *siv = to_siv(v);
>
> - if (!siv->string) {
> - error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> - "integer");
> - return;
> - }
> -
> if (parse_str(siv, name, errp) < 0) {
> return;
> }
> @@ -242,13 +236,7 @@ static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
> Error *err = NULL;
> uint64_t val;
>
> - if (siv->string) {
> - parse_option_size(name, siv->string, &val, &err);
> - } else {
> - error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> - "size");
> - return;
> - }
> + parse_option_size(name, siv->string, &val, &err);
> if (err) {
> error_propagate(errp, err);
> return;
> @@ -262,19 +250,17 @@ static void parse_type_bool(Visitor *v, const char *name, bool *obj,
> {
> StringInputVisitor *siv = to_siv(v);
>
> - if (siv->string) {
> - if (!strcasecmp(siv->string, "on") ||
> - !strcasecmp(siv->string, "yes") ||
> - !strcasecmp(siv->string, "true")) {
> - *obj = true;
> - return;
> - }
> - if (!strcasecmp(siv->string, "off") ||
> - !strcasecmp(siv->string, "no") ||
> - !strcasecmp(siv->string, "false")) {
> - *obj = false;
> - return;
> - }
> + if (!strcasecmp(siv->string, "on") ||
> + !strcasecmp(siv->string, "yes") ||
> + !strcasecmp(siv->string, "true")) {
> + *obj = true;
> + return;
> + }
> + if (!strcasecmp(siv->string, "off") ||
> + !strcasecmp(siv->string, "no") ||
> + !strcasecmp(siv->string, "false")) {
> + *obj = false;
> + return;
> }
>
> error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> @@ -285,13 +271,8 @@ static void parse_type_str(Visitor *v, const char *name, char **obj,
> Error **errp)
> {
> StringInputVisitor *siv = to_siv(v);
> - if (siv->string) {
> - *obj = g_strdup(siv->string);
> - } else {
> - *obj = NULL;
> - error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> - "string");
> - }
> +
> + *obj = g_strdup(siv->string);
> }
>
> static void parse_type_number(Visitor *v, const char *name, double *obj,
> @@ -302,10 +283,8 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
> double val;
>
> errno = 0;
> - if (siv->string) {
> - val = strtod(siv->string, &endp);
> - }
> - if (!siv->string || errno || endp == siv->string || *endp) {
> + val = strtod(siv->string, &endp);
> + if (errno || endp == siv->string || *endp) {
> error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> "number");
> return;
> @@ -327,6 +306,7 @@ Visitor *string_input_visitor_new(const char *str)
> {
> StringInputVisitor *v;
>
> + assert(str);
> v = g_malloc0(sizeof(*v));
>
> v->visitor.type = VISITOR_INPUT;
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 10/28] qmp: Improve QMP dispatch error messages
2017-03-06 16:10 ` Eric Blake
@ 2017-03-07 7:45 ` Markus Armbruster
0 siblings, 0 replies; 53+ messages in thread
From: Markus Armbruster @ 2017-03-07 7:45 UTC (permalink / raw)
To: Eric Blake; +Cc: Philippe Mathieu-Daudé, qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 03/05/2017 02:01 AM, Markus Armbruster wrote:
>
>>>> @@ -41,15 +41,17 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>>>>
>>>> if (!strcmp(arg_name, "execute")) {
>>>> if (qobject_type(arg_obj) != QTYPE_QSTRING) {
>>>> - error_setg(errp, "QMP input object member '%s' expects '%s'",
>>>> - "execute", "string");
>>>> + error_setg(errp,
>>>> + "QMP input object member '%s' must be %s",
>>>> + "execute", "a string");
>>>
>>> let's avoid formatting like the rest of this patch.
>>>
>
>> Since substantial other work depends on this series, it needs to go in
>> sooner rather than later. I'm therefore *dropping* this patch from the
>> series. We can then bikeshed^Wpolish to our heart's content without
>> holding up other work.
>
> Agreed.
>
>>
>> That said: what about this?
>
> Looks better. Do you want R-b now, or when you re-post it as an
> official patch?
I'll gladly take it either way :)
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 16/28] qapi: Make string input and opts visitor require non-null input
2017-03-06 17:07 ` Philippe Mathieu-Daudé
@ 2017-03-07 7:47 ` Markus Armbruster
2017-03-07 12:17 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 53+ messages in thread
From: Markus Armbruster @ 2017-03-07 7:47 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> On 03/03/2017 09:32 AM, Markus Armbruster wrote:
>> The string input visitor tries to cope with null input. Null input
>> isn't used anywhere, and isn't covered by tests. Unsurprisingly, it
>> doesn't fully work: start_list() crashes because it passes the input
>> via parse_str() to strtoll() unchecked.
>>
>> Make string_input_visitor_new() assert its argument isn't null, and
>> drop the code trying to deal with null input.
>>
>> The opts visitor crashes when you try to actually visit something with
>> null input. Make opts_visitor_new() assert its argument isn't null,
>> mostly for clarity.
>>
>> qobject_input_visitor_new() already asserts its argument isn't null.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Since the series has been merged already, there's no way for me to
record your review with the commit. Thanks anyway!
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 16/28] qapi: Make string input and opts visitor require non-null input
2017-03-07 7:47 ` Markus Armbruster
@ 2017-03-07 12:17 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 53+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-03-07 12:17 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
On 03/07/2017 04:47 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>
>> On 03/03/2017 09:32 AM, Markus Armbruster wrote:
>>> The string input visitor tries to cope with null input. Null input
>>> isn't used anywhere, and isn't covered by tests. Unsurprisingly, it
>>> doesn't fully work: start_list() crashes because it passes the input
>>> via parse_str() to strtoll() unchecked.
>>>
>>> Make string_input_visitor_new() assert its argument isn't null, and
>>> drop the code trying to deal with null input.
>>>
>>> The opts visitor crashes when you try to actually visit something with
>>> null input. Make opts_visitor_new() assert its argument isn't null,
>>> mostly for clarity.
>>>
>>> qobject_input_visitor_new() already asserts its argument isn't null.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Since the series has been merged already, there's no way for me to
> record your review with the commit. Thanks anyway!
>
ups didn't notice the PULL, no worries ;)
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 10/28] qmp: Improve QMP dispatch error messages
2017-03-05 8:01 ` Markus Armbruster
2017-03-06 16:10 ` Eric Blake
@ 2017-03-07 14:21 ` Eric Blake
2017-03-07 14:26 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 53+ messages in thread
From: Eric Blake @ 2017-03-07 14:21 UTC (permalink / raw)
To: Markus Armbruster, Philippe Mathieu-Daudé; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2523 bytes --]
On 03/05/2017 02:01 AM, Markus Armbruster wrote:
> Since substantial other work depends on this series, it needs to go in
> sooner rather than later. I'm therefore *dropping* this patch from the
> series. We can then bikeshed^Wpolish to our heart's content without
> holding up other work.
>
> That said: what about this?
>
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index dc50212..5ad36f8 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -30,7 +30,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>
> dict = qobject_to_qdict(request);
> if (!dict) {
> - error_setg(errp, "Expected '%s' in QMP input", "object");
> + error_setg(errp, "QMP input must be a JSON object");
> return NULL;
> }
>
> @@ -41,26 +41,26 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>
> if (!strcmp(arg_name, "execute")) {
> if (qobject_type(arg_obj) != QTYPE_QSTRING) {
> - error_setg(errp, "QMP input object member '%s' expects '%s'",
> - "execute", "string");
> + error_setg(errp,
> + "QMP input member 'execute' must be a string");
> return NULL;
> }
> has_exec_key = true;
> } else if (!strcmp(arg_name, "arguments")) {
> if (qobject_type(arg_obj) != QTYPE_QDICT) {
> - error_setg(errp, "QMP input object member '%s' expects '%s'",
> - "arguments", "object");
> + error_setg(errp,
> + "QMP input member 'arguments' must be an object");
> return NULL;
> }
> } else {
> - error_setg(errp, "QMP input object member '%s' is unexpected",
> + error_setg(errp, "QMP input member '%s' is unexpected",
> arg_name);
> return NULL;
> }
> }
>
> if (!has_exec_key) {
> - error_setg(errp, "Expected '%s' in QMP input", "execute");
> + error_setg(errp, "QMP input lacks member 'execute'");
> return NULL;
> }
>
>
>
Based on the rest of this sub-thread, I'm making it explicit that you
can add:
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] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v4 10/28] qmp: Improve QMP dispatch error messages
2017-03-07 14:21 ` Eric Blake
@ 2017-03-07 14:26 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 53+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-03-07 14:26 UTC (permalink / raw)
To: Eric Blake, Markus Armbruster; +Cc: qemu-devel
On 03/07/2017 11:21 AM, Eric Blake wrote:
> On 03/05/2017 02:01 AM, Markus Armbruster wrote:
>
>> Since substantial other work depends on this series, it needs to go in
>> sooner rather than later. I'm therefore *dropping* this patch from the
>> series. We can then bikeshed^Wpolish to our heart's content without
>> holding up other work.
>>
>> That said: what about this?
>>
>> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
>> index dc50212..5ad36f8 100644
>> --- a/qapi/qmp-dispatch.c
>> +++ b/qapi/qmp-dispatch.c
>> @@ -30,7 +30,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>>
>> dict = qobject_to_qdict(request);
>> if (!dict) {
>> - error_setg(errp, "Expected '%s' in QMP input", "object");
>> + error_setg(errp, "QMP input must be a JSON object");
>> return NULL;
>> }
>>
>> @@ -41,26 +41,26 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>>
>> if (!strcmp(arg_name, "execute")) {
>> if (qobject_type(arg_obj) != QTYPE_QSTRING) {
>> - error_setg(errp, "QMP input object member '%s' expects '%s'",
>> - "execute", "string");
>> + error_setg(errp,
>> + "QMP input member 'execute' must be a string");
>> return NULL;
>> }
>> has_exec_key = true;
>> } else if (!strcmp(arg_name, "arguments")) {
>> if (qobject_type(arg_obj) != QTYPE_QDICT) {
>> - error_setg(errp, "QMP input object member '%s' expects '%s'",
>> - "arguments", "object");
>> + error_setg(errp,
>> + "QMP input member 'arguments' must be an object");
>> return NULL;
>> }
>> } else {
>> - error_setg(errp, "QMP input object member '%s' is unexpected",
>> + error_setg(errp, "QMP input member '%s' is unexpected",
>> arg_name);
>> return NULL;
>> }
>> }
>>
>> if (!has_exec_key) {
>> - error_setg(errp, "Expected '%s' in QMP input", "execute");
>> + error_setg(errp, "QMP input lacks member 'execute'");
>> return NULL;
>> }
>>
>>
>>
>
> Based on the rest of this sub-thread, I'm making it explicit that you
> can add:
> Reviewed-by: Eric Blake <eblake@redhat.com>
this is not a blocking issue, this code is OK, no need to drop!
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 53+ messages in thread
end of thread, other threads:[~2017-03-07 14:26 UTC | newest]
Thread overview: 53+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-03 12:32 [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 01/28] qga: Fix crash on non-dictionary QMP argument Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 02/28] libqtest: Work around a "QMP wants a newline" bug Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 03/28] qmp-test: New, covering basic QMP protocol Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 04/28] qmp: Dumb down how we run QMP command registration Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 05/28] qapi: Support multiple command registries per program Markus Armbruster
2017-03-03 18:24 ` Eric Blake
2017-03-03 19:37 ` Markus Armbruster
2017-03-03 19:52 ` Eric Blake
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 06/28] qapi-introspect: Mangle --prefix argument properly for C Markus Armbruster
2017-03-03 18:29 ` Eric Blake
2017-03-03 19:41 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 07/28] qmp: Clean up how we enforce capability negotiation Markus Armbruster
2017-03-03 18:40 ` Eric Blake
2017-03-03 19:45 ` Markus Armbruster
2017-03-03 19:57 ` Eric Blake
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 08/28] qmp: Drop duplicated QMP command object checks Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 09/28] qmp: Eliminate silly QERR_QMP_* macros Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 10/28] qmp: Improve QMP dispatch error messages Markus Armbruster
2017-03-03 19:55 ` Philippe Mathieu-Daudé
2017-03-05 8:01 ` Markus Armbruster
2017-03-06 16:10 ` Eric Blake
2017-03-07 7:45 ` Markus Armbruster
2017-03-07 14:21 ` Eric Blake
2017-03-07 14:26 ` Philippe Mathieu-Daudé
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 11/28] qapi: Improve a QObject input visitor error message Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 12/28] qapi: Clean up after commit 3d344c2 Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 13/28] qapi: Make QObject input visitor set *list reliably Markus Armbruster
2017-03-03 19:57 ` Philippe Mathieu-Daudé
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 14/28] qapi: Improve qobject input visitor error reporting Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 15/28] qapi: Drop string input visitor method optional() Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 16/28] qapi: Make string input and opts visitor require non-null input Markus Armbruster
2017-03-06 17:07 ` Philippe Mathieu-Daudé
2017-03-07 7:47 ` Markus Armbruster
2017-03-07 12:17 ` Philippe Mathieu-Daudé
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 17/28] qom: Make object_property_set_qobject()'s input visitor strict Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 18/28] test-qobject-input-visitor: Use strict visitor Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 19/28] qapi: Drop unused non-strict qobject input visitor Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 20/28] tests-qobject-input-strict: Merge into test-qobject-input-visitor Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 21/28] test-string-input-visitor: Tear down existing test automatically Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 22/28] test-string-input-visitor: Improve list coverage Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 23/28] tests: Cover partial input visit of list Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 24/28] test-qobject-input-visitor: Cover missing nested struct member Markus Armbruster
2017-03-03 18:45 ` Eric Blake
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails Markus Armbruster
2017-03-03 19:15 ` Eric Blake
2017-03-03 19:50 ` Markus Armbruster
2017-03-03 20:01 ` Eric Blake
2017-03-05 8:06 ` Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 26/28] tests: Cover input visit beyond end of list Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 27/28] qapi: Fix object " Markus Armbruster
2017-03-03 12:32 ` [Qemu-devel] [PATCH v4 28/28] qapi: Improve qobject visitor documentation Markus Armbruster
2017-03-03 12:58 ` [Qemu-devel] [PATCH v4 00/28] qapi: QMP dispatch and input visitor work no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.