[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v6 21/27] qmp: support out-of-band (oob) execution
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [RFC v6 21/27] qmp: support out-of-band (oob) execution |
Date: |
Fri, 12 Jan 2018 14:23:51 +0800 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
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