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

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

[Wenchao Xia]

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

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

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

  Address Luiz's comments:
  Split patch into small 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.

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

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

[Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions

[Wenchao Xia]

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

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/monitor/readline.h |    3 +-
 monitor.c                  |   55 ++++++++++++++++++++++++++-----------------
 readline.c                 |    5 +--
 3 files changed, 37 insertions(+), 26 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 9be515c..29e3a95 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3998,7 +3998,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];
@@ -4016,7 +4016,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;
@@ -4024,7 +4024,7 @@ static void cmd_completion(const char *name, const char *list)
     }
 }
 
-static void file_completion(const char *input)
+static void file_completion(Monitor *mon, const char *input)
 {
     DIR *ffs;
     struct dirent *d;
@@ -4047,7 +4047,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);
@@ -4074,20 +4074,27 @@ static void file_completion(const char *input)
             if (stat(file, &sb) == 0 && S_ISDIR(sb.st_mode)) {
                 pstrcat(file, sizeof(file), "/");
             }
-            readline_add_completion(cur_mon->rs, file);
+            readline_add_completion(mon->rs, file);
         }
     }
     closedir(ffs);
 }
 
+typedef struct MonitorBlockComplete {
+    Monitor *mon;
+    const char *input;
+} MonitorBlockComplete;
+
 static void block_completion_it(void *opaque, BlockDriverState *bs)
 {
     const char *name = bdrv_get_device_name(bs);
-    const char *input = opaque;
+    MonitorBlockComplete *mbc = opaque;
+    Monitor *mon = mbc->mon;
+    const char *input = mbc->input;
 
     if (input[0] == '\0' ||
         !strncmp(name, (char *)input, strlen(input))) {
-        readline_add_completion(cur_mon->rs, name);
+        readline_add_completion(mon->rs, name);
     }
 }
 
@@ -4123,18 +4130,20 @@ static const char *next_arg_type(const char *typestr)
     return (p != NULL ? ++p : typestr);
 }
 
-static void monitor_find_completion(const char *cmdline)
+static void monitor_find_completion(Monitor *mon,
+                                    const char *cmdline)
 {
     const char *cmdname;
     char *args[MAX_ARGS];
     int nb_args, i, len;
     const char *ptype, *str;
     const mon_cmd_t *cmd;
+    MonitorBlockComplete mbs;
 
     parse_cmdline(cmdline, &nb_args, args);
 #ifdef DEBUG_COMPLETION
-    for(i = 0; i < nb_args; i++) {
-        monitor_printf(cur_mon, "arg%d = '%s'\n", i, (char *)args[i]);
+    for (i = 0; i < nb_args; i++) {
+        monitor_printf(mon, "arg%d = '%s'\n", i, args[i]);
     }
 #endif
 
@@ -4153,9 +4162,9 @@ static void monitor_find_completion(const char *cmdline)
             cmdname = "";
         else
             cmdname = args[0];
-        readline_set_completion_index(cur_mon->rs, strlen(cmdname));
+        readline_set_completion_index(mon->rs, strlen(cmdname));
         for(cmd = mon_cmds; cmd->name != NULL; cmd++) {
-            cmd_completion(cmdname, cmd->name);
+            cmd_completion(mon, cmdname, cmd->name);
         }
     } else {
         /* find the command */
@@ -4183,33 +4192,35 @@ static void monitor_find_completion(const char *cmdline)
         switch(*ptype) {
         case 'F':
             /* file completion */
-            readline_set_completion_index(cur_mon->rs, strlen(str));
-            file_completion(str);
+            readline_set_completion_index(mon->rs, strlen(str));
+            file_completion(mon, str);
             break;
         case 'B':
             /* block device name completion */
-            readline_set_completion_index(cur_mon->rs, strlen(str));
-            bdrv_iterate(block_completion_it, (void *)str);
+            mbs.mon = mon;
+            mbs.input = str;
+            readline_set_completion_index(mon->rs, strlen(str));
+            bdrv_iterate(block_completion_it, &mbs);
             break;
         case 's':
             /* XXX: more generic ? */
             if (!strcmp(cmd->name, "info")) {
-                readline_set_completion_index(cur_mon->rs, strlen(str));
+                readline_set_completion_index(mon->rs, strlen(str));
                 for(cmd = info_cmds; cmd->name != NULL; cmd++) {
-                    cmd_completion(str, cmd->name);
+                    cmd_completion(mon, str, cmd->name);
                 }
             } else if (!strcmp(cmd->name, "sendkey")) {
                 char *sep = strrchr(str, '-');
                 if (sep)
                     str = sep + 1;
-                readline_set_completion_index(cur_mon->rs, strlen(str));
+                readline_set_completion_index(mon->rs, strlen(str));
                 for (i = 0; i < Q_KEY_CODE_MAX; i++) {
-                    cmd_completion(str, QKeyCode_lookup[i]);
+                    cmd_completion(mon, str, QKeyCode_lookup[i]);
                 }
             } else if (!strcmp(cmd->name, "help|?")) {
-                readline_set_completion_index(cur_mon->rs, strlen(str));
+                readline_set_completion_index(mon->rs, strlen(str));
                 for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
-                    cmd_completion(str, cmd->name);
+                    cmd_completion(mon, str, cmd->name);
                 }
             }
             break;
diff --git a/readline.c b/readline.c
index 1c0f7ee..abf27dd 100644
--- a/readline.c
+++ b/readline.c
@@ -276,7 +276,6 @@ void readline_set_completion_index(ReadLineState *rs, int index)
 
 static void readline_completion(ReadLineState *rs)
 {
-    Monitor *mon = cur_mon;
     int len, i, j, max_width, nb_cols, max_prefix;
     char *cmdline;
 
@@ -285,7 +284,7 @@ static void readline_completion(ReadLineState *rs)
     cmdline = g_malloc(rs->cmd_buf_index + 1);
     memcpy(cmdline, rs->cmd_buf, rs->cmd_buf_index);
     cmdline[rs->cmd_buf_index] = '\0';
-    rs->completion_finder(cmdline);
+    rs->completion_finder(rs->mon, cmdline);
     g_free(cmdline);
 
     /* no completion found */
@@ -300,7 +299,7 @@ static void readline_completion(ReadLineState *rs)
         if (len > 0 && rs->completions[0][len - 1] != '/')
             readline_insert_char(rs, ' ');
     } else {
-        monitor_printf(mon, "\n");
+        monitor_printf(rs->mon, "\n");
         max_width = 0;
         max_prefix = 0;	
         for(i = 0; i < rs->nb_completions; i++) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions

[Luiz Capitulino]

On Sat, 29 Jun 2013 11:52:55 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

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

This patch could be made easier to review if you further split it.
I'd make the following split (each item is a separate patch):

 1. Convert cmd_completion()
 2. Convert file_completion()
 3. Convert block_completion_it()
 4. Convert monitor_find_completion()
 5. Convert readline_completion()

One more comment at the bottom.

> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  include/monitor/readline.h |    3 +-
>  monitor.c                  |   55 ++++++++++++++++++++++++++-----------------
>  readline.c                 |    5 +--
>  3 files changed, 37 insertions(+), 26 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 9be515c..29e3a95 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3998,7 +3998,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];
> @@ -4016,7 +4016,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;
> @@ -4024,7 +4024,7 @@ static void cmd_completion(const char *name, const char *list)
>      }
>  }
>  
> -static void file_completion(const char *input)
> +static void file_completion(Monitor *mon, const char *input)
>  {
>      DIR *ffs;
>      struct dirent *d;
> @@ -4047,7 +4047,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);
> @@ -4074,20 +4074,27 @@ static void file_completion(const char *input)
>              if (stat(file, &sb) == 0 && S_ISDIR(sb.st_mode)) {
>                  pstrcat(file, sizeof(file), "/");
>              }
> -            readline_add_completion(cur_mon->rs, file);
> +            readline_add_completion(mon->rs, file);
>          }
>      }
>      closedir(ffs);
>  }
>  
> +typedef struct MonitorBlockComplete {
> +    Monitor *mon;
> +    const char *input;
> +} MonitorBlockComplete;
> +
>  static void block_completion_it(void *opaque, BlockDriverState *bs)
>  {
>      const char *name = bdrv_get_device_name(bs);
> -    const char *input = opaque;
> +    MonitorBlockComplete *mbc = opaque;
> +    Monitor *mon = mbc->mon;
> +    const char *input = mbc->input;
>  
>      if (input[0] == '\0' ||
>          !strncmp(name, (char *)input, strlen(input))) {
> -        readline_add_completion(cur_mon->rs, name);
> +        readline_add_completion(mon->rs, name);
>      }
>  }
>  
> @@ -4123,18 +4130,20 @@ static const char *next_arg_type(const char *typestr)
>      return (p != NULL ? ++p : typestr);
>  }
>  
> -static void monitor_find_completion(const char *cmdline)
> +static void monitor_find_completion(Monitor *mon,
> +                                    const char *cmdline)
>  {
>      const char *cmdname;
>      char *args[MAX_ARGS];
>      int nb_args, i, len;
>      const char *ptype, *str;
>      const mon_cmd_t *cmd;
> +    MonitorBlockComplete mbs;
>  
>      parse_cmdline(cmdline, &nb_args, args);
>  #ifdef DEBUG_COMPLETION
> -    for(i = 0; i < nb_args; i++) {
> -        monitor_printf(cur_mon, "arg%d = '%s'\n", i, (char *)args[i]);
> +    for (i = 0; i < nb_args; i++) {
> +        monitor_printf(mon, "arg%d = '%s'\n", i, args[i]);
>      }
>  #endif
>  
> @@ -4153,9 +4162,9 @@ static void monitor_find_completion(const char *cmdline)
>              cmdname = "";
>          else
>              cmdname = args[0];
> -        readline_set_completion_index(cur_mon->rs, strlen(cmdname));
> +        readline_set_completion_index(mon->rs, strlen(cmdname));
>          for(cmd = mon_cmds; cmd->name != NULL; cmd++) {
> -            cmd_completion(cmdname, cmd->name);
> +            cmd_completion(mon, cmdname, cmd->name);
>          }
>      } else {
>          /* find the command */
> @@ -4183,33 +4192,35 @@ static void monitor_find_completion(const char *cmdline)
>          switch(*ptype) {
>          case 'F':
>              /* file completion */
> -            readline_set_completion_index(cur_mon->rs, strlen(str));
> -            file_completion(str);
> +            readline_set_completion_index(mon->rs, strlen(str));
> +            file_completion(mon, str);
>              break;
>          case 'B':
>              /* block device name completion */
> -            readline_set_completion_index(cur_mon->rs, strlen(str));
> -            bdrv_iterate(block_completion_it, (void *)str);
> +            mbs.mon = mon;
> +            mbs.input = str;
> +            readline_set_completion_index(mon->rs, strlen(str));
> +            bdrv_iterate(block_completion_it, &mbs);
>              break;
>          case 's':
>              /* XXX: more generic ? */
>              if (!strcmp(cmd->name, "info")) {
> -                readline_set_completion_index(cur_mon->rs, strlen(str));
> +                readline_set_completion_index(mon->rs, strlen(str));
>                  for(cmd = info_cmds; cmd->name != NULL; cmd++) {
> -                    cmd_completion(str, cmd->name);
> +                    cmd_completion(mon, str, cmd->name);
>                  }
>              } else if (!strcmp(cmd->name, "sendkey")) {
>                  char *sep = strrchr(str, '-');
>                  if (sep)
>                      str = sep + 1;
> -                readline_set_completion_index(cur_mon->rs, strlen(str));
> +                readline_set_completion_index(mon->rs, strlen(str));
>                  for (i = 0; i < Q_KEY_CODE_MAX; i++) {
> -                    cmd_completion(str, QKeyCode_lookup[i]);
> +                    cmd_completion(mon, str, QKeyCode_lookup[i]);
>                  }
>              } else if (!strcmp(cmd->name, "help|?")) {
> -                readline_set_completion_index(cur_mon->rs, strlen(str));
> +                readline_set_completion_index(mon->rs, strlen(str));
>                  for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
> -                    cmd_completion(str, cmd->name);
> +                    cmd_completion(mon, str, cmd->name);
>                  }
>              }
>              break;
> diff --git a/readline.c b/readline.c
> index 1c0f7ee..abf27dd 100644
> --- a/readline.c
> +++ b/readline.c
> @@ -276,7 +276,6 @@ void readline_set_completion_index(ReadLineState *rs, int index)
>  
>  static void readline_completion(ReadLineState *rs)
>  {
> -    Monitor *mon = cur_mon;
>      int len, i, j, max_width, nb_cols, max_prefix;
>      char *cmdline;
>  
> @@ -285,7 +284,7 @@ static void readline_completion(ReadLineState *rs)
>      cmdline = g_malloc(rs->cmd_buf_index + 1);
>      memcpy(cmdline, rs->cmd_buf, rs->cmd_buf_index);
>      cmdline[rs->cmd_buf_index] = '\0';
> -    rs->completion_finder(cmdline);
> +    rs->completion_finder(rs->mon, cmdline);
>      g_free(cmdline);
>  
>      /* no completion found */
> @@ -300,7 +299,7 @@ static void readline_completion(ReadLineState *rs)
>          if (len > 0 && rs->completions[0][len - 1] != '/')
>              readline_insert_char(rs, ' ');
>      } else {
> -        monitor_printf(mon, "\n");
> +        monitor_printf(rs->mon, "\n");
>          max_width = 0;
>          max_prefix = 0;	
>          for(i = 0; i < rs->nb_completions; i++) {

The rest of the function already uses rs->mon, so you didn't have
to patch that. However, this shows that the current code makes a
distinction between cur_mon and rs->mon. Either, there's a difference
between the two and your code is wrong *or* current code is just
redundant and your commit fixes it.

I _guess_ it's the latter, but I'm not sure.

Did you think about this? Did you test this series with multiple
monitors running at the same time?

You could pass cur_mon to readline_completion() in readline_handle_byte()
to avoid all this, but it would be preferable to clarify the matter.

This is also another benefit of having readline_completion() in a
different patch, you can (and should!) clarify this point in the commit
log along with testing results.

Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions

[Wenchao Xia]

于 2013-7-8 23:17, Luiz Capitulino 写道:
> On Sat, 29 Jun 2013 11:52:55 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> Parameter *mon is added to replace *cur_mon, and readline_completion()
>> pass rs->mon as value, which should be initialized in readline_init()
>> called by monitor_init(). In short, structure ReadLineState controls
>> where the action would be taken now.
>
> This patch could be made easier to review if you further split it.
> I'd make the following split (each item is a separate patch):
>
>   1. Convert cmd_completion()
>   2. Convert file_completion()
>   3. Convert block_completion_it()
>   4. Convert monitor_find_completion()
>   5. Convert readline_completion()
>
> One more comment at the bottom.
>
   I'll split it as above.

>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   include/monitor/readline.h |    3 +-
>>   monitor.c                  |   55 ++++++++++++++++++++++++++-----------------
>>   readline.c                 |    5 +--
>>   3 files changed, 37 insertions(+), 26 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 9be515c..29e3a95 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -3998,7 +3998,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];
>> @@ -4016,7 +4016,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;
>> @@ -4024,7 +4024,7 @@ static void cmd_completion(const char *name, const char *list)
>>       }
>>   }
>>
>> -static void file_completion(const char *input)
>> +static void file_completion(Monitor *mon, const char *input)
>>   {
>>       DIR *ffs;
>>       struct dirent *d;
>> @@ -4047,7 +4047,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);
>> @@ -4074,20 +4074,27 @@ static void file_completion(const char *input)
>>               if (stat(file, &sb) == 0 && S_ISDIR(sb.st_mode)) {
>>                   pstrcat(file, sizeof(file), "/");
>>               }
>> -            readline_add_completion(cur_mon->rs, file);
>> +            readline_add_completion(mon->rs, file);
>>           }
>>       }
>>       closedir(ffs);
>>   }
>>
>> +typedef struct MonitorBlockComplete {
>> +    Monitor *mon;
>> +    const char *input;
>> +} MonitorBlockComplete;
>> +
>>   static void block_completion_it(void *opaque, BlockDriverState *bs)
>>   {
>>       const char *name = bdrv_get_device_name(bs);
>> -    const char *input = opaque;
>> +    MonitorBlockComplete *mbc = opaque;
>> +    Monitor *mon = mbc->mon;
>> +    const char *input = mbc->input;
>>
>>       if (input[0] == '\0' ||
>>           !strncmp(name, (char *)input, strlen(input))) {
>> -        readline_add_completion(cur_mon->rs, name);
>> +        readline_add_completion(mon->rs, name);
>>       }
>>   }
>>
>> @@ -4123,18 +4130,20 @@ static const char *next_arg_type(const char *typestr)
>>       return (p != NULL ? ++p : typestr);
>>   }
>>
>> -static void monitor_find_completion(const char *cmdline)
>> +static void monitor_find_completion(Monitor *mon,
>> +                                    const char *cmdline)
>>   {
>>       const char *cmdname;
>>       char *args[MAX_ARGS];
>>       int nb_args, i, len;
>>       const char *ptype, *str;
>>       const mon_cmd_t *cmd;
>> +    MonitorBlockComplete mbs;
>>
>>       parse_cmdline(cmdline, &nb_args, args);
>>   #ifdef DEBUG_COMPLETION
>> -    for(i = 0; i < nb_args; i++) {
>> -        monitor_printf(cur_mon, "arg%d = '%s'\n", i, (char *)args[i]);
>> +    for (i = 0; i < nb_args; i++) {
>> +        monitor_printf(mon, "arg%d = '%s'\n", i, args[i]);
>>       }
>>   #endif
>>
>> @@ -4153,9 +4162,9 @@ static void monitor_find_completion(const char *cmdline)
>>               cmdname = "";
>>           else
>>               cmdname = args[0];
>> -        readline_set_completion_index(cur_mon->rs, strlen(cmdname));
>> +        readline_set_completion_index(mon->rs, strlen(cmdname));
>>           for(cmd = mon_cmds; cmd->name != NULL; cmd++) {
>> -            cmd_completion(cmdname, cmd->name);
>> +            cmd_completion(mon, cmdname, cmd->name);
>>           }
>>       } else {
>>           /* find the command */
>> @@ -4183,33 +4192,35 @@ static void monitor_find_completion(const char *cmdline)
>>           switch(*ptype) {
>>           case 'F':
>>               /* file completion */
>> -            readline_set_completion_index(cur_mon->rs, strlen(str));
>> -            file_completion(str);
>> +            readline_set_completion_index(mon->rs, strlen(str));
>> +            file_completion(mon, str);
>>               break;
>>           case 'B':
>>               /* block device name completion */
>> -            readline_set_completion_index(cur_mon->rs, strlen(str));
>> -            bdrv_iterate(block_completion_it, (void *)str);
>> +            mbs.mon = mon;
>> +            mbs.input = str;
>> +            readline_set_completion_index(mon->rs, strlen(str));
>> +            bdrv_iterate(block_completion_it, &mbs);
>>               break;
>>           case 's':
>>               /* XXX: more generic ? */
>>               if (!strcmp(cmd->name, "info")) {
>> -                readline_set_completion_index(cur_mon->rs, strlen(str));
>> +                readline_set_completion_index(mon->rs, strlen(str));
>>                   for(cmd = info_cmds; cmd->name != NULL; cmd++) {
>> -                    cmd_completion(str, cmd->name);
>> +                    cmd_completion(mon, str, cmd->name);
>>                   }
>>               } else if (!strcmp(cmd->name, "sendkey")) {
>>                   char *sep = strrchr(str, '-');
>>                   if (sep)
>>                       str = sep + 1;
>> -                readline_set_completion_index(cur_mon->rs, strlen(str));
>> +                readline_set_completion_index(mon->rs, strlen(str));
>>                   for (i = 0; i < Q_KEY_CODE_MAX; i++) {
>> -                    cmd_completion(str, QKeyCode_lookup[i]);
>> +                    cmd_completion(mon, str, QKeyCode_lookup[i]);
>>                   }
>>               } else if (!strcmp(cmd->name, "help|?")) {
>> -                readline_set_completion_index(cur_mon->rs, strlen(str));
>> +                readline_set_completion_index(mon->rs, strlen(str));
>>                   for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
>> -                    cmd_completion(str, cmd->name);
>> +                    cmd_completion(mon, str, cmd->name);
>>                   }
>>               }
>>               break;
>> diff --git a/readline.c b/readline.c
>> index 1c0f7ee..abf27dd 100644
>> --- a/readline.c
>> +++ b/readline.c
>> @@ -276,7 +276,6 @@ void readline_set_completion_index(ReadLineState *rs, int index)
>>
>>   static void readline_completion(ReadLineState *rs)
>>   {
>> -    Monitor *mon = cur_mon;
>>       int len, i, j, max_width, nb_cols, max_prefix;
>>       char *cmdline;
>>
>> @@ -285,7 +284,7 @@ static void readline_completion(ReadLineState *rs)
>>       cmdline = g_malloc(rs->cmd_buf_index + 1);
>>       memcpy(cmdline, rs->cmd_buf, rs->cmd_buf_index);
>>       cmdline[rs->cmd_buf_index] = '\0';
>> -    rs->completion_finder(cmdline);
>> +    rs->completion_finder(rs->mon, cmdline);
>>       g_free(cmdline);
>>
>>       /* no completion found */
>> @@ -300,7 +299,7 @@ static void readline_completion(ReadLineState *rs)
>>           if (len > 0 && rs->completions[0][len - 1] != '/')
>>               readline_insert_char(rs, ' ');
>>       } else {
>> -        monitor_printf(mon, "\n");
>> +        monitor_printf(rs->mon, "\n");
>>           max_width = 0;
>>           max_prefix = 0;	
>>           for(i = 0; i < rs->nb_completions; i++) {
>
> The rest of the function already uses rs->mon, so you didn't have
> to patch that. However, this shows that the current code makes a
> distinction between cur_mon and rs->mon. Either, there's a difference
> between the two and your code is wrong *or* current code is just
> redundant and your commit fixes it.
>
> I _guess_ it's the latter, but I'm not sure.
>
> Did you think about this? Did you test this series with multiple
> monitors running at the same time?
>
> You could pass cur_mon to readline_completion() in readline_handle_byte()
> to avoid all this, but it would be preferable to clarify the matter.
>
> This is also another benefit of having readline_completion() in a
> different patch, you can (and should!) clarify this point in the commit
> log along with testing results.
>
   OK, I'll test multiple monitors case.



-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions

[Wenchao Xia]

>> You could pass cur_mon to readline_completion() in readline_handle_byte()
>> to avoid all this, but it would be preferable to clarify the matter.
>>
>> This is also another benefit of having readline_completion() in a
>> different patch, you can (and should!) clarify this point in the commit
>> log along with testing results.
>>
>    OK, I'll test multiple monitors case.
>
>
>
   Do you have an example about how invoke qemu with multiple monitor?
Do you have recommendation about the way to connect to qemu's moniotr?
I tried pty with minicom but it seems not good, so wonder your method.


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions

[Luiz Capitulino]

On Tue, 09 Jul 2013 22:03:42 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 
> >> You could pass cur_mon to readline_completion() in readline_handle_byte()
> >> to avoid all this, but it would be preferable to clarify the matter.
> >>
> >> This is also another benefit of having readline_completion() in a
> >> different patch, you can (and should!) clarify this point in the commit
> >> log along with testing results.
> >>
> >    OK, I'll test multiple monitors case.
> >
> >
> >
>    Do you have an example about how invoke qemu with multiple monitor?
> Do you have recommendation about the way to connect to qemu's moniotr?
> I tried pty with minicom but it seems not good, so wonder your method.

You can open telnet sessions:

 -monitor tcp:0:4444,server,nowait,telnet \
 -monitor tcp:0:4445,server,nowait,telnet

But note that, besides testing, I'd like to see an explanation on
the commitlog as to why dropping cur_mon doesn't brake the current
code.

Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions

[Wenchao Xia]

于 2013-7-9 22:14, Luiz Capitulino 写道:
> On Tue, 09 Jul 2013 22:03:42 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>>
>>>> You could pass cur_mon to readline_completion() in readline_handle_byte()
>>>> to avoid all this, but it would be preferable to clarify the matter.
>>>>
>>>> This is also another benefit of having readline_completion() in a
>>>> different patch, you can (and should!) clarify this point in the commit
>>>> log along with testing results.
>>>>
>>>     OK, I'll test multiple monitors case.
>>>
>>>
>>>
>>     Do you have an example about how invoke qemu with multiple monitor?
>> Do you have recommendation about the way to connect to qemu's moniotr?
>> I tried pty with minicom but it seems not good, so wonder your method.
>
> You can open telnet sessions:
>
>   -monitor tcp:0:4444,server,nowait,telnet \
>   -monitor tcp:0:4445,server,nowait,telnet
>
> But note that, besides testing, I'd like to see an explanation on
> the commitlog as to why dropping cur_mon doesn't brake the current
> code.
>

   Thanks for the info, it works now.
   After re-check the code in readline_completion(), I think current
code with cur_mon, is redundant:

     mon = cur_mon;
     ...

     } else {
         monitor_printf(mon, "\n");
         ...
          ...
         for(i = 0; i < rs->nb_completions; i++) {
             monitor_printf(rs->mon, "%-*s", max_width, rs->completions[i]);
             if (++j == nb_cols || i == (rs->nb_completions - 1)) {
                 monitor_printf(rs->mon, "\n");
                 j = 0;
             }
         }
         readline_show_prompt(rs);
     }

   You can see later, it uses rs->mon to printf the auto completion
result, so the first 'monitor_printf(mon, "\n");', is used to start
a new line for the completion result, so mon should always be equal
to rs->mon, I can't think out a case that one monitor instance does
not print tips in new line while another monitor instance does. I'll
add a few line for it in commit message.

   About pass cur_mon to readline_completion(), I think parameter *rs
already have member rs->mon, allowing specify *mon would specify
the encapsulation a bit.

-- 
Best Regards

Wenchao Xia

[Qemu-devel] [PATCH V5 2/7] 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 29e3a95..68e2e80 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");
@@ -3974,7 +3975,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;
 
@@ -4163,12 +4164,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;
             }
@@ -4219,7 +4220,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);
                 }
             }
@@ -4782,6 +4783,9 @@ void monitor_init(CharDriverState *chr, int flags)
     if (!default_mon || (flags & MONITOR_IS_DEFAULT))
         default_mon = mon;
 
+    /* Always use *mon_cmds as root command table now. */
+    mon->cmd_table = mon_cmds;
+
     sortcmdlist();
 }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH V5 2/7] monitor: avoid direct use of global variable *mon_cmds

[Luiz Capitulino]

On Sat, 29 Jun 2013 11:52:56 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> New member *cmd_table is added in structure Monitor to avoid direct usage of
> *mon_cmds. Now monitor have an associated command table, when global variable
> *info_cmds is also discarded, structure Monitor would gain full control about
> how to deal with user input.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> 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 29e3a95..68e2e80 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");
> @@ -3974,7 +3975,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;
>  
> @@ -4163,12 +4164,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;
>              }
> @@ -4219,7 +4220,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);
>                  }
>              }
> @@ -4782,6 +4783,9 @@ void monitor_init(CharDriverState *chr, int flags)
>      if (!default_mon || (flags & MONITOR_IS_DEFAULT))
>          default_mon = mon;
>  
> +    /* Always use *mon_cmds as root command table now. */

If you mean it's the default, I'd just say that.

> +    mon->cmd_table = mon_cmds;
> +
>      sortcmdlist();
>  }
>  

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

[Wenchao Xia]

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 68e2e80..03a017d 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)
 {
@@ -3411,71 +3509,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.
@@ -3532,8 +3565,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];
@@ -4099,32 +4130,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

Re: [Qemu-devel] [PATCH V5 3/7] monitor: code move for parse_cmdline()

[Luiz Capitulino]

On Sat, 29 Jun 2013 11:52:57 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> get_str() is called by parse_cmdline() so it is moved also. Some
> code style error reported by check script, is also fixed.

Please, explain why you're doing this move.

> 
> 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 68e2e80..03a017d 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)
>  {
> @@ -3411,71 +3509,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.
> @@ -3532,8 +3565,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];
> @@ -4099,32 +4130,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, ':');

[Qemu-devel] [PATCH V5 4/7] monitor: avoid direct use of global *info_cmds in help functions

[Wenchao Xia]

In help functions info_cmds is treated as sub command group now, not as
a special case any more. Still help can't show message for single command
under "info", since command parser reject additional parameter, which
can be improved by change "help" item parameter define later. "log" is
still treated as special help case. compare_cmd() is used instead of strcmp()
in command searching.

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 03a017d..bc62fc7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -831,33 +831,76 @@ static void parse_cmdline(const char *cmdline,
     *pnb_args = nb_args;
 }
 
+static void help_cmd_dump_one(Monitor *mon,
+                              const mon_cmd_t *cmd,
+                              char **prefix_args,
+                              int prefix_args_nb)
+{
+    int i;
+
+    for (i = 0; i < prefix_args_nb; i++) {
+        monitor_printf(mon, "%s ", prefix_args[i]);
+    }
+    monitor_printf(mon, "%s %s -- %s\n", cmd->name, cmd->params, cmd->help);
+}
+
 static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
-                          const char *prefix, const char *name)
+                          char **args, int nb_args, int arg_index)
 {
     const mon_cmd_t *cmd;
 
-    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);
+    /* Dump all */
+    if (arg_index >= nb_args) {
+        for (cmd = cmds; cmd->name != NULL; cmd++) {
+            help_cmd_dump_one(mon, cmd, args, arg_index);
+        }
+        return;
+    }
+
+    /* Find one entry to dump */
+    for (cmd = cmds; cmd->name != NULL; cmd++) {
+        if (compare_cmd(args[arg_index], cmd->name)) {
+            if (cmd->sub_table) {
+                help_cmd_dump(mon, cmd->sub_table,
+                              args, nb_args, arg_index + 1);
+            } else {
+                help_cmd_dump_one(mon, cmd, args, arg_index);
+            }
+            break;
+        }
     }
 }
 
 static void help_cmd(Monitor *mon, const char *name)
 {
-    if (name && !strcmp(name, "info")) {
-        help_cmd_dump(mon, info_cmds, "info ", NULL);
-    } else {
-        help_cmd_dump(mon, mon->cmd_table, "", name);
-        if (name && !strcmp(name, "log")) {
+    char *args[MAX_ARGS];
+    int nb_args = 0, i;
+
+    if (name) {
+        /* special case for log */
+        if (!strcmp(name, "log")) {
             const QEMULogItem *item;
             monitor_printf(mon, "Log items (comma separated):\n");
             monitor_printf(mon, "%-10s %s\n", "none", "remove all logs");
             for (item = qemu_log_items; item->mask != 0; item++) {
                 monitor_printf(mon, "%-10s %s\n", item->name, item->help);
             }
+            return;
+        }
+
+        parse_cmdline(name, &nb_args, args);
+        if (nb_args >= MAX_ARGS) {
+            goto cleanup;
         }
     }
+
+    help_cmd_dump(mon, mon->cmd_table, args, nb_args, 0);
+
+cleanup:
+    nb_args = nb_args < MAX_ARGS ? nb_args : MAX_ARGS;
+    for (i = 0; i < nb_args; i++) {
+        g_free(args[i]);
+    }
 }
 
 static void do_help_cmd(Monitor *mon, const QDict *qdict)
-- 
1.7.1

Re: [Qemu-devel] [PATCH V5 4/7] monitor: avoid direct use of global *info_cmds in help functions

[Luiz Capitulino]

On Sat, 29 Jun 2013 11:52:58 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> In help functions info_cmds is treated as sub command group now, not as
> a special case any more. Still help can't show message for single command
> under "info", since command parser reject additional parameter, which

What you meant by "help can't show message for single command"?

> can be improved by change "help" item parameter define later. "log" is
> still treated as special help case. compare_cmd() is used instead of strcmp()
> in command searching.

I'm honestly a bit confused with this patch, I think it will be clarified
further down in the series, but might be a good idea to re-work the commit log.
More questions below.

> 
> 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 03a017d..bc62fc7 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -831,33 +831,76 @@ static void parse_cmdline(const char *cmdline,
>      *pnb_args = nb_args;
>  }
>  
> +static void help_cmd_dump_one(Monitor *mon,
> +                              const mon_cmd_t *cmd,
> +                              char **prefix_args,
> +                              int prefix_args_nb)
> +{
> +    int i;
> +
> +    for (i = 0; i < prefix_args_nb; i++) {
> +        monitor_printf(mon, "%s ", prefix_args[i]);
> +    }

What is this for?

> +    monitor_printf(mon, "%s %s -- %s\n", cmd->name, cmd->params, cmd->help);
> +}
> +
>  static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
> -                          const char *prefix, const char *name)
> +                          char **args, int nb_args, int arg_index)
>  {
>      const mon_cmd_t *cmd;
>  
> -    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);
> +    /* Dump all */
> +    if (arg_index >= nb_args) {
> +        for (cmd = cmds; cmd->name != NULL; cmd++) {
> +            help_cmd_dump_one(mon, cmd, args, arg_index);
> +        }
> +        return;
> +    }

Maybe this should be moved to help_cmd() so that it's not a special
case here?

> +
> +    /* Find one entry to dump */
> +    for (cmd = cmds; cmd->name != NULL; cmd++) {
> +        if (compare_cmd(args[arg_index], cmd->name)) {
> +            if (cmd->sub_table) {
> +                help_cmd_dump(mon, cmd->sub_table,
> +                              args, nb_args, arg_index + 1);
> +            } else {
> +                help_cmd_dump_one(mon, cmd, args, arg_index);
> +            }
> +            break;
> +        }
>      }
>  }
>  
>  static void help_cmd(Monitor *mon, const char *name)
>  {
> -    if (name && !strcmp(name, "info")) {
> -        help_cmd_dump(mon, info_cmds, "info ", NULL);
> -    } else {
> -        help_cmd_dump(mon, mon->cmd_table, "", name);
> -        if (name && !strcmp(name, "log")) {
> +    char *args[MAX_ARGS];
> +    int nb_args = 0, i;
> +
> +    if (name) {
> +        /* special case for log */
> +        if (!strcmp(name, "log")) {
>              const QEMULogItem *item;
>              monitor_printf(mon, "Log items (comma separated):\n");
>              monitor_printf(mon, "%-10s %s\n", "none", "remove all logs");
>              for (item = qemu_log_items; item->mask != 0; item++) {
>                  monitor_printf(mon, "%-10s %s\n", item->name, item->help);
>              }
> +            return;
> +        }
> +
> +        parse_cmdline(name, &nb_args, args);
> +        if (nb_args >= MAX_ARGS) {
> +            goto cleanup;
>          }

parse_cmdline() should handle de-allocation on failure, also checking
nb_args for failure is a bad API. This hasn't been a problem so far
because parse_cmdline() is used only once, but now you're making it a
bit more generic so it should be improved.

>      }
> +
> +    help_cmd_dump(mon, mon->cmd_table, args, nb_args, 0);
> +
> +cleanup:
> +    nb_args = nb_args < MAX_ARGS ? nb_args : MAX_ARGS;
> +    for (i = 0; i < nb_args; i++) {
> +        g_free(args[i]);
> +    }

I'd add free_cmdline_args().

>  }
>  
>  static void do_help_cmd(Monitor *mon, const QDict *qdict)

Re: [Qemu-devel] [PATCH V5 4/7] monitor: avoid direct use of global *info_cmds in help functions

[Wenchao Xia]

于 2013-7-8 23:45, Luiz Capitulino 写道:
> On Sat, 29 Jun 2013 11:52:58 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> In help functions info_cmds is treated as sub command group now, not as
>> a special case any more. Still help can't show message for single command
>> under "info", since command parser reject additional parameter, which
>
> What you meant by "help can't show message for single command"?
>
   I mean "help info block" can't work.

>> can be improved by change "help" item parameter define later. "log" is
>> still treated as special help case. compare_cmd() is used instead of strcmp()
>> in command searching.
>
> I'm honestly a bit confused with this patch, I think it will be clarified
> further down in the series, but might be a good idea to re-work the commit log.
> More questions below.
>
   To avoid use of info_cmds, the help function need to use sub command
structure, otherwise "info" is a special case, so I modified the
functions. I will refine the commit message.

>>
>> 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 03a017d..bc62fc7 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -831,33 +831,76 @@ static void parse_cmdline(const char *cmdline,
>>       *pnb_args = nb_args;
>>   }
>>
>> +static void help_cmd_dump_one(Monitor *mon,
>> +                              const mon_cmd_t *cmd,
>> +                              char **prefix_args,
>> +                              int prefix_args_nb)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < prefix_args_nb; i++) {
>> +        monitor_printf(mon, "%s ", prefix_args[i]);
>> +    }
>
> What is this for?
>
   It is used to print parent command, such as "info" in "info block"

   help_cmd_dump():
-            monitor_printf(mon, "%s%s %s -- %s\n", prefix, cmd->name,
-                           cmd->params, cmd->help);

   The old code can't work with sub command for parameter prefix. To
solve it, I introduced function help_cmd_dump_one(), which dedicate
to format control and knows parent commands. This can avoid dynamic
snprintf to a buffer for prefix.

>> +    monitor_printf(mon, "%s %s -- %s\n", cmd->name, cmd->params, cmd->help);
>> +}
>> +
>>   static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds,
>> -                          const char *prefix, const char *name)
>> +                          char **args, int nb_args, int arg_index)
>>   {
>>       const mon_cmd_t *cmd;
>>
>> -    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);
>> +    /* Dump all */
>> +    if (arg_index >= nb_args) {
>> +        for (cmd = cmds; cmd->name != NULL; cmd++) {
>> +            help_cmd_dump_one(mon, cmd, args, arg_index);
>> +        }
>> +        return;
>> +    }
>
> Maybe this should be moved to help_cmd() so that it's not a special
> case here?
>
   help_cmd_dump() is changed as a re-enterable function to handle
sub command case, so above lines need to be inside a re-enterable
function, to handle the case that dump all commands under one group,
such as "help info". Moving it out will make it work only when
folder depth is 1.

>> +
>> +    /* Find one entry to dump */
>> +    for (cmd = cmds; cmd->name != NULL; cmd++) {
>> +        if (compare_cmd(args[arg_index], cmd->name)) {
>> +            if (cmd->sub_table) {
>> +                help_cmd_dump(mon, cmd->sub_table,
>> +                              args, nb_args, arg_index + 1);
>> +            } else {
>> +                help_cmd_dump_one(mon, cmd, args, arg_index);
>> +            }
>> +            break;
>> +        }
>>       }
>>   }
>>
>>   static void help_cmd(Monitor *mon, const char *name)
>>   {
>> -    if (name && !strcmp(name, "info")) {
>> -        help_cmd_dump(mon, info_cmds, "info ", NULL);
>> -    } else {
>> -        help_cmd_dump(mon, mon->cmd_table, "", name);
>> -        if (name && !strcmp(name, "log")) {
>> +    char *args[MAX_ARGS];
>> +    int nb_args = 0, i;
>> +
>> +    if (name) {
>> +        /* special case for log */
>> +        if (!strcmp(name, "log")) {
>>               const QEMULogItem *item;
>>               monitor_printf(mon, "Log items (comma separated):\n");
>>               monitor_printf(mon, "%-10s %s\n", "none", "remove all logs");
>>               for (item = qemu_log_items; item->mask != 0; item++) {
>>                   monitor_printf(mon, "%-10s %s\n", item->name, item->help);
>>               }
>> +            return;
>> +        }
>> +
>> +        parse_cmdline(name, &nb_args, args);
>> +        if (nb_args >= MAX_ARGS) {
>> +            goto cleanup;
>>           }
>
> parse_cmdline() should handle de-allocation on failure, also checking
> nb_args for failure is a bad API. This hasn't been a problem so far
> because parse_cmdline() is used only once, but now you're making it a
> bit more generic so it should be improved.
>
   OK, I will improve it in an previous patch.

>>       }
>> +
>> +    help_cmd_dump(mon, mon->cmd_table, args, nb_args, 0);
>> +
>> +cleanup:
>> +    nb_args = nb_args < MAX_ARGS ? nb_args : MAX_ARGS;
>> +    for (i = 0; i < nb_args; i++) {
>> +        g_free(args[i]);
>> +    }
>
> I'd add free_cmdline_args().
>
   OK.

>>   }
>>
>>   static void do_help_cmd(Monitor *mon, const QDict *qdict)
>


-- 
Best Regards

Wenchao Xia

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

[Wenchao Xia]

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

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

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

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

diff --git a/monitor.c b/monitor.c
index bc62fc7..2f5b91d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -801,9 +801,24 @@ static int get_str(char *buf, int buf_size, const char **pp)
 
 #define MAX_ARGS 16
 
-/* NOTE: this parser is an approximate form of the real command parser */
+/*
+ * Parse the command line to get valid args.
+ * @cmdline: command line to be parsed.
+ * @pnb_args: location to store the number of args, must NOT be NULL.
+ * @args: location to store the args, which should be freed by caller, must
+ *        NOT be NULL.
+ * @args_cmdline: location to store char position in @cmdline correspond to
+ *                @args include '\0', can be NULL.
+ *
+ * For example, if @cmdline = " ab" and @args_cmdline != NULL, result will be:
+ * pnb_args = 1, args[0] = "ab", args_cmdline[0] = "ab",
+ * args_cmdline[1] = "\0".
+ *
+ * NOTE: this parser is an approximate form of the real command parser. Number
+ *       of args have a limit of MAX_ARGS.
+ */
 static void parse_cmdline(const char *cmdline,
-                          int *pnb_args, char **args)
+                         int *pnb_args, char **args, const char **args_cmdline)
 {
     const char *p;
     int nb_args, ret;
@@ -811,14 +826,14 @@ static void parse_cmdline(const char *cmdline,
 
     p = cmdline;
     nb_args = 0;
-    for (;;) {
+    while (nb_args < MAX_ARGS) {
         while (qemu_isspace(*p)) {
             p++;
         }
-        if (*p == '\0') {
-            break;
+        if (args_cmdline) {
+            args_cmdline[nb_args] = p;
         }
-        if (nb_args >= MAX_ARGS) {
+        if (*p == '\0') {
             break;
         }
         ret = get_str(buf, sizeof(buf), &p);
@@ -888,7 +903,7 @@ static void help_cmd(Monitor *mon, const char *name)
             return;
         }
 
-        parse_cmdline(name, &nb_args, args);
+        parse_cmdline(name, &nb_args, args, NULL);
         if (nb_args >= MAX_ARGS) {
             goto cleanup;
         }
@@ -4179,17 +4194,18 @@ static const char *next_arg_type(const char *typestr)
     return (p != NULL ? ++p : typestr);
 }
 
-static void monitor_find_completion(Monitor *mon,
-                                    const char *cmdline)
+static void monitor_find_completion_by_table(Monitor *mon,
+                                             const mon_cmd_t *cmd_table,
+                                             const char *cmdline)
 {
-    const char *cmdname;
+    const char *cmdname, *args_cmdline[MAX_ARGS];
     char *args[MAX_ARGS];
     int nb_args, i, len;
     const char *ptype, *str;
     const mon_cmd_t *cmd;
     MonitorBlockComplete mbs;
 
-    parse_cmdline(cmdline, &nb_args, args);
+    parse_cmdline(cmdline, &nb_args, args, args_cmdline);
 #ifdef DEBUG_COMPLETION
     for (i = 0; i < nb_args; i++) {
         monitor_printf(mon, "arg%d = '%s'\n", i, args[i]);
@@ -4212,12 +4228,12 @@ static void monitor_find_completion(Monitor *mon,
         else
             cmdname = args[0];
         readline_set_completion_index(mon->rs, strlen(cmdname));
-        for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
+        for (cmd = cmd_table; cmd->name != NULL; cmd++) {
             cmd_completion(mon, cmdname, cmd->name);
         }
     } else {
         /* find the command */
-        for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
+        for (cmd = cmd_table; cmd->name != NULL; cmd++) {
             if (compare_cmd(args[0], cmd->name)) {
                 break;
             }
@@ -4226,6 +4242,12 @@ static void monitor_find_completion(Monitor *mon,
             goto cleanup;
         }
 
+        if (cmd->sub_table) {
+            monitor_find_completion_by_table(mon, cmd->sub_table,
+                                             args_cmdline[1]);
+            goto cleanup;
+        }
+
         ptype = next_arg_type(cmd->args_type);
         for(i = 0; i < nb_args - 2; i++) {
             if (*ptype != '\0') {
@@ -4252,13 +4274,7 @@ static void monitor_find_completion(Monitor *mon,
             bdrv_iterate(block_completion_it, &mbs);
             break;
         case 's':
-            /* XXX: more generic ? */
-            if (!strcmp(cmd->name, "info")) {
-                readline_set_completion_index(mon->rs, strlen(str));
-                for(cmd = info_cmds; cmd->name != NULL; cmd++) {
-                    cmd_completion(mon, str, cmd->name);
-                }
-            } else if (!strcmp(cmd->name, "sendkey")) {
+            if (!strcmp(cmd->name, "sendkey")) {
                 char *sep = strrchr(str, '-');
                 if (sep)
                     str = sep + 1;
@@ -4284,6 +4300,12 @@ cleanup:
     }
 }
 
+static void monitor_find_completion(Monitor *mon,
+                                    const char *cmdline)
+{
+    return monitor_find_completion_by_table(mon, mon->cmd_table, cmdline);
+}
+
 static int monitor_can_read(void *opaque)
 {
     Monitor *mon = opaque;
-- 
1.7.1

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

[Wenchao Xia]

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

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

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

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

[Luiz Capitulino]

On Sat, 29 Jun 2013 11:53:00 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

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

The auto-completion works, but the command is still refused:

(qemu) help info u
usb       usbhost   usernet   uuid
(qemu) help info uuid
help: extraneous characters at the end of line

This is not hugely important, but I think it would make more sense
to make the command work before having auto-completion support.

> 
> 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 2f5b91d..3ef18ee 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4283,10 +4283,8 @@ static void monitor_find_completion_by_table(Monitor *mon,
>                      cmd_completion(mon, str, QKeyCode_lookup[i]);
>                  }
>              } else if (!strcmp(cmd->name, "help|?")) {
> -                readline_set_completion_index(mon->rs, strlen(str));
> -                for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
> -                    cmd_completion(mon, str, cmd->name);
> -                }
> +                monitor_find_completion_by_table(mon, cmd_table,
> +                                                 args_cmdline[1]);
>              }
>              break;
>          default:

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

[Wenchao Xia]

于 2013-7-9 0:09, Luiz Capitulino 写道:
> On Sat, 29 Jun 2013 11:53:00 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> Now special case "help *" in auto completion can work with sub commands,
>> such as "help info a*".
>
> The auto-completion works, but the command is still refused:
>
> (qemu) help info u
> usb       usbhost   usernet   uuid
> (qemu) help info uuid
> help: extraneous characters at the end of line
>
> This is not hugely important, but I think it would make more sense
> to make the command work before having auto-completion support.
>
   Patch 7 fix it, I will adjust the patch sequence.


>>
>> 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 2f5b91d..3ef18ee 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -4283,10 +4283,8 @@ static void monitor_find_completion_by_table(Monitor *mon,
>>                       cmd_completion(mon, str, QKeyCode_lookup[i]);
>>                   }
>>               } else if (!strcmp(cmd->name, "help|?")) {
>> -                readline_set_completion_index(mon->rs, strlen(str));
>> -                for (cmd = mon->cmd_table; cmd->name != NULL; cmd++) {
>> -                    cmd_completion(mon, str, cmd->name);
>> -                }
>> +                monitor_find_completion_by_table(mon, cmd_table,
>> +                                                 args_cmdline[1]);
>>               }
>>               break;
>>           default:
>


-- 
Best Regards

Wenchao Xia

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

[Wenchao Xia]

A new parameter type 'S' is introduced to allow user input any string.
"help info block" do not tip extra parameter error now.

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

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

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

[Luiz Capitulino]

On Sat, 29 Jun 2013 11:53:01 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> A new parameter type 'S' is introduced to allow user input any string.
> "help info block" do not tip extra parameter error now.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  hmp-commands.hx |    2 +-
>  monitor.c       |   30 +++++++++++++++++++++++++++++-
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 915b0d1..8cf5f29 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -11,7 +11,7 @@ ETEXI
>  
>      {
>          .name       = "help|?",
> -        .args_type  = "name:s?",
> +        .args_type  = "name:S?",
>          .params     = "[cmd]",
>          .help       = "show the help",
>          .mhandler.cmd = do_help_cmd,
> diff --git a/monitor.c b/monitor.c
> index 3ef18ee..ebdc2a3 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -83,6 +83,7 @@
>   * 'F'          filename
>   * 'B'          block device name
>   * 's'          string (accept optional quote)
> + * 'S'          supported types, can be any string (accept optional quote)

I think you want to say it just appends the rest of the string.

>   * 'O'          option string of the form NAME=VALUE,...
>   *              parsed according to QemuOptsList given by its name
>   *              Example: 'device:O' uses qemu_device_opts.
> @@ -4011,6 +4012,31 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                  }
>              }
>              break;
> +        case 'S':
> +            {
> +                /* package all remaining string */
> +                int len;
> +
> +                while (qemu_isspace(*p)) {
> +                    p++;
> +                }
> +                if (*typestr == '?') {
> +                    typestr++;
> +                    if (*p == '\0') {
> +                        /* no remaining string: NULL argument */
> +                        break;
> +                    }
> +                }
> +                len = strlen(p);
> +                if (len <= 0) {
> +                    monitor_printf(mon, "%s: string expected\n",
> +                                   cmdname);
> +                    break;
> +                }
> +                qdict_put(qdict, key, qstring_from_str(p));
> +                p += len;
> +            }
> +            break;
>          default:
>          bad_type:
>              monitor_printf(mon, "%s: unknown type '%c'\n", cmdname, c);
> @@ -4282,7 +4308,9 @@ static void monitor_find_completion_by_table(Monitor *mon,
>                  for (i = 0; i < Q_KEY_CODE_MAX; i++) {
>                      cmd_completion(mon, str, QKeyCode_lookup[i]);
>                  }
> -            } else if (!strcmp(cmd->name, "help|?")) {
> +            }
> +        case 'S':
> +            if (!strcmp(cmd->name, "help|?")) {
>                  monitor_find_completion_by_table(mon, cmd_table,
>                                                   args_cmdline[1]);

This gives a very specific meaning to the S type, doesn't it? Why can't
you treat it just like 's'? Meaning that you could have:

  case 's':
  case 'S':

>              }

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

[Wenchao Xia]

于 2013-7-9 0:17, Luiz Capitulino 写道:
> On Sat, 29 Jun 2013 11:53:01 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> A new parameter type 'S' is introduced to allow user input any string.
>> "help info block" do not tip extra parameter error now.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   hmp-commands.hx |    2 +-
>>   monitor.c       |   30 +++++++++++++++++++++++++++++-
>>   2 files changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 915b0d1..8cf5f29 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -11,7 +11,7 @@ ETEXI
>>
>>       {
>>           .name       = "help|?",
>> -        .args_type  = "name:s?",
>> +        .args_type  = "name:S?",
>>           .params     = "[cmd]",
>>           .help       = "show the help",
>>           .mhandler.cmd = do_help_cmd,
>> diff --git a/monitor.c b/monitor.c
>> index 3ef18ee..ebdc2a3 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -83,6 +83,7 @@
>>    * 'F'          filename
>>    * 'B'          block device name
>>    * 's'          string (accept optional quote)
>> + * 'S'          supported types, can be any string (accept optional quote)
>
> I think you want to say it just appends the rest of the string.
>
>>    * 'O'          option string of the form NAME=VALUE,...
>>    *              parsed according to QemuOptsList given by its name
>>    *              Example: 'device:O' uses qemu_device_opts.
>> @@ -4011,6 +4012,31 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>                   }
>>               }
>>               break;
>> +        case 'S':
>> +            {
>> +                /* package all remaining string */
>> +                int len;
>> +
>> +                while (qemu_isspace(*p)) {
>> +                    p++;
>> +                }
>> +                if (*typestr == '?') {
>> +                    typestr++;
>> +                    if (*p == '\0') {
>> +                        /* no remaining string: NULL argument */
>> +                        break;
>> +                    }
>> +                }
>> +                len = strlen(p);
>> +                if (len <= 0) {
>> +                    monitor_printf(mon, "%s: string expected\n",
>> +                                   cmdname);
>> +                    break;
>> +                }
>> +                qdict_put(qdict, key, qstring_from_str(p));
>> +                p += len;
>> +            }
>> +            break;
>>           default:
>>           bad_type:
>>               monitor_printf(mon, "%s: unknown type '%c'\n", cmdname, c);
>> @@ -4282,7 +4308,9 @@ static void monitor_find_completion_by_table(Monitor *mon,
>>                   for (i = 0; i < Q_KEY_CODE_MAX; i++) {
>>                       cmd_completion(mon, str, QKeyCode_lookup[i]);
>>                   }
>> -            } else if (!strcmp(cmd->name, "help|?")) {
>> +            }
>> +        case 'S':
>> +            if (!strcmp(cmd->name, "help|?")) {
>>                   monitor_find_completion_by_table(mon, cmd_table,
>>                                                    args_cmdline[1]);
>
> This gives a very specific meaning to the S type, doesn't it? Why can't
> you treat it just like 's'? Meaning that you could have:
>
>    case 's':
>    case 'S':
>
   OK, will fix it.


>>               }
>
>


-- 
Best Regards

Wenchao Xia

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

[Wenchao Xia]

  Any comments for it?

> Global variable *mon_cmds and *info_cmds are not directly used any more,
> *cur_mon is not used in completion related functions. It is possible to create
> a monitor with different command table now, but that requirement do not exist
> yet, so not changed it to save trouble. Log command is still a special case
> now, may be it can be converted as sub group later.
> 
> Patch 1-3 make sure the functions can be re-entered safely.
> 
> V2:
>    General:
>    To discard *info_comds more graceful, help related function is modified to support
> sub command too.
>    Patch 6/7 are added to improve help related functions.
>    Patch 5: not directly return to make sure args are freed.
> 
>    Address Luiz's comments:
>    Split patch into small 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.
> 
> Wenchao Xia (7):
>    1 monitor: avoid direct use of global *cur_mon in completion functions
>    2 monitor: avoid direct use of global variable *mon_cmds
>    3 monitor: code move for parse_cmdline()
>    4 monitor: avoid direct use of global *info_cmds in help functions
>    5 monitor: support sub commands in auto completion
>    6 monitor: improve "help" in auto completion for sub command
>    7 monitor: improve "help" to allow show details of single command in sub group
> 
>   hmp-commands.hx            |    2 +-
>   include/monitor/readline.h |    3 +-
>   monitor.c                  |  387 ++++++++++++++++++++++++++++----------------
>   readline.c                 |    5 +-
>   4 files changed, 254 insertions(+), 143 deletions(-)
> 
> 


-- 
Best Regards

Wenchao Xia

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

[Luiz Capitulino]

On Mon, 08 Jul 2013 06:39:52 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

>   Any comments for it?

I should review it shortly.