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

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

于 2013-7-8 23:45, Luiz Capitulino 写道:
> 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"?
>
   I mean "help info block" can't work.

>> 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.
>
   To avoid use of info_cmds, the help function need to use sub command
structure, otherwise "info" is a special case, so I modified the
functions. I will refine the commit message.

>>
>> 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?
>
   It is used to print parent command, such as "info" in "info block"

   help_cmd_dump():
-            monitor_printf(mon, "%s%s %s -- %s\n", prefix, cmd->name,
-                           cmd->params, cmd->help);

   The old code can't work with sub command for parameter prefix. To
solve it, I introduced function help_cmd_dump_one(), which dedicate
to format control and knows parent commands. This can avoid dynamic
snprintf to a buffer for prefix.

>> +    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?
>
   help_cmd_dump() is changed as a re-enterable function to handle
sub command case, so above lines need to be inside a re-enterable
function, to handle the case that dump all commands under one group,
such as "help info". Moving it out will make it work only when
folder depth is 1.

>> +
>> +    /* 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.
>
   OK, I will improve it in an previous patch.

>>       }
>> +
>> +    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().
>
   OK.

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


-- 
Best Regards

Wenchao Xia