From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33647) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VEVSe-0003fD-UI for qemu-devel@nongnu.org; Tue, 27 Aug 2013 22:26:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VEVSX-0000tg-0D for qemu-devel@nongnu.org; Tue, 27 Aug 2013 22:26:20 -0400 Received: from e23smtp06.au.ibm.com ([202.81.31.148]:46136) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VEVSO-0000qx-RY for qemu-devel@nongnu.org; Tue, 27 Aug 2013 22:26:12 -0400 Received: from /spool/local by e23smtp06.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 28 Aug 2013 12:17:11 +1000 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [9.190.235.21]) by d23dlp01.au.ibm.com (Postfix) with ESMTP id 3F2752CE804C for ; Wed, 28 Aug 2013 12:25:42 +1000 (EST) Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r7S2PUM762849106 for ; Wed, 28 Aug 2013 12:25:31 +1000 Received: from d23av01.au.ibm.com (localhost [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id r7S2Pfo4032619 for ; Wed, 28 Aug 2013 12:25:41 +1000 Message-ID: <521D5F65.2050603@linux.vnet.ibm.com> Date: Wed, 28 Aug 2013 10:24:37 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1374808842-11051-1-git-send-email-xiawenc@linux.vnet.ibm.com> <20130730120311.7f1e44a7@redhat.com> <20130820100442.1abc070d@redhat.com> <5215D6E7.30708@linux.vnet.ibm.com> <20130822091248.19ad2e2a@redhat.com> In-Reply-To: <20130822091248.19ad2e2a@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V8 00/13] monitor: support sub command group in auto completion and help List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: pbonzini@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org 于 2013-8-22 21:12, Luiz Capitulino 写道: > On Thu, 22 Aug 2013 17:16:23 +0800 > Wenchao Xia wrote: > >> 于 2013-8-20 22:04, Luiz Capitulino 写道: >>> On Tue, 30 Jul 2013 12:03:11 -0400 >>> Luiz Capitulino wrote: >>> >>>> On Fri, 26 Jul 2013 11:20:29 +0800 >>>> Wenchao Xia 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. >>> >>> Unfortunately this series brakes make check: >>> >>> GTESTER check-qtest-x86_64 >>> Broken pipe >>> GTester: last random seed: R02S3492bd34f44dd17460851643383be44d >>> main-loop: WARNING: I/O thread spun for 1000 iterations >>> make: *** [check-qtest-x86_64] Error 1 >>> >>> I debugged it (with some help from Laszlo) and the problem is that it >>> broke the human-monitor-command command. Any usage of this command >>> triggers the bug like: >>> >>> { "execute": "human-monitor-command", >>> "arguments": { "command-line": "info registers" } } >>> >>> It seems simple to fix, I think you just have to initialize >>> mon->cmd_table in qmp_human_monitor_command(), but I'd recommend two >>> things: >>> >>> 1. It's better to split off some/all QMP initialization from >>> monitor_init() and call it from qmp_human_monitor_command() >>> >>> 2. Can you please take the opportunity and test all commands using >>> cur_mon? Just grep for it >>> >>> Sorry for noticing this only now, but I only run make check before >>> sending a pull request (although this very likely shows you didn't >>> run it either). >>> >> About the fd related qmp interface, to test it, send_msg() is needed, >> which was not supported in python 2, but new added python 3.3. I think >> there are three ways to add test cases for fd qmp APIs: >> 1 test only when python > 3.3. >> 2 python plus C: compile a .so and call it with ctypes. >> 3 a new test framework: pure C code to call qmp interfaces. >> Which one do you prefer? > > Can't we have a C program plus a shell script to test this? Anyway, if > this gets complicated you can skip having the test-case. This series took > a long way already and holding it because of that test-case isn't fair. > C program can work, I tested it with currently qemu-iotest infra, when fork() the fd wouldn't be closed in child process, so the child C program can use the fds openned in parent python program. With shell script I need to build up some basic infra such as vm boot up, monitor connect, json parsing, so used qemu-iotest python infra instead of shell script. Based on that, I have sent a separate series to add the test case. http://lists.nongnu.org/archive/html/qemu-devel/2013-08/msg03978.html -- Best Regards Wenchao Xia