On Sat, 29 Jun 2013 11:52:58 +0800
Wenchao Xia <xiaw...@linux.vnet.ibm.com> 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"?
> 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.
>
> Signed-off-by: Wenchao Xia <xiaw...@linux.vnet.ibm.com>
> ---
> 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?
> + 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?
> +
> + /* 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.
> }
> +
> + 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().
> }
>
> static void do_help_cmd(Monitor *mon, const QDict *qdict)
