Re: [Qemu-devel] [PATCH v2 01/12] qapi-schema: use generated marshaller for 'qmp_capabilities'

[Markus Armbruster <armbru@redhat.com>]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> qapi'fy the 'qmp_capabilities' command.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  monitor.c        |  4 ++--
>  qapi-schema.json | 21 +++++++++++++++++++++
>  qmp-commands.hx  |  2 +-
>  3 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 5d68a5d..cb1a0c7 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -617,7 +617,7 @@ static void monitor_qapi_event_init(void)
>      qmp_event_set_func_emit(monitor_qapi_event_queue);
>  }
>  
> -static void qmp_capabilities(QDict *params, QObject **ret_data, Error **errp)
> +void qmp_qmp_capabilities(Error **errp)
>  {
>      cur_mon->qmp.in_command_mode = true;
>  }
> @@ -3649,7 +3649,7 @@ static int monitor_can_read(void *opaque)
>  static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd,
>                               Error **errp)
>  {
> -    bool is_cap = cmd->mhandler.cmd_new == qmp_capabilities;
> +    bool is_cap = cmd->mhandler.cmd_new == qmp_marshal_qmp_capabilities;
>  
>      if (is_cap && mon->qmp.in_command_mode) {
>          error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5658723..03c15d8 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -20,6 +20,27 @@
>  # QAPI introspection
>  { 'include': 'qapi/introspect.json' }
>  
> +##
> +# @qmp_capabilities:
> +#
> +# Enable QMP capabilities.
> +#
> +# Arguments: None.
> +#
> +# Example:
> +#
> +# -> { "execute": "qmp_capabilities" }
> +# <- { "return": {} }
> +#
> +# Notes: This command is valid exactly when first connecting: it must be
> +# issued before any other command will be accepted, and will fail once the
> +# monitor is accepting other commands.

Pointer to docs/qmp-spec.txt wouldn't hurt.

> +#
> +# Since: 0.13
> +#
> +##
> +{ 'command': 'qmp_capabilities' }
> +
>  ##
>  # @LostTickPolicy:
>  #
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index c8d360a..13707ac 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2209,7 +2209,7 @@ EQMP
>          .args_type  = "",
>          .params     = "",
>          .help       = "enable QMP capabilities",
> -        .mhandler.cmd_new = qmp_capabilities,
> +        .mhandler.cmd_new = qmp_marshal_qmp_capabilities,
>      },
>  
>  SQMP

According to docs/qmp-spec.txt section 4, the command is for negotiating
capabilities:

    Clients should use the qmp_capabilities command to enable
    capabilities advertised in the Server's greeting (section '2.2
    Server Greeting') they support.

It neglects to specify how that might be done, though.

Section 2.2 specifies the greeting's member "capabilities" as
json-array, without saying anything about its members.

Section 2.2.1 seems to imply the members are strings, which makes some
sense.

To actually support capabilities negotiation, the command needs an
argument matching the greeting's capabilities array.  The argument must
be optional, defaulting to the empty array.  No need to add that now.

Clarifying the docs would be nice, but it's not this patch's job.

Reviewed-by: Markus Armbruster <armbru@redhat.com>