Re: [Qemu-devel] [PATCH v2 1/2] monitor: cleanup parsing of cmd name and cmd arguments

[Markus Armbruster]

Bandan Das <address@hidden> writes:

> There's too much going on in monitor_parse_command().
> Split up the arguments parsing bits into a separate function
> monitor_parse_arguments(). Let the original function check for
> command validity and sub-commands if any and return data (*cmd)
> that the newly introduced function can process and return a
> QDict.
>
> Suggested-by: Markus Armbruster <address@hidden>
> Signed-off-by: Bandan Das <address@hidden>
> ---
>  monitor.c | 84 
> ++++++++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 51 insertions(+), 33 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index b2561e1..f0c1a8c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3685,8 +3685,7 @@ static const mon_cmd_t *qmp_find_cmd(const char 
> *cmdname)
>  /*
>   * 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.
> + * If @cmdline is invalid, report to @mon and return NULL.
>   * If a sub-command table exists, and if @cmdline contains an additional 
> string
>   * for a sub-command, this function will try to search the sub-command table.
>   * If no additional string for a sub-command is present, this function will

Function comment fails to document @start.  Since you're touching it,
you get to fix that :)

I figure this would become a bit simpler if we replaced in/out parameter
int start by an out parameter const char **endp.

> @@ -3696,23 +3695,19 @@ static const mon_cmd_t *qmp_find_cmd(const char 
> *cmdname)
>   */
>  static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                                                const char *cmdline,
> -                                              int start,
> -                                              mon_cmd_t *table,
> -                                              QDict *qdict)
> +                                              int *start,
> +                                              mon_cmd_t *table)
>  {
> -    const char *p, *typestr;
> -    int c;
> +    const char *p;
>      const mon_cmd_t *cmd;
>      char cmdname[256];
> -    char buf[1024];
> -    char *key;
>  
>  #ifdef DEBUG
> -    monitor_printf(mon, "command='%s', start='%d'\n", cmdline, start);
> +    monitor_printf(mon, "command='%s', start='%d'\n", cmdline, *start);
>  #endif
>  
>      /* extract the command name */
> -    p = get_command_name(cmdline + start, cmdname, sizeof(cmdname));
> +    p = get_command_name(cmdline + *start, cmdname, sizeof(cmdname));
>      if (!p)
>          return NULL;
>  
> @@ -3727,16 +3722,35 @@ static const mon_cmd_t *monitor_parse_command(Monitor 
> *mon,
>      while (qemu_isspace(*p)) {
>          p++;
>      }
> +
> +    *start = p - cmdline;
>      /* search sub command */
> -    if (cmd->sub_table != NULL) {
> -        /* check if user set additional command */
> -        if (*p == '\0') {
> -            return cmd;
> -        }
> -        return monitor_parse_command(mon, cmdline, p - cmdline,
> -                                     cmd->sub_table, qdict);
> +    if (cmd->sub_table != NULL && *p != '\0') {
> +        return monitor_parse_command(mon, cmdline, start,
> +                                     cmd->sub_table);
>      }
>  
> +    return cmd;
> +}
> +
> +/*
> + * Parse arguments for @cmd
> + * If it can't be parsed, report to @mon, and return NULL.
> + * Else, insert command arguments into @qdict, and return it.
> + */

Please document @cmdline and @start.

> +
> +static QDict *monitor_parse_arguments(Monitor *mon,
> +                                      const char *cmdline,
> +                                      int *start,
> +                                      const mon_cmd_t *cmd)
> +{
> +    const char *typestr;
> +    char *key;
> +    int c;
> +    const char *p = cmdline + *start;
> +    char buf[1024];
> +    QDict *qdict = qdict_new();
> +
>      /* parse the parameters */
>      typestr = cmd->args_type;
>      for(;;) {
> @@ -3766,14 +3780,14 @@ static const mon_cmd_t *monitor_parse_command(Monitor 
> *mon,
>                      switch(c) {
>                      case 'F':
>                          monitor_printf(mon, "%s: filename expected\n",
> -                                       cmdname);
> +                                       cmd->name);
>                          break;
>                      case 'B':
>                          monitor_printf(mon, "%s: block device name 
> expected\n",
> -                                       cmdname);
> +                                       cmd->name);
>                          break;
>                      default:
> -                        monitor_printf(mon, "%s: string expected\n", 
> cmdname);
> +                        monitor_printf(mon, "%s: string expected\n", 
> cmd->name);
>                          break;
>                      }
>                      goto fail;
> @@ -3915,7 +3929,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor 
> *mon,
>                      goto fail;
>                  /* Check if 'i' is greater than 32-bit */
>                  if ((c == 'i') && ((val >> 32) & 0xffffffff)) {
> -                    monitor_printf(mon, "\'%s\' has failed: ", cmdname);
> +                    monitor_printf(mon, "\'%s\' has failed: ", cmd->name);
>                      monitor_printf(mon, "integer is for 32-bit values\n");
>                      goto fail;
>                  } else if (c == 'M') {
> @@ -4023,7 +4037,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor 
> *mon,
>                          if(!is_valid_option(p, typestr)) {
>                    
>                              monitor_printf(mon, "%s: unsupported option 
> -%c\n",
> -                                           cmdname, *p);
> +                                           cmd->name, *p);
>                              goto fail;
>                          } else {
>                              skip_key = 1;
> @@ -4057,7 +4071,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor 
> *mon,
>                  len = strlen(p);
>                  if (len <= 0) {
>                      monitor_printf(mon, "%s: string expected\n",
> -                                   cmdname);
> +                                   cmd->name);
>                      break;
>                  }
>                  qdict_put(qdict, key, qstring_from_str(p));
> @@ -4066,7 +4080,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor 
> *mon,
>              break;
>          default:
>          bad_type:
> -            monitor_printf(mon, "%s: unknown type '%c'\n", cmdname, c);
> +            monitor_printf(mon, "%s: unknown type '%c'\n", cmd->name, c);
>              goto fail;
>          }
>          g_free(key);
> @@ -4077,13 +4091,14 @@ static const mon_cmd_t *monitor_parse_command(Monitor 
> *mon,
>          p++;
>      if (*p != '\0') {
>          monitor_printf(mon, "%s: extraneous characters at the end of line\n",
> -                       cmdname);
> +                       cmd->name);
>          goto fail;
>      }
>  
> -    return cmd;
> +    return qdict;
>  
>  fail:
> +    QDECREF(qdict);
>      g_free(key);
>      return NULL;
>  }
> @@ -4114,12 +4129,17 @@ static void handle_user_command(Monitor *mon, const 
> char *cmdline)

This hunk conflicts with my "[PATCH v2 00/20] monitor: Wean core off
QError, and other cleanups".  Shouldn't be hard to resolve.

>  {
>      QDict *qdict;
>      const mon_cmd_t *cmd;
> +    int start = 0;
>  
> -    qdict = qdict_new();
> +    cmd = monitor_parse_command(mon, cmdline, &start, mon->cmd_table);
> +    if (!cmd) {
> +        return;
> +    }
>  
> -    cmd = monitor_parse_command(mon, cmdline, 0, mon->cmd_table, qdict);
> -    if (!cmd)
> -        goto out;
> +    qdict = monitor_parse_arguments(mon, cmdline, &start, cmd);

Since we're not interested in text following arguments (if that's even
possible), we could simply do

    qdict = monitor_parse_arguments(mon, cmdline + start, cmd);

If we replace in/out start by out endp as mentioned above, things become
simpler still:

    cmd = monitor_parse_command(mon, cmdline, &endp, mon->cmd_table);
    if (!cmd) {
        return;
    }
    qdict = monitor_parse_arguments(mon, endp, cmd);

> +    if (!qdict) {
> +        return;
> +    }
>  
>      if (handler_is_async(cmd)) {
>          user_async_cmd_handler(mon, cmd, qdict);
> @@ -4137,8 +4157,6 @@ static void handle_user_command(Monitor *mon, const 
> char *cmdline)
>          cmd->mhandler.cmd(mon, qdict);
>      }
>  
> -out:
> -    QDECREF(qdict);

Doesn't this leak qdict?

>  }
>  
>  static void cmd_completion(Monitor *mon, const char *name, const char *list)

Overall, I rather like how this turned out.