[V2,3/7] monitor: discard global variable *info_cmds in help functions

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
parse_cmdline() ahead.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 monitor.c |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 56 insertions(+), 11 deletions(-)

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