[PATCH 1/2] keyval: accept escaped commas in implied option

[Paolo Bonzini]

This is used with the weirdly-named device "SUNFD,two", so accepting it
is also a preparatory step towards keyval-ifying -device and the
device_add monitor command.  But in general it is an unexpected wart
of the keyval syntax and leads to suboptimal errors compared to QemuOpts:

  $ ./qemu-system-x86_64 -object foo,,bar,id=obj
  qemu-system-x86_64: -object foo,,bar,id=obj: invalid object type: foo,bar
  $ storage-daemon/qemu-storage-daemon --object foo,,bar,id=obj
  qemu-storage-daemon: Invalid parameter ''

To implement this, the flow of the parser is changed to first unescape
everything up to the next comma or equal sign.  This is done in a
new function keyval_fetch_string for both the key and value part.
Keys therefore are now parsed in unescaped form, but this makes no
difference in practice because a comma is an invalid character for a
QAPI name.  Thus keys with a comma in them are rejected anyway, as
demonstrated by the new testcase.

As a side effect of the new code, parse errors are slightly improved as
well: "Invalid parameter ''" becomes "Expected parameter before '='"
when keyval is fed a string starting with an equal sign.

The slightly baroque interface of keyval_fetch_string lets me keep the
key parsing loop mostly untouched.  It is simplified in the next patch,
however.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/help_option.h |  11 ---
 tests/test-keyval.c        |  21 +++---
 util/keyval.c              | 142 ++++++++++++++++++++-----------------
 3 files changed, 91 insertions(+), 83 deletions(-)

diff --git a/include/qemu/help_option.h b/include/qemu/help_option.h
index ca6389a154..328d2a89fd 100644
--- a/include/qemu/help_option.h
+++ b/include/qemu/help_option.h
@@ -19,15 +19,4 @@ static inline bool is_help_option(const char *s)
     return !strcmp(s, "?") || !strcmp(s, "help");
 }
 
-static inline int starts_with_help_option(const char *s)
-{
-    if (*s == '?') {
-        return 1;
-    }
-    if (g_str_has_prefix(s, "help")) {
-        return 4;
-    }
-    return 0;
-}
-
 #endif
diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index ee927fe4e4..19f664f535 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -89,6 +89,11 @@ static void test_keyval_parse(void)
     error_free_or_abort(&err);
     g_assert(!qdict);
 
+    /* Keys must be QAPI identifiers */
+    qdict = keyval_parse("weird,,=key", NULL, NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
     /* Multiple keys, last one wins */
     qdict = keyval_parse("a=1,b=2,,x,a=3", NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 2);
@@ -178,15 +183,15 @@ static void test_keyval_parse(void)
     error_free_or_abort(&err);
     g_assert(!qdict);
 
-    /* Likewise (qemu_opts_parse(): implied key with comma value) */
-    qdict = keyval_parse(",,,a=1", "implied", NULL, &err);
-    error_free_or_abort(&err);
-    g_assert(!qdict);
+    /* Implied key's value can have a comma */
+    qdict = keyval_parse(",,,a=1", "implied", NULL, &error_abort);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, ",");
+    g_assert_cmpstr(qdict_get_try_str(qdict, "a"), ==, "1");
+    qobject_unref(qdict);
 
-    /* Implied key's value can't have comma (qemu_opts_parse(): it can) */
-    qdict = keyval_parse("val,,ue", "implied", NULL, &err);
-    error_free_or_abort(&err);
-    g_assert(!qdict);
+    qdict = keyval_parse("val,,ue", "implied", NULL, &error_abort);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "val,ue");
+    qobject_unref(qdict);
 
     /* Empty key is not an implied key */
     qdict = keyval_parse("=val", "implied", NULL, &err);
diff --git a/util/keyval.c b/util/keyval.c
index 7f625ad33c..2213a5fe78 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -16,8 +16,8 @@
  *   key-vals     = [ key-val { ',' key-val } [ ',' ] ]
  *   key-val      = key '=' val | help
  *   key          = key-fragment { '.' key-fragment }
- *   key-fragment = / [^=,.]+ /
- *   val          = { / [^,]+ / | ',,' }
+ *   key-fragment = { / [^=,.] / | ',,' }
+ *   val          = { / [^,] / | ',,' }
  *   help         = 'help' | '?'
  *
  * Semantics defined by reduction to JSON:
@@ -78,13 +78,13 @@
  * Alternative syntax for use with an implied key:
  *
  *   key-vals     = [ key-val-1st { ',' key-val } [ ',' ] ]
- *   key-val-1st  = val-no-key | key-val
- *   val-no-key   = / [^=,]+ / - help
+ *   key-val-1st  = (val-no-key - help) | key-val
+ *   val-no-key   = { / [^=,] / | ',,' }
  *
  * where val-no-key is syntactic sugar for implied-key=val-no-key.
  *
- * Note that you can't use the sugared form when the value contains
- * '=' or ','.
+ * Note that you can't use the sugared form when the value is empty
+ * or contains '='.
  */
 
 #include "qemu/osdep.h"
@@ -141,7 +141,7 @@ static int key_to_index(const char *key, const char **end)
  * On failure, store an error through @errp and return NULL.
  */
 static QObject *keyval_parse_put(QDict *cur,
-                                 const char *key_in_cur, QString *value,
+                                 const char *key_in_cur, const char *value,
                                  const char *key, const char *key_cursor,
                                  Error **errp)
 {
@@ -152,20 +152,56 @@ static QObject *keyval_parse_put(QDict *cur,
         if (qobject_type(old) != (value ? QTYPE_QSTRING : QTYPE_QDICT)) {
             error_setg(errp, "Parameters '%.*s.*' used inconsistently",
                        (int)(key_cursor - key), key);
-            qobject_unref(value);
             return NULL;
         }
         if (!value) {
             return old;         /* already QDict, do nothing */
         }
-        new = QOBJECT(value);   /* replacement */
-    } else {
-        new = value ? QOBJECT(value) : QOBJECT(qdict_new());
     }
+    new = value ? QOBJECT(qstring_from_str(value)) : QOBJECT(qdict_new());
     qdict_put_obj(cur, key_in_cur, new);
     return new;
 }
 
+/*
+ * Parse and unescape the string (key or value) pointed to by @start,
+ * stopping at a single comma or if @key is true an equal sign.
+ * The string is unescaped and NUL-terminated in place.
+ *
+ * On return:
+ * - either NUL or the separator (comma or equal sign) is returned.
+ * - the length of the string is stored in @len.
+ * - @start is advanced to either the NUL or the first character past the
+ *   separator.
+ */
+static char keyval_fetch_string(char **start, size_t *len, bool key)
+{
+    char sep;
+    char *p, *unescaped;
+    p = unescaped = *start;
+    for (;;) {
+        sep = *p;
+        if (!sep) {
+            break;
+        }
+        if (key && sep == '=') {
+            ++p;
+            break;
+        }
+        if (sep == ',') {
+            if (*++p != ',') {
+                break;
+            }
+        }
+        *unescaped++ = *p++;
+    }
+
+    *unescaped = 0;
+    *len = unescaped - *start;
+    *start = p;
+    return sep;
+}
+
 /*
  * Parse one parameter from @params.
  *
@@ -179,35 +215,42 @@ static QObject *keyval_parse_put(QDict *cur,
  * On success, return a pointer to the next parameter, or else to '\0'.
  * On failure, return NULL.
  */
-static const char *keyval_parse_one(QDict *qdict, const char *params,
-                                    const char *implied_key, bool *help,
-                                    Error **errp)
+static char *keyval_parse_one(QDict *qdict, char *params,
+                              const char *implied_key, bool *help,
+                              Error **errp)
 {
-    const char *key, *key_end, *val_end, *s, *end;
+    const char *key, *key_end, *s, *end;
+    const char *val = NULL;
+    char sep;
     size_t len;
     char key_in_cur[128];
     QDict *cur;
     int ret;
     QObject *next;
-    QString *val;
 
     key = params;
-    val_end = NULL;
-    len = strcspn(params, "=,");
-    if (len && key[len] != '=') {
-        if (starts_with_help_option(key) == len) {
+    sep = keyval_fetch_string(&params, &len, true);
+    if (!len) {
+        if (sep) {
+            error_setg(errp, "Expected parameter before '%c'", sep);
+        } else {
+            error_setg(errp, "Expected parameter at end of string");
+        }
+        return NULL;
+    }
+    if (sep != '=') {
+        if (is_help_option(key)) {
             *help = true;
-            s = key + len;
-            if (*s == ',') {
-                s++;
-            }
-            return s;
+            return params;
         }
         if (implied_key) {
             /* Desugar implied key */
+            val = key;
             key = implied_key;
-            val_end = params + len;
             len = strlen(implied_key);
+        } else {
+            error_setg(errp, "Expected '=' after parameter '%s'", key);
+            return NULL;
         }
     }
     key_end = key + len;
@@ -218,7 +261,7 @@ static const char *keyval_parse_one(QDict *qdict, const 
char *params,
      */
     cur = qdict;
     s = key;
-    for (;;) {
+    do {
         /* Want a key index (unless it's first) or a QAPI name */
         if (s != key && key_to_index(s, &end) >= 0) {
             len = end - s;
@@ -254,46 +297,16 @@ static const char *keyval_parse_one(QDict *qdict, const 
char *params,
         memcpy(key_in_cur, s, len);
         key_in_cur[len] = 0;
         s += len;
+    } while (*s++ == '.');
 
-        if (*s != '.') {
-            break;
-        }
-        s++;
-    }
-
-    if (key == implied_key) {
-        assert(!*s);
-        val = qstring_from_substr(params, 0, val_end - params);
-        s = val_end;
-        if (*s == ',') {
-            s++;
-        }
-    } else {
-        if (*s != '=') {
-            error_setg(errp, "Expected '=' after parameter '%.*s'",
-                       (int)(s - key), key);
-            return NULL;
-        }
-        s++;
-
-        val = qstring_new();
-        for (;;) {
-            if (!*s) {
-                break;
-            } else if (*s == ',') {
-                s++;
-                if (*s != ',') {
-                    break;
-                }
-            }
-            qstring_append_chr(val, *s++);
-        }
+    if (key != implied_key) {
+        val = params;
+        keyval_fetch_string(&params, &len, false);
     }
-
     if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) {
         return NULL;
     }
-    return s;
+    return params;
 }
 
 static char *reassemble_key(GSList *key)
@@ -438,10 +451,11 @@ QDict *keyval_parse(const char *params, const char 
*implied_key,
 {
     QDict *qdict = qdict_new();
     QObject *listified;
-    const char *s;
+    g_autofree char *dup;
+    char *s;
     bool help = false;
 
-    s = params;
+    s = dup = g_strdup(params);
     while (*s) {
         s = keyval_parse_one(qdict, s, implied_key, &help, errp);
         if (!s) {
-- 
2.26.2