monitor: suggest running "help" for command errors

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

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

---
 monitor.c | 90 +++++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 53 insertions(+), 37 deletions(-)

monitor.c +0 -0

index b2561e1..521258d 100644
@@ -3683,11 +3683,10 @@ static const mon_cmd_t *qmp_find_cmd(const char *cmdname)
 }
 

monitor.c +2 -5

  * 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;

monitor.c +5 -9

 #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') {

monitor.c +12 -7

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

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

Suggested-by Markus Armbruster <[email protected]>

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

---
 monitor.c | 90 +++++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 53 insertions(+), 37 deletions(-)

monitor.c +0 -0

index b2561e1..521258d 100644
@@ -3683,11 +3683,10 @@ static const mon_cmd_t *qmp_find_cmd(const char *cmdname)
 }
 

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

Quoted 10 lines

monitor.c +4 -5

 /*
- * 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.

Quoted 22 lines

monitor.c +3 -8

  * 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;
 

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

Quoted 3 lines

monitor.c +1 -1

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

Quoted 31 lines

monitor.c +5 -9

 #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') {

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

Quoted 10 lines

monitor.c +8 -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.

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

Quoted 54 lines

monitor.c +18 -5

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

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

Quoted 6 lines

monitor.c +1 -1

                 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.

Quoted 1 line

monitor.c +0 -0

                 }

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

Quoted 4 lines

monitor.c +1 -1

-out:
+    /* Drop reference taken in monitor_parse_arguments */
     QDECREF(qdict);
 }

Very close to earning my R-by.

Quoted 11 lines · Markus Armbruster

Quoted 9 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

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

Quoted 3 lines

#ifdef DEBUG

monitor.c +1 -1

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

Quoted 13 lines · Bandan Das

Quoted 11 lines · Markus Armbruster

Quoted 9 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 7 lines

Quoted 5 lines

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

Quoted 3 lines

#ifdef DEBUG

monitor.c +1 -1

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

Quoted 36 lines · Eric Blake

Quoted 13 lines · Bandan Das

Quoted 11 lines · Markus Armbruster

Quoted 9 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 7 lines

Quoted 5 lines

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

Quoted 3 lines

#ifdef DEBUG

monitor.c +1 -1

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

Quoted 6 lines

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.