[Qemu-devel] [PATCH RFC v3 0/5] block: Crude initial implementation of -blockdev

[Qemu-devel] [PATCH RFC v3 0/5] block: Crude initial implementation of -blockdev

[Markus Armbruster]

Still missing: list support, better error messages, general polish

This is based on
* [PATCH v2 00/24] QemuOpts util/cutils: Fix and clean up number
  conversions"
* [PATCH 00/14] qobject: Cleanups, mostly in tests
* [PATCH 0/2] qapi: Improve qobject input visitor error reporting

Branch blockdev-cmdline in my public repo
http://repo.or.cz/w/qemu/armbru.git

v3: Support non-string types with KEY=VALUE
v2: Support KEY=VALUE,... syntax as well.

Daniel P. Berrange (1):
  qapi: Permit scalar type conversions in QObjectInputVisitor

Markus Armbruster (4):
  tests: Fix gcov-files-test-qemu-opts-y, gcov-files-test-logging-y
  keyval: New keyval_parse()
  qapi: Factor qobject_input_get_autocast() out of methods
  block: Crude initial implementation of -blockdev

 include/qapi/qobject-input-visitor.h |  32 ++-
 include/qemu/option.h                |   3 +
 qapi/qobject-input-visitor.c         | 148 +++++++++++++
 qemu-options.hx                      |   3 +
 tests/.gitignore                     |   1 +
 tests/Makefile.include               |   7 +-
 tests/test-keyval.c                  | 395 +++++++++++++++++++++++++++++++++++
 tests/test-qobject-input-visitor.c   | 194 ++++++++++++++++-
 util/Makefile.objs                   |   1 +
 util/keyval.c                        | 150 +++++++++++++
 vl.c                                 |  50 +++++
 11 files changed, 974 insertions(+), 10 deletions(-)
 create mode 100644 tests/test-keyval.c
 create mode 100644 util/keyval.c

-- 
2.7.4

[Qemu-devel] [PATCH RFC v3 1/5] tests: Fix gcov-files-test-qemu-opts-y, gcov-files-test-logging-y

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/Makefile.include | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index e60bb6c..5591f60 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -94,7 +94,7 @@ gcov-files-check-qom-interface-y = qom/object.c
 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 = qom/test-qemu-opts.c
+gcov-files-test-qemu-opts-y = util/qemu-option.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)
@@ -119,8 +119,8 @@ check-unit-y += tests/test-crypto-ivgen$(EXESUF)
 check-unit-y += tests/test-crypto-afsplit$(EXESUF)
 check-unit-y += tests/test-crypto-xts$(EXESUF)
 check-unit-y += tests/test-crypto-block$(EXESUF)
-gcov-files-test-logging-y = tests/test-logging.c
 check-unit-y += tests/test-logging$(EXESUF)
+gcov-files-test-logging-y = util/log.c
 check-unit-$(CONFIG_REPLICATION) += tests/test-replication$(EXESUF)
 check-unit-y += tests/test-bufferiszero$(EXESUF)
 gcov-files-check-bufferiszero-y = util/bufferiszero.c
-- 
2.7.4

Re: [Qemu-devel] [PATCH RFC v3 1/5] tests: Fix gcov-files-test-qemu-opts-y, gcov-files-test-logging-y

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]

On 02/21/2017 03:01 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/Makefile.include | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index e60bb6c..5591f60 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -94,7 +94,7 @@ gcov-files-check-qom-interface-y = qom/object.c
>  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 = qom/test-qemu-opts.c
> +gcov-files-test-qemu-opts-y = util/qemu-option.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)
> @@ -119,8 +119,8 @@ check-unit-y += tests/test-crypto-ivgen$(EXESUF)
>  check-unit-y += tests/test-crypto-afsplit$(EXESUF)
>  check-unit-y += tests/test-crypto-xts$(EXESUF)
>  check-unit-y += tests/test-crypto-block$(EXESUF)
> -gcov-files-test-logging-y = tests/test-logging.c
>  check-unit-y += tests/test-logging$(EXESUF)
> +gcov-files-test-logging-y = util/log.c
>  check-unit-$(CONFIG_REPLICATION) += tests/test-replication$(EXESUF)
>  check-unit-y += tests/test-bufferiszero$(EXESUF)
>  gcov-files-check-bufferiszero-y = util/bufferiszero.c
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH RFC v3 2/5] keyval: New keyval_parse()

[Markus Armbruster]

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

Re: [Qemu-devel] [PATCH RFC v3 2/5] keyval: New keyval_parse()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 14795 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC v3 2/5] keyval: New keyval_parse()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> 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.=" ?

For ".=", keyval_parse() balks at '.'  and doesn't look any further.
Same for any string starting with '.', '=', or ','.

For "key.=", it takes the exact same path as for "key.".  Same for
"key.." and "key.,".

My point is: coverage is fine as is.  Additional test cases aren't
exactly expensive, or course.  Want some?

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

QemuOpts is even worse: it silently truncates.

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

One step at a time.  I might even do arrays as a separate patch to ease
review.

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

I hate the 'no$key' sugar, and would love to get rid of it.  It makes
things ambiguous (thus confusing) for precious little gain: 'novocaine'
can mean 'novocaine=on' or 'vocaine=off'.  QemuOpts picks the latter,
even when a QemuOpt named 'novocaine' has been declared.

keyval_parse() is meant to behave like QemuOpts, so I'm reproducing this
bit of bad sugar, reluctantly.

>> +
>> +    /* 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?

Editing accident, will fix.

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

I'll search for it.

Intentional deviation from QemuOpts, by the way: QemuOpts interprets ","
as key "" with value "on".  I find that too ridiculous to copy.  Hmm,
should mention it in the commit message.

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

That's how it behaves now.  Matches QemuOpts.

I don't like it, because keyval_parse(",", NULL, &err) fails.  QemuOpts
doesn't (see above).  Hmm.

I'll add a test case.

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

Yes, the two tests should be kept in sync.

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

"Fortunately", that wart isn't just ugly, but also buggy, which I feel
gives me license not reproduce it in keyval_parse().

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

Not really.

Normally, each qemu_opts_parse() creates a new QemuOpts.  Fails when a
QemuOpts with the same ID already exists.

With .mergelists = true, don't fail, but reuse the existing QemuOpts,
i.e. append the new settings.  Do this even without an ID, i.e. you can
have at most one QemuOpts without an ID then.

keyval_parse() doesn't attempt to support this part of QemuOpts for now.

Lists in the sense of this RFC's TODO are something else, namely list
values.  QemuOpts sort of supports them via repeated keys.
Implementation accident that got pressed into service.

keyval_parse() doesn't attempt to support that part of QemuOpts for now.

However, test-qemu-opts.c needs a TODO Cover repeated keys.

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

Exactly.

keyval_parse() is like QemuOpts with an empty .desc[] (any key goes).
Making sense of the keys and parsing (string) values according to their
key is left for later.

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

I'll try to make it easier to understand.

>> +}
>> +
>> +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)?

QemuOpts interprets this as empty key with value "foo".

keyval_parse() does the same, but rejects empty keys.

I'll add a test case to both.

>> +    } 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?

The loop eats a key, which extends to the first '=', ',' or '\0'.  Empty
keys are rejected.

Note that QemuOpts treats double comma specially only in *values*, not
in keys.  So does keyval_parse().  Therefore, stopping at the first ','
is correct.

When we get to the conditional, @s points to a '=', ',' or '\0' behind a
non-empty key.  Two cases: it points to "=VALUE" (condition is true), or
no value follows (condition is false).

> Missing testsuite coverage, I think?

What test cases do you have in mind beyond "=foo"?

>> +        } 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,

No, the QemuOpts sugar works differently:

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

No, it's "implied" = ""

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

"noimplied" = ""

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

Yes.

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

QemuOpts is a diabetic.

Hard rule for the initial version of keyval_parse(): no new sugar.

Soft rule: all the old sugar.

"Soft" means we can consider dropping really bad / confusing / useless
old sugar.

>> +             */
>> +            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?

You're driving me nuts :)  Or actually, QemuOpts is driving us both
nuts.

Remember that double comma is only special in values.  We're looking at
a key here.  Implied key never gets here, it runs the other arm of the
outermost conditional.

I'll add a test case for ",,,a=1".  Quick quiz, what does it mean?
Solution at [*].

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

Thanks!

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

Wait and see :)

>> +
>> +/* 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?

I absolutely intend to restrict keys according to QAPI naming rules, as
discussed in "Non-flat command line option argument syntax".

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

Thanks a lot for your review, it was timely and useful!


[*] Quick quiz solution:

    "implied" = ","
    "a"       = "1"

Re: [Qemu-devel] [PATCH RFC v3 2/5] keyval: New keyval_parse()

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Eric Blake <eblake@redhat.com> writes:
>
>> On 02/21/2017 03:01 PM, Markus Armbruster wrote:
[...]
>>> +    /* 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.
>
> I hate the 'no$key' sugar, and would love to get rid of it.  It makes
> things ambiguous (thus confusing) for precious little gain: 'novocaine'
> can mean 'novocaine=on' or 'vocaine=off'.  QemuOpts picks the latter,
> even when a QemuOpt named 'novocaine' has been declared.
>
> keyval_parse() is meant to behave like QemuOpts, so I'm reproducing this
> bit of bad sugar, reluctantly.

Implied value alternatives / variations:

(1) Don't implement the "no" sugar

    I think we'd have to deprecate it in QemuOpts now, to give users
    some time to adapt before keyval_parse() replaces QemuOpts.

(2) Restrict to boolean

    Have keyval_parse() store omitted value as QBool true/false instead
    of QString "on"/"off".  Make visit_type_bool() accept both.  Reject
    QBool everywhere else.

    This breaks abuse like nofilename=.

    It also breaks more legitimate use of implied values with
    enumeration types that happen to have values "on" or "off".
    Fixable: make visit_type_enum() do the right thing.  Requires
    bringing Visitor method type_enum() back.

(3) More magic

    The basic idea is to let the visitor figure out the implied value.

    First try: store "KEY" with value none.  When visit_type_bool() sees
    its key mapping to none, it picks true.  If it sees its key
    unmapped, it prepends "no", and if that's mapped to none, it picks
    false.  Add the obvious type errors.

    Sadly, this doesn't work, because it breaks wait,nowait:
    visit_type_bool() sees "wait" mapped to none and picks true, even
    though the later nowait should override it to false.  Also checking
    "no" doesn't help, because it can't distinguish nowait,wait from
    wait,nowait.

    Second try, keyval_parse() part:
    - If we have "noKEY": if "KEY" is already mapped to a boolean, set
      it to false, else set "noKEY" to true.
    - Else we have "KEY": if "noKEY" is already mapped to a boolean,
      delete it.  Then set "KEY" to true.
    This ensures that "noKEY" can map to boolean only when "KEY" does
    not.

    Visitor part: try "KEY" (accept both boolean and string), and if it
    doesn't exist, fall back to "noKEY" (accept only boolean, reverse
    sense).

    I'm not sure this magic is airtight as is.  Needs a good think.

If we're happy with bad sugar like "can't abbreviate novocaine=on to
novocaine" (confusing), "filename without a value gets you the file
'on'", we reimplement the bad sugar and move on.

If not, we have the choice between less magic or more magic.

Less magic: (1) and/or (2).

More magic: (3) or something like it.

If we don't want to decide right now, we can do

(4) Don't implement any implied value sugar for now

    You have to spell out =on and =off.  Big fat comment to remind us
    about the QemuOpts incompatibility.

I think this would be acceptable for -blockdev in 2.9.

Opinions?

[...]

Re: [Qemu-devel] [PATCH RFC v3 2/5] keyval: New keyval_parse()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3102 bytes --]

On 02/24/2017 01:58 AM, Markus Armbruster wrote:

>> I hate the 'no$key' sugar, and would love to get rid of it.  It makes
>> things ambiguous (thus confusing) for precious little gain: 'novocaine'
>> can mean 'novocaine=on' or 'vocaine=off'.  QemuOpts picks the latter,
>> even when a QemuOpt named 'novocaine' has been declared.
>>
>> keyval_parse() is meant to behave like QemuOpts, so I'm reproducing this
>> bit of bad sugar, reluctantly.
> 
> Implied value alternatives / variations:
> 
> (1) Don't implement the "no" sugar
> 
>     I think we'd have to deprecate it in QemuOpts now, to give users
>     some time to adapt before keyval_parse() replaces QemuOpts.
> 

> 
> If we're happy with bad sugar like "can't abbreviate novocaine=on to
> novocaine" (confusing), "filename without a value gets you the file
> 'on'", we reimplement the bad sugar and move on.
> 
> If not, we have the choice between less magic or more magic.
> 
> Less magic: (1) and/or (2).
> 
> More magic: (3) or something like it.
> 
> If we don't want to decide right now, we can do
> 
> (4) Don't implement any implied value sugar for now
> 
>     You have to spell out =on and =off.  Big fat comment to remind us
>     about the QemuOpts incompatibility.
> 
> I think this would be acceptable for -blockdev in 2.9.
> 
> Opinions?

Do any existing blockdev sub-parameters begin with 'no'?  Ouch:
BlockdevOptions has '*cache':'BlockdevCacheOptions' which in turn has
'*no-flush':'bool'.  Even worse, I think you could abuse 'nono-flush' to
enable flushing - my head hurts.

And of course, there's the obvious BlockDevOptions '*node-name':'str'.
No one in their right mind would expect 'node-name' without parameters
to result in 'de-name=no'.

Also, I found that at least libvirt has existing command line usage
-numa node,nodeid=0, where qemu_numa_opts uses .implied_opt_name="type",
.desc = {{0}} to be visited by OptsVisitor.  Because of the implied opt,
we get the desired "type=node" rather than the accidental "de=no", but
the magic of implied keys can certainly make it weird to think about.

How about:

for 2.9: -blockdev has no magic at all (you HAVE to spell 'foo=off'
rather than 'foo' for any boolean parameters that you want disabled in
-blockdev); and in QemuOpts, we issue deprecation warnings but keep
existing behavior any time someone uses 'noFOO' that we turn into
'FOO=no'.  Then, for 2.10, we can decide to remove the deprecation
warnings if they pointed out real (ab)use of the sugar in the wild, or
(hopefully) to kill the sugar entirely (as part of converting QemuOpts
over to keyval_parse).

In short, getting rid of the 'no' prefix magic, after suitable
deprecation warnings, seems like a good plan to me; and having -blockdev
be slightly tighter than the rest of command line options for 2.9 only
is no real loss (no one uses -blockdev yet, so they can be written to
avoid the use of 'no' magic to begin with).

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC v3 2/5] keyval: New keyval_parse()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/24/2017 01:58 AM, Markus Armbruster wrote:
>
>>> I hate the 'no$key' sugar, and would love to get rid of it.  It makes
>>> things ambiguous (thus confusing) for precious little gain: 'novocaine'
>>> can mean 'novocaine=on' or 'vocaine=off'.  QemuOpts picks the latter,
>>> even when a QemuOpt named 'novocaine' has been declared.
>>>
>>> keyval_parse() is meant to behave like QemuOpts, so I'm reproducing this
>>> bit of bad sugar, reluctantly.
>> 
>> Implied value alternatives / variations:
>> 
>> (1) Don't implement the "no" sugar
>> 
>>     I think we'd have to deprecate it in QemuOpts now, to give users
>>     some time to adapt before keyval_parse() replaces QemuOpts.
>> 
>
>> 
>> If we're happy with bad sugar like "can't abbreviate novocaine=on to
>> novocaine" (confusing), "filename without a value gets you the file
>> 'on'", we reimplement the bad sugar and move on.
>> 
>> If not, we have the choice between less magic or more magic.
>> 
>> Less magic: (1) and/or (2).
>> 
>> More magic: (3) or something like it.
>> 
>> If we don't want to decide right now, we can do
>> 
>> (4) Don't implement any implied value sugar for now
>> 
>>     You have to spell out =on and =off.  Big fat comment to remind us
>>     about the QemuOpts incompatibility.
>> 
>> I think this would be acceptable for -blockdev in 2.9.
>> 
>> Opinions?
>
> Do any existing blockdev sub-parameters begin with 'no'?  Ouch:
> BlockdevOptions has '*cache':'BlockdevCacheOptions' which in turn has
> '*no-flush':'bool'.

I hate negative flags.

>                      Even worse, I think you could abuse 'nono-flush' to
> enable flushing - my head hurts.

"no-flush" gets desugared to "-flush=off", which doesn't work.

"nono-flush" gets desugared to "no-flush=off", which works.

> And of course, there's the obvious BlockDevOptions '*node-name':'str'.
> No one in their right mind would expect 'node-name' without parameters
> to result in 'de-name=no'.

This is something I'd be even willing to break for existing options.

> Also, I found that at least libvirt has existing command line usage
> -numa node,nodeid=0, where qemu_numa_opts uses .implied_opt_name="type",
> .desc = {{0}} to be visited by OptsVisitor.  Because of the implied opt,
> we get the desired "type=node" rather than the accidental "de=no", but
> the magic of implied keys can certainly make it weird to think about.

Correct.

The value of an implied key is not subject to key desugaring.

Without an implied key, the "node" would desugar to "de=off".

> How about:
>
> for 2.9: -blockdev has no magic at all (you HAVE to spell 'foo=off'
> rather than 'foo' for any boolean parameters that you want disabled in
> -blockdev);

You mean "rather than 'nofoo'".

"No magic at all" implies you also have to spell "foo=on" rather than
"foo".

I believe we need to have both or none in 2.9.  If we have only the "on"
sugar, "no-flush" works, and we can't add the "off" sugar without
breaking it.

> -blockdev); and in QemuOpts, we issue deprecation warnings but keep
> existing behavior any time someone uses 'noFOO' that we turn into
> 'FOO=no'.  Then, for 2.10, we can decide to remove the deprecation
> warnings if they pointed out real (ab)use of the sugar in the wild, or
> (hopefully) to kill the sugar entirely (as part of converting QemuOpts
> over to keyval_parse).
>
> In short, getting rid of the 'no' prefix magic, after suitable
> deprecation warnings, seems like a good plan to me; and having -blockdev
> be slightly tighter than the rest of command line options for 2.9 only
> is no real loss (no one uses -blockdev yet, so they can be written to
> avoid the use of 'no' magic to begin with).

Okay, I'll post QemuOpts patch to deprecate noKEY sugar.  Let's see
whether anyone screams.

Re: [Qemu-devel] [PATCH RFC v3 2/5] keyval: New keyval_parse()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3763 bytes --]

On 02/24/2017 01:06 PM, Markus Armbruster wrote:

> 
> The value of an implied key is not subject to key desugaring.

And that's a good thing.

> 
> Without an implied key, the "node" would desugar to "de=off".
> 
>> How about:
>>
>> for 2.9: -blockdev has no magic at all (you HAVE to spell 'foo=off'
>> rather than 'foo' for any boolean parameters that you want disabled in
>> -blockdev);
> 
> You mean "rather than 'nofoo'".

Yes.  Thanks for catching the intent.

> 
> "No magic at all" implies you also have to spell "foo=on" rather than
> "foo".

I'm a little more lenient for allowing 'foo' meaning 'foo=on' as a
shortcut (although making it force a QBool rather than a string "on" may
be nicer).  I'm also okay if -blockdev has no sugar at all in 2.9 (we
can add "foo" => "foo=on" sugar later if it makes QemuOpts conversion
easier in 2.10; that's much more palatable than adding "nofoo" =>
"foo=off" sugar).

> 
> I believe we need to have both or none in 2.9.  If we have only the "on"
> sugar, "no-flush" works, and we can't add the "off" sugar without
> breaking it.

I think the "off" sugar should die, if we can convince ourselves that no
one is using it that can't catch up to modern usage.

> 
>> -blockdev); and in QemuOpts, we issue deprecation warnings but keep
>> existing behavior any time someone uses 'noFOO' that we turn into
>> 'FOO=no'.  Then, for 2.10, we can decide to remove the deprecation
>> warnings if they pointed out real (ab)use of the sugar in the wild, or
>> (hopefully) to kill the sugar entirely (as part of converting QemuOpts
>> over to keyval_parse).
>>
>> In short, getting rid of the 'no' prefix magic, after suitable
>> deprecation warnings, seems like a good plan to me; and having -blockdev
>> be slightly tighter than the rest of command line options for 2.9 only
>> is no real loss (no one uses -blockdev yet, so they can be written to
>> avoid the use of 'no' magic to begin with).
> 
> Okay, I'll post QemuOpts patch to deprecate noKEY sugar.  Let's see
> whether anyone screams.

Summarizing to make sure we're on the same track:

2.9:
QemuOpts:
  "noFOO" => "FOO=off" sugar - deprecation warning
  "FOO" => "FOO=on" sugar - silently stays the same

-blockdev option 1:
  "FOO" => error, no '=' present (we haven't decided on sugaring this yet)
  "noFOO" => error, no '=' present (no off sugar, but also avoids
"noFOO" => "noFOO=on" sugar)

-blockdev option 2:
  "FOO" => "FOO=on" - enabled boolean sugar remains compact, if it is
not ambiguous, but anything we can do to limit it to just boolean values
rather than the string "on" might be nice
  "noFOO" => "noFOO=on" - different behavior than QemuOpts, but less magic

2.10:
QemuOpts will be implemented on keyval_parse(), behaving the same as
-blockdev. If we took option 1 in 2.9, we have:
  "noFOO" => "noFOO=on" (which probably triggers an error that "noFOO"
is not an option when strict parsing is in effect) (the deprecation
period is over, so we changed command-line behavior, but only for people
that didn't pay attention to the warnings in 2.9)
  "FOO" => "FOO=on" - keep this one to minimize back-compat breakage

If we took option 2 in 2.9, we can still turn on "FOO" => "FOO=on" sugar
to get back to option 1 in 2.10

or we can stick to option 2 forevermore:
  "FOO" => error, no '=' present. No longer possible to specify booleans
without the more verbose "FOO=on"

I have a preference for option 1 in the long run, but as it seems to be
upwards compatible from option 2 for -blockdev in 2.9, I'm leaning
towards option 2 for this release.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC v3 2/5] keyval: New keyval_parse()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/24/2017 01:06 PM, Markus Armbruster wrote:
>
>> 
>> The value of an implied key is not subject to key desugaring.
>
> And that's a good thing.
>
>> 
>> Without an implied key, the "node" would desugar to "de=off".
>> 
>>> How about:
>>>
>>> for 2.9: -blockdev has no magic at all (you HAVE to spell 'foo=off'
>>> rather than 'foo' for any boolean parameters that you want disabled in
>>> -blockdev);
>> 
>> You mean "rather than 'nofoo'".
>
> Yes.  Thanks for catching the intent.
>
>> 
>> "No magic at all" implies you also have to spell "foo=on" rather than
>> "foo".
>
> I'm a little more lenient for allowing 'foo' meaning 'foo=on' as a
> shortcut (although making it force a QBool rather than a string "on" may
> be nicer).

No objection.

>             I'm also okay if -blockdev has no sugar at all in 2.9 (we
> can add "foo" => "foo=on" sugar later if it makes QemuOpts conversion
> easier in 2.10; that's much more palatable than adding "nofoo" =>
> "foo=off" sugar).
>
>> I believe we need to have both or none in 2.9.  If we have only the "on"
>> sugar, "no-flush" works, and we can't add the "off" sugar without
>> breaking it.
>
> I think the "off" sugar should die, if we can convince ourselves that no
> one is using it that can't catch up to modern usage.

Yes, but we should be careful not to create problems now in case we
can't.

>>> -blockdev); and in QemuOpts, we issue deprecation warnings but keep
>>> existing behavior any time someone uses 'noFOO' that we turn into
>>> 'FOO=no'.  Then, for 2.10, we can decide to remove the deprecation
>>> warnings if they pointed out real (ab)use of the sugar in the wild, or
>>> (hopefully) to kill the sugar entirely (as part of converting QemuOpts
>>> over to keyval_parse).
>>>
>>> In short, getting rid of the 'no' prefix magic, after suitable
>>> deprecation warnings, seems like a good plan to me; and having -blockdev
>>> be slightly tighter than the rest of command line options for 2.9 only
>>> is no real loss (no one uses -blockdev yet, so they can be written to
>>> avoid the use of 'no' magic to begin with).
>> 
>> Okay, I'll post QemuOpts patch to deprecate noKEY sugar.  Let's see
>> whether anyone screams.
>
> Summarizing to make sure we're on the same track:
>
> 2.9:
> QemuOpts:
>   "noFOO" => "FOO=off" sugar - deprecation warning
>   "FOO" => "FOO=on" sugar - silently stays the same

Yes, if we can get consensus.

> -blockdev option 1:
>   "FOO" => error, no '=' present (we haven't decided on sugaring this yet)
>   "noFOO" => error, no '=' present (no off sugar, but also avoids
> "noFOO" => "noFOO=on" sugar)

Yes.

> -blockdev option 2:
>   "FOO" => "FOO=on" - enabled boolean sugar remains compact, if it is
> not ambiguous, but anything we can do to limit it to just boolean values
> rather than the string "on" might be nice
>   "noFOO" => "noFOO=on" - different behavior than QemuOpts, but less magic

No, because that would create problems if we can't get rid of noFOO
sugar elsewhere.

> 2.10:
> QemuOpts will be implemented on keyval_parse(), behaving the same as
> -blockdev.

Not sure I can complete the job in 2.10, but the text works just as well
for a later version.

>            If we took option 1 in 2.9, we have:
>   "noFOO" => "noFOO=on" (which probably triggers an error that "noFOO"
> is not an option when strict parsing is in effect) (the deprecation
> period is over, so we changed command-line behavior, but only for people
> that didn't pay attention to the warnings in 2.9)

Yes.

>   "FOO" => "FOO=on" - keep this one to minimize back-compat breakage

Yes.

> If we took option 2 in 2.9, we can still turn on "FOO" => "FOO=on" sugar
> to get back to option 1 in 2.10
>
> or we can stick to option 2 forevermore:
>   "FOO" => error, no '=' present. No longer possible to specify booleans
> without the more verbose "FOO=on"

Now I'm getting confused.  Isn't that "-blockdev option 1"?

> I have a preference for option 1 in the long run, but as it seems to be
> upwards compatible from option 2 for -blockdev in 2.9, I'm leaning
> towards option 2 for this release.

Let me rename the options:

* "no sugar -blockdev": both "FOO" and "noFOO" are rejected.

* "positive sugar blockdev": "FOO" is desugared to "FOO=on", "noFOO" to
  "noFOO=on".

Which one do you prefer for 2.9?

Re: [Qemu-devel] [PATCH RFC v3 2/5] keyval: New keyval_parse()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1307 bytes --]

On 02/24/2017 02:18 PM, Markus Armbruster wrote:
>> I have a preference for option 1 in the long run, but as it seems to be
>> upwards compatible from option 2 for -blockdev in 2.9, I'm leaning
>> towards option 2 for this release.
> 
> Let me rename the options:
> 
> * "no sugar -blockdev": both "FOO" and "noFOO" are rejected.
> 
> * "positive sugar blockdev": "FOO" is desugared to "FOO=on", "noFOO" to
>   "noFOO=on".
> 
> Which one do you prefer for 2.9?

My current leanings:

For 2.9: keyval_parse() should have no sugar.  Both "FOO" and "noFOO"
are rejected; an '=' must be present except for an implicit key -
although if -blockdev is the only client of keyval_parse(), then it's a
tossup whether we want to use it with an implicit key.

Also for 2.9: turn on the deprecation warning for QemuOpts negative
sugar, but leave positive sugar unchanged (it seems like positive sugar
is probably more in use than negative sugar, other than chardev nowait).

That way, we still have the option of enabling positive sugar blockdev
in a later version, if we decide its useful (and after dealing with any
QemuOpts fallout), but are not locked into having to use it.


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC v3 2/5] keyval: New keyval_parse()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

On 02/24/2017 03:06 PM, Eric Blake wrote:

> 
> Also for 2.9: turn on the deprecation warning for QemuOpts negative
> sugar, but leave positive sugar unchanged (it seems like positive sugar
> is probably more in use than negative sugar, other than chardev nowait).

Capturing the gist of an IRC conversation:

Right now, it appears that '-chardev nowait' is in heavy use ('git grep
nowait' shows that qemu itself recommends it in documentation, nodelay
being another one).  Right now, QemuOpts says that '-chardev
wait,nowait' results in 'wait=off', and '-chardev nowait,wait' results
in 'wait=on' (that is, last-one-wins semantics) - but in reality, there
are probably few (if any) clients that rely on this particular semantics.

If we get rid of 'noFOO' magic, we can still teach the chardev QemuOpts
to recognize optional keys for BOTH 'wait' and 'nowait'; a user that
specifies neither gets the default behavior as always; a user that
specifies only 'wait' or 'nowait' gets the requested behavior (although
now chardev has to check both key spellings rather than one), and a user
that specifies 'wait,nowait' or 'nowait,wait' causes an error, not at
QemuOpts parse time, but in the chardev initialization code.  That is,
for any boolean option where 'noFOO' magic is currently handled by
QemuOpts, we can still mostly keep back-compat by adding a new 'noFOO'
key to the object, and teaching that object to check that 'FOO' and
'noFOO' are not both selected at once.

But whether this change to QemuOpts is the right thing to do is not 2.9
material, so let's take a breather and revisit it for 2.10 (or whenever
we try to fix QemuOpts to layer on top of keyval_parse()).


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC v3 2/5] keyval: New keyval_parse()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/24/2017 03:06 PM, Eric Blake wrote:
>
>> On 02/24/2017 02:18 PM, Markus Armbruster wrote:
>>>> I have a preference for option 1 in the long run, but as it seems to be
>>>> upwards compatible from option 2 for -blockdev in 2.9, I'm leaning
>>>> towards option 2 for this release.
>>> 
>>> Let me rename the options:
>>> 
>>> * "no sugar -blockdev": both "FOO" and "noFOO" are rejected.
>>> 
>>> * "positive sugar blockdev": "FOO" is desugared to "FOO=on", "noFOO" to
>>>   "noFOO=on".
>>> 
>>> Which one do you prefer for 2.9?
>>
>> My current leanings:
>>
>> For 2.9: keyval_parse() should have no sugar.  Both "FOO" and "noFOO"
>> are rejected; an '=' must be present except for an implicit key -
>> although if -blockdev is the only client of keyval_parse(), then it's a
>> tossup whether we want to use it with an implicit key.

I agree on not doing implicit values for 2.9.  We have some thinking to
do on "noFOO", and having keyval_parse() support just "FOO" now could
backfire, as explained upthread.

Regarding implicit keys: I think we can just as well have them.  We're
not going to get rid of existing implicit keys.  Since keyval_parse()
has the code for supporting them already, we can just as well keep it.

>> Also for 2.9: turn on the deprecation warning for QemuOpts negative
>> sugar, but leave positive sugar unchanged (it seems like positive sugar
>> is probably more in use than negative sugar, other than chardev nowait).
>
> Capturing the gist of an IRC conversation:
>
> Right now, it appears that '-chardev nowait' is in heavy use ('git grep
> nowait' shows that qemu itself recommends it in documentation, nodelay
> being another one).  Right now, QemuOpts says that '-chardev
> wait,nowait' results in 'wait=off', and '-chardev nowait,wait' results
> in 'wait=on' (that is, last-one-wins semantics) - but in reality, there
> are probably few (if any) clients that rely on this particular semantics.
>
> If we get rid of 'noFOO' magic, we can still teach the chardev QemuOpts
> to recognize optional keys for BOTH 'wait' and 'nowait'; a user that
> specifies neither gets the default behavior as always; a user that
> specifies only 'wait' or 'nowait' gets the requested behavior (although
> now chardev has to check both key spellings rather than one), and a user
> that specifies 'wait,nowait' or 'nowait,wait' causes an error, not at
> QemuOpts parse time, but in the chardev initialization code.  That is,
> for any boolean option where 'noFOO' magic is currently handled by
> QemuOpts, we can still mostly keep back-compat by adding a new 'noFOO'
> key to the object, and teaching that object to check that 'FOO' and
> 'noFOO' are not both selected at once.

Another possibility is to start with deprecating "noFOO" only when we
don't have description for "FOO".

Hmm, could just as well also deprecate it when we do have one, but it
isn't QEMU_OPT_BOOL.

> But whether this change to QemuOpts is the right thing to do is not 2.9
> material, so let's take a breather and revisit it for 2.10 (or whenever
> we try to fix QemuOpts to layer on top of keyval_parse()).

Makes sense.

>> That way, we still have the option of enabling positive sugar blockdev
>> in a later version, if we decide its useful (and after dealing with any
>> QemuOpts fallout), but are not locked into having to use it.

Exactly.

[Qemu-devel] [PATCH RFC v3 3/5] qapi: Permit scalar type conversions in QObjectInputVisitor

[Markus Armbruster]

From: "Daniel P. Berrange" <berrange@redhat.com>

Currently the QObjectInputVisitor assumes that all scalar values are
directly represented as the final types declared by the thing being
visited. i.e. it assumes an 'int' is using QInt, and a 'bool' is using
QBool, etc.  This is good when QObjectInputVisitor is fed a QObject
that came from a JSON document on the QMP monitor, as it will strictly
validate correctness.

To allow QObjectInputVisitor to be reused for visiting a QObject
originating from keyval_parse(), an alternative mode is needed where
all the scalars types are represented as QString and converted on the
fly to the final desired type.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1475246744-29302-8-git-send-email-berrange@redhat.com>

Rebased, conflicts resolved, commit message updated to refer to
keyval_parse().

Control flow in qobject_input_type_number_autocast() simplified.

Additional tests in test-qemu-opts.c to verify QemuOpts compatibility.
To make the tests pass, use qemu_strtou64() instead of
parse_uint_full().

Use qemu_strtou64() and qemu_strtosz() instead of
parse_option_number() and parse_option_size() so we have to call
qobject_input_get_name() only when actually needed.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/qobject-input-visitor.h |  32 ++++-
 qapi/qobject-input-visitor.c         | 165 ++++++++++++++++++++++++
 tests/test-keyval.c                  | 241 +++++++++++++++++++++++++++++++++++
 tests/test-qobject-input-visitor.c   | 194 +++++++++++++++++++++++++++-
 4 files changed, 624 insertions(+), 8 deletions(-)

diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h
index cde328d..5022297 100644
--- a/include/qapi/qobject-input-visitor.h
+++ b/include/qapi/qobject-input-visitor.h
@@ -19,12 +19,36 @@
 
 typedef struct QObjectInputVisitor QObjectInputVisitor;
 
-/*
- * Return a new input visitor that converts a QObject to a QAPI object.
+/**
+ * Create a new input visitor that converts @obj to a QAPI object.
  *
- * Set @strict to reject a parse that doesn't consume all keys of a
- * dictionary; otherwise excess input is ignored.
+ * Any scalar values in the @obj input data structure should be in the
+ * required type already. i.e. if visiting a bool, the value should
+ * already be a QBool instance.
+ *
+ * If @strict is set to true, then an error will be reported if any
+ * dict keys are not consumed during visitation. If @strict is false
+ * then extra dict keys are silently ignored.
+ *
+ * The returned input visitor should be released by calling
+ * visit_free() when no longer required.
  */
 Visitor *qobject_input_visitor_new(QObject *obj, bool strict);
 
+/**
+ * Create a new input visitor that converts @obj to a QAPI object.
+ *
+ * Any scalar values in the @obj input data structure should always be
+ * represented as strings. i.e. if visiting a boolean, the value should
+ * be a QString whose contents represent a valid boolean.
+ *
+ * The visitor always operates in strict mode, requiring all dict keys
+ * to be consumed during visitation. An error will be reported if this
+ * does not happen.
+ *
+ * The returned input visitor should be released by calling
+ * visit_free() when no longer required.
+ */
+Visitor *qobject_input_visitor_new_autocast(QObject *obj);
+
 #endif
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 2439f1a..0bf6849 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -20,6 +20,7 @@
 #include "qemu-common.h"
 #include "qapi/qmp/types.h"
 #include "qapi/qmp/qerror.h"
+#include "qemu/cutils.h"
 
 typedef struct StackObject {
     const char *name;        /* Name if parent is QDict */
@@ -71,6 +72,7 @@ static const char *qobject_input_get_name(QObjectInputVisitor *qiv,
             g_string_prepend(qiv->errname, name);
             g_string_prepend_c(qiv->errname, '.');
         } else {
+            /* TODO needs to be .%zu for autocast */
             snprintf(buf, sizeof(buf), "[%zu]", so->index);
             g_string_prepend(qiv->errname, buf);
         }
@@ -321,6 +323,31 @@ static void qobject_input_type_int64(Visitor *v, const char *name, int64_t *obj,
     *obj = qint_get_int(qint);
 }
 
+
+static void qobject_input_type_int64_autocast(Visitor *v, const char *name,
+                                              int64_t *obj, Error **errp)
+{
+    QObjectInputVisitor *qiv = to_qiv(v);
+    QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+    QString *qstr;
+
+    if (!qobj) {
+        return;
+    }
+    qstr = qobject_to_qstring(qobj);
+    if (!qstr) {
+        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+                   qobject_input_get_name(qiv, name), "string");
+        return;
+    }
+
+    if (qemu_strtoi64(qstring_get_str(qstr), NULL, 0, obj) < 0) {
+        /* TODO report -ERANGE more nicely */
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   qobject_input_get_name(qiv, name), "integer");
+    }
+}
+
 static void qobject_input_type_uint64(Visitor *v, const char *name,
                                       uint64_t *obj, Error **errp)
 {
@@ -342,6 +369,30 @@ static void qobject_input_type_uint64(Visitor *v, const char *name,
     *obj = qint_get_int(qint);
 }
 
+static void qobject_input_type_uint64_autocast(Visitor *v, const char *name,
+                                               uint64_t *obj, Error **errp)
+{
+    QObjectInputVisitor *qiv = to_qiv(v);
+    QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+    QString *qstr;
+
+    if (!qobj) {
+        return;
+    }
+    qstr = qobject_to_qstring(qobj);
+    if (!qstr) {
+        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+                   qobject_input_get_name(qiv, name), "string");
+        return;
+    }
+
+    if (qemu_strtou64(qstring_get_str(qstr), NULL, 0, obj) < 0) {
+        /* TODO report -ERANGE more nicely */
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   qobject_input_get_name(qiv, name), "integer");
+    }
+}
+
 static void qobject_input_type_bool(Visitor *v, const char *name, bool *obj,
                                     Error **errp)
 {
@@ -362,6 +413,35 @@ static void qobject_input_type_bool(Visitor *v, const char *name, bool *obj,
     *obj = qbool_get_bool(qbool);
 }
 
+static void qobject_input_type_bool_autocast(Visitor *v, const char *name,
+                                             bool *obj, Error **errp)
+{
+    QObjectInputVisitor *qiv = to_qiv(v);
+    QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+    QString *qstr;
+    const char *str;
+
+    if (!qobj) {
+        return;
+    }
+    qstr = qobject_to_qstring(qobj);
+    if (!qstr) {
+        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+                   qobject_input_get_name(qiv, name), "string");
+        return;
+    }
+
+    str = qstring_get_str(qstr);
+    if (!strcmp(str, "on")) {
+        *obj = true;
+    } else if (!strcmp(str, "off")) {
+        *obj = false;
+    } else {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   qobject_input_get_name(qiv, name), "'on' or 'off'");
+    }
+}
+
 static void qobject_input_type_str(Visitor *v, const char *name, char **obj,
                                    Error **errp)
 {
@@ -410,6 +490,35 @@ static void qobject_input_type_number(Visitor *v, const char *name, double *obj,
                qobject_input_get_name(qiv, name), "number");
 }
 
+static void qobject_input_type_number_autocast(Visitor *v, const char *name,
+                                               double *obj, Error **errp)
+{
+    QObjectInputVisitor *qiv = to_qiv(v);
+    QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+    QString *qstr;
+    const char *str;
+    char *endp;
+
+    if (!qobj) {
+        return;
+    }
+    qstr = qobject_to_qstring(qobj);
+    if (!qstr) {
+        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+                   qobject_input_get_name(qiv, name), "string");
+        return;
+    }
+
+    str = qstring_get_str(qstr);
+    errno = 0;
+    *obj = strtod(str, &endp);
+    if (errno || endp == str || *endp) {
+        /* TODO report -ERANGE more nicely */
+        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+                   qobject_input_get_name(qiv, name), "number");
+    }
+}
+
 static void qobject_input_type_any(Visitor *v, const char *name, QObject **obj,
                                    Error **errp)
 {
@@ -440,6 +549,30 @@ static void qobject_input_type_null(Visitor *v, const char *name, Error **errp)
     }
 }
 
+static void qobject_input_type_size_autocast(Visitor *v, const char *name,
+                                             uint64_t *obj, Error **errp)
+{
+    QObjectInputVisitor *qiv = to_qiv(v);
+    QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
+    QString *qstr;
+
+    if (!qobj) {
+        return;
+    }
+    qstr = qobject_to_qstring(qobj);
+    if (!qstr) {
+        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+                   qobject_input_get_name(qiv, name), "string");
+        return;
+    }
+
+    if (qemu_strtosz(qstring_get_str(qstr), NULL, obj) < 0) {
+        /* TODO report -ERANGE more nicely */
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   qobject_input_get_name(qiv, name), "size");
+    }
+}
+
 static void qobject_input_optional(Visitor *v, const char *name, bool *present)
 {
     QObjectInputVisitor *qiv = to_qiv(v);
@@ -502,3 +635,35 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool strict)
 
     return &v->visitor;
 }
+
+Visitor *qobject_input_visitor_new_autocast(QObject *obj)
+{
+    QObjectInputVisitor *v;
+
+    v = g_malloc0(sizeof(*v));
+
+    v->visitor.type = VISITOR_INPUT;
+    v->visitor.start_struct = qobject_input_start_struct;
+    v->visitor.check_struct = qobject_input_check_struct;
+    v->visitor.end_struct = qobject_input_pop;
+    v->visitor.start_list = qobject_input_start_list;
+    v->visitor.next_list = qobject_input_next_list;
+    v->visitor.end_list = qobject_input_pop;
+    v->visitor.start_alternate = qobject_input_start_alternate;
+    v->visitor.type_int64 = qobject_input_type_int64_autocast;
+    v->visitor.type_uint64 = qobject_input_type_uint64_autocast;
+    v->visitor.type_bool = qobject_input_type_bool_autocast;
+    v->visitor.type_str = qobject_input_type_str;
+    v->visitor.type_number = qobject_input_type_number_autocast;
+    v->visitor.type_any = qobject_input_type_any;
+    v->visitor.type_null = qobject_input_type_null;
+    v->visitor.type_size = qobject_input_type_size_autocast;
+    v->visitor.optional = qobject_input_optional;
+    v->visitor.free = qobject_input_free;
+    v->strict = true;
+
+    v->root = obj;
+    qobject_incref(obj);
+
+    return &v->visitor;
+}
diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index 91b4391..116ff2e 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -12,7 +12,9 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qobject-input-visitor.h"
 #include "qemu/option.h"
+#include "qemu/cutils.h"
 
 static void test_keyval_parse(void)
 {
@@ -145,10 +147,249 @@ static void test_keyval_parse(void)
     QDECREF(qdict);
 }
 
+static void test_parse_visit_bool(void)
+{
+    Error *err = NULL;
+    Visitor *v;
+    QDict *qdict;
+    bool b;
+
+    qdict = keyval_parse("bool1=on,bool2=off", NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_bool(v, "bool1", &b, &error_abort);
+    g_assert(b);
+    visit_type_bool(v, "bool2", &b, &error_abort);
+    g_assert(!b);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    qdict = keyval_parse("bool1=offer", NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_bool(v, "bool1", &b, &err);
+    error_free_or_abort(&err);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+}
+
+static void test_parse_visit_number(void)
+{
+    Error *err = NULL;
+    Visitor *v;
+    QDict *qdict;
+    uint64_t u;
+
+    /* Lower limit zero */
+    qdict = keyval_parse("number1=0", NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_uint64(v, "number1", &u, &error_abort);
+    g_assert_cmpuint(u, ==, 0);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Upper limit 2^64-1 */
+    qdict = keyval_parse("number1=18446744073709551615,number2=-1",
+                         NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_uint64(v, "number1", &u, &error_abort);
+    g_assert_cmphex(u, ==, UINT64_MAX);
+    visit_type_uint64(v, "number2", &u, &error_abort);
+    g_assert_cmphex(u, ==, UINT64_MAX);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Above upper limit */
+    qdict = keyval_parse("number1=18446744073709551616",
+                         NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_uint64(v, "number1", &u, &err);
+    error_free_or_abort(&err);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Below lower limit */
+    qdict = keyval_parse("number1=-18446744073709551616",
+                         NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_uint64(v, "number1", &u, &err);
+    error_free_or_abort(&err);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Hex and octal */
+    qdict = keyval_parse("number1=0x2a,number2=052",
+                         NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_uint64(v, "number1", &u, &error_abort);
+    g_assert_cmpuint(u, ==, 42);
+    visit_type_uint64(v, "number2", &u, &error_abort);
+    g_assert_cmpuint(u, ==, 42);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Trailing crap */
+    qdict = keyval_parse("number1=3.14,number2=08",
+                         NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_uint64(v, "number1", &u, &err);
+    error_free_or_abort(&err);
+    visit_type_uint64(v, "number2", &u, &err);
+    error_free_or_abort(&err);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+}
+
+static void test_parse_visit_size(void)
+{
+    Error *err = NULL;
+    Visitor *v;
+    QDict *qdict;
+    uint64_t sz;
+
+    /* Lower limit zero */
+    qdict = keyval_parse("sz1=0", NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_size(v, "sz1", &sz, &error_abort);
+    g_assert_cmpuint(sz, ==, 0);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Note: precision is 53 bits since we're parsing with strtod() */
+
+    /* Around limit of precision: 2^53-1, 2^53, 2^54 */
+    qdict = keyval_parse("sz1=9007199254740991,"
+                         "sz2=9007199254740992,"
+                         "sz3=9007199254740993",
+                         NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_size(v, "sz1", &sz, &error_abort);
+    g_assert_cmphex(sz, ==, 0x1fffffffffffff);
+    visit_type_size(v, "sz2", &sz, &error_abort);
+    g_assert_cmphex(sz, ==, 0x20000000000000);
+    visit_type_size(v, "sz3", &sz, &error_abort);
+    g_assert_cmphex(sz, ==, 0x20000000000000);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
+    qdict = keyval_parse("sz1=9223372036854774784," /* 7ffffffffffffc00 */
+                         "sz2=9223372036854775295", /* 7ffffffffffffdff */
+                         NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_size(v, "sz1", &sz, &error_abort);
+    g_assert_cmphex(sz, ==, 0x7ffffffffffffc00);
+    visit_type_size(v, "sz2", &sz, &error_abort);
+    g_assert_cmphex(sz, ==, 0x7ffffffffffffc00);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Close to actual upper limit 0xfffffffffffff800 (53 msbs set) */
+    qdict = keyval_parse("sz1=18446744073709549568," /* fffffffffffff800 */
+                         "sz2=18446744073709550591", /* fffffffffffffbff */
+                         NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_size(v, "sz1", &sz, &error_abort);
+    g_assert_cmphex(sz, ==, 0xfffffffffffff800);
+    visit_type_size(v, "sz2", &sz, &error_abort);
+    g_assert_cmphex(sz, ==, 0xfffffffffffff800);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Beyond limits */
+    qdict = keyval_parse("sz1=-1,"
+                         "sz2=18446744073709550592", /* fffffffffffffc00 */
+                         NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_size(v, "sz1", &sz, &err);
+    error_free_or_abort(&err);
+    visit_type_size(v, "sz2", &sz, &err);
+    error_free_or_abort(&err);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Suffixes */
+    qdict = keyval_parse("sz1=8b,sz2=1.5k,sz3=2M,sz4=0.1G,sz5=16777215T",
+                         NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_size(v, "sz1", &sz, &error_abort);
+    g_assert_cmpuint(sz, ==, 8);
+    visit_type_size(v, "sz2", &sz, &error_abort);
+    g_assert_cmpuint(sz, ==, 1536);
+    visit_type_size(v, "sz3", &sz, &error_abort);
+    g_assert_cmphex(sz, ==, 2 * M_BYTE);
+    visit_type_size(v, "sz4", &sz, &error_abort);
+    g_assert_cmphex(sz, ==, G_BYTE / 10);
+    visit_type_size(v, "sz5", &sz, &error_abort);
+    g_assert_cmphex(sz, ==, 16777215 * T_BYTE);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Beyond limit with suffix */
+    qdict = keyval_parse("sz1=16777216T", NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_size(v, "sz1", &sz, &err);
+    error_free_or_abort(&err);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Trailing crap */
+    qdict = keyval_parse("sz1=16E,sz2=16Gi", NULL, &error_abort);
+    v = qobject_input_visitor_new_autocast(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_size(v, "sz1", &sz, &err);
+    error_free_or_abort(&err);
+    visit_type_size(v, "sz2", &sz, &err);
+    error_free_or_abort(&err);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+}
+
 int main(int argc, char *argv[])
 {
     g_test_init(&argc, &argv, NULL);
     g_test_add_func("/keyval/keyval_parse", test_keyval_parse);
+    g_test_add_func("/keyval/parse_visit/bool", test_parse_visit_bool);
+    g_test_add_func("/keyval/parse_visit/number", test_parse_visit_number);
+    g_test_add_func("/keyval/parse_visit/size", test_parse_visit_size);
     g_test_run();
     return 0;
 }
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 945404a..95ad855 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -41,6 +41,7 @@ static void visitor_input_teardown(TestInputVisitorData *data,
    function so that the JSON string used by the tests are kept in the test
    functions (and not in main()). */
 static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data,
+                                                 bool strict, bool autocast,
                                                  const char *json_string,
                                                  va_list *ap)
 {
@@ -49,11 +50,31 @@ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data,
     data->obj = qobject_from_jsonv(json_string, ap);
     g_assert(data->obj);
 
-    data->qiv = qobject_input_visitor_new(data->obj, false);
+    if (autocast) {
+        assert(strict);
+        data->qiv = qobject_input_visitor_new_autocast(data->obj);
+    } else {
+        data->qiv = qobject_input_visitor_new(data->obj, strict);
+    }
     g_assert(data->qiv);
     return data->qiv;
 }
 
+static GCC_FMT_ATTR(4, 5)
+Visitor *visitor_input_test_init_full(TestInputVisitorData *data,
+                                      bool strict, bool autocast,
+                                      const char *json_string, ...)
+{
+    Visitor *v;
+    va_list ap;
+
+    va_start(ap, json_string);
+    v = visitor_input_test_init_internal(data, strict, autocast,
+                                         json_string, &ap);
+    va_end(ap);
+    return v;
+}
+
 static GCC_FMT_ATTR(2, 3)
 Visitor *visitor_input_test_init(TestInputVisitorData *data,
                                  const char *json_string, ...)
@@ -62,7 +83,8 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
     va_list ap;
 
     va_start(ap, json_string);
-    v = visitor_input_test_init_internal(data, json_string, &ap);
+    v = visitor_input_test_init_internal(data, true, false,
+                                         json_string, &ap);
     va_end(ap);
     return v;
 }
@@ -77,7 +99,8 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
 static Visitor *visitor_input_test_init_raw(TestInputVisitorData *data,
                                             const char *json_string)
 {
-    return visitor_input_test_init_internal(data, json_string, NULL);
+    return visitor_input_test_init_internal(data, true, false,
+                                            json_string, NULL);
 }
 
 static void test_visitor_in_int(TestInputVisitorData *data,
@@ -110,6 +133,45 @@ static void test_visitor_in_int_overflow(TestInputVisitorData *data,
     error_free_or_abort(&err);
 }
 
+static void test_visitor_in_int_autocast(TestInputVisitorData *data,
+                                         const void *unused)
+{
+    int64_t res = 0, value = -42;
+    Error *err = NULL;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, true,
+                                     "%" PRId64, value);
+    visit_type_int(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+}
+
+static void test_visitor_in_int_str_autocast(TestInputVisitorData *data,
+                                             const void *unused)
+{
+    int64_t res = 0, value = -42;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, true,
+                                     "\"-42\"");
+
+    visit_type_int(v, NULL, &res, &error_abort);
+    g_assert_cmpint(res, ==, value);
+}
+
+static void test_visitor_in_int_str_noautocast(TestInputVisitorData *data,
+                                               const void *unused)
+{
+    int64_t res = 0;
+    Visitor *v;
+    Error *err = NULL;
+
+    v = visitor_input_test_init(data, "\"-42\"");
+
+    visit_type_int(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+}
+
 static void test_visitor_in_bool(TestInputVisitorData *data,
                                  const void *unused)
 {
@@ -122,6 +184,44 @@ static void test_visitor_in_bool(TestInputVisitorData *data,
     g_assert_cmpint(res, ==, true);
 }
 
+static void test_visitor_in_bool_autocast(TestInputVisitorData *data,
+                                          const void *unused)
+{
+    bool res = false;
+    Error *err = NULL;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, true, "true");
+
+    visit_type_bool(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+}
+
+static void test_visitor_in_bool_str_autocast(TestInputVisitorData *data,
+                                              const void *unused)
+{
+    bool res = false;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, true, "\"on\"");
+
+    visit_type_bool(v, NULL, &res, &error_abort);
+    g_assert_cmpint(res, ==, true);
+}
+
+static void test_visitor_in_bool_str_noautocast(TestInputVisitorData *data,
+                                                const void *unused)
+{
+    bool res = false;
+    Visitor *v;
+    Error *err = NULL;
+
+    v = visitor_input_test_init(data, "\"true\"");
+
+    visit_type_bool(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+}
+
 static void test_visitor_in_number(TestInputVisitorData *data,
                                    const void *unused)
 {
@@ -134,6 +234,69 @@ static void test_visitor_in_number(TestInputVisitorData *data,
     g_assert_cmpfloat(res, ==, value);
 }
 
+static void test_visitor_in_number_autocast(TestInputVisitorData *data,
+                                            const void *unused)
+{
+    double res = 0, value = 3.14;
+    Error *err = NULL;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, true, "%f", value);
+
+    visit_type_number(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+}
+
+static void test_visitor_in_number_str_autocast(TestInputVisitorData *data,
+                                                const void *unused)
+{
+    double res = 0, value = 3.14;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, true, "\"3.14\"");
+
+    visit_type_number(v, NULL, &res, &error_abort);
+    g_assert_cmpfloat(res, ==, value);
+}
+
+static void test_visitor_in_number_str_noautocast(TestInputVisitorData *data,
+                                                  const void *unused)
+{
+    double res = 0;
+    Visitor *v;
+    Error *err = NULL;
+
+    v = visitor_input_test_init(data, "\"3.14\"");
+
+    visit_type_number(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+}
+
+static void test_visitor_in_size_str_autocast(TestInputVisitorData *data,
+                                              const void *unused)
+{
+    uint64_t res, value = 500 * 1024 * 1024;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, true, "\"500M\"");
+
+    visit_type_size(v, NULL, &res, &error_abort);
+    g_assert_cmpfloat(res, ==, value);
+}
+
+static void test_visitor_in_size_str_noautocast(TestInputVisitorData *data,
+                                                const void *unused)
+{
+    uint64_t res = 0;
+    Visitor *v;
+    Error *err = NULL;
+
+    v = visitor_input_test_init(data, "\"500M\"");
+
+    visit_type_size(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+}
+
 static void test_visitor_in_string(TestInputVisitorData *data,
                                    const void *unused)
 {
@@ -290,7 +453,8 @@ static void test_visitor_in_null(TestInputVisitorData *data,
      * when input is not null.
      */
 
-    v = visitor_input_test_init(data, "{ 'a': null, 'b': '' }");
+    v = visitor_input_test_init_full(data, false, false,
+                                     "{ 'a': null, 'b': '' }");
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
     visit_type_null(v, "a", &error_abort);
     visit_type_str(v, "a", &tmp, &err);
@@ -841,10 +1005,32 @@ int main(int argc, char **argv)
                            NULL, test_visitor_in_int);
     input_visitor_test_add("/visitor/input/int_overflow",
                            NULL, test_visitor_in_int_overflow);
+    input_visitor_test_add("/visitor/input/int_autocast",
+                           NULL, test_visitor_in_int_autocast);
+    input_visitor_test_add("/visitor/input/int_str_autocast",
+                           NULL, test_visitor_in_int_str_autocast);
+    input_visitor_test_add("/visitor/input/int_str_noautocast",
+                           NULL, test_visitor_in_int_str_noautocast);
     input_visitor_test_add("/visitor/input/bool",
                            NULL, test_visitor_in_bool);
+    input_visitor_test_add("/visitor/input/bool_autocast",
+                           NULL, test_visitor_in_bool_autocast);
+    input_visitor_test_add("/visitor/input/bool_str_autocast",
+                           NULL, test_visitor_in_bool_str_autocast);
+    input_visitor_test_add("/visitor/input/bool_str_noautocast",
+                           NULL, test_visitor_in_bool_str_noautocast);
     input_visitor_test_add("/visitor/input/number",
                            NULL, test_visitor_in_number);
+    input_visitor_test_add("/visitor/input/number_autocast",
+                           NULL, test_visitor_in_number_autocast);
+    input_visitor_test_add("/visitor/input/number_str_autocast",
+                           NULL, test_visitor_in_number_str_autocast);
+    input_visitor_test_add("/visitor/input/number_str_noautocast",
+                           NULL, test_visitor_in_number_str_noautocast);
+    input_visitor_test_add("/visitor/input/size_str_autocast",
+                           NULL, test_visitor_in_size_str_autocast);
+    input_visitor_test_add("/visitor/input/size_str_noautocast",
+                           NULL, test_visitor_in_size_str_noautocast);
     input_visitor_test_add("/visitor/input/string",
                            NULL, test_visitor_in_string);
     input_visitor_test_add("/visitor/input/enum",
-- 
2.7.4

Re: [Qemu-devel] [PATCH RFC v3 3/5] qapi: Permit scalar type conversions in QObjectInputVisitor

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3260 bytes --]

On 02/21/2017 03:01 PM, Markus Armbruster wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> Currently the QObjectInputVisitor assumes that all scalar values are
> directly represented as the final types declared by the thing being
> visited. i.e. it assumes an 'int' is using QInt, and a 'bool' is using
> QBool, etc.  This is good when QObjectInputVisitor is fed a QObject
> that came from a JSON document on the QMP monitor, as it will strictly
> validate correctness.
> 
> To allow QObjectInputVisitor to be reused for visiting a QObject
> originating from keyval_parse(), an alternative mode is needed where
> all the scalars types are represented as QString and converted on the
> fly to the final desired type.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> Message-Id: <1475246744-29302-8-git-send-email-berrange@redhat.com>
> 
> Rebased, conflicts resolved, commit message updated to refer to
> keyval_parse().
> 
> Control flow in qobject_input_type_number_autocast() simplified.
> 
> Additional tests in test-qemu-opts.c to verify QemuOpts compatibility.
> To make the tests pass, use qemu_strtou64() instead of
> parse_uint_full().
> 
> Use qemu_strtou64() and qemu_strtosz() instead of
> parse_option_number() and parse_option_size() so we have to call
> qobject_input_get_name() only when actually needed.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/qobject-input-visitor.h |  32 ++++-
>  qapi/qobject-input-visitor.c         | 165 ++++++++++++++++++++++++
>  tests/test-keyval.c                  | 241 +++++++++++++++++++++++++++++++++++
>  tests/test-qobject-input-visitor.c   | 194 +++++++++++++++++++++++++++-
>  4 files changed, 624 insertions(+), 8 deletions(-)
> 

> @@ -71,6 +72,7 @@ static const char *qobject_input_get_name(QObjectInputVisitor *qiv,
>              g_string_prepend(qiv->errname, name);
>              g_string_prepend_c(qiv->errname, '.');
>          } else {
> +            /* TODO needs to be .%zu for autocast */
>              snprintf(buf, sizeof(buf), "[%zu]", so->index);

Ah, the TODO means you started thinking about list integration, and
that's part of why this is still RFC.


> +static void qobject_input_type_bool_autocast(Visitor *v, const char *name,
> +                                             bool *obj, Error **errp)
> +{
> +    QObjectInputVisitor *qiv = to_qiv(v);
> +    QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
> +    QString *qstr;
> +    const char *str;
> +
> +    if (!qobj) {
> +        return;
> +    }
> +    qstr = qobject_to_qstring(qobj);
> +    if (!qstr) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
> +                   qobject_input_get_name(qiv, name), "string");
> +        return;
> +    }
> +
> +    str = qstring_get_str(qstr);
> +    if (!strcmp(str, "on")) {
> +        *obj = true;
> +    } else if (!strcmp(str, "off")) {
> +        *obj = false;

Why are we reimplementing only a subset of parse_option_bool() here?

Overall, most of the changes from Dan's original look sane.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC v3 3/5] qapi: Permit scalar type conversions in QObjectInputVisitor

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/21/2017 03:01 PM, Markus Armbruster wrote:
>> From: "Daniel P. Berrange" <berrange@redhat.com>
>> 
>> Currently the QObjectInputVisitor assumes that all scalar values are
>> directly represented as the final types declared by the thing being
>> visited. i.e. it assumes an 'int' is using QInt, and a 'bool' is using
>> QBool, etc.  This is good when QObjectInputVisitor is fed a QObject
>> that came from a JSON document on the QMP monitor, as it will strictly
>> validate correctness.
>> 
>> To allow QObjectInputVisitor to be reused for visiting a QObject
>> originating from keyval_parse(), an alternative mode is needed where
>> all the scalars types are represented as QString and converted on the
>> fly to the final desired type.
>> 
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> Message-Id: <1475246744-29302-8-git-send-email-berrange@redhat.com>
>> 
>> Rebased, conflicts resolved, commit message updated to refer to
>> keyval_parse().
>> 
>> Control flow in qobject_input_type_number_autocast() simplified.
>> 
>> Additional tests in test-qemu-opts.c to verify QemuOpts compatibility.
>> To make the tests pass, use qemu_strtou64() instead of
>> parse_uint_full().
>> 
>> Use qemu_strtou64() and qemu_strtosz() instead of
>> parse_option_number() and parse_option_size() so we have to call
>> qobject_input_get_name() only when actually needed.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/qapi/qobject-input-visitor.h |  32 ++++-
>>  qapi/qobject-input-visitor.c         | 165 ++++++++++++++++++++++++
>>  tests/test-keyval.c                  | 241 +++++++++++++++++++++++++++++++++++
>>  tests/test-qobject-input-visitor.c   | 194 +++++++++++++++++++++++++++-
>>  4 files changed, 624 insertions(+), 8 deletions(-)
>> 
>
>> @@ -71,6 +72,7 @@ static const char *qobject_input_get_name(QObjectInputVisitor *qiv,
>>              g_string_prepend(qiv->errname, name);
>>              g_string_prepend_c(qiv->errname, '.');
>>          } else {
>> +            /* TODO needs to be .%zu for autocast */
>>              snprintf(buf, sizeof(buf), "[%zu]", so->index);
>
> Ah, the TODO means you started thinking about list integration, and
> that's part of why this is still RFC.
>
>
>> +static void qobject_input_type_bool_autocast(Visitor *v, const char *name,
>> +                                             bool *obj, Error **errp)
>> +{
>> +    QObjectInputVisitor *qiv = to_qiv(v);
>> +    QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
>> +    QString *qstr;
>> +    const char *str;
>> +
>> +    if (!qobj) {
>> +        return;
>> +    }
>> +    qstr = qobject_to_qstring(qobj);
>> +    if (!qstr) {
>> +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
>> +                   qobject_input_get_name(qiv, name), "string");
>> +        return;
>> +    }
>> +
>> +    str = qstring_get_str(qstr);
>> +    if (!strcmp(str, "on")) {
>> +        *obj = true;
>> +    } else if (!strcmp(str, "off")) {
>> +        *obj = false;
>
> Why are we reimplementing only a subset of parse_option_bool() here?

We're reimplementing parse_option_bool() exactly:

    static void parse_option_bool(const char *name, const char *value, bool *ret,
                                  Error **errp)
    {
        if (!strcmp(value, "on")) {
            *ret = 1;
        } else if (!strcmp(value, "off")) {
            *ret = 0;
        } else {
            error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                       name, "'on' or 'off'");
        }
    }

You're probably thinking of opts_type_bool(), which recognizes
additional spellings.  But options visitor compatibility is a headache
left for later.

> Overall, most of the changes from Dan's original look sane.

Thanks!

[Qemu-devel] [PATCH RFC v3 4/5] qapi: Factor qobject_input_get_autocast() out of methods

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/qobject-input-visitor.c | 87 ++++++++++++++++++--------------------------
 1 file changed, 35 insertions(+), 52 deletions(-)

diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 0bf6849..970bb37 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -143,6 +143,28 @@ static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
     return obj;
 }
 
+static const char *qobject_input_get_autocast(QObjectInputVisitor *qiv,
+                                              const char *name,
+                                              Error **errp)
+{
+    QObject *qobj;
+    QString *qstr;
+
+    qobj = qobject_input_get_object(qiv, name, true, errp);
+    if (!qobj) {
+        return NULL;
+    }
+
+    qstr = qobject_to_qstring(qobj);
+    if (!qstr) {
+        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
+                   qobject_input_get_name(qiv, name), "string");
+        return NULL;
+    }
+
+    return qstring_get_str(qstr);
+}
+
 static void qdict_add_key(const char *key, QObject *obj, void *opaque)
 {
     GHashTable *h = opaque;
@@ -328,20 +350,13 @@ static void qobject_input_type_int64_autocast(Visitor *v, const char *name,
                                               int64_t *obj, Error **errp)
 {
     QObjectInputVisitor *qiv = to_qiv(v);
-    QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
-    QString *qstr;
+    const char *str = qobject_input_get_autocast(qiv, name, errp);
 
-    if (!qobj) {
-        return;
-    }
-    qstr = qobject_to_qstring(qobj);
-    if (!qstr) {
-        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-                   qobject_input_get_name(qiv, name), "string");
+    if (!str) {
         return;
     }
 
-    if (qemu_strtoi64(qstring_get_str(qstr), NULL, 0, obj) < 0) {
+    if (qemu_strtoi64(str, NULL, 0, obj) < 0) {
         /* TODO report -ERANGE more nicely */
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    qobject_input_get_name(qiv, name), "integer");
@@ -373,20 +388,13 @@ static void qobject_input_type_uint64_autocast(Visitor *v, const char *name,
                                                uint64_t *obj, Error **errp)
 {
     QObjectInputVisitor *qiv = to_qiv(v);
-    QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
-    QString *qstr;
+    const char *str = qobject_input_get_autocast(qiv, name, errp);
 
-    if (!qobj) {
-        return;
-    }
-    qstr = qobject_to_qstring(qobj);
-    if (!qstr) {
-        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-                   qobject_input_get_name(qiv, name), "string");
+    if (!str) {
         return;
     }
 
-    if (qemu_strtou64(qstring_get_str(qstr), NULL, 0, obj) < 0) {
+    if (qemu_strtou64(str, NULL, 0, obj) < 0) {
         /* TODO report -ERANGE more nicely */
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    qobject_input_get_name(qiv, name), "integer");
@@ -417,21 +425,12 @@ static void qobject_input_type_bool_autocast(Visitor *v, const char *name,
                                              bool *obj, Error **errp)
 {
     QObjectInputVisitor *qiv = to_qiv(v);
-    QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
-    QString *qstr;
-    const char *str;
+    const char *str = qobject_input_get_autocast(qiv, name, errp);
 
-    if (!qobj) {
-        return;
-    }
-    qstr = qobject_to_qstring(qobj);
-    if (!qstr) {
-        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-                   qobject_input_get_name(qiv, name), "string");
+    if (!str) {
         return;
     }
 
-    str = qstring_get_str(qstr);
     if (!strcmp(str, "on")) {
         *obj = true;
     } else if (!strcmp(str, "off")) {
@@ -494,22 +493,13 @@ static void qobject_input_type_number_autocast(Visitor *v, const char *name,
                                                double *obj, Error **errp)
 {
     QObjectInputVisitor *qiv = to_qiv(v);
-    QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
-    QString *qstr;
-    const char *str;
+    const char *str = qobject_input_get_autocast(qiv, name, errp);
     char *endp;
 
-    if (!qobj) {
-        return;
-    }
-    qstr = qobject_to_qstring(qobj);
-    if (!qstr) {
-        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-                   qobject_input_get_name(qiv, name), "string");
+    if (!str) {
         return;
     }
 
-    str = qstring_get_str(qstr);
     errno = 0;
     *obj = strtod(str, &endp);
     if (errno || endp == str || *endp) {
@@ -553,20 +543,13 @@ static void qobject_input_type_size_autocast(Visitor *v, const char *name,
                                              uint64_t *obj, Error **errp)
 {
     QObjectInputVisitor *qiv = to_qiv(v);
-    QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
-    QString *qstr;
+    const char *str = qobject_input_get_autocast(qiv, name, errp);
 
-    if (!qobj) {
-        return;
-    }
-    qstr = qobject_to_qstring(qobj);
-    if (!qstr) {
-        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-                   qobject_input_get_name(qiv, name), "string");
+    if (!str) {
         return;
     }
 
-    if (qemu_strtosz(qstring_get_str(qstr), NULL, obj) < 0) {
+    if (qemu_strtosz(str, NULL, obj) < 0) {
         /* TODO report -ERANGE more nicely */
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    qobject_input_get_name(qiv, name), "size");
-- 
2.7.4

Re: [Qemu-devel] [PATCH RFC v3 4/5] qapi: Factor qobject_input_get_autocast() out of methods

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 509 bytes --]

On 02/21/2017 03:01 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi/qobject-input-visitor.c | 87 ++++++++++++++++++--------------------------
>  1 file changed, 35 insertions(+), 52 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

I guess you aren't squashing this into 3/5 because that one kept Dan as
the original author?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC v3 4/5] qapi: Factor qobject_input_get_autocast() out of methods

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/21/2017 03:01 PM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  qapi/qobject-input-visitor.c | 87 ++++++++++++++++++--------------------------
>>  1 file changed, 35 insertions(+), 52 deletions(-)
>> 
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> I guess you aren't squashing this into 3/5 because that one kept Dan as
> the original author?

Yes.

[Qemu-devel] [PATCH RFC v3 5/5] block: Crude initial implementation of -blockdev

[Markus Armbruster]

The new command line option -blockdev works like QMP command
blockdev-add.

The option argument may be given in JSON syntax, exactly as in QMP.
Example usage:

    -blockdev '{"node-name": "foo", "driver": "raw", "file": {"driver": "file", "filename": "foo.img"} }'

The JSON argument doesn't exactly blend into the existing option
syntax, so the traditional KEY=VALUE,... syntax is also supported,
using dotted keys to do the nesting:

    -blockdev node-name=foo,driver=raw,file.driver=file,file.filename=foo.img

Note that calling qmp_blockdev_add() (say via qmp_marshal_block_add())
right away would crash.  We need to stash the configuration for later
instead.  This is crudely done, and bypasses QemuOpts, even though
storing configuration is what QemuOpts is for.  Need to revamp option
infrastructure to support QAPI types like BlockdevOptions.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-options.hx |  3 +++
 vl.c            | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index 9936cf3..36a38d7 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -512,6 +512,9 @@ Use @var{file} as CD-ROM image (you cannot use @option{-hdc} and
 using @file{/dev/cdrom} as filename (@pxref{host_drives}).
 ETEXI
 
+DEF("blockdev", HAS_ARG, QEMU_OPTION_blockdev,
+    "-blockdev FIXME document\n", QEMU_OPTION_blockdev)
+
 DEF("drive", HAS_ARG, QEMU_OPTION_drive,
     "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
     "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
diff --git a/vl.c b/vl.c
index b5d0a19..37be73c 100644
--- a/vl.c
+++ b/vl.c
@@ -95,6 +95,9 @@ int main(int argc, char **argv)
 #include "migration/colo.h"
 #include "sysemu/kvm.h"
 #include "sysemu/hax.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi-visit.h"
 #include "qapi/qmp/qjson.h"
 #include "qemu/option.h"
 #include "qemu/config-file.h"
@@ -2954,6 +2957,12 @@ int main(int argc, char **argv, char **envp)
     Error *main_loop_err = NULL;
     Error *err = NULL;
     bool list_data_dirs = false;
+    typedef struct BlockdevOptions_queue {
+        BlockdevOptions *bdo;
+        Location loc;
+        QSIMPLEQ_ENTRY(BlockdevOptions_queue) entry;
+    } BlockdevOptions_queue;
+    QSIMPLEQ_HEAD(, BlockdevOptions_queue) bdo_queue = QSIMPLEQ_HEAD_INITIALIZER(bdo_queue);
 
     module_call_init(MODULE_INIT_TRACE);
 
@@ -3095,6 +3104,37 @@ int main(int argc, char **argv, char **envp)
                 drive_add(IF_DEFAULT, popt->index - QEMU_OPTION_hda, optarg,
                           HD_OPTS);
                 break;
+            case QEMU_OPTION_blockdev:
+                {
+                    BlockdevOptions_queue *bdo = g_new(BlockdevOptions_queue, 1);
+                    bool is_json = optarg[0] == '{';
+                    QObject *obj;
+                    QDict *args;
+                    Visitor *v;
+
+                    if (is_json) {
+                        obj = qobject_from_json(optarg);
+                        // TODO get error out of parser
+                        if (!obj) {
+                            error_report("invalid JSON");
+                            exit(1);
+                        }
+                        args = qobject_to_qdict(obj);
+                        assert(args);
+                        v = qobject_input_visitor_new(QOBJECT(args), true);
+                    } else {
+                        args = keyval_parse(optarg, "driver", &error_fatal);
+                        v = qobject_input_visitor_new_autocast(QOBJECT(args));
+                    }
+
+                    visit_type_BlockdevOptions(v, NULL, &bdo->bdo,
+                                               &error_fatal);
+                    visit_free(v);
+                    QDECREF(args);
+                    loc_save(&bdo->loc);
+                    QSIMPLEQ_INSERT_TAIL(&bdo_queue, bdo, entry);
+                    break;
+                }
             case QEMU_OPTION_drive:
                 if (drive_def(optarg) == NULL) {
                     exit(1);
@@ -4407,6 +4447,16 @@ int main(int argc, char **argv, char **envp)
         qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
                           NULL, NULL);
     }
+    while (!QSIMPLEQ_EMPTY(&bdo_queue)) {
+        BlockdevOptions_queue *bdo = QSIMPLEQ_FIRST(&bdo_queue);
+
+        QSIMPLEQ_REMOVE_HEAD(&bdo_queue, entry);
+        loc_push_restore(&bdo->loc);
+        qmp_blockdev_add(bdo->bdo, &error_fatal);
+        loc_pop(&bdo->loc);
+        qapi_free_BlockdevOptions(bdo->bdo);
+        g_free(bdo);
+    }
     if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
                           &machine_class->block_default_type, NULL)) {
         exit(1);
-- 
2.7.4

Re: [Qemu-devel] [PATCH RFC v3 5/5] block: Crude initial implementation of -blockdev

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3460 bytes --]

On 02/21/2017 03:01 PM, Markus Armbruster wrote:
> The new command line option -blockdev works like QMP command
> blockdev-add.
> 
> The option argument may be given in JSON syntax, exactly as in QMP.
> Example usage:
> 
>     -blockdev '{"node-name": "foo", "driver": "raw", "file": {"driver": "file", "filename": "foo.img"} }'
> 
> The JSON argument doesn't exactly blend into the existing option
> syntax, so the traditional KEY=VALUE,... syntax is also supported,
> using dotted keys to do the nesting:
> 
>     -blockdev node-name=foo,driver=raw,file.driver=file,file.filename=foo.img

Some of the list ideas were allowing mix-and-match or re-opening the
dict; do we want to allow any of these? (maybe as followups?)

-blockdev
node-name=foo,driver=raw,file='{"driver":"file"}',file.filename=foo.img


> 
> Note that calling qmp_blockdev_add() (say via qmp_marshal_block_add())
> right away would crash.  We need to stash the configuration for later
> instead.  This is crudely done, and bypasses QemuOpts, even though
> storing configuration is what QemuOpts is for.  Need to revamp option
> infrastructure to support QAPI types like BlockdevOptions.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-options.hx |  3 +++
>  vl.c            | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9936cf3..36a38d7 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -512,6 +512,9 @@ Use @var{file} as CD-ROM image (you cannot use @option{-hdc} and
>  using @file{/dev/cdrom} as filename (@pxref{host_drives}).
>  ETEXI
>  
> +DEF("blockdev", HAS_ARG, QEMU_OPTION_blockdev,
> +    "-blockdev FIXME document\n", QEMU_OPTION_blockdev)

Adequate for an RFC, but I can see how you want to improve it.

> @@ -3095,6 +3104,37 @@ int main(int argc, char **argv, char **envp)
>                  drive_add(IF_DEFAULT, popt->index - QEMU_OPTION_hda, optarg,
>                            HD_OPTS);
>                  break;
> +            case QEMU_OPTION_blockdev:
> +                {
> +                    BlockdevOptions_queue *bdo = g_new(BlockdevOptions_queue, 1);
> +                    bool is_json = optarg[0] == '{';
> +                    QObject *obj;
> +                    QDict *args;
> +                    Visitor *v;
> +
> +                    if (is_json) {
> +                        obj = qobject_from_json(optarg);
> +                        // TODO get error out of parser

Yeah, that's something we still need to improve.

> +                        if (!obj) {
> +                            error_report("invalid JSON");
> +                            exit(1);
> +                        }
> +                        args = qobject_to_qdict(obj);
> +                        assert(args);
> +                        v = qobject_input_visitor_new(QOBJECT(args), true);
> +                    } else {
> +                        args = keyval_parse(optarg, "driver", &error_fatal);
> +                        v = qobject_input_visitor_new_autocast(QOBJECT(args));
> +                    }

Pretty slick!  And answers my question: no mix-and-match of JSON and
dotted (all one or the other) within a single -blockdev argument.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC v3 5/5] block: Crude initial implementation of -blockdev

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/21/2017 03:01 PM, Markus Armbruster wrote:
>> The new command line option -blockdev works like QMP command
>> blockdev-add.
>> 
>> The option argument may be given in JSON syntax, exactly as in QMP.
>> Example usage:
>> 
>>     -blockdev '{"node-name": "foo", "driver": "raw", "file": {"driver": "file", "filename": "foo.img"} }'
>> 
>> The JSON argument doesn't exactly blend into the existing option
>> syntax, so the traditional KEY=VALUE,... syntax is also supported,
>> using dotted keys to do the nesting:
>> 
>>     -blockdev node-name=foo,driver=raw,file.driver=file,file.filename=foo.img
>
> Some of the list ideas were allowing mix-and-match or re-opening the
> dict; do we want to allow any of these? (maybe as followups?)
>
> -blockdev
> node-name=foo,driver=raw,file='{"driver":"file"}',file.filename=foo.img

No.  The initial version needs to be as simple as I can make it to give
me a chance at making 2.9.

Also, coming right from QemuOpts work, I'm in no mood for sugar.

>> Note that calling qmp_blockdev_add() (say via qmp_marshal_block_add())
>> right away would crash.  We need to stash the configuration for later
>> instead.  This is crudely done, and bypasses QemuOpts, even though
>> storing configuration is what QemuOpts is for.  Need to revamp option
>> infrastructure to support QAPI types like BlockdevOptions.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  qemu-options.hx |  3 +++
>>  vl.c            | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 53 insertions(+)
>> 
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 9936cf3..36a38d7 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -512,6 +512,9 @@ Use @var{file} as CD-ROM image (you cannot use @option{-hdc} and
>>  using @file{/dev/cdrom} as filename (@pxref{host_drives}).
>>  ETEXI
>>  
>> +DEF("blockdev", HAS_ARG, QEMU_OPTION_blockdev,
>> +    "-blockdev FIXME document\n", QEMU_OPTION_blockdev)
>
> Adequate for an RFC, but I can see how you want to improve it.

:)

>> @@ -3095,6 +3104,37 @@ int main(int argc, char **argv, char **envp)
>>                  drive_add(IF_DEFAULT, popt->index - QEMU_OPTION_hda, optarg,
>>                            HD_OPTS);
>>                  break;
>> +            case QEMU_OPTION_blockdev:
>> +                {
>> +                    BlockdevOptions_queue *bdo = g_new(BlockdevOptions_queue, 1);
>> +                    bool is_json = optarg[0] == '{';
>> +                    QObject *obj;
>> +                    QDict *args;
>> +                    Visitor *v;
>> +
>> +                    if (is_json) {
>> +                        obj = qobject_from_json(optarg);
>> +                        // TODO get error out of parser
>
> Yeah, that's something we still need to improve.

Elsewhere, too:

    req = json_parser_parse_err(tokens, NULL, &err);
    if (err || !req || qobject_type(req) != QTYPE_QDICT) {
        if (!err) {
            error_setg(&err, QERR_JSON_PARSING);
        }
        goto err_out;
    }

>> +                        if (!obj) {
>> +                            error_report("invalid JSON");
>> +                            exit(1);
>> +                        }
>> +                        args = qobject_to_qdict(obj);
>> +                        assert(args);
>> +                        v = qobject_input_visitor_new(QOBJECT(args), true);
>> +                    } else {
>> +                        args = keyval_parse(optarg, "driver", &error_fatal);
>> +                        v = qobject_input_visitor_new_autocast(QOBJECT(args));
>> +                    }
>
> Pretty slick!  And answers my question: no mix-and-match of JSON and
> dotted (all one or the other) within a single -blockdev argument.

Thanks!  I think you can see where I'm trying to head: towards a
QAPIfied command line.