Message ID | 1372477981-7512-5-git-send-email-xiawenc@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
On Sat, 29 Jun 2013 11:52:58 +0800 Wenchao Xia <xiawenc@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 <xiawenc@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)
δΊ 2013-7-8 23:45, Luiz Capitulino ει: > On Sat, 29 Jun 2013 11:52:58 +0800 > Wenchao Xia <xiawenc@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"? > 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 <xiawenc@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? > 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) >
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]); + } + 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; + } + + /* 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; } } + + 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]); + } } static void do_help_cmd(Monitor *mon, const QDict *qdict)
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 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. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- monitor.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 files changed, 53 insertions(+), 10 deletions(-)