[V5,1/7] monitor: avoid direct use of global *cur_mon in completion functions

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(-)

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.

于 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.

>> 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.

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.

于 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.