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

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

[Wenchao Xia]

Global variable *mon_cmds and *info_cmds are not directly 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().

v3:
  5/7: add parameter **args_cmdline in parse_cmdline() to tell next valid
parameter's position. This fix the issue in case command length in input is not
equal to its name's length such as "help|?", and the case input start with
space such as "  s". 
  7/7: better commit message.

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                  |  382 ++++++++++++++++++++++++++++----------------
 readline.c                 |    5 +-
 4 files changed, 251 insertions(+), 141 deletions(-)

[Qemu-devel] [PATCH V3 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 V3 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 V3 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 V3 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 V3 5/7] monitor: support sub commands in auto completion

[Wenchao Xia]

This patch allow auto completion work normal for 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
in 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.

parse_cmdline() takes another parameter now to tell next valid
parameter's position in original cmdline.

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

diff --git a/monitor.c b/monitor.c
index aa641de..e0a4b67 100644
--- a/monitor.c
+++ b/monitor.c
@@ -801,9 +801,24 @@ static int get_str(char *buf, int buf_size, const char **pp)
 
 #define MAX_ARGS 16
 
-/* NOTE: this parser is an approximate form of the real command parser */
+/*
+ * Parse the command line to get valid args.
+ * @cmdline: command line to be parsed.
+ * @pnb_args: location to store the number of args, must NOT be NULL.
+ * @args: location to store the args, which should be freed by caller, must
+ *        NOT be NULL.
+ * @args_cmdline: location to store char position in @cmdline correspond to
+ *                @args include '\0', can be NULL.
+ *
+ * For example, if @cmdline = " ab" and @args_cmdline != NULL, result will be:
+ * pnb_args = 1, args[0] = "ab", args_cmdline[0] = "ab",
+ * args_cmdline[1] = "\0".
+ *
+ * NOTE: this parser is an approximate form of the real command parser. Number
+ *       of args have a limit of MAX_ARGS.
+ */
 static void parse_cmdline(const char *cmdline,
-                         int *pnb_args, char **args)
+                         int *pnb_args, char **args, const char **args_cmdline)
 {
     const char *p;
     int nb_args, ret;
@@ -811,14 +826,14 @@ static void parse_cmdline(const char *cmdline,
 
     p = cmdline;
     nb_args = 0;
-    for (;;) {
+    while (nb_args < MAX_ARGS) {
         while (qemu_isspace(*p)) {
             p++;
         }
-        if (*p == '\0') {
-            break;
+        if (args_cmdline) {
+            args_cmdline[nb_args] = p;
         }
-        if (nb_args >= MAX_ARGS) {
+        if (*p == '\0') {
             break;
         }
         ret = get_str(buf, sizeof(buf), &p);
@@ -888,7 +903,7 @@ static void help_cmd(Monitor *mon, const char *name)
             return;
         }
 
-        parse_cmdline(name, &nb_args, args);
+        parse_cmdline(name, &nb_args, args, NULL);
         if (nb_args >= MAX_ARGS) {
             goto cleanup;
         }
@@ -4179,17 +4194,18 @@ 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, *args_cmdline[MAX_ARGS];
     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);
+    parse_cmdline(cmdline, &nb_args, args, args_cmdline);
 #ifdef DEBUG_COMPLETION
     for(i = 0; i < nb_args; i++) {
         monitor_printf(mon, "arg%d = '%s'\n", i, (char *)args[i]);
@@ -4212,12 +4228,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 +4242,12 @@ static void monitor_find_completion(Monitor *mon,
             goto cleanup;
         }
 
+        if (cmd->sub_table) {
+            monitor_find_completion_by_table(mon, cmd->sub_table,
+                                             args_cmdline[1]);
+            goto cleanup;
+        }
+
         ptype = next_arg_type(cmd->args_type);
         for(i = 0; i < nb_args - 2; i++) {
             if (*ptype != '\0') {
@@ -4252,13 +4274,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 +4300,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 V3 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 |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index e0a4b67..1126540 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4283,10 +4283,8 @@ 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,
+                                                 args_cmdline[1]);
             }
             break;
         default:
-- 
1.7.1

[Qemu-devel] [PATCH V3 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.
"help info block" do not tip extra parameter error now.

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 1126540..480f6a4 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.
@@ -4011,6 +4012,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);
@@ -4282,7 +4308,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,
                                                  args_cmdline[1]);
             }
-- 
1.7.1

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

[Eric Blake]

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

On 06/26/2013 09:27 PM, 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>
> ---

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

Oops, I started reviewing that before noticing you posted a v3.  At any
rate, my review of v2 still stands on this patch.  Meanwhile, I just
barely noticed that this pre-existing code might cause a -Wshadow
warning in the future if <strings.h> is ever included.  I'm not sure
what qemu's policy is on being clean against -Wshadow warnings, but I
personally tend to avoid local variables whose name might conflict with
global functions.

-- 
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 V3 1/7] monitor: discard global variable *cur_mon in completion functions

[Wenchao Xia]

于 2013-6-28 5:26, Eric Blake 写道:
> On 06/26/2013 09:27 PM, 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>
>> ---
>
>> @@ -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);
>
> Oops, I started reviewing that before noticing you posted a v3.  At any
> rate, my review of v2 still stands on this patch.  Meanwhile, I just
> barely noticed that this pre-existing code might cause a -Wshadow
> warning in the future if <strings.h> is ever included.  I'm not sure
> what qemu's policy is on being clean against -Wshadow warnings, but I
> personally tend to avoid local variables whose name might conflict with
> global functions.
>
   Thanks for reviewing. I'll fix what you mentioned in v2.

   For the -Wshadow warning, I guess you mean "file" right? It would
be a bit hard to check this warning for new patch, since this warning
now exist in many code. In my opinion, it would be better to have a
separate series to clean up this warnings first, then check new patches
for this issue.

-- 
Best Regards

Wenchao Xia

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

[Eric Blake]

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

On 06/27/2013 08:25 PM, Wenchao Xia wrote:
>>> +            readline_add_completion(mon->rs, file);
>>>           }
>>>       }
>>>       closedir(ffs);
>>
>> Oops, I started reviewing that before noticing you posted a v3.  At any
>> rate, my review of v2 still stands on this patch.  Meanwhile, I just
>> barely noticed that this pre-existing code might cause a -Wshadow
>> warning in the future if <strings.h> is ever included.  I'm not sure
>> what qemu's policy is on being clean against -Wshadow warnings, but I
>> personally tend to avoid local variables whose name might conflict with
>> global functions.
>>
>   Thanks for reviewing. I'll fix what you mentioned in v2.
> 
>   For the -Wshadow warning, I guess you mean "file" right?

No, I meant ffs.  ffs() is a global function, in scope if <strings.h> is
included; so using it as a local variable name will trigger -Wshadow
warnings from (at least some versions of) gcc.

> It would
> be a bit hard to check this warning for new patch, since this warning
> now exist in many code. In my opinion, it would be better to have a
> separate series to clean up this warnings first, then check new patches
> for this issue.

Agreed that the issue (of using ffs as a local variable name) is
pre-existing, and therefore cleanups for -Wshadow are not related to
this series and not something you need to necessarily worry about.

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