On Tue, Jan 09, 2018 at 02:08:10PM +0000, 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(&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."); > > 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