Re: [Qemu-devel] [RFC v6 21/27] qmp: support out-of-band (oob) execution
On Fri, Jan 12, 2018 at 02:23:51PM +0800, Peter Xu wrote: > Good to know the "?:" syntax... Thanks, It's a gcc extension and not part of the C standard: https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html#Conditionals However, QEMU already uses it in several places so we can take advantage of it. Stefan signature.asc Description: PGP signature
Re: [Qemu-devel] [RFC v6 21/27] qmp: support out-of-band (oob) execution
On Tue, Jan 09, 2018 at 02:08:10PM +, Stefan Hajnoczi wrote:
> On Tue, Dec 19, 2017 at 04:45:51PM +0800, Peter Xu wrote:
> > diff --git a/monitor.c b/monitor.c
> > index b571866659..505db439d8 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -1090,6 +1090,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 handshake");
>
> The qmp-spec.txt document calls it "capabilities negotiation". I don't
> see "handshake" mentioned. Please use the terminology from the QMP spec
> so it's clear what this error message means.
>
> > +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)
> > {
> > @@ -4032,6 +4070,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));
>
> Not all tracers on all platforms support NULL string pointers. Please
> use 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);
> > @@ -4055,17 +4094,31 @@ static void handle_qmp_command(JSONMessageParser
> > *parser, GQueue *tokens)
> > error_setg(, 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, );
> > +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, )) {
> > +goto err;
> > +}
> > +
> > +id = qdict_get(qdict, "id");
> > +
> > +/* When OOB is enabled, the "id" field is mandatory. */
> > +if (qmp_oob_enabled(mon) && !id) {
> > +error_setg(, "Out-Of-Band capability requires that "
> > + "every command contains an 'id' field.");
>
> error_setg() messages shouldn't end with a period ('.').
>
> > +goto err;
> > +}
> > +
> > +qobject_incref(id);
> > +qdict_del(qdict, "id");
> >
> > req_obj = g_new0(QMPRequest, 1);
> > req_obj->mon = mon;
> > @@ -4073,6 +4126,13 @@ 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));
>
> Same NULL "%s" issue here.
Fixing them all.
Good to know the "?:" syntax... Thanks,
--
Peter Xu
Re: [Qemu-devel] [RFC v6 21/27] qmp: support out-of-band (oob) execution
On Tue, Dec 19, 2017 at 04:45:51PM +0800, Peter Xu wrote:
> diff --git a/monitor.c b/monitor.c
> index b571866659..505db439d8 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1090,6 +1090,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 handshake");
The qmp-spec.txt document calls it "capabilities negotiation". I don't
see "handshake" mentioned. Please use the terminology from the QMP spec
so it's clear what this error message means.
> +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)
> {
> @@ -4032,6 +4070,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));
Not all tracers on all platforms support NULL string pointers. Please
use 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);
> @@ -4055,17 +4094,31 @@ static void handle_qmp_command(JSONMessageParser
> *parser, GQueue *tokens)
> error_setg(, 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, );
> +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, )) {
> +goto err;
> +}
> +
> +id = qdict_get(qdict, "id");
> +
> +/* When OOB is enabled, the "id" field is mandatory. */
> +if (qmp_oob_enabled(mon) && !id) {
> +error_setg(, "Out-Of-Band capability requires that "
> + "every command contains an 'id' field.");
error_setg() messages shouldn't end with a period ('.').
> +goto err;
> +}
> +
> +qobject_incref(id);
> +qdict_del(qdict, "id");
>
> req_obj = g_new0(QMPRequest, 1);
> req_obj->mon = mon;
> @@ -4073,6 +4126,13 @@ 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));
Same NULL "%s" issue here.
signature.asc
Description: PGP signature
Re: [Qemu-devel] [RFC v6 21/27] qmp: support out-of-band (oob) execution
On Tue, 12/19 16:45, 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.
>
> Signed-off-by: Peter Xu
Reviewed-by: Fam Zheng
[Qemu-devel] [RFC v6 21/27] qmp: support out-of-band (oob) execution
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.
Signed-off-by: Peter Xu
---
include/qapi/qmp/dispatch.h | 1 +
monitor.c | 83 -
qapi/qmp-dispatch.c | 28 +++
trace-events| 2 ++
4 files changed, 105 insertions(+), 9 deletions(-)
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index cf2657b5c9..d8d913b338 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -50,6 +50,7 @@ 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 b571866659..505db439d8 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1090,6 +1090,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 handshake");
+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)
{
@@ -4032,6 +4070,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);
@@ -4055,17 +4094,31 @@ static void handle_qmp_command(JSONMessageParser
*parser, GQueue *tokens)
error_setg(, 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, );
+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, )) {
+goto err;
+}
+
+id = qdict_get(qdict, "id");
+
+/* When OOB is enabled, the "id" field is mandatory. */
+if (qmp_oob_enabled(mon) && !id) {
+error_setg(, "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;
@@ -4073,6 +4126,13 @@ 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));
+monitor_qmp_dispatch_one(req_obj);
+return;
+}
+
/* Protect qmp_requests and fetching its length. */
qemu_mutex_lock(>qmp.qmp_queue_lock);
@@ -4108,6 +4168,11 @@ static void handle_qmp_command(JSONMessageParser
*parser, GQueue *tokens)
/* Kick
