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

login
register
mail settings
Submitter Wayne Xia
Date June 24, 2013, 12:48 p.m.
Message ID <1372078125-31085-4-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/253828/
State New
Headers show

Comments

Wayne Xia - June 24, 2013, 12:48 p.m.
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(-)
Eric Blake - June 27, 2013, 7:26 p.m.
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>.

Patch

diff --git a/monitor.c b/monitor.c
index 528e4a8..54fc36f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -733,33 +733,80 @@  static int compare_cmd(const char *name, const char *list)
     return 0;
 }
 
+#define MAX_ARGS 16
+
+static void parse_cmdline(const char *cmdline,
+                         int *pnb_args, char **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]);
+    }
+    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;
 
+    /* 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++) {
-        if (!name || !strcmp(name, cmd->name))
-            monitor_printf(mon, "%s%s %s -- %s\n", prefix, cmd->name,
-                           cmd->params, cmd->help);
+        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;
         }
     }
+
+    help_cmd_dump(mon, mon->cmd_table, args, nb_args, 0);
+
+cleanup:
+    for (i = 0; i < nb_args; i++) {
+        g_free(args[i]);
+    }
 }
 
 static void do_help_cmd(Monitor *mon, const QDict *qdict)
@@ -3533,8 +3580,6 @@  static char *key_get_info(const char *type, char **key)
 static int default_fmt_format = 'x';
 static int default_fmt_size = 4;
 
-#define MAX_ARGS 16
-
 static int is_valid_option(const char *c, const char *typestr)
 {
     char option[3];