Re: [Qemu-devel] [PATCH V7 00/13] monitor: support sub command group in auto completion and help

[Wenchao Xia]

于 2013-7-20 9:44, Wenchao Xia 写道:
> This series make auto completion and help functions works normal for sub
> command, by using reentrant functions. In order to do that, global variables
> are not directly used in those functions any more. With this series, cmd_table
> is a member of structure Monitor so it is possible to create a monitor with
> different command table now, auto completion will work in that monitor. In
> short, "info" is not treated as a special case now, this series ensure help
> and auto complete function works normal for any sub command added in the 
> future.
> 
> Patch 5 replaced cur_mon with rs->mon, it is safe because:
> monitor_init() calls readline_init() which initialize mon->rs, result is
> mon->rs->mon == mon. Then qemu_chr_add_handlers() is called, which make
> monitor_read() function take *mon as its opaque. Later, when user input,
> monitor_read() is called, where cur_mon is set to *mon by "cur_mon = opaque".
> If qemu's monitors run in one thread, then later in readline_handle_byte()
> and readline_comletion(), cur_mon is actually equal to rs->mon, in another
> word, it points to the monitor instance, so it is safe to replace *cur_mon
> in those functions.
> 
> Thanks for Luiz's reviewing.
> 
> V2:
>    General:
>    To discard *info_comds more graceful, help related function is modified to 
> support
> sub command too.
>    Patch 6/7 are added to improve help related functions.
>    Patch 5: not directly return to make sure args are freed.
> 
>    Address Luiz's comments:
>    Split patch into small series.
>    struct mon_cmd_t was not moved into header file, instead mon_cmnd_t 
> *cmd_table is
> added as a member in struct Monitor.
>    5/7: drop original code comments for "info" in monitor_find_completion().
> 
> v3:
>    5/7: add parameter **args_cmdline in parse_cmdline() to tell next valid
> parameter's position. This fix the issue in case command length in input is 
> not
> equal to its name's length such as "help|?", and the case input start with
> space such as "  s".
>    7/7: better commit message.
> 
> v4:
>    Address Eric's comments:
>    1/7, 2/7, 4/7: better commit title and message.
>    1/7 remove useless (char *) in old code, add space around "for ()" in old 
> code.
>    3/7: separate code moving patch before usage.
>    4/7: add space around "for ()" in old code, add min(nb_args, MAX_ARGS) in 
> free
> to make code stronger.
> 
> v5:
>    4/7: use "a < b ? a : b" instead of macro min.
> 
> v6:
>    Address Luiz's comments:
>    1/13 ~ 5/13: splitted small patches.
>    5/13: added commit message about the correctness of replacing of cur_mon 
> and
> test result.
>    6/13: better comments in code.
>    7/13: added commit message about the reason of code moving.
>    8/13: new patch to improve parse_cmdline(), since it is a more generic
> function now.
>    9/13: reworked the commit message, better commentes in code, use
> free_cmdline_args() in clean. It is a bit hard to split this patch into
> smaller meaning ful ones, so kepted this patch as a relative larger one,
> with better commit message.
>    12/13: put case 'S' with case 's' in monitor_find_completion_by_table().
> moved this patch ahead of patch 13/13.
>    13/13: this patch is moved behind patch 12/13.
> 
>    Generic change:
>    10/13: splitted patch which moved out the reentrant part into a separate
> function, make review easier. This also avoided re-parsing the command line
> which does in previous version.
>    11/13: splitted patch, which simply remove usage of info_cmds and support
> sub command by re-enter the function.
> 
> v7:
>    5/13: moved the comments why the change is safe, to cover-letter.
>    8/13: use assert in free_cmdline_args(), fail when args in input exceed
> the limit in parse_cmdline().
> 
> Wenchao Xia (13):
>    1 monitor: avoid use of global *cur_mon in cmd_completion()
>    2 monitor: avoid use of global *cur_mon in file_completion()
>    3 monitor: avoid use of global *cur_mon in block_completion_it()
>    4 monitor: avoid use of global *cur_mon in monitor_find_completion()
>    5 monitor: avoid use of global *cur_mon in readline_completion()
>    6 monitor: avoid direct use of global variable *mon_cmds
>    7 monitor: code move for parse_cmdline()
>    8 monitor: refine parse_cmdline()
>    9 monitor: support sub command in help
>    10 monitor: refine monitor_find_completion()
>    11 monitor: support sub command in auto completion
>    12 monitor: allow "help" show message for single command in sub group
>    13 monitor: improve auto complete of "help" for single command in sub group
> 
>   hmp-commands.hx            |    2 +-
>   include/monitor/readline.h |    3 +-
>   monitor.c                  |  438 
> ++++++++++++++++++++++++++++----------------
>   readline.c                 |    5 +-
>   4 files changed, 289 insertions(+), 159 deletions(-)
> 
> 
  Luiz, do you like a respin for this version? There are some typo in
commit message Eric have pointed out.

-- 
Best Regards

Wenchao Xia