Re: [Qemu-devel] [PATCH 07/32] qmp: Make "id" optional again even in "oob" monitors

[Eric Blake]

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.

>   
> -
>   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