Skip to contentSkip to conversation

monitor: suggest running "help" for command errors

PATCH Superseded Bandan Das 2 participants 5 messages May 26, 2015 – May 27, 2015

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Bandan Das

author
#0 lore raw

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

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
monitor: suggest running "help" for command errors

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

PATCH v2 1/2 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.

Suggested-by Markus Armbruster <[email protected]>
Signed-off-by Bandan Das <[email protected]>
---
 monitor.c | 84 ++++++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 33 deletions(-)
monitor.c +51 -33
index b2561e1..f0c1a8c 100644
@@ -3685,8 +3685,7 @@ 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 @cmdline is invalid, report to @mon and return NULL.
  * 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

@@ -3696,23 +3695,19 @@ static const mon_cmd_t *qmp_find_cmd(const char *cmdname)
  */
 static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                                               const char *cmdline,
-                                              int start,
-                                              mon_cmd_t *table,
-                                              QDict *qdict)
+                                              int *start,
+                                              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(cmdline + *start, cmdname, sizeof(cmdname));
     if (!p)
         return NULL;
 

@@ -3727,16 +3722,35 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
     while (qemu_isspace(*p)) {
         p++;
     }
+
+    *start = p - cmdline;
     /* 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, cmdline, start,
+                                     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 @qdict, and return it.
+ */
+
+static QDict *monitor_parse_arguments(Monitor *mon,
+                                      const char *cmdline,
+                                      int *start,
+                                      const mon_cmd_t *cmd)
+{
+    const char *typestr;
+    char *key;
+    int c;
+    const char *p = cmdline + *start;
+    char buf[1024];
+    QDict *qdict = qdict_new();
+
     /* parse the parameters */
     typestr = cmd->args_type;
     for(;;) {

@@ -3766,14 +3780,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 +3929,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 +4037,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 +4071,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 +4080,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 +4091,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;
 }

@@ -4114,12 +4129,17 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
 {
     QDict *qdict;
     const mon_cmd_t *cmd;
+    int start = 0;
 
-    qdict = qdict_new();
+    cmd = monitor_parse_command(mon, cmdline, &start, 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, &start, cmd);
+    if (!qdict) {
+        return;
+    }
 
     if (handler_is_async(cmd)) {
         user_async_cmd_handler(mon, cmd, qdict);

@@ -4137,8 +4157,6 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
         cmd->mhandler.cmd(mon, qdict);
     }
 
-out:
-    QDECREF(qdict);
 }
 
 static void cmd_completion(Monitor *mon, const char *name, const char *list)
Signature · 1 line
2.1.0
#2 lore raw

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

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

Suggested-by Markus Armbruster <[email protected]>
Signed-off-by Bandan Das <[email protected]>
---
 monitor.c | 84 ++++++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 33 deletions(-)
monitor.c +0 -0
index b2561e1..f0c1a8c 100644

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

Quoted 9 lines
monitor.c +1 -2
@@ -3685,8 +3685,7 @@ 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 @cmdline is invalid, report to @mon and return NULL.
  * 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

Function comment fails to document @start. Since you're touching it, you get to fix that :)

I figure this would become a bit simpler if we replaced in/out parameter int start by an out parameter const char **endp.

Quoted 46 lines
monitor.c +10 -16
@@ -3696,23 +3695,19 @@ static const mon_cmd_t *qmp_find_cmd(const char *cmdname)
  */
 static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                                               const char *cmdline,
-                                              int start,
-                                              mon_cmd_t *table,
-                                              QDict *qdict)
+                                              int *start,
+                                              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(cmdline + *start, cmdname, sizeof(cmdname));
     if (!p)
         return NULL;
 

@@ -3727,16 +3722,35 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
     while (qemu_isspace(*p)) {
         p++;
     }
+
+    *start = p - cmdline;
     /* 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, cmdline, start,
+                                     cmd->sub_table);

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

Quoted 10 lines
monitor.c +8 -0
     }
 
+    return cmd;
+}
+
+/*
+ * Parse arguments for @cmd
+ * If it can't be parsed, report to @mon, and return NULL.
+ * Else, insert command arguments into @qdict, and return it.
+ */

Please document @cmdline and @start.

monitor.c static void handle_user_command(Monitor *mon, const char *cmdline)

Quoted 88 lines
monitor.c +23 -9
+
+static QDict *monitor_parse_arguments(Monitor *mon,
+                                      const char *cmdline,
+                                      int *start,
+                                      const mon_cmd_t *cmd)
+{
+    const char *typestr;
+    char *key;
+    int c;
+    const char *p = cmdline + *start;
+    char buf[1024];
+    QDict *qdict = qdict_new();
+
     /* parse the parameters */
     typestr = cmd->args_type;
     for(;;) {

@@ -3766,14 +3780,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 +3929,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 +4037,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 +4071,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 +4080,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 +4091,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;
 }

@@ -4114,12 +4129,17 @@ static void handle_user_command(Monitor *mon, const char *cmdline)

This hunk conflicts with my "[PATCH v2 00/20] monitor: Wean core off QError, and other cleanups". Shouldn't be hard to resolve.

Quoted 5 lines
monitor.c +1 -0
 {
     QDict *qdict;
     const mon_cmd_t *cmd;
+    int start = 0;

monitor.c static void handle_user_command(Monitor *mon, const char *cmdline) · line 4139

Quoted 10 lines
monitor.c +5 -4
-    qdict = qdict_new();
+    cmd = monitor_parse_command(mon, cmdline, &start, 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, &start, cmd);

Since we're not interested in text following arguments (if that's even possible), we could simply do

    qdict = monitor_parse_arguments(mon, cmdline + start, cmd);

If we replace in/out start by out endp as mentioned above, things become simpler still:

    cmd = monitor_parse_command(mon, cmdline, &endp, mon->cmd_table);
    if (!cmd) {
        return;
    }
    qdict = monitor_parse_arguments(mon, endp, cmd);

monitor.c static void handle_user_command(Monitor *mon, const char *cmdline) · line 4141

Quoted 12 lines
monitor.c +3 -2
+    if (!qdict) {
+        return;
+    }
 
     if (handler_is_async(cmd)) {
         user_async_cmd_handler(mon, cmd, qdict);

@@ -4137,8 +4157,6 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
         cmd->mhandler.cmd(mon, qdict);
     }
 
-out:
-    QDECREF(qdict);

Doesn't this leak qdict?

monitor.c static void handle_user_command(Monitor *mon, const char *cmdline) · line 4162

Quoted 3 lines
monitor.c +0 -0
 }
 
 static void cmd_completion(Monitor *mon, const char *name, const char *list)

Overall, I rather like how this turned out.

author
#3 lore raw

PATCH v2 2/2 monitor: suggest running "help" for command errors

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.

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

Signed-off-by Bandan Das <[email protected]>
---
 monitor.c | 2 ++
 1 file changed, 2 insertions(+)
monitor.c +2 -0
index f0c1a8c..af73195 100644
@@ -4138,6 +4138,8 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
 
     qdict = monitor_parse_arguments(mon, cmdline, &start, cmd);
     if (!qdict) {
+        monitor_printf(mon, "Try \"help %s\" for more information\n",
+                       cmd->name);
         return;
     }
Signature · 1 line
2.1.0
Reviewed-by on v2 2/2
#4 lore raw
Quoted 11 lines · 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.

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