monitor: suggest running "help" for command errors

PATCH Superseded Bandan Das 2 participants 9 messages Jun 3, 2015

Share & tools

Bandan Das

author

2015-06-03 03:29 UTC #0 lore raw

v4 cover monitor: suggest running "help" for command errors

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(-)

Signature · 1 line

2.1.0

Bandan Das

author

2015-06-03 03:29 UTC #1 lore raw

PATCH v4 1/4 monitor: cleanup parsing of cmd name and cmd arguments

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.

Signed-off-by Bandan Das <[email protected]>

---
 monitor.c | 100 ++++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 58 insertions(+), 42 deletions(-)

monitor.c +58 ~~-42~~

index b2561e1..a89cbbb 100644
@@ -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);
 }

Signature · 1 line

2.1.0

Markus Armbruster

2015-06-03 11:26 UTC #2 lore raw

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

Quoted 51 lines · Bandan Das

Signed-off-by Bandan Das <[email protected]>

---
 monitor.c | 100 ++++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 58 insertions(+), 42 deletions(-)

monitor.c +10 ~~-16~~

index b2561e1..a89cbbb 100644
@@ -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;

monitor.c static const mon_cmd_t *qmp_find_cmd(const char *cmdname) · line 3705

Quoted 10 lines

monitor.c +2 -3

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

Quoted 32 lines

monitor.c +6 -9

     /* 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);
     }

monitor.c static const mon_cmd_t *monitor_parse_command(Monitor *mon, · line 3738

Quoted 10 lines

monitor.c +9 -0

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

[...]

Bandan Das

author

2015-06-03 03:29 UTC #3 lore raw

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.

In reply to 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 <[email protected]>

Signed-off-by Bandan Das <[email protected]>

---
 monitor.c | 2 ++
 1 file changed, 2 insertions(+)

monitor.c +2 -0

index a89cbbb..6777cbe 100644
@@ -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;
     }

Signature · 1 line

2.1.0

Markus Armbruster

Reviewed-by on v4 2/4

2015-06-03 11:30 UTC #4 lore raw

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.

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 <[email protected]>

Reviewed-by Markus Armbruster <[email protected]>

Missed that in my original review, sorry.

Bandan Das

author

2015-06-03 03:29 UTC #5 lore raw

PATCH v4 3/4 monitor: Fix failure path for "S" argument type

In reply to Bandan Das

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

Signed-off-by Bandan Das <[email protected]>

---
 monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

monitor.c +1 -1

index 6777cbe..cbc3cc6 100644
@@ -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;

Signature · 1 line

2.1.0

Markus Armbruster

Reviewed-by on v4 3/4

2015-06-03 11:32 UTC #6 lore raw

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

Quoted 2 lines · Bandan Das

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 <[email protected]>

Bandan Das

author

2015-06-03 03:29 UTC #7 lore raw

PATCH v4 4/4 monitor: remove debug prints

In reply to 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.

Signed-off-by Bandan Das <[email protected]>

---
 monitor.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

monitor.c +1 ~~-18~~

index cbc3cc6..44bd787 100644
@@ -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 */

Signature · 1 line

2.1.0

Markus Armbruster

Reviewed-by on v4 4/4

2015-06-03 11:33 UTC #8 lore raw

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

Quoted 6 lines · 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 <[email protected]>
Signed-off-by Bandan Das <[email protected]>

Reviewed-by Markus Armbruster <[email protected]>