Re: [Qemu-devel] [PATCH 05/12] monitor: register the qapi generated commands

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3467 bytes --]

On 06/22/2016 06:08 PM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Stop using the so-called 'middle' mode. Instead, use qmp_find_command()
> from generated qapi commands registry.
> 
> Note: this commit requires a 'make clean' prior to make, since the
> generated files do not depend on Makefile (due to a cyclic rule
> introduced in 4115852bb0).

I'm not as well-versed as Paolo on Makefile issues, so I won't comment
on that part of the patch.

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  Makefile        |   2 +-
>  monitor.c       |  12 +++--
>  qmp-commands.hx | 143 --------------------------------------------------------
>  vl.c            |   1 +
>  4 files changed, 11 insertions(+), 147 deletions(-)
> 

> +++ b/monitor.c
> @@ -3884,6 +3884,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>      QObject *obj, *data;
>      QDict *input, *args;
>      const mon_cmd_t *cmd;
> +    QmpCommand *qcmd;
>      const char *cmd_name;
>      Monitor *mon = cur_mon;
>  
> @@ -3909,7 +3910,8 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>      cmd_name = qdict_get_str(input, "execute");
>      trace_handle_qmp_command(mon, cmd_name);
>      cmd = qmp_find_cmd(cmd_name);
> -    if (!cmd) {
> +    qcmd = qmp_find_command(cmd_name);

Is it worth creating a single type that contains both mon_cmd_t and
QmpCommand information, so that we don't need two similarly-named
functions to look up two related pieces of information?  Not necessarily
in this patch.

> +    if (!qcmd || !cmd) {
>          error_set(&local_err, ERROR_CLASS_COMMAND_NOT_FOUND,
>                    "The command %s has not been found", cmd_name);
>          goto err_out;
> @@ -3931,7 +3933,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>          goto err_out;
>      }
>  
> -    cmd->mhandler.cmd_new(args, &data, &local_err);
> +    qcmd->fn(args, &data, &local_err);
>  
>  err_out:
>      monitor_protocol_emitter(mon, data, local_err);
> @@ -4000,9 +4002,13 @@ void monitor_resume(Monitor *mon)
>  
>  static QObject *get_qmp_greeting(void)
>  {
> +    QmpCommand *cmd;
>      QObject *ver = NULL;
>  
> -    qmp_marshal_query_version(NULL, &ver, NULL);
> +    cmd = qmp_find_command("query-version");
> +    assert(cmd && cmd->fn);
> +    cmd->fn(NULL, &ver, NULL);
> +
>      return qobject_from_jsonf("{'QMP':{'version': %p,'capabilities': []}}",ver);

Worth fixing the missing space after ',' while touching near here?

>  }
>  
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ee88e48..95c1e7d 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -63,7 +63,6 @@ EQMP
>      {
>          .name       = "quit",
>          .args_type  = "",
> -        .mhandler.cmd_new = qmp_marshal_quit,

At one point, I posted an RFC patch for removing .args_type on most QMP
command listings in this file. Maybe you still do that later in your
series, but as my work definitely conflicts with yours, and your series
is older, I don't mind getting through your series first.

Overall, I like the general idea of automating things rather than having
to duplicate information in qmp-commands.hx.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]