[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions |
Date: |
Mon, 8 Jul 2013 11:17:11 -0400 |
On Sat, 29 Jun 2013 11:52:55 +0800
Wenchao Xia <address@hidden> 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 <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
> 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.
- Re: [Qemu-devel] [PATCH V5 1/7] monitor: avoid direct use of global *cur_mon in completion functions,
Luiz Capitulino <=