Re: [Qemu-devel] [PATCH 1/2] monitor: discard global variable in auto completion functions

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

于 2013-6-21 23:24, Luiz Capitulino 写道:
> On Fri, 21 Jun 2013 14:37:37 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> In monitor_find_completion() and related functions, Global variable
>> *mon_cmds is not used any more, make them reenterable safely.
>> *cur_mon is also not used now. *info_cmds is still there, but soon
>> will be removed by a new way of sub command completion.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   include/monitor/readline.h |    8 ++++-
>>   monitor.c                  |   70 ++++++++++++++++++++++++++-----------------
>>   readline.c                 |    4 ++-
>>   3 files changed, 52 insertions(+), 30 deletions(-)
>>
>> diff --git a/include/monitor/readline.h b/include/monitor/readline.h
>> index fc9806e..a4c7bce 100644
>> --- a/include/monitor/readline.h
>> +++ b/include/monitor/readline.h
>> @@ -7,8 +7,12 @@
>>   #define READLINE_MAX_CMDS 64
>>   #define READLINE_MAX_COMPLETIONS 256
>>
>> +typedef struct mon_cmd_t mon_cmd_t;
>
> This should be in monitor.h. Maybe you could move struct mon_cmd_t there
> too.
>
 >
   Originally monitor.h include readline.h, readline.h need this def in
this patch, so putting it in monitor.h require readline.h include
monitor.h, break build. Do you have a better way to solve it? introduce
monitor-def.h?

   about struct mon_cmd_t, I think it is good to hide it in monitor.c.

> Also, can you split this series is smaller patches? It looks good, but
> it isn't easy to review because it changes too much code at once.
>
   Sure, let me try.

> Otherwise this is a nice feature!
>
>> +
>>   typedef void ReadLineFunc(Monitor *mon, const char *str, void *opaque);
>> -typedef void ReadLineCompletionFunc(const char *cmdline);
>> +typedef void ReadLineCompletionFunc(Monitor *mon,
>> +                                    mon_cmd_t *cmd_table,
>> +                                    const char *cmdline);
>>
>>   typedef struct ReadLineState {
>>       char cmd_buf[READLINE_CMD_BUF_SIZE + 1];
>> @@ -35,6 +39,7 @@ typedef struct ReadLineState {
>>       int read_password;
>>       char prompt[256];
>>       Monitor *mon;
>> +    mon_cmd_t *cmd_table;
>>   } ReadLineState;
>>
>>   void readline_add_completion(ReadLineState *rs, const char *str);
>> @@ -50,6 +55,7 @@ void readline_restart(ReadLineState *rs);
>>   void readline_show_prompt(ReadLineState *rs);
>>
>>   ReadLineState *readline_init(Monitor *mon,
>> +                             mon_cmd_t *cmd_table,
>>                                ReadLineCompletionFunc *completion_finder);
>>
>>   #endif /* !READLINE_H */
>> diff --git a/monitor.c b/monitor.c
>> index 70ae8f5..bc60171 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -116,7 +116,7 @@ struct MonitorCompletionData {
>>       void (*user_print)(Monitor *mon, const QObject *data);
>>   };
>>
>> -typedef struct mon_cmd_t {
>> +struct mon_cmd_t {
>>       const char *name;
>>       const char *args_type;
>>       const char *params;
>> @@ -134,7 +134,7 @@ typedef struct mon_cmd_t {
>>        * used, and mhandler of 1st level plays the role of help function.
>>        */
>>       struct mon_cmd_t *sub_table;
>> -} mon_cmd_t;
>> +};
>>
>>   /* file descriptors passed via SCM_RIGHTS */
>>   typedef struct mon_fd_t mon_fd_t;
>> @@ -3999,7 +3999,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];
>> @@ -4017,7 +4017,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;
>> @@ -4025,7 +4025,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;
>> @@ -4048,7 +4048,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);
>> @@ -4075,20 +4075,28 @@ 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);
>>   }
>>
>> -static void block_completion_it(void *opaque, BlockDriverState *bs)
>> +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);
>>       }
>>   }
>>
>> @@ -4124,18 +4132,21 @@ 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,
>> +                                    mon_cmd_t *cmd_table,
>> +                                    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]);
>> +        monitor_printf(mon, "arg%d = '%s'\n", i, (char *)args[i]);
>>       }
>>   #endif
>>
>> @@ -4154,13 +4165,13 @@ static void monitor_find_completion(const char *cmdline)
>>               cmdname = "";
>>           else
>>               cmdname = args[0];
>> -        readline_set_completion_index(cur_mon->rs, strlen(cmdname));
>> -        for(cmd = mon_cmds; cmd->name != NULL; cmd++) {
>> -            cmd_completion(cmdname, cmd->name);
>> +        readline_set_completion_index(mon->rs, strlen(cmdname));
>> +        for (cmd = cmd_table; cmd->name != NULL; cmd++) {
>> +            cmd_completion(mon, cmdname, cmd->name);
>>           }
>>       } else {
>>           /* find the command */
>> -        for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
>> +        for (cmd = cmd_table; cmd->name != NULL; cmd++) {
>>               if (compare_cmd(args[0], cmd->name)) {
>>                   break;
>>               }
>> @@ -4184,33 +4195,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));
>> -                for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
>> -                    cmd_completion(str, cmd->name);
>> +                readline_set_completion_index(mon->rs, strlen(str));
>> +                for (cmd = cmd_table; cmd->name != NULL; cmd++) {
>> +                    cmd_completion(mon, str, cmd->name);
>>                   }
>>               }
>>               break;
>> @@ -4751,7 +4764,8 @@ void monitor_init(CharDriverState *chr, int flags)
>>       mon->chr = chr;
>>       mon->flags = flags;
>>       if (flags & MONITOR_USE_READLINE) {
>> -        mon->rs = readline_init(mon, monitor_find_completion);
>> +        /* Always use *mon_cmds as entry now. */
>> +        mon->rs = readline_init(mon, mon_cmds, monitor_find_completion);
>>           monitor_read_command(mon, 0);
>>       }
>>
>> diff --git a/readline.c b/readline.c
>> index 1c0f7ee..d97e062 100644
>> --- a/readline.c
>> +++ b/readline.c
>> @@ -285,7 +285,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, rs->cmd_table, cmdline);
>>       g_free(cmdline);
>>
>>       /* no completion found */
>> @@ -482,12 +482,14 @@ const char *readline_get_history(ReadLineState *rs, unsigned int index)
>>   }
>>
>>   ReadLineState *readline_init(Monitor *mon,
>> +                             mon_cmd_t *cmd_table,
>>                                ReadLineCompletionFunc *completion_finder)
>>   {
>>       ReadLineState *rs = g_malloc0(sizeof(*rs));
>>
>>       rs->hist_entry = -1;
>>       rs->mon = mon;
>> +    rs->cmd_table = cmd_table;
>>       rs->completion_finder = completion_finder;
>>
>>       return rs;
>
>


-- 
Best Regards

Wenchao Xia