On Thu, 20 Dec 2012 11:02:16 +0800
Wenchao Xia <xiaw...@linux.vnet.ibm.com> wrote:
> > On Wed, 19 Dec 2012 18:17:09 +0800
> > Wenchao Xia <xiaw...@linux.vnet.ibm.com> wrote:
> >
> >> This patch enable sub info command handler getting meaningful
> >> parameter.
> >>
> >> Signed-off-by: Wenchao Xia <xiaw...@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;
> >
> >
>
>
