diff mbox

rewamp/simplify option parsing

Message ID 1399881324-14859-1-git-send-email-mjt@msgid.tls.msk.ru
State New
Headers show

Commit Message

Michael Tokarev May 12, 2014, 7:55 a.m. UTC
Main change is to allow get_opt_name() to accept
a set of delimiters (string) instead of a single
delimiter (char).  This way it is easier to search
for the next (sub)option in an option string, so
other code using get_opt_name() can be simplified.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
This is an old patch (from Jun 2013) which was
sitting in my local git repository since that.
I rebased it to current master (with 2 trivial
conflicts resolved).

FWIW.

 include/qemu/option.h |    3 +-
 util/qemu-option.c    |  136 +++++++++++++++++++++++--------------------------
 vl.c                  |    4 +-
 3 files changed, 67 insertions(+), 76 deletions(-)

Comments

Eric Blake May 12, 2014, 4:26 p.m. UTC | #1
On 05/12/2014 01:55 AM, Michael Tokarev wrote:

s/rewamp/revamp/ in the subject

> Main change is to allow get_opt_name() to accept
> a set of delimiters (string) instead of a single
> delimiter (char).  This way it is easier to search
> for the next (sub)option in an option string, so
> other code using get_opt_name() can be simplified.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
> This is an old patch (from Jun 2013) which was
> sitting in my local git repository since that.
> I rebased it to current master (with 2 trivial
> conflicts resolved).
> 
> FWIW.
> 
>  include/qemu/option.h |    3 +-
>  util/qemu-option.c    |  136 +++++++++++++++++++++++--------------------------
>  vl.c                  |    4 +-
>  3 files changed, 67 insertions(+), 76 deletions(-)
> 

> @@ -97,24 +95,27 @@ int get_next_param_value(char *buf, int buf_size,
>  
>      p = *pstr;
>      for(;;) {
> -        p = get_opt_name(option, sizeof(option), p, '=');
> -        if (*p != '=')
> -            break;
> -        p++;
> -        if (!strcmp(tag, option)) {
> -            *pstr = get_opt_value(buf, buf_size, p);
> -            if (**pstr == ',') {
> -                (*pstr)++;
> +        if (*p == '\0') {
> +            return 0;
> +        }
> +        p = get_opt_name(option, sizeof(option), p, "=,");
> +        if (strcmp(tag, option) == 0) {
> +            if (*p == '=') {
> +                p = get_opt_value(buf, buf_size, p + 1);
> +            } else {
> +                buf[0] = '\0';
>              }
> -            return strlen(buf);
> +            *pstr = *p == ',' ? p + 1 : p;
> +            return 1;

Does this botch handling of ',,' escaping that allows literal commas as
part of an option value?  I suspect there may be some corner cases you
are altering.  Based just on a quick read of 'qemu-kvm -help', I see:

-name string1[,process=string2][,debug-threads=on|off]

Which, if I'm interpreting correctly, means that pre-patch I can do:

qemu-kvm -name foo,,bar /dev/null

as a way to start a (pretty pointless) machine with the window title of
"QEMU (foo,bar)".  Reading the source code, I also found the implicit
name of string1:

qemu-kvm -name guest=debug-threads= /dev/null

creates the window title "QEMU (debug-threads=)", and:

qemu-kvm -name debug-threads,, /dev/null

creates the window title "QEMU (debug-threads,)".  That is, I have a way
of specifying ANY name, including trailing '=' or ',', for my guest.

I did not apply your patch, but off-hand, I'm guessing that your patch
breaks at least one, if not multiple, of these cases.  We need unit
tests in place first.
diff mbox

Patch

diff --git a/include/qemu/option.h b/include/qemu/option.h
index 8c0ac34..24fdec8 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -50,7 +50,8 @@  typedef struct QEMUOptionParameter {
 } QEMUOptionParameter;
 
 
-const char *get_opt_name(char *buf, int buf_size, const char *p, char delim);
+const char *get_opt_name(char *buf, int buf_size,
+                         const char *p, const char *delim);
 const char *get_opt_value(char *buf, int buf_size, const char *p);
 int get_next_param_value(char *buf, int buf_size,
                          const char *tag, const char **pstr);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 324e4c5..332b177 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -37,27 +37,25 @@ 
  * Extracts the name of an option from the parameter string (p points at the
  * first byte of the option name)
  *
- * The option name is delimited by delim (usually , or =) or the string end
+ * The option name is delimited by delim (usually "=,") or the string end
  * and is copied into buf. If the option name is longer than buf_size, it is
  * truncated. buf is always zero terminated.
  *
  * The return value is the position of the delimiter/zero byte after the option
  * name in p.
  */
-const char *get_opt_name(char *buf, int buf_size, const char *p, char delim)
+const char *get_opt_name(char *buf, int buf_size,
+                         const char *p, const char *delim)
 {
-    char *q;
-
-    q = buf;
-    while (*p != '\0' && *p != delim) {
-        if (q && (q - buf) < buf_size - 1)
-            *q++ = *p;
-        p++;
+    int len = strcspn(p, delim);
+    if (buf_size > len) {
+        buf_size = len;
+    } else {
+        --buf_size;
     }
-    if (q)
-        *q = '\0';
-
-    return p;
+    memcpy(buf, p, buf_size);
+    buf[buf_size] = '\0';
+    return p + len;
 }
 
 /*
@@ -97,24 +95,27 @@  int get_next_param_value(char *buf, int buf_size,
 
     p = *pstr;
     for(;;) {
-        p = get_opt_name(option, sizeof(option), p, '=');
-        if (*p != '=')
-            break;
-        p++;
-        if (!strcmp(tag, option)) {
-            *pstr = get_opt_value(buf, buf_size, p);
-            if (**pstr == ',') {
-                (*pstr)++;
+        if (*p == '\0') {
+            return 0;
+        }
+        p = get_opt_name(option, sizeof(option), p, "=,");
+        if (strcmp(tag, option) == 0) {
+            if (*p == '=') {
+                p = get_opt_value(buf, buf_size, p + 1);
+            } else {
+                buf[0] = '\0';
             }
-            return strlen(buf);
+            *pstr = *p == ',' ? p + 1 : p;
+            return 1;
         } else {
-            p = get_opt_value(NULL, 0, p);
+            if (*p == '=') {
+                p = get_opt_value(NULL, 0, p + 1);
+            }
+            if (*p == ',') {
+                ++p;
+            }
         }
-        if (*p != ',')
-            break;
-        p++;
     }
-    return 0;
 }
 
 int get_param_value(char *buf, int buf_size,
@@ -399,8 +400,7 @@  QEMUOptionParameter *parse_option_parameters(const char *param,
     QEMUOptionParameter *allocated = NULL;
     char name[256];
     char value[256];
-    char *param_delim, *value_delim;
-    char next_delim;
+    char *value_p;
     int i;
 
     if (list == NULL) {
@@ -417,27 +417,19 @@  QEMUOptionParameter *parse_option_parameters(const char *param,
 
     while (*param) {
 
-        // Find parameter name and value in the string
-        param_delim = strchr(param, ',');
-        value_delim = strchr(param, '=');
-
-        if (value_delim && (value_delim < param_delim || !param_delim)) {
-            next_delim = '=';
-        } else {
-            next_delim = ',';
-            value_delim = NULL;
-        }
-
-        param = get_opt_name(name, sizeof(name), param, next_delim);
-        if (value_delim) {
+        param = get_opt_name(name, sizeof(name), param, "=,");
+        if (*param == '=') {
             param = get_opt_value(value, sizeof(value), param + 1);
+            value_p = value;
+        } else {
+            value_p = NULL;
         }
         if (*param != '\0') {
             param++;
         }
 
         // Set the parameter
-        if (set_option_parameter(dest, name, value_delim ? value : NULL)) {
+        if (set_option_parameter(dest, name, value_p)) {
             goto fail;
         }
     }
@@ -911,49 +903,47 @@  int qemu_opts_print(QemuOpts *opts, void *dummy)
 static int opts_do_parse(QemuOpts *opts, const char *params,
                          const char *firstname, bool prepend)
 {
-    char option[128], value[1024];
-    const char *p,*pe,*pc;
+    char buf[1024+128], *bp;
+    const char *opt, *val;
+    const char *p = params;
+    bool first = true;
     Error *local_err = NULL;
 
-    for (p = params; *p != '\0'; p++) {
-        pe = strchr(p, '=');
-        pc = strchr(p, ',');
-        if (!pe || (pc && pc < pe)) {
+    while (*p != '\0') {
+        p = get_opt_name(buf, sizeof(buf), p, "=,");
+        if (*p == '=') {
+            /* found "foo=bar,more" */
+            opt = buf;
+            bp = buf + strlen(buf) + 1;
+            p = get_opt_value(bp, buf + sizeof(buf) - bp, p + 1);
+            val = bp;
+        } else if (first && firstname) {
             /* found "foo,more" */
-            if (p == params && firstname) {
-                /* implicitly named first option */
-                pstrcpy(option, sizeof(option), firstname);
-                p = get_opt_value(value, sizeof(value), p);
-            } else {
-                /* option without value, probably a flag */
-                p = get_opt_name(option, sizeof(option), p, ',');
-                if (strncmp(option, "no", 2) == 0) {
-                    memmove(option, option+2, strlen(option+2)+1);
-                    pstrcpy(value, sizeof(value), "off");
-                } else {
-                    pstrcpy(value, sizeof(value), "on");
-                }
-            }
+            /* implicitly named first option */
+            val = buf;
+            opt = firstname;
         } else {
-            /* found "foo=bar,more" */
-            p = get_opt_name(option, sizeof(option), p, '=');
-            if (*p != '=') {
-                break;
+            /* option without value, probably a flag */
+            if (strncmp(buf, "no", 2) == 0) {
+                opt = buf + 2;
+                val = "off";
+            } else {
+                opt = buf;
+                val = "on";
             }
-            p++;
-            p = get_opt_value(value, sizeof(value), p);
         }
-        if (strcmp(option, "id") != 0) {
+        if (strcmp(opt, "id") != 0) {
             /* store and parse */
-            opt_set(opts, option, value, prepend, &local_err);
+            opt_set(opts, opt, val, prepend, &local_err);
             if (local_err) {
                 qerror_report_err(local_err);
                 error_free(local_err);
                 return -1;
             }
         }
-        if (*p != ',') {
-            break;
+        first = false;
+        if (*p == ',') {
+            ++p;
         }
     }
     return 0;
diff --git a/vl.c b/vl.c
index 73e0661..8d858d2 100644
--- a/vl.c
+++ b/vl.c
@@ -1315,7 +1315,7 @@  static void numa_add(const char *optarg)
     char *endptr;
     unsigned long long nodenr;
 
-    optarg = get_opt_name(option, 128, optarg, ',');
+    optarg = get_opt_name(option, sizeof(option), optarg, ",");
     if (*optarg == ',') {
         optarg++;
     }
@@ -2757,7 +2757,7 @@  static int configure_accelerator(MachineClass *mc)
         if (*p == ':') {
             p++;
         }
-        p = get_opt_name(buf, sizeof (buf), p, ':');
+        p = get_opt_name(buf, sizeof(buf), p, ":");
         for (i = 0; i < ARRAY_SIZE(accel_list); i++) {
             if (strcmp(accel_list[i].opt_name, buf) == 0) {
                 if (!accel_list[i].available()) {