From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38641) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TtV3D-0003fF-O3 for qemu-devel@nongnu.org; Thu, 10 Jan 2013 22:13:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TtV3A-0005rT-EA for qemu-devel@nongnu.org; Thu, 10 Jan 2013 22:12:59 -0500 Received: from e28smtp01.in.ibm.com ([122.248.162.1]:54762) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TtV39-0005qy-Hk for qemu-devel@nongnu.org; Thu, 10 Jan 2013 22:12:56 -0500 Received: from /spool/local by e28smtp01.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 11 Jan 2013 08:41:26 +0530 Received: from d28relay04.in.ibm.com (d28relay04.in.ibm.com [9.184.220.61]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id 9291A394004E for ; Fri, 11 Jan 2013 08:42:35 +0530 (IST) Received: from d28av02.in.ibm.com (d28av02.in.ibm.com [9.184.220.64]) by d28relay04.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r0B3CXZe2621852 for ; Fri, 11 Jan 2013 08:42:33 +0530 Received: from d28av02.in.ibm.com (loopback [127.0.0.1]) by d28av02.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r0B3CYMk028964 for ; Fri, 11 Jan 2013 14:12:35 +1100 Message-ID: <50EF82CD.7000902@linux.vnet.ibm.com> Date: Fri, 11 Jan 2013 11:11:09 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1357806754-8552-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1357806754-8552-3-git-send-email-xiawenc@linux.vnet.ibm.com> <87k3rll4te.fsf@blackfin.pond.sub.org> In-Reply-To: <87k3rll4te.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=GB2312 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V4 2/4] HMP: add infrastructure for sub command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, chenwj@iis.sinica.edu.tw, lcapitulino@redhat.com > Wenchao Xia writes: > >> This patch make parsing of hmp command aware of that it may >> have sub command. Also discard simple encapsulation function >> monitor_find_command() and qmp_find_cmd(). >> >> Signed-off-by: Wenchao Xia >> --- >> monitor.c | 31 +++++++++++++++++++++++-------- >> 1 files changed, 23 insertions(+), 8 deletions(-) >> >> diff --git a/monitor.c b/monitor.c >> index c7b3014..d49a362 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -130,6 +130,7 @@ typedef struct mon_cmd_t { >> MonitorCompletion *cb, void *opaque); >> } mhandler; >> int flags; >> + struct mon_cmd_t *sub_table; >> } mon_cmd_t; >> >> /* file descriptors passed via SCM_RIGHTS */ >> @@ -3534,21 +3535,22 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table, >> return NULL; >> } >> >> -static const mon_cmd_t *monitor_find_command(const char *cmdname) >> -{ >> - return search_dispatch_table(mon_cmds, cmdname); >> -} >> - >> static const mon_cmd_t *qmp_find_cmd(const char *cmdname) >> { >> return search_dispatch_table(qmp_cmds, cmdname); >> } >> >> +/* >> + * Try find the target mon_cmd_t instance. If it have sub_table and have string >> + * for it, this function will try search it with remaining string, and if not >> + * found it return NULL. >> + */ > > Thanks for going the extra mile and document the function. What about: > > /* > * Parse @cmdline according to command table @table. > * If @cmdline is blank, return NULL. > * If it can't be parsed, report to @mon, and return NULL. > * Else, insert command arguments into @qdict, and return the command. > * Do not assume the returned command points into @table! It doesn't > * when the command is a sub-command. > */ > OK, I'll use it in V5. >> static const mon_cmd_t *monitor_parse_command(Monitor *mon, >> const char *cmdline, >> + mon_cmd_t *table, >> QDict *qdict) >> { >> - const char *p, *typestr; >> + const char *p, *p1, *typestr; >> int c; >> const mon_cmd_t *cmd; >> char cmdname[256]; >> @@ -3564,12 +3566,25 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, >> if (!p) >> return NULL; >> >> - cmd = monitor_find_command(cmdname); >> + cmd = search_dispatch_table(table, cmdname); >> if (!cmd) { >> monitor_printf(mon, "unknown command: '%s'\n", cmdname); >> return NULL; >> } >> >> + /* search sub command */ >> + if (cmd->sub_table != NULL) { >> + p1 = p; >> + /* check if user set additional command */ >> + while (qemu_isspace(*p1)) { >> + p1++; >> + } >> + if (*p1 == '\0') { >> + return cmd; >> + } >> + return monitor_parse_command(mon, p, cmd->sub_table, qdict); >> + } >> + >> /* parse the parameters */ >> typestr = cmd->args_type; >> for(;;) { > > The check whether non-space characters follow is awkward. We need it > only because we want to handle "@cmdline is blank" differently than "it > can't be parsed", but monitor_parse_command() returns NULL for both > cases. If we care, we can try to do better in a follow-up patch. > Yes it is checked to avoid getting NULL for case "info ". I'll split up a patch for the check code. > We'll get sub-optimal error messages when the infrastructure is put to > use in patch 4/4, e.g.: > > (qemu) info apple pie > unknown command: 'apple' > > That's because cmdname is just "apple" at the place where the error is > diagnosed. > > Several other messages also print cmdname, and thus are similarly > affected. Many of them could probably just omit printing it, but that's > a separate issue. > > A possible way to improve this: additional parameter int start, parse > starts at cmdline + start, and instead of printing cmdname, print > cmdline up to end of cmdname. Something like > > monitor_printf(mon, "unknown command: '%.*s'\n", > (int)(p - cmdline), cmdline); > Thanks, I think this could work. >> @@ -3925,7 +3940,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline) >> >> qdict = qdict_new(); >> >> - cmd = monitor_parse_command(mon, cmdline, qdict); >> + cmd = monitor_parse_command(mon, cmdline, mon_cmds, qdict); >> if (!cmd) >> goto out; > -- Best Regards Wenchao Xia