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

[Qemu-devel] [PATCH V8 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.

Patch 5 replaced cur_mon with rs->mon, it is safe because:
monitor_init() calls readline_init() which initialize mon->rs, result is
mon->rs->mon == mon. Then qemu_chr_add_handlers() is called, 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 monitor instance, so it is safe to replace *cur_mon
in those functions.

Thanks for Luiz and Eric for 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.

v7:
  Address Luiz's comments:
  5/13: moved the comments why the change is safe, to cover-letter.
  8/13: use assert in free_cmdline_args(), fail when args in input exceed
the limit in parse_cmdline().

v8:
  Address Eric's comments:
  Fix typo in commit messages.

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                  |  438 ++++++++++++++++++++++++++++----------------
 readline.c                 |    5 +-
 4 files changed, 289 insertions(+), 159 deletions(-)

[Qemu-devel] [PATCH V8 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
conversion patch for monitor_find_completion().

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

diff --git a/monitor.c b/monitor.c
index 5dc0aa9..c2babba 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4004,7 +4004,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];
@@ -4022,7 +4022,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;
@@ -4136,6 +4136,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
@@ -4161,7 +4162,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 */
@@ -4202,7 +4203,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, '-');
@@ -4210,12 +4211,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 V8 02/13] monitor: avoid use of global *cur_mon in file_completion()

[Wenchao Xia]

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

diff --git a/monitor.c b/monitor.c
index c2babba..dc66dca 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4030,7 +4030,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;
@@ -4053,7 +4053,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);
@@ -4080,7 +4080,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);
@@ -4191,7 +4191,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 V8 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 monitor.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index dc66dca..0ecb70a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4086,14 +4086,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);
     }
 }
 
@@ -4137,6 +4144,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
@@ -4195,8 +4203,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 V8 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>
Reviewed-by: Eric Blake <eblake@redhat.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 0ecb70a..06b4681 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4136,20 +4136,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
 
@@ -4168,7 +4168,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);
         }
@@ -4198,7 +4198,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':
@@ -4211,7 +4211,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);
                 }
@@ -4219,12 +4219,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 V8 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.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.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 V8 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 monitor.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/monitor.c b/monitor.c
index 06b4681..dd71785 100644
--- a/monitor.c
+++ b/monitor.c
@@ -195,6 +195,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;
@@ -757,7 +758,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");
@@ -3980,7 +3981,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;
 
@@ -4169,12 +4170,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;
             }
@@ -4225,7 +4226,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);
                 }
             }
@@ -4788,6 +4789,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 V8 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 monitor.c |  191 +++++++++++++++++++++++++++++++------------------------------
 1 files changed, 98 insertions(+), 93 deletions(-)

diff --git a/monitor.c b/monitor.c
index dd71785..4a0c739 100644
--- a/monitor.c
+++ b/monitor.c
@@ -741,6 +741,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)
 {
@@ -3417,71 +3515,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.
@@ -3538,8 +3571,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];
@@ -4105,32 +4136,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 V8 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 too as paired function to free the result.

One change of this function is that, when the valid args in input
exceed the limit of MAX_ARGS, it fails now, instead of return with
MAX_ARGS of parsed args in old code. This should not impact much
since it is rare that user input many args in monitor's "help" and
auto complete scenario.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 monitor.c |   51 ++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/monitor.c b/monitor.c
index 4a0c739..c942b77 100644
--- a/monitor.c
+++ b/monitor.c
@@ -809,9 +809,33 @@ 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;
+
+    assert(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. If cmdline contains more, it will
+ *       return with failure.
+ */
+static int parse_cmdline(const char *cmdline,
+                         int *pnb_args, char **args)
 {
     const char *p;
     int nb_args, ret;
@@ -827,16 +851,21 @@ static void parse_cmdline(const char *cmdline,
             break;
         }
         if (nb_args >= MAX_ARGS) {
-            break;
+            goto fail;
         }
         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,
@@ -4152,7 +4181,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]);
@@ -4242,9 +4273,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 V8 09/13] monitor: support sub command in help

[Wenchao Xia]

The old code in help_cmd() uses global 'info_cmds' and treats it as a
special case. Actually 'info_cmds' is a sub command group of 'mon_cmds',
in order to avoid direct use of it, help_cmd() needs 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 a reentrant function. When it meets a sub
command, it simply enters the function again. Since help dumping needs 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 supports 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 monitor.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/monitor.c b/monitor.c
index c942b77..77df88d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -868,33 +868,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

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

[Luiz Capitulino]

On Fri, 26 Jul 2013 11:20:38 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> The old code in help_cmd() uses global 'info_cmds' and treats it as a
> special case. Actually 'info_cmds' is a sub command group of 'mon_cmds',
> in order to avoid direct use of it, help_cmd() needs 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 a reentrant function. When it meets a sub
> command, it simply enters the function again. Since help dumping needs 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 supports 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>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  monitor.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 53 insertions(+), 10 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index c942b77..77df88d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -868,33 +868,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;

Won't this result in a double-free if parse_cmdline() fails?

The fix is just to return instead of going to cleanup. I can do this
change if you agree and if I don't spot another bug.

>          }
>      }
> +
> +    /* 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)

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

[Wenchao Xia]

于 2013-7-30 22:51, Luiz Capitulino 写道:
> On Fri, 26 Jul 2013 11:20:38 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> The old code in help_cmd() uses global 'info_cmds' and treats it as a
>> special case. Actually 'info_cmds' is a sub command group of 'mon_cmds',
>> in order to avoid direct use of it, help_cmd() needs 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 a reentrant function. When it meets a sub
>> command, it simply enters the function again. Since help dumping needs 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 supports 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>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   monitor.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
>>   1 files changed, 53 insertions(+), 10 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index c942b77..77df88d 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -868,33 +868,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;
>
> Won't this result in a double-free if parse_cmdline() fails?
>
> The fix is just to return instead of going to cleanup. I can do this
> change if you agree and if I don't spot another bug.
>
   nb_args = 0 in that case, I think it would not free again. Still,  I
am OK to do this change, since the parse_cmdline() already does the
clean up, not need to ask caller to do it again.

>>           }
>>       }
>> +
>> +    /* 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)
>


-- 
Best Regards

Wenchao Xia

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

[Wenchao Xia]

In order to support sub command in auto completion, a reentrant function
is needed, so monitor_find_completion() is split 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 monitor.c |   65 ++++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 39 insertions(+), 26 deletions(-)

diff --git a/monitor.c b/monitor.c
index 77df88d..9b4e9b4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4214,34 +4214,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)
@@ -4249,18 +4232,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);
@@ -4305,7 +4288,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);
                 }
             }
@@ -4314,6 +4297,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 V8 11/13] monitor: support sub command in auto completion

[Wenchao Xia]

This patch allows 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 monitor.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/monitor.c b/monitor.c
index 9b4e9b4..2d6363c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4246,6 +4246,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') {
@@ -4272,13 +4278,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 V8 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>
Reviewed-by: Eric Blake <eblake@redhat.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 8c6b91a..c161fe9 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 2d6363c..99b9f2e 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.
@@ -4031,6 +4032,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);
@@ -4278,6 +4304,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 V8 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 monitor.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index 99b9f2e..754cf4c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4314,10 +4314,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 V8 00/13] monitor: support sub command group in auto completion and help

[Luiz Capitulino]

On Fri, 26 Jul 2013 11:20:29 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 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.
> 
> Patch 5 replaced cur_mon with rs->mon, it is safe because:
> monitor_init() calls readline_init() which initialize mon->rs, result is
> mon->rs->mon == mon. Then qemu_chr_add_handlers() is called, 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 monitor instance, so it is safe to replace *cur_mon
> in those functions.

I've applied this to qmp-next with the change I suggested for
patch 09/13.

> 
> Thanks for Luiz and Eric for 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.
> 
> v7:
>   Address Luiz's comments:
>   5/13: moved the comments why the change is safe, to cover-letter.
>   8/13: use assert in free_cmdline_args(), fail when args in input exceed
> the limit in parse_cmdline().
> 
> v8:
>   Address Eric's comments:
>   Fix typo in commit messages.
> 
> 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                  |  438 ++++++++++++++++++++++++++++----------------
>  readline.c                 |    5 +-
>  4 files changed, 289 insertions(+), 159 deletions(-)
> 
> 

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

[Wenchao Xia]

于 2013-7-31 0:03, Luiz Capitulino 写道:
> On Fri, 26 Jul 2013 11:20:29 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> 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.
>>
>> Patch 5 replaced cur_mon with rs->mon, it is safe because:
>> monitor_init() calls readline_init() which initialize mon->rs, result is
>> mon->rs->mon == mon. Then qemu_chr_add_handlers() is called, 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 monitor instance, so it is safe to replace *cur_mon
>> in those functions.
>
> I've applied this to qmp-next with the change I suggested for
> patch 09/13.
>
   Thanks a lot!


>>
>> Thanks for Luiz and Eric for 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.
>>
>> v7:
>>    Address Luiz's comments:
>>    5/13: moved the comments why the change is safe, to cover-letter.
>>    8/13: use assert in free_cmdline_args(), fail when args in input exceed
>> the limit in parse_cmdline().
>>
>> v8:
>>    Address Eric's comments:
>>    Fix typo in commit messages.
>>
>> 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                  |  438 ++++++++++++++++++++++++++++----------------
>>   readline.c                 |    5 +-
>>   4 files changed, 289 insertions(+), 159 deletions(-)
>>
>>
>


-- 
Best Regards

Wenchao Xia

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

[Luiz Capitulino]

On Tue, 30 Jul 2013 12:03:11 -0400
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Fri, 26 Jul 2013 11:20:29 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> 
> > 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.
> > 
> > Patch 5 replaced cur_mon with rs->mon, it is safe because:
> > monitor_init() calls readline_init() which initialize mon->rs, result is
> > mon->rs->mon == mon. Then qemu_chr_add_handlers() is called, 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 monitor instance, so it is safe to replace *cur_mon
> > in those functions.
> 
> I've applied this to qmp-next with the change I suggested for
> patch 09/13.

Unfortunately this series brakes make check:

GTESTER check-qtest-x86_64
Broken pipe
GTester: last random seed: R02S3492bd34f44dd17460851643383be44d
main-loop: WARNING: I/O thread spun for 1000 iterations
make: *** [check-qtest-x86_64] Error 1

I debugged it (with some help from Laszlo) and the problem is that it
broke the human-monitor-command command. Any usage of this command
triggers the bug like:

{ "execute": "human-monitor-command",
             "arguments": { "command-line": "info registers" } }

It seems simple to fix, I think you just have to initialize
mon->cmd_table in qmp_human_monitor_command(), but I'd recommend two
things:

 1. It's better to split off some/all QMP initialization from
    monitor_init() and call it from qmp_human_monitor_command()

 2. Can you please take the opportunity and test all commands using
    cur_mon? Just grep for it

Sorry for noticing this only now, but I only run make check before
sending a pull request (although this very likely shows you didn't
run it either).

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

[Wenchao Xia]

于 2013-8-20 22:04, Luiz Capitulino 写道:
> On Tue, 30 Jul 2013 12:03:11 -0400
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>
>> On Fri, 26 Jul 2013 11:20:29 +0800
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>
>>> 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.
>>>
>>> Patch 5 replaced cur_mon with rs->mon, it is safe because:
>>> monitor_init() calls readline_init() which initialize mon->rs, result is
>>> mon->rs->mon == mon. Then qemu_chr_add_handlers() is called, 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 monitor instance, so it is safe to replace *cur_mon
>>> in those functions.
>>
>> I've applied this to qmp-next with the change I suggested for
>> patch 09/13.
>
> Unfortunately this series brakes make check:
>
> GTESTER check-qtest-x86_64
> Broken pipe
> GTester: last random seed: R02S3492bd34f44dd17460851643383be44d
> main-loop: WARNING: I/O thread spun for 1000 iterations
> make: *** [check-qtest-x86_64] Error 1
>
> I debugged it (with some help from Laszlo) and the problem is that it
> broke the human-monitor-command command. Any usage of this command
> triggers the bug like:
>
> { "execute": "human-monitor-command",
>               "arguments": { "command-line": "info registers" } }
>
> It seems simple to fix, I think you just have to initialize
> mon->cmd_table in qmp_human_monitor_command(), but I'd recommend two
> things:
>
>   1. It's better to split off some/all QMP initialization from
>      monitor_init() and call it from qmp_human_monitor_command()
>
>   2. Can you please take the opportunity and test all commands using
>      cur_mon? Just grep for it
>
> Sorry for noticing this only now, but I only run make check before
> sending a pull request (although this very likely shows you didn't
> run it either).
>
   My bad that not ran make check before, will fix and retry, sorry for
the trouble.

-- 
Best Regards

Wenchao Xia

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

[Wenchao Xia]

于 2013-8-20 22:04, Luiz Capitulino 写道:
> On Tue, 30 Jul 2013 12:03:11 -0400
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>
>> On Fri, 26 Jul 2013 11:20:29 +0800
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>
>>> 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.
>>>
>>> Patch 5 replaced cur_mon with rs->mon, it is safe because:
>>> monitor_init() calls readline_init() which initialize mon->rs, result is
>>> mon->rs->mon == mon. Then qemu_chr_add_handlers() is called, 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 monitor instance, so it is safe to replace *cur_mon
>>> in those functions.
>>
>> I've applied this to qmp-next with the change I suggested for
>> patch 09/13.
>
> Unfortunately this series brakes make check:
>
> GTESTER check-qtest-x86_64
> Broken pipe
> GTester: last random seed: R02S3492bd34f44dd17460851643383be44d
> main-loop: WARNING: I/O thread spun for 1000 iterations
> make: *** [check-qtest-x86_64] Error 1
>
> I debugged it (with some help from Laszlo) and the problem is that it
> broke the human-monitor-command command. Any usage of this command
> triggers the bug like:
>
> { "execute": "human-monitor-command",
>               "arguments": { "command-line": "info registers" } }
>
> It seems simple to fix, I think you just have to initialize
> mon->cmd_table in qmp_human_monitor_command(), but I'd recommend two
> things:
>
>   1. It's better to split off some/all QMP initialization from
>      monitor_init() and call it from qmp_human_monitor_command()
>
>   2. Can you please take the opportunity and test all commands using
>      cur_mon? Just grep for it
>
> Sorry for noticing this only now, but I only run make check before
> sending a pull request (although this very likely shows you didn't
> run it either).
>
   About the fd related qmp interface, to test it, send_msg() is needed,
which was not supported in python 2, but new added python 3.3. I think
there are three ways to add test cases for fd qmp APIs:
1 test only when python > 3.3.
2 python plus C: compile a .so and call it with ctypes.
3 a new test framework: pure C code to call qmp interfaces.
   Which one do you prefer?

-- 
Best Regards

Wenchao Xia

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

[Luiz Capitulino]

On Thu, 22 Aug 2013 17:16:23 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 于 2013-8-20 22:04, Luiz Capitulino 写道:
> > On Tue, 30 Jul 2013 12:03:11 -0400
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> >
> >> On Fri, 26 Jul 2013 11:20:29 +0800
> >> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>
> >>> 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.
> >>>
> >>> Patch 5 replaced cur_mon with rs->mon, it is safe because:
> >>> monitor_init() calls readline_init() which initialize mon->rs, result is
> >>> mon->rs->mon == mon. Then qemu_chr_add_handlers() is called, 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 monitor instance, so it is safe to replace *cur_mon
> >>> in those functions.
> >>
> >> I've applied this to qmp-next with the change I suggested for
> >> patch 09/13.
> >
> > Unfortunately this series brakes make check:
> >
> > GTESTER check-qtest-x86_64
> > Broken pipe
> > GTester: last random seed: R02S3492bd34f44dd17460851643383be44d
> > main-loop: WARNING: I/O thread spun for 1000 iterations
> > make: *** [check-qtest-x86_64] Error 1
> >
> > I debugged it (with some help from Laszlo) and the problem is that it
> > broke the human-monitor-command command. Any usage of this command
> > triggers the bug like:
> >
> > { "execute": "human-monitor-command",
> >               "arguments": { "command-line": "info registers" } }
> >
> > It seems simple to fix, I think you just have to initialize
> > mon->cmd_table in qmp_human_monitor_command(), but I'd recommend two
> > things:
> >
> >   1. It's better to split off some/all QMP initialization from
> >      monitor_init() and call it from qmp_human_monitor_command()
> >
> >   2. Can you please take the opportunity and test all commands using
> >      cur_mon? Just grep for it
> >
> > Sorry for noticing this only now, but I only run make check before
> > sending a pull request (although this very likely shows you didn't
> > run it either).
> >
>    About the fd related qmp interface, to test it, send_msg() is needed,
> which was not supported in python 2, but new added python 3.3. I think
> there are three ways to add test cases for fd qmp APIs:
> 1 test only when python > 3.3.
> 2 python plus C: compile a .so and call it with ctypes.
> 3 a new test framework: pure C code to call qmp interfaces.
>    Which one do you prefer?

Can't we have a C program plus a shell script to test this? Anyway, if
this gets complicated you can skip having the test-case. This series took
a long way already and holding it because of that test-case isn't fair.

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

[Wenchao Xia]

于 2013-8-22 21:12, Luiz Capitulino 写道:
> On Thu, 22 Aug 2013 17:16:23 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> 于 2013-8-20 22:04, Luiz Capitulino 写道:
>>> On Tue, 30 Jul 2013 12:03:11 -0400
>>> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>>>
>>>> On Fri, 26 Jul 2013 11:20:29 +0800
>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>
>>>>> 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.
>>>>>
>>>>> Patch 5 replaced cur_mon with rs->mon, it is safe because:
>>>>> monitor_init() calls readline_init() which initialize mon->rs, result is
>>>>> mon->rs->mon == mon. Then qemu_chr_add_handlers() is called, 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 monitor instance, so it is safe to replace *cur_mon
>>>>> in those functions.
>>>>
>>>> I've applied this to qmp-next with the change I suggested for
>>>> patch 09/13.
>>>
>>> Unfortunately this series brakes make check:
>>>
>>> GTESTER check-qtest-x86_64
>>> Broken pipe
>>> GTester: last random seed: R02S3492bd34f44dd17460851643383be44d
>>> main-loop: WARNING: I/O thread spun for 1000 iterations
>>> make: *** [check-qtest-x86_64] Error 1
>>>
>>> I debugged it (with some help from Laszlo) and the problem is that it
>>> broke the human-monitor-command command. Any usage of this command
>>> triggers the bug like:
>>>
>>> { "execute": "human-monitor-command",
>>>                "arguments": { "command-line": "info registers" } }
>>>
>>> It seems simple to fix, I think you just have to initialize
>>> mon->cmd_table in qmp_human_monitor_command(), but I'd recommend two
>>> things:
>>>
>>>    1. It's better to split off some/all QMP initialization from
>>>       monitor_init() and call it from qmp_human_monitor_command()
>>>
>>>    2. Can you please take the opportunity and test all commands using
>>>       cur_mon? Just grep for it
>>>
>>> Sorry for noticing this only now, but I only run make check before
>>> sending a pull request (although this very likely shows you didn't
>>> run it either).
>>>
>>     About the fd related qmp interface, to test it, send_msg() is needed,
>> which was not supported in python 2, but new added python 3.3. I think
>> there are three ways to add test cases for fd qmp APIs:
>> 1 test only when python > 3.3.
>> 2 python plus C: compile a .so and call it with ctypes.
>> 3 a new test framework: pure C code to call qmp interfaces.
>>     Which one do you prefer?
>
> Can't we have a C program plus a shell script to test this? Anyway, if
> this gets complicated you can skip having the test-case. This series took
> a long way already and holding it because of that test-case isn't fair.
>
   C program can work, I tested it with currently qemu-iotest infra,
when fork() the fd wouldn't be closed in child process, so the child
C program can use the fds openned in parent python program. With shell
script I need to build up some basic infra such as vm boot up, monitor
connect, json parsing, so used qemu-iotest python infra instead of shell
script.
   Based on that, I have sent a separate series to add the test case.
http://lists.nongnu.org/archive/html/qemu-devel/2013-08/msg03978.html

-- 
Best Regards

Wenchao Xia