Re: [Qemu-devel] [PATCH 2/3] HMP: pass in parameter for info sub command

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

 > 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;
>
>


-- 
Best Regards

Wenchao Xia