keyval_parse() parses KEY=VALUE,... into a QDict.  Works like
qemu_opts_parse(), except:

* Returns a QDict instead of a QemuOpts (d'oh).

* It supports nesting, unlike QemuOpts: a KEY is split into key
  components at '.' (dotted key convention; the block layer does
  something similar on top of QemuOpts).  The key components are QDict
  keys, and the last one's value is updated to VALUE.

* Each key component may be up to 127 bytes long.  qemu_opts_parse()
  limits the entire key to 127 bytes.

* Overlong key components are rejected.  qemu_opts_parse() silently
  truncates them.

* Empty key components are rejected.  qemu_opts_parse() happily
  accepts empty keys.

* It does not store the returned value.  qemu_opts_parse() stores it
  in the QemuOptsList.

* It does not treat parameter "id" specially.  qemu_opts_parse()
  ignores all but the first "id", and fails when its value isn't
  id_wellformed(), or duplicate (a QemuOpts with the same ID is
  already stored).  It also screws up when a value contains ",id=".

I intend to grow this into a saner replacement for QemuOpts.  It'll
take time, though.

TODO Support lists

TODO Function comment is missing.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qemu/option.h  |   3 +
 tests/.gitignore       |   1 +
 tests/Makefile.include |   3 +
 tests/test-keyval.c    | 154 +++++++++++++++++++++++++++++++++++++++++++++++++
 util/Makefile.objs     |   1 +
 util/keyval.c          | 150 +++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 312 insertions(+)
 create mode 100644 tests/test-keyval.c
 create mode 100644 util/keyval.c

diff --git a/include/qemu/option.h b/include/qemu/option.h
index e786df0..f7338db 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -141,4 +141,7 @@ void qemu_opts_print_help(QemuOptsList *list);
 void qemu_opts_free(QemuOptsList *list);
 QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list);
 
+QDict *keyval_parse(const char *params, const char *implied_key,
+                    Error **errp);
+
 #endif
diff --git a/tests/.gitignore b/tests/.gitignore
index dc37519..30b7740 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -47,6 +47,7 @@ test-io-channel-file.txt
 test-io-channel-socket
 test-io-channel-tls
 test-io-task
+test-keyval
 test-logging
 test-mul64
 test-opts-visitor
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 5591f60..5b66651 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -95,6 +95,8 @@ check-unit-y += tests/check-qom-proplist$(EXESUF)
 gcov-files-check-qom-proplist-y = qom/object.c
 check-unit-y += tests/test-qemu-opts$(EXESUF)
 gcov-files-test-qemu-opts-y = util/qemu-option.c
+check-unit-y += tests/test-keyval$(EXESUF)
+gcov-files-test-keyval-y = util/keyval.c
 check-unit-y += tests/test-write-threshold$(EXESUF)
 gcov-files-test-write-threshold-y = block/write-threshold.c
 check-unit-y += tests/test-crypto-hash$(EXESUF)
@@ -720,6 +722,7 @@ tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o $(test-util-obj-y) \
 	$(chardev-obj-y)
 tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
 tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o $(test-util-obj-y)
+tests/test-keyval$(EXESUF): tests/test-keyval.o $(test-util-obj-y)
 tests/test-write-threshold$(EXESUF): tests/test-write-threshold.o $(test-block-obj-y)
 tests/test-netfilter$(EXESUF): tests/test-netfilter.o $(qtest-obj-y)
 tests/test-filter-mirror$(EXESUF): tests/test-filter-mirror.o $(qtest-obj-y)
diff --git a/tests/test-keyval.c b/tests/test-keyval.c
new file mode 100644
index 0000000..91b4391
--- /dev/null
+++ b/tests/test-keyval.c
@@ -0,0 +1,154 @@
+/*
+ * Unit tests for parsing of KEY=VALUE,... strings
+ *
+ * Copyright (C) 2017 Red Hat Inc.
+ *
+ * Authors:
+ *  Markus Armbruster <armbru@redhat.com>,
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/option.h"
+
+static void test_keyval_parse(void)
+{
+    Error *err = NULL;
+    QDict *qdict, *sub_qdict;
+    char long_key[129];
+    char *params;
+
+    /* Nothing */
+    qdict = keyval_parse("", NULL, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 0);
+    QDECREF(qdict);
+
+    /* Empty key */
+    qdict = keyval_parse("=val", NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* Empty key component */
+    qdict = keyval_parse(".", NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+    qdict = keyval_parse("key.", NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* Overlong key */
+    memset(long_key, 'a', 127);
+    long_key[127] = 'z';
+    long_key[128] = 0;
+    params = g_strdup_printf("k.%s=v", long_key);
+    qdict = keyval_parse(params + 2, NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* Overlong key component */
+    qdict = keyval_parse(params, NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+    g_free(params);
+
+    /* Long key */
+    params = g_strdup_printf("k.%s=v", long_key + 1);
+    qdict = keyval_parse(params + 2, NULL, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(qdict, long_key + 1), ==, "v");
+    QDECREF(qdict);
+
+    /* Long key component */
+    qdict = keyval_parse(params, NULL, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    sub_qdict = qdict_get_qdict(qdict, "k");
+    g_assert(sub_qdict);
+    g_assert_cmpuint(qdict_size(sub_qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(sub_qdict, long_key + 1), ==, "v");
+    QDECREF(qdict);
+    g_free(params);
+
+    /* Multiple keys, last one wins */
+    qdict = keyval_parse("a=1,b=2,,x,a=3", NULL, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 2);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "a"), ==, "3");
+    g_assert_cmpstr(qdict_get_try_str(qdict, "b"), ==, "2,x");
+    QDECREF(qdict);
+
+    /* Even when it doesn't in QemuOpts */
+    qdict = keyval_parse("id=foo,id=bar", NULL, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "id"), ==, "bar");
+    QDECREF(qdict);
+
+    /* Dotted keys */
+    qdict = keyval_parse("a.b.c=1,a.b.c=2,d=3", NULL, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 2);
+    sub_qdict = qdict_get_qdict(qdict, "a");
+    g_assert(sub_qdict);
+    g_assert_cmpuint(qdict_size(sub_qdict), ==, 1);
+    sub_qdict = qdict_get_qdict(sub_qdict, "b");
+    g_assert(sub_qdict);
+    g_assert_cmpuint(qdict_size(sub_qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(sub_qdict, "c"), ==, "2");
+    g_assert_cmpstr(qdict_get_try_str(qdict, "d"), ==, "3");
+    QDECREF(qdict);
+
+    /* Inconsistent dotted keys */
+    qdict = keyval_parse("a.b=1,a=2", NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+    qdict = keyval_parse("a.b=1,a.b.c=2", NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* Implied value */
+    qdict = keyval_parse("an,noaus,noaus=", NULL, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 3);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "an"), ==, "on");
+    g_assert_cmpstr(qdict_get_try_str(qdict, "aus"), ==, "off");
+    g_assert_cmpstr(qdict_get_try_str(qdict, "noaus"), ==, "");
+    QDECREF(qdict);
+
+    /* Implied key */
+    qdict = keyval_parse("an,noaus,noaus=", "implied", &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 3);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "an");
+    g_assert_cmpstr(qdict_get_try_str(qdict, "aus"), ==, "off");
+    g_assert_cmpstr(qdict_get_try_str(qdict, "noaus"), ==, "");
+    QDECREF(qdict);
+
+    /* Trailing comma is ignored */
+    qdict = keyval_parse("x=y,", NULL,  &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "x"), ==, "y");
+    QDECREF(qdict);
+
+    /* Except when it isn't */
+    qdict = keyval_parse(",", NULL,  &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* Value containing ,id= not misinterpreted as QemuOpts does */
+    qdict = keyval_parse("x=,,id=bar", NULL,  &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "x"), ==, ",id=bar");
+    QDECREF(qdict);
+
+    /* Anti-social ID is left to caller */
+    qdict = keyval_parse("id=666", NULL, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "id"), ==, "666");
+    QDECREF(qdict);
+}
+
+int main(int argc, char *argv[])
+{
+    g_test_init(&argc, &argv, NULL);
+    g_test_add_func("/keyval/keyval_parse", test_keyval_parse);
+    g_test_run();
+    return 0;
+}
diff --git a/util/Makefile.objs b/util/Makefile.objs
index bc629e2..06366b5 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -24,6 +24,7 @@ util-obj-y += error.o qemu-error.o
 util-obj-y += id.o
 util-obj-y += iov.o qemu-config.o qemu-sockets.o uri.o notify.o
 util-obj-y += qemu-option.o qemu-progress.o
+util-obj-y += keyval.o
 util-obj-y += hexdump.o
 util-obj-y += crc32c.o
 util-obj-y += uuid.o
diff --git a/util/keyval.c b/util/keyval.c
new file mode 100644
index 0000000..c6cdd22
--- /dev/null
+++ b/util/keyval.c
@@ -0,0 +1,150 @@
+/*
+ * Parsing KEY=VALUE,... strings
+ *
+ * Copyright (C) 2017 Red Hat Inc.
+ *
+ * Authors:
+ *  Markus Armbruster <armbru@redhat.com>,
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qapi/qmp/qstring.h"
+#include "qemu/option.h"
+
+/* TODO Support lists */
+
+static QObject *keyval_parse_put(QDict *qdict, const char *key, QString *value,
+                                 Error **errp)
+{
+    QObject *old, *new;
+
+    old = qdict_get(qdict, key);
+    if (old) {
+        if (qobject_type(old) != (value ? QTYPE_QSTRING : QTYPE_QDICT)) {
+            error_setg(errp, "Option key '%s' used inconsistently", key);
+            return NULL;
+        }
+        if (!value) {
+            return old;
+        }
+        new = QOBJECT(value);
+    } else {
+        new = QOBJECT(value) ?: QOBJECT(qdict_new());
+    }
+    qdict_put_obj(qdict, key, new);
+    return new;
+}
+
+static const char *keyval_parse_one(QDict *qdict, const char *params,
+                                    const char *implied_key,
+                                    Error **errp)
+{
+    QDict *cur = qdict;
+    QObject *next;
+    const char *s, *key;
+    size_t len;
+    char key_buf[128];
+    QString *val;
+
+    s = params;
+    len = strcspn(s, ".=,");
+    if (implied_key && (s[len] == ',' || !s[len])) {
+        /* Desugar implied key */
+        key = implied_key;
+    } else {
+        key_buf[0] = 0;
+        for (;;) {
+            if (!len) {
+                error_setg(errp, "Invalid option key");
+                return NULL;
+            }
+            if (len >= sizeof(key_buf)) {
+                error_setg(errp, "Option key component '%.*s' is too long",
+                           (int)len, s);
+                return NULL;
+            }
+
+            if (key_buf[0]) {
+                next = keyval_parse_put(cur, key_buf, NULL, errp);
+                if (!next) {
+                    return NULL;
+                }
+                cur = qobject_to_qdict(next);
+                assert(cur);
+            }
+
+            memcpy(key_buf, s, len);
+            key_buf[len] = 0;
+            s += len;
+            if (*s != '.') {
+                break;
+            }
+            s++;
+            len = strcspn(s, ".=,");
+        }
+        key = key_buf;
+
+        if (*s == '=') {
+            s++;
+        } else {
+            /*
+             * Desugar implied value: it's "on", except when @key
+             * starts with "no", it's "off".  Thus, key "novocaine"
+             * gets desugard to "vocaine=off", not to "novocaine=on".
+             * If sugar isn't bad enough for you, make it ambiguous...
+             */
+            if (*s == ',')
+                s++;
+            if (!strncmp(key, "no", 2)) {
+                key += 2;
+                val = qstring_from_str("off");
+            } else {
+                val = qstring_from_str("on");
+            }
+            goto got_val;
+        }
+    }
+
+    val = qstring_new();
+    for (;;) {
+        if (!*s) {
+            break;
+        } else if (*s == ',') {
+            s++;
+            if (*s != ',') {
+                break;
+            }
+        }
+        qstring_append_chr(val, *s++);
+    }
+
+got_val:
+    if (!keyval_parse_put(cur, key, val, errp)) {
+        return NULL;
+    }
+    return s;
+}
+
+/* TODO function comment */
+QDict *keyval_parse(const char *params, const char *implied_key,
+                    Error **errp)
+{
+    QDict *qdict = qdict_new();
+    const char *s;
+
+    s = params;
+    while (*s) {
+        s = keyval_parse_one(qdict, s, implied_key, errp);
+        if (!s) {
+            QDECREF(qdict);
+            return NULL;
+        }
+        implied_key = NULL;
+    }
+
+    return qdict;
+}
-- 
2.7.4

On 02/21/2017 03:01 PM, Markus Armbruster wrote:
> keyval_parse() parses KEY=VALUE,... into a QDict.  Works like
> qemu_opts_parse(), except:
> 
> * Returns a QDict instead of a QemuOpts (d'oh).
> 
> * It supports nesting, unlike QemuOpts: a KEY is split into key
>   components at '.' (dotted key convention; the block layer does
>   something similar on top of QemuOpts).  The key components are QDict
>   keys, and the last one's value is updated to VALUE.
> 
> * Each key component may be up to 127 bytes long.  qemu_opts_parse()
>   limits the entire key to 127 bytes.
> 
> * Overlong key components are rejected.  qemu_opts_parse() silently
>   truncates them.
> 
> * Empty key components are rejected.  qemu_opts_parse() happily
>   accepts empty keys.
> 
> * It does not store the returned value.  qemu_opts_parse() stores it
>   in the QemuOptsList.
> 
> * It does not treat parameter "id" specially.  qemu_opts_parse()
>   ignores all but the first "id", and fails when its value isn't
>   id_wellformed(), or duplicate (a QemuOpts with the same ID is
>   already stored).  It also screws up when a value contains ",id=".
> 
> I intend to grow this into a saner replacement for QemuOpts.  It'll
> take time, though.
> 
> TODO Support lists
> 
> TODO Function comment is missing.

Hence still being RFC, and I won't give R-b, but will still review.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

>  
> +QDict *keyval_parse(const char *params, const char *implied_key,
> +                    Error **errp);
> +

> +++ b/tests/test-keyval.c

> +
> +static void test_keyval_parse(void)
> +{
> +    Error *err = NULL;
> +    QDict *qdict, *sub_qdict;
> +    char long_key[129];
> +    char *params;
> +
> +    /* Nothing */
> +    qdict = keyval_parse("", NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 0);
> +    QDECREF(qdict);
> +
> +    /* Empty key */
> +    qdict = keyval_parse("=val", NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);
> +
> +    /* Empty key component */
> +    qdict = keyval_parse(".", NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);
> +    qdict = keyval_parse("key.", NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);

Do you also want to test ".=" or "key.=" ?

> +
> +    /* Overlong key */
> +    memset(long_key, 'a', 127);
> +    long_key[127] = 'z';
> +    long_key[128] = 0;
> +    params = g_strdup_printf("k.%s=v", long_key);
> +    qdict = keyval_parse(params + 2, NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);
> +
> +    /* Overlong key component */
> +    qdict = keyval_parse(params, NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);
> +    g_free(params);
> +
> +    /* Long key */
> +    params = g_strdup_printf("k.%s=v", long_key + 1);
> +    qdict = keyval_parse(params + 2, NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, long_key + 1), ==, "v");
> +    QDECREF(qdict);
> +
> +    /* Long key component */
> +    qdict = keyval_parse(params, NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    sub_qdict = qdict_get_qdict(qdict, "k");
> +    g_assert(sub_qdict);
> +    g_assert_cmpuint(qdict_size(sub_qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(sub_qdict, long_key + 1), ==, "v");
> +    QDECREF(qdict);
> +    g_free(params);

As mentioned in the commit message, this works when QemuOpts would have
rejected it as overlong.  Good, in my opinion.

> +
> +    /* Multiple keys, last one wins */
> +    qdict = keyval_parse("a=1,b=2,,x,a=3", NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 2);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "a"), ==, "3");
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "b"), ==, "2,x");
> +    QDECREF(qdict);
> +
> +    /* Even when it doesn't in QemuOpts */
> +    qdict = keyval_parse("id=foo,id=bar", NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "id"), ==, "bar");
> +    QDECREF(qdict);
> +
> +    /* Dotted keys */
> +    qdict = keyval_parse("a.b.c=1,a.b.c=2,d=3", NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 2);
> +    sub_qdict = qdict_get_qdict(qdict, "a");
> +    g_assert(sub_qdict);
> +    g_assert_cmpuint(qdict_size(sub_qdict), ==, 1);
> +    sub_qdict = qdict_get_qdict(sub_qdict, "b");
> +    g_assert(sub_qdict);
> +    g_assert_cmpuint(qdict_size(sub_qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(sub_qdict, "c"), ==, "2");
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "d"), ==, "3");
> +    QDECREF(qdict);
> +
> +    /* Inconsistent dotted keys */
> +    qdict = keyval_parse("a.b=1,a=2", NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);
> +    qdict = keyval_parse("a.b=1,a.b.c=2", NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);

Will probably need more tests for inconsistencies when you support arrays.

> +
> +    /* Implied value */
> +    qdict = keyval_parse("an,noaus,noaus=", NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 3);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "an"), ==, "on");
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "aus"), ==, "off");
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "noaus"), ==, "");
> +    QDECREF(qdict);

Oh, so '$key' implies the same as '$key=true'; 'no$key' implies the same
as '$key=false', and the presence of = is what changes an implicit bool
(with magic 'no' prefix handling) into a full keyname.  Looks a bit
weird, but it matches what QemuOpts does so we do need to keep that
magic around.

> +
> +    /* Implied key */
> +    qdict = keyval_parse("an,noaus,noaus=", "implied", &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 3);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "an");
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "aus"), ==, "off");
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "noaus"), ==, "");
> +    QDECREF(qdict);
> +
> +    /* Trailing comma is ignored */
> +    qdict = keyval_parse("x=y,", NULL,  &error_abort);

Why two spaces here?

> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "x"), ==, "y");
> +    QDECREF(qdict);

Nice. Too bad JSON can't be as forgiving about trailing commas.

> +
> +    /* Except when it isn't */
> +    qdict = keyval_parse(",", NULL,  &err);

Again, why two spaces? (I'll quit pointing it out, if there's more)

> +    error_free_or_abort(&err);
> +    g_assert(!qdict);

Question: what happens with:

keyval_parse(",", "implied", &err)

Should that be the same as:

keyval_parse("implied=,", NULL, &err)

which in turn is the same as:

keyval_parse("implied=", NULL, &err)

Should you update "test-qemu-opts: Cover qemu_opts_parse()" to do a
QemuOpts counterpart of this question?

> +
> +    /* Value containing ,id= not misinterpreted as QemuOpts does */
> +    qdict = keyval_parse("x=,,id=bar", NULL,  &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "x"), ==, ",id=bar");
> +    QDECREF(qdict);

Yes, definitely better than QemuOpts' wart.

> +
> +    /* Anti-social ID is left to caller */
> +    qdict = keyval_parse("id=666", NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "id"), ==, "666");
> +    QDECREF(qdict);

Nice to compare this to your earlier patch on enhancing QemuOpts tests
(your reminder of my chuckle on the "anti-social ID" on that patch was
worth it).  That test had:

+    /* TODO Cover .merge_lists = true */

which matches this RFC's TODO for adding list support; it also had:

+    /* Unknown key */

which is not needed here, since this focuses on just the parsing rather
than also the mapping into recognized QemuOpt key names (the counterpart
here would be that the caller would flag extra keys by using a strict
qobject-input parse on the resulting QObject).

> +++ b/util/keyval.c
> @@ -0,0 +1,150 @@
> +/*
> + * Parsing KEY=VALUE,... strings
> + *
> + * Copyright (C) 2017 Red Hat Inc.
> + *
> + * Authors:
> + *  Markus Armbruster <armbru@redhat.com>,
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qapi/qmp/qstring.h"
> +#include "qemu/option.h"
> +
> +/* TODO Support lists */
> +
> +static QObject *keyval_parse_put(QDict *qdict, const char *key, QString *value,
> +                                 Error **errp)
> +{
> +    QObject *old, *new;
> +
> +    old = qdict_get(qdict, key);
> +    if (old) {
> +        if (qobject_type(old) != (value ? QTYPE_QSTRING : QTYPE_QDICT)) {
> +            error_setg(errp, "Option key '%s' used inconsistently", key);
> +            return NULL;
> +        }
> +        if (!value) {
> +            return old;
> +        }
> +        new = QOBJECT(value);

So if we've already seen key, it is either a subdict (and we return the
existing sub-dict to add more into it) or a string (and we replace the
old string with the new)...

> +    } else {
> +        new = QOBJECT(value) ?: QOBJECT(qdict_new());

...and if we haven't seen key, we either add the string or a new subdict...

> +    }
> +    qdict_put_obj(qdict, key, new);
> +    return new;

...either way, we are returning the current value of the key within
qdict to the caller.

Comments may indeed help, because it took me a couple of reads before I
saw what was going on, but the function looks sane.

> +}
> +
> +static const char *keyval_parse_one(QDict *qdict, const char *params,
> +                                    const char *implied_key,
> +                                    Error **errp)
> +{
> +    QDict *cur = qdict;
> +    QObject *next;
> +    const char *s, *key;
> +    size_t len;
> +    char key_buf[128];
> +    QString *val;
> +
> +    s = params;
> +    len = strcspn(s, ".=,");
> +    if (implied_key && (s[len] == ',' || !s[len])) {
> +        /* Desugar implied key */
> +        key = implied_key;

Should keyval_parse("=foo", "implied", &err) also behave the same as
keyval_parse("implied==foo", NULL, &err) (resulting in "=foo" as the value)?

> +    } else {
> +        key_buf[0] = 0;
> +        for (;;) {
> +            if (!len) {
> +                error_setg(errp, "Invalid option key");
> +                return NULL;
> +            }
> +            if (len >= sizeof(key_buf)) {
> +                error_setg(errp, "Option key component '%.*s' is too long",
> +                           (int)len, s);
> +                return NULL;
> +            }
> +
> +            if (key_buf[0]) {
> +                next = keyval_parse_put(cur, key_buf, NULL, errp);
> +                if (!next) {
> +                    return NULL;
> +                }
> +                cur = qobject_to_qdict(next);
> +                assert(cur);
> +            }
> +
> +            memcpy(key_buf, s, len);
> +            key_buf[len] = 0;
> +            s += len;
> +            if (*s != '.') {
> +                break;
> +            }
> +            s++;
> +            len = strcspn(s, ".=,");
> +        }
> +        key = key_buf;
> +
> +        if (*s == '=') {
> +            s++;

I'm not sure this condition is correct, if there is an implied key where
we want the value string to start with "=".  Or are we requiring that
starting a value with = requires that we can't use implicit key?
Missing testsuite coverage, I think?

> +        } else {
> +            /*
> +             * Desugar implied value: it's "on", except when @key
> +             * starts with "no", it's "off".  Thus, key "novocaine"
> +             * gets desugard to "vocaine=off", not to "novocaine=on".
> +             * If sugar isn't bad enough for you, make it ambiguous...

So if I get this right,

keyval_parse(",", "implied", &err) => "implied" = "on"
keyval_parse(",", "noimplied", &err) => "implied" = "off"
keyval_parse("on", "noimplied", &err) => "noimplied" = "on"

Eww. Not necessarily your fault.  But maybe this is our chance to tweak
it to be slightly different?

> +             */
> +            if (*s == ',')
> +                s++;

Should this account for leading ",," with implied key (meaning a value
that starts with a comma)?  Or is that another thing where a leading
comma in a value cannot be mixed with an implied key?

> +            if (!strncmp(key, "no", 2)) {
> +                key += 2;
> +                val = qstring_from_str("off");
> +            } else {
> +                val = qstring_from_str("on");
> +            }
> +            goto got_val;
> +        }
> +    }
> +
> +    val = qstring_new();
> +    for (;;) {
> +        if (!*s) {
> +            break;
> +        } else if (*s == ',') {
> +            s++;
> +            if (*s != ',') {
> +                break;
> +            }
> +        }
> +        qstring_append_chr(val, *s++);
> +    }
> +
> +got_val:
> +    if (!keyval_parse_put(cur, key, val, errp)) {
> +        return NULL;
> +    }
> +    return s;
> +}

You managed to make this fairly compact for how hairy it is!  I don't
know how lists will impact it (well, maybe you'll have integral keys on
this pass, and then a second pass that turns integral keys into list
members...)

> +
> +/* TODO function comment */
> +QDict *keyval_parse(const char *params, const char *implied_key,
> +                    Error **errp)
> +{
> +    QDict *qdict = qdict_new();
> +    const char *s;

Should we do any sort of validation on implied_key, such as making sure
it is non-NULL and non-empty, does not contain ".=,", looks like a
well-formed id?  Or is that merely documentation that a caller that
passes a garbage implied_key gets a garbage QDict result?

> +
> +    s = params;
> +    while (*s) {
> +        s = keyval_parse_one(qdict, s, implied_key, errp);
> +        if (!s) {
> +            QDECREF(qdict);
> +            return NULL;
> +        }
> +        implied_key = NULL;
> +    }
> +
> +    return qdict;
> +}
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org