diff mbox

[v4,1/4] monitor: cleanup parsing of cmd name and cmd arguments

Message ID 1433302122-27397-2-git-send-email-bsd@redhat.com
State New
Headers show

Commit Message

Bandan Das June 3, 2015, 3:28 a.m. UTC
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(-)

Comments

Markus Armbruster June 3, 2015, 11:26 a.m. UTC | #1
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.

[...]
diff mbox

Patch

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