δΊ 2012-12-21 22:49, Markus Armbruster ει:
> 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".
>
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
