From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54715) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TlWPZ-0008ME-2L for qemu-devel@nongnu.org; Wed, 19 Dec 2012 22:03:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TlWPX-0003Oj-Iq for qemu-devel@nongnu.org; Wed, 19 Dec 2012 22:03:04 -0500 Received: from e23smtp08.au.ibm.com ([202.81.31.141]:46952) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TlWPW-0003OT-Pd for qemu-devel@nongnu.org; Wed, 19 Dec 2012 22:03:03 -0500 Received: from /spool/local by e23smtp08.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 20 Dec 2012 13:01:39 +1000 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [9.190.235.21]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id E59962BB004B for ; Thu, 20 Dec 2012 14:02:52 +1100 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id qBK32pTt62193870 for ; Thu, 20 Dec 2012 14:02:52 +1100 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id qBK32pWt010231 for ; Thu, 20 Dec 2012 14:02:52 +1100 Message-ID: <50D27FB8.4030000@linux.vnet.ibm.com> Date: Thu, 20 Dec 2012 11:02:16 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1355912230-25132-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1355912230-25132-3-git-send-email-xiawenc@linux.vnet.ibm.com> <20121219160743.34cdfd73@doriath.home> In-Reply-To: <20121219160743.34cdfd73@doriath.home> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/3] HMP: pass in parameter for info sub command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@gmail.com, qemu-devel@nongnu.org, armbru@redhat.com, pbonzini@redhat.com > On Wed, 19 Dec 2012 18:17:09 +0800 > Wenchao Xia wrote: > >> This patch enable sub info command handler getting meaningful >> parameter. >> >> Signed-off-by: Wenchao Xia >> --- >> 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. >> + >> /* >> * 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 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; > > -- Best Regards Wenchao Xia