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

[Luiz Capitulino]

On Wed, 19 Dec 2012 18:17:09 +0800
Wenchao Xia <address@hidden> wrote:

>   This patch enable sub info command handler getting meaningful
> parameter.
> 
> Signed-off-by: Wenchao Xia <address@hidden>
> ---
>  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
> @@ -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.

> +
>  /*
>   * 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 */
> +        return;
>      }
>  
> -    cmd->mhandler.info(mon, NULL);
> +    cmd->mhandler.info(mon, qdict_info);
> +    QDECREF(qdict_info);
>      return;
>  
>  help:
>      help_cmd(mon, "info");
> +    return;
>  }
>  
>  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().

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);
> +                    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?

> +            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;
> @@ -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;