On 05/08/2016 14:42, Markus Armbruster wrote:
> marcandre.lur...@redhat.com writes:
>
>> From: Marc-André Lureau <marcandre.lur...@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'"?
Nope, sorry. :( But if you squash the generator patch that makes
qmp_marshal_* non-static, it should work.
The idea in that commit comes from autotools. I'm not sure how they
deal with it.
Paolo
>> Signed-off-by: Marc-André Lureau <marcandre.lur...@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.
>
