On 06/24/2013 06:48 AM, Wenchao Xia wrote:

In the subject line, you aren't actually discarding a global variable,
so much as avoiding its direct use (the global still exists).  Maybe a
better subject line would be:

monitor: avoid direct use of global *info_cmds in help functions

(2/7 has the same wording issue, where you aren't discarding the variable).

> 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
> 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.
> 
> To tip better what the patch does, code moving is avoided by declare

s/tip better/give better hints about/
s/moving/motion/
s/declare/declaring/

> parse_cmdline() ahead.

Rather than avoiding code motion by adding a forward declaration, I
would instead split this into two patches - one that does JUST code
motion, then the other that takes advantage of the correct motion.
However, it's not a show-stopper to review.

>  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;
>  
> +    /* Dump all */
> +    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++) {

Pre-existing formatting issue, but as long as you are touching both
before and after, you might as well add the space after 'for'.

>  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;

[1]

>          }
>      }
> +
> +    help_cmd_dump(mon, mon->cmd_table, args, nb_args, 0);
> +
> +cleanup:
> +    for (i = 0; i < nb_args; i++) {
> +        g_free(args[i]);

If we got here because nb_args > MAX_ARGS at point [1], then this calls
g_free() on memory that is beyond the array (bad).  Thankfully, I just
read parse_cmdline, and it never sets nb_args > MAX_ARGS.  But this
whole parsing feels rather fragile (not necessarily your fault).

Although I still recommend doing proper code motion for topological
ordering instead of using a crutch of forward declarations, I'm okay if
you fix the commit message and add Reviewed-by: Eric Blake
<eblake@redhat.com>.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org