qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH v2 05/12] monitor: register the qapi generated commands
Date: Mon, 8 Aug 2016 12:10:08 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0


On 05/08/2016 14:42, Markus Armbruster wrote:
> address@hidden writes:
> 
>> From: Marc-André Lureau <address@hidden>
>>
>> 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 <address@hidden>
>> ---
>>  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.
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]