Re: [Qemu-devel] [PATCH 1/2] monitor: discard global variable in auto completion functions

[Luiz Capitulino <lcapitulino@redhat.com>]

On Fri, 21 Jun 2013 14:37:37 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> In monitor_find_completion() and related functions, Global variable
> *mon_cmds is not used any more, make them reenterable safely.
> *cur_mon is also not used now. *info_cmds is still there, but soon
> will be removed by a new way of sub command completion.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  include/monitor/readline.h |    8 ++++-
>  monitor.c                  |   70 ++++++++++++++++++++++++++-----------------
>  readline.c                 |    4 ++-
>  3 files changed, 52 insertions(+), 30 deletions(-)
> 
> diff --git a/include/monitor/readline.h b/include/monitor/readline.h
> index fc9806e..a4c7bce 100644
> --- a/include/monitor/readline.h
> +++ b/include/monitor/readline.h
> @@ -7,8 +7,12 @@
>  #define READLINE_MAX_CMDS 64
>  #define READLINE_MAX_COMPLETIONS 256
>  
> +typedef struct mon_cmd_t mon_cmd_t;

This should be in monitor.h. Maybe you could move struct mon_cmd_t there
too.

Also, can you split this series is smaller patches? It looks good, but
it isn't easy to review because it changes too much code at once.

Otherwise this is a nice feature!

> +
>  typedef void ReadLineFunc(Monitor *mon, const char *str, void *opaque);
> -typedef void ReadLineCompletionFunc(const char *cmdline);
> +typedef void ReadLineCompletionFunc(Monitor *mon,
> +                                    mon_cmd_t *cmd_table,
> +                                    const char *cmdline);
>  
>  typedef struct ReadLineState {
>      char cmd_buf[READLINE_CMD_BUF_SIZE + 1];
> @@ -35,6 +39,7 @@ typedef struct ReadLineState {
>      int read_password;
>      char prompt[256];
>      Monitor *mon;
> +    mon_cmd_t *cmd_table;
>  } ReadLineState;
>  
>  void readline_add_completion(ReadLineState *rs, const char *str);
> @@ -50,6 +55,7 @@ void readline_restart(ReadLineState *rs);
>  void readline_show_prompt(ReadLineState *rs);
>  
>  ReadLineState *readline_init(Monitor *mon,
> +                             mon_cmd_t *cmd_table,
>                               ReadLineCompletionFunc *completion_finder);
>  
>  #endif /* !READLINE_H */
> diff --git a/monitor.c b/monitor.c
> index 70ae8f5..bc60171 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -116,7 +116,7 @@ struct MonitorCompletionData {
>      void (*user_print)(Monitor *mon, const QObject *data);
>  };
>  
> -typedef struct mon_cmd_t {
> +struct mon_cmd_t {
>      const char *name;
>      const char *args_type;
>      const char *params;
> @@ -134,7 +134,7 @@ typedef struct mon_cmd_t {
>       * used, and mhandler of 1st level plays the role of help function.
>       */
>      struct mon_cmd_t *sub_table;
> -} mon_cmd_t;
> +};
>  
>  /* file descriptors passed via SCM_RIGHTS */
>  typedef struct mon_fd_t mon_fd_t;
> @@ -3999,7 +3999,7 @@ out:
>      QDECREF(qdict);
>  }
>  
> -static void cmd_completion(const char *name, const char *list)
> +static void cmd_completion(Monitor *mon, const char *name, const char *list)
>  {
>      const char *p, *pstart;
>      char cmd[128];
> @@ -4017,7 +4017,7 @@ static void cmd_completion(const char *name, const char *list)
>          memcpy(cmd, pstart, len);
>          cmd[len] = '\0';
>          if (name[0] == '\0' || !strncmp(name, cmd, strlen(name))) {
> -            readline_add_completion(cur_mon->rs, cmd);
> +            readline_add_completion(mon->rs, cmd);
>          }
>          if (*p == '\0')
>              break;
> @@ -4025,7 +4025,7 @@ static void cmd_completion(const char *name, const char *list)
>      }
>  }
>  
> -static void file_completion(const char *input)
> +static void file_completion(Monitor *mon, const char *input)
>  {
>      DIR *ffs;
>      struct dirent *d;
> @@ -4048,7 +4048,7 @@ static void file_completion(const char *input)
>          pstrcpy(file_prefix, sizeof(file_prefix), p + 1);
>      }
>  #ifdef DEBUG_COMPLETION
> -    monitor_printf(cur_mon, "input='%s' path='%s' prefix='%s'\n",
> +    monitor_printf(mon, "input='%s' path='%s' prefix='%s'\n",
>                     input, path, file_prefix);
>  #endif
>      ffs = opendir(path);
> @@ -4075,20 +4075,28 @@ 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);
>  }
>  
> -static void block_completion_it(void *opaque, BlockDriverState *bs)
> +typedef struct MonitorBlockComplete {
> +    Monitor *mon;
> +    const char *input;
> +} MonitorBlockComplete;
> +
> +static void block_completion_it(void *opaque,
> +                                BlockDriverState *bs)
>  {
>      const char *name = bdrv_get_device_name(bs);
> -    const char *input = opaque;
> +    MonitorBlockComplete *mbc = opaque;
> +    Monitor *mon = mbc->mon;
> +    const char *input = mbc->input;
>  
>      if (input[0] == '\0' ||
>          !strncmp(name, (char *)input, strlen(input))) {
> -        readline_add_completion(cur_mon->rs, name);
> +        readline_add_completion(mon->rs, name);
>      }
>  }
>  
> @@ -4124,18 +4132,21 @@ 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,
> +                                    mon_cmd_t *cmd_table,
> +                                    const char *cmdline)
>  {
>      const char *cmdname;
>      char *args[MAX_ARGS];
>      int nb_args, i, len;
>      const char *ptype, *str;
>      const mon_cmd_t *cmd;
> +    MonitorBlockComplete mbs;
>  
>      parse_cmdline(cmdline, &nb_args, args);
>  #ifdef DEBUG_COMPLETION
>      for(i = 0; i < nb_args; i++) {
> -        monitor_printf(cur_mon, "arg%d = '%s'\n", i, (char *)args[i]);
> +        monitor_printf(mon, "arg%d = '%s'\n", i, (char *)args[i]);
>      }
>  #endif
>  
> @@ -4154,13 +4165,13 @@ static void monitor_find_completion(const char *cmdline)
>              cmdname = "";
>          else
>              cmdname = args[0];
> -        readline_set_completion_index(cur_mon->rs, strlen(cmdname));
> -        for(cmd = mon_cmds; cmd->name != NULL; cmd++) {
> -            cmd_completion(cmdname, cmd->name);
> +        readline_set_completion_index(mon->rs, strlen(cmdname));
> +        for (cmd = 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 = cmd_table; cmd->name != NULL; cmd++) {
>              if (compare_cmd(args[0], cmd->name)) {
>                  break;
>              }
> @@ -4184,33 +4195,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));
> -                for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
> -                    cmd_completion(str, cmd->name);
> +                readline_set_completion_index(mon->rs, strlen(str));
> +                for (cmd = cmd_table; cmd->name != NULL; cmd++) {
> +                    cmd_completion(mon, str, cmd->name);
>                  }
>              }
>              break;
> @@ -4751,7 +4764,8 @@ void monitor_init(CharDriverState *chr, int flags)
>      mon->chr = chr;
>      mon->flags = flags;
>      if (flags & MONITOR_USE_READLINE) {
> -        mon->rs = readline_init(mon, monitor_find_completion);
> +        /* Always use *mon_cmds as entry now. */
> +        mon->rs = readline_init(mon, mon_cmds, monitor_find_completion);
>          monitor_read_command(mon, 0);
>      }
>  
> diff --git a/readline.c b/readline.c
> index 1c0f7ee..d97e062 100644
> --- a/readline.c
> +++ b/readline.c
> @@ -285,7 +285,7 @@ static void readline_completion(ReadLineState *rs)
>      cmdline = g_malloc(rs->cmd_buf_index + 1);
>      memcpy(cmdline, rs->cmd_buf, rs->cmd_buf_index);
>      cmdline[rs->cmd_buf_index] = '\0';
> -    rs->completion_finder(cmdline);
> +    rs->completion_finder(rs->mon, rs->cmd_table, cmdline);
>      g_free(cmdline);
>  
>      /* no completion found */
> @@ -482,12 +482,14 @@ const char *readline_get_history(ReadLineState *rs, unsigned int index)
>  }
>  
>  ReadLineState *readline_init(Monitor *mon,
> +                             mon_cmd_t *cmd_table,
>                               ReadLineCompletionFunc *completion_finder)
>  {
>      ReadLineState *rs = g_malloc0(sizeof(*rs));
>  
>      rs->hist_entry = -1;
>      rs->mon = mon;
> +    rs->cmd_table = cmd_table;
>      rs->completion_finder = completion_finder;
>  
>      return rs;