[Qemu-devel] [PATCH v3 0/2] monitor: suggest running "help" for command errors

[Qemu-devel] [PATCH v3 0/2] monitor: suggest running "help" for command errors

[Bandan Das]

Hi Markus,
It seems we can directly use cmdline to track current location
when parsing. It's not being reused anywhere else and my unit
tests didn't show any problems. Am I missing something ?

Once everything looks ok, I can rebase this on top of your
patches. Thanks for taking the time to review this!

Bandan

v3:
Track the current location directly in the command line [1/2]
Fix potential qdict leak [1/2]
Document char **endp [1/2]
Rebase on top of changes and add reviewed-by [2/2]

v2:
Split up the command name and arguments parsing into
separate functions. [1/2]
Skip checking for failures with commands that use the .cmd_new
interface or the async interface since they are scheduled
for removal [2/2]

Bandan Das (2):
  monitor: cleanup parsing of cmd name and cmd arguments
  When a command fails due to incorrect syntax or input,     suggest
    using the "help" command to get more information     about the
    command. This is only applicable for HMP.

 monitor.c | 92 ++++++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 55 insertions(+), 37 deletions(-)

-- 
2.1.0

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

[Bandan Das]

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 <armbru@redhat.com>
Signed-off-by: Bandan Das <bsd@redhat.com>
---
 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
  * 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);
 #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.
+ */
+
+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,
                 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 +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);
 }
 
-- 
2.1.0

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

[Markus Armbruster]

Bandan Das <bsd@redhat.com> 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 <armbru@redhat.com>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  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.

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

[Bandan Das]

Markus Armbruster <armbru@redhat.com> writes:

> Bandan Das <bsd@redhat.com> 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 <armbru@redhat.com>
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On 05/28/2015 12:48 PM, Bandan Das wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Bandan Das <bsd@redhat.com> 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.

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

That's why I like solutions that can't bitrot; something like this
framework (needs a bit more to actually compile, but you get the picture):

#ifdef DEBUG
# define DEBUG_MONITOR 1
#else
# define DEBUG_MONITOR 0
#endif
#define DEBUG_MONITOR_PRINTF(stuff...) do { \
    if (DEBUG_MONITOR) { \
         monitor_printf(stuff...); \
    } \
} while (0)

then you can avoid the #ifdef in the function body, and just do
DEBUG_MONITOR_PRINTF(mon, "command='%s'....) and the compiler will
always check for correct format vs. arguments, even when debugging is off.

Of course, adding such a framework in this file would be a separate
patch, and does not have to be done as a prerequisite of this series.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Bandan Das]

Eric Blake <eblake@redhat.com> writes:

> On 05/28/2015 12:48 PM, Bandan Das wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>>> Bandan Das <bsd@redhat.com> 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.
>
>>>>  
>>>>  #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.
>
> That's why I like solutions that can't bitrot; something like this
> framework (needs a bit more to actually compile, but you get the picture):
>
> #ifdef DEBUG
> # define DEBUG_MONITOR 1
> #else
> # define DEBUG_MONITOR 0
> #endif
> #define DEBUG_MONITOR_PRINTF(stuff...) do { \
>     if (DEBUG_MONITOR) { \
>          monitor_printf(stuff...); \
>     } \
> } while (0)
>
> then you can avoid the #ifdef in the function body, and just do
> DEBUG_MONITOR_PRINTF(mon, "command='%s'....) and the compiler will
> always check for correct format vs. arguments, even when debugging is off.
>
> Of course, adding such a framework in this file would be a separate
> patch, and does not have to be done as a prerequisite of this series.

Thanks, Eric. Ok, I will post patch(es) separately.

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

[Markus Armbruster]

Bandan Das <bsd@redhat.com> writes:

> Eric Blake <eblake@redhat.com> writes:
>
>> On 05/28/2015 12:48 PM, Bandan Das wrote:
>>> Markus Armbruster <armbru@redhat.com> writes:
>>> 
>>>> Bandan Das <bsd@redhat.com> 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.
>>
>>>>>  
>>>>>  #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.
>>
>> That's why I like solutions that can't bitrot; something like this
>> framework (needs a bit more to actually compile, but you get the picture):
>>
>> #ifdef DEBUG
>> # define DEBUG_MONITOR 1
>> #else
>> # define DEBUG_MONITOR 0
>> #endif
>> #define DEBUG_MONITOR_PRINTF(stuff...) do { \
>>     if (DEBUG_MONITOR) { \
>>          monitor_printf(stuff...); \
>>     } \
>> } while (0)
>>
>> then you can avoid the #ifdef in the function body, and just do
>> DEBUG_MONITOR_PRINTF(mon, "command='%s'....) and the compiler will
>> always check for correct format vs. arguments, even when debugging is off.
>>
>> Of course, adding such a framework in this file would be a separate
>> patch, and does not have to be done as a prerequisite of this series.
>
> Thanks, Eric. Ok, I will post patch(es) separately.

The preferred solution in QEMU is tracepoints.

In this case, I wouldn't mind ditching the debugging prints instead.
All three of them.

[Qemu-devel] [PATCH v3 2/2] When a command fails due to incorrect syntax or input, suggest using the "help" command to get more information about the command. This is only applicable for HMP.

[Bandan Das]

Before:
(qemu) drive_add usb_flash_drive
drive_add: string expected
After:
(qemu) drive_add usb_flash_drive
drive_add: string expected
Try "help drive_add" for more information

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Bandan Das <bsd@redhat.com>
---
 monitor.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/monitor.c b/monitor.c
index 521258d..2ed9fb7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4134,6 +4134,8 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
 
     qdict = monitor_parse_arguments(mon, &cmdline, cmd);
     if (!qdict) {
+        monitor_printf(mon, "Try \"help %s\" for more information\n",
+                       cmd->name);
         return;
     }
 
-- 
2.1.0