Re: [Qemu-devel] [PATCH v7 18/23] qmp: support out-of-band (oob) execution

2018-02-21 Thread Stefan Hajnoczi
On Wed, Jan 24, 2018 at 01:39:52PM +0800, Peter Xu wrote:
> Having "allow-oob" to true for a command does not mean that this command
> will always be run in out-of-band mode.  The out-of-band quick path will
> only be executed if we specify the extra "run-oob" flag when sending the
> QMP request:
> 
> { "execute":   "command-that-allows-oob",
>   "arguments": { ... },
>   "control":   { "run-oob": true } }
> 
> The "control" key is introduced to store this extra flag.  "control"
> field is used to store arguments that are shared by all the commands,
> rather than command specific arguments.  Let "run-oob" be the first.
> 
> Note that in the patch I exported qmp_dispatch_check_obj() to be used to
> check the request earlier, and at the same time allowed "id" field to be
> there since actually we always allow that.
> 
> Signed-off-by: Peter Xu 
> ---
>  include/qapi/qmp/dispatch.h |  2 ++
>  monitor.c   | 84 
> -
>  qapi/qmp-dispatch.c | 32 -
>  trace-events|  2 ++
>  4 files changed, 110 insertions(+), 10 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature


[Qemu-devel] [PATCH v7 18/23] qmp: support out-of-band (oob) execution

2018-01-23 Thread Peter Xu
Having "allow-oob" to true for a command does not mean that this command
will always be run in out-of-band mode.  The out-of-band quick path will
only be executed if we specify the extra "run-oob" flag when sending the
QMP request:

{ "execute":   "command-that-allows-oob",
  "arguments": { ... },
  "control":   { "run-oob": true } }

The "control" key is introduced to store this extra flag.  "control"
field is used to store arguments that are shared by all the commands,
rather than command specific arguments.  Let "run-oob" be the first.

Note that in the patch I exported qmp_dispatch_check_obj() to be used to
check the request earlier, and at the same time allowed "id" field to be
there since actually we always allow that.

Signed-off-by: Peter Xu 
---
 include/qapi/qmp/dispatch.h |  2 ++
 monitor.c   | 84 -
 qapi/qmp-dispatch.c | 32 -
 trace-events|  2 ++
 4 files changed, 110 insertions(+), 10 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index b76798800c..d8d913b338 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -49,6 +49,8 @@ bool qmp_command_is_enabled(const QmpCommand *cmd);
 const char *qmp_command_name(const QmpCommand *cmd);
 bool qmp_has_success_response(const QmpCommand *cmd);
 QObject *qmp_build_error_object(Error *err);
+QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp);
+bool qmp_is_oob(QDict *dict);
 
 typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
 
diff --git a/monitor.c b/monitor.c
index bf0f3d1662..d31da3fd1b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1106,6 +1106,44 @@ static void qmp_caps_apply(Monitor *mon, 
QMPCapabilityList *list)
 }
 }
 
+/*
+ * Return true if check successful, or false otherwise.  When false is
+ * returned, detailed error will be in errp if provided.
+ */
+static bool qmp_cmd_oob_check(Monitor *mon, QDict *req, Error **errp)
+{
+const char *command;
+QmpCommand *cmd;
+
+command = qdict_get_try_str(req, "execute");
+if (!command) {
+error_setg(errp, "Command field 'execute' missing");
+return false;
+}
+
+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);
+return false;
+}
+
+if (qmp_is_oob(req)) {
+if (!qmp_oob_enabled(mon)) {
+error_setg(errp, "Please enable Out-Of-Band first "
+   "for the session during capabilities negociation");
+return false;
+}
+if (!(cmd->options & QCO_ALLOW_OOB)) {
+error_setg(errp, "The command %s does not support OOB",
+   command);
+return false;
+}
+}
+
+return true;
+}
+
 void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
   Error **errp)
 {
@@ -4049,6 +4087,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
 QMPRequest *req_obj = monitor_qmp_requests_pop_one();
 
 if (req_obj) {
+trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
 monitor_qmp_dispatch_one(req_obj);
 /* Reschedule instead of looping so the main loop stays responsive */
 qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
@@ -4072,17 +4111,31 @@ static void handle_qmp_command(JSONMessageParser 
*parser, GQueue *tokens)
 error_setg(&err, QERR_JSON_PARSING);
 }
 if (err) {
-monitor_qmp_respond(mon, NULL, err, NULL);
-qobject_decref(req);
-return;
+goto err;
+}
+
+/* Check against the request in general layout */
+qdict = qmp_dispatch_check_obj(req, &err);
+if (!qdict) {
+goto err;
 }
 
-qdict = qobject_to_qdict(req);
-if (qdict) {
-id = qdict_get(qdict, "id");
-qobject_incref(id);
-qdict_del(qdict, "id");
-} /* else will fail qmp_dispatch() */
+/* Check against OOB specific */
+if (!qmp_cmd_oob_check(mon, qdict, &err)) {
+goto err;
+}
+
+id = qdict_get(qdict, "id");
+
+/* When OOB is enabled, the "id" field is mandatory. */
+if (qmp_oob_enabled(mon) && !id) {
+error_setg(&err, "Out-Of-Band capability requires that "
+   "every command contains an 'id' field");
+goto err;
+}
+
+qobject_incref(id);
+qdict_del(qdict, "id");
 
 req_obj = g_new0(QMPRequest, 1);
 req_obj->mon = mon;
@@ -4090,6 +4143,14 @@ static void handle_qmp_command(JSONMessageParser 
*parser, GQueue *tokens)
 req_obj->req = req;
 req_obj->need_resume = false;
 
+if (qmp_is_oob(qdict)) {
+/* Out-Of-Band (OOB) requests are executed directly in parser. */
+trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id)
+