| Message ID | 1355912230-25132-3-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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. > + > /* > * 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(). 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); > + 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? > + 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;
> 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. >> + >> /* >> * 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; > >
Luiz Capitulino <lcapitulino@redhat.com> writes: > 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 Missing: documentation for the new arg type S in the comment that starts with "Supported types". >> @@ -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. >> + >> /* >> * 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! >> + 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; } >> >> 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); } > 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". >> + 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. 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. >> + 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. >> @@ -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;
On 12/21/2012 07:49 AM, Markus Armbruster wrote: >>> +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. If you do move the function, then split this into two patches - one for JUST the code motion, and another that adds the 'table' argument to the function.
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; > > > > > >
δΊ 2012-12-21 22:49, Markus Armbruster ει: > Luiz Capitulino <lcapitulino@redhat.com> writes: > >> 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 > > 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; >
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); + /* * 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); } 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; + } + 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;
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(-)