From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45975) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TnMAk-0005Yz-Aa for qemu-devel@nongnu.org; Mon, 24 Dec 2012 23:31:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TnMA9-0005Oj-AN for qemu-devel@nongnu.org; Mon, 24 Dec 2012 23:31:22 -0500 Received: from e23smtp04.au.ibm.com ([202.81.31.146]:52304) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TnMA7-0005F3-WD for qemu-devel@nongnu.org; Mon, 24 Dec 2012 23:30:44 -0500 Received: from /spool/local by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 25 Dec 2012 14:23:50 +1000 Received: from d23relay04.au.ibm.com (d23relay04.au.ibm.com [9.190.234.120]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id 07B7A2BB004A for ; Tue, 25 Dec 2012 15:30:33 +1100 (EST) Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id qBP4JH3u262608 for ; Tue, 25 Dec 2012 15:19:21 +1100 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id qBP4US6d004187 for ; Tue, 25 Dec 2012 15:30:28 +1100 Message-ID: <50D92BA0.5040408@linux.vnet.ibm.com> Date: Tue, 25 Dec 2012 12:29:20 +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> <87vcbv1mvu.fsf@blackfin.pond.sub.org> In-Reply-To: <87vcbv1mvu.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=GB2312 Content-Transfer-Encoding: 8bit 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: Markus Armbruster Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@gmail.com, qemu-devel@nongnu.org, Luiz Capitulino , pbonzini@redhat.com ÓÚ 2012-12-21 22:49, Markus Armbruster дµÀ: > Luiz Capitulino writes: > >> 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 > > Missing: documentation for the new arg type S in the comment that starts > with "Supported types". > OK, will add it. >>> @@ -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. > > Move will make review harder. I don't mind forward declarations. > I will move the function in a separate patch. >>> + >>> /* >>> * 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 */ > > I'm afraid I don't understand your comment. > > Oh, you mean you don't want to call help_cmd() here! > Yes, not to show the help contents, same as not call help_cmd(). >>> + return; >>> } >>> >>> - cmd->mhandler.info(mon, NULL); >>> + cmd->mhandler.info(mon, qdict_info); >>> + QDECREF(qdict_info); >>> return; >>> >>> help: >>> help_cmd(mon, "info"); >>> + return; >>> } > > The function now looks like this: > > 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; > } > > qdict_info = qdict_new(); > > 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, qdict_info); > QDECREF(qdict_info); > return; > > help: > help_cmd(mon, "info"); > return; > } > > Control flow isn't nice. What about: > > 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) { > help_cmd(mon, "info"); > return; > } > > qdict_info = qdict_new(); > > cmd = monitor_parse_command(mon, item, info_cmds, qdict_info); > if (!cmd) { > goto out; > } > > cmd->mhandler.info(mon, qdict_info); > out: > QDECREF(qdict_info); > return; > } > OK, I'll following this way. >>> >>> 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(). > > Yes, because the resulting function is silly :) > > static const mon_cmd_t *monitor_find_command(const char *cmdname, > const mon_cmd_t *table) > { > return search_dispatch_table(table, cmdname); > } > I'll remove simple encapsulate function monitor_find_command(), which's naming make me confused. >> 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(). >> >>> } >>> >>> 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); > > A "string" in this context is an argument for arg type 's', i.e. a > sequence of characters delimited by unescaped '"', or a sequence of > non-whitespace characters not starting with '"'. That's not what we > expect here. We expect arbitrary arguments. > > Suggest "arguments expected". > OK. >>> + 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? > > Let me try :) > > The new arg type 'S' consumes the rest of the line unparsed, and puts it > into the argument qdict. Has to come last, obviously. > > do_info() extracts it, then parses it like a full command. The info > command effectively adds like a prefix that switches command parsing to > an alternate table of commands. Works, quite flexible, but pretty > arcane. > > Try #1: > > Implement the command prefix concept the direct way. Mark the command > as prefix. Instead of a handler, it gets a pointer to the alternate > table of commands. When monitor_parse_command() recognizes a prefix > command, it drops the first word, switches to the alternate table, and > starts over. > It seems "info" command need to be marked and searched in monitor_parse_command(), > Try #2: > > We already have commands that take key=value,... arguments (arg type > 'O'): device_add and netdev_add. Perhaps info could use args_type > "item:s?,opts:O". First argument is unchanged. The new second argument > accepts key=value,... options. 'O' argument is always optional. One > key can be declared optional, so that value (no '=') is shorthand for > that key=value. Personally I feel approach #1 is better, which let the command parse layer knows there would be another sub command layer, and enable embbed sub commands with deeper layers transparently. #2 and my previous patch is a hack but each layer don't know sub command exist. For #1 I guess additional tag about "sub command' need to be added, I guess that would be typedef struct mon_cmd_t { const char *name; const char *args_type; const char *params; const char *help; void (*user_print)(Monitor *mon, const QObject *data); union { void (*info)(Monitor *mon); void (*cmd)(Monitor *mon, const QDict *qdict); int (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data); int (*cmd_async)(Monitor *mon, const QDict *params, MonitorCompletion *cb, void *opaque); } mhandler; int flags; struct mon_cmd_t *sub_table; } mon_cmd_t; *sub_table flag if it have a 2nd level of command table. > >>> + 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; > > You move the special case hack for info argument completion to case 'S' > (next hunk), but leave behind the /* XXX: more generic ? */ that marks > it as hack! Please move it along with the hack. > OK. >>> @@ -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