Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

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



-- 
Best Regards

Wenchao Xia