Skip to contentSkip to conversation

monitor: suggest running "help" for command errors

PATCH Applied Bandan Das 3 participants 10 messages Jun 3, 2015 – Jun 16, 2015

lore
Share & tools
lore

Bandan Das

author
#0 lore raw

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

v5:
Move "monitor: remove debug prints" to first in the series Minor fixes to comments and commit messages

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: remove debug prints
monitor: cleanup parsing of cmd name and cmd arguments
monitor: Point to "help" command on syntax error
monitor: Fix failure path for "S" argument

 monitor.c | 122 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 61 insertions(+), 61 deletions(-)
Signature · 1 line
2.1.0
author
#1 lore raw

PATCH v5 1/4 monitor: remove debug prints

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.

---
 monitor.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)
monitor.c +1 -19
index b2561e1..cc4e7d1 100644
@@ -81,9 +81,6 @@
 #endif
 #include "hw/lm32/lm32_pic.h"
 
-//#define DEBUG
-//#define DEBUG_COMPLETION
-
 /*
  * Supported types:
  *
@@ -3707,10 +3704,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
     char buf[1024];
     char *key;
 
-#ifdef DEBUG
-    monitor_printf(mon, "command='%s', start='%d'\n", cmdline, start);
-#endif
-
     /* extract the command name */
     p = get_command_name(cmdline + start, cmdname, sizeof(cmdname));
     if (!p)
@@ -4189,10 +4182,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;
@@ -4770,14 +4760,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
author
#2 lore raw

PATCH v5 2/4 monitor: cleanup parsing of cmd name and cmd arguments

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

---
 monitor.c | 98 +++++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 57 insertions(+), 41 deletions(-)
monitor.c +57 -41
index cc4e7d1..33d088e 100644
@@ -3680,39 +3680,32 @@ 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;
 
     /* 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;
     }
 
@@ -3720,16 +3713,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(;;) {
@@ -3759,14 +3770,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;
@@ -3908,7 +3919,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') {
@@ -4016,7 +4027,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;
@@ -4050,7 +4061,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));
@@ -4059,7 +4070,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);
@@ -4070,13 +4081,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;
 }
@@ -4108,11 +4120,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);
@@ -4130,7 +4146,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
#3 lore raw

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

Quoted 10 lines · 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.

Doesn't apply cleanly anymore. Please double-check my conflict resolution carefully:

monitor.c +0 -0
index bcb88cd..0b0a8df 100644

[...]
@@ -4156,13 +4168,17 @@ static void handle_hmp_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) {
- cmd->mhandler.cmd(mon, qdict);
+ qdict = monitor_parse_arguments(mon, &cmdline, cmd);
+ if (!qdict) {

+        return;
     }
+    cmd->mhandler.cmd(mon, qdict);
     QDECREF(qdict);

}

author
#4 lore raw

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

Quoted 10 lines · Markus Armbruster
Quoted 8 lines · 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.

Quoted 5 lines
Quoted 2 lines

Doesn't apply cleanly anymore. Please double-check my conflict resolution carefully:

Looks good, Markus. Thank you for taking care of the conflict.

Bandan

Quoted 27 lines
monitor.c +8 -4
index bcb88cd..0b0a8df 100644
[...]
@@ -4156,13 +4168,17 @@ static void handle_hmp_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) {
-        cmd->mhandler.cmd(mon, qdict);
+    qdict = monitor_parse_arguments(mon, &cmdline, cmd);
+    if (!qdict) {
+        return;
     }
 
+    cmd->mhandler.cmd(mon, qdict);
     QDECREF(qdict);
 }
 
author
#5 lore raw

PATCH v5 3/4 monitor: Point to "help" command on syntax error

In reply to Bandan Das

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 | 2 ++
 1 file changed, 2 insertions(+)
monitor.c +2 -0
index 33d088e..640c05c 100644
@@ -4127,6 +4127,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
author
#6 lore raw

PATCH v5 4/4 monitor: Fix failure path for "S" argument

In reply to Bandan Das

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

---
 monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
monitor.c +1 -1
index 640c05c..5a18844 100644
@@ -4062,7 +4062,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
#8 lore raw
Quoted 10 lines · On Mon, 08 Jun 2015 10:53:23 +0200 Markus Armbruster <[email protected]> wrote:

Copying HMP maintainer Luiz.

Series

Bandan, thanks for your patience.

Luiz, my monitor/QMP queue is currently empty, but if it fills up before you get around to doing a monitor/HMP pull request, I'm happy to take this series along, if it gets your Acked-by.

I'd be immensely grateful if you pick this series along with the other ones, as we've spoken in pvt. Thanks a lot Markus for your help!