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

[Markus Armbruster <armbru@redhat.com>]

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;