Re: [Qemu-devel] [PATCH v4 1/4] 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 | 100 
> ++++++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 58 insertions(+), 42 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index b2561e1..a89cbbb 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3683,43 +3683,36 @@ 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
> - * 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.
> - * Do not assume the returned command points into @table!  It doesn't
> - * when the command is a sub-command.
> + * 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.
>   */
>  static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> -                                              const char *cmdline,
> -                                              int start,
> -                                              mon_cmd_t *table,
> -                                              QDict *qdict)
> +                                              const char **cmdp,
> +                                              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='%c'\n", cmdline, **cmdp);
>  #endif

With DEBUG defined:

  CC    x86_64-softmmu/monitor.o
/work/armbru/qemu/monitor.c: In function ‘monitor_parse_command’:
/work/armbru/qemu/monitor.c:3704:55: error: ‘cmdline’ undeclared (first use in 
this function)
     monitor_printf(mon, "command='%s', start='%c'\n", cmdline, **cmdp);

Recommend to fix this by reordering patches: 4 3 1 2.

>  
>      /* extract the command name */
> -    p = get_command_name(cmdline + start, cmdname, sizeof(cmdname));
> +    p = get_command_name(*cmdp, 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 - *cmdp), *cmdp);
>          return NULL;
>      }
>  
> @@ -3727,16 +3720,34 @@ static const mon_cmd_t *monitor_parse_command(Monitor 
> *mon,
>      while (qemu_isspace(*p)) {
>          p++;
>      }
> +
> +    *cmdp = 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, cmdp, 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 a QDict, and return it.
> + * Note: On success, caller has to free the QDict structure
> + */

Since you'll have to respin anyway: humor me, and end the sentence with
a period.

[...]