[Qemu-devel] [PATCH V6 00/13] monitor: support sub command group in auto completion and help

[Qemu-devel] [PATCH V6 00/13] monitor: support sub command group in auto completion and help

[Wenchao Xia]

This series make auto completion and help functions works normal for sub
command, by using reentrant functions. In order to do that, global variables
are not directly used in those functions any more. With this series, cmd_table
is a member of structure Monitor so it is possible to create a monitor with
different command table now, auto completion will work in that monitor. In
short, "info" is not treated as a special case now, this series ensure help
and auto complete function works normal for any sub command added in the future.

Thanks for Luiz's reviewing.

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

v4:
  Address Eric's comments:
  1/7, 2/7, 4/7: better commit title and message.
  1/7 remove useless (char *) in old code, add space around "for ()" in old code.
  3/7: separate code moving patch before usage.
  4/7: add space around "for ()" in old code, add min(nb_args, MAX_ARGS) in free
to make code stronger.

v5:
  4/7: use "a < b ? a : b" instead of macro min.

v6:
  Address Luiz's comments:
  1/13 ~ 5/13: splitted small patches.
  5/13: added commit message about the correctness of replacing of cur_mon and
test result.
  6/13: better comments in code.
  7/13: added commit message about the reason of code moving.
  8/13: new patch to improve parse_cmdline(), since it is a more generic
function now.
  9/13: reworked the commit message, better commentes in code, use
free_cmdline_args() in clean. It is a bit hard to split this patch into
smaller meaning ful ones, so kepted this patch as a relative larger one,
with better commit message.
  12/13: put case 'S' with case 's' in monitor_find_completion_by_table().
moved this patch ahead of patch 13/13.
  13/13: this patch is moved behind patch 12/13.

  Generic change:
  10/13: splitted patch which moved out the reentrant part into a separate
function, make review easier. This also avoided re-parsing the command line
which does in previous version.
  11/13: splitted patch, which simply remove usage of info_cmds and support
sub command by re-enter the function.


Wenchao Xia (13):
  1 monitor: avoid use of global *cur_mon in cmd_completion()
  2 monitor: avoid use of global *cur_mon in file_completion()
  3 monitor: avoid use of global *cur_mon in block_completion_it()
  4 monitor: avoid use of global *cur_mon in monitor_find_completion()
  5 monitor: avoid use of global *cur_mon in readline_completion()
  6 monitor: avoid direct use of global variable *mon_cmds
  7 monitor: code move for parse_cmdline()
  8 monitor: refine parse_cmdline()
  9 monitor: support sub command in help
  10 monitor: refine monitor_find_completion()
  11 monitor: support sub command in auto completion
  12 monitor: allow "help" show message for single command in sub group
  13 monitor: improve auto complete of "help" for single command in sub group

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

[Qemu-devel] [PATCH V6 01/13] monitor: avoid use of global *cur_mon in cmd_completion()

[Wenchao Xia]

A new local variable *mon is added in monitor_find_completion()
to make compile pass, which will be removed later in
convertion patch for monitor_find_completion().

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

diff --git a/monitor.c b/monitor.c
index 2ba7876..7398d00 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3996,7 +3996,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];
@@ -4014,7 +4014,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;
@@ -4128,6 +4128,7 @@ static void monitor_find_completion(const char *cmdline)
     int nb_args, i, len;
     const char *ptype, *str;
     const mon_cmd_t *cmd;
+    Monitor *mon = cur_mon;
 
     parse_cmdline(cmdline, &nb_args, args);
 #ifdef DEBUG_COMPLETION
@@ -4153,7 +4154,7 @@ static void monitor_find_completion(const char *cmdline)
             cmdname = args[0];
         readline_set_completion_index(cur_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 */
@@ -4194,7 +4195,7 @@ static void monitor_find_completion(const char *cmdline)
             if (!strcmp(cmd->name, "info")) {
                 readline_set_completion_index(cur_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, '-');
@@ -4202,12 +4203,12 @@ static void monitor_find_completion(const char *cmdline)
                     str = sep + 1;
                 readline_set_completion_index(cur_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));
                 for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
-                    cmd_completion(str, cmd->name);
+                    cmd_completion(mon, str, cmd->name);
                 }
             }
             break;
-- 
1.7.1

[Qemu-devel] [PATCH V6 02/13] monitor: avoid use of global *cur_mon in file_completion()

[Wenchao Xia]

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

diff --git a/monitor.c b/monitor.c
index 7398d00..181d5b0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4022,7 +4022,7 @@ static void cmd_completion(Monitor *mon, 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;
@@ -4045,7 +4045,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);
@@ -4072,7 +4072,7 @@ 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);
@@ -4183,7 +4183,7 @@ static void monitor_find_completion(const char *cmdline)
         case 'F':
             /* file completion */
             readline_set_completion_index(cur_mon->rs, strlen(str));
-            file_completion(str);
+            file_completion(mon, str);
             break;
         case 'B':
             /* block device name completion */
-- 
1.7.1

[Qemu-devel] [PATCH V6 03/13] monitor: avoid use of global *cur_mon in block_completion_it()

[Wenchao Xia]

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

diff --git a/monitor.c b/monitor.c
index 181d5b0..68f281e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4078,14 +4078,21 @@ static void file_completion(Monitor *mon, const char *input)
     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);
     }
 }
 
@@ -4129,6 +4136,7 @@ static void monitor_find_completion(const char *cmdline)
     const char *ptype, *str;
     const mon_cmd_t *cmd;
     Monitor *mon = cur_mon;
+    MonitorBlockComplete mbs;
 
     parse_cmdline(cmdline, &nb_args, args);
 #ifdef DEBUG_COMPLETION
@@ -4187,8 +4195,10 @@ static void monitor_find_completion(const char *cmdline)
             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 ? */
-- 
1.7.1

[Qemu-devel] [PATCH V6 04/13] monitor: avoid use of global *cur_mon in monitor_find_completion()

[Wenchao Xia]

Parameter *mon is added, and local variable *mon added in previous patch
is removed. The caller readline_completion(), pass rs->mon as value, which
should be initialized in readline_init() called by monitor_init().

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 include/monitor/readline.h |    3 ++-
 monitor.c                  |   18 +++++++++---------
 readline.c                 |    2 +-
 3 files changed, 12 insertions(+), 11 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 68f281e..c1e969c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4128,20 +4128,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;
-    Monitor *mon = cur_mon;
     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]);
+    for (i = 0; i < nb_args; i++) {
+        monitor_printf(mon, "arg%d = '%s'\n", i, args[i]);
     }
 #endif
 
@@ -4160,7 +4160,7 @@ 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(mon, cmdname, cmd->name);
         }
@@ -4190,7 +4190,7 @@ static void monitor_find_completion(const char *cmdline)
         switch(*ptype) {
         case 'F':
             /* file completion */
-            readline_set_completion_index(cur_mon->rs, strlen(str));
+            readline_set_completion_index(mon->rs, strlen(str));
             file_completion(mon, str);
             break;
         case 'B':
@@ -4203,7 +4203,7 @@ static void monitor_find_completion(const char *cmdline)
         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(mon, str, cmd->name);
                 }
@@ -4211,12 +4211,12 @@ static void monitor_find_completion(const char *cmdline)
                 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(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(mon, str, cmd->name);
                 }
diff --git a/readline.c b/readline.c
index 1c0f7ee..c91b324 100644
--- a/readline.c
+++ b/readline.c
@@ -285,7 +285,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 */
-- 
1.7.1

[Qemu-devel] [PATCH V6 05/13] monitor: avoid use of global *cur_mon in readline_completion()

[Wenchao Xia]

Now all completion functions do not use *cur_mon any more, instead
they use rs->mon. In short, structure ReadLineState decide where
the complete action would be taken now.

Tested with the case that qemu have two telnet monitors, auto
completion function works normal.

Take a better look at monitor_init(), it calls readline_init() which
initialize mon->rs, result is mon->rs->mon == mon. Then it calls
qemu_chr_add_handlers(), which make monitor_read() function
take *mon as its opaque. Later, when user input, monitor_read()
is called, where cur_mon is set to *mon by "cur_mon = opaque".
If qemu's monitors run in one thread, then later in
readline_handle_byte() and readline_comletion(), cur_mon is actually
equal to rs->mon, in another word it points to the actuall monitor
instance, so it is safe to replace *cur_mon in those functions.

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

diff --git a/readline.c b/readline.c
index c91b324..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;
 
@@ -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 V6 06/13] monitor: avoid direct use of 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 c1e969c..2430572 100644
--- a/monitor.c
+++ b/monitor.c
@@ -194,6 +194,7 @@ struct Monitor {
     CPUState *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");
@@ -3972,7 +3973,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;
 
@@ -4161,12 +4162,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;
             }
@@ -4217,7 +4218,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);
                 }
             }
@@ -4780,6 +4781,9 @@ void monitor_init(CharDriverState *chr, int flags)
     if (!default_mon || (flags & MONITOR_IS_DEFAULT))
         default_mon = mon;
 
+    /* Use *mon_cmds by default. */
+    mon->cmd_table = mon_cmds;
+
     sortcmdlist();
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH V6 07/13] monitor: code move for parse_cmdline()

[Wenchao Xia]

help_cmd() need this function later, so move it. get_str() is called by
parse_cmdline() so it is moved also. Some code style error reported by
check script, is also fixed.

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

diff --git a/monitor.c b/monitor.c
index 2430572..db63223 100644
--- a/monitor.c
+++ b/monitor.c
@@ -733,6 +733,104 @@ 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)
+{
+    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(Monitor *mon, const mon_cmd_t *cmds,
                           const char *prefix, const char *name)
 {
@@ -3409,71 +3507,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.
@@ -3530,8 +3563,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];
@@ -4097,32 +4128,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 V6 08/13] monitor: refine parse_cmdline()

[Wenchao Xia]

Since this function will be used by help_cmd() later, so improve
it to make it more generic and easier to use. free_cmdline_args()
is added to as paired function to free the result.

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

diff --git a/monitor.c b/monitor.c
index db63223..2d4f699 100644
--- a/monitor.c
+++ b/monitor.c
@@ -801,9 +801,31 @@ 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 */
-static void parse_cmdline(const char *cmdline,
-                          int *pnb_args, char **args)
+static void free_cmdline_args(char **args, int nb_args)
+{
+    int i;
+
+    nb_args = nb_args < MAX_ARGS ? nb_args : MAX_ARGS;
+    for (i = 0; i < nb_args; i++) {
+        g_free(args[i]);
+    }
+
+}
+
+/*
+ * 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.
+ *
+ * Returns 0 on success, negative on failure.
+ *
+ * NOTE: this parser is an approximate form of the real command parser. Number
+ *       of args have a limit of MAX_ARGS.
+ */
+static int parse_cmdline(const char *cmdline,
+                         int *pnb_args, char **args)
 {
     const char *p;
     int nb_args, ret;
@@ -811,24 +833,26 @@ 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 (nb_args >= MAX_ARGS) {
-            break;
-        }
         ret = get_str(buf, sizeof(buf), &p);
-        args[nb_args] = g_strdup(buf);
-        nb_args++;
         if (ret < 0) {
-            break;
+            goto fail;
         }
+        args[nb_args] = g_strdup(buf);
+        nb_args++;
     }
     *pnb_args = nb_args;
+    return 0;
+
+ fail:
+    free_cmdline_args(args, nb_args);
+    return -1;
 }
 
 static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
@@ -4144,7 +4168,9 @@ static void monitor_find_completion(Monitor *mon,
     const mon_cmd_t *cmd;
     MonitorBlockComplete mbs;
 
-    parse_cmdline(cmdline, &nb_args, args);
+    if (parse_cmdline(cmdline, &nb_args, args) < 0) {
+        return;
+    }
 #ifdef DEBUG_COMPLETION
     for (i = 0; i < nb_args; i++) {
         monitor_printf(mon, "arg%d = '%s'\n", i, args[i]);
@@ -4234,9 +4260,7 @@ static void monitor_find_completion(Monitor *mon,
     }
 
 cleanup:
-    for (i = 0; i < nb_args; i++) {
-        g_free(args[i]);
-    }
+    free_cmdline_args(args, nb_args);
 }
 
 static int monitor_can_read(void *opaque)
-- 
1.7.1

[Qemu-devel] [PATCH V6 09/13] monitor: support sub command in help

[Wenchao Xia]

The old code in help_cmd() use global 'info_cmds' and treat it as a
special case. Actually 'info_cmds' is an sub command group of 'mon_cmds',
in order to avoid direct use of it, help_cmd() need to change its work
mechanism to support sub command and not treat it as a special case
any more.

To support sub command, help_cmd() will first parse the input and then call
help_cmd_dump(), which works as an reentrant function. When it mets sub
command, it simply re-enter the function again. Since help dumping need to
know whole input to printf full help message include prefix, for example,
"help info block" need to printf prefix "info", so help_cmd_dump() takes all
args from input and extra parameter arg_index to identify the progress.
Another function help_cmd_dump_one() is introduced to printf the prefix
and command's help message.

Now help support sub command, so later if another sub command group is
added in any depth, help will automatically work for it. Still "help info
block" will show error since command parser reject additional parameter,
which can be improved later. "log" is still treated as a special case.

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

diff --git a/monitor.c b/monitor.c
index 2d4f699..c7667c3 100644
--- a/monitor.c
+++ b/monitor.c
@@ -855,33 +855,76 @@ static int parse_cmdline(const char *cmdline,
     return -1;
 }
 
+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);
+}
+
+/* @args[@arg_index] is the valid command need to find in @cmds */
 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;
 
-    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);
+    /* No valid arg need to compare with, dump all in *cmds */
+    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 (compare_cmd(args[arg_index], cmd->name)) {
+            if (cmd->sub_table) {
+                /* continue with next arg */
+                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;
+
+    /* 1. parse user input */
+    if (name) {
+        /* special case for log, directly dump and return */
+        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;
+        }
+
+        if (parse_cmdline(name, &nb_args, args) < 0) {
+            goto cleanup;
         }
     }
+
+    /* 2. dump the contents according to parsed args */
+    help_cmd_dump(mon, mon->cmd_table, args, nb_args, 0);
+
+cleanup:
+    free_cmdline_args(args, nb_args);
 }
 
 static void do_help_cmd(Monitor *mon, const QDict *qdict)
-- 
1.7.1

[Qemu-devel] [PATCH V6 10/13] monitor: refine monitor_find_completion()

[Wenchao Xia]

In order to support sub command in auto completion, an reentrant function
is needed, so monitor_find_completion() is splitted into two parts. The
first part does parsing of user input which need to be done only once,
the second part does the auto completion job according to the parsing
result, which contains the necessary code to support sub command and
works as the reentrant function. The global "info_cmds" is still used
in second part, which will be replaced by sub command code later.

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

diff --git a/monitor.c b/monitor.c
index c7667c3..2721443 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4201,34 +4201,17 @@ 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,
+                                             char **args,
+                                             int nb_args)
 {
     const char *cmdname;
-    char *args[MAX_ARGS];
-    int nb_args, i, len;
+    int i;
     const char *ptype, *str;
     const mon_cmd_t *cmd;
     MonitorBlockComplete mbs;
 
-    if (parse_cmdline(cmdline, &nb_args, args) < 0) {
-        return;
-    }
-#ifdef DEBUG_COMPLETION
-    for (i = 0; i < nb_args; i++) {
-        monitor_printf(mon, "arg%d = '%s'\n", i, args[i]);
-    }
-#endif
-
-    /* if the line ends with a space, it means we want to complete the
-       next arg */
-    len = strlen(cmdline);
-    if (len > 0 && qemu_isspace(cmdline[len - 1])) {
-        if (nb_args >= MAX_ARGS) {
-            goto cleanup;
-        }
-        args[nb_args++] = g_strdup("");
-    }
     if (nb_args <= 1) {
         /* command completion */
         if (nb_args == 0)
@@ -4236,18 +4219,18 @@ 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;
             }
         }
         if (!cmd->name) {
-            goto cleanup;
+            return;
         }
 
         ptype = next_arg_type(cmd->args_type);
@@ -4292,7 +4275,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->cmd_table; cmd->name != NULL; cmd++) {
+                for (cmd = cmd_table; cmd->name != NULL; cmd++) {
                     cmd_completion(mon, str, cmd->name);
                 }
             }
@@ -4301,6 +4284,36 @@ static void monitor_find_completion(Monitor *mon,
             break;
         }
     }
+}
+
+static void monitor_find_completion(Monitor *mon,
+                                    const char *cmdline)
+{
+    char *args[MAX_ARGS];
+    int nb_args, len;
+
+    /* 1. parse the cmdline */
+    if (parse_cmdline(cmdline, &nb_args, args) < 0) {
+        return;
+    }
+#ifdef DEBUG_COMPLETION
+    for (i = 0; i < nb_args; i++) {
+        monitor_printf(mon, "arg%d = '%s'\n", i, args[i]);
+    }
+#endif
+
+    /* if the line ends with a space, it means we want to complete the
+       next arg */
+    len = strlen(cmdline);
+    if (len > 0 && qemu_isspace(cmdline[len - 1])) {
+        if (nb_args >= MAX_ARGS) {
+            goto cleanup;
+        }
+        args[nb_args++] = g_strdup("");
+    }
+
+    /* 2. auto complete according to args */
+    monitor_find_completion_by_table(mon, mon->cmd_table, args, nb_args);
 
 cleanup:
     free_cmdline_args(args, nb_args);
-- 
1.7.1

[Qemu-devel] [PATCH V6 11/13] monitor: support sub command 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. In original code "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 |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/monitor.c b/monitor.c
index 2721443..9d3a61f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4233,6 +4233,12 @@ static void monitor_find_completion_by_table(Monitor *mon,
             return;
         }
 
+        if (cmd->sub_table) {
+            /* do the job again */
+            return monitor_find_completion_by_table(mon, cmd->sub_table,
+                                                    &args[1], nb_args - 1);
+        }
+
         ptype = next_arg_type(cmd->args_type);
         for(i = 0; i < nb_args - 2; i++) {
             if (*ptype != '\0') {
@@ -4259,13 +4265,7 @@ static void monitor_find_completion_by_table(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;
-- 
1.7.1

[Qemu-devel] [PATCH V6 12/13] monitor: allow "help" show message for single command in sub group

[Wenchao Xia]

A new parameter type 'S' is introduced to allow user input any string.
"help info block" works normal now.

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

diff --git a/hmp-commands.hx b/hmp-commands.hx
index d1cdcfb..3910db2 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 9d3a61f..52fb8c7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -83,6 +83,7 @@
  * 'F'          filename
  * 'B'          block device name
  * 's'          string (accept optional quote)
+ * 'S'          it just appends the rest of the 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.
@@ -4018,6 +4019,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);
@@ -4265,6 +4291,7 @@ static void monitor_find_completion_by_table(Monitor *mon,
             bdrv_iterate(block_completion_it, &mbs);
             break;
         case 's':
+        case 'S':
             if (!strcmp(cmd->name, "sendkey")) {
                 char *sep = strrchr(str, '-');
                 if (sep)
-- 
1.7.1

[Qemu-devel] [PATCH V6 13/13] monitor: improve auto complete of "help" for single command in sub group

[Wenchao Xia]

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

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 52fb8c7..68041e6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4301,10 +4301,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 = cmd_table; cmd->name != NULL; cmd++) {
-                    cmd_completion(mon, str, cmd->name);
-                }
+                monitor_find_completion_by_table(mon, cmd_table,
+                                                 &args[1], nb_args - 1);
             }
             break;
         default:
-- 
1.7.1

Re: [Qemu-devel] [PATCH V6 05/13] monitor: avoid use of global *cur_mon in readline_completion()

[Luiz Capitulino]

On Thu, 11 Jul 2013 11:13:41 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> Now all completion functions do not use *cur_mon any more, instead
> they use rs->mon. In short, structure ReadLineState decide where
> the complete action would be taken now.
> 
> Tested with the case that qemu have two telnet monitors, auto
> completion function works normal.
> 
> Take a better look at monitor_init(), it calls readline_init() which
> initialize mon->rs, result is mon->rs->mon == mon. Then it calls
> qemu_chr_add_handlers(), which make monitor_read() function
> take *mon as its opaque. Later, when user input, monitor_read()
> is called, where cur_mon is set to *mon by "cur_mon = opaque".
> If qemu's monitors run in one thread, then later in
> readline_handle_byte() and readline_comletion(), cur_mon is actually
> equal to rs->mon, in another word it points to the actuall monitor
> instance, so it is safe to replace *cur_mon in those functions.

I find this paragraph a bit confusing and not exactly related to
this patch. I know I asked you to explain why dropping cur_mon usage
shouldn't brake things, but looks like you don't have a good place
to have that info.

> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  readline.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/readline.c b/readline.c
> index c91b324..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;
>  
> @@ -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++) {

Re: [Qemu-devel] [PATCH V6 08/13] monitor: refine parse_cmdline()

[Luiz Capitulino]

On Thu, 11 Jul 2013 11:13:44 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> Since this function will be used by help_cmd() later, so improve
> it to make it more generic and easier to use. free_cmdline_args()
> is added to as paired function to free the result.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  monitor.c |   52 ++++++++++++++++++++++++++++++++++++++--------------
>  1 files changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index db63223..2d4f699 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -801,9 +801,31 @@ 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 */
> -static void parse_cmdline(const char *cmdline,
> -                          int *pnb_args, char **args)
> +static void free_cmdline_args(char **args, int nb_args)
> +{
> +    int i;
> +
> +    nb_args = nb_args < MAX_ARGS ? nb_args : MAX_ARGS;

Why is this needed? nb_args is guaranteed to be at most MAX_ARGS,
isn't it? If you really want to ensure it, then you can assert() it.

> +    for (i = 0; i < nb_args; i++) {
> +        g_free(args[i]);
> +    }
> +
> +}
> +
> +/*
> + * 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.
> + *
> + * Returns 0 on success, negative on failure.
> + *
> + * NOTE: this parser is an approximate form of the real command parser. Number
> + *       of args have a limit of MAX_ARGS.
> + */
> +static int parse_cmdline(const char *cmdline,
> +                         int *pnb_args, char **args)
>  {
>      const char *p;
>      int nb_args, ret;
> @@ -811,24 +833,26 @@ static void parse_cmdline(const char *cmdline,
>  
>      p = cmdline;
>      nb_args = 0;
> -    for (;;) {
> +    while (nb_args < MAX_ARGS) {

I think it would be better to fail if nb_args > MAX_ARGS. Well, ideally
we shouldn't have any artificial limit, but I'd guess that dropping
MAX_ARGS goes a bit to far for this series' scope.

>          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;
> +            goto fail;
>          }
> +        args[nb_args] = g_strdup(buf);
> +        nb_args++;
>      }
>      *pnb_args = nb_args;
> +    return 0;
> +
> + fail:
> +    free_cmdline_args(args, nb_args);
> +    return -1;
>  }
>  
>  static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
> @@ -4144,7 +4168,9 @@ static void monitor_find_completion(Monitor *mon,
>      const mon_cmd_t *cmd;
>      MonitorBlockComplete mbs;
>  
> -    parse_cmdline(cmdline, &nb_args, args);
> +    if (parse_cmdline(cmdline, &nb_args, args) < 0) {
> +        return;
> +    }
>  #ifdef DEBUG_COMPLETION
>      for (i = 0; i < nb_args; i++) {
>          monitor_printf(mon, "arg%d = '%s'\n", i, args[i]);
> @@ -4234,9 +4260,7 @@ static void monitor_find_completion(Monitor *mon,
>      }
>  
>  cleanup:
> -    for (i = 0; i < nb_args; i++) {
> -        g_free(args[i]);
> -    }
> +    free_cmdline_args(args, nb_args);
>  }
>  
>  static int monitor_can_read(void *opaque)

Re: [Qemu-devel] [PATCH V6 05/13] monitor: avoid use of global *cur_mon in readline_completion()

[Wenchao Xia]

于 2013-7-18 3:34, Luiz Capitulino 写道:
> On Thu, 11 Jul 2013 11:13:41 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> Now all completion functions do not use *cur_mon any more, instead
>> they use rs->mon. In short, structure ReadLineState decide where
>> the complete action would be taken now.
>>
>> Tested with the case that qemu have two telnet monitors, auto
>> completion function works normal.
>>
>> Take a better look at monitor_init(), it calls readline_init() which
>> initialize mon->rs, result is mon->rs->mon == mon. Then it calls
>> qemu_chr_add_handlers(), which make monitor_read() function
>> take *mon as its opaque. Later, when user input, monitor_read()
>> is called, where cur_mon is set to *mon by "cur_mon = opaque".
>> If qemu's monitors run in one thread, then later in
>> readline_handle_byte() and readline_comletion(), cur_mon is actually
>> equal to rs->mon, in another word it points to the actuall monitor
>> instance, so it is safe to replace *cur_mon in those functions.
>
> I find this paragraph a bit confusing and not exactly related to
> this patch. I know I asked you to explain why dropping cur_mon usage
> shouldn't brake things, but looks like you don't have a good place
> to have that info.
>
>>
   How about refine the message as:
"Now usage of cur_mon is dropped in readline_completion(), it is safe
because:
.....
"
?

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V6 08/13] monitor: refine parse_cmdline()

[Wenchao Xia]

于 2013-7-18 3:39, Luiz Capitulino 写道:
> On Thu, 11 Jul 2013 11:13:44 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> Since this function will be used by help_cmd() later, so improve
>> it to make it more generic and easier to use. free_cmdline_args()
>> is added to as paired function to free the result.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   monitor.c |   52 ++++++++++++++++++++++++++++++++++++++--------------
>>   1 files changed, 38 insertions(+), 14 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index db63223..2d4f699 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -801,9 +801,31 @@ 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 */
>> -static void parse_cmdline(const char *cmdline,
>> -                          int *pnb_args, char **args)
>> +static void free_cmdline_args(char **args, int nb_args)
>> +{
>> +    int i;
>> +
>> +    nb_args = nb_args < MAX_ARGS ? nb_args : MAX_ARGS;
>
> Why is this needed? nb_args is guaranteed to be at most MAX_ARGS,
> isn't it? If you really want to ensure it, then you can assert() it.
>
   I'll use assert().

>> +    for (i = 0; i < nb_args; i++) {
>> +        g_free(args[i]);
>> +    }
>> +
>> +}
>> +
>> +/*
>> + * 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.
>> + *
>> + * Returns 0 on success, negative on failure.
>> + *
>> + * NOTE: this parser is an approximate form of the real command parser. Number
>> + *       of args have a limit of MAX_ARGS.
>> + */
>> +static int parse_cmdline(const char *cmdline,
>> +                         int *pnb_args, char **args)
>>   {
>>       const char *p;
>>       int nb_args, ret;
>> @@ -811,24 +833,26 @@ static void parse_cmdline(const char *cmdline,
>>
>>       p = cmdline;
>>       nb_args = 0;
>> -    for (;;) {
>> +    while (nb_args < MAX_ARGS) {
>
> I think it would be better to fail if nb_args > MAX_ARGS. Well, ideally
   will fail the function if nb_args > MAX_ARGS in next version.

> we shouldn't have any artificial limit, but I'd guess that dropping
> MAX_ARGS goes a bit to far for this series' scope.
>
>>           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;
>> +            goto fail;
>>           }
>> +        args[nb_args] = g_strdup(buf);
>> +        nb_args++;
>>       }
>>       *pnb_args = nb_args;
>> +    return 0;
>> +
>> + fail:
>> +    free_cmdline_args(args, nb_args);
>> +    return -1;
>>   }
>>
>>   static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
>> @@ -4144,7 +4168,9 @@ static void monitor_find_completion(Monitor *mon,
>>       const mon_cmd_t *cmd;
>>       MonitorBlockComplete mbs;
>>
>> -    parse_cmdline(cmdline, &nb_args, args);
>> +    if (parse_cmdline(cmdline, &nb_args, args) < 0) {
>> +        return;
>> +    }
>>   #ifdef DEBUG_COMPLETION
>>       for (i = 0; i < nb_args; i++) {
>>           monitor_printf(mon, "arg%d = '%s'\n", i, args[i]);
>> @@ -4234,9 +4260,7 @@ static void monitor_find_completion(Monitor *mon,
>>       }
>>
>>   cleanup:
>> -    for (i = 0; i < nb_args; i++) {
>> -        g_free(args[i]);
>> -    }
>> +    free_cmdline_args(args, nb_args);
>>   }
>>
>>   static int monitor_can_read(void *opaque)
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V6 05/13] monitor: avoid use of global *cur_mon in readline_completion()

[Luiz Capitulino]

On Thu, 18 Jul 2013 09:55:51 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 于 2013-7-18 3:34, Luiz Capitulino 写道:
> > On Thu, 11 Jul 2013 11:13:41 +0800
> > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >
> >> Now all completion functions do not use *cur_mon any more, instead
> >> they use rs->mon. In short, structure ReadLineState decide where
> >> the complete action would be taken now.
> >>
> >> Tested with the case that qemu have two telnet monitors, auto
> >> completion function works normal.
> >>
> >> Take a better look at monitor_init(), it calls readline_init() which
> >> initialize mon->rs, result is mon->rs->mon == mon. Then it calls
> >> qemu_chr_add_handlers(), which make monitor_read() function
> >> take *mon as its opaque. Later, when user input, monitor_read()
> >> is called, where cur_mon is set to *mon by "cur_mon = opaque".
> >> If qemu's monitors run in one thread, then later in
> >> readline_handle_byte() and readline_comletion(), cur_mon is actually
> >> equal to rs->mon, in another word it points to the actuall monitor
> >> instance, so it is safe to replace *cur_mon in those functions.
> >
> > I find this paragraph a bit confusing and not exactly related to
> > this patch. I know I asked you to explain why dropping cur_mon usage
> > shouldn't brake things, but looks like you don't have a good place
> > to have that info.
> >
> >>
>    How about refine the message as:
> "Now usage of cur_mon is dropped in readline_completion(), it is safe
> because:
> .....
> "
> ?

I think you have to just describe what this commit does.