Re: [Qemu-devel] [PATCH V5 4/7] monitor: avoid direct use of global *info_cmds in help functions

[Luiz Capitulino <lcapitulino@redhat.com>]

On Sat, 29 Jun 2013 11:52:58 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> In help functions info_cmds is treated as sub command group now, not as
> a special case any more. Still help can't show message for single command
> under "info", since command parser reject additional parameter, which

What you meant by "help can't show message for single command"?

> can be improved by change "help" item parameter define later. "log" is
> still treated as special help case. compare_cmd() is used instead of strcmp()
> in command searching.

I'm honestly a bit confused with this patch, I think it will be clarified
further down in the series, but might be a good idea to re-work the commit log.
More questions below.

> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  monitor.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 53 insertions(+), 10 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 03a017d..bc62fc7 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -831,33 +831,76 @@ static void parse_cmdline(const char *cmdline,
>      *pnb_args = nb_args;
>  }
>  
> +static void help_cmd_dump_one(Monitor *mon,
> +                              const mon_cmd_t *cmd,
> +                              char **prefix_args,
> +                              int prefix_args_nb)
> +{
> +    int i;
> +
> +    for (i = 0; i < prefix_args_nb; i++) {
> +        monitor_printf(mon, "%s ", prefix_args[i]);
> +    }

What is this for?

> +    monitor_printf(mon, "%s %s -- %s\n", cmd->name, cmd->params, cmd->help);
> +}
> +
>  static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
> -                          const char *prefix, const char *name)
> +                          char **args, int nb_args, int arg_index)
>  {
>      const mon_cmd_t *cmd;
>  
> -    for(cmd = cmds; cmd->name != NULL; cmd++) {
> -        if (!name || !strcmp(name, cmd->name))
> -            monitor_printf(mon, "%s%s %s -- %s\n", prefix, cmd->name,
> -                           cmd->params, cmd->help);
> +    /* Dump all */
> +    if (arg_index >= nb_args) {
> +        for (cmd = cmds; cmd->name != NULL; cmd++) {
> +            help_cmd_dump_one(mon, cmd, args, arg_index);
> +        }
> +        return;
> +    }

Maybe this should be moved to help_cmd() so that it's not a special
case here?

> +
> +    /* Find one entry to dump */
> +    for (cmd = cmds; cmd->name != NULL; cmd++) {
> +        if (compare_cmd(args[arg_index], cmd->name)) {
> +            if (cmd->sub_table) {
> +                help_cmd_dump(mon, cmd->sub_table,
> +                              args, nb_args, arg_index + 1);
> +            } else {
> +                help_cmd_dump_one(mon, cmd, args, arg_index);
> +            }
> +            break;
> +        }
>      }
>  }
>  
>  static void help_cmd(Monitor *mon, const char *name)
>  {
> -    if (name && !strcmp(name, "info")) {
> -        help_cmd_dump(mon, info_cmds, "info ", NULL);
> -    } else {
> -        help_cmd_dump(mon, mon->cmd_table, "", name);
> -        if (name && !strcmp(name, "log")) {
> +    char *args[MAX_ARGS];
> +    int nb_args = 0, i;
> +
> +    if (name) {
> +        /* special case for log */
> +        if (!strcmp(name, "log")) {
>              const QEMULogItem *item;
>              monitor_printf(mon, "Log items (comma separated):\n");
>              monitor_printf(mon, "%-10s %s\n", "none", "remove all logs");
>              for (item = qemu_log_items; item->mask != 0; item++) {
>                  monitor_printf(mon, "%-10s %s\n", item->name, item->help);
>              }
> +            return;
> +        }
> +
> +        parse_cmdline(name, &nb_args, args);
> +        if (nb_args >= MAX_ARGS) {
> +            goto cleanup;
>          }

parse_cmdline() should handle de-allocation on failure, also checking
nb_args for failure is a bad API. This hasn't been a problem so far
because parse_cmdline() is used only once, but now you're making it a
bit more generic so it should be improved.

>      }
> +
> +    help_cmd_dump(mon, mon->cmd_table, args, nb_args, 0);
> +
> +cleanup:
> +    nb_args = nb_args < MAX_ARGS ? nb_args : MAX_ARGS;
> +    for (i = 0; i < nb_args; i++) {
> +        g_free(args[i]);
> +    }

I'd add free_cmdline_args().

>  }
>  
>  static void do_help_cmd(Monitor *mon, const QDict *qdict)