From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37107) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4M57-0005jT-JT for qemu-devel@nongnu.org; Tue, 30 Jul 2013 22:24:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4M4y-0005fo-8N for qemu-devel@nongnu.org; Tue, 30 Jul 2013 22:24:05 -0400 Received: from e28smtp03.in.ibm.com ([122.248.162.3]:41724) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4M4x-0005fG-Jd for qemu-devel@nongnu.org; Tue, 30 Jul 2013 22:23:56 -0400 Received: from /spool/local by e28smtp03.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 31 Jul 2013 07:46:46 +0530 Received: from d28relay03.in.ibm.com (d28relay03.in.ibm.com [9.184.220.60]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id C9D44394004D for ; Wed, 31 Jul 2013 07:53:40 +0530 (IST) Received: from d28av03.in.ibm.com (d28av03.in.ibm.com [9.184.220.65]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r6V2Olc744171412 for ; Wed, 31 Jul 2013 07:54:48 +0530 Received: from d28av03.in.ibm.com (loopback [127.0.0.1]) by d28av03.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r6V2NjB3007016 for ; Wed, 31 Jul 2013 12:23:45 +1000 Message-ID: <51F8752B.4050805@linux.vnet.ibm.com> Date: Wed, 31 Jul 2013 10:23:39 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1374808842-11051-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1374808842-11051-10-git-send-email-xiawenc@linux.vnet.ibm.com> <20130730105120.2b9dd16a@redhat.com> In-Reply-To: <20130730105120.2b9dd16a@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V8 09/13] monitor: support sub command in help List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com 于 2013-7-30 22:51, Luiz Capitulino 写道: > On Fri, 26 Jul 2013 11:20:38 +0800 > Wenchao Xia wrote: > >> The old code in help_cmd() uses global 'info_cmds' and treats it as a >> special case. Actually 'info_cmds' is a sub command group of 'mon_cmds', >> in order to avoid direct use of it, help_cmd() needs to change its work >> mechanism to support sub command and not treat it as a special case >> any more. >> >> To support sub command, help_cmd() will first parse the input and then call >> help_cmd_dump(), which works as a reentrant function. When it meets a sub >> command, it simply enters the function again. Since help dumping needs to >> know whole input to printf full help message include prefix, for example, >> "help info block" need to printf prefix "info", so help_cmd_dump() takes all >> args from input and extra parameter arg_index to identify the progress. >> Another function help_cmd_dump_one() is introduced to printf the prefix >> and command's help message. >> >> Now help supports sub command, so later if another sub command group is >> added in any depth, help will automatically work for it. Still "help info >> block" will show error since command parser reject additional parameter, >> which can be improved later. "log" is still treated as a special case. >> >> Signed-off-by: Wenchao Xia >> Reviewed-by: Eric Blake >> --- >> monitor.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++--------- >> 1 files changed, 53 insertions(+), 10 deletions(-) >> >> diff --git a/monitor.c b/monitor.c >> index c942b77..77df88d 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -868,33 +868,76 @@ static int parse_cmdline(const char *cmdline, >> return -1; >> } >> >> +static void help_cmd_dump_one(Monitor *mon, >> + const mon_cmd_t *cmd, >> + char **prefix_args, >> + int prefix_args_nb) >> +{ >> + int i; >> + >> + for (i = 0; i < prefix_args_nb; i++) { >> + monitor_printf(mon, "%s ", prefix_args[i]); >> + } >> + monitor_printf(mon, "%s %s -- %s\n", cmd->name, cmd->params, cmd->help); >> +} >> + >> +/* @args[@arg_index] is the valid command need to find in @cmds */ >> static void help_cmd_dump(Monitor *mon, const mon_cmd_t *cmds, >> - const char *prefix, const char *name) >> + char **args, int nb_args, int arg_index) >> { >> const mon_cmd_t *cmd; >> >> - for(cmd = cmds; cmd->name != NULL; cmd++) { >> - if (!name || !strcmp(name, cmd->name)) >> - monitor_printf(mon, "%s%s %s -- %s\n", prefix, cmd->name, >> - cmd->params, cmd->help); >> + /* No valid arg need to compare with, dump all in *cmds */ >> + if (arg_index >= nb_args) { >> + for (cmd = cmds; cmd->name != NULL; cmd++) { >> + help_cmd_dump_one(mon, cmd, args, arg_index); >> + } >> + return; >> + } >> + >> + /* Find one entry to dump */ >> + for (cmd = cmds; cmd->name != NULL; cmd++) { >> + if (compare_cmd(args[arg_index], cmd->name)) { >> + if (cmd->sub_table) { >> + /* continue with next arg */ >> + help_cmd_dump(mon, cmd->sub_table, >> + args, nb_args, arg_index + 1); >> + } else { >> + help_cmd_dump_one(mon, cmd, args, arg_index); >> + } >> + break; >> + } >> } >> } >> >> static void help_cmd(Monitor *mon, const char *name) >> { >> - if (name && !strcmp(name, "info")) { >> - help_cmd_dump(mon, info_cmds, "info ", NULL); >> - } else { >> - help_cmd_dump(mon, mon->cmd_table, "", name); >> - if (name && !strcmp(name, "log")) { >> + char *args[MAX_ARGS]; >> + int nb_args = 0; >> + >> + /* 1. parse user input */ >> + if (name) { >> + /* special case for log, directly dump and return */ >> + if (!strcmp(name, "log")) { >> const QEMULogItem *item; >> monitor_printf(mon, "Log items (comma separated):\n"); >> monitor_printf(mon, "%-10s %s\n", "none", "remove all logs"); >> for (item = qemu_log_items; item->mask != 0; item++) { >> monitor_printf(mon, "%-10s %s\n", item->name, item->help); >> } >> + return; >> + } >> + >> + if (parse_cmdline(name, &nb_args, args) < 0) { >> + goto cleanup; > > Won't this result in a double-free if parse_cmdline() fails? > > The fix is just to return instead of going to cleanup. I can do this > change if you agree and if I don't spot another bug. > nb_args = 0 in that case, I think it would not free again. Still, I am OK to do this change, since the parse_cmdline() already does the clean up, not need to ask caller to do it again. >> } >> } >> + >> + /* 2. dump the contents according to parsed args */ >> + help_cmd_dump(mon, mon->cmd_table, args, nb_args, 0); >> + >> +cleanup: >> + free_cmdline_args(args, nb_args); >> } >> >> static void do_help_cmd(Monitor *mon, const QDict *qdict) > -- Best Regards Wenchao Xia