[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V2 3/7] monitor: discard global variable *info_c
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH V2 3/7] monitor: discard global variable *info_cmds in help functions |
Date: |
Thu, 27 Jun 2013 13:26:36 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6 |
On 06/24/2013 06:48 AM, Wenchao Xia wrote:
In the subject line, you aren't actually discarding a global variable,
so much as avoiding its direct use (the global still exists). Maybe a
better subject line would be:
monitor: avoid direct use of global *info_cmds in help functions
(2/7 has the same wording issue, where you aren't discarding the variable).
> 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.
>
> To tip better what the patch does, code moving is avoided by declare
s/tip better/give better hints about/
s/moving/motion/
s/declare/declaring/
> parse_cmdline() ahead.
Rather than avoiding code motion by adding a forward declaration, I
would instead split this into two patches - one that does JUST code
motion, then the other that takes advantage of the correct motion.
However, it's not a show-stopper to review.
> 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;
>
> + /* 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++) {
Pre-existing formatting issue, but as long as you are touching both
before and after, you might as well add the space after 'for'.
> 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;
[1]
> }
> }
> +
> + help_cmd_dump(mon, mon->cmd_table, args, nb_args, 0);
> +
> +cleanup:
> + for (i = 0; i < nb_args; i++) {
> + g_free(args[i]);
If we got here because nb_args > MAX_ARGS at point [1], then this calls
g_free() on memory that is beyond the array (bad). Thankfully, I just
read parse_cmdline, and it never sets nb_args > MAX_ARGS. But this
whole parsing feels rather fragile (not necessarily your fault).
Although I still recommend doing proper code motion for topological
ordering instead of using a crutch of forward declarations, I'm okay if
you fix the commit message and add Reviewed-by: Eric Blake
<address@hidden>.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH V2 0/7] monitor: support sub command group in auto completion and help, Wenchao Xia, 2013/06/24
- [Qemu-devel] [PATCH V2 4/7] monitor: code move for parse_cmdline(), Wenchao Xia, 2013/06/24
- [Qemu-devel] [PATCH V2 7/7] monitor: improve "help" to allow show tip of single command in sub group, Wenchao Xia, 2013/06/24
- [Qemu-devel] [PATCH V2 6/7] monitor: improve "help" in auto completion for sub command, Wenchao Xia, 2013/06/24
- Re: [Qemu-devel] [PATCH V2 0/7] monitor: support sub command group in auto completion and help, Eric Blake, 2013/06/27