Re: [Qemu-devel] [PATCH v4 05/28] qapi: Support multiple command registries per program

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>> The command registry encapsulates a single command list.  Give the
>> functions using it a parameter instead.  Define suitable command lists
>> in monitor, guest agent and test-qmp-commands.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/qapi/qmp/dispatch.h | 22 ++++++++++++++--------
>>  monitor.c                   | 31 +++++++++++++++++--------------
>>  qapi/qmp-dispatch.c         | 17 +++++++++++++----
>>  qapi/qmp-registry.c         | 37 ++++++++++++++++++-------------------
>>  qga/commands.c              |  2 +-
>>  qga/guest-agent-core.h      |  2 ++
>>  qga/main.c                  | 19 ++++++++++---------
>>  scripts/qapi-commands.py    | 16 ++++++++++------
>>  tests/test-qmp-commands.c   | 12 +++++++-----
>>  9 files changed, 92 insertions(+), 66 deletions(-)
>> 
>
>> +++ b/qapi/qmp-dispatch.c
>> @@ -67,7 +67,11 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>>      return dict;
>>  }
>>  
>> -static QObject *do_qmp_dispatch(QObject *request, Error **errp)
>> +volatile QmpCommand *save_cmd;
>
> A comment why volatile is necessary would be nice...

Uh...

>> +QmpCommand cmd2;
>> +
>> +static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
>> +                                Error **errp)
>>  {
>>      Error *local_err = NULL;
>>      const char *command;
>> @@ -81,7 +85,7 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp)
>>      }
>>  
>>      command = qdict_get_str(dict, "execute");
>> -    cmd = qmp_find_command(command);
>> +    cmd = qmp_find_command(cmds, command);
>>      if (cmd == NULL) {
>>          error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
>>                    "The command %s has not been found", command);
>> @@ -93,6 +97,9 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp)
>>          return NULL;
>>      }
>>  
>> +    assert(!cmd->options & QCO_NO_SUCCESS_RESP);
>> +    save_cmd = cmd;
>> +    cmd2 = *cmd;
>>      if (!qdict_haskey(dict, "arguments")) {
>>          args = qdict_new();
>>      } else {
>> @@ -111,6 +118,8 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp)
>>  
>>      QDECREF(args);
>>  
>> +    assert(!cmd->options & QCO_NO_SUCCESS_RESP);
>> +    assert(ret || local_err);
>
> ...or is this leftovers from your debugging?

Corret.  I'll drop it.

> The rest of the patch looks fine; it is converting a global variable
> into a per-instance variable.

Squashing in the obvious garbage collection.  May I add your R-by?



diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 95a0f48..72827a3 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -67,9 +67,6 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
     return dict;
 }
 
-volatile QmpCommand *save_cmd;
-QmpCommand cmd2;
-
 static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
                                 Error **errp)
 {
@@ -97,9 +94,6 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
         return NULL;
     }
 
-    assert(!cmd->options & QCO_NO_SUCCESS_RESP);
-    save_cmd = cmd;
-    cmd2 = *cmd;
     if (!qdict_haskey(dict, "arguments")) {
         args = qdict_new();
     } else {
@@ -118,8 +112,6 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
 
     QDECREF(args);
 
-    assert(!cmd->options & QCO_NO_SUCCESS_RESP);
-    assert(ret || local_err);
     return ret;
 }