From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43332) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UwNLc-0006Lt-99 for qemu-devel@nongnu.org; Mon, 08 Jul 2013 22:08:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UwNLa-0001IN-NL for qemu-devel@nongnu.org; Mon, 08 Jul 2013 22:08:08 -0400 Received: from e28smtp04.in.ibm.com ([122.248.162.4]:37309) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UwNLZ-0001H9-QM for qemu-devel@nongnu.org; Mon, 08 Jul 2013 22:08:06 -0400 Received: from /spool/local by e28smtp04.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 9 Jul 2013 07:31:23 +0530 Received: from d28relay03.in.ibm.com (d28relay03.in.ibm.com [9.184.220.60]) by d28dlp03.in.ibm.com (Postfix) with ESMTP id 6CAD31258051 for ; Tue, 9 Jul 2013 07:37:13 +0530 (IST) Received: from d28av02.in.ibm.com (d28av02.in.ibm.com [9.184.220.64]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r6928VWS23265336 for ; Tue, 9 Jul 2013 07:38:32 +0530 Received: from d28av02.in.ibm.com (loopback [127.0.0.1]) by d28av02.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r6927weT019039 for ; Tue, 9 Jul 2013 12:07:58 +1000 Message-ID: <51DB7040.8020306@linux.vnet.ibm.com> Date: Tue, 09 Jul 2013 10:06:56 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1372477981-7512-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1372477981-7512-2-git-send-email-xiawenc@linux.vnet.ibm.com> <20130708111711.0b2631d6@redhat.com> In-Reply-To: <20130708111711.0b2631d6@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com 于 2013-7-8 23:17, Luiz Capitulino 写道: > On Sat, 29 Jun 2013 11:52:55 +0800 > 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. > > 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 >> Reviewed-by: Eric Blake >> --- >> 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