Luiz Capitulino <lcapitul...@redhat.com> writes:
> 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
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;
