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

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

[Bandan Das]

v4:
Better name for cmdline index pointer [1/4]
Change comment for monitor_parse_command as suggested in review [1/4]
Fix potential compilation failure in debug print [1/4]
New - Fix failure path for argument type "S" [3/4]
New - Remove debug prints [4/4]

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 (4):
  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: Fix failure path for "S" argument
  monitor: remove debug prints

 monitor.c | 121 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 61 insertions(+), 60 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v4 1/4] 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 | 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
 
     /* 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
+ */
+
+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 v4 1/4] 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 | 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.

[...]

[Qemu-devel] [PATCH v4 2/4] 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 a89cbbb..6777cbe 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

Re: [Qemu-devel] [PATCH v4 2/4] 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.

[Markus Armbruster]

Patch is fine, but the commit message shouldn't be just one long line.
Suggest:

    monitor: Point to "help" command on syntax error

    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.

    Signed-off-by: Bandan Das <bsd@redhat.com>
    Reviewed-by: Markus Armbruster <armbru@redhat.com>

Missed that in my original review, sorry.

[Qemu-devel] [PATCH v4 3/4] monitor: Fix failure path for "S" argument type

[Bandan Das]

The "S" argument is only used with ? and we don't
reach it. Fix it nevertheless.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/monitor.c b/monitor.c
index 6777cbe..cbc3cc6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4069,7 +4069,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
                 if (len <= 0) {
                     monitor_printf(mon, "%s: string expected\n",
                                    cmd->name);
-                    break;
+                    goto fail;
                 }
                 qdict_put(qdict, key, qstring_from_str(p));
                 p += len;
-- 
2.1.0

Re: [Qemu-devel] [PATCH v4 3/4] monitor: Fix failure path for "S" argument type

[Markus Armbruster]

Bandan Das <bsd@redhat.com> writes:

> The "S" argument is only used with ? and we don't
> reach it. Fix it nevertheless.

"and we don't reach it" is confusing.  Suggest

    Since the "S" argument type is only used with the "?" flag, the bug
    can't bite.

With the commit message touched up:

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v4 4/4] monitor: remove debug prints

[Bandan Das]

The preferred solution is to use tracepoints and there
is good chance of bitrot with the debug prints not being
enabled at compile time. Remove them.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Bandan Das <bsd@redhat.com>
---
 monitor.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/monitor.c b/monitor.c
index cbc3cc6..44bd787 100644
--- a/monitor.c
+++ b/monitor.c
@@ -81,9 +81,6 @@
 #endif
 #include "hw/lm32/lm32_pic.h"
 
-//#define DEBUG
-//#define DEBUG_COMPLETION
-
 /*
  * Supported types:
  *
@@ -3700,9 +3697,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
     const mon_cmd_t *cmd;
     char cmdname[256];
 
-#ifdef DEBUG
-    monitor_printf(mon, "command='%s', start='%c'\n", cmdline, **cmdp);
-#endif
 
     /* extract the command name */
     p = get_command_name(*cmdp, cmdname, sizeof(cmdname));
@@ -4207,10 +4201,7 @@ static void file_completion(Monitor *mon, const char *input)
         path[input_path_len] = '\0';
         pstrcpy(file_prefix, sizeof(file_prefix), p + 1);
     }
-#ifdef DEBUG_COMPLETION
-    monitor_printf(mon, "input='%s' path='%s' prefix='%s'\n",
-                   input, path, file_prefix);
-#endif
+
     ffs = opendir(path);
     if (!ffs)
         return;
@@ -4788,14 +4779,6 @@ static void monitor_find_completion(void *opaque,
     if (parse_cmdline(cmdline, &nb_args, args) < 0) {
         return;
     }
-#ifdef DEBUG_COMPLETION
-    {
-        int i;
-        for (i = 0; i < nb_args; i++) {
-            monitor_printf(mon, "arg%d = '%s'\n", i, args[i]);
-        }
-    }
-#endif
 
     /* if the line ends with a space, it means we want to complete the
        next arg */
-- 
2.1.0

Re: [Qemu-devel] [PATCH v4 4/4] monitor: remove debug prints

[Markus Armbruster]

Bandan Das <bsd@redhat.com> writes:

> The preferred solution is to use tracepoints and there
> is good chance of bitrot with the debug prints not being
> enabled at compile time. Remove them.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Bandan Das <bsd@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>