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

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

于 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)
>


-- 
Best Regards

Wenchao Xia