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

[Markus Armbruster <armbru@redhat.com>]

marcandre.lureau@redhat.com writes:

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

We generally say "commit 4115852bb0".

Sounds like we had a cyclic dependency.  Do you mean "they don't depend
on Makefile, because that would be a cyclic dependency"?

Paolo, any smart ideas on how to avoid "requires a 'make clean'"?

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c       |  15 ++++--
>  Makefile        |   2 +-
>  qmp-commands.hx | 143 --------------------------------------------------------
>  vl.c            |   1 +
>  4 files changed, 13 insertions(+), 148 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 3a28b43..5bbe4bb 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3934,6 +3934,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;
>  
> @@ -3959,7 +3960,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);
> +    if (!qcmd || !cmd) {

Looks awkward, but it's temporary.  Makes sense.

>          error_set(&local_err, ERROR_CLASS_COMMAND_NOT_FOUND,
>                    "The command %s has not been found", cmd_name);
>          goto err_out;
> @@ -3981,7 +3983,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);
> @@ -4050,10 +4052,15 @@ void monitor_resume(Monitor *mon)
>  
>  static QObject *get_qmp_greeting(void)
>  {
> +    QmpCommand *cmd;
>      QObject *ver = NULL;
>  
> -    qmp_marshal_query_version(NULL, &ver, NULL);
> -    return qobject_from_jsonf("{'QMP':{'version': %p,'capabilities': []}}",ver);
> +    cmd = qmp_find_command("query-version");
> +    assert(cmd && cmd->fn);
> +    cmd->fn(NULL, &ver, NULL);
> +
> +    return qobject_from_jsonf("{'QMP':{'version': %p, 'capabilities': []}}",
> +                              ver);

Meh.

The generator makes the generated qmp_marshal_FOOs() static unless
middle mode.  Middle mode is going away (good riddance).  But replacing
the linker's work by qmp_find_command() just so we can keep the
qmp_marshal_FOOs() static isn't an improvement.  Especially since it
adds another run-time failure mode.  Let's change the generator instead.

>  }
>  
>  static void monitor_qmp_event(void *opaque, int event)
> diff --git a/Makefile b/Makefile
> index 0d7647f..fcdc192 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -311,7 +311,7 @@ $(qapi-modules) $(SRC_PATH)/scripts/qapi-event.py $(qapi-py)
>  qmp-commands.h qmp-marshal.c :\
>  $(qapi-modules) $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
>  	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py \
> -		$(gen-out-type) -o "." -m $<, \
> +		$(gen-out-type) -o "." $<, \
>  		"  GEN   $@")
>  qmp-introspect.h qmp-introspect.c :\
>  $(qapi-modules) $(SRC_PATH)/scripts/qapi-introspect.py $(qapi-py)
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 13707ac..1ad8dda 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -63,7 +63,6 @@ EQMP
>      {
>          .name       = "quit",
>          .args_type  = "",
> -        .mhandler.cmd_new = qmp_marshal_quit,
>      },
>  
>  SQMP
[More of the same snipped...]
> diff --git a/vl.c b/vl.c
> index a455947..a819e05 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2971,6 +2971,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_init_exec_dir(argv[0]);
>  
>      module_call_init(MODULE_INIT_QOM);
> +    module_call_init(MODULE_INIT_QAPI);
>  
>      qemu_add_opts(&qemu_drive_opts);
>      qemu_add_drive_opts(&qemu_legacy_drive_opts);

So the code added by PATCH 03 doesn't actually run without this, right?
Okay with me, but let's mention it in the commit message of PATCH 03.