Re: [Qemu-devel] [PATCH 2/3] HMP: pass in parameter for info sub command

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2012-12-21 22:49, Markus Armbruster 写道:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
>> On Wed, 19 Dec 2012 18:17:09 +0800
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>
>>>    This patch enable sub info command handler getting meaningful
>>> parameter.
>>>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> ---
>>>   hmp-commands.hx |    2 +-
>>>   monitor.c       |   79 +++++++++++++++++++++++++++++++++++++++----------------
>>>   2 files changed, 57 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>>> index 010b8c9..667fab8 100644
>>> --- a/hmp-commands.hx
>>> +++ b/hmp-commands.hx
>>> @@ -1486,7 +1486,7 @@ ETEXI
>>>   
>>>       {
>>>           .name       = "info",
>>> -        .args_type  = "item:s?",
>>> +        .args_type  = "item:S?",
>>>           .params     = "[subcommand]",
>>>           .help       = "show various information about the system state",
>>>           .mhandler.cmd = do_info,
>>> diff --git a/monitor.c b/monitor.c
>>> index 797680f..ce0e74d 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
> 
> Missing: documentation for the new arg type S in the comment that starts
> with "Supported types".
> 
  OK, will add it.

>>> @@ -464,6 +464,11 @@ QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
>>>   MonitorEventState monitor_event_state[QEVENT_MAX];
>>>   QemuMutex monitor_event_state_lock;
>>>   
>>> +static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>> +                                              const char *cmdline,
>>> +                                              const mon_cmd_t *table,
>>> +                                              QDict *qdict);
>>
>> Please, move the function instead.
> 
> Move will make review harder.  I don't mind forward declarations.
> 
  I will move the function in a separate patch.

>>> +
>>>   /*
>>>    * Emits the event to every monitor instance
>>>    */
>>> @@ -809,26 +814,29 @@ static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
>>>   static void do_info(Monitor *mon, const QDict *qdict)
>>>   {
>>>       const mon_cmd_t *cmd;
>>> +    QDict *qdict_info;
>>>       const char *item = qdict_get_try_str(qdict, "item");
>>>   
>>>       if (!item) {
>>>           goto help;
>>>       }
>>>   
>>> -    for (cmd = info_cmds; cmd->name != NULL; cmd++) {
>>> -        if (compare_cmd(item, cmd->name))
>>> -            break;
>>> -    }
>>> +    qdict_info = qdict_new();
>>>   
>>> -    if (cmd->name == NULL) {
>>> -        goto help;
>>> +    cmd = monitor_parse_command(mon, item, info_cmds, qdict_info);
>>> +    if (!cmd) {
>>> +        QDECREF(qdict_info);
>>> +        /* don't help here, to avoid error message got ignored */
> 
> I'm afraid I don't understand your comment.
> 
> Oh, you mean you don't want to call help_cmd() here!
> 
  Yes, not to show the help contents, same as not call help_cmd().

>>> +        return;
>>>       }
>>>   
>>> -    cmd->mhandler.info(mon, NULL);
>>> +    cmd->mhandler.info(mon, qdict_info);
>>> +    QDECREF(qdict_info);
>>>       return;
>>>   
>>>   help:
>>>       help_cmd(mon, "info");
>>> +    return;
>>>   }
> 
> The function now looks like this:
> 
>      static void do_info(Monitor *mon, const QDict *qdict)
>      {
>          const mon_cmd_t *cmd;
>          QDict *qdict_info;
>          const char *item = qdict_get_try_str(qdict, "item");
> 
>          if (!item) {
>              goto help;
>          }
> 
>          qdict_info = qdict_new();
> 
>          cmd = monitor_parse_command(mon, item, info_cmds, qdict_info);
>          if (!cmd) {
>              QDECREF(qdict_info);
>              /* don't help here, to avoid error message got ignored */
>              return;
>          }
> 
>          cmd->mhandler.info(mon, qdict_info);
>          QDECREF(qdict_info);
>          return;
> 
>      help:
>          help_cmd(mon, "info");
>          return;
>      }
> 
> Control flow isn't nice.  What about:
> 
>      static void do_info(Monitor *mon, const QDict *qdict)
>      {
>          const mon_cmd_t *cmd;
>          QDict *qdict_info;
>          const char *item = qdict_get_try_str(qdict, "item");
> 
>          if (!item) {
>              help_cmd(mon, "info");
>              return;
>          }
> 
>          qdict_info = qdict_new();
> 
>          cmd = monitor_parse_command(mon, item, info_cmds, qdict_info);
>          if (!cmd) {
>              goto out;
>          }
> 
>          cmd->mhandler.info(mon, qdict_info);
>      out:
>          QDECREF(qdict_info);
>          return;
>      }
>
  OK, I'll following this way.

>>>   
>>>   CommandInfoList *qmp_query_commands(Error **errp)
>>> @@ -3534,18 +3542,15 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table,
>>>       return NULL;
>>>   }
>>>   
>>> -static const mon_cmd_t *monitor_find_command(const char *cmdname)
>>> +static const mon_cmd_t *monitor_find_command(const char *cmdname,
>>> +                                             const mon_cmd_t *table)
>>>   {
>>> -    return search_dispatch_table(mon_cmds, cmdname);
>>> -}
>>> -
>>> -static const mon_cmd_t *qmp_find_cmd(const char *cmdname)
>>> -{
>>> -    return search_dispatch_table(qmp_cmds, cmdname);
>>> +    return search_dispatch_table(table, cmdname);
>>
>> Please, keep only search_dispatch_table().
> 
> Yes, because the resulting function is silly :)
> 
>      static const mon_cmd_t *monitor_find_command(const char *cmdname,
>                                                   const mon_cmd_t *table)
>      {
>          return search_dispatch_table(table, cmdname);
>      }
> 
  I'll remove simple encapsulate function monitor_find_command(),
which's naming make me confused.

>> Actually, maybe you could change handle_qmp_command() to just loop through
>> command names and compare them with memcpy() (in a different patch, please)
>> then you keep monitor_find_command() for handle_hmp_command().
>>
>>>   }
>>>   
>>>   static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>>                                                 const char *cmdline,
>>> +                                              const mon_cmd_t *table,
>>>                                                 QDict *qdict)
>>>   {
>>>       const char *p, *typestr;
>>> @@ -3564,7 +3569,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>>       if (!p)
>>>           return NULL;
>>>   
>>> -    cmd = monitor_find_command(cmdname);
>>> +    cmd = monitor_find_command(cmdname, table);
>>>       if (!cmd) {
>>>           monitor_printf(mon, "unknown command: '%s'\n", cmdname);
>>>           return NULL;
>>> @@ -3872,6 +3877,31 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>>                   }
>>>               }
>>>               break;
>>> +        case 'S':
>>> +            {
>>> +                /* package all remaining string */
>>> +                int len;
>>> +
>>> +                while (qemu_isspace(*p)) {
>>> +                    p++;
>>> +                }
>>> +                if (*typestr == '?') {
>>> +                    typestr++;
>>> +                    if (*p == '\0') {
>>> +                        /* no remaining string: NULL argument */
>>> +                        break;
>>> +                    }
>>> +                }
>>> +                len = strlen(p);
>>> +                if (len <= 0) {
>>> +                    monitor_printf(mon, "%s: string expected\n",
>>> +                                   cmdname);
> 
> A "string" in this context is an argument for arg type 's', i.e. a
> sequence of characters delimited by unescaped '"', or a sequence of
> non-whitespace characters not starting with '"'.  That's not what we
> expect here.  We expect arbitrary arguments.
> 
> Suggest "arguments expected".
> 
  OK.

>>> +                    break;
>>> +                }
>>> +                qdict_put(qdict, key, qstring_from_str(p));
>>> +                p += len;
>>> +            }
>>
>> This is a true HMP-style hack :)
>>
>> I don't know how to make this better though (at least not without asking you
>> to re-write the whole thing). Maybe Markus has a better idea?
> 
> Let me try :)
> 
> The new arg type 'S' consumes the rest of the line unparsed, and puts it
> into the argument qdict.  Has to come last, obviously.
> 
> do_info() extracts it, then parses it like a full command.  The info
> command effectively adds like a prefix that switches command parsing to
> an alternate table of commands.  Works, quite flexible, but pretty
> arcane.
> 
> Try #1:
> 
> Implement the command prefix concept the direct way.  Mark the command
> as prefix.  Instead of a handler, it gets a pointer to the alternate
> table of commands.  When monitor_parse_command() recognizes a prefix
> command, it drops the first word, switches to the alternate table, and
> starts over.
> 
  It seems "info" command need to be marked and searched in
monitor_parse_command(),
> Try #2:
> 
> We already have commands that take key=value,... arguments (arg type
> 'O'): device_add and netdev_add.  Perhaps info could use args_type
> "item:s?,opts:O".  First argument is unchanged.  The new second argument
> accepts key=value,... options.  'O' argument is always optional.  One
> key can be declared optional, so that value (no '=') is shorthand for
> that key=value.
  Personally I feel approach #1 is better, which let the command parse
layer knows there would be another sub command layer, and enable embbed
sub commands with deeper layers transparently. #2 and my previous patch
is a hack but each layer don't know sub command exist.
  For #1 I guess additional tag about "sub command' need to be added,
I guess that would be

typedef struct mon_cmd_t {
    const char *name;
    const char *args_type;
    const char *params;
    const char *help;
    void (*user_print)(Monitor *mon, const QObject *data);
    union {
        void (*info)(Monitor *mon);
        void (*cmd)(Monitor *mon, const QDict *qdict);
        int  (*cmd_new)(Monitor *mon, const QDict *params, QObject
**ret_data);
        int  (*cmd_async)(Monitor *mon, const QDict *params,
                          MonitorCompletion *cb, void *opaque);
    } mhandler;
    int flags;
    struct mon_cmd_t *sub_table;
} mon_cmd_t;

  *sub_table flag if it have a 2nd level of command table.
> 
>>> +            break;
>>>           default:
>>>           bad_type:
>>>               monitor_printf(mon, "%s: unknown type '%c'\n", cmdname, c);
>>> @@ -3925,7 +3955,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
>>>   
>>>       qdict = qdict_new();
>>>   
>>> -    cmd = monitor_parse_command(mon, cmdline, qdict);
>>> +    cmd = monitor_parse_command(mon, cmdline, mon_cmds, qdict);
>>>       if (!cmd)
>>>           goto out;
>>>   
>>> @@ -4144,12 +4174,7 @@ static void monitor_find_completion(const char *cmdline)
>>>               break;
>>>           case 's':
>>>               /* XXX: more generic ? */
>>> -            if (!strcmp(cmd->name, "info")) {
>>> -                readline_set_completion_index(cur_mon->rs, strlen(str));
>>> -                for(cmd = info_cmds; cmd->name != NULL; cmd++) {
>>> -                    cmd_completion(str, cmd->name);
>>> -                }
>>> -            } else if (!strcmp(cmd->name, "sendkey")) {
>>> +            if (!strcmp(cmd->name, "sendkey")) {
>>>                   char *sep = strrchr(str, '-');
>>>                   if (sep)
>>>                       str = sep + 1;
> 
> You move the special case hack for info argument completion to case 'S'
> (next hunk), but leave behind the /* XXX: more generic ? */ that marks
> it as hack!  Please move it along with the hack.
> 
  OK.

>>> @@ -4163,6 +4188,14 @@ static void monitor_find_completion(const char *cmdline)
>>>                       cmd_completion(str, cmd->name);
>>>                   }
>>>               }
>>> +        case 'S':
>>> +            /* generic string packaged */
>>> +            if (!strcmp(cmd->name, "info")) {
>>> +                readline_set_completion_index(cur_mon->rs, strlen(str));
>>> +                for (cmd = info_cmds; cmd->name != NULL; cmd++) {
>>> +                    cmd_completion(str, cmd->name);
>>> +                }
>>> +            }
>>>               break;
>>>           default:
>>>               break;
>>> @@ -4483,7 +4516,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>>>           goto err_out;
>>>       }
>>>   
>>> -    cmd = qmp_find_cmd(cmd_name);
>>> +    cmd = monitor_find_command(cmd_name, qmp_cmds);
>>>       if (!cmd) {
>>>           qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
>>>           goto err_out;
> 


-- 
Best Regards

Wenchao Xia