Re: [Qemu-devel] [PATCH V3 2/4] HMP: add infrastructure for sub command

[Luiz Capitulino]

On Thu, 27 Dec 2012 17:40:25 +0800
Wenchao Xia <address@hidden> wrote:

>   This patch make parsing of hmp command aware of that it may
> have sub command. Also discard simple encapsulation function
> monitor_find_command() and qmp_find_cmd().
> 
> Signed-off-by: Wenchao Xia <address@hidden>
> ---
>  monitor.c |   44 +++++++++++++++++++++++++++-----------------
>  1 files changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index c7b3014..e4d4c59 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -130,6 +130,7 @@ typedef struct mon_cmd_t {
>                            MonitorCompletion *cb, void *opaque);
>      } mhandler;
>      int flags;
> +    struct mon_cmd_t *sub_table;
>  } mon_cmd_t;
>  
>  /* file descriptors passed via SCM_RIGHTS */
> @@ -3534,21 +3535,17 @@ 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)
> -{
> -    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);
> -}

Touching qmp_find_cmd() is unrelated to this series.

> -
> -static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> -                                              const char *cmdline,
> -                                              QDict *qdict)
> +/*
> + * Try find the target mon_cmd_t instance, if it have sub_table and have 
> string
> + * for it, this function will try search it with remain string, and if not
> + * found it return NULL.
> + */
> +static const mon_cmd_t *hmp_parse_command(Monitor *mon,
> +                                          const char *cmdline,
> +                                          mon_cmd_t *table,
> +                                          QDict *qdict)

Renaming is also not necessary, but if you do it please do in a different
patch.

>  {
> -    const char *p, *typestr;
> +    const char *p, *p1, *typestr;
>      int c;
>      const mon_cmd_t *cmd;
>      char cmdname[256];
> @@ -3564,12 +3561,25 @@ static const mon_cmd_t *monitor_parse_command(Monitor 
> *mon,
>      if (!p)
>          return NULL;
>  
> -    cmd = monitor_find_command(cmdname);
> +    cmd = search_dispatch_table(table, cmdname);
>      if (!cmd) {
>          monitor_printf(mon, "unknown command: '%s'\n", cmdname);
>          return NULL;
>      }
>  
> +    /* search sub command */
> +    if (cmd->sub_table != NULL) {
> +        p1 = p;
> +        /* check if user set additional command */
> +        while (qemu_isspace(*p1)) {
> +            p1++;
> +        }
> +        if (*p1 == '\0') {
> +            return cmd;
> +        }
> +        return hmp_parse_command(mon, p, cmd->sub_table, qdict);

This works, and seems less hack then the previous attempt.

Except for a small detail. This:

  (qemu) info bla

Currently returns a list of info commands, but with this series it returns
an error.

I'd like to get a reviewed-by from Markus before applying this as, besides
this being his idea, I honestly have a very hard time knowing how to move old
hmp hackery forward.

> +    }
> +
>      /* parse the parameters */
>      typestr = cmd->args_type;
>      for(;;) {
> @@ -3925,7 +3935,7 @@ static void handle_user_command(Monitor *mon, const 
> char *cmdline)
>  
>      qdict = qdict_new();
>  
> -    cmd = monitor_parse_command(mon, cmdline, qdict);
> +    cmd = hmp_parse_command(mon, cmdline, mon_cmds, qdict);
>      if (!cmd)
>          goto out;
>  
> @@ -4483,7 +4493,7 @@ static void handle_qmp_command(JSONMessageParser 
> *parser, QList *tokens)
>          goto err_out;
>      }
>  
> -    cmd = qmp_find_cmd(cmd_name);
> +    cmd = search_dispatch_table(qmp_cmds, cmd_name);
>      if (!cmd) {
>          qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
>          goto err_out;