From patchwork Thu Mar 22 10:22:14 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Marc-Andr=C3=A9_Lureau?= X-Patchwork-Id: 10301161 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id A425E60385 for ; Thu, 22 Mar 2018 10:23:04 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 94C7B287CF for ; Thu, 22 Mar 2018 10:23:04 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 88FD229AA1; Thu, 22 Mar 2018 10:23:04 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id AAE66287CF for ; Thu, 22 Mar 2018 10:23:02 +0000 (UTC) Received: from localhost ([::1]:59824 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eyxN7-0005lG-Up for patchwork-qemu-devel@patchwork.kernel.org; Thu, 22 Mar 2018 06:23:01 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46961) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eyxMQ-0005SF-Ov for qemu-devel@nongnu.org; Thu, 22 Mar 2018 06:22:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eyxMP-0002Cp-18 for qemu-devel@nongnu.org; Thu, 22 Mar 2018 06:22:18 -0400 Received: from mail-wr0-x233.google.com ([2a00:1450:400c:c0c::233]:45283) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1eyxMO-0002CP-MT for qemu-devel@nongnu.org; Thu, 22 Mar 2018 06:22:16 -0400 Received: by mail-wr0-x233.google.com with SMTP id h2so8099133wre.12 for ; Thu, 22 Mar 2018 03:22:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=jfQ3ihCZZWfCc13Af6okEfXDAFEZmuWV2etwN5BdOUA=; b=djZ6Y+yXwLtKMU4sHgpDjEJ4MHxReQbY7hRnosdj+dTKWeIRru7ACn2J1LgxaJLyj6 64a8ag8GjfZsHPTPKCUHD6wcX/lSGy4UiHOYx4gxjlX+pDEvgx/OhZkljWuRMyVP8Zt/ zLaa9dsiyXwSeecsNy8KEe24kd6/KaCcH6x+t+gQh+0kXEImIPWmccofbYzdFeFZWRp2 +tOes9qgKzkLZ47GAoil2zSOvulU49+M7xV87NGv0Pc9feULIk10Tb1u8Y98jK6QEp4u oZrXvAEUkCRc8p8Od2mJaQsbZbSn4yrRtNN+ZUWsbiXErlQ621HjrB9aETntB/iqlQJA blNg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=jfQ3ihCZZWfCc13Af6okEfXDAFEZmuWV2etwN5BdOUA=; b=QvHc/V0qAKABII6qvSgjfszuVnbvbfrZ1WK2E4a9p++BX7HYfAusmJxUi4p2Vig26Q 2VFG7m2M0NQ4zugJahhtNrGMfTG/pfu8NDE5UBFk4PQgeO0MJSIPLVa5oxSYThFmWik+ fW33WksdBFGYLdfqbWE+P+n0gzsLGJP7/gV6SH2HmwrGfjVcxLoyxrlww0tl/uELgp1N C37gcjR9PJHiomBfL3EQTnzn3wogUtjCPKooJ3a7S3Nk6Sccdl9k8/T06+LKvpowmMNx J80WeTWG67fhlcnZSJJsib8ygk9i62CoVEU6U4H+j94hXTM6evSN8wqq55JfroJ3aHQN /nqA== X-Gm-Message-State: AElRT7GnkWeFg6/tTn7LPbm6wGkJhFCrVGkYaPnNVt04u91HyVxeLvPc Oc7hPtDH6nkIrymiYfSDER4/9LYSEAQ1hzQbadE= X-Google-Smtp-Source: AG47ELuMYoJoqY7rjL1d4F6jeNjafY7wWrOVv3nogArov9csZ5EGMJM5mBeNDAtqoNgfLun5Y53gX2wz/bvY2E9qRjE= X-Received: by 10.223.167.12 with SMTP id c12mr11736947wrd.194.1521714135547; Thu, 22 Mar 2018 03:22:15 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.128.226 with HTTP; Thu, 22 Mar 2018 03:22:14 -0700 (PDT) In-Reply-To: <20180309090006.10018-19-peterx@redhat.com> References: <20180309090006.10018-1-peterx@redhat.com> <20180309090006.10018-19-peterx@redhat.com> From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Thu, 22 Mar 2018 11:22:14 +0100 Message-ID: To: Peter Xu X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2a00:1450:400c:c0c::233 Subject: Re: [Qemu-devel] [PATCH v8 18/23] qmp: support out-of-band (oob) execution X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Laurent Vivier , Fam Zheng , Juan Quintela , Markus Armbruster , Michael Roth , QEMU , Stefan Hajnoczi , Paolo Bonzini , "Dr . David Alan Gilbert" Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP Hi On Fri, Mar 9, 2018 at 10:00 AM, 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. > > Reviewed-by: Stefan Hajnoczi > Signed-off-by: Peter Xu > --- > include/qapi/qmp/dispatch.h | 2 ++ > monitor.c | 84 ++++++++++++++++++++++++++++++++++++++++----- > qapi/qmp-dispatch.c | 33 +++++++++++++++++- > trace-events | 2 ++ > 4 files changed, 111 insertions(+), 10 deletions(-) > > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h > index 26eb13ff41..ffb4652f71 100644 > --- a/include/qapi/qmp/dispatch.h > +++ b/include/qapi/qmp/dispatch.h > @@ -48,6 +48,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 4d57a8d341..5c8afe9f50 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -1110,6 +1110,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) > { > @@ -4001,6 +4039,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); > @@ -4024,17 +4063,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; > + } This check happens before monitor_qmp_dispatch_one(), where there is now dead code for COMMAND_NOT_FOUND error handling (changing the error description), since it will error here. A solution is to move the error manipulation code to monitor_qmp_respond(): monitor_json_emitter(mon, rsp); } @@ -4022,7 +4034,6 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool oob_cmd) { Monitor *mon, *old_mon; QObject *req, *rsp = NULL, *id; - QDict *qdict = NULL; bool need_resume; req = req_obj->req; @@ -4045,18 +4056,6 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool oob_cmd) 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); > + > + 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; > @@ -4042,6 +4095,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) > + ?: ""); > + monitor_qmp_dispatch_one(req_obj); > + return; > + } > + > /* Protect qmp_requests and fetching its length. */ > qemu_mutex_lock(&mon->qmp.qmp_queue_lock); > > @@ -4078,6 +4139,11 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) > > /* Kick the dispatcher routine */ > qemu_bh_schedule(mon_global.qmp_dispatcher_bh); > + return; > + > +err: > + monitor_qmp_respond(mon, NULL, err, NULL); > + qobject_decref(req); > } > > static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size) > diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c > index e31ac4be1f..9a2e18c768 100644 > --- a/qapi/qmp-dispatch.c > +++ b/qapi/qmp-dispatch.c > @@ -17,8 +17,9 @@ > #include "qapi/qmp/json-parser.h" > #include "qapi/qmp/qdict.h" > #include "qapi/qmp/qjson.h" > +#include "qapi/qmp/qbool.h" > > -static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp) > +QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp) > { > const QDictEntry *ent; > const char *arg_name; > @@ -50,6 +51,14 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp) > "QMP input member 'arguments' must be an object"); > return NULL; > } > + } else if (!strcmp(arg_name, "id")) { > + continue; > + } else if (!strcmp(arg_name, "control")) { > + if (qobject_type(arg_obj) != QTYPE_QDICT) { > + error_setg(errp, > + "QMP input member 'control' must be a dict"); > + return NULL; > + } > } else { > error_setg(errp, "QMP input member '%s' is unexpected", > arg_name); > @@ -120,6 +129,28 @@ QObject *qmp_build_error_object(Error *err) > error_get_pretty(err)); > } > > +/* > + * Detect whether a request should be run out-of-band, by quickly > + * peeking at whether we have: { "control": { "run-oob": True } }. By > + * default commands are run in-band. > + */ > +bool qmp_is_oob(QDict *dict) > +{ > + QBool *bool_obj; > + > + dict = qdict_get_qdict(dict, "control"); > + if (!dict) { > + return false; > + } > + > + bool_obj = qobject_to_qbool(qdict_get(dict, "run-oob")); > + if (!bool_obj) { > + return false; > + } > + > + return qbool_get_bool(bool_obj); > +} > + > QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request) > { > Error *err = NULL; > diff --git a/trace-events b/trace-events > index fe10c3b487..bd036da278 100644 > --- a/trace-events > +++ b/trace-events > @@ -48,6 +48,8 @@ monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) "event= > handle_hmp_command(void *mon, const char *cmdline) "mon %p cmdline: %s" > handle_qmp_command(void *mon, const char *req) "mon %p req: %s" > monitor_suspend(void *ptr, int cnt) "mon %p: %d" > +monitor_qmp_cmd_in_band(const char *id) "%s" > +monitor_qmp_cmd_out_of_band(const char *id) "%s" > > # dma-helpers.c > dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d" > -- > 2.14.3 > > diff --git a/monitor.c b/monitor.c index 52b4c77ac3..df09a1657d 100644 --- a/monitor.c +++ b/monitor.c @@ -3997,6 +3997,18 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp, qdict_put_obj(qobject_to(QDict, rsp), "id", id); } + 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'"); + } + } +