diff mbox

[V3,1/7] monitor: discard global variable *cur_mon in completion functions

Message ID 1372303666-12323-2-git-send-email-xiawenc@linux.vnet.ibm.com
State New
Headers show

Commit Message

Wayne Xia June 27, 2013, 3:27 a.m. UTC 
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>
---
 include/monitor/readline.h |    3 +-
 monitor.c                  |   53 ++++++++++++++++++++++++++-----------------
 readline.c                 |    5 +--
 3 files changed, 36 insertions(+), 25 deletions(-)

Comments

Eric Blake June 27, 2013, 9:26 p.m. UTC | #1 
On 06/26/2013 09:27 PM, Wenchao Xia 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.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---

> @@ -4075,20 +4075,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);

Oops, I started reviewing that before noticing you posted a v3.  At any
rate, my review of v2 still stands on this patch.  Meanwhile, I just
barely noticed that this pre-existing code might cause a -Wshadow
warning in the future if <strings.h> is ever included.  I'm not sure
what qemu's policy is on being clean against -Wshadow warnings, but I
personally tend to avoid local variables whose name might conflict with
global functions.
Wayne Xia June 28, 2013, 2:25 a.m. UTC | #2 
于 2013-6-28 5:26, Eric Blake 写道:
> On 06/26/2013 09:27 PM, Wenchao Xia 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.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>
>> @@ -4075,20 +4075,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);
>
> Oops, I started reviewing that before noticing you posted a v3.  At any
> rate, my review of v2 still stands on this patch.  Meanwhile, I just
> barely noticed that this pre-existing code might cause a -Wshadow
> warning in the future if <strings.h> is ever included.  I'm not sure
> what qemu's policy is on being clean against -Wshadow warnings, but I
> personally tend to avoid local variables whose name might conflict with
> global functions.
>
   Thanks for reviewing. I'll fix what you mentioned in v2.

   For the -Wshadow warning, I guess you mean "file" right? It would
be a bit hard to check this warning for new patch, since this warning
now exist in many code. In my opinion, it would be better to have a
separate series to clean up this warnings first, then check new patches
for this issue.
Eric Blake June 28, 2013, 1:42 p.m. UTC | #3 
On 06/27/2013 08:25 PM, Wenchao Xia wrote:
>>> +            readline_add_completion(mon->rs, file);
>>>           }
>>>       }
>>>       closedir(ffs);
>>
>> Oops, I started reviewing that before noticing you posted a v3.  At any
>> rate, my review of v2 still stands on this patch.  Meanwhile, I just
>> barely noticed that this pre-existing code might cause a -Wshadow
>> warning in the future if <strings.h> is ever included.  I'm not sure
>> what qemu's policy is on being clean against -Wshadow warnings, but I
>> personally tend to avoid local variables whose name might conflict with
>> global functions.
>>
>   Thanks for reviewing. I'll fix what you mentioned in v2.
> 
>   For the -Wshadow warning, I guess you mean "file" right?

No, I meant ffs.  ffs() is a global function, in scope if <strings.h> is
included; so using it as a local variable name will trigger -Wshadow
warnings from (at least some versions of) gcc.

> It would
> be a bit hard to check this warning for new patch, since this warning
> now exist in many code. In my opinion, it would be better to have a
> separate series to clean up this warnings first, then check new patches
> for this issue.

Agreed that the issue (of using ffs as a local variable name) is
pre-existing, and therefore cleanups for -Wshadow are not related to
this series and not something you need to necessarily worry about.
diff mbox

Patch

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 70ae8f5..f3fdaa4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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,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);
     }
 }
 
@@ -4124,18 +4131,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]);
+        monitor_printf(mon, "arg%d = '%s'\n", i, (char *)args[i]);
     }
 #endif
 
@@ -4154,9 +4163,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 */
@@ -4184,33 +4193,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++) {