Re: [Qemu-devel] [PATCH V8 09/13] monitor: support sub command in help

[Luiz Capitulino]

On Fri, 26 Jul 2013 11:20:38 +0800
Wenchao Xia <address@hidden> wrote:

> The old code in help_cmd() uses global 'info_cmds' and treats it as a
> special case. Actually 'info_cmds' is a sub command group of 'mon_cmds',
> in order to avoid direct use of it, help_cmd() needs to change its work
> mechanism to support sub command and not treat it as a special case
> any more.
> 
> To support sub command, help_cmd() will first parse the input and then call
> help_cmd_dump(), which works as a reentrant function. When it meets a sub
> command, it simply enters the function again. Since help dumping needs to
> know whole input to printf full help message include prefix, for example,
> "help info block" need to printf prefix "info", so help_cmd_dump() takes all
> args from input and extra parameter arg_index to identify the progress.
> Another function help_cmd_dump_one() is introduced to printf the prefix
> and command's help message.
> 
> Now help supports sub command, so later if another sub command group is
> added in any depth, help will automatically work for it. Still "help info
> block" will show error since command parser reject additional parameter,
> which can be improved later. "log" is still treated as a special case.
> 
> Signed-off-by: Wenchao Xia <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  monitor.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 53 insertions(+), 10 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index c942b77..77df88d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -868,33 +868,76 @@ static int parse_cmdline(const char *cmdline,
>      return -1;
>  }
>  
> +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]);
> +    }
> +    monitor_printf(mon, "%s %s -- %s\n", cmd->name, cmd->params, cmd->help);
> +}
> +
> +/* @address@hidden is the valid command need to find in @cmds */
>  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);
> +    /* No valid arg need to compare with, dump all in *cmds */
> +    if (arg_index >= nb_args) {
> +        for (cmd = cmds; cmd->name != NULL; cmd++) {
> +            help_cmd_dump_one(mon, cmd, args, arg_index);
> +        }
> +        return;
> +    }
> +
> +    /* Find one entry to dump */
> +    for (cmd = cmds; cmd->name != NULL; cmd++) {
> +        if (compare_cmd(args[arg_index], cmd->name)) {
> +            if (cmd->sub_table) {
> +                /* continue with next arg */
> +                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;
> +
> +    /* 1. parse user input */
> +    if (name) {
> +        /* special case for log, directly dump and return */
> +        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;
> +        }
> +
> +        if (parse_cmdline(name, &nb_args, args) < 0) {
> +            goto cleanup;

Won't this result in a double-free if parse_cmdline() fails?

The fix is just to return instead of going to cleanup. I can do this
change if you agree and if I don't spot another bug.

>          }
>      }
> +
> +    /* 2. dump the contents according to parsed args */
> +    help_cmd_dump(mon, mon->cmd_table, args, nb_args, 0);
> +
> +cleanup:
> +    free_cmdline_args(args, nb_args);
>  }
>  
>  static void do_help_cmd(Monitor *mon, const QDict *qdict)