Re: [Qemu-devel] [PATCH v3 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. Also, pass a pointer to the cmdline to track current
> parser location.
>
> Suggested-by: Markus Armbruster <address@hidden>
> Signed-off-by: Bandan Das <address@hidden>
> ---
>  monitor.c | 90 
> +++++++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 53 insertions(+), 37 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index b2561e1..521258d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3683,11 +3683,10 @@ 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 a sub-command table exists, and if @cmdline contains an additional 
> string
> + * Parse cmdline anchored at @endp according to command table @table.
> + * If @*endp is blank, return NULL.
> + * If @*endp is invalid, report to @mon and return NULL.
> + * If a sub-command table exists, and if @*endp contains an additional string

@endp is a rather odd name now, don't you think?  What about naming it
@cmdp?

Preexisting: comment suggests we have at most two levels of command
tables.  Code actually supports arbitrary nesting.

What about:

/*
 * Parse command name from @cmdp according to command table @table.
 * If blank, return NULL.
 * Else, if no valid command can be found, report to @mon, and return
 * NULL.
 * Else, change @cmdp to point right behind the name, and return its
 * command table entry.
 * Do not assume the return value points into @table!  It doesn't when
 * the command is found in a sub-command table.
 */

No longer explains how sub-commands work.  Proper, because it's none of
the callers business, and this is a function contract.  The explanation
belongs into mon_cmd_t.  Which already has one that's good enough.

If the function's code dealing with sub-commands was unobvious, we'd
want to explain it some there.  But it isn't.  We explain a bit there
anyway, which is fine with me.

>   * 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
>   * return the command found in @table.
> @@ -3695,31 +3694,26 @@ static const mon_cmd_t *qmp_find_cmd(const char 
> *cmdname)
>   * when the command is a sub-command.
>   */
>  static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> -                                              const char *cmdline,
> -                                              int start,
> -                                              mon_cmd_t *table,
> -                                              QDict *qdict)
> +                                              const char **endp,
> +                                              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);

Would this compile if we defined DEBUG?

>  #endif
>  
>      /* extract the command name */
> -    p = get_command_name(cmdline + start, cmdname, sizeof(cmdname));
> +    p = get_command_name(*endp, cmdname, sizeof(cmdname));
>      if (!p)
>          return NULL;
>  
>      cmd = search_dispatch_table(table, cmdname);
>      if (!cmd) {
>          monitor_printf(mon, "unknown command: '%.*s'\n",
> -                       (int)(p - cmdline), cmdline);
> +                       (int)(p - *endp), *endp);
>          return NULL;
>      }
>  
> @@ -3727,16 +3721,33 @@ static const mon_cmd_t *monitor_parse_command(Monitor 
> *mon,
>      while (qemu_isspace(*p)) {
>          p++;
>      }
> +
> +    *endp = p;
>      /* 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, endp, cmd->sub_table);
>      }
>  
> +    return cmd;
> +}
> +
> +/*
> + * Parse arguments for @cmd anchored at @endp
> + * If it can't be parsed, report to @mon, and return NULL.
> + * Else, insert command arguments into @qdict, and return it.

@qdict suggests there's a parameter with that name.  Suggest "a QDict",
or "a new QDict".

Adding "Caller is responsible for freeing it" wouldn't hurt.

> + */
> +
> +static QDict *monitor_parse_arguments(Monitor *mon,
> +                                      const char **endp,
> +                                      const mon_cmd_t *cmd)
> +{
> +    const char *typestr;
> +    char *key;
> +    int c;
> +    const char *p = *endp;
> +    char buf[1024];
> +    QDict *qdict = qdict_new();
> +
>      /* parse the parameters */
>      typestr = cmd->args_type;
>      for(;;) {
> @@ -3766,14 +3777,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 +3926,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 +4034,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 +4068,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor 
> *mon,
                   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);
> +                                   cmd->name);
>                      break;

Preexisting: this error fails to fail the function.

Bug currently can't bite, because argument type 'S' is only used
together with '?', and then we don't reach this error.

Mind to throw in the obvious fix?  Separate patch, of course.

>                  }
>                  qdict_put(qdict, key, qstring_from_str(p));
> @@ -4066,7 +4077,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 +4088,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;
>  }
> @@ -4115,11 +4127,15 @@ static void handle_user_command(Monitor *mon, const 
> char *cmdline)
>      QDict *qdict;
>      const mon_cmd_t *cmd;
>  
> -    qdict = qdict_new();
> +    cmd = monitor_parse_command(mon, &cmdline, 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, cmd);
> +    if (!qdict) {
> +        return;
> +    }
>  
>      if (handler_is_async(cmd)) {
>          user_async_cmd_handler(mon, cmd, qdict);
> @@ -4137,7 +4153,7 @@ static void handle_user_command(Monitor *mon, const 
> char *cmdline)
>          cmd->mhandler.cmd(mon, qdict);
>      }
>  
> -out:
> +    /* Drop reference taken in monitor_parse_arguments */
>      QDECREF(qdict);
>  }

Very close to earning my R-by.