From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53649) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YiPqx-0001Z7-Fl for qemu-devel@nongnu.org; Wed, 15 Apr 2015 12:07:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YiPqu-0003g4-Lh for qemu-devel@nongnu.org; Wed, 15 Apr 2015 12:07:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52142) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YiPqu-0003fp-FE for qemu-devel@nongnu.org; Wed, 15 Apr 2015 12:07:48 -0400 Date: Wed, 15 Apr 2015 18:07:43 +0200 From: Kashyap Chamarthy Message-ID: <20150415160743.GA11726@tesla.redhat.com> References: <1429111163-27870-1-git-send-email-eblake@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1429111163-27870-1-git-send-email-eblake@redhat.com> Subject: Re: [Qemu-devel] [PATCH] qmp: Give saner messages related to qmp_capabilities misuse List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: erik.rull@rdsoftware.de, qemu-devel@nongnu.org, paulo.vital@profitbricks.com, lcapitulino@redhat.com On Wed, Apr 15, 2015 at 09:19:23AM -0600, Eric Blake wrote: > Pretending that QMP doesn't understand a command merely because > we are not in the right mode doesn't help first-time users figure > out what to do to correct things. Although the documentation for > QMP calls out capabilities negotiation, we should also make it > clear in our error messages what we were expecting. With this > patch, I now get the following transcript: > > $ ./x86_64-softmmu/qemu-system-x86_64 -qmp stdio -nodefaults > {"QMP": {"version": {"qemu": {"micro": 93, "minor": 2, "major": 2}, "package": ""}, "capabilities": []}} > {"execute":"huh"} > {"error": {"class": "CommandNotFound", "desc": "The command huh has not been found"}} > {"execute":"quit"} > {"error": {"class": "CommandNotFound", "desc": "Expecting capabilities negotiation with 'qmp_capabilities' before command 'quit'"}} > {"execute":"qmp_capabilities"} > {"return": {}} > {"execute":"qmp_capabilities"} > {"error": {"class": "CommandNotFound", "desc": "Capabilities negotiation is already complete, command 'qmp_capabilities' ignored"}} > {"execute":"quit"} > {"return": {}} > {"timestamp": {"seconds": 1429110729, "microseconds": 181935}, "event": "SHUTDOWN"} Thanks for this (much needed) nice improvement! Long time ago when I began testing directly w/ QMP, and was wondering _why_ the commands weren't taking effect despite correct symtax, Marku Armbruster on #qemu helpfully mentioned I _must_: 'execute': 'qmp_capabilities' before invoking any other QMP commands. Tested this locally, w/ similar test as yours: $ ./x86_64-softmmu/qemu-system-x86_64 -qmp stdio -nodefaults {"QMP": {"version": {"qemu": {"micro": 93, "minor": 2, "major": 2}, "package": ""}, "capabilities": []}} { "execute": "query-version" } {"error": {"class": "CommandNotFound", "desc": "Expecting capabilities negotiation with 'qmp_capabilities' before command 'query-version'"}} {"execute":"qmp_capabilities"} {"return": {}} { "execute": "query-version" } {"return": {"qemu": {"micro": 93, "minor": 2, "major": 2}, "package": ""}} So, FWIW: Tested-By: Kashyap Chamarthy > Signed-off-by: Eric Blake > --- > > Based in part on this thread: > https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00831.html > > monitor.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/monitor.c b/monitor.c > index 68873ec..9f37700 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -4783,10 +4783,22 @@ static int monitor_can_read(void *opaque) > return (mon->suspend_cnt == 0) ? 1 : 0; > } > > -static int invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd) > +static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd) > { > - int is_cap = cmd->mhandler.cmd_new == do_qmp_capabilities; > - return (qmp_cmd_mode(mon) ? is_cap : !is_cap); > + bool is_cap = cmd->mhandler.cmd_new == do_qmp_capabilities; > + if (is_cap && qmp_cmd_mode(mon)) { > + qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND, > + "Capabilities negotiation is already complete, command " > + "'%s' ignored", cmd->name); > + return true; > + } > + if (!is_cap && !qmp_cmd_mode(mon)) { > + qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND, > + "Expecting capabilities negotiation with " > + "'qmp_capabilities' before command '%s'", cmd->name); > + return true; > + } > + return false; > } > > /* > @@ -5080,11 +5092,14 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) > cmd_name = qdict_get_str(input, "execute"); > trace_handle_qmp_command(mon, cmd_name); > cmd = qmp_find_cmd(cmd_name); > - if (!cmd || invalid_qmp_mode(mon, cmd)) { > + if (!cmd) { > qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND, > "The command %s has not been found", cmd_name); > goto err_out; > } > + if (invalid_qmp_mode(mon, cmd)) { > + goto err_out; > + } > > obj = qdict_get(input, "arguments"); > if (!obj) { > -- > 2.1.0 > > -- /kashyap