[Qemu-devel] [PATCH V2 0/7] monitor: support sub command group in auto completion and help

[Qemu-devel] [PATCH V2 0/7] monitor: support sub command group in auto completion and help

[Wenchao Xia]

Global variable *mon_cmds and *info_cmds are not used any more, *cur_mon is not
used in completion related functions. It is possible to create a monitor with
different command table now, but that requirement do not exist yet, so not changed
it to save trouble. Log command is still a special case now, may be it can be converted
as sub group later.

Patch 1-3 make sure the functions can be re-entered safely.

V2:
  General:
  To discard *info_comds more graceful, help related function is modified to support
sub command too.
  Patch 6/7 are added to improve help related functions.
  Patch 5: not directly return to make sure args are freed.

  Address Luiz's comments:
  Split patch into small serial.
  struct mon_cmd_t was not moved into header file, instead mon_cmnd_t *cmd_table is
added as a member in struct Monitor.
  5/7: drop original code comments for "info" in monitor_find_completion().

Wenchao Xia (7):
  1 monitor: discard global variable *cur_mon in completion functions
  2 monitor: discard global variable *mon_cmds
  3 monitor: discard global variable *info_cmds in help functions
  4 monitor: code move for parse_cmdline()
  5 monitor: support sub commands in auto completion
  6 monitor: improve "help" in auto completion for sub command
  7 monitor: improve "help" to allow show tip of single command in sub group

 hmp-commands.hx            |    2 +-
 include/monitor/readline.h |    3 +-
 monitor.c                  |  369 ++++++++++++++++++++++++++++----------------
 readline.c                 |    5 +-
 4 files changed, 239 insertions(+), 140 deletions(-)

[Qemu-devel] [PATCH V2 1/7] monitor: discard global variable *cur_mon in completion functions

[Wenchao Xia]

Parameter *mon is added to replace *cur_mon, and readline_completion()
pass rs->mon as value, which should be initialized in readline_init()
called by monitor_init(). In short, structure ReadLineState controls
where the action would be taken now.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 include/monitor/readline.h |    3 +-
 monitor.c                  |   53 ++++++++++++++++++++++++++-----------------
 readline.c                 |    5 +--
 3 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/include/monitor/readline.h b/include/monitor/readline.h
index fc9806e..0faf6e1 100644
--- a/include/monitor/readline.h
+++ b/include/monitor/readline.h
@@ -8,7 +8,8 @@
 #define READLINE_MAX_COMPLETIONS 256
 
 typedef void ReadLineFunc(Monitor *mon, const char *str, void *opaque);
-typedef void ReadLineCompletionFunc(const char *cmdline);
+typedef void ReadLineCompletionFunc(Monitor *mon,
+                                    const char *cmdline);
 
 typedef struct ReadLineState {
     char cmd_buf[READLINE_CMD_BUF_SIZE + 1];
diff --git a/monitor.c b/monitor.c
index 70ae8f5..f3fdaa4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3999,7 +3999,7 @@ out:
     QDECREF(qdict);
 }
 
-static void cmd_completion(const char *name, const char *list)
+static void cmd_completion(Monitor *mon, const char *name, const char *list)
 {
     const char *p, *pstart;
     char cmd[128];
@@ -4017,7 +4017,7 @@ static void cmd_completion(const char *name, const char *list)
         memcpy(cmd, pstart, len);
         cmd[len] = '\0';
         if (name[0] == '\0' || !strncmp(name, cmd, strlen(name))) {
-            readline_add_completion(cur_mon->rs, cmd);
+            readline_add_completion(mon->rs, cmd);
         }
         if (*p == '\0')
             break;
@@ -4025,7 +4025,7 @@ static void cmd_completion(const char *name, const char *list)
     }
 }
 
-static void file_completion(const char *input)
+static void file_completion(Monitor *mon, const char *input)
 {
     DIR *ffs;
     struct dirent *d;
@@ -4048,7 +4048,7 @@ static void file_completion(const char *input)
         pstrcpy(file_prefix, sizeof(file_prefix), p + 1);
     }
 #ifdef DEBUG_COMPLETION
-    monitor_printf(cur_mon, "input='%s' path='%s' prefix='%s'\n",
+    monitor_printf(mon, "input='%s' path='%s' prefix='%s'\n",
                    input, path, file_prefix);
 #endif
     ffs = opendir(path);
@@ -4075,20 +4075,27 @@ static void file_completion(const char *input)
             if (stat(file, &sb) == 0 && S_ISDIR(sb.st_mode)) {
                 pstrcat(file, sizeof(file), "/");
             }
-            readline_add_completion(cur_mon->rs, file);
+            readline_add_completion(mon->rs, file);
         }
     }
     closedir(ffs);
 }
 
+typedef struct MonitorBlockComplete {
+    Monitor *mon;
+    const char *input;
+} MonitorBlockComplete;
+
 static void block_completion_it(void *opaque, BlockDriverState *bs)
 {
     const char *name = bdrv_get_device_name(bs);
-    const char *input = opaque;
+    MonitorBlockComplete *mbc = opaque;
+    Monitor *mon = mbc->mon;
+    const char *input = mbc->input;
 
     if (input[0] == '\0' ||
         !strncmp(name, (char *)input, strlen(input))) {
-        readline_add_completion(cur_mon->rs, name);
+        readline_add_completion(mon->rs, name);
     }
 }
 
@@ -4124,18 +4131,20 @@ static const char *next_arg_type(const char *typestr)
     return (p != NULL ? ++p : typestr);
 }
 
-static void monitor_find_completion(const char *cmdline)
+static void monitor_find_completion(Monitor *mon,
+                                    const char *cmdline)
 {
     const char *cmdname;
     char *args[MAX_ARGS];
     int nb_args, i, len;
     const char *ptype, *str;
     const mon_cmd_t *cmd;
+    MonitorBlockComplete mbs;
 
     parse_cmdline(cmdline, &nb_args, args);
 #ifdef DEBUG_COMPLETION
     for(i = 0; i < nb_args; i++) {
-        monitor_printf(cur_mon, "arg%d = '%s'\n", i, (char *)args[i]);
+        monitor_printf(mon, "arg%d = '%s'\n", i, (char *)args[i]);
     }
 #endif
 
@@ -4154,9 +4163,9 @@ static void monitor_find_completion(const char *cmdline)
             cmdname = "";
         else
             cmdname = args[0];
-        readline_set_completion_index(cur_mon->rs, strlen(cmdname));
+        readline_set_completion_index(mon->rs, strlen(cmdname));
         for(cmd = mon_cmds; cmd->name != NULL; cmd++) {
-            cmd_completion(cmdname, cmd->name);
+            cmd_completion(mon, cmdname, cmd->name);
         }
     } else {
         /* find the command */
@@ -4184,33 +4193,35 @@ static void monitor_find_completion(const char *cmdline)
         switch(*ptype) {
         case 'F':
             /* file completion */
-            readline_set_completion_index(cur_mon->rs, strlen(str));
-            file_completion(str);
+            readline_set_completion_index(mon->rs, strlen(str));
+            file_completion(mon, str);
             break;
         case 'B':
             /* block device name completion */
-            readline_set_completion_index(cur_mon->rs, strlen(str));
-            bdrv_iterate(block_completion_it, (void *)str);
+            mbs.mon = mon;
+            mbs.input = str;
+            readline_set_completion_index(mon->rs, strlen(str));
+            bdrv_iterate(block_completion_it, &mbs);
             break;
         case 's':
             /* XXX: more generic ? */
             if (!strcmp(cmd->name, "info")) {
-                readline_set_completion_index(cur_mon->rs, strlen(str));
+                readline_set_completion_index(mon->rs, strlen(str));
                 for(cmd = info_cmds; cmd->name != NULL; cmd++) {
-                    cmd_completion(str, cmd->name);
+                    cmd_completion(mon, str, cmd->name);
                 }
             } else if (!strcmp(cmd->name, "sendkey")) {
                 char *sep = strrchr(str, '-');
                 if (sep)
                     str = sep + 1;
-                readline_set_completion_index(cur_mon->rs, strlen(str));
+                readline_set_completion_index(mon->rs, strlen(str));
                 for (i = 0; i < Q_KEY_CODE_MAX; i++) {
-                    cmd_completion(str, QKeyCode_lookup[i]);
+                    cmd_completion(mon, str, QKeyCode_lookup[i]);
                 }
             } else if (!strcmp(cmd->name, "help|?")) {
-                readline_set_completion_index(cur_mon->rs, strlen(str));
+                readline_set_completion_index(mon->rs, strlen(str));
                 for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
-                    cmd_completion(str, cmd->name);
+                    cmd_completion(mon, str, cmd->name);
                 }
             }
             break;
diff --git a/readline.c b/readline.c
index 1c0f7ee..abf27dd 100644
--- a/readline.c
+++ b/readline.c
@@ -276,7 +276,6 @@ void readline_set_completion_index(ReadLineState *rs, int index)
 
 static void readline_completion(ReadLineState *rs)
 {
-    Monitor *mon = cur_mon;
     int len, i, j, max_width, nb_cols, max_prefix;
     char *cmdline;
 
@@ -285,7 +284,7 @@ static void readline_completion(ReadLineState *rs)
     cmdline = g_malloc(rs->cmd_buf_index + 1);
     memcpy(cmdline, rs->cmd_buf, rs->cmd_buf_index);
     cmdline[rs->cmd_buf_index] = '\0';
-    rs->completion_finder(cmdline);
+    rs->completion_finder(rs->mon, cmdline);
     g_free(cmdline);
 
     /* no completion found */
@@ -300,7 +299,7 @@ static void readline_completion(ReadLineState *rs)
         if (len > 0 && rs->completions[0][len - 1] != '/')
             readline_insert_char(rs, ' ');
     } else {
-        monitor_printf(mon, "\n");
+        monitor_printf(rs->mon, "\n");
         max_width = 0;
         max_prefix = 0;	
         for(i = 0; i < rs->nb_completions; i++) {
-- 
1.7.1

[Qemu-devel] [PATCH V2 2/7] monitor: discard global variable *mon_cmds

[Wenchao Xia]

New member *cmd_table is added in structure Monitor to avoid direct usage of
*mon_cmds. Now monitor have an associated command table, when global variable
*info_cmds is also discarded, structure Monitor would gain full control about
how to deal with user input.

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

diff --git a/monitor.c b/monitor.c
index f3fdaa4..528e4a8 100644
--- a/monitor.c
+++ b/monitor.c
@@ -194,6 +194,7 @@ struct Monitor {
     CPUArchState *mon_cpu;
     BlockDriverCompletionFunc *password_completion_cb;
     void *password_opaque;
+    mon_cmd_t *cmd_table;
     QError *error;
     QLIST_HEAD(,mon_fd_t) fds;
     QLIST_ENTRY(Monitor) entry;
@@ -749,7 +750,7 @@ 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_cmds, "", name);
+        help_cmd_dump(mon, mon->cmd_table, "", name);
         if (name && !strcmp(name, "log")) {
             const QEMULogItem *item;
             monitor_printf(mon, "Log items (comma separated):\n");
@@ -3975,7 +3976,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
 
     qdict = qdict_new();
 
-    cmd = monitor_parse_command(mon, cmdline, 0, mon_cmds, qdict);
+    cmd = monitor_parse_command(mon, cmdline, 0, mon->cmd_table, qdict);
     if (!cmd)
         goto out;
 
@@ -4164,12 +4165,12 @@ static void monitor_find_completion(Monitor *mon,
         else
             cmdname = args[0];
         readline_set_completion_index(mon->rs, strlen(cmdname));
-        for(cmd = mon_cmds; cmd->name != NULL; cmd++) {
+        for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
             cmd_completion(mon, cmdname, cmd->name);
         }
     } else {
         /* find the command */
-        for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
+        for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
             if (compare_cmd(args[0], cmd->name)) {
                 break;
             }
@@ -4220,7 +4221,7 @@ static void monitor_find_completion(Monitor *mon,
                 }
             } else if (!strcmp(cmd->name, "help|?")) {
                 readline_set_completion_index(mon->rs, strlen(str));
-                for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
+                for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
                     cmd_completion(mon, str, cmd->name);
                 }
             }
@@ -4783,6 +4784,9 @@ void monitor_init(CharDriverState *chr, int flags)
     if (!default_mon || (flags & MONITOR_IS_DEFAULT))
         default_mon = mon;
 
+    /* Always use *mon_cmds as root command table now. */
+    mon->cmd_table = mon_cmds;
+
     sortcmdlist();
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH V2 3/7] monitor: discard global variable *info_cmds in help functions

[Wenchao Xia]

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(-)

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

[Qemu-devel] [PATCH V2 4/7] monitor: code move for parse_cmdline()

[Wenchao Xia]

Also fix code style error reported by check script.

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

diff --git a/monitor.c b/monitor.c
index 54fc36f..aa641de 100644
--- a/monitor.c
+++ b/monitor.c
@@ -733,10 +733,103 @@ static int compare_cmd(const char *name, const char *list)
     return 0;
 }
 
+static int get_str(char *buf, int buf_size, const char **pp)
+{
+    const char *p;
+    char *q;
+    int c;
+
+    q = buf;
+    p = *pp;
+    while (qemu_isspace(*p)) {
+        p++;
+    }
+    if (*p == '\0') {
+    fail:
+        *q = '\0';
+        *pp = p;
+        return -1;
+    }
+    if (*p == '\"') {
+        p++;
+        while (*p != '\0' && *p != '\"') {
+            if (*p == '\\') {
+                p++;
+                c = *p++;
+                switch (c) {
+                case 'n':
+                    c = '\n';
+                    break;
+                case 'r':
+                    c = '\r';
+                    break;
+                case '\\':
+                case '\'':
+                case '\"':
+                    break;
+                default:
+                    qemu_printf("unsupported escape code: '\\%c'\n", c);
+                    goto fail;
+                }
+                if ((q - buf) < buf_size - 1) {
+                    *q++ = c;
+                }
+            } else {
+                if ((q - buf) < buf_size - 1) {
+                    *q++ = *p;
+                }
+                p++;
+            }
+        }
+        if (*p != '\"') {
+            qemu_printf("unterminated string\n");
+            goto fail;
+        }
+        p++;
+    } else {
+        while (*p != '\0' && !qemu_isspace(*p)) {
+            if ((q - buf) < buf_size - 1) {
+                *q++ = *p;
+            }
+            p++;
+        }
+    }
+    *q = '\0';
+    *pp = p;
+    return 0;
+}
+
 #define MAX_ARGS 16
 
+/* NOTE: this parser is an approximate form of the real command parser */
 static void parse_cmdline(const char *cmdline,
-                         int *pnb_args, char **args);
+                         int *pnb_args, char **args)
+{
+    const char *p;
+    int nb_args, ret;
+    char buf[1024];
+
+    p = cmdline;
+    nb_args = 0;
+    for (;;) {
+        while (qemu_isspace(*p)) {
+            p++;
+        }
+        if (*p == '\0') {
+            break;
+        }
+        if (nb_args >= MAX_ARGS) {
+            break;
+        }
+        ret = get_str(buf, sizeof(buf), &p);
+        args[nb_args] = g_strdup(buf);
+        nb_args++;
+        if (ret < 0) {
+            break;
+        }
+    }
+    *pnb_args = nb_args;
+}
 
 static void help_cmd_dump_one(Monitor *mon,
                               const mon_cmd_t *cmd,
@@ -3459,71 +3552,6 @@ static int get_double(Monitor *mon, double *pval, const char **pp)
     return 0;
 }
 
-static int get_str(char *buf, int buf_size, const char **pp)
-{
-    const char *p;
-    char *q;
-    int c;
-
-    q = buf;
-    p = *pp;
-    while (qemu_isspace(*p))
-        p++;
-    if (*p == '\0') {
-    fail:
-        *q = '\0';
-        *pp = p;
-        return -1;
-    }
-    if (*p == '\"') {
-        p++;
-        while (*p != '\0' && *p != '\"') {
-            if (*p == '\\') {
-                p++;
-                c = *p++;
-                switch(c) {
-                case 'n':
-                    c = '\n';
-                    break;
-                case 'r':
-                    c = '\r';
-                    break;
-                case '\\':
-                case '\'':
-                case '\"':
-                    break;
-                default:
-                    qemu_printf("unsupported escape code: '\\%c'\n", c);
-                    goto fail;
-                }
-                if ((q - buf) < buf_size - 1) {
-                    *q++ = c;
-                }
-            } else {
-                if ((q - buf) < buf_size - 1) {
-                    *q++ = *p;
-                }
-                p++;
-            }
-        }
-        if (*p != '\"') {
-            qemu_printf("unterminated string\n");
-            goto fail;
-        }
-        p++;
-    } else {
-        while (*p != '\0' && !qemu_isspace(*p)) {
-            if ((q - buf) < buf_size - 1) {
-                *q++ = *p;
-            }
-            p++;
-        }
-    }
-    *q = '\0';
-    *pp = p;
-    return 0;
-}
-
 /*
  * Store the command-name in cmdname, and return a pointer to
  * the remaining of the command string.
@@ -4145,32 +4173,6 @@ static void block_completion_it(void *opaque, BlockDriverState *bs)
     }
 }
 
-/* NOTE: this parser is an approximate form of the real command parser */
-static void parse_cmdline(const char *cmdline,
-                         int *pnb_args, char **args)
-{
-    const char *p;
-    int nb_args, ret;
-    char buf[1024];
-
-    p = cmdline;
-    nb_args = 0;
-    for(;;) {
-        while (qemu_isspace(*p))
-            p++;
-        if (*p == '\0')
-            break;
-        if (nb_args >= MAX_ARGS)
-            break;
-        ret = get_str(buf, sizeof(buf), &p);
-        args[nb_args] = g_strdup(buf);
-        nb_args++;
-        if (ret < 0)
-            break;
-    }
-    *pnb_args = nb_args;
-}
-
 static const char *next_arg_type(const char *typestr)
 {
     const char *p = strchr(typestr, ':');
-- 
1.7.1

[Qemu-devel] [PATCH V2 5/7] monitor: support sub commands in auto completion

[Wenchao Xia]

This patch allow auot completion work normal in sub command case,
"info block [DEVICE]" can auto complete now, by re-enter the completion
function. Also, original "info" is treated as a special case, now it is
treated as a sub command group, global variable info_cmds is not used
any more.

"help" command is still treated as a special case, since it is not a sub
command group but want to auto complete command in root command table.

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

diff --git a/monitor.c b/monitor.c
index aa641de..f364a0d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4179,10 +4179,11 @@ static const char *next_arg_type(const char *typestr)
     return (p != NULL ? ++p : typestr);
 }
 
-static void monitor_find_completion(Monitor *mon,
-                                    const char *cmdline)
+static void monitor_find_completion_by_table(Monitor *mon,
+                                             const mon_cmd_t *cmd_table,
+                                             const char *cmdline)
 {
-    const char *cmdname;
+    const char *cmdname, *p;
     char *args[MAX_ARGS];
     int nb_args, i, len;
     const char *ptype, *str;
@@ -4212,12 +4213,12 @@ static void monitor_find_completion(Monitor *mon,
         else
             cmdname = args[0];
         readline_set_completion_index(mon->rs, strlen(cmdname));
-        for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
+        for (cmd = cmd_table; cmd->name != NULL; cmd++) {
             cmd_completion(mon, cmdname, cmd->name);
         }
     } else {
         /* find the command */
-        for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
+        for (cmd = cmd_table; cmd->name != NULL; cmd++) {
             if (compare_cmd(args[0], cmd->name)) {
                 break;
             }
@@ -4226,6 +4227,17 @@ static void monitor_find_completion(Monitor *mon,
             goto cleanup;
         }
 
+        /* locate next valid string in original cmdline used by re-enter */
+        p = cmdline + strlen(args[0]);
+        while (qemu_isspace(*p)) {
+            p++;
+        }
+
+        if (cmd->sub_table) {
+            monitor_find_completion_by_table(mon, cmd->sub_table, p);
+            goto cleanup;
+        }
+
         ptype = next_arg_type(cmd->args_type);
         for(i = 0; i < nb_args - 2; i++) {
             if (*ptype != '\0') {
@@ -4252,13 +4264,7 @@ static void monitor_find_completion(Monitor *mon,
             bdrv_iterate(block_completion_it, &mbs);
             break;
         case 's':
-            /* XXX: more generic ? */
-            if (!strcmp(cmd->name, "info")) {
-                readline_set_completion_index(mon->rs, strlen(str));
-                for(cmd = info_cmds; cmd->name != NULL; cmd++) {
-                    cmd_completion(mon, str, cmd->name);
-                }
-            } else if (!strcmp(cmd->name, "sendkey")) {
+            if (!strcmp(cmd->name, "sendkey")) {
                 char *sep = strrchr(str, '-');
                 if (sep)
                     str = sep + 1;
@@ -4284,6 +4290,12 @@ cleanup:
     }
 }
 
+static void monitor_find_completion(Monitor *mon,
+                                    const char *cmdline)
+{
+    return monitor_find_completion_by_table(mon, mon->cmd_table, cmdline);
+}
+
 static int monitor_can_read(void *opaque)
 {
     Monitor *mon = opaque;
-- 
1.7.1

[Qemu-devel] [PATCH V2 6/7] monitor: improve "help" in auto completion for sub command

[Wenchao Xia]

Now special case "help *" in auto completion can work with sub commands,
such as "help info a*".

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

diff --git a/monitor.c b/monitor.c
index f364a0d..8b26b48 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4273,10 +4273,7 @@ static void monitor_find_completion_by_table(Monitor *mon,
                     cmd_completion(mon, str, QKeyCode_lookup[i]);
                 }
             } else if (!strcmp(cmd->name, "help|?")) {
-                readline_set_completion_index(mon->rs, strlen(str));
-                for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
-                    cmd_completion(mon, str, cmd->name);
-                }
+                monitor_find_completion_by_table(mon, cmd_table, p);
             }
             break;
         default:
-- 
1.7.1

[Qemu-devel] [PATCH V2 7/7] monitor: improve "help" to allow show tip of single command in sub group

[Wenchao Xia]

A new parameter type 'S' is introduced to allow user input any string.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 hmp-commands.hx |    2 +-
 monitor.c       |   30 +++++++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 915b0d1..8cf5f29 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -11,7 +11,7 @@ ETEXI
 
     {
         .name       = "help|?",
-        .args_type  = "name:s?",
+        .args_type  = "name:S?",
         .params     = "[cmd]",
         .help       = "show the help",
         .mhandler.cmd = do_help_cmd,
diff --git a/monitor.c b/monitor.c
index 8b26b48..9a7ed67 100644
--- a/monitor.c
+++ b/monitor.c
@@ -83,6 +83,7 @@
  * 'F'          filename
  * 'B'          block device name
  * 's'          string (accept optional quote)
+ * 'S'          supported types, can be any string (accept optional quote)
  * 'O'          option string of the form NAME=VALUE,...
  *              parsed according to QemuOptsList given by its name
  *              Example: 'device:O' uses qemu_device_opts.
@@ -3996,6 +3997,31 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 }
             }
             break;
+        case 'S':
+            {
+                /* package all remaining string */
+                int len;
+
+                while (qemu_isspace(*p)) {
+                    p++;
+                }
+                if (*typestr == '?') {
+                    typestr++;
+                    if (*p == '\0') {
+                        /* no remaining string: NULL argument */
+                        break;
+                    }
+                }
+                len = strlen(p);
+                if (len <= 0) {
+                    monitor_printf(mon, "%s: string expected\n",
+                                   cmdname);
+                    break;
+                }
+                qdict_put(qdict, key, qstring_from_str(p));
+                p += len;
+            }
+            break;
         default:
         bad_type:
             monitor_printf(mon, "%s: unknown type '%c'\n", cmdname, c);
@@ -4272,7 +4298,9 @@ static void monitor_find_completion_by_table(Monitor *mon,
                 for (i = 0; i < Q_KEY_CODE_MAX; i++) {
                     cmd_completion(mon, str, QKeyCode_lookup[i]);
                 }
-            } else if (!strcmp(cmd->name, "help|?")) {
+            }
+        case 'S':
+            if (!strcmp(cmd->name, "help|?")) {
                 monitor_find_completion_by_table(mon, cmd_table, p);
             }
             break;
-- 
1.7.1

Re: [Qemu-devel] [PATCH V2 5/7] monitor: support sub commands in auto completion

[Wenchao Xia]

于 2013-6-24 20:48, Wenchao Xia 写道:
> This patch allow auot completion work normal in sub command case,
> "info block [DEVICE]" can auto complete now, by re-enter the completion
> function. Also, original "info" is treated as a special case, now it is
> treated as a sub command group, global variable info_cmds is not used
> any more.
> 
> "help" command is still treated as a special case, since it is not a sub
> command group but want to auto complete command in root command table.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>   monitor.c |   36 ++++++++++++++++++++++++------------
>   1 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index aa641de..f364a0d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4179,10 +4179,11 @@ static const char *next_arg_type(const char *typestr)
>       return (p != NULL ? ++p : typestr);
>   }
> 
> -static void monitor_find_completion(Monitor *mon,
> -                                    const char *cmdline)
> +static void monitor_find_completion_by_table(Monitor *mon,
> +                                             const mon_cmd_t *cmd_table,
> +                                             const char *cmdline)
>   {
> -    const char *cmdname;
> +    const char *cmdname, *p;
>       char *args[MAX_ARGS];
>       int nb_args, i, len;
>       const char *ptype, *str;
> @@ -4212,12 +4213,12 @@ static void monitor_find_completion(Monitor *mon,
>           else
>               cmdname = args[0];
>           readline_set_completion_index(mon->rs, strlen(cmdname));
> -        for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
> +        for (cmd = cmd_table; cmd->name != NULL; cmd++) {
>               cmd_completion(mon, cmdname, cmd->name);
>           }
>       } else {
>           /* find the command */
> -        for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
> +        for (cmd = cmd_table; cmd->name != NULL; cmd++) {
>               if (compare_cmd(args[0], cmd->name)) {
>                   break;
>               }
> @@ -4226,6 +4227,17 @@ static void monitor_find_completion(Monitor *mon,
>               goto cleanup;
>           }
> 
> +        /* locate next valid string in original cmdline used by re-enter */
> +        p = cmdline + strlen(args[0]);
> +        while (qemu_isspace(*p)) {
> +            p++;
> +        }
> +
  Here it can't handle command start with space such as "   blk", I plan
make parse_cmdline() return additional const char **args_cmdline, which
point to cmdline correspond to each args. Just want to
mention it to save reviewer's time, I'll fix it with any other
comments:).


> +        if (cmd->sub_table) {
> +            monitor_find_completion_by_table(mon, cmd->sub_table, p);
> +            goto cleanup;
> +        }
> +
>           ptype = next_arg_type(cmd->args_type);
>           for(i = 0; i < nb_args - 2; i++) {
>               if (*ptype != '\0') {
> @@ -4252,13 +4264,7 @@ static void monitor_find_completion(Monitor *mon,
>               bdrv_iterate(block_completion_it, &mbs);
>               break;
>           case 's':
> -            /* XXX: more generic ? */
> -            if (!strcmp(cmd->name, "info")) {
> -                readline_set_completion_index(mon->rs, strlen(str));
> -                for(cmd = info_cmds; cmd->name != NULL; cmd++) {
> -                    cmd_completion(mon, str, cmd->name);
> -                }
> -            } else if (!strcmp(cmd->name, "sendkey")) {
> +            if (!strcmp(cmd->name, "sendkey")) {
>                   char *sep = strrchr(str, '-');
>                   if (sep)
>                       str = sep + 1;
> @@ -4284,6 +4290,12 @@ cleanup:
>       }
>   }
> 
> +static void monitor_find_completion(Monitor *mon,
> +                                    const char *cmdline)
> +{
> +    return monitor_find_completion_by_table(mon, mon->cmd_table, cmdline);
> +}
> +
>   static int monitor_can_read(void *opaque)
>   {
>       Monitor *mon = opaque;
> 


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 5/7] monitor: support sub commands in auto completion

[Luiz Capitulino]

On Wed, 26 Jun 2013 12:03:39 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 于 2013-6-24 20:48, Wenchao Xia 写道:
> > This patch allow auot completion work normal in sub command case,
> > "info block [DEVICE]" can auto complete now, by re-enter the completion
> > function. Also, original "info" is treated as a special case, now it is
> > treated as a sub command group, global variable info_cmds is not used
> > any more.
> > 
> > "help" command is still treated as a special case, since it is not a sub
> > command group but want to auto complete command in root command table.
> > 
> > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> > ---
> >   monitor.c |   36 ++++++++++++++++++++++++------------
> >   1 files changed, 24 insertions(+), 12 deletions(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index aa641de..f364a0d 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4179,10 +4179,11 @@ static const char *next_arg_type(const char *typestr)
> >       return (p != NULL ? ++p : typestr);
> >   }
> > 
> > -static void monitor_find_completion(Monitor *mon,
> > -                                    const char *cmdline)
> > +static void monitor_find_completion_by_table(Monitor *mon,
> > +                                             const mon_cmd_t *cmd_table,
> > +                                             const char *cmdline)
> >   {
> > -    const char *cmdname;
> > +    const char *cmdname, *p;
> >       char *args[MAX_ARGS];
> >       int nb_args, i, len;
> >       const char *ptype, *str;
> > @@ -4212,12 +4213,12 @@ static void monitor_find_completion(Monitor *mon,
> >           else
> >               cmdname = args[0];
> >           readline_set_completion_index(mon->rs, strlen(cmdname));
> > -        for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
> > +        for (cmd = cmd_table; cmd->name != NULL; cmd++) {
> >               cmd_completion(mon, cmdname, cmd->name);
> >           }
> >       } else {
> >           /* find the command */
> > -        for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
> > +        for (cmd = cmd_table; cmd->name != NULL; cmd++) {
> >               if (compare_cmd(args[0], cmd->name)) {
> >                   break;
> >               }
> > @@ -4226,6 +4227,17 @@ static void monitor_find_completion(Monitor *mon,
> >               goto cleanup;
> >           }
> > 
> > +        /* locate next valid string in original cmdline used by re-enter */
> > +        p = cmdline + strlen(args[0]);
> > +        while (qemu_isspace(*p)) {
> > +            p++;
> > +        }
> > +
>   Here it can't handle command start with space such as "   blk", I plan
> make parse_cmdline() return additional const char **args_cmdline, which
> point to cmdline correspond to each args. Just want to
> mention it to save reviewer's time, I'll fix it with any other
> comments:).

I prefer you respin it first, because it's not unusual that a single
change like this one ends up requiring more changes. Also, if the series
is good enough I can apply it w/o having to wait for another version.

> 
> 
> > +        if (cmd->sub_table) {
> > +            monitor_find_completion_by_table(mon, cmd->sub_table, p);
> > +            goto cleanup;
> > +        }
> > +
> >           ptype = next_arg_type(cmd->args_type);
> >           for(i = 0; i < nb_args - 2; i++) {
> >               if (*ptype != '\0') {
> > @@ -4252,13 +4264,7 @@ static void monitor_find_completion(Monitor *mon,
> >               bdrv_iterate(block_completion_it, &mbs);
> >               break;
> >           case 's':
> > -            /* XXX: more generic ? */
> > -            if (!strcmp(cmd->name, "info")) {
> > -                readline_set_completion_index(mon->rs, strlen(str));
> > -                for(cmd = info_cmds; cmd->name != NULL; cmd++) {
> > -                    cmd_completion(mon, str, cmd->name);
> > -                }
> > -            } else if (!strcmp(cmd->name, "sendkey")) {
> > +            if (!strcmp(cmd->name, "sendkey")) {
> >                   char *sep = strrchr(str, '-');
> >                   if (sep)
> >                       str = sep + 1;
> > @@ -4284,6 +4290,12 @@ cleanup:
> >       }
> >   }
> > 
> > +static void monitor_find_completion(Monitor *mon,
> > +                                    const char *cmdline)
> > +{
> > +    return monitor_find_completion_by_table(mon, mon->cmd_table, cmdline);
> > +}
> > +
> >   static int monitor_can_read(void *opaque)
> >   {
> >       Monitor *mon = opaque;
> > 
> 
> 

Re: [Qemu-devel] [PATCH V2 5/7] monitor: support sub commands in auto completion

[Wenchao Xia]

于 2013-6-27 0:03, Luiz Capitulino 写道:
> On Wed, 26 Jun 2013 12:03:39 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> 于 2013-6-24 20:48, Wenchao Xia 写道:
>>> This patch allow auot completion work normal in sub command case,
>>> "info block [DEVICE]" can auto complete now, by re-enter the completion
>>> function. Also, original "info" is treated as a special case, now it is
>>> treated as a sub command group, global variable info_cmds is not used
>>> any more.
>>>
>>> "help" command is still treated as a special case, since it is not a sub
>>> command group but want to auto complete command in root command table.
>>>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> ---
>>>    monitor.c |   36 ++++++++++++++++++++++++------------
>>>    1 files changed, 24 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/monitor.c b/monitor.c
>>> index aa641de..f364a0d 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -4179,10 +4179,11 @@ static const char *next_arg_type(const char *typestr)
>>>        return (p != NULL ? ++p : typestr);
>>>    }
>>>
>>> -static void monitor_find_completion(Monitor *mon,
>>> -                                    const char *cmdline)
>>> +static void monitor_find_completion_by_table(Monitor *mon,
>>> +                                             const mon_cmd_t *cmd_table,
>>> +                                             const char *cmdline)
>>>    {
>>> -    const char *cmdname;
>>> +    const char *cmdname, *p;
>>>        char *args[MAX_ARGS];
>>>        int nb_args, i, len;
>>>        const char *ptype, *str;
>>> @@ -4212,12 +4213,12 @@ static void monitor_find_completion(Monitor *mon,
>>>            else
>>>                cmdname = args[0];
>>>            readline_set_completion_index(mon->rs, strlen(cmdname));
>>> -        for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
>>> +        for (cmd = cmd_table; cmd->name != NULL; cmd++) {
>>>                cmd_completion(mon, cmdname, cmd->name);
>>>            }
>>>        } else {
>>>            /* find the command */
>>> -        for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
>>> +        for (cmd = cmd_table; cmd->name != NULL; cmd++) {
>>>                if (compare_cmd(args[0], cmd->name)) {
>>>                    break;
>>>                }
>>> @@ -4226,6 +4227,17 @@ static void monitor_find_completion(Monitor *mon,
>>>                goto cleanup;
>>>            }
>>>
>>> +        /* locate next valid string in original cmdline used by re-enter */
>>> +        p = cmdline + strlen(args[0]);
>>> +        while (qemu_isspace(*p)) {
>>> +            p++;
>>> +        }
>>> +
>>    Here it can't handle command start with space such as "   blk", I plan
>> make parse_cmdline() return additional const char **args_cmdline, which
>> point to cmdline correspond to each args. Just want to
>> mention it to save reviewer's time, I'll fix it with any other
>> comments:).
>
> I prefer you respin it first, because it's not unusual that a single
> change like this one ends up requiring more changes. Also, if the series
> is good enough I can apply it w/o having to wait for another version.
>
   OK, I'll respin. Thanks for your time.

>>
>>
>>> +        if (cmd->sub_table) {
>>> +            monitor_find_completion_by_table(mon, cmd->sub_table, p);
>>> +            goto cleanup;
>>> +        }
>>> +
>>>            ptype = next_arg_type(cmd->args_type);
>>>            for(i = 0; i < nb_args - 2; i++) {
>>>                if (*ptype != '\0') {
>>> @@ -4252,13 +4264,7 @@ static void monitor_find_completion(Monitor *mon,
>>>                bdrv_iterate(block_completion_it, &mbs);
>>>                break;
>>>            case 's':
>>> -            /* XXX: more generic ? */
>>> -            if (!strcmp(cmd->name, "info")) {
>>> -                readline_set_completion_index(mon->rs, strlen(str));
>>> -                for(cmd = info_cmds; cmd->name != NULL; cmd++) {
>>> -                    cmd_completion(mon, str, cmd->name);
>>> -                }
>>> -            } else if (!strcmp(cmd->name, "sendkey")) {
>>> +            if (!strcmp(cmd->name, "sendkey")) {
>>>                    char *sep = strrchr(str, '-');
>>>                    if (sep)
>>>                        str = sep + 1;
>>> @@ -4284,6 +4290,12 @@ cleanup:
>>>        }
>>>    }
>>>
>>> +static void monitor_find_completion(Monitor *mon,
>>> +                                    const char *cmdline)
>>> +{
>>> +    return monitor_find_completion_by_table(mon, mon->cmd_table, cmdline);
>>> +}
>>> +
>>>    static int monitor_can_read(void *opaque)
>>>    {
>>>        Monitor *mon = opaque;
>>>
>>
>>
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 0/7] monitor: support sub command group in auto completion and help

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1693 bytes --]

On 06/24/2013 06:48 AM, Wenchao Xia wrote:
> Global variable *mon_cmds and *info_cmds are not used any more, *cur_mon is not
> used in completion related functions. It is possible to create a monitor with
> different command table now, but that requirement do not exist yet, so not changed
> it to save trouble. Log command is still a special case now, may be it can be converted
> as sub group later.
> 
> Patch 1-3 make sure the functions can be re-entered safely.
> 
> V2:
>   General:
>   To discard *info_comds more graceful, help related function is modified to support
> sub command too.
>   Patch 6/7 are added to improve help related functions.
>   Patch 5: not directly return to make sure args are freed.
> 
>   Address Luiz's comments:
>   Split patch into small serial.

s/serial/series/

(remember, "serial" is a device, "series" is a sequence of patches)

>   struct mon_cmd_t was not moved into header file, instead mon_cmnd_t *cmd_table is
> added as a member in struct Monitor.
>   5/7: drop original code comments for "info" in monitor_find_completion().
> 
> Wenchao Xia (7):
>   1 monitor: discard global variable *cur_mon in completion functions
>   2 monitor: discard global variable *mon_cmds
>   3 monitor: discard global variable *info_cmds in help functions
>   4 monitor: code move for parse_cmdline()
>   5 monitor: support sub commands in auto completion
>   6 monitor: improve "help" in auto completion for sub command
>   7 monitor: improve "help" to allow show tip of single command in sub group

s/tip/details/

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V2 1/7] monitor: discard global variable *cur_mon in completion functions

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]

On 06/24/2013 06:48 AM, Wenchao Xia wrote:
> Parameter *mon is added to replace *cur_mon, and readline_completion()
> pass rs->mon as value, which should be initialized in readline_init()
> called by monitor_init(). In short, structure ReadLineState controls
> where the action would be taken now.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---

> +static void monitor_find_completion(Monitor *mon,
> +                                    const char *cmdline)
>  {
>      const char *cmdname;
>      char *args[MAX_ARGS];
>      int nb_args, i, len;
>      const char *ptype, *str;
>      const mon_cmd_t *cmd;
> +    MonitorBlockComplete mbs;
>  
>      parse_cmdline(cmdline, &nb_args, args);
>  #ifdef DEBUG_COMPLETION
>      for(i = 0; i < nb_args; i++) {
> -        monitor_printf(cur_mon, "arg%d = '%s'\n", i, (char *)args[i]);
> +        monitor_printf(mon, "arg%d = '%s'\n", i, (char *)args[i]);

Pre-existing, but that cast to (char*) looks pointless, since args[i] is
already typed as 'char *'.  Might as well remove it while touching this
line.

Looks like a decent refactor;
Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V2 2/7] monitor: discard global variable *mon_cmds

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 635 bytes --]

On 06/24/2013 06:48 AM, Wenchao Xia wrote:
> New member *cmd_table is added in structure Monitor to avoid direct usage of
> *mon_cmds. Now monitor have an associated command table, when global variable
> *info_cmds is also discarded, structure Monitor would gain full control about
> how to deal with user input.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  monitor.c |   14 +++++++++-----
>  1 files changed, 9 insertions(+), 5 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH V2 3/7] monitor: discard global variable *info_cmds in help functions

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3691 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]