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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Fri, 26 Jul 2013 11:20:29 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 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.

I've applied this to qmp-next with the change I suggested for
patch 09/13.

> 
> Thanks for Luiz and Eric for 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:
>   Address Luiz's comments:
>   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().
> 
> v8:
>   Address Eric's comments:
>   Fix typo in commit messages.
> 
> 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(-)
> 
>