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

[Markus Armbruster]

Wenchao Xia <address@hidden> writes:

>   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 |   31 +++++++++++++++++++++++--------
>  1 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index c7b3014..d49a362 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,22 @@ 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);
>  }
>  
> +/*
> + * 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 remaining string, and if not
> + * found it return NULL.
> + */

Thanks for going the extra mile and document the function.  What about:

   /*
    * Parse @cmdline according to command table @table.
    * If @cmdline is blank, return NULL.
    * If it can't be parsed, report to @mon, and return NULL.
    * Else, insert command arguments into @qdict, and return the command.
    * Do not assume the returned command points into @table!  It doesn't
    * when the command is a sub-command.
    */

>  static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                                                const char *cmdline,
> +                                              mon_cmd_t *table,
>                                                QDict *qdict)
>  {
> -    const char *p, *typestr;
> +    const char *p, *p1, *typestr;
>      int c;
>      const mon_cmd_t *cmd;
>      char cmdname[256];
> @@ -3564,12 +3566,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 monitor_parse_command(mon, p, cmd->sub_table, qdict);
> +    }
> +
>      /* parse the parameters */
>      typestr = cmd->args_type;
>      for(;;) {

The check whether non-space characters follow is awkward.  We need it
only because we want to handle "@cmdline is blank" differently than "it
can't be parsed", but monitor_parse_command() returns NULL for both
cases.  If we care, we can try to do better in a follow-up patch.

We'll get sub-optimal error messages when the infrastructure is put to
use in patch 4/4, e.g.:

    (qemu) info apple pie
    unknown command: 'apple'

That's because cmdname is just "apple" at the place where the error is
diagnosed.

Several other messages also print cmdname, and thus are similarly
affected.  Many of them could probably just omit printing it, but that's
a separate issue.

A possible way to improve this: additional parameter int start, parse
starts at cmdline + start, and instead of printing cmdname, print
cmdline up to end of cmdname.  Something like

        monitor_printf(mon, "unknown command: '%.*s'\n",
                       (int)(p - cmdline), cmdline);

> @@ -3925,7 +3940,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;