From fa15cf8b5c77fc5837bbd67066735b6da5b00ee2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Mon, 26 Mar 2018 19:20:41 +0200 Subject: [PATCH 01/14] qmp-test: fix response leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apparently introduced in commit a4f90923b520f1dc0a768634877eb412e5052c26. Signed-off-by: Marc-André Lureau Message-Id: <20180326172041.21009-1-marcandre.lureau@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Eric Blake --- tests/qmp-test.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qmp-test.c b/tests/qmp-test.c index 558e83540c..8de99a4727 100644 --- a/tests/qmp-test.c +++ b/tests/qmp-test.c @@ -90,6 +90,7 @@ static void test_qmp_protocol(void) 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 = qtest_qmp(qts, "{ 'execute': 'query-version' }"); From fdf235ba15167faa05fedb838562fc06e238f400 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 23 Mar 2018 15:43:41 -0500 Subject: [PATCH 02/14] tests: Silence false positive warning on generated test name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running 'make check' on rawhide with gcc 8.0.1 fails: tests/test-visitor-serialization.c: In function 'main': tests/test-visitor-serialization.c:1127:34: error: '/primitives/' directive writing 12 bytes into a region of size between 1 and 128 [-Werror=format-overflow=] The warning is a false positive (we have two buffers of size 128, so yes, if we FULLY used the first buffer, then sprint'ing it into the second will overflow the second). But in practice, our first buffer will not be longer than "/visitor/serialization/String", so sizing it smaller is enough to let gcc see that we don't overflow the second. Signed-off-by: Eric Blake Message-Id: <20180323204341.1501664-1-eblake@redhat.com> Reviewed-by: Marc-André Lureau --- tests/test-visitor-serialization.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c index 438c18a0d6..d18d90db2c 100644 --- a/tests/test-visitor-serialization.c +++ b/tests/test-visitor-serialization.c @@ -1115,7 +1115,7 @@ static const SerializeOps visitors[] = { static void add_visitor_type(const SerializeOps *ops) { - char testname_prefix[128]; + char testname_prefix[32]; char testname[128]; TestArgs *args; int i = 0; From 2d9178d90f426d10860ac655f9748aa088870b1e Mon Sep 17 00:00:00 2001 From: Laurent Vivier Date: Fri, 23 Mar 2018 15:31:59 +0100 Subject: [PATCH 03/14] error: Strip trailing '\n' from error string arguments (again again) Re-run Coccinelle script scripts/coccinelle/err-bad-newline.cocci, and found new error_report() occurrences with '\n'. Signed-off-by: Laurent Vivier Message-Id: <20180323143202.28879-3-lvivier@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Eric Blake --- target/i386/hvf/hvf.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c index 15870a4f36..c36753954b 100644 --- a/target/i386/hvf/hvf.c +++ b/target/i386/hvf/hvf.c @@ -86,25 +86,25 @@ static void assert_hvf_ok(hv_return_t ret) switch (ret) { case HV_ERROR: - error_report("Error: HV_ERROR\n"); + error_report("Error: HV_ERROR"); break; case HV_BUSY: - error_report("Error: HV_BUSY\n"); + error_report("Error: HV_BUSY"); break; case HV_BAD_ARGUMENT: - error_report("Error: HV_BAD_ARGUMENT\n"); + error_report("Error: HV_BAD_ARGUMENT"); break; case HV_NO_RESOURCES: - error_report("Error: HV_NO_RESOURCES\n"); + error_report("Error: HV_NO_RESOURCES"); break; case HV_NO_DEVICE: - error_report("Error: HV_NO_DEVICE\n"); + error_report("Error: HV_NO_DEVICE"); break; case HV_UNSUPPORTED: - error_report("Error: HV_UNSUPPORTED\n"); + error_report("Error: HV_UNSUPPORTED"); break; default: - error_report("Unknown Error\n"); + error_report("Unknown Error"); } abort(); @@ -191,7 +191,7 @@ void hvf_set_phys_mem(MemoryRegionSection *section, bool add) if (mem) { mem->size = 0; if (do_hvf_set_memory(mem)) { - error_report("Failed to reset overlapping slot\n"); + error_report("Failed to reset overlapping slot"); abort(); } } @@ -211,7 +211,7 @@ void hvf_set_phys_mem(MemoryRegionSection *section, bool add) } if (x == hvf_state->num_slots) { - error_report("No free slots\n"); + error_report("No free slots"); abort(); } @@ -221,7 +221,7 @@ void hvf_set_phys_mem(MemoryRegionSection *section, bool add) mem->region = area; if (do_hvf_set_memory(mem)) { - error_report("Error registering new memory slot\n"); + error_report("Error registering new memory slot"); abort(); } } @@ -884,7 +884,7 @@ int hvf_vcpu_exec(CPUState *cpu) break; } default: - error_report("Unrecognized CR %d\n", cr); + error_report("Unrecognized CR %d", cr); abort(); } RIP(env) += ins_len; @@ -930,7 +930,7 @@ int hvf_vcpu_exec(CPUState *cpu) env->error_code = 0; break; default: - error_report("%llx: unhandled exit %llx\n", rip, exit_reason); + error_report("%llx: unhandled exit %llx", rip, exit_reason); } } while (ret == 0); From 710c2634078c030c1ca9cd3c15a96224ff3d90e2 Mon Sep 17 00:00:00 2001 From: Laurent Vivier Date: Fri, 23 Mar 2018 15:32:00 +0100 Subject: [PATCH 04/14] error: Remove NULL checks on error_propagate() calls Re-run Coccinelle patch scripts/coccinelle/error_propagate_null.cocci Signed-off-by: Laurent Vivier Message-Id: <20180323143202.28879-4-lvivier@redhat.com> Reviewed-by: Thomas Huth Signed-off-by: Eric Blake --- io/channel-websock.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/io/channel-websock.c b/io/channel-websock.c index ec48a305f0..e6608b969d 100644 --- a/io/channel-websock.c +++ b/io/channel-websock.c @@ -586,9 +586,7 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc, return TRUE; } - if (err) { - error_propagate(&wioc->io_err, err); - } + error_propagate(&wioc->io_err, err); trace_qio_channel_websock_handshake_reply(ioc); qio_channel_add_watch( From 625eaca9e516234ac70feeee5c4dad394c970820 Mon Sep 17 00:00:00 2001 From: Laurent Vivier Date: Fri, 23 Mar 2018 15:32:01 +0100 Subject: [PATCH 05/14] qdict: remove useless cast Re-run Coccinelle script scripts/coccinelle/qobject.cocci Signed-off-by: Laurent Vivier Message-Id: <20180323143202.28879-5-lvivier@redhat.com> Reviewed-by: Eric Blake Acked-by: Dr. David Alan Gilbert Acked-by: Fam Zheng Signed-off-by: Eric Blake --- block/nvme.c | 11 +++++------ monitor.c | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/block/nvme.c b/block/nvme.c index 8bca57aae6..c4f3a7bc94 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -695,12 +695,11 @@ static void nvme_parse_filename(const char *filename, QDict *options, unsigned long ns; const char *slash = strchr(tmp, '/'); if (!slash) { - qdict_put(options, NVME_BLOCK_OPT_DEVICE, - qstring_from_str(tmp)); + qdict_put_str(options, NVME_BLOCK_OPT_DEVICE, tmp); return; } device = g_strndup(tmp, slash - tmp); - qdict_put(options, NVME_BLOCK_OPT_DEVICE, qstring_from_str(device)); + qdict_put_str(options, NVME_BLOCK_OPT_DEVICE, device); g_free(device); namespace = slash + 1; if (*namespace && qemu_strtoul(namespace, NULL, 10, &ns)) { @@ -708,8 +707,8 @@ static void nvme_parse_filename(const char *filename, QDict *options, namespace); return; } - qdict_put(options, NVME_BLOCK_OPT_NAMESPACE, - qstring_from_str(*namespace ? namespace : "1")); + qdict_put_str(options, NVME_BLOCK_OPT_NAMESPACE, + *namespace ? namespace : "1"); } } @@ -1082,7 +1081,7 @@ static void nvme_refresh_filename(BlockDriverState *bs, QDict *opts) bs->drv->format_name); } - qdict_put(opts, "driver", qstring_from_str(bs->drv->format_name)); + qdict_put_str(opts, "driver", bs->drv->format_name); bs->full_open_options = opts; } diff --git a/monitor.c b/monitor.c index 77f4c41cfa..de709fc2e5 100644 --- a/monitor.c +++ b/monitor.c @@ -4315,7 +4315,7 @@ static QObject *get_qmp_greeting(Monitor *mon) /* Monitors that are not using IOThread won't support OOB */ continue; } - qlist_append(cap_list, qstring_from_str(QMPCapability_str(cap))); + qlist_append_str(cap_list, QMPCapability_str(cap)); } return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': %p}}", From 9ddb7456c87cd4f8fea2a08bb63183441709a872 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Mon, 26 Mar 2018 14:38:54 +0800 Subject: [PATCH 06/14] qmp: fix qmp_capabilities error regression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When someone sends a command before QMP handshake, the error used to be like this: {"execute": "query-cpus"} {"error": {"class": "CommandNotFound", "desc": "Expecting capabilities negotiation with 'qmp_capabilities'"}} While after cf869d5317 it becomes: {"execute": "query-cpus"} {"error": {"class": "CommandNotFound", "desc": "The command query-cpus has not been found"}} Fix it back to the nicer one. Fixes: cf869d5317 ("qmp: support out-of-band (oob) execution", 2018-03-19) Reported-by: Marc-André Lureau Signed-off-by: Peter Xu Message-Id: <20180326063901.27425-2-peterx@redhat.com> Reviewed-by: Marc-André Lureau Reviewed-by: Eric Blake [eblake: commit message grammar tweaks] Signed-off-by: Eric Blake --- monitor.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/monitor.c b/monitor.c index de709fc2e5..3b1ef34711 100644 --- a/monitor.c +++ b/monitor.c @@ -1203,8 +1203,14 @@ static bool qmp_cmd_oob_check(Monitor *mon, QDict *req, Error **errp) cmd = qmp_find_command(mon->qmp.commands, command); if (!cmd) { - error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND, - "The command %s has not been found", command); + 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; } @@ -4027,7 +4033,6 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj) { Monitor *mon, *old_mon; QObject *req, *rsp = NULL, *id; - QDict *qdict = NULL; bool need_resume; req = req_obj->req; @@ -4050,18 +4055,6 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj) cur_mon = old_mon; - 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_str(ERROR_CLASS_COMMAND_NOT_FOUND))) { - /* Provide a more useful error message */ - qdict_del(qdict, "desc"); - qdict_put_str(qdict, "desc", "Expecting capabilities negotiation" - " with 'qmp_capabilities'"); - } - } - /* Respond if necessary */ monitor_qmp_respond(mon, rsp, NULL, id); From 9408860165e07aaadec66c336f3dc849b945a8ed Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Mon, 26 Mar 2018 14:38:57 +0800 Subject: [PATCH 07/14] qapi: restrict allow-oob value to be "true" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It was missed in the first version of OOB series. We should check this to make sure we throw the right error when fault value is passed in. Signed-off-by: Peter Xu Message-Id: <20180326063901.27425-5-peterx@redhat.com> Reviewed-by: Marc-André Lureau Reviewed-by: Eric Blake Signed-off-by: Eric Blake --- scripts/qapi/common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 2c05e3c284..3e14bc41f2 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -872,7 +872,7 @@ def check_keys(expr_elem, meta, required, optional=[]): raise QAPISemError(info, "'%s' of %s '%s' should only use false value" % (key, meta, name)) - if key == 'boxed' and value is not True: + if (key == 'boxed' or key == 'allow-oob') and value is not True: raise QAPISemError(info, "'%s' of %s '%s' should only use true value" % (key, meta, name)) From 4bebca1e429a276bf9553dc2221862d2ea23a939 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Mon, 26 Mar 2018 14:38:58 +0800 Subject: [PATCH 08/14] tests: let qapi-schema tests detect oob MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The allow_oob parameter was passed in but not used in tests. Now reflect that in the tests, so we need to touch up other command testers with that new change. Reviewed-by: Eric Blake Signed-off-by: Peter Xu Message-Id: <20180326063901.27425-6-peterx@redhat.com> Reviewed-by: Marc-André Lureau Signed-off-by: Eric Blake --- tests/qapi-schema/doc-good.out | 4 ++-- tests/qapi-schema/ident-with-escape.out | 2 +- tests/qapi-schema/indented-expr.out | 4 ++-- tests/qapi-schema/qapi-schema-test.out | 18 +++++++++--------- tests/qapi-schema/test-qapi.py | 4 ++-- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out index 430b5a87db..63058b1590 100644 --- a/tests/qapi-schema/doc-good.out +++ b/tests/qapi-schema/doc-good.out @@ -28,9 +28,9 @@ object q_obj_cmd-arg member arg2: str optional=True member arg3: bool optional=False command cmd q_obj_cmd-arg -> Object - gen=True success_response=True boxed=False + gen=True success_response=True boxed=False oob=False command cmd-boxed Object -> None - gen=True success_response=True boxed=True + gen=True success_response=True boxed=True oob=False doc freeform body= = Section diff --git a/tests/qapi-schema/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out index ee3b34e623..82213aa51d 100644 --- a/tests/qapi-schema/ident-with-escape.out +++ b/tests/qapi-schema/ident-with-escape.out @@ -5,4 +5,4 @@ module ident-with-escape.json object q_obj_fooA-arg member bar1: str optional=False command fooA q_obj_fooA-arg -> None - gen=True success_response=True boxed=False + gen=True success_response=True boxed=False oob=False diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out index a79935e8c3..862678f8f4 100644 --- a/tests/qapi-schema/indented-expr.out +++ b/tests/qapi-schema/indented-expr.out @@ -3,6 +3,6 @@ enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool'] prefix QTYPE module indented-expr.json command eins None -> None - gen=True success_response=True boxed=False + gen=True success_response=True boxed=False oob=False command zwei None -> None - gen=True success_response=True boxed=False + gen=True success_response=True boxed=False oob=False diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 012e7fc06a..4f43370017 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -16,7 +16,7 @@ object Empty1 object Empty2 base Empty1 command user_def_cmd0 Empty2 -> Empty2 - gen=True success_response=True boxed=False + gen=True success_response=True boxed=False oob=False enum QEnumTwo ['value1', 'value2'] prefix QENUM_TWO object UserDefOne @@ -143,29 +143,29 @@ object UserDefNativeListUnion case sizes: q_obj_sizeList-wrapper case any: q_obj_anyList-wrapper command user_def_cmd None -> None - gen=True success_response=True boxed=False + gen=True success_response=True boxed=False oob=False object q_obj_user_def_cmd1-arg member ud1a: UserDefOne optional=False command user_def_cmd1 q_obj_user_def_cmd1-arg -> None - gen=True success_response=True boxed=False + gen=True success_response=True boxed=False oob=False object q_obj_user_def_cmd2-arg member ud1a: UserDefOne optional=False member ud1b: UserDefOne optional=True command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo - gen=True success_response=True boxed=False + gen=True success_response=True boxed=False oob=False object q_obj_guest-get-time-arg member a: int optional=False member b: int optional=True command guest-get-time q_obj_guest-get-time-arg -> int - gen=True success_response=True boxed=False + gen=True success_response=True boxed=False oob=False object q_obj_guest-sync-arg member arg: any optional=False command guest-sync q_obj_guest-sync-arg -> any - gen=True success_response=True boxed=False + gen=True success_response=True boxed=False oob=False command boxed-struct UserDefZero -> None - gen=True success_response=True boxed=True + gen=True success_response=True boxed=True oob=False command boxed-union UserDefNativeListUnion -> None - gen=True success_response=True boxed=True + gen=True success_response=True boxed=True oob=False object UserDefOptions member i64: intList optional=True member u64: uint64List optional=True @@ -229,4 +229,4 @@ object q_obj___org.qemu_x-command-arg member c: __org.qemu_x-Union2 optional=False member d: __org.qemu_x-Alt optional=False command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> __org.qemu_x-Union1 - gen=True success_response=True boxed=False + gen=True success_response=True boxed=False oob=False diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py index 10e68b01d9..c1a144ba29 100644 --- a/tests/qapi-schema/test-qapi.py +++ b/tests/qapi-schema/test-qapi.py @@ -45,8 +45,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): gen, success_response, boxed, allow_oob): print('command %s %s -> %s' % \ (name, arg_type and arg_type.name, ret_type and ret_type.name)) - print(' gen=%s success_response=%s boxed=%s' % \ - (gen, success_response, boxed)) + print(' gen=%s success_response=%s boxed=%s oob=%s' % \ + (gen, success_response, boxed, allow_oob)) def visit_event(self, name, info, arg_type, boxed): print('event %s %s' % (name, arg_type and arg_type.name)) From 1a1b11dc0fb519f6dbc420925bde032e772fd610 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Mon, 26 Mar 2018 14:38:59 +0800 Subject: [PATCH 09/14] tests: add oob-test for qapi-schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It simply tests the new OOB capability, and make sure the QAPISchema can parse it correctly. Signed-off-by: Peter Xu Message-Id: <20180326063901.27425-7-peterx@redhat.com> Reviewed-by: Marc-André Lureau Reviewed-by: Eric Blake Signed-off-by: Eric Blake --- tests/Makefile.include | 1 + tests/qapi-schema/oob-test.err | 1 + tests/qapi-schema/oob-test.exit | 1 + tests/qapi-schema/oob-test.json | 2 ++ tests/qapi-schema/oob-test.out | 0 tests/qapi-schema/qapi-schema-test.json | 3 +++ tests/qapi-schema/qapi-schema-test.out | 2 ++ tests/test-qmp-cmds.c | 4 ++++ 8 files changed, 14 insertions(+) create mode 100644 tests/qapi-schema/oob-test.err create mode 100644 tests/qapi-schema/oob-test.exit create mode 100644 tests/qapi-schema/oob-test.json create mode 100644 tests/qapi-schema/oob-test.out diff --git a/tests/Makefile.include b/tests/Makefile.include index eb218a9539..3b9a5e31a2 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -523,6 +523,7 @@ qapi-schema += missing-comma-object.json qapi-schema += missing-type.json qapi-schema += nested-struct-data.json qapi-schema += non-objects.json +qapi-schema += oob-test.json qapi-schema += pragma-doc-required-crap.json qapi-schema += pragma-extra-junk.json qapi-schema += pragma-name-case-whitelist-crap.json diff --git a/tests/qapi-schema/oob-test.err b/tests/qapi-schema/oob-test.err new file mode 100644 index 0000000000..35b60f7480 --- /dev/null +++ b/tests/qapi-schema/oob-test.err @@ -0,0 +1 @@ +tests/qapi-schema/oob-test.json:2: 'allow-oob' of command 'oob-command-1' should only use true value diff --git a/tests/qapi-schema/oob-test.exit b/tests/qapi-schema/oob-test.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/oob-test.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/oob-test.json b/tests/qapi-schema/oob-test.json new file mode 100644 index 0000000000..da9635920f --- /dev/null +++ b/tests/qapi-schema/oob-test.json @@ -0,0 +1,2 @@ +# Check against oob illegal value +{ 'command': 'oob-command-1', 'allow-oob': 'some-string' } diff --git a/tests/qapi-schema/oob-test.out b/tests/qapi-schema/oob-test.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index c72dbd8050..06e30f452e 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -139,6 +139,9 @@ { 'command': 'boxed-struct', 'boxed': true, 'data': 'UserDefZero' } { 'command': 'boxed-union', 'data': 'UserDefNativeListUnion', 'boxed': true } +# Smoke test on Out-Of-Band +{ 'command': 'an-oob-command', 'allow-oob': true } + # For testing integer range flattening in opts-visitor. The following schema # corresponds to the option format: # diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 4f43370017..467577d770 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -166,6 +166,8 @@ command boxed-struct UserDefZero -> None gen=True success_response=True boxed=True oob=False command boxed-union UserDefNativeListUnion -> None gen=True success_response=True boxed=True oob=False +command an-oob-command None -> None + gen=True success_response=True boxed=False oob=True object UserDefOptions member i64: intList optional=True member u64: uint64List optional=True diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c index 93fbbb1b73..db690cc5ae 100644 --- a/tests/test-qmp-cmds.c +++ b/tests/test-qmp-cmds.c @@ -16,6 +16,10 @@ void qmp_user_def_cmd(Error **errp) { } +void qmp_an_oob_command(Error **errp) +{ +} + Empty2 *qmp_user_def_cmd0(Error **errp) { return g_new0(Empty2, 1); From 6d2d563f8ccc20a08a60ac87513ac113e0c881e3 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Mon, 26 Mar 2018 14:38:55 +0800 Subject: [PATCH 10/14] qmp: cleanup qmp queues properly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Marc-André Lureau reported that we can have this happen: 1. client1 connects, send command C1 2. client1 disconnects before getting response for C1 3. client2 connects, who might receive response of C1 However client2 should not receive remaining responses for client1. Basically, we should clean up the request/response queue elements when: - after a session is closed - before destroying the queues Some helpers are introduced to achieve that. We need to make sure we're with the lock when operating on those queues. This also needed the declaration of QMPRequest moved earlier. Reported-by: Marc-André Lureau Signed-off-by: Peter Xu Message-Id: <20180326063901.27425-3-peterx@redhat.com> Reviewed-by: Marc-André Lureau [eblake: drop pointless qmp_response_free(), drop queue flush on connect since a clean queue on disconnect is sufficient] Tested-by: Christian Borntraeger Signed-off-by: Eric Blake --- monitor.c | 71 ++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 19 deletions(-) diff --git a/monitor.c b/monitor.c index 3b1ef34711..32d6114ac3 100644 --- a/monitor.c +++ b/monitor.c @@ -234,6 +234,22 @@ static struct { QEMUBH *qmp_respond_bh; } mon_global; +struct QMPRequest { + /* Owner of the request */ + Monitor *mon; + /* "id" field of the request */ + QObject *id; + /* Request object to be handled */ + QObject *req; + /* + * Whether we need to resume the monitor afterward. This flag is + * used to emulate the old QMP server behavior that the current + * command must be completed before execution of the next one. + */ + bool need_resume; +}; +typedef struct QMPRequest QMPRequest; + /* QMP checker flags */ #define QMP_ACCEPT_UNKNOWNS 1 @@ -310,6 +326,38 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func, } } +static void qmp_request_free(QMPRequest *req) +{ + qobject_decref(req->id); + qobject_decref(req->req); + g_free(req); +} + +/* Must with the mon->qmp.qmp_queue_lock held */ +static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon) +{ + while (!g_queue_is_empty(mon->qmp.qmp_requests)) { + qmp_request_free(g_queue_pop_head(mon->qmp.qmp_requests)); + } +} + +/* Must with the mon->qmp.qmp_queue_lock held */ +static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon) +{ + while (!g_queue_is_empty(mon->qmp.qmp_responses)) { + qobject_decref(g_queue_pop_head(mon->qmp.qmp_responses)); + } +} + +static void monitor_qmp_cleanup_queues(Monitor *mon) +{ + qemu_mutex_lock(&mon->qmp.qmp_queue_lock); + monitor_qmp_cleanup_req_queue_locked(mon); + monitor_qmp_cleanup_resp_queue_locked(mon); + qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); +} + + static void monitor_flush_locked(Monitor *mon); static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond, @@ -701,6 +749,8 @@ static void monitor_data_destroy(Monitor *mon) QDECREF(mon->outbuf); qemu_mutex_destroy(&mon->out_lock); qemu_mutex_destroy(&mon->qmp.qmp_queue_lock); + monitor_qmp_cleanup_req_queue_locked(mon); + monitor_qmp_cleanup_resp_queue_locked(mon); g_queue_free(mon->qmp.qmp_requests); g_queue_free(mon->qmp.qmp_responses); } @@ -4009,22 +4059,6 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp, qobject_decref(rsp); } -struct QMPRequest { - /* Owner of the request */ - Monitor *mon; - /* "id" field of the request */ - QObject *id; - /* Request object to be handled */ - QObject *req; - /* - * Whether we need to resume the monitor afterward. This flag is - * used to emulate the old QMP server behavior that the current - * command must be completed before execution of the next one. - */ - bool need_resume; -}; -typedef struct QMPRequest QMPRequest; - /* * Dispatch one single QMP request. The function will free the req_obj * and objects inside it before return. @@ -4191,9 +4225,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) qapi_event_send_command_dropped(id, COMMAND_DROP_REASON_QUEUE_FULL, &error_abort); - qobject_decref(id); - qobject_decref(req); - g_free(req_obj); + qmp_request_free(req_obj); return; } } @@ -4335,6 +4367,7 @@ static void monitor_qmp_event(void *opaque, int event) mon_refcount++; break; case CHR_EVENT_CLOSED: + monitor_qmp_cleanup_queues(mon); json_message_parser_destroy(&mon->qmp.parser); json_message_parser_init(&mon->qmp.parser, handle_qmp_command); mon_refcount--; From be933ffc238dcedf0ba2bfa347b20b6dcb075b82 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Mon, 26 Mar 2018 14:38:56 +0800 Subject: [PATCH 11/14] monitor: new parameter "x-oob" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add new parameter to optionally enable Out-Of-Band for a QMP server. An example command line: ./qemu-system-x86_64 -chardev stdio,id=char0 \ -mon chardev=char0,mode=control,x-oob=on By default, Out-Of-Band is off. It is not allowed if either MUX or non-QMP is detected, since Out-Of-Band is currently only for QMP, and non-MUX chardev backends. Note that the client STILL has to request 'oob' during qmp_capabilities; in part because the x-oob command line option may disappear in the future if we decide the capabilities negotiation is sufficient. Signed-off-by: Peter Xu Message-Id: <20180326063901.27425-4-peterx@redhat.com> Reviewed-by: Marc-André Lureau [eblake: enhance commit message] Tested-by: Christian Borntraeger Signed-off-by: Eric Blake --- include/monitor/monitor.h | 1 + monitor.c | 22 ++++++++++++++++++++-- vl.c | 5 +++++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 0cb0538a31..d6ab70cae2 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -13,6 +13,7 @@ extern Monitor *cur_mon; #define MONITOR_USE_READLINE 0x02 #define MONITOR_USE_CONTROL 0x04 #define MONITOR_USE_PRETTY 0x08 +#define MONITOR_USE_OOB 0x10 bool monitor_cur_is_qmp(void); diff --git a/monitor.c b/monitor.c index 32d6114ac3..51f4cf480f 100644 --- a/monitor.c +++ b/monitor.c @@ -36,6 +36,7 @@ #include "net/slirp.h" #include "chardev/char-fe.h" #include "chardev/char-io.h" +#include "chardev/char-mux.h" #include "ui/qemu-spice.h" #include "sysemu/numa.h" #include "monitor/monitor.h" @@ -4562,12 +4563,26 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) void monitor_init(Chardev *chr, int flags) { Monitor *mon = g_malloc(sizeof(*mon)); + bool use_readline = flags & MONITOR_USE_READLINE; + bool use_oob = flags & MONITOR_USE_OOB; - monitor_data_init(mon, false, false); + if (use_oob) { + if (CHARDEV_IS_MUX(chr)) { + error_report("Monitor Out-Of-Band is not supported with " + "MUX typed chardev backend"); + exit(1); + } + if (use_readline) { + error_report("Monitor Out-Of-band is only supported by QMP"); + exit(1); + } + } + + monitor_data_init(mon, false, use_oob); qemu_chr_fe_init(&mon->chr, chr, &error_abort); mon->flags = flags; - if (flags & MONITOR_USE_READLINE) { + if (use_readline) { mon->rs = readline_init(monitor_readline_printf, monitor_readline_flush, mon, @@ -4663,6 +4678,9 @@ QemuOptsList qemu_mon_opts = { },{ .name = "pretty", .type = QEMU_OPT_BOOL, + },{ + .name = "x-oob", + .type = QEMU_OPT_BOOL, }, { /* end of list */ } }, diff --git a/vl.c b/vl.c index c81cc86607..5fd01bd5f6 100644 --- a/vl.c +++ b/vl.c @@ -2404,6 +2404,11 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp) if (qemu_opt_get_bool(opts, "pretty", 0)) flags |= MONITOR_USE_PRETTY; + /* OOB is off by default */ + if (qemu_opt_get_bool(opts, "x-oob", 0)) { + flags |= MONITOR_USE_OOB; + } + chardev = qemu_opt_get(opts, "chardev"); chr = qemu_chr_find(chardev); if (chr == NULL) { From ddee57e0176f6ab53b13c6c97605b62737a8fd7a Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 26 Mar 2018 20:36:19 -0500 Subject: [PATCH 12/14] tests: Add parameter to qtest_init_without_qmp_handshake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allow callers to choose whether to allow OOB support during a test; for now, all existing callers pass false, but the next patch will add a new caller. Also, rewrite the monitor setup to be generic (using the -qmp shorthand is insufficient for honoring the parameter). Based on an idea by Peter Xu, in <20180326063901.27425-8-peterx@redhat.com> Signed-off-by: Eric Blake Message-Id: <20180327013620.1644387-4-eblake@redhat.com> Tested-by: Christian Borntraeger Acked-by: Peter Xu Reviewed-by: Marc-André Lureau --- tests/libqtest.c | 10 ++++++---- tests/libqtest.h | 7 +++++-- tests/qmp-test.c | 2 +- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/tests/libqtest.c b/tests/libqtest.c index 200b2b9e92..6f33a37667 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -166,7 +166,8 @@ static const char *qtest_qemu_binary(void) return qemu_bin; } -QTestState *qtest_init_without_qmp_handshake(const char *extra_args) +QTestState *qtest_init_without_qmp_handshake(bool use_oob, + const char *extra_args) { QTestState *s; int sock, qmpsock, i; @@ -199,12 +200,13 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args) command = g_strdup_printf("exec %s " "-qtest unix:%s,nowait " "-qtest-log %s " - "-qmp unix:%s,nowait " + "-chardev socket,path=%s,nowait,id=char0 " + "-mon chardev=char0,mode=control%s " "-machine accel=qtest " "-display none " "%s", qemu_binary, socket_path, getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null", - qmp_socket_path, + qmp_socket_path, use_oob ? ",x-oob=on" : "", extra_args ?: ""); execlp("/bin/sh", "sh", "-c", command, NULL); exit(1); @@ -239,7 +241,7 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args) QTestState *qtest_init(const char *extra_args) { - QTestState *s = qtest_init_without_qmp_handshake(extra_args); + QTestState *s = qtest_init_without_qmp_handshake(false, extra_args); /* Read the QMP greeting and then do the handshake */ qtest_qmp_discard_response(s, ""); diff --git a/tests/libqtest.h b/tests/libqtest.h index 811169453a..cbe8df4473 100644 --- a/tests/libqtest.h +++ b/tests/libqtest.h @@ -56,11 +56,14 @@ QTestState *qtest_init(const char *extra_args); /** * qtest_init_without_qmp_handshake: - * @extra_args: other arguments to pass to QEMU. + * @use_oob: true to have the server advertise OOB support + * @extra_args: other arguments to pass to QEMU. CAUTION: these + * arguments are subject to word splitting and shell evaluation. * * Returns: #QTestState instance. */ -QTestState *qtest_init_without_qmp_handshake(const char *extra_args); +QTestState *qtest_init_without_qmp_handshake(bool use_oob, + const char *extra_args); /** * qtest_quit: diff --git a/tests/qmp-test.c b/tests/qmp-test.c index 8de99a4727..2134d95db9 100644 --- a/tests/qmp-test.c +++ b/tests/qmp-test.c @@ -81,7 +81,7 @@ static void test_qmp_protocol(void) QList *capabilities; QTestState *qts; - qts = qtest_init_without_qmp_handshake(common_args); + qts = qtest_init_without_qmp_handshake(false, common_args); /* Test greeting */ resp = qtest_qmp_receive(qts); From fa198ad9bdef7303d4e3613c69ea00eece6b3a75 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Mon, 26 Mar 2018 14:39:01 +0800 Subject: [PATCH 13/14] tests: qmp-test: add test for new "x-oob" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test the new OOB capability. It's mostly the reverted OOB test (see commit 4fd78ad7), but differs in that: - It uses the new qtest_init_without_qmp_handshake() parameter to create the monitor with "x-oob" - Squashed the capability tests on greeting message - Don't use qtest_global any more, instead use self-maintained QTestState, which is the trend Signed-off-by: Peter Xu Message-Id: <20180326063901.27425-9-peterx@redhat.com> Reviewed-by: Eric Blake [eblake: rebase to qtest_init changes] Reviewed-by: Marc-André Lureau Tested-by: Christian Borntraeger Signed-off-by: Eric Blake --- tests/qmp-test.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/tests/qmp-test.c b/tests/qmp-test.c index 2134d95db9..772058fc4c 100644 --- a/tests/qmp-test.c +++ b/tests/qmp-test.c @@ -135,6 +135,87 @@ static void test_qmp_protocol(void) qtest_quit(qts); } +/* Tests for Out-Of-Band support. */ +static void test_qmp_oob(void) +{ + QTestState *qts; + QDict *resp, *q; + int acks = 0; + const QListEntry *entry; + QList *capabilities; + QString *qstr; + const char *cmd_id; + + qts = qtest_init_without_qmp_handshake(true, common_args); + + /* Check the greeting message. */ + resp = qtest_qmp_receive(qts); + q = qdict_get_qdict(resp, "QMP"); + g_assert(q); + capabilities = qdict_get_qlist(q, "capabilities"); + g_assert(capabilities && !qlist_empty(capabilities)); + entry = qlist_first(capabilities); + g_assert(entry); + qstr = qobject_to(QString, entry->value); + g_assert(qstr); + g_assert_cmpstr(qstring_get_str(qstr), ==, "oob"); + QDECREF(resp); + + /* Try a fake capability, it should fail. */ + resp = qtest_qmp(qts, + "{ 'execute': 'qmp_capabilities', " + " 'arguments': { 'enable': [ 'cap-does-not-exist' ] } }"); + g_assert(qdict_haskey(resp, "error")); + QDECREF(resp); + + /* Now, enable OOB in current QMP session, it should succeed. */ + resp = qtest_qmp(qts, + "{ 'execute': 'qmp_capabilities', " + " 'arguments': { 'enable': [ 'oob' ] } }"); + g_assert(qdict_haskey(resp, "return")); + QDECREF(resp); + + /* + * 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 } }"); + g_assert(qdict_haskey(resp, "error")); + QDECREF(resp); + + /* + * First send the "x-oob-test" command with lock=true and + * oob=false, it should hang the dispatcher and main thread; + * later, we send another lock=false with oob=true to continue + * that thread processing. Finally we should receive replies from + * both commands. + */ + qtest_async_qmp(qts, + "{ 'execute': 'x-oob-test'," + " 'arguments': { 'lock': true }, " + " 'id': 'lock-cmd'}"); + qtest_async_qmp(qts, + "{ 'execute': 'x-oob-test', " + " 'arguments': { 'lock': false }, " + " 'control': { 'run-oob': true }, " + " 'id': 'unlock-cmd' }"); + + /* Ignore all events. Wait for 2 acks */ + while (acks < 2) { + resp = qtest_qmp_receive(qts); + cmd_id = qdict_get_str(resp, "id"); + if (!g_strcmp0(cmd_id, "lock-cmd") || + !g_strcmp0(cmd_id, "unlock-cmd")) { + acks++; + } + QDECREF(resp); + } + + qtest_quit(qts); +} + static int query_error_class(const char *cmd) { static struct { @@ -319,6 +400,7 @@ int main(int argc, char *argv[]) g_test_init(&argc, &argv, NULL); qtest_add_func("qmp/protocol", test_qmp_protocol); + qtest_add_func("qmp/oob", test_qmp_oob); qmp_schema_init(&schema); add_query_tests(&schema); From 0dfddbb537fcb0fbd045e1c890bc0e95f2ea5177 Mon Sep 17 00:00:00 2001 From: Satheesh Rajendran Date: Tue, 27 Mar 2018 18:08:00 +0530 Subject: [PATCH 14/14] hmp.c: Revert hmp_info_cpus output format change Commit 137b5cb6 refactored 'info cpus' output, changing 'thread_id' to 'thread-id'. While HMP is not a stable interface, it is trivial to keep the spelling consistent for test frameworks that have not yet updated to using QMP. This patch just reverts back output format to 'thread_id'. CC: Viktor Mihajlovski Signed-off-by: Satheesh Rajendran Message-Id: <20180327123800.28851-1-sathnaga@linux.vnet.ibm.com> Reviewed-by: Eric Blake Acked-by: Dr. David Alan Gilbert [eblake: improve commit message] Reviewed-by: Cornelia Huck Signed-off-by: Eric Blake --- hmp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hmp.c b/hmp.c index 679467d85a..a25c7bd9a8 100644 --- a/hmp.c +++ b/hmp.c @@ -381,7 +381,7 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict) monitor_printf(mon, "%c CPU #%" PRId64 ":", active, cpu->value->cpu_index); - monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id); + monitor_printf(mon, " thread_id=%" PRId64 "\n", cpu->value->thread_id); } qapi_free_CpuInfoFastList(cpu_list);