* [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests)
@ 2018-03-26 6:38 Peter Xu
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 1/8] qmp: fix qmp_capabilities error regression Peter Xu
` (9 more replies)
0 siblings, 10 replies; 29+ messages in thread
From: Peter Xu @ 2018-03-26 6:38 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Markus Armbruster,
Stefan Hajnoczi, Dr . David Alan Gilbert, peterx
I suppose these are all good even for 2.12, so marked in subject.
Tested with "make check" for all targets on x86_64, and iotest -raw.
Patch 1 fixes one OOB error message regression reported by Marc-Andre.
Patch 2 fixes one potential OOB problem when more than one clients are
there, reported by Marc-Andre (too).
Patch 3 introduce "-mon x-oob=on" parameter to allow user to
explicitly enable Out-Of-Band for a specific monitor.
Patch 4-6 are qapi-schema fixes and tests for Out-Of-Band.
Patch 7-8 add back the OOB test on the new parameter (with more
enhancements).
Please review, thanks.
Peter Xu (8):
qmp: fix qmp_capabilities error regression
qmp: cleanup qmp queues properly
monitor: new parameter "x-oob"
qapi: restrict allow-oob value to be "true"
tests: let qapi-schema tests detect oob
tests: add oob-test for qapi-schema
tests: introduce qtest_init_with_qmp_format()
tests: qmp-test: add test for new "x-oob"
include/monitor/monitor.h | 1 +
monitor.c | 124 ++++++++++++++++++++++----------
scripts/qapi/common.py | 2 +-
tests/Makefile.include | 1 +
tests/libqtest.c | 14 +++-
tests/libqtest.h | 14 ++++
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/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 | 20 +++---
tests/qapi-schema/test-qapi.py | 4 +-
tests/qmp-test.c | 84 ++++++++++++++++++++++
tests/test-qmp-cmds.c | 4 ++
vl.c | 5 ++
19 files changed, 233 insertions(+), 57 deletions(-)
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
--
2.14.3
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH for-2.12 1/8] qmp: fix qmp_capabilities error regression
2018-03-26 6:38 [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests) Peter Xu
@ 2018-03-26 6:38 ` Peter Xu
2018-03-26 8:40 ` Marc-André Lureau
2018-03-26 19:35 ` Eric Blake
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly Peter Xu
` (8 subsequent siblings)
9 siblings, 2 replies; 29+ messages in thread
From: Peter Xu @ 2018-03-26 6:38 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Markus Armbruster,
Stefan Hajnoczi, Dr . David Alan Gilbert, peterx
When someone sents a command before QMP handshake, error was 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 <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
monitor.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/monitor.c b/monitor.c
index 77f4c41cfa..849fa23bf9 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);
--
2.14.3
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly
2018-03-26 6:38 [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests) Peter Xu
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 1/8] qmp: fix qmp_capabilities error regression Peter Xu
@ 2018-03-26 6:38 ` Peter Xu
2018-03-26 8:44 ` Marc-André Lureau
2018-03-26 19:42 ` Eric Blake
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob" Peter Xu
` (7 subsequent siblings)
9 siblings, 2 replies; 29+ messages in thread
From: Peter Xu @ 2018-03-26 6:38 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Markus Armbruster,
Stefan Hajnoczi, Dr . David Alan Gilbert, peterx
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:
- before a session established
- 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.
Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
monitor.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 59 insertions(+), 20 deletions(-)
diff --git a/monitor.c b/monitor.c
index 849fa23bf9..eba98df9da 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,43 @@ 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);
+}
+
+static void qmp_response_free(QObject *obj)
+{
+ qobject_decref(obj);
+}
+
+/* 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)) {
+ qmp_response_free(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,
@@ -497,7 +550,7 @@ static void monitor_qmp_bh_responder(void *opaque)
break;
}
monitor_json_emitter_raw(response.mon, response.data);
- qobject_decref(response.data);
+ qmp_response_free(response.data);
}
}
@@ -701,6 +754,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 +4064,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 +4230,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;
}
}
@@ -4327,6 +4364,7 @@ static void monitor_qmp_event(void *opaque, int event)
switch (event) {
case CHR_EVENT_OPENED:
+ monitor_qmp_cleanup_queues(mon);
mon->qmp.commands = &qmp_cap_negotiation_commands;
monitor_qmp_caps_reset(mon);
data = get_qmp_greeting(mon);
@@ -4335,6 +4373,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--;
--
2.14.3
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob"
2018-03-26 6:38 [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests) Peter Xu
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 1/8] qmp: fix qmp_capabilities error regression Peter Xu
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly Peter Xu
@ 2018-03-26 6:38 ` Peter Xu
2018-03-26 9:10 ` Marc-André Lureau
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 4/8] qapi: restrict allow-oob value to be "true" Peter Xu
` (6 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Peter Xu @ 2018-03-26 6:38 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Markus Armbruster,
Stefan Hajnoczi, Dr . David Alan Gilbert, peterx
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.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
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 eba98df9da..d77ccc8785 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"
@@ -4568,12 +4569,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;
+
+ 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, false);
+ 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,
@@ -4669,6 +4684,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) {
--
2.14.3
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH for-2.12 4/8] qapi: restrict allow-oob value to be "true"
2018-03-26 6:38 [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests) Peter Xu
` (2 preceding siblings ...)
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob" Peter Xu
@ 2018-03-26 6:38 ` Peter Xu
2018-03-26 9:11 ` Marc-André Lureau
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 5/8] tests: let qapi-schema tests detect oob Peter Xu
` (5 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Peter Xu @ 2018-03-26 6:38 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Markus Armbruster,
Stefan Hajnoczi, Dr . David Alan Gilbert, peterx
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 <peterx@redhat.com>
---
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))
--
2.14.3
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH for-2.12 5/8] tests: let qapi-schema tests detect oob
2018-03-26 6:38 [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests) Peter Xu
` (3 preceding siblings ...)
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 4/8] qapi: restrict allow-oob value to be "true" Peter Xu
@ 2018-03-26 6:38 ` Peter Xu
2018-03-26 9:13 ` Marc-André Lureau
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 6/8] tests: add oob-test for qapi-schema Peter Xu
` (4 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Peter Xu @ 2018-03-26 6:38 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Markus Armbruster,
Stefan Hajnoczi, Dr . David Alan Gilbert, peterx
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 <eblake@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
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))
--
2.14.3
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH for-2.12 6/8] tests: add oob-test for qapi-schema
2018-03-26 6:38 [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests) Peter Xu
` (4 preceding siblings ...)
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 5/8] tests: let qapi-schema tests detect oob Peter Xu
@ 2018-03-26 6:38 ` Peter Xu
2018-03-26 9:18 ` Marc-André Lureau
2018-03-26 20:26 ` Eric Blake
2018-03-26 6:39 ` [Qemu-devel] [PATCH for-2.12 7/8] tests: introduce qtest_init_with_qmp_format() Peter Xu
` (3 subsequent siblings)
9 siblings, 2 replies; 29+ messages in thread
From: Peter Xu @ 2018-03-26 6:38 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Markus Armbruster,
Stefan Hajnoczi, Dr . David Alan Gilbert, peterx
It simply tests the new OOB capability, and make sure the QAPISchema can
parse it correctly.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
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);
--
2.14.3
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH for-2.12 7/8] tests: introduce qtest_init_with_qmp_format()
2018-03-26 6:38 [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests) Peter Xu
` (5 preceding siblings ...)
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 6/8] tests: add oob-test for qapi-schema Peter Xu
@ 2018-03-26 6:39 ` Peter Xu
2018-03-26 9:18 ` Marc-André Lureau
` (2 more replies)
2018-03-26 6:39 ` [Qemu-devel] [PATCH for-2.12 8/8] tests: qmp-test: add test for new "x-oob" Peter Xu
` (2 subsequent siblings)
9 siblings, 3 replies; 29+ messages in thread
From: Peter Xu @ 2018-03-26 6:39 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Markus Armbruster,
Stefan Hajnoczi, Dr . David Alan Gilbert, peterx
It is abstracted from qtest_init_without_qmp_handshake(). It works just
like qtest_init_without_qmp_handshake() but further it would allow the
caller to specify the QMP parameter.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
tests/libqtest.c | 14 +++++++++++---
tests/libqtest.h | 14 ++++++++++++++
2 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 200b2b9e92..d2af1b17f0 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -166,19 +166,22 @@ static const char *qtest_qemu_binary(void)
return qemu_bin;
}
-QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
+QTestState *qtest_init_with_qmp_format(const char *extra_args,
+ const char *qmp_format)
{
QTestState *s;
int sock, qmpsock, i;
gchar *socket_path;
gchar *qmp_socket_path;
gchar *command;
+ gchar *qmp_params;
const char *qemu_binary = qtest_qemu_binary();
s = g_new(QTestState, 1);
socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
+ qmp_params = g_strdup_printf(qmp_format, qmp_socket_path);
/* It's possible that if an earlier test run crashed it might
* have left a stale unix socket lying around. Delete any
@@ -199,12 +202,12 @@ 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 "
+ "%s "
"-machine accel=qtest "
"-display none "
"%s", qemu_binary, socket_path,
getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null",
- qmp_socket_path,
+ qmp_params,
extra_args ?: "");
execlp("/bin/sh", "sh", "-c", command, NULL);
exit(1);
@@ -237,6 +240,11 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
return s;
}
+QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
+{
+ return qtest_init_with_qmp_format(extra_args, "-qmp unix:%s,nowait");
+}
+
QTestState *qtest_init(const char *extra_args)
{
QTestState *s = qtest_init_without_qmp_handshake(extra_args);
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 811169453a..1f3605ce73 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -62,6 +62,20 @@ QTestState *qtest_init(const char *extra_args);
*/
QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
+/**
+ * qtest_init_with_qmp_format:
+ * @extra_args: other arguments to pass to QEMU.
+ * @qmp_format: format of QMP parameters, should contain one "%s"
+ * field so that the socket path will be filled later.
+ *
+ * Note that this function will work just like
+ * qtest_init_without_qmp_handshake(), so no QMP handshake will be done.
+ *
+ * Returns: #QTestState instance.
+ */
+QTestState *qtest_init_with_qmp_format(const char *extra_args,
+ const char *qmp_format);
+
/**
* qtest_quit:
* @s: #QTestState instance to operate on.
--
2.14.3
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH for-2.12 8/8] tests: qmp-test: add test for new "x-oob"
2018-03-26 6:38 [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests) Peter Xu
` (6 preceding siblings ...)
2018-03-26 6:39 ` [Qemu-devel] [PATCH for-2.12 7/8] tests: introduce qtest_init_with_qmp_format() Peter Xu
@ 2018-03-26 6:39 ` Peter Xu
2018-03-26 20:54 ` Eric Blake
2018-03-26 10:18 ` [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests) Christian Borntraeger
2018-03-26 22:29 ` Eric Blake
9 siblings, 1 reply; 29+ messages in thread
From: Peter Xu @ 2018-03-26 6:39 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Markus Armbruster,
Stefan Hajnoczi, Dr . David Alan Gilbert, peterx
Test the new OOB capability. It's mostly the reverted OOB test, but
differs in that:
- It uses the new qtest_init_with_qmp_format() to create the monitor
with the new monitor parameter "-mon 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 <peterx@redhat.com>
---
tests/qmp-test.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 84 insertions(+)
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 558e83540c..4d15b0fca5 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -134,6 +134,89 @@ 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;
+ const char *qmp_params = "-chardev socket,path=%s,nowait,id=char0 "
+ "-mon chardev=char0,mode=control,x-oob=on";
+
+ qts = qtest_init_with_qmp_format(common_args, qmp_params);
+
+ /* Ignore 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 {
@@ -318,6 +401,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);
--
2.14.3
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 1/8] qmp: fix qmp_capabilities error regression
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 1/8] qmp: fix qmp_capabilities error regression Peter Xu
@ 2018-03-26 8:40 ` Marc-André Lureau
2018-03-26 19:35 ` Eric Blake
1 sibling, 0 replies; 29+ messages in thread
From: Marc-André Lureau @ 2018-03-26 8:40 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Eric Blake, Markus Armbruster, Stefan Hajnoczi,
Dr . David Alan Gilbert
On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
> When someone sents a command before QMP handshake, error was 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 <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> monitor.c | 23 ++++++++---------------
> 1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 77f4c41cfa..849fa23bf9 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);
>
> --
> 2.14.3
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly Peter Xu
@ 2018-03-26 8:44 ` Marc-André Lureau
2018-03-26 19:42 ` Eric Blake
1 sibling, 0 replies; 29+ messages in thread
From: Marc-André Lureau @ 2018-03-26 8:44 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Eric Blake, Markus Armbruster, Stefan Hajnoczi,
Dr . David Alan Gilbert
On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
> 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:
>
> - before a session established
> - 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.
>
> Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> monitor.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 59 insertions(+), 20 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 849fa23bf9..eba98df9da 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,43 @@ 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);
> +}
> +
> +static void qmp_response_free(QObject *obj)
> +{
> + qobject_decref(obj);
> +}
> +
> +/* 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)) {
> + qmp_response_free(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,
> @@ -497,7 +550,7 @@ static void monitor_qmp_bh_responder(void *opaque)
> break;
> }
> monitor_json_emitter_raw(response.mon, response.data);
> - qobject_decref(response.data);
> + qmp_response_free(response.data);
> }
> }
>
> @@ -701,6 +754,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 +4064,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 +4230,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;
> }
> }
> @@ -4327,6 +4364,7 @@ static void monitor_qmp_event(void *opaque, int event)
>
> switch (event) {
> case CHR_EVENT_OPENED:
> + monitor_qmp_cleanup_queues(mon);
> mon->qmp.commands = &qmp_cap_negotiation_commands;
> monitor_qmp_caps_reset(mon);
> data = get_qmp_greeting(mon);
> @@ -4335,6 +4373,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--;
> --
> 2.14.3
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob"
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob" Peter Xu
@ 2018-03-26 9:10 ` Marc-André Lureau
2018-03-26 9:13 ` Peter Xu
2018-03-26 20:01 ` Eric Blake
0 siblings, 2 replies; 29+ messages in thread
From: Marc-André Lureau @ 2018-03-26 9:10 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Eric Blake, Markus Armbruster, Stefan Hajnoczi,
Dr . David Alan Gilbert
Hi
On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
> 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.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> 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 eba98df9da..d77ccc8785 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"
> @@ -4568,12 +4569,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;
> +
> + 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);
> + }
> + }
I would rather see the error reporting / exit in vl.c:mon_init_func()
function, to have a single place for exit()
>
> - monitor_data_init(mon, false, false);
> + 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,
> @@ -4669,6 +4684,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) {
> --
> 2.14.3
>
Other than that, it looks fine.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 4/8] qapi: restrict allow-oob value to be "true"
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 4/8] qapi: restrict allow-oob value to be "true" Peter Xu
@ 2018-03-26 9:11 ` Marc-André Lureau
2018-03-26 19:43 ` Eric Blake
0 siblings, 1 reply; 29+ messages in thread
From: Marc-André Lureau @ 2018-03-26 9:11 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Eric Blake, Markus Armbruster, Stefan Hajnoczi,
Dr . David Alan Gilbert
On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
> 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 <peterx@redhat.com>
Not exactly required imho, but why not:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> 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))
> --
> 2.14.3
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob"
2018-03-26 9:10 ` Marc-André Lureau
@ 2018-03-26 9:13 ` Peter Xu
2018-03-26 20:01 ` Eric Blake
1 sibling, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-03-26 9:13 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Eric Blake, Markus Armbruster, Stefan Hajnoczi,
Dr . David Alan Gilbert
On Mon, Mar 26, 2018 at 11:10:30AM +0200, Marc-André Lureau wrote:
[...]
> > @@ -4568,12 +4569,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;
> > +
> > + 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);
> > + }
> > + }
>
> I would rather see the error reporting / exit in vl.c:mon_init_func()
> function, to have a single place for exit()
But there can be other callers for monitor_init() so I would assume
checking it here would be safer (though for now indeed mon_init_func()
is the only one that can pass OOB flag in).
[...]
> Other than that, it looks fine.
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Thanks for your quick reviews!
--
Peter Xu
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 5/8] tests: let qapi-schema tests detect oob
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 5/8] tests: let qapi-schema tests detect oob Peter Xu
@ 2018-03-26 9:13 ` Marc-André Lureau
0 siblings, 0 replies; 29+ messages in thread
From: Marc-André Lureau @ 2018-03-26 9:13 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Eric Blake, Markus Armbruster, Stefan Hajnoczi,
Dr . David Alan Gilbert
On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
> 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 <eblake@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> 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))
> --
> 2.14.3
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 7/8] tests: introduce qtest_init_with_qmp_format()
2018-03-26 6:39 ` [Qemu-devel] [PATCH for-2.12 7/8] tests: introduce qtest_init_with_qmp_format() Peter Xu
@ 2018-03-26 9:18 ` Marc-André Lureau
2018-03-26 20:42 ` Eric Blake
2018-03-26 21:48 ` Eric Blake
2 siblings, 0 replies; 29+ messages in thread
From: Marc-André Lureau @ 2018-03-26 9:18 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Eric Blake, Markus Armbruster, Stefan Hajnoczi,
Dr . David Alan Gilbert
On Mon, Mar 26, 2018 at 8:39 AM, Peter Xu <peterx@redhat.com> wrote:
> It is abstracted from qtest_init_without_qmp_handshake(). It works just
> like qtest_init_without_qmp_handshake() but further it would allow the
> caller to specify the QMP parameter.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> tests/libqtest.c | 14 +++++++++++---
> tests/libqtest.h | 14 ++++++++++++++
> 2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 200b2b9e92..d2af1b17f0 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -166,19 +166,22 @@ static const char *qtest_qemu_binary(void)
> return qemu_bin;
> }
>
> -QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
> +QTestState *qtest_init_with_qmp_format(const char *extra_args,
> + const char *qmp_format)
> {
> QTestState *s;
> int sock, qmpsock, i;
> gchar *socket_path;
> gchar *qmp_socket_path;
> gchar *command;
> + gchar *qmp_params;
> const char *qemu_binary = qtest_qemu_binary();
>
> s = g_new(QTestState, 1);
>
> socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
> qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
> + qmp_params = g_strdup_printf(qmp_format, qmp_socket_path);
>
> /* It's possible that if an earlier test run crashed it might
> * have left a stale unix socket lying around. Delete any
> @@ -199,12 +202,12 @@ 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 "
> + "%s "
> "-machine accel=qtest "
> "-display none "
> "%s", qemu_binary, socket_path,
> getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null",
> - qmp_socket_path,
> + qmp_params,
> extra_args ?: "");
> execlp("/bin/sh", "sh", "-c", command, NULL);
> exit(1);
> @@ -237,6 +240,11 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
> return s;
> }
>
> +QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
> +{
> + return qtest_init_with_qmp_format(extra_args, "-qmp unix:%s,nowait");
> +}
> +
> QTestState *qtest_init(const char *extra_args)
> {
> QTestState *s = qtest_init_without_qmp_handshake(extra_args);
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 811169453a..1f3605ce73 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -62,6 +62,20 @@ QTestState *qtest_init(const char *extra_args);
> */
> QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
>
> +/**
> + * qtest_init_with_qmp_format:
> + * @extra_args: other arguments to pass to QEMU.
> + * @qmp_format: format of QMP parameters, should contain one "%s"
> + * field so that the socket path will be filled later.
> + *
> + * Note that this function will work just like
> + * qtest_init_without_qmp_handshake(), so no QMP handshake will be done.
> + *
> + * Returns: #QTestState instance.
> + */
> +QTestState *qtest_init_with_qmp_format(const char *extra_args,
> + const char *qmp_format);
> +
> /**
> * qtest_quit:
> * @s: #QTestState instance to operate on.
> --
> 2.14.3
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 6/8] tests: add oob-test for qapi-schema
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 6/8] tests: add oob-test for qapi-schema Peter Xu
@ 2018-03-26 9:18 ` Marc-André Lureau
2018-03-26 20:26 ` Eric Blake
1 sibling, 0 replies; 29+ messages in thread
From: Marc-André Lureau @ 2018-03-26 9:18 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Eric Blake, Markus Armbruster, Stefan Hajnoczi,
Dr . David Alan Gilbert
On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
> It simply tests the new OOB capability, and make sure the QAPISchema can
> parse it correctly.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> 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);
> --
> 2.14.3
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests)
2018-03-26 6:38 [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests) Peter Xu
` (7 preceding siblings ...)
2018-03-26 6:39 ` [Qemu-devel] [PATCH for-2.12 8/8] tests: qmp-test: add test for new "x-oob" Peter Xu
@ 2018-03-26 10:18 ` Christian Borntraeger
2018-03-27 2:26 ` Peter Xu
2018-03-26 22:29 ` Eric Blake
9 siblings, 1 reply; 29+ messages in thread
From: Christian Borntraeger @ 2018-03-26 10:18 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Markus Armbruster, Dr . David Alan Gilbert, Stefan Hajnoczi,
Marc-André Lureau
Thanks for the quick fixing. This series on top of master
works fine on s390. (make check and iotests)
Christian
On 03/26/2018 08:38 AM, Peter Xu wrote:
> I suppose these are all good even for 2.12, so marked in subject.
> Tested with "make check" for all targets on x86_64, and iotest -raw.
>
> Patch 1 fixes one OOB error message regression reported by Marc-Andre.
>
> Patch 2 fixes one potential OOB problem when more than one clients are
> there, reported by Marc-Andre (too).
>
> Patch 3 introduce "-mon x-oob=on" parameter to allow user to
> explicitly enable Out-Of-Band for a specific monitor.
>
> Patch 4-6 are qapi-schema fixes and tests for Out-Of-Band.
>
> Patch 7-8 add back the OOB test on the new parameter (with more
> enhancements).
>
> Please review, thanks.
>
> Peter Xu (8):
> qmp: fix qmp_capabilities error regression
> qmp: cleanup qmp queues properly
> monitor: new parameter "x-oob"
> qapi: restrict allow-oob value to be "true"
> tests: let qapi-schema tests detect oob
> tests: add oob-test for qapi-schema
> tests: introduce qtest_init_with_qmp_format()
> tests: qmp-test: add test for new "x-oob"
>
> include/monitor/monitor.h | 1 +
> monitor.c | 124 ++++++++++++++++++++++----------
> scripts/qapi/common.py | 2 +-
> tests/Makefile.include | 1 +
> tests/libqtest.c | 14 +++-
> tests/libqtest.h | 14 ++++
> 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/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 | 20 +++---
> tests/qapi-schema/test-qapi.py | 4 +-
> tests/qmp-test.c | 84 ++++++++++++++++++++++
> tests/test-qmp-cmds.c | 4 ++
> vl.c | 5 ++
> 19 files changed, 233 insertions(+), 57 deletions(-)
> 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
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 1/8] qmp: fix qmp_capabilities error regression
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 1/8] qmp: fix qmp_capabilities error regression Peter Xu
2018-03-26 8:40 ` Marc-André Lureau
@ 2018-03-26 19:35 ` Eric Blake
1 sibling, 0 replies; 29+ messages in thread
From: Eric Blake @ 2018-03-26 19:35 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Marc-André Lureau, Markus Armbruster, Stefan Hajnoczi,
Dr . David Alan Gilbert
On 03/26/2018 01:38 AM, Peter Xu wrote:
> When someone sents a command before QMP handshake, error was like this:
s/sents/sends/
s/error was/the error used to be/
>
> {"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 <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> monitor.c | 23 ++++++++---------------
> 1 file changed, 8 insertions(+), 15 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly Peter Xu
2018-03-26 8:44 ` Marc-André Lureau
@ 2018-03-26 19:42 ` Eric Blake
2018-03-27 2:30 ` Peter Xu
1 sibling, 1 reply; 29+ messages in thread
From: Eric Blake @ 2018-03-26 19:42 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Marc-André Lureau, Markus Armbruster, Stefan Hajnoczi,
Dr . David Alan Gilbert
On 03/26/2018 01:38 AM, Peter Xu wrote:
> 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:
>
> - before a session established
Why here? [1]
> - 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.
>
It would also be helpful to mention that the patch includes code motion
to declare struct QMPRequest earlier in the file.
> Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> monitor.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 59 insertions(+), 20 deletions(-)
>
> +static void qmp_request_free(QMPRequest *req)
> +{
> + qobject_decref(req->id);
> + qobject_decref(req->req);
> + g_free(req);
> +}
> +
> +static void qmp_response_free(QObject *obj)
> +{
> + qobject_decref(obj);
> +}
Why do we need this function? Unless you plan to add to it in later
patches, I'd rather just inline things and directly call
qobject_decref() at the callsites...
> +
> +/* 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)) {
> + qmp_response_free(g_queue_pop_head(mon->qmp.qmp_responses));
...here,
> + }
> +}
> +
> +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,
> @@ -497,7 +550,7 @@ static void monitor_qmp_bh_responder(void *opaque)
> break;
> }
> monitor_json_emitter_raw(response.mon, response.data);
> - qobject_decref(response.data);
> + qmp_response_free(response.data);
and here.
> @@ -4327,6 +4364,7 @@ static void monitor_qmp_event(void *opaque, int event)
>
> switch (event) {
> case CHR_EVENT_OPENED:
> + monitor_qmp_cleanup_queues(mon);
[1] How would something be queued to need cleanup at this point, if we
already start with a clean queue before the first monitor, and if all
monitor close actions clean the queue?
> mon->qmp.commands = &qmp_cap_negotiation_commands;
> monitor_qmp_caps_reset(mon);
> data = get_qmp_greeting(mon);
> @@ -4335,6 +4373,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--;
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 4/8] qapi: restrict allow-oob value to be "true"
2018-03-26 9:11 ` Marc-André Lureau
@ 2018-03-26 19:43 ` Eric Blake
0 siblings, 0 replies; 29+ messages in thread
From: Eric Blake @ 2018-03-26 19:43 UTC (permalink / raw)
To: Marc-André Lureau, Peter Xu
Cc: qemu-devel, Markus Armbruster, Stefan Hajnoczi, Dr . David Alan Gilbert
On 03/26/2018 04:11 AM, Marc-André Lureau wrote:
> On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
>> 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 <peterx@redhat.com>
>
> Not exactly required imho, but why not:
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Not technically required, but does add some consistency to the QAPI parser.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob"
2018-03-26 9:10 ` Marc-André Lureau
2018-03-26 9:13 ` Peter Xu
@ 2018-03-26 20:01 ` Eric Blake
1 sibling, 0 replies; 29+ messages in thread
From: Eric Blake @ 2018-03-26 20:01 UTC (permalink / raw)
To: Marc-André Lureau, Peter Xu
Cc: qemu-devel, Markus Armbruster, Stefan Hajnoczi, Dr . David Alan Gilbert
On 03/26/2018 04:10 AM, Marc-André Lureau wrote:
> Hi
>
> On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> wrote:
>> 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.
Worth documenting in the commit message at least that even when OOB is
enabled, the client must STILL opt-in to using it by replying correctly
to qmp_capabilities, as well as mention that in the future, we may
remove x-oob and rely on JUST qmp_capabilities for enabling OOB.
>> @@ -4568,12 +4569,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;
>> +
>> + 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);
Should these two checks be swapped? Otherwise, if you use a MUX-typed
chardev for HMP, the message implies that switching chardev backend
might make it work, even though if you actually do that you'd then get
the failure for not being QMP.
>> + }
>> + }
>
> I would rather see the error reporting / exit in vl.c:mon_init_func()
> function, to have a single place for exit()
To do that, monitor_init() should change signatures to take Error
**errp. Probably worth doing if you spin a v2 of this series (adding
the parameter can be done as a separate patch, although there are only 5
callers in the tree so adjusting the callers at the same time is
probably not that hard to review).
>
> Other than that, it looks fine.
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Okay, I'll see how my review goes on the rest of the series before
deciding whether to request a v2.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 6/8] tests: add oob-test for qapi-schema
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 6/8] tests: add oob-test for qapi-schema Peter Xu
2018-03-26 9:18 ` Marc-André Lureau
@ 2018-03-26 20:26 ` Eric Blake
1 sibling, 0 replies; 29+ messages in thread
From: Eric Blake @ 2018-03-26 20:26 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Marc-André Lureau, Markus Armbruster, Stefan Hajnoczi,
Dr . David Alan Gilbert
On 03/26/2018 01:38 AM, Peter Xu wrote:
> It simply tests the new OOB capability, and make sure the QAPISchema can
> parse it correctly.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> 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
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 7/8] tests: introduce qtest_init_with_qmp_format()
2018-03-26 6:39 ` [Qemu-devel] [PATCH for-2.12 7/8] tests: introduce qtest_init_with_qmp_format() Peter Xu
2018-03-26 9:18 ` Marc-André Lureau
@ 2018-03-26 20:42 ` Eric Blake
2018-03-26 21:48 ` Eric Blake
2 siblings, 0 replies; 29+ messages in thread
From: Eric Blake @ 2018-03-26 20:42 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Marc-André Lureau, Markus Armbruster, Stefan Hajnoczi,
Dr . David Alan Gilbert
On 03/26/2018 01:39 AM, Peter Xu wrote:
> It is abstracted from qtest_init_without_qmp_handshake(). It works just
> like qtest_init_without_qmp_handshake() but further it would allow the
> caller to specify the QMP parameter.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> tests/libqtest.c | 14 +++++++++++---
> tests/libqtest.h | 14 ++++++++++++++
> 2 files changed, 25 insertions(+), 3 deletions(-)
>
[Reviewing in reverse order; you may want to look at
scripts/git.orderfile for how to present your patches in a more logical
manner with .h changes first.]
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 811169453a..1f3605ce73 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -62,6 +62,20 @@ QTestState *qtest_init(const char *extra_args);
> */
> QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
>
> +/**
> + * qtest_init_with_qmp_format:
> + * @extra_args: other arguments to pass to QEMU.
Not your fault, but I'm already not a fan of 'extra_args'; it would be
better to make these functions take an array of arguments, or even use
varargs, instead of relying on shell word splitting. Our testsuite is a
gaping security hole if executed in a directory where a shell
metacharacter causes the shell word splitting to do something different
than planned.
> + * @qmp_format: format of QMP parameters, should contain one "%s"
> + * field so that the socket path will be filled later.
> + *
> + * Note that this function will work just like
> + * qtest_init_without_qmp_handshake(), so no QMP handshake will be done.
> + *
> + * Returns: #QTestState instance.
> + */
> +QTestState *qtest_init_with_qmp_format(const char *extra_args,
> + const char *qmp_format);
Ouch - you didn't use any attribute to mark the format string so that
the compiler can enforce that it is treated as a printf-style argument.
I wonder if it would have been better to just have a 'bool use_oob'
parameter rather than playing ugly games with passing printf-style
format arguments around.
> +
> /**
> * qtest_quit:
> * @s: #QTestState instance to operate on.
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 200b2b9e92..d2af1b17f0 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -166,19 +166,22 @@ static const char *qtest_qemu_binary(void)
> return qemu_bin;
> }
>
> -QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
> +QTestState *qtest_init_with_qmp_format(const char *extra_args,
> + const char *qmp_format)
> {
> QTestState *s;
> int sock, qmpsock, i;
> gchar *socket_path;
> gchar *qmp_socket_path;
> gchar *command;
> + gchar *qmp_params;
> const char *qemu_binary = qtest_qemu_binary();
>
> s = g_new(QTestState, 1);
>
> socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
> qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
> + qmp_params = g_strdup_printf(qmp_format, qmp_socket_path);
And in addition to my earlier comment about not using a compiler
attribute, you aren't even bothering to assert that the caller didn't
pass in a garbage string, which can really lead to weird breakages.
>
> /* It's possible that if an earlier test run crashed it might
> * have left a stale unix socket lying around. Delete any
> @@ -199,12 +202,12 @@ 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 "
> + "%s "
> "-machine accel=qtest "
> "-display none "
> "%s", qemu_binary, socket_path,
> getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null",
> - qmp_socket_path,
> + qmp_params,
Again, if you used the idea of a 'bool use_oob', this would look more like:
"-qmp unix:%s,nowait%s ",
...
qmp_socket_path, use_oob ? "x-oob=on" : "",
which is a lot more limited in scope to prevent auditing nightmares,
while no less powerful for what you are actually using this new function
for.
> extra_args ?: "");
> execlp("/bin/sh", "sh", "-c", command, NULL);
> exit(1);
> @@ -237,6 +240,11 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
> return s;
> }
>
> +QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
> +{
> + return qtest_init_with_qmp_format(extra_args, "-qmp unix:%s,nowait");
> +}
There are so few callers of qtest_init_without_qmp_handshake() that I'd
just add the parameter to the existing function and update its two
callers, instead of adding yet another forwarding wrapper. Especially
if it is just adding a 'bool use_oob' parameter.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 8/8] tests: qmp-test: add test for new "x-oob"
2018-03-26 6:39 ` [Qemu-devel] [PATCH for-2.12 8/8] tests: qmp-test: add test for new "x-oob" Peter Xu
@ 2018-03-26 20:54 ` Eric Blake
0 siblings, 0 replies; 29+ messages in thread
From: Eric Blake @ 2018-03-26 20:54 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Marc-André Lureau, Markus Armbruster, Stefan Hajnoczi,
Dr . David Alan Gilbert
On 03/26/2018 01:39 AM, Peter Xu wrote:
> Test the new OOB capability. It's mostly the reverted OOB test, but
Helpful to mention which commit id has the reverts you are restoring.
Looks like 4fd78ad. Are you also restoring the tests reverted in cc79760
and a4f9092?
> differs in that:
>
> - It uses the new qtest_init_with_qmp_format() to create the monitor
> with the new monitor parameter "-mon 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 <peterx@redhat.com>
> ---
> tests/qmp-test.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 84 insertions(+)
>
> +++ b/tests/qmp-test.c
> @@ -134,6 +134,89 @@ 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;
> + const char *qmp_params = "-chardev socket,path=%s,nowait,id=char0 "
> + "-mon chardev=char0,mode=control,x-oob=on";
Again, I'd rather fold this into 7/8 qtest_init_without_qmp_handshake by
changing "-qmp unix:%s,nowait" into "-chardev
socket,path=%s,nowait,id=char0 -mon chardev=char0,mode=control" for ALL
tests (as that always works), then have a bool that decides whether we
also append ",x-oob=on" as needed.
> +
> + qts = qtest_init_with_qmp_format(common_args, qmp_params);
> +
> + /* Ignore the greeting message. */
Comment is wrong - you aren't ignoring it, but...
> + 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));
...actually inspecting what it contains.
> + 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);
> +
In the interest of getting this in -rc1, I will probably try and make
the changes I've mentioned in this thread, and post a v2 along those
lines; if you like my changes, I can send the pull request on my Tuesday
morning instead of waiting for another round of back-and-forth.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 7/8] tests: introduce qtest_init_with_qmp_format()
2018-03-26 6:39 ` [Qemu-devel] [PATCH for-2.12 7/8] tests: introduce qtest_init_with_qmp_format() Peter Xu
2018-03-26 9:18 ` Marc-André Lureau
2018-03-26 20:42 ` Eric Blake
@ 2018-03-26 21:48 ` Eric Blake
2 siblings, 0 replies; 29+ messages in thread
From: Eric Blake @ 2018-03-26 21:48 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Marc-André Lureau, Markus Armbruster, Stefan Hajnoczi,
Dr . David Alan Gilbert
On 03/26/2018 01:39 AM, Peter Xu wrote:
> It is abstracted from qtest_init_without_qmp_handshake(). It works just
> like qtest_init_without_qmp_handshake() but further it would allow the
> caller to specify the QMP parameter.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> tests/libqtest.c | 14 +++++++++++---
> tests/libqtest.h | 14 ++++++++++++++
> 2 files changed, 25 insertions(+), 3 deletions(-)
>
> + gchar *qmp_params;
> const char *qemu_binary = qtest_qemu_binary();
>
> s = g_new(QTestState, 1);
>
> socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
> qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
> + qmp_params = g_strdup_printf(qmp_format, qmp_socket_path);
>
Memory leak of qmp_params. Avoided in my rewrite to use bool instead of
a format string.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests)
2018-03-26 6:38 [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests) Peter Xu
` (8 preceding siblings ...)
2018-03-26 10:18 ` [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests) Christian Borntraeger
@ 2018-03-26 22:29 ` Eric Blake
9 siblings, 0 replies; 29+ messages in thread
From: Eric Blake @ 2018-03-26 22:29 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Marc-André Lureau, Markus Armbruster, Stefan Hajnoczi,
Dr . David Alan Gilbert
On 03/26/2018 01:38 AM, Peter Xu wrote:
> I suppose these are all good even for 2.12, so marked in subject.
> Tested with "make check" for all targets on x86_64, and iotest -raw.
>
> Patch 1 fixes one OOB error message regression reported by Marc-Andre.
>
> Patch 2 fixes one potential OOB problem when more than one clients are
> there, reported by Marc-Andre (too).
>
> Patch 3 introduce "-mon x-oob=on" parameter to allow user to
> explicitly enable Out-Of-Band for a specific monitor.
>
> Patch 4-6 are qapi-schema fixes and tests for Out-Of-Band.
>
> Patch 7-8 add back the OOB test on the new parameter (with more
> enhancements).
>
> Please review, thanks.
>
> Peter Xu (8):
> qmp: fix qmp_capabilities error regression
> qmp: cleanup qmp queues properly
> monitor: new parameter "x-oob"
> qapi: restrict allow-oob value to be "true"
> tests: let qapi-schema tests detect oob
> tests: add oob-test for qapi-schema
> tests: introduce qtest_init_with_qmp_format()
> tests: qmp-test: add test for new "x-oob"
Queuing 1, 4-6 without further changes. I'll repost a v2 of 2, 3, my
rewrite of 7, and 8.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests)
2018-03-26 10:18 ` [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests) Christian Borntraeger
@ 2018-03-27 2:26 ` Peter Xu
0 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-03-27 2:26 UTC (permalink / raw)
To: Christian Borntraeger
Cc: qemu-devel, Markus Armbruster, Dr . David Alan Gilbert,
Stefan Hajnoczi, Marc-André Lureau
On Mon, Mar 26, 2018 at 12:18:28PM +0200, Christian Borntraeger wrote:
> Thanks for the quick fixing. This series on top of master
> works fine on s390. (make check and iotests)
Appreciate the quick follow up. Thanks Christian.
--
Peter Xu
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly
2018-03-26 19:42 ` Eric Blake
@ 2018-03-27 2:30 ` Peter Xu
0 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-03-27 2:30 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, Marc-André Lureau, Markus Armbruster,
Stefan Hajnoczi, Dr . David Alan Gilbert
On Mon, Mar 26, 2018 at 02:42:23PM -0500, Eric Blake wrote:
> On 03/26/2018 01:38 AM, Peter Xu wrote:
> > 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:
> >
> > - before a session established
>
> Why here? [1]
Yes it can be omitted. We can do it either here or closing, the only
difference should be that when added here there's more possibility
that the pending commands (requests from disconnected clients) be
executed rather than dropped.
Here I did the cleanup on both places. Drop any of them would be fine
too.
>
> > - 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.
> >
>
> It would also be helpful to mention that the patch includes code motion to
> declare struct QMPRequest earlier in the file.
>
> > Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > monitor.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++----------------
> > 1 file changed, 59 insertions(+), 20 deletions(-)
> >
>
> > +static void qmp_request_free(QMPRequest *req)
> > +{
> > + qobject_decref(req->id);
> > + qobject_decref(req->req);
> > + g_free(req);
> > +}
> > +
> > +static void qmp_response_free(QObject *obj)
> > +{
> > + qobject_decref(obj);
> > +}
>
> Why do we need this function? Unless you plan to add to it in later
> patches, I'd rather just inline things and directly call qobject_decref() at
> the callsites...
Yes we can omit that.
>
> > +
> > +/* 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)) {
> > + qmp_response_free(g_queue_pop_head(mon->qmp.qmp_responses));
>
> ...here,
>
> > + }
> > +}
> > +
> > +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,
> > @@ -497,7 +550,7 @@ static void monitor_qmp_bh_responder(void *opaque)
> > break;
> > }
> > monitor_json_emitter_raw(response.mon, response.data);
> > - qobject_decref(response.data);
> > + qmp_response_free(response.data);
>
> and here.
>
> > @@ -4327,6 +4364,7 @@ static void monitor_qmp_event(void *opaque, int event)
> > switch (event) {
> > case CHR_EVENT_OPENED:
> > + monitor_qmp_cleanup_queues(mon);
>
> [1] How would something be queued to need cleanup at this point, if we
> already start with a clean queue before the first monitor, and if all
> monitor close actions clean the queue?
(answered above)
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2018-03-27 2:31 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-26 6:38 [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests) Peter Xu
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 1/8] qmp: fix qmp_capabilities error regression Peter Xu
2018-03-26 8:40 ` Marc-André Lureau
2018-03-26 19:35 ` Eric Blake
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly Peter Xu
2018-03-26 8:44 ` Marc-André Lureau
2018-03-26 19:42 ` Eric Blake
2018-03-27 2:30 ` Peter Xu
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob" Peter Xu
2018-03-26 9:10 ` Marc-André Lureau
2018-03-26 9:13 ` Peter Xu
2018-03-26 20:01 ` Eric Blake
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 4/8] qapi: restrict allow-oob value to be "true" Peter Xu
2018-03-26 9:11 ` Marc-André Lureau
2018-03-26 19:43 ` Eric Blake
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 5/8] tests: let qapi-schema tests detect oob Peter Xu
2018-03-26 9:13 ` Marc-André Lureau
2018-03-26 6:38 ` [Qemu-devel] [PATCH for-2.12 6/8] tests: add oob-test for qapi-schema Peter Xu
2018-03-26 9:18 ` Marc-André Lureau
2018-03-26 20:26 ` Eric Blake
2018-03-26 6:39 ` [Qemu-devel] [PATCH for-2.12 7/8] tests: introduce qtest_init_with_qmp_format() Peter Xu
2018-03-26 9:18 ` Marc-André Lureau
2018-03-26 20:42 ` Eric Blake
2018-03-26 21:48 ` Eric Blake
2018-03-26 6:39 ` [Qemu-devel] [PATCH for-2.12 8/8] tests: qmp-test: add test for new "x-oob" Peter Xu
2018-03-26 20:54 ` Eric Blake
2018-03-26 10:18 ` [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests) Christian Borntraeger
2018-03-27 2:26 ` Peter Xu
2018-03-26 22:29 ` Eric Blake
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.