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 <address@hidden> 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 <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  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.