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

[Wenchao Xia]

于 2013-7-8 23:45, Luiz Capitulino 写道:
> On Sat, 29 Jun 2013 11:52:58 +0800
> Wenchao Xia <address@hidden> 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 <address@hidden>
> > ---
> >   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