From 0fa39d0b0374b983535de8591e5e561401d1d5c6 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:35 +0200 Subject: qmp qemu-ga: Revert change that accidentally made qemu-ga accept "id" Commit cf869d53172 "qmp: support out-of-band (oob) execution" changed how we check "id": Note that in the patch I exported qmp_dispatch_check_obj() to be used to check the request earlier, and at the same time allowed "id" field to be there since actually we always allow that. The part after "and" is ill-advised: it makes qemu-ga accept and ignore "id". Revert. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-10-armbru@redhat.com> --- qapi/qmp-dispatch.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'qapi/qmp-dispatch.c') diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 935f9e159c..3d5d5e110f 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -52,8 +52,6 @@ QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp) "QMP input member 'arguments' must be an object"); return NULL; } - } else if (!strcmp(arg_name, "id")) { - continue; } else if (!strcmp(arg_name, "control")) { if (qobject_type(arg_obj) != QTYPE_QDICT) { error_setg(errp, -- cgit 1.4.1 From 674ed7228f03150d15703961ea2a59cd744f3beb Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:37 +0200 Subject: qmp qemu-ga: Fix qemu-ga not to accept "control" Commit cf869d53172 "qmp: support out-of-band (oob) execution" accidentally made qemu-ga accept and ignore "control". Fix that. Out-of-band execution in a monitor that doesn't support it now fails with {"error": {"class": "GenericError", "desc": "QMP input member 'control' is unexpected"}} instead of {"error": {"class": "GenericError", "desc": "Please enable out-of-band first for the session during capabilities negotiation"}} The old description is suboptimal when out-of-band cannot not be enabled, or the command doesn't support out-of-band execution. The new description is a bit unspecific, but it'll do. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-12-armbru@redhat.com> --- include/qapi/qmp/dispatch.h | 6 ++++-- monitor.c | 9 ++------- qapi/qmp-dispatch.c | 14 ++++++++------ qga/main.c | 2 +- tests/test-qga.c | 9 +++++---- tests/test-qmp-cmds.c | 8 ++++---- 6 files changed, 24 insertions(+), 24 deletions(-) (limited to 'qapi/qmp-dispatch.c') diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index b366bb48bd..303a15ba84 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -41,7 +41,6 @@ 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); @@ -49,7 +48,10 @@ 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); -QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp); +QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob, + Error **errp); +QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request, + bool allow_oob); bool qmp_is_oob(QDict *dict); typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque); diff --git a/monitor.c b/monitor.c index 7245ee37ce..2e443bba13 100644 --- a/monitor.c +++ b/monitor.c @@ -1319,11 +1319,6 @@ static bool qmp_cmd_oob_check(Monitor *mon, QDict *req, Error **errp) } if (qmp_is_oob(req)) { - if (!qmp_oob_enabled(mon)) { - error_setg(errp, "Please enable out-of-band first " - "for the session during capabilities negotiation"); - return false; - } if (!(cmd->options & QCO_ALLOW_OOB)) { error_setg(errp, "The command %s does not support OOB", command); @@ -4195,7 +4190,7 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj) old_mon = cur_mon; cur_mon = mon; - rsp = qmp_dispatch(mon->qmp.commands, req); + rsp = qmp_dispatch(mon->qmp.commands, req, qmp_oob_enabled(mon)); cur_mon = old_mon; @@ -4286,7 +4281,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) } /* else will fail qmp_dispatch() */ /* Check against the request in general layout */ - qdict = qmp_dispatch_check_obj(req, &err); + qdict = qmp_dispatch_check_obj(req, qmp_oob_enabled(mon), &err); if (!qdict) { goto err; } diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 3d5d5e110f..0ad0fab8ed 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -20,7 +20,8 @@ #include "qapi/qmp/qbool.h" #include "sysemu/sysemu.h" -QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp) +QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob, + Error **errp) { const QDictEntry *ent; const char *arg_name; @@ -52,7 +53,7 @@ QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp) "QMP input member 'arguments' must be an object"); return NULL; } - } else if (!strcmp(arg_name, "control")) { + } else if (!strcmp(arg_name, "control") && allow_oob) { if (qobject_type(arg_obj) != QTYPE_QDICT) { error_setg(errp, "QMP input member 'control' must be a dict"); @@ -74,7 +75,7 @@ QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp) } static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request, - Error **errp) + bool allow_oob, Error **errp) { Error *local_err = NULL; const char *command; @@ -82,7 +83,7 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request, QmpCommand *cmd; QObject *ret = NULL; - dict = qmp_dispatch_check_obj(request, errp); + dict = qmp_dispatch_check_obj(request, allow_oob, errp); if (!dict) { return NULL; } @@ -157,13 +158,14 @@ bool qmp_is_oob(QDict *dict) return qbool_get_bool(bool_obj); } -QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request) +QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request, + bool allow_oob) { Error *err = NULL; QObject *ret; QDict *rsp; - ret = do_qmp_dispatch(cmds, request, &err); + ret = do_qmp_dispatch(cmds, request, allow_oob, &err); rsp = qdict_new(); if (err) { diff --git a/qga/main.c b/qga/main.c index ea7540edcc..d332bacce5 100644 --- a/qga/main.c +++ b/qga/main.c @@ -586,7 +586,7 @@ static void process_command(GAState *s, QDict *req) g_assert(req); g_debug("processing command"); - rsp = qmp_dispatch(&ga_commands, QOBJECT(req)); + rsp = qmp_dispatch(&ga_commands, QOBJECT(req), false); if (rsp) { ret = send_response(s, rsp); if (ret < 0) { diff --git a/tests/test-qga.c b/tests/test-qga.c index 2e9e0f73bb..febabc7ad5 100644 --- a/tests/test-qga.c +++ b/tests/test-qga.c @@ -245,16 +245,17 @@ static void test_qga_invalid_id(gconstpointer fix) static void test_qga_invalid_oob(gconstpointer fix) { - /* FIXME "control" is ignored; it should be rejected */ const TestFixture *fixture = fix; - QDict *ret; + QDict *ret, *error; + const char *class; ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping'," " 'control': {'run-oob': true}}"); g_assert_nonnull(ret); - qmp_assert_no_error(ret); - qdict_get_qdict(ret, "return"); + error = qdict_get_qdict(ret, "error"); + class = qdict_get_try_str(error, "class"); + g_assert_cmpstr(class, ==, "GenericError"); qobject_unref(ret); } diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c index 491b0c4a44..10c7ba40c1 100644 --- a/tests/test-qmp-cmds.c +++ b/tests/test-qmp-cmds.c @@ -102,7 +102,7 @@ static void test_dispatch_cmd(void) qdict_put_str(req, "execute", "user_def_cmd"); - resp = qmp_dispatch(&qmp_commands, QOBJECT(req)); + resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false); assert(resp != NULL); assert(!qdict_haskey(qobject_to(QDict, resp), "error")); @@ -119,7 +119,7 @@ static void test_dispatch_cmd_failure(void) qdict_put_str(req, "execute", "user_def_cmd2"); - resp = qmp_dispatch(&qmp_commands, QOBJECT(req)); + resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false); assert(resp != NULL); assert(qdict_haskey(qobject_to(QDict, resp), "error")); @@ -133,7 +133,7 @@ static void test_dispatch_cmd_failure(void) qdict_put_str(req, "execute", "user_def_cmd"); - resp = qmp_dispatch(&qmp_commands, QOBJECT(req)); + resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false); assert(resp != NULL); assert(qdict_haskey(qobject_to(QDict, resp), "error")); @@ -147,7 +147,7 @@ static QObject *test_qmp_dispatch(QDict *req) QDict *resp; QObject *ret; - resp_obj = qmp_dispatch(&qmp_commands, QOBJECT(req)); + resp_obj = qmp_dispatch(&qmp_commands, QOBJECT(req), false); assert(resp_obj); resp = qobject_to(QDict, resp_obj); assert(resp && !qdict_haskey(resp, "error")); -- cgit 1.4.1 From 00ecec151d2323e742af94cccf2de77025f3c0c1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:38 +0200 Subject: qmp: Redo how the client requests out-of-band execution Commit cf869d53172 "qmp: support out-of-band (oob) execution" added a general mechanism for command-independent arguments just for an out-of-band flag: The "control" key is introduced to store this extra flag. "control" field is used to store arguments that are shared by all the commands, rather than command specific arguments. Let "run-oob" be the first. However, it failed to reject unknown members of "control". For instance, in QMP command {"execute": "query-name", "id": 42, "control": {"crap": true}} "crap" gets silently ignored. Instead of fixing this, revert the general "control" mechanism (because YAGNI), and do it the way I initially proposed, with key "exec-oob". Simpler code, simpler interface. An out-of-band command {"execute": "migrate-pause", "id": 42, "control": {"run-oob": true}} becomes {"exec-oob": "migrate-pause", "id": 42} Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-13-armbru@redhat.com> [Commit message typo fixed] --- docs/devel/qapi-code-gen.txt | 10 ++++----- docs/interop/qmp-spec.txt | 18 +++++++--------- monitor.c | 3 +++ qapi/qmp-dispatch.c | 51 ++++++++++++++++++-------------------------- tests/qmp-test.c | 7 ++---- tests/test-qga.c | 3 +-- 6 files changed, 39 insertions(+), 53 deletions(-) (limited to 'qapi/qmp-dispatch.c') diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt index 9625798d16..f020f6bab2 100644 --- a/docs/devel/qapi-code-gen.txt +++ b/docs/devel/qapi-code-gen.txt @@ -649,13 +649,11 @@ example: { 'command': 'migrate_recover', 'data': { 'uri': 'str' }, 'allow-oob': true } -To execute a command with out-of-band priority, the client specifies -the "control" field in the request, with "run-oob" set to -true. Example: +To execute a command with out-of-band priority, the client uses key +"exec-oob" instead of "execute". Example: - => { "execute": "command-support-oob", - "arguments": { ... }, - "control": { "run-oob": true } } + => { "exec-oob": "migrate-recover", + "arguments": { "uri": "tcp:192.168.1.200:12345" } } <= { "return": { } } Without it, even the commands that support out-of-band execution will diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt index a1d6f9ee06..1566b8ae5e 100644 --- a/docs/interop/qmp-spec.txt +++ b/docs/interop/qmp-spec.txt @@ -92,12 +92,16 @@ Currently supported capabilities are: The format for command execution is: -{ "execute": json-string, "arguments": json-object, "id": json-value, - "control": json-object } +{ "execute": json-string, "arguments": json-object, "id": json-value } + +or + +{ "exec-oob": json-string, "arguments": json-object, "id": json-value } Where, -- The "execute" member identifies the command to be executed by the Server +- The "execute" or "exec-oob" member identifies the command to be + executed by the server. The latter requests out-of-band execution. - The "arguments" member is used to pass any arguments required for the execution of the command, it is optional when no arguments are required. Each command documents what contents will be considered @@ -106,9 +110,6 @@ The format for command execution is: command execution, it is optional and will be part of the response if provided. The "id" member can be any json-value. A json-number incremented for each successive command works fine. -- The optional "control" member further specifies how the command is - to be executed. Currently, its only member is optional "run-oob". - See section "2.3.1 Out-of-band execution" for details. 2.3.1 Out-of-band execution --------------------------- @@ -129,9 +130,6 @@ To be able to match responses back to their commands, the client needs to pass "id" with out-of-band commands. Passing it with all commands is recommended for clients that accept capability "oob". -To execute a command out-of-band, the client puts "run-oob": true into -execute's member "control". - If the client sends in-band commands faster than the server can execute them, the server will eventually drop commands to limit the queue length. The sever sends event COMMAND_DROPPED then. @@ -274,7 +272,7 @@ S: { "timestamp": { "seconds": 1258551470, "microseconds": 802384 }, 3.7 Out-of-band execution ------------------------- -C: { "execute": "migrate-pause", "id": 42, "control": { "run-oob": true } } +C: { "exec-oob": "migrate-pause", "id": 42 } S: { "id": 42, "error": { "class": "GenericError", "desc": "migrate-pause is currently only supported during postcopy-active state" } } diff --git a/monitor.c b/monitor.c index 2e443bba13..3bf3e68bdc 100644 --- a/monitor.c +++ b/monitor.c @@ -1300,6 +1300,9 @@ static bool qmp_cmd_oob_check(Monitor *mon, QDict *req, Error **errp) QmpCommand *cmd; command = qdict_get_try_str(req, "execute"); + if (!command) { + command = qdict_get_try_str(req, "exec-oob"); + } if (!command) { error_setg(errp, "Command field 'execute' missing"); return false; diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 0ad0fab8ed..12be120fe7 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -23,11 +23,11 @@ QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob, Error **errp) { + const char *exec_key = NULL; const QDictEntry *ent; const char *arg_name; const QObject *arg_obj; - bool has_exec_key = false; - QDict *dict = NULL; + QDict *dict; dict = qobject_to(QDict, request); if (!dict) { @@ -40,23 +40,23 @@ QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob, arg_name = qdict_entry_key(ent); arg_obj = qdict_entry_value(ent); - if (!strcmp(arg_name, "execute")) { + if (!strcmp(arg_name, "execute") + || (!strcmp(arg_name, "exec-oob") && allow_oob)) { if (qobject_type(arg_obj) != QTYPE_QSTRING) { - error_setg(errp, - "QMP input member 'execute' must be a string"); + error_setg(errp, "QMP input member '%s' must be a string", + arg_name); return NULL; } - has_exec_key = true; - } else if (!strcmp(arg_name, "arguments")) { - if (qobject_type(arg_obj) != QTYPE_QDICT) { - error_setg(errp, - "QMP input member 'arguments' must be an object"); + if (exec_key) { + error_setg(errp, "QMP input member '%s' clashes with '%s'", + arg_name, exec_key); return NULL; } - } else if (!strcmp(arg_name, "control") && allow_oob) { + exec_key = arg_name; + } else if (!strcmp(arg_name, "arguments")) { if (qobject_type(arg_obj) != QTYPE_QDICT) { error_setg(errp, - "QMP input member 'control' must be a dict"); + "QMP input member 'arguments' must be an object"); return NULL; } } else { @@ -66,7 +66,7 @@ QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob, } } - if (!has_exec_key) { + if (!exec_key) { error_setg(errp, "QMP input lacks member 'execute'"); return NULL; } @@ -88,7 +88,11 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request, return NULL; } - command = qdict_get_str(dict, "execute"); + command = qdict_get_try_str(dict, "execute"); + if (!command) { + assert(allow_oob); + command = qdict_get_str(dict, "exec-oob"); + } cmd = qmp_find_command(cmds, command); if (cmd == NULL) { error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND, @@ -137,25 +141,12 @@ QObject *qmp_build_error_object(Error *err) } /* - * Detect whether a request should be run out-of-band, by quickly - * peeking at whether we have: { "control": { "run-oob": true } }. By - * default commands are run in-band. + * Does @qdict look like a command to be run out-of-band? */ bool qmp_is_oob(QDict *dict) { - QBool *bool_obj; - - dict = qdict_get_qdict(dict, "control"); - if (!dict) { - return false; - } - - bool_obj = qobject_to(QBool, qdict_get(dict, "run-oob")); - if (!bool_obj) { - return false; - } - - return qbool_get_bool(bool_obj); + return qdict_haskey(dict, "exec-oob") + && !qdict_haskey(dict, "execute"); } QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request, diff --git a/tests/qmp-test.c b/tests/qmp-test.c index c9d01b87ca..a7b6ec98e4 100644 --- a/tests/qmp-test.c +++ b/tests/qmp-test.c @@ -176,8 +176,7 @@ static void unblock_blocked_cmd(void) static void send_oob_cmd_that_fails(QTestState *s, const char *id) { - qtest_async_qmp(s, "{ 'execute': 'migrate-pause', 'id': %s," - " 'control': { 'run-oob': true } }", id); + qtest_async_qmp(s, "{ 'exec-oob': 'migrate-pause', 'id': %s }", id); } static void recv_cmd_id(QTestState *s, const char *id) @@ -229,9 +228,7 @@ static void test_qmp_oob(void) * Try any command that does not support OOB but with OOB flag. We * should get failure. */ - resp = qtest_qmp(qts, - "{ 'execute': 'query-cpus'," - " 'control': { 'run-oob': true } }"); + resp = qtest_qmp(qts, "{ 'exec-oob': 'query-cpus' }"); g_assert(qdict_haskey(resp, "error")); qobject_unref(resp); diff --git a/tests/test-qga.c b/tests/test-qga.c index febabc7ad5..daadf22ea3 100644 --- a/tests/test-qga.c +++ b/tests/test-qga.c @@ -249,8 +249,7 @@ static void test_qga_invalid_oob(gconstpointer fix) QDict *ret, *error; const char *class; - ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping'," - " 'control': {'run-oob': true}}"); + ret = qmp_fd(fixture->fd, "{'exec-oob': 'guest-ping'}"); g_assert_nonnull(ret); error = qdict_get_qdict(ret, "error"); -- cgit 1.4.1 From 69240fe62d1ae02257bc0694a11c478b10378948 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:43 +0200 Subject: qmp: Don't let malformed in-band commands jump the queue handle_qmp_command() reports certain errors right away. This is wrong when OOB is enabled, because the errors can "jump the queue" then, as the previous commit demonstrates. To fix, we need to delay errors until dispatch. Do that for semantic errors, mostly by reverting ill-advised parts of commit cf869d53172 "qmp: support out-of-band (oob) execution". Bonus: doesn't run qmp_dispatch_check_obj() twice, once in handle_qmp_command(), and again in do_qmp_dispatch(). That's also due to commit cf869d53172. The next commit will fix queue jumping for syntax errors. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-18-armbru@redhat.com> --- include/qapi/qmp/dispatch.h | 2 -- monitor.c | 79 +++++++++++---------------------------------- qapi/qmp-dispatch.c | 12 +++++-- tests/qmp-test.c | 4 +-- 4 files changed, 30 insertions(+), 67 deletions(-) (limited to 'qapi/qmp-dispatch.c') diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 303a15ba84..514bfc45b0 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -48,8 +48,6 @@ 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); -QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob, - Error **errp); QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request, bool allow_oob); bool qmp_is_oob(QDict *dict); diff --git a/monitor.c b/monitor.c index c49214cd8f..be2a856d1c 100644 --- a/monitor.c +++ b/monitor.c @@ -1290,48 +1290,6 @@ static void qmp_caps_apply(Monitor *mon, QMPCapabilityList *list) } } -/* - * Return true if check successful, or false otherwise. When false is - * returned, detailed error will be in errp if provided. - */ -static bool qmp_cmd_oob_check(Monitor *mon, QDict *req, Error **errp) -{ - const char *command; - QmpCommand *cmd; - - command = qdict_get_try_str(req, "execute"); - if (!command) { - command = qdict_get_try_str(req, "exec-oob"); - } - if (!command) { - error_setg(errp, "Command field 'execute' missing"); - return false; - } - - cmd = qmp_find_command(mon->qmp.commands, command); - if (!cmd) { - if (mon->qmp.commands == &qmp_cap_negotiation_commands) { - error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND, - "Expecting capabilities negotiation " - "with 'qmp_capabilities'"); - } else { - error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND, - "The command %s has not been found", command); - } - return false; - } - - if (qmp_is_oob(req)) { - if (!(cmd->options & QCO_ALLOW_OOB)) { - error_setg(errp, "The command %s does not support OOB", - command); - return false; - } - } - - return true; -} - void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable, Error **errp) { @@ -4171,6 +4129,7 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id) { Monitor *old_mon; QObject *rsp; + QDict *error; old_mon = cur_mon; cur_mon = mon; @@ -4179,6 +4138,19 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id) cur_mon = old_mon; + if (mon->qmp.commands == &qmp_cap_negotiation_commands) { + error = qdict_get_qdict(qobject_to(QDict, rsp), "error"); + if (error + && !g_strcmp0(qdict_get_try_str(error, "class"), + QapiErrorClass_str(ERROR_CLASS_COMMAND_NOT_FOUND))) { + /* Provide a more useful error message */ + qdict_del(error, "desc"); + qdict_put_str(error, "desc", "Expecting capabilities negotiation" + " with 'qmp_capabilities'"); + } + } + + /* Respond if necessary */ monitor_qmp_respond(mon, rsp, NULL, qobject_ref(id)); } @@ -4256,7 +4228,9 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) error_setg(&err, QERR_JSON_PARSING); } if (err) { - goto err; + assert(!req); + monitor_qmp_respond(mon, NULL, err, NULL); + return; } qdict = qobject_to(QDict, req); @@ -4271,18 +4245,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) qobject_unref(req_json); } - /* Check against the request in general layout */ - qdict = qmp_dispatch_check_obj(req, qmp_oob_enabled(mon), &err); - if (!qdict) { - goto err; - } - - /* Check against OOB specific */ - if (!qmp_cmd_oob_check(mon, qdict, &err)) { - goto err; - } - - if (qmp_is_oob(qdict)) { + if (qdict && qmp_is_oob(qdict)) { /* Out-of-band (OOB) requests are executed directly in parser. */ trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id) ?: ""); @@ -4336,12 +4299,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) /* Kick the dispatcher routine */ qemu_bh_schedule(mon_global.qmp_dispatcher_bh); - return; - -err: - /* FIXME overtakes queued in-band commands, wrong when !qmp_is_oob() */ - monitor_qmp_respond(mon, NULL, err, NULL); - qobject_unref(req); } static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size) diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 12be120fe7..6d78f3e9f6 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -20,8 +20,8 @@ #include "qapi/qmp/qbool.h" #include "sysemu/sysemu.h" -QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob, - Error **errp) +static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob, + Error **errp) { const char *exec_key = NULL; const QDictEntry *ent; @@ -78,6 +78,7 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request, bool allow_oob, Error **errp) { Error *local_err = NULL; + bool oob; const char *command; QDict *args, *dict; QmpCommand *cmd; @@ -89,9 +90,11 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request, } command = qdict_get_try_str(dict, "execute"); + oob = false; if (!command) { assert(allow_oob); command = qdict_get_str(dict, "exec-oob"); + oob = true; } cmd = qmp_find_command(cmds, command); if (cmd == NULL) { @@ -104,6 +107,11 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request, command); return NULL; } + if (oob && !(cmd->options & QCO_ALLOW_OOB)) { + error_setg(errp, "The command %s does not support OOB", + command); + return false; + } if (runstate_check(RUN_STATE_PRECONFIG) && !(cmd->options & QCO_ALLOW_PRECONFIG)) { diff --git a/tests/qmp-test.c b/tests/qmp-test.c index 9eb20a1a18..ceaf4a6789 100644 --- a/tests/qmp-test.c +++ b/tests/qmp-test.c @@ -242,12 +242,12 @@ static void test_qmp_oob(void) recv_cmd_id(qts, "ib-blocks-1"); recv_cmd_id(qts, "ib-quick-1"); - /* FIXME certain in-band errors overtake slow in-band command */ + /* Even malformed in-band command fails in-band */ send_cmd_that_blocks(qts, "blocks-2"); qtest_async_qmp(qts, "{ 'id': 'err-2' }"); - recv_cmd_id(qts, NULL); unblock_blocked_cmd(); recv_cmd_id(qts, "blocks-2"); + recv_cmd_id(qts, "err-2"); cleanup_blocking_cmd(); qtest_quit(qts); -- cgit 1.4.1 From cee32796cadc9510ee00f029a933009df7a28ae2 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:48 +0200 Subject: qmp: De-duplicate error response building All callers of qmp_build_error_object() duplicate the code to wrap it in a response object. Replace it by qmp_error_response() that captures the duplicated code, including error_free(). Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-23-armbru@redhat.com> --- include/qapi/qmp/dispatch.h | 2 +- monitor.c | 7 +------ qapi/qmp-dispatch.c | 20 +++++++++++--------- qga/main.c | 8 ++------ 4 files changed, 15 insertions(+), 22 deletions(-) (limited to 'qapi/qmp-dispatch.c') diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 514bfc45b0..a53e11c9b1 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -47,7 +47,7 @@ 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); +QDict *qmp_error_response(Error *err); QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request, bool allow_oob); bool qmp_is_oob(QDict *dict); diff --git a/monitor.c b/monitor.c index 26de90e4d2..6284efe33e 100644 --- a/monitor.c +++ b/monitor.c @@ -4107,14 +4107,9 @@ static int monitor_can_read(void *opaque) static void monitor_qmp_respond(Monitor *mon, QObject *rsp, Error *err, QObject *id) { - QDict *qdict = NULL; - if (err) { assert(!rsp); - qdict = qdict_new(); - qdict_put_obj(qdict, "error", qmp_build_error_object(err)); - error_free(err); - rsp = QOBJECT(qdict); + rsp = QOBJECT(qmp_error_response(err)); } if (rsp) { diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 6d78f3e9f6..c85748a33f 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -141,11 +141,15 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request, return ret; } -QObject *qmp_build_error_object(Error *err) +QDict *qmp_error_response(Error *err) { - return qobject_from_jsonf("{ 'class': %s, 'desc': %s }", - QapiErrorClass_str(error_get_class(err)), - error_get_pretty(err)); + QDict *rsp; + + rsp = qdict_from_jsonf_nofail("{ 'error': { 'class': %s, 'desc': %s } }", + QapiErrorClass_str(error_get_class(err)), + error_get_pretty(err)); + error_free(err); + return rsp; } /* @@ -166,15 +170,13 @@ QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request, ret = do_qmp_dispatch(cmds, request, allow_oob, &err); - rsp = qdict_new(); if (err) { - qdict_put_obj(rsp, "error", qmp_build_error_object(err)); - error_free(err); + rsp = qmp_error_response(err); } else if (ret) { + rsp = qdict_new(); qdict_put_obj(rsp, "return", ret); } else { - qobject_unref(rsp); - return NULL; + rsp = NULL; } return QOBJECT(rsp); diff --git a/qga/main.c b/qga/main.c index d332bacce5..0e30e30248 100644 --- a/qga/main.c +++ b/qga/main.c @@ -610,15 +610,13 @@ static void process_event(JSONMessageParser *parser, GQueue *tokens) qdict = qobject_to(QDict, json_parser_parse_err(tokens, NULL, &err)); if (err || !qdict) { qobject_unref(qdict); - qdict = qdict_new(); if (!err) { g_warning("failed to parse event: unknown error"); error_setg(&err, QERR_JSON_PARSING); } else { g_warning("failed to parse event: %s", error_get_pretty(err)); } - qdict_put_obj(qdict, "error", qmp_build_error_object(err)); - error_free(err); + qdict = qmp_error_response(err); } /* handle host->guest commands */ @@ -627,11 +625,9 @@ static void process_event(JSONMessageParser *parser, GQueue *tokens) } else { if (!qdict_haskey(qdict, "error")) { qobject_unref(qdict); - qdict = qdict_new(); g_warning("unrecognized payload format"); error_setg(&err, QERR_UNSUPPORTED); - qdict_put_obj(qdict, "error", qmp_build_error_object(err)); - error_free(err); + qdict = qmp_error_response(err); } ret = send_response(s, QOBJECT(qdict)); if (ret < 0) { -- cgit 1.4.1 From d43b16945afa8457b03aee543a110c4ff0b7f070 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:49 +0200 Subject: qmp: Use QDict * instead of QObject * for response objects By using the more specific type, we get fewer downcasts. The downcasts are safe, but not obviously so, at least not locally. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-24-armbru@redhat.com> --- include/qapi/qmp/dispatch.h | 4 ++-- monitor.c | 31 ++++++++++++++++--------------- qapi/qmp-dispatch.c | 6 +++--- qga/main.c | 8 ++++---- tests/test-qmp-cmds.c | 17 +++++++---------- 5 files changed, 32 insertions(+), 34 deletions(-) (limited to 'qapi/qmp-dispatch.c') diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index a53e11c9b1..4e2e749faf 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -48,8 +48,8 @@ bool qmp_command_is_enabled(const QmpCommand *cmd); const char *qmp_command_name(const QmpCommand *cmd); bool qmp_has_success_response(const QmpCommand *cmd); QDict *qmp_error_response(Error *err); -QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request, - bool allow_oob); +QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request, + bool allow_oob); bool qmp_is_oob(QDict *dict); typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque); diff --git a/monitor.c b/monitor.c index 6284efe33e..8237b1c916 100644 --- a/monitor.c +++ b/monitor.c @@ -379,7 +379,7 @@ static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon) static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon) { while (!g_queue_is_empty(mon->qmp.qmp_responses)) { - qobject_unref((QObject *)g_queue_pop_head(mon->qmp.qmp_responses)); + qobject_unref((QDict *)g_queue_pop_head(mon->qmp.qmp_responses)); } } @@ -528,7 +528,8 @@ static void monitor_json_emitter(Monitor *mon, QObject *data) * responder thread). */ qemu_mutex_lock(&mon->qmp.qmp_queue_lock); - g_queue_push_tail(mon->qmp.qmp_responses, qobject_ref(data)); + g_queue_push_tail(mon->qmp.qmp_responses, + qobject_ref(qobject_to(QDict, data))); qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); qemu_bh_schedule(qmp_respond_bh); } else { @@ -542,13 +543,13 @@ static void monitor_json_emitter(Monitor *mon, QObject *data) struct QMPResponse { Monitor *mon; - QObject *data; + QDict *data; }; typedef struct QMPResponse QMPResponse; -static QObject *monitor_qmp_response_pop_one(Monitor *mon) +static QDict *monitor_qmp_response_pop_one(Monitor *mon) { - QObject *data; + QDict *data; qemu_mutex_lock(&mon->qmp.qmp_queue_lock); data = g_queue_pop_head(mon->qmp.qmp_responses); @@ -559,10 +560,10 @@ static QObject *monitor_qmp_response_pop_one(Monitor *mon) static void monitor_qmp_response_flush(Monitor *mon) { - QObject *data; + QDict *data; while ((data = monitor_qmp_response_pop_one(mon))) { - monitor_json_emitter_raw(mon, data); + monitor_json_emitter_raw(mon, QOBJECT(data)); qobject_unref(data); } } @@ -574,7 +575,7 @@ static void monitor_qmp_response_flush(Monitor *mon) static bool monitor_qmp_response_pop_any(QMPResponse *response) { Monitor *mon; - QObject *data = NULL; + QDict *data = NULL; qemu_mutex_lock(&monitor_lock); QTAILQ_FOREACH(mon, &mon_list, entry) { @@ -594,7 +595,7 @@ static void monitor_qmp_bh_responder(void *opaque) QMPResponse response; while (monitor_qmp_response_pop_any(&response)) { - monitor_json_emitter_raw(response.mon, response.data); + monitor_json_emitter_raw(response.mon, QOBJECT(response.data)); qobject_unref(response.data); } } @@ -4104,20 +4105,20 @@ static int monitor_can_read(void *opaque) * 2. rsp, err, and id may be NULL. * 3. If err != NULL then rsp must be NULL. */ -static void monitor_qmp_respond(Monitor *mon, QObject *rsp, +static void monitor_qmp_respond(Monitor *mon, QDict *rsp, Error *err, QObject *id) { if (err) { assert(!rsp); - rsp = QOBJECT(qmp_error_response(err)); + rsp = qmp_error_response(err); } if (rsp) { if (id) { - qdict_put_obj(qobject_to(QDict, rsp), "id", qobject_ref(id)); + qdict_put_obj(rsp, "id", qobject_ref(id)); } - monitor_json_emitter(mon, rsp); + monitor_json_emitter(mon, QOBJECT(rsp)); } qobject_unref(id); @@ -4127,7 +4128,7 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp, static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id) { Monitor *old_mon; - QObject *rsp; + QDict *rsp; QDict *error; old_mon = cur_mon; @@ -4138,7 +4139,7 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id) cur_mon = old_mon; if (mon->qmp.commands == &qmp_cap_negotiation_commands) { - error = qdict_get_qdict(qobject_to(QDict, rsp), "error"); + error = qdict_get_qdict(rsp, "error"); if (error && !g_strcmp0(qdict_get_try_str(error, "class"), QapiErrorClass_str(ERROR_CLASS_COMMAND_NOT_FOUND))) { diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index c85748a33f..761812e924 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -161,8 +161,8 @@ bool qmp_is_oob(QDict *dict) && !qdict_haskey(dict, "execute"); } -QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request, - bool allow_oob) +QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request, + bool allow_oob) { Error *err = NULL; QObject *ret; @@ -179,5 +179,5 @@ QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request, rsp = NULL; } - return QOBJECT(rsp); + return rsp; } diff --git a/qga/main.c b/qga/main.c index 0e30e30248..537cc0e162 100644 --- a/qga/main.c +++ b/qga/main.c @@ -545,7 +545,7 @@ fail: #endif } -static int send_response(GAState *s, QObject *payload) +static int send_response(GAState *s, QDict *payload) { const char *buf; QString *payload_qstr, *response_qstr; @@ -553,7 +553,7 @@ static int send_response(GAState *s, QObject *payload) g_assert(payload && s->channel); - payload_qstr = qobject_to_json(payload); + payload_qstr = qobject_to_json(QOBJECT(payload)); if (!payload_qstr) { return -EINVAL; } @@ -581,7 +581,7 @@ static int send_response(GAState *s, QObject *payload) static void process_command(GAState *s, QDict *req) { - QObject *rsp = NULL; + QDict *rsp; int ret; g_assert(req); @@ -629,7 +629,7 @@ static void process_event(JSONMessageParser *parser, GQueue *tokens) error_setg(&err, QERR_UNSUPPORTED); qdict = qmp_error_response(err); } - ret = send_response(s, QOBJECT(qdict)); + ret = send_response(s, qdict); if (ret < 0) { g_warning("error sending error response: %s", strerror(-ret)); } diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c index 10c7ba40c1..afb338a61e 100644 --- a/tests/test-qmp-cmds.c +++ b/tests/test-qmp-cmds.c @@ -98,13 +98,13 @@ __org_qemu_x_Union1 *qmp___org_qemu_x_command(__org_qemu_x_EnumList *a, static void test_dispatch_cmd(void) { QDict *req = qdict_new(); - QObject *resp; + QDict *resp; qdict_put_str(req, "execute", "user_def_cmd"); resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false); assert(resp != NULL); - assert(!qdict_haskey(qobject_to(QDict, resp), "error")); + assert(!qdict_haskey(resp, "error")); qobject_unref(resp); qobject_unref(req); @@ -115,13 +115,13 @@ static void test_dispatch_cmd_failure(void) { QDict *req = qdict_new(); QDict *args = qdict_new(); - QObject *resp; + QDict *resp; qdict_put_str(req, "execute", "user_def_cmd2"); resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false); assert(resp != NULL); - assert(qdict_haskey(qobject_to(QDict, resp), "error")); + assert(qdict_haskey(resp, "error")); qobject_unref(resp); qobject_unref(req); @@ -135,7 +135,7 @@ static void test_dispatch_cmd_failure(void) resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false); assert(resp != NULL); - assert(qdict_haskey(qobject_to(QDict, resp), "error")); + assert(qdict_haskey(resp, "error")); qobject_unref(resp); qobject_unref(req); @@ -143,18 +143,15 @@ static void test_dispatch_cmd_failure(void) static QObject *test_qmp_dispatch(QDict *req) { - QObject *resp_obj; QDict *resp; QObject *ret; - resp_obj = qmp_dispatch(&qmp_commands, QOBJECT(req), false); - assert(resp_obj); - resp = qobject_to(QDict, resp_obj); + resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false); assert(resp && !qdict_haskey(resp, "error")); ret = qdict_get(resp, "return"); assert(ret); qobject_ref(ret); - qobject_unref(resp_obj); + qobject_unref(resp); return ret; } -- cgit 1.4.1 From 62e93be286626839ef2597da81f70a3ba4a31fff Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 3 Jul 2018 10:53:53 +0200 Subject: qmp: Add some comments around null responses Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180703085358.13941-28-armbru@redhat.com> --- qapi/qmp-dispatch.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'qapi/qmp-dispatch.c') diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 761812e924..6f2d466596 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -133,6 +133,7 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request, } else if (cmd->options & QCO_NO_SUCCESS_RESP) { g_assert(!ret); } else if (!ret) { + /* TODO turn into assertion */ ret = QOBJECT(qdict_new()); } @@ -176,6 +177,7 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request, rsp = qdict_new(); qdict_put_obj(rsp, "return", ret); } else { + /* Can only happen for commands with QCO_NO_SUCCESS_RESP */ rsp = NULL; } -- cgit 1.4.1