qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur

From: Wenchao Xia
Subject: Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions
Date: Tue, 09 Jul 2013 10:06:56 +0800
User-agent: Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20130620 Thunderbird/17.0.7 
于 2013-7-8 23:17, 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.


  I'll split it as above.


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.


  OK, I'll test multiple monitors case.



--
Best Regards

Wenchao Xia
reply via email to
[Prev in Thread] Current Thread [Next in Thread]