[V8,09/13] monitor: support sub command in help

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 <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 monitor.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 53 insertions(+), 10 deletions(-)

On Fri, 26 Jul 2013 11:20:38 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> 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 <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  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);
> +}
> +
> +/* @args[@arg_index] 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)

于 2013-7-30 22:51, Luiz Capitulino 写道:
> On Fri, 26 Jul 2013 11:20:38 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> 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 <xiawenc@linux.vnet.ibm.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   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);
>> +}
>> +
>> +/* @args[@arg_index] 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.
>
   nb_args = 0 in that case, I think it would not free again. Still,  I
am OK to do this change, since the parse_cmdline() already does the
clean up, not need to ask caller to do it again.

>>           }
>>       }
>> +
>> +    /* 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)
>