On Sat, 29 Jun 2013 11:52:55 +0800 Wenchao Xia <xiaw...@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 <xiaw...@linux.vnet.ibm.com> > Reviewed-by: Eric Blake <ebl...@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.