[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 07/32] qmp: Make "id" optional again even in "oo
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 07/32] qmp: Make "id" optional again even in "oob" monitors |
Date: |
Tue, 03 Jul 2018 08:06:47 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Eric Blake <address@hidden> writes:
> 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 <address@hidden>
>> ---
>> 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.
Yes. I'll take care of it in v2.
>
>> -
>> 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