Patchwork [2/3] HMP: pass in parameter for info sub command

login
register
mail settings
Submitter Wayne Xia
Date Dec. 19, 2012, 10:17 a.m.
Message ID <1355912230-25132-3-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/207305/
State New
Headers show

Comments

Wayne Xia - Dec. 19, 2012, 10:17 a.m.
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(-)
Luiz Capitulino - Dec. 19, 2012, 6:07 p.m.
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;
Wayne Xia - Dec. 20, 2012, 3:02 a.m.
> 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;
>
>
Markus Armbruster - Dec. 21, 2012, 2:49 p.m.
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;
Eric Blake - Dec. 21, 2012, 6:17 p.m.
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.
Luiz Capitulino - Dec. 22, 2012, 9:36 p.m.
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;
> >
> >
> 
>
Wayne Xia - Dec. 25, 2012, 4:29 a.m.
于 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;
>

Patch

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;