Re: [Qemu-devel] [PATCH V9 00/15] monitor: support sub command group in auto completion and help
On Tue, 27 Aug 2013 20:39:37 +0800 Wenchao Xia xiaw...@linux.vnet.ibm.com wrote: 于 2013-8-27 9:43, Wenchao Xia 写道: 于 2013-8-26 23:55, Luiz Capitulino 写道: On Fri, 23 Aug 2013 16:17:52 +0800 Wenchao Xia xiaw...@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. Thanks for Luiz and Eric for reviewing. This one doesn't even build :( /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c: In function ‘help_cmd’: /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c:952:1: error: label ‘cleanup’ defined but not used [-Werror=unused-label] cc1: all warnings being treated as errors make[1]: *** [monitor.o] Error 1 make[1]: *** Waiting for unfinished jobs make: *** [subdir-x86_64-softmmu] Error 2 Sorry for it, I missed that CC flag in my configure, will fix. It would be great if you can share your configure settings. Respined v10 and tested with -Werror=unused-label, hope no other issue is missed in my test. My configure line doesn't have anything special: ../configure --target-list=x86_64-softmmu
Re: [Qemu-devel] [PATCH V9 00/15] monitor: support sub command group in auto completion and help
于 2013-8-27 9:43, Wenchao Xia 写道: 于 2013-8-26 23:55, Luiz Capitulino 写道: On Fri, 23 Aug 2013 16:17:52 +0800 Wenchao Xia xiaw...@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. Thanks for Luiz and Eric for reviewing. This one doesn't even build :( /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c: In function ‘help_cmd’: /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c:952:1: error: label ‘cleanup’ defined but not used [-Werror=unused-label] cc1: all warnings being treated as errors make[1]: *** [monitor.o] Error 1 make[1]: *** Waiting for unfinished jobs make: *** [subdir-x86_64-softmmu] Error 2 Sorry for it, I missed that CC flag in my configure, will fix. It would be great if you can share your configure settings. Respined v10 and tested with -Werror=unused-label, hope no other issue is missed in my test. -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH V9 00/15] monitor: support sub command group in auto completion and help
于 2013-8-27 21:19, Luiz Capitulino 写道: On Tue, 27 Aug 2013 20:39:37 +0800 Wenchao Xia xiaw...@linux.vnet.ibm.com wrote: 于 2013-8-27 9:43, Wenchao Xia 写道: 于 2013-8-26 23:55, Luiz Capitulino 写道: On Fri, 23 Aug 2013 16:17:52 +0800 Wenchao Xia xiaw...@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. Thanks for Luiz and Eric for reviewing. This one doesn't even build :( /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c: In function ‘help_cmd’: /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c:952:1: error: label ‘cleanup’ defined but not used [-Werror=unused-label] cc1: all warnings being treated as errors make[1]: *** [monitor.o] Error 1 make[1]: *** Waiting for unfinished jobs make: *** [subdir-x86_64-softmmu] Error 2 Sorry for it, I missed that CC flag in my configure, will fix. It would be great if you can share your configure settings. Respined v10 and tested with -Werror=unused-label, hope no other issue is missed in my test. My configure line doesn't have anything special: ../configure --target-list=x86_64-softmmu I used special cc warn flag before, it seems that by default more flags would be set, will double check with default configure. -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH V9 00/15] monitor: support sub command group in auto completion and help
On Fri, 23 Aug 2013 16:17:52 +0800 Wenchao Xia xiaw...@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. Thanks for Luiz and Eric for reviewing. This one doesn't even build :( /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c: In function ‘help_cmd’: /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c:952:1: error: label ‘cleanup’ defined but not used [-Werror=unused-label] cc1: all warnings being treated as errors make[1]: *** [monitor.o] Error 1 make[1]: *** Waiting for unfinished jobs make: *** [subdir-x86_64-softmmu] Error 2
Re: [Qemu-devel] [PATCH V9 00/15] monitor: support sub command group in auto completion and help
于 2013-8-26 23:55, Luiz Capitulino 写道: On Fri, 23 Aug 2013 16:17:52 +0800 Wenchao Xia xiaw...@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. Thanks for Luiz and Eric for reviewing. This one doesn't even build :( /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c: In function ‘help_cmd’: /home/lcapitulino/work/src/upstream/qmp-unstable/monitor.c:952:1: error: label ‘cleanup’ defined but not used [-Werror=unused-label] cc1: all warnings being treated as errors make[1]: *** [monitor.o] Error 1 make[1]: *** Waiting for unfinished jobs make: *** [subdir-x86_64-softmmu] Error 2 Sorry for it, I missed that CC flag in my configure, will fix. It would be great if you can share your configure settings. -- Best Regards Wenchao Xia
[Qemu-devel] [PATCH V9 00/15] monitor: support sub command group in auto completion and help
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 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. V9: General: 6: new patch, I found that call can be optimized, so move it. Address Luiz's comments: 7: split function to init the monitor. 8: move the cmd_table init code to parse_cmdlinethe splitted function. 11: directly return in fail calling parse_cmdline(), in help_cmd_dump(). Other: After discuss with Luiz, it would be good to add test case for qmp fd-add related stuff. Current test case is 045, which doesn't cover the case that fd is added by SCM at runtime, and that case is related to monitor. I have drafted a version, which can pass both on upstream and this series, but it may require a bit more code, so send it later as another series. Wenchao Xia (15): 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: call sortcmdlist() only one time 7 monitor: split off monitor_data_init() 8 monitor: avoid direct use of global variable *mon_cmds 9 monitor: code move for parse_cmdline() 10 monitor: refine parse_cmdline() 11 monitor: support sub command in help 12 monitor: refine monitor_find_completion() 13 monitor: support
