Re: [Qemu-devel] [PATCH 2/3] HMP: pass in parameter for info sub command

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 20 Dec 2012 11:02:16 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

>  > On Wed, 19 Dec 2012 18:17:09 +0800
> > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >
> >>    This patch enable sub info command handler getting meaningful
> >> parameter.
> >>
> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >> ---
> >>   hmp-commands.hx |    2 +-
> >>   monitor.c       |   79 +++++++++++++++++++++++++++++++++++++++----------------
> >>   2 files changed, 57 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/hmp-commands.hx b/hmp-commands.hx
> >> index 010b8c9..667fab8 100644
> >> --- a/hmp-commands.hx
> >> +++ b/hmp-commands.hx
> >> @@ -1486,7 +1486,7 @@ ETEXI
> >>
> >>       {
> >>           .name       = "info",
> >> -        .args_type  = "item:s?",
> >> +        .args_type  = "item:S?",
> >>           .params     = "[subcommand]",
> >>           .help       = "show various information about the system state",
> >>           .mhandler.cmd = do_info,
> >> diff --git a/monitor.c b/monitor.c
> >> index 797680f..ce0e74d 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -464,6 +464,11 @@ QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
> >>   MonitorEventState monitor_event_state[QEVENT_MAX];
> >>   QemuMutex monitor_event_state_lock;
> >>
> >> +static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> >> +                                              const char *cmdline,
> >> +                                              const mon_cmd_t *table,
> >> +                                              QDict *qdict);
> >
> > Please, move the function instead.
> >
>    There will be a bit big of code moving then, hope you would be OK with
> it.

That's fine, but please do it in a different patch(es).

> >> +
> >>   /*
> >>    * Emits the event to every monitor instance
> >>    */
> >> @@ -809,26 +814,29 @@ static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
> >>   static void do_info(Monitor *mon, const QDict *qdict)
> >>   {
> >>       const mon_cmd_t *cmd;
> >> +    QDict *qdict_info;
> >>       const char *item = qdict_get_try_str(qdict, "item");
> >>
> >>       if (!item) {
> >>           goto help;
> >>       }
> >>
> >> -    for (cmd = info_cmds; cmd->name != NULL; cmd++) {
> >> -        if (compare_cmd(item, cmd->name))
> >> -            break;
> >> -    }
> >> +    qdict_info = qdict_new();
> >>
> >> -    if (cmd->name == NULL) {
> >> -        goto help;
> >> +    cmd = monitor_parse_command(mon, item, info_cmds, qdict_info);
> >> +    if (!cmd) {
> >> +        QDECREF(qdict_info);
> >> +        /* don't help here, to avoid error message got ignored */
> >> +        return;
> >>       }
> >>
> >> -    cmd->mhandler.info(mon, NULL);
> >> +    cmd->mhandler.info(mon, qdict_info);
> >> +    QDECREF(qdict_info);
> >>       return;
> >>
> >>   help:
> >>       help_cmd(mon, "info");
> >> +    return;
> >>   }
> >>
> >>   CommandInfoList *qmp_query_commands(Error **errp)
> >> @@ -3534,18 +3542,15 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table,
> >>       return NULL;
> >>   }
> >>
> >> -static const mon_cmd_t *monitor_find_command(const char *cmdname)
> >> +static const mon_cmd_t *monitor_find_command(const char *cmdname,
> >> +                                             const mon_cmd_t *table)
> >>   {
> >> -    return search_dispatch_table(mon_cmds, cmdname);
> >> -}
> >> -
> >> -static const mon_cmd_t *qmp_find_cmd(const char *cmdname)
> >> -{
> >> -    return search_dispatch_table(qmp_cmds, cmdname);
> >> +    return search_dispatch_table(table, cmdname);
> >
> > Please, keep only search_dispatch_table().
> >
>    OK.
> 
> > Actually, maybe you could change handle_qmp_command() to just loop through
> > command names and compare them with memcpy() (in a different patch, please)
> > then you keep monitor_find_command() for handle_hmp_command().
> >
>    ah, then monitor_find_command() serve HMP only. How about rename it to
> hmp_find_command(table, name)? This can have a more clear meaning. Or

That would be fine.

> just remove this funtion but use search_dispatch_table() every where?
> 
> >>   }
> >>
> >>   static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> >>                                                 const char *cmdline,
> >> +                                              const mon_cmd_t *table,
> >>                                                 QDict *qdict)
> >>   {
> >>       const char *p, *typestr;
> >> @@ -3564,7 +3569,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> >>       if (!p)
> >>           return NULL;
> >>
> >> -    cmd = monitor_find_command(cmdname);
> >> +    cmd = monitor_find_command(cmdname, table);
> >>       if (!cmd) {
> >>           monitor_printf(mon, "unknown command: '%s'\n", cmdname);
> >>           return NULL;
> >> @@ -3872,6 +3877,31 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> >>                   }
> >>               }
> >>               break;
> >> +        case 'S':
> >> +            {
> >> +                /* package all remaining string */
> >> +                int len;
> >> +
> >> +                while (qemu_isspace(*p)) {
> >> +                    p++;
> >> +                }
> >> +                if (*typestr == '?') {
> >> +                    typestr++;
> >> +                    if (*p == '\0') {
> >> +                        /* no remaining string: NULL argument */
> >> +                        break;
> >> +                    }
> >> +                }
> >> +                len = strlen(p);
> >> +                if (len <= 0) {
> >> +                    monitor_printf(mon, "%s: string expected\n",
> >> +                                   cmdname);
> >> +                    break;
> >> +                }
> >> +                qdict_put(qdict, key, qstring_from_str(p));
> >> +                p += len;
> >> +            }
> >
> > This is a true HMP-style hack :)
> >
> > I don't know how to make this better though (at least not without asking you
> > to re-write the whole thing). Maybe Markus has a better idea?
> >
>    My idea was adding a new tag meaning "package all remaining string
> into the key value", what was I can found best before. Hope you have
> a better solution.
> 
> >> +            break;
> >>           default:
> >>           bad_type:
> >>               monitor_printf(mon, "%s: unknown type '%c'\n", cmdname, c);
> >> @@ -3925,7 +3955,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
> >>
> >>       qdict = qdict_new();
> >>
> >> -    cmd = monitor_parse_command(mon, cmdline, qdict);
> >> +    cmd = monitor_parse_command(mon, cmdline, mon_cmds, qdict);
> >>       if (!cmd)
> >>           goto out;
> >>
> >> @@ -4144,12 +4174,7 @@ static void monitor_find_completion(const char *cmdline)
> >>               break;
> >>           case 's':
> >>               /* XXX: more generic ? */
> >> -            if (!strcmp(cmd->name, "info")) {
> >> -                readline_set_completion_index(cur_mon->rs, strlen(str));
> >> -                for(cmd = info_cmds; cmd->name != NULL; cmd++) {
> >> -                    cmd_completion(str, cmd->name);
> >> -                }
> >> -            } else if (!strcmp(cmd->name, "sendkey")) {
> >> +            if (!strcmp(cmd->name, "sendkey")) {
> >>                   char *sep = strrchr(str, '-');
> >>                   if (sep)
> >>                       str = sep + 1;
> >> @@ -4163,6 +4188,14 @@ static void monitor_find_completion(const char *cmdline)
> >>                       cmd_completion(str, cmd->name);
> >>                   }
> >>               }
> >> +        case 'S':
> >> +            /* generic string packaged */
> >> +            if (!strcmp(cmd->name, "info")) {
> >> +                readline_set_completion_index(cur_mon->rs, strlen(str));
> >> +                for (cmd = info_cmds; cmd->name != NULL; cmd++) {
> >> +                    cmd_completion(str, cmd->name);
> >> +                }
> >> +            }
> >>               break;
> >>           default:
> >>               break;
> >> @@ -4483,7 +4516,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
> >>           goto err_out;
> >>       }
> >>
> >> -    cmd = qmp_find_cmd(cmd_name);
> >> +    cmd = monitor_find_command(cmd_name, qmp_cmds);
> >>       if (!cmd) {
> >>           qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
> >>           goto err_out;
> >
> >
> 
>