From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35519) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uwo9y-0003sR-DF for qemu-devel@nongnu.org; Wed, 10 Jul 2013 02:45:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uwo9v-0004ZB-UO for qemu-devel@nongnu.org; Wed, 10 Jul 2013 02:45:54 -0400 Received: from e23smtp04.au.ibm.com ([202.81.31.146]:36475) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uwo9v-0004Yu-BP for qemu-devel@nongnu.org; Wed, 10 Jul 2013 02:45:51 -0400 Received: from /spool/local by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 10 Jul 2013 16:30:38 +1000 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [9.190.235.21]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id 0A7A02BB004F for ; Wed, 10 Jul 2013 16:45:44 +1000 (EST) Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r6A6jYg72097642 for ; Wed, 10 Jul 2013 16:45:34 +1000 Received: from d23av02.au.ibm.com (loopback [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r6A6jh8Z006712 for ; Wed, 10 Jul 2013 16:45:43 +1000 Message-ID: <51DD02F1.8010904@linux.vnet.ibm.com> Date: Wed, 10 Jul 2013 14:45:05 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1372477981-7512-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1372477981-7512-5-git-send-email-xiawenc@linux.vnet.ibm.com> <20130708114547.2a52d409@redhat.com> In-Reply-To: <20130708114547.2a52d409@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V5 4/7] monitor: avoid direct use of global *info_cmds in help functions 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-8 23:45, Luiz Capitulino 写道: > On Sat, 29 Jun 2013 11:52:58 +0800 > Wenchao Xia wrote: > >> In help functions info_cmds is treated as sub command group now, not as >> a special case any more. Still help can't show message for single command >> under "info", since command parser reject additional parameter, which > > What you meant by "help can't show message for single command"? > I mean "help info block" can't work. >> can be improved by change "help" item parameter define later. "log" is >> still treated as special help case. compare_cmd() is used instead of strcmp() >> in command searching. > > I'm honestly a bit confused with this patch, I think it will be clarified > further down in the series, but might be a good idea to re-work the commit log. > More questions below. > To avoid use of info_cmds, the help function need to use sub command structure, otherwise "info" is a special case, so I modified the functions. I will refine the commit message. >> >> Signed-off-by: Wenchao Xia >> --- >> monitor.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++--------- >> 1 files changed, 53 insertions(+), 10 deletions(-) >> >> diff --git a/monitor.c b/monitor.c >> index 03a017d..bc62fc7 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -831,33 +831,76 @@ static void parse_cmdline(const char *cmdline, >> *pnb_args = nb_args; >> } >> >> +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]); >> + } > > What is this for? > It is used to print parent command, such as "info" in "info block" help_cmd_dump(): - monitor_printf(mon, "%s%s %s -- %s\n", prefix, cmd->name, - cmd->params, cmd->help); The old code can't work with sub command for parameter prefix. To solve it, I introduced function help_cmd_dump_one(), which dedicate to format control and knows parent commands. This can avoid dynamic snprintf to a buffer for prefix. >> + monitor_printf(mon, "%s %s -- %s\n", cmd->name, cmd->params, cmd->help); >> +} >> + >> 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); >> + /* Dump all */ >> + if (arg_index >= nb_args) { >> + for (cmd = cmds; cmd->name != NULL; cmd++) { >> + help_cmd_dump_one(mon, cmd, args, arg_index); >> + } >> + return; >> + } > > Maybe this should be moved to help_cmd() so that it's not a special > case here? > help_cmd_dump() is changed as a re-enterable function to handle sub command case, so above lines need to be inside a re-enterable function, to handle the case that dump all commands under one group, such as "help info". Moving it out will make it work only when folder depth is 1. >> + >> + /* Find one entry to dump */ >> + for (cmd = cmds; cmd->name != NULL; cmd++) { >> + if (compare_cmd(args[arg_index], cmd->name)) { >> + if (cmd->sub_table) { >> + 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, i; >> + >> + if (name) { >> + /* special case for log */ >> + 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; >> + } >> + >> + parse_cmdline(name, &nb_args, args); >> + if (nb_args >= MAX_ARGS) { >> + goto cleanup; >> } > > parse_cmdline() should handle de-allocation on failure, also checking > nb_args for failure is a bad API. This hasn't been a problem so far > because parse_cmdline() is used only once, but now you're making it a > bit more generic so it should be improved. > OK, I will improve it in an previous patch. >> } >> + >> + help_cmd_dump(mon, mon->cmd_table, args, nb_args, 0); >> + >> +cleanup: >> + nb_args = nb_args < MAX_ARGS ? nb_args : MAX_ARGS; >> + for (i = 0; i < nb_args; i++) { >> + g_free(args[i]); >> + } > > I'd add free_cmdline_args(). > OK. >> } >> >> static void do_help_cmd(Monitor *mon, const QDict *qdict) > -- Best Regards Wenchao Xia