From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59930) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fa68A-0003aj-U7 for qemu-devel@nongnu.org; Mon, 02 Jul 2018 17:13:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fa686-0006We-VB for qemu-devel@nongnu.org; Mon, 02 Jul 2018 17:13:06 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:57982 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fa686-0006WJ-QI for qemu-devel@nongnu.org; Mon, 02 Jul 2018 17:13:02 -0400 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7C14334669 for ; Mon, 2 Jul 2018 21:13:02 +0000 (UTC) References: <20180702162218.13678-1-armbru@redhat.com> <20180702162218.13678-8-armbru@redhat.com> From: Eric Blake Message-ID: Date: Mon, 2 Jul 2018 16:13:01 -0500 MIME-Version: 1.0 In-Reply-To: <20180702162218.13678-8-armbru@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 07/32] qmp: Make "id" optional again even in "oob" monitors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: peterx@redhat.com, stefanha@redhat.com, dgilbert@redhat.com On 07/02/2018 11:21 AM, Markus Armbruster wrote: > Commit cf869d53172 "qmp: support out-of-band (oob) execution" made > "id" mandatory for all commands when the client accepted capability > "oob". This is rather onerous when you play with QMP by hand, and > unnecessarily so: only out-of-band commands need an ID for reliable > matching of response to command. > > Revert that part of commit cf869d53172 for now. We may still make > "id" mandatory for out-of-band commands. Fair enough for now (I still think mandatory "id" for oob commands is worthwhile, but I also think "exec-oob" is worthwhile, and it's probably easier to write the logic for mandating "id" after that tweak). > > Signed-off-by: Markus Armbruster > --- > docs/interop/qmp-spec.txt | 9 +++------ > monitor.c | 7 ------- > 2 files changed, 3 insertions(+), 13 deletions(-) > > +++ b/docs/interop/qmp-spec.txt > @@ -103,16 +103,13 @@ The format for command execution is: > required. Each command documents what contents will be considered > valid when handling the json-argument > - The "id" member is a transaction identification associated with the > - command execution. It is required for all commands if the OOB - > - capability was enabled at startup, and optional otherwise. The same > - "id" field will be part of the response if provided. The "id" > - member can be any json-value. A json-number incremented for each > - successive command works fine. > + command execution, it is optional and will be part of the response > + if provided. The "id" member can be any json-value. A json-number > + incremented for each successive command works fine. > - The optional "control" member further specifies how the command is > to be executed. Currently, its only member is optional "run-oob". > See section "2.3.1 Out-of-band execution" for details. We should STILL recommend that a client should use "id" when using oob, whether or not we mandate it later. > > - > 2.3.1 Out-of-band execution > --------------------------- > > diff --git a/monitor.c b/monitor.c > index 96e87d8664..b7d74b01b4 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -4291,13 +4291,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) > > 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; > - } I can live with this hunk right away, but the documentation feels weak enough that I'm reluctant to give R-b to the patch as-is -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org