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

[Bandan Das]

Markus Armbruster <address@hidden> writes:

> 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.

Ok sounds good.

>>   * 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?

No, it won't :) Sorry, will fix.

>>  #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.

Yes, good idea. Will do.

>
>> + */
>> +
>> +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.

Ok sure!

>>                  }
....
>> -out:
>> +    /* Drop reference taken in monitor_parse_arguments */
>>      QDECREF(qdict);
>>  }
>
> Very close to earning my R-by.
Yay!!

Thanks again for the meticulous review,
Bandan