[Qemu-devel] [PATCH 00/24] block: Command line option -blockdev

[Qemu-devel] [PATCH 00/24] block: Command line option -blockdev

[Markus Armbruster]

Actually, the command line option is the least part of this series.
Its bulk is about building infrastructure and getting errors out of
the JSON parser[*].  The latter part could be spun out in its own
series, if that helps.  We'll see.

The design of the command line interface was discussed here:
Subject: Non-flat command line option argument syntax
Message-ID: <87bmukmlau.fsf@dusky.pond.sub.org>
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00555.html

This series is based on "[PATCH v2 00/26] qapi: QMP dispatch and input
visitor work".  Available as branch blockdev-cmdline in my public repo
<http://repo.or.cz/w/qemu/armbru.git>, except that one still has a
patch to add implied value sugar, followed by its revert.

It supersedes "[PATCH RFC v3 0/5] block: Crude initial implementation
of -blockdev".

[*] Who'd have thunk anyone actually wants to know what's wrong with
his JSON!

Daniel P. Berrange (1):
  qapi: qobject input visitor variant for use with keyval_parse()

Markus Armbruster (23):
  test-qemu-opts: Cover qemu_opts_parse() of "no"
  tests: Fix gcov-files-test-qemu-opts-y, gcov-files-test-logging-y
  keyval: New keyval_parse()
  test-keyval: Cover use with qobject input visitor
  qapi: Factor out common part of qobject input visitor creation
  qapi: Factor out common qobject_input_get_keyval()
  qobject: Propagate parse errors through qobject_from_jsonv()
  libqtest: Fix qmp() & friends to abort on JSON parse errors
  qjson: Abort earlier on qobject_from_jsonf() misuse
  test-qobject-input-visitor: Abort earlier on bad test input
  qobject: Propagate parse errors through qobject_from_json()
  block: More detailed syntax error reporting for JSON filenames
  check-qjson: Test errors from qobject_from_json()
  test-visitor-serialization: Pass &error_abort to qobject_from_json()
  monitor: Assert qmp_schema_json[] is sane
  qapi: New qobject_input_visitor_new_str() for convenience
  block: Initial implementation of -blockdev
  qapi: Improve how keyval input visitor reports unexpected dicts
  docs/qapi-code-gen.txt: Clarify naming rules
  test-qapi-util: New, covering qapi/qapi-util.c
  qapi: New parse_qapi_name()
  keyval: Restrict key components to valid QAPI names
  keyval: Support lists

 block.c                              |   9 +-
 docs/qapi-code-gen.txt               |  61 ++--
 include/qapi/qmp/qjson.h             |   8 +-
 include/qapi/qobject-input-visitor.h |  21 ++
 include/qapi/util.h                  |   2 +
 include/qemu/option.h                |   3 +
 monitor.c                            |   2 +-
 qapi/qapi-util.c                     |  47 +++
 qapi/qobject-input-visitor.c         | 210 +++++++++++-
 qemu-options.hx                      |   7 +
 qobject/qjson.c                      |  14 +-
 tests/.gitignore                     |   2 +
 tests/Makefile.include               |  10 +-
 tests/check-qjson.c                  |  88 +++--
 tests/libqtest.c                     |   3 +-
 tests/test-keyval.c                  | 619 +++++++++++++++++++++++++++++++++++
 tests/test-qapi-util.c               |  85 +++++
 tests/test-qemu-opts.c               |   5 +
 tests/test-qobject-input-visitor.c   | 190 ++++++++++-
 tests/test-visitor-serialization.c   |   2 +-
 util/Makefile.objs                   |   1 +
 util/keyval.c                        | 385 ++++++++++++++++++++++
 vl.c                                 |  39 +++
 23 files changed, 1730 insertions(+), 83 deletions(-)
 create mode 100644 tests/test-keyval.c
 create mode 100644 tests/test-qapi-util.c
 create mode 100644 util/keyval.c

-- 
2.7.4

[Qemu-devel] [PATCH 01/24] test-qemu-opts: Cover qemu_opts_parse() of "no"

[Markus Armbruster]

qemu_opts_parse() interprets "no" as negated empty key.  Consistent
with its acceptance of empty keys elsewhere, whatever that's worth.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qemu-opts.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index c46ef31..f6310b3 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -532,6 +532,11 @@ static void test_opts_parse(void)
     g_assert_cmpstr(qemu_opt_get(opts, "aus"), ==, "off");
     g_assert_cmpstr(qemu_opt_get(opts, "noaus"), ==, "");
 
+    /* Implied value, negated empty key */
+    opts = qemu_opts_parse(&opts_list_03, "no", false, &error_abort);
+    g_assert_cmpuint(opts_count(opts), ==, 1);
+    g_assert_cmpstr(qemu_opt_get(opts, ""), ==, "off");
+
     /* Implied key */
     opts = qemu_opts_parse(&opts_list_03, "an,noaus,noaus=", true,
                            &error_abort);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 01/24] test-qemu-opts: Cover qemu_opts_parse() of "no"

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> qemu_opts_parse() interprets "no" as negated empty key.  Consistent
> with its acceptance of empty keys elsewhere, whatever that's worth.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 02/24] tests: Fix gcov-files-test-qemu-opts-y, gcov-files-test-logging-y

[Markus Armbruster]

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

diff --git a/tests/Makefile.include b/tests/Makefile.include
index cb97473..fdf528c 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -93,7 +93,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)
@@ -118,8 +118,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 02/24] tests: Fix gcov-files-test-qemu-opts-y, gcov-files-test-logging-y

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 03/24] 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).

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

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

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

* Empty key fragments 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=".

* Implied value is not supported.  qemu_opts_parse() desugars "foo" to
  "foo=on", and "nofoo" to "foo=off".

* An implied key's value can't be empty, and can't contain ','.

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

Note: keyval_parse() provides no way to do lists, and its key syntax
is incompatible with the __RFQDN_ prefix convention for downstream
extensions, because it blindly splits at '.', even in __RFQDN_.  Both
issues will be addressed later in the series.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qemu/option.h  |   3 +
 tests/.gitignore       |   1 +
 tests/Makefile.include |   3 +
 tests/test-keyval.c    | 180 ++++++++++++++++++++++++++++++++++++++
 util/Makefile.objs     |   1 +
 util/keyval.c          | 228 +++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 416 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 fdf528c..2171e4a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -94,6 +94,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)
@@ -721,6 +723,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..27f6625
--- /dev/null
+++ b/tests/test-keyval.c
@@ -0,0 +1,180 @@
+/*
+ * 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 (qemu_opts_parse() accepts this) */
+    qdict = keyval_parse("=val", NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* Empty key fragment */
+    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 fragment */
+    qdict = keyval_parse(params, NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+    g_free(params);
+
+    /* Long key (qemu_opts_parse() accepts and truncates silently) */
+    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 fragment */
+    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 qemu_opts_parse() */
+    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);
+
+    /* 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 qemu_opts_parse() 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 (qemu_opts_parse() rejects it) */
+    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);
+
+    /* Implied value not supported (unlike qemu_opts_parse()) */
+    qdict = keyval_parse("an,noaus,noaus=", NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* Implied value, key "no" (qemu_opts_parse(): negated empty key) */
+    qdict = keyval_parse("no", NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* Implied key */
+    qdict = keyval_parse("an,aus=off,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);
+
+    /* Implied dotted key */
+    qdict = keyval_parse("val", "eins.zwei", &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    sub_qdict = qdict_get_qdict(qdict, "eins");
+    g_assert(sub_qdict);
+    g_assert_cmpuint(qdict_size(sub_qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(sub_qdict, "zwei"), ==, "val");
+    QDECREF(qdict);
+
+    /* Implied key with empty value (qemu_opts_parse() accepts this) */
+    qdict = keyval_parse(",", "implied", &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* Likewise (qemu_opts_parse(): implied key with comma value) */
+    qdict = keyval_parse(",,,a=1", "implied", &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* Empty key is not an implied key */
+    qdict = keyval_parse("=val", "implied", &err);
+    error_free_or_abort(&err);
+    g_assert(!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..3904c39
--- /dev/null
+++ b/util/keyval.c
@@ -0,0 +1,228 @@
+/*
+ * 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.
+ */
+
+/*
+ * KEY=VALUE,... syntax:
+ *
+ *   key-vals     = [ key-val { ',' key-vals } ]
+ *   key-val      = key '=' val
+ *   key          = key-fragment { '.' key-fragment }
+ *   key-fragment = / [^=,.]* /
+ *   val          = { / [^,]* / | ',,' }
+ *
+ * Semantics defined by reduction to JSON:
+ *
+ *   key-vals defines a tree of objects rooted at R
+ *   where for each key-val = key-fragment . ... = val in key-vals
+ *       R op key-fragment op ... = val'
+ *       where (left-associative) op is member reference L.key-fragment
+ *             val' is val with ',,' replaced by ','
+ *   and only R may be empty.
+ *
+ *   Duplicate keys are permitted; all but the last one are ignored.
+ *
+ *   The equations must have a solution.  Counter-example: a.b=1,a=2
+ *   doesn't have one, because R.a must be an object to satisfy a.b=1
+ *   and a string to satisfy a=2.
+ *
+ * The length of any key-fragment must be between 1 and 127.
+ *
+ * Design flaw: there is no way to denote an empty non-root object.
+ * While interpreting "key absent" as empty object seems natural
+ * (removing a key-val from the input string removes the member when
+ * there are more, so why not when it's the last), it doesn't work:
+ * "key absent" already means "optional object absent", which isn't
+ * the same as "empty object present".
+ *
+ * Additional syntax for use with an implied key:
+ *
+ *   key-vals-ik  = val-no-key [ ',' key-vals ]
+ *   val-no-key   = / [^,]* /
+ *
+ * where no-key is syntactic sugar for implied-key=val-no-key.
+ *
+ * TODO support lists
+ * TODO support key-fragment with __RFQDN_ prefix (downstream extensions)
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qapi/qmp/qstring.h"
+#include "qemu/option.h"
+
+/*
+ * Ensure @cur maps @key_in_cur the right way.
+ * If @value is null, it needs to map to a QDict, else to this
+ * QString.
+ * If @cur doesn't have @key_in_cur, put an empty QDict or @value,
+ * respectively.
+ * Else, if it needs to map to a QDict, and already does, do nothing.
+ * Else, if it needs to map to this QString, and already maps to a
+ * QString, replace it by @value.
+ * Else, fail because we have conflicting needs on how to map
+ * @key_in_cur.
+ * Use @key up to @key_cursor to identify the key in error messages.
+ * On success, return the mapped value.
+ * On failure, store an error through @errp and return NULL.
+ */
+static QObject *keyval_parse_put(QDict *cur,
+                                 const char *key_in_cur, QString *value,
+                                 const char *key, const char *key_cursor,
+                                 Error **errp)
+{
+    QObject *old, *new;
+
+    old = qdict_get(cur, key_in_cur);
+    if (old) {
+        if (qobject_type(old) != (value ? QTYPE_QSTRING : QTYPE_QDICT)) {
+            error_setg(errp, "Parameters '%.*s.*' used inconsistently",
+                       (int)(key_cursor - key), key);
+            return NULL;
+        }
+        if (!value) {
+            return old;         /* already QDict, do nothing */
+        }
+        new = QOBJECT(value);   /* replacement */
+    } else {
+        new = QOBJECT(value) ?: QOBJECT(qdict_new());
+    }
+    qdict_put_obj(cur, key_in_cur, new);
+    return new;
+}
+
+/*
+ * Parse one KEY=VALUE from @params, store result in @qdict.
+ * The first fragment of KEY applies to @qdict.  Subsequent fragments
+ * apply to nested QDicts, which are created on demand.  @implied_key
+ * is as in keyval_parse().
+ * On success, return a pointer to the next KEY=VALUE, or else to '\0'.
+ * On failure, return NULL.
+ */
+static const char *keyval_parse_one(QDict *qdict, const char *params,
+                                    const char *implied_key,
+                                    Error **errp)
+{
+    const char *key, *key_end, *s;
+    size_t len;
+    char key_in_cur[128];
+    QDict *cur;
+    QObject *next;
+    QString *val;
+
+    key = params;
+    len = strcspn(params, "=,");
+    if (implied_key && len && key[len] != '=') {
+        /* Desugar implied key */
+        key = implied_key;
+        len = strlen(implied_key);
+    }
+    key_end = key + len;
+
+    /*
+     * Loop over key fragments: @s points to current fragment, it
+     * applies to @cur.  @key_in_cur[] holds the previous fragment.
+     */
+    cur = qdict;
+    s = key;
+    for (;;) {
+        for (len = 0; s + len < key_end && s[len] != '.'; len++) {
+        }
+        if (!len) {
+            assert(key != implied_key);
+            error_setg(errp, "Invalid parameter '%.*s'",
+                       (int)(key_end - key), key);
+            return NULL;
+        }
+        if (len >= sizeof(key_in_cur)) {
+            assert(key != implied_key);
+            error_setg(errp, "Parameter%s '%.*s' is too long",
+                       s != key || s + len != key_end ? " fragment" : "",
+                       (int)len, s);
+            return NULL;
+        }
+
+        if (s != key) {
+            next = keyval_parse_put(cur, key_in_cur, NULL,
+                                    key, s - 1, errp);
+            if (!next) {
+                return NULL;
+            }
+            cur = qobject_to_qdict(next);
+            assert(cur);
+        }
+
+        memcpy(key_in_cur, s, len);
+        key_in_cur[len] = 0;
+        s += len;
+
+        if (*s != '.') {
+            break;
+        }
+        s++;
+    }
+
+    if (key == implied_key) {
+        assert(!*s);
+        s = params;
+    } else {
+        if (*s != '=') {
+            error_setg(errp, "Expected '=' after parameter '%.*s'",
+                       (int)(s - key), key);
+            return NULL;
+        }
+        s++;
+    }
+
+    val = qstring_new();
+    for (;;) {
+        if (!*s) {
+            break;
+        } else if (*s == ',') {
+            s++;
+            if (*s != ',') {
+                break;
+            }
+        }
+        qstring_append_chr(val, *s++);
+    }
+
+    if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) {
+        return NULL;
+    }
+    return s;
+}
+
+/*
+ * Parse @params in QEMU's traditional KEY=VALUE,... syntax.
+ * If @implied_key, the first KEY= can be omitted.  @implied_key is
+ * implied then, and VALUE can't be empty or contain ',' or '='.
+ * On success, return a dictionary of the parsed keys and values.
+ * On failure, store an error through @errp and return NULL.
+ */
+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 03/24] keyval: New keyval_parse()

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> keyval_parse() parses KEY=VALUE,... into a QDict.  Works like
> qemu_opts_parse(), except:
> 
> * Returns a QDict instead of a QemuOpts (d'oh).
> 
> * Supports nesting, unlike QemuOpts: a KEY is split into key
>   fragments at '.' (dotted key convention; the block layer does
>   something similar on top of QemuOpts).  The key fragments are QDict
>   keys, and the last one's value is updated to VALUE.
> 
> * Each key fragment may be up to 127 bytes long.  qemu_opts_parse()
>   limits the entire key to 127 bytes.
> 
> * Overlong key fragments are rejected.  qemu_opts_parse() silently
>   truncates them.
> 
> * Empty key fragments 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=".

This is important to keep in mind, callers need to explicitly check
validity of the "id" key themselves.

> * Implied value is not supported.  qemu_opts_parse() desugars "foo" to
>   "foo=on", and "nofoo" to "foo=off".
> 
> * An implied key's value can't be empty, and can't contain ','.
> 
> I intend to grow this into a saner replacement for QemuOpts.  It'll
> take time, though.
> 
> Note: keyval_parse() provides no way to do lists, and its key syntax
> is incompatible with the __RFQDN_ prefix convention for downstream
> extensions, because it blindly splits at '.', even in __RFQDN_.  Both
> issues will be addressed later in the series.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

> diff --git a/util/keyval.c b/util/keyval.c
> new file mode 100644
> index 0000000..3904c39
> --- /dev/null
> +++ b/util/keyval.c
> @@ -0,0 +1,228 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * KEY=VALUE,... syntax:
> + *
> + *   key-vals     = [ key-val { ',' key-vals } ]
> + *   key-val      = key '=' val
> + *   key          = key-fragment { '.' key-fragment }
> + *   key-fragment = / [^=,.]* /
> + *   val          = { / [^,]* / | ',,' }
> + *
> + * Semantics defined by reduction to JSON:
> + *
> + *   key-vals defines a tree of objects rooted at R
> + *   where for each key-val = key-fragment . ... = val in key-vals
> + *       R op key-fragment op ... = val'
> + *       where (left-associative) op is member reference L.key-fragment

Maybe it's just me, but I can't say that I fully understand what these
last two lines are supposed to tell me.

> + *             val' is val with ',,' replaced by ','
> + *   and only R may be empty.
> + *
> + *   Duplicate keys are permitted; all but the last one are ignored.
> + *
> + *   The equations must have a solution.  Counter-example: a.b=1,a=2
> + *   doesn't have one, because R.a must be an object to satisfy a.b=1
> + *   and a string to satisfy a=2.
> + *
> + * The length of any key-fragment must be between 1 and 127.
> + *
> + * Design flaw: there is no way to denote an empty non-root object.
> + * While interpreting "key absent" as empty object seems natural
> + * (removing a key-val from the input string removes the member when
> + * there are more, so why not when it's the last), it doesn't work:
> + * "key absent" already means "optional object absent", which isn't
> + * the same as "empty object present".
> + *
> + * Additional syntax for use with an implied key:
> + *
> + *   key-vals-ik  = val-no-key [ ',' key-vals ]
> + *   val-no-key   = / [^,]* /
> + *
> + * where no-key is syntactic sugar for implied-key=val-no-key.

s/no-key/val-no-key/ ?

> + *
> + * TODO support lists
> + * TODO support key-fragment with __RFQDN_ prefix (downstream extensions)

Worth another TODO comment for implied values that contain a comma? The
current restriction feels a bit artificial.

> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qapi/qmp/qstring.h"
> +#include "qemu/option.h"
> +
> +/*
> + * Ensure @cur maps @key_in_cur the right way.
> + * If @value is null, it needs to map to a QDict, else to this
> + * QString.
> + * If @cur doesn't have @key_in_cur, put an empty QDict or @value,
> + * respectively.
> + * Else, if it needs to map to a QDict, and already does, do nothing.
> + * Else, if it needs to map to this QString, and already maps to a
> + * QString, replace it by @value.
> + * Else, fail because we have conflicting needs on how to map
> + * @key_in_cur.
> + * Use @key up to @key_cursor to identify the key in error messages.
> + * On success, return the mapped value.
> + * On failure, store an error through @errp and return NULL.
> + */
> +static QObject *keyval_parse_put(QDict *cur,
> +                                 const char *key_in_cur, QString *value,
> +                                 const char *key, const char *key_cursor,
> +                                 Error **errp)
> +{
> +    QObject *old, *new;
> +
> +    old = qdict_get(cur, key_in_cur);
> +    if (old) {
> +        if (qobject_type(old) != (value ? QTYPE_QSTRING : QTYPE_QDICT)) {
> +            error_setg(errp, "Parameters '%.*s.*' used inconsistently",
> +                       (int)(key_cursor - key), key);
> +            return NULL;
> +        }
> +        if (!value) {
> +            return old;         /* already QDict, do nothing */
> +        }
> +        new = QOBJECT(value);   /* replacement */
> +    } else {
> +        new = QOBJECT(value) ?: QOBJECT(qdict_new());
> +    }
> +    qdict_put_obj(cur, key_in_cur, new);
> +    return new;
> +}
> +
> +/*
> + * Parse one KEY=VALUE from @params, store result in @qdict.
> + * The first fragment of KEY applies to @qdict.  Subsequent fragments
> + * apply to nested QDicts, which are created on demand.  @implied_key
> + * is as in keyval_parse().
> + * On success, return a pointer to the next KEY=VALUE, or else to '\0'.
> + * On failure, return NULL.
> + */
> +static const char *keyval_parse_one(QDict *qdict, const char *params,
> +                                    const char *implied_key,
> +                                    Error **errp)
> +{
> +    const char *key, *key_end, *s;
> +    size_t len;
> +    char key_in_cur[128];
> +    QDict *cur;
> +    QObject *next;
> +    QString *val;
> +
> +    key = params;
> +    len = strcspn(params, "=,");
> +    if (implied_key && len && key[len] != '=') {
> +        /* Desugar implied key */
> +        key = implied_key;
> +        len = strlen(implied_key);
> +    }
> +    key_end = key + len;
> +
> +    /*
> +     * Loop over key fragments: @s points to current fragment, it
> +     * applies to @cur.  @key_in_cur[] holds the previous fragment.
> +     */
> +    cur = qdict;
> +    s = key;
> +    for (;;) {
> +        for (len = 0; s + len < key_end && s[len] != '.'; len++) {
> +        }
> +        if (!len) {
> +            assert(key != implied_key);
> +            error_setg(errp, "Invalid parameter '%.*s'",
> +                       (int)(key_end - key), key);
> +            return NULL;
> +        }
> +        if (len >= sizeof(key_in_cur)) {
> +            assert(key != implied_key);
> +            error_setg(errp, "Parameter%s '%.*s' is too long",
> +                       s != key || s + len != key_end ? " fragment" : "",
> +                       (int)len, s);
> +            return NULL;
> +        }
> +
> +        if (s != key) {
> +            next = keyval_parse_put(cur, key_in_cur, NULL,
> +                                    key, s - 1, errp);
> +            if (!next) {
> +                return NULL;
> +            }
> +            cur = qobject_to_qdict(next);
> +            assert(cur);
> +        }
> +
> +        memcpy(key_in_cur, s, len);
> +        key_in_cur[len] = 0;
> +        s += len;
> +
> +        if (*s != '.') {
> +            break;
> +        }
> +        s++;
> +    }
> +
> +    if (key == implied_key) {
> +        assert(!*s);
> +        s = params;
> +    } else {
> +        if (*s != '=') {
> +            error_setg(errp, "Expected '=' after parameter '%.*s'",
> +                       (int)(s - key), key);
> +            return NULL;
> +        }
> +        s++;
> +    }
> +
> +    val = qstring_new();
> +    for (;;) {
> +        if (!*s) {
> +            break;
> +        } else if (*s == ',') {
> +            s++;
> +            if (*s != ',') {
> +                break;
> +            }
> +        }
> +        qstring_append_chr(val, *s++);
> +    }
> +
> +    if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) {
> +        return NULL;

This leaks val.

> +    }
> +    return s;
> +}

Kevin

Re: [Qemu-devel] [PATCH 03/24] keyval: New keyval_parse()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>> keyval_parse() parses KEY=VALUE,... into a QDict.  Works like
>> qemu_opts_parse(), except:
>> 
>> * Returns a QDict instead of a QemuOpts (d'oh).
>> 
>> * Supports nesting, unlike QemuOpts: a KEY is split into key
>>   fragments at '.' (dotted key convention; the block layer does
>>   something similar on top of QemuOpts).  The key fragments are QDict
>>   keys, and the last one's value is updated to VALUE.
>> 
>> * Each key fragment may be up to 127 bytes long.  qemu_opts_parse()
>>   limits the entire key to 127 bytes.
>> 
>> * Overlong key fragments are rejected.  qemu_opts_parse() silently
>>   truncates them.
>> 
>> * Empty key fragments 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=".
>
> This is important to keep in mind, callers need to explicitly check
> validity of the "id" key themselves.

Good point.  Come to think of it, I'm not sure I did %-}

>> * Implied value is not supported.  qemu_opts_parse() desugars "foo" to
>>   "foo=on", and "nofoo" to "foo=off".
>> 
>> * An implied key's value can't be empty, and can't contain ','.
>> 
>> I intend to grow this into a saner replacement for QemuOpts.  It'll
>> take time, though.
>> 
>> Note: keyval_parse() provides no way to do lists, and its key syntax
>> is incompatible with the __RFQDN_ prefix convention for downstream
>> extensions, because it blindly splits at '.', even in __RFQDN_.  Both
>> issues will be addressed later in the series.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>> diff --git a/util/keyval.c b/util/keyval.c
>> new file mode 100644
>> index 0000000..3904c39
>> --- /dev/null
>> +++ b/util/keyval.c
>> @@ -0,0 +1,228 @@
>> +/*
>> + * 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.
>> + */
>> +
>> +/*
>> + * KEY=VALUE,... syntax:
>> + *
>> + *   key-vals     = [ key-val { ',' key-vals } ]
>> + *   key-val      = key '=' val
>> + *   key          = key-fragment { '.' key-fragment }
>> + *   key-fragment = / [^=,.]* /
>> + *   val          = { / [^,]* / | ',,' }
>> + *
>> + * Semantics defined by reduction to JSON:
>> + *
>> + *   key-vals defines a tree of objects rooted at R
>> + *   where for each key-val = key-fragment . ... = val in key-vals
>> + *       R op key-fragment op ... = val'
>> + *       where (left-associative) op is member reference L.key-fragment
>
> Maybe it's just me, but I can't say that I fully understand what these
> last two lines are supposed to tell me.

Okay, I'll try to clarify.

Last patch amends this part for lists.

>> + *             val' is val with ',,' replaced by ','
>> + *   and only R may be empty.
>> + *
>> + *   Duplicate keys are permitted; all but the last one are ignored.
>> + *
>> + *   The equations must have a solution.  Counter-example: a.b=1,a=2
>> + *   doesn't have one, because R.a must be an object to satisfy a.b=1
>> + *   and a string to satisfy a=2.
>> + *
>> + * The length of any key-fragment must be between 1 and 127.
>> + *
>> + * Design flaw: there is no way to denote an empty non-root object.
>> + * While interpreting "key absent" as empty object seems natural
>> + * (removing a key-val from the input string removes the member when
>> + * there are more, so why not when it's the last), it doesn't work:
>> + * "key absent" already means "optional object absent", which isn't
>> + * the same as "empty object present".
>> + *
>> + * Additional syntax for use with an implied key:
>> + *
>> + *   key-vals-ik  = val-no-key [ ',' key-vals ]
>> + *   val-no-key   = / [^,]* /
>> + *
>> + * where no-key is syntactic sugar for implied-key=val-no-key.
>
> s/no-key/val-no-key/ ?

Yes.

>> + *
>> + * TODO support lists
>> + * TODO support key-fragment with __RFQDN_ prefix (downstream extensions)
>
> Worth another TODO comment for implied values that contain a comma? The
> current restriction feels a bit artificial.

My reasons for the restriction are perhaps best explained with an
example.  Consider params = "a,,,b".

qemu_opts_parse(opt_list, params, true) splits into comma-separated
parts like this:

    #   part    key       value
    0   "a,,"   implied   "a,"
    1   "b"     "b"       "on"

qemu_opts_parse(list, params, false) like this:

    #   part    key       value
    0   "a"     "a"       "on"
    1   ""      ""        "on"
    2   ""      ""        "on"
    3   "b"     "b"       "on"

The split of depends on the permit_abbrev argument!  I consider that
tasteless design.  Or rather I would if I could bring myself to believe
in the existence of a design here.

The big hammer to make *syntactic* analysis invariant of whether we have
an implied key is to outlaw ',' val-no-key.  That way, the leftmost part
of param can be isolated the same with and without implied key: it ends
at the first '=' or ','.

Perhaps a smaller hammer can be found, but right now I'm in no mood for
checking grammars for ambiguity :)

>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qapi/qmp/qstring.h"
>> +#include "qemu/option.h"
>> +
>> +/*
>> + * Ensure @cur maps @key_in_cur the right way.
>> + * If @value is null, it needs to map to a QDict, else to this
>> + * QString.
>> + * If @cur doesn't have @key_in_cur, put an empty QDict or @value,
>> + * respectively.
>> + * Else, if it needs to map to a QDict, and already does, do nothing.
>> + * Else, if it needs to map to this QString, and already maps to a
>> + * QString, replace it by @value.
>> + * Else, fail because we have conflicting needs on how to map
>> + * @key_in_cur.
>> + * Use @key up to @key_cursor to identify the key in error messages.
>> + * On success, return the mapped value.
>> + * On failure, store an error through @errp and return NULL.
>> + */
>> +static QObject *keyval_parse_put(QDict *cur,
>> +                                 const char *key_in_cur, QString *value,
>> +                                 const char *key, const char *key_cursor,
>> +                                 Error **errp)
>> +{
>> +    QObject *old, *new;
>> +
>> +    old = qdict_get(cur, key_in_cur);
>> +    if (old) {
>> +        if (qobject_type(old) != (value ? QTYPE_QSTRING : QTYPE_QDICT)) {
>> +            error_setg(errp, "Parameters '%.*s.*' used inconsistently",
>> +                       (int)(key_cursor - key), key);
>> +            return NULL;
>> +        }
>> +        if (!value) {
>> +            return old;         /* already QDict, do nothing */
>> +        }
>> +        new = QOBJECT(value);   /* replacement */
>> +    } else {
>> +        new = QOBJECT(value) ?: QOBJECT(qdict_new());
>> +    }
>> +    qdict_put_obj(cur, key_in_cur, new);
>> +    return new;
>> +}
>> +
>> +/*
>> + * Parse one KEY=VALUE from @params, store result in @qdict.
>> + * The first fragment of KEY applies to @qdict.  Subsequent fragments
>> + * apply to nested QDicts, which are created on demand.  @implied_key
>> + * is as in keyval_parse().
>> + * On success, return a pointer to the next KEY=VALUE, or else to '\0'.
>> + * On failure, return NULL.
>> + */
>> +static const char *keyval_parse_one(QDict *qdict, const char *params,
>> +                                    const char *implied_key,
>> +                                    Error **errp)
>> +{
>> +    const char *key, *key_end, *s;
>> +    size_t len;
>> +    char key_in_cur[128];
>> +    QDict *cur;
>> +    QObject *next;
>> +    QString *val;
>> +
>> +    key = params;
>> +    len = strcspn(params, "=,");
>> +    if (implied_key && len && key[len] != '=') {
>> +        /* Desugar implied key */
>> +        key = implied_key;
>> +        len = strlen(implied_key);
>> +    }
>> +    key_end = key + len;
>> +
>> +    /*
>> +     * Loop over key fragments: @s points to current fragment, it
>> +     * applies to @cur.  @key_in_cur[] holds the previous fragment.
>> +     */
>> +    cur = qdict;
>> +    s = key;
>> +    for (;;) {
>> +        for (len = 0; s + len < key_end && s[len] != '.'; len++) {
>> +        }
>> +        if (!len) {
>> +            assert(key != implied_key);
>> +            error_setg(errp, "Invalid parameter '%.*s'",
>> +                       (int)(key_end - key), key);
>> +            return NULL;
>> +        }
>> +        if (len >= sizeof(key_in_cur)) {
>> +            assert(key != implied_key);
>> +            error_setg(errp, "Parameter%s '%.*s' is too long",
>> +                       s != key || s + len != key_end ? " fragment" : "",
>> +                       (int)len, s);
>> +            return NULL;
>> +        }
>> +
>> +        if (s != key) {
>> +            next = keyval_parse_put(cur, key_in_cur, NULL,
>> +                                    key, s - 1, errp);
>> +            if (!next) {
>> +                return NULL;
>> +            }
>> +            cur = qobject_to_qdict(next);
>> +            assert(cur);
>> +        }
>> +
>> +        memcpy(key_in_cur, s, len);
>> +        key_in_cur[len] = 0;
>> +        s += len;
>> +
>> +        if (*s != '.') {
>> +            break;
>> +        }
>> +        s++;
>> +    }
>> +
>> +    if (key == implied_key) {
>> +        assert(!*s);
>> +        s = params;
>> +    } else {
>> +        if (*s != '=') {
>> +            error_setg(errp, "Expected '=' after parameter '%.*s'",
>> +                       (int)(s - key), key);
>> +            return NULL;
>> +        }
>> +        s++;
>> +    }
>> +
>> +    val = qstring_new();
>> +    for (;;) {
>> +        if (!*s) {
>> +            break;
>> +        } else if (*s == ',') {
>> +            s++;
>> +            if (*s != ',') {
>> +                break;
>> +            }
>> +        }
>> +        qstring_append_chr(val, *s++);
>> +    }
>> +
>> +    if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) {
>> +        return NULL;
>
> This leaks val.

Will fix, thanks!

>> +    }
>> +    return s;
>> +}
>
> Kevin

Re: [Qemu-devel] [PATCH 03/24] keyval: New keyval_parse()

[Eric Blake]

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

On 02/28/2017 09:48 AM, Kevin Wolf wrote:
> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>> keyval_parse() parses KEY=VALUE,... into a QDict.  Works like
>> qemu_opts_parse(), except:
>>

>> +
>> +/*
>> + * KEY=VALUE,... syntax:
>> + *
>> + *   key-vals     = [ key-val { ',' key-vals } ]

Just refreshing my memory: in this grammar, [] means optional (0 or 1),
and {} means repeating (0 or more).

That means an empty string satisfies key-vals (as in "-option ''"),
intentional?

I don't see how this permits a trailing comma, but isn't that one of
your goals to allow "-option key=val," the same as "-option key=val"?

>> + *   key-val      = key '=' val
>> + *   key          = key-fragment { '.' key-fragment }

Ambiguous.

>> + *   key-fragment = / [^=,.]* /

Do you want + instead of * in the regex, so as to require a non-empty
string for key-fragment?  After all, you want to reject "-option a..b=val".

>> + *   val          = { / [^,]* / | ',,' }

Here, * makes sense, since an empty value is permitted in '-option key=".

>> + *
>> + * Semantics defined by reduction to JSON:
>> + *
>> + *   key-vals defines a tree of objects rooted at R
>> + *   where for each key-val = key-fragment . ... = val in key-vals
>> + *       R op key-fragment op ... = val'
>> + *       where (left-associative) op is member reference L.key-fragment
> 
> Maybe it's just me, but I can't say that I fully understand what these
> last two lines are supposed to tell me.

I think it's trying to portray dictionary member lookup semantics (each
key-fragment represents another member lookup one dictionary deeper,
before reaching the final lookup to the scalar value) - but yeah, it was
a confusing read to me as well.

> 
>> + *             val' is val with ',,' replaced by ','
>> + *   and only R may be empty.
>> + *
>> + *   Duplicate keys are permitted; all but the last one are ignored.
>> + *
>> + *   The equations must have a solution.  Counter-example: a.b=1,a=2
>> + *   doesn't have one, because R.a must be an object to satisfy a.b=1
>> + *   and a string to satisfy a=2.
>> + *
>> + * The length of any key-fragment must be between 1 and 127.
>> + *
>> + * Design flaw: there is no way to denote an empty non-root object.
>> + * While interpreting "key absent" as empty object seems natural
>> + * (removing a key-val from the input string removes the member when
>> + * there are more, so why not when it's the last), it doesn't work:
>> + * "key absent" already means "optional object absent", which isn't
>> + * the same as "empty object present".
>> + *
>> + * Additional syntax for use with an implied key:
>> + *
>> + *   key-vals-ik  = val-no-key [ ',' key-vals ]
>> + *   val-no-key   = / [^,]* /

I think this needs to be [^,=]*, since the presence of an = means you've
supplied a key, and are not using the implied-key sugar.

>> + *
>> + * where no-key is syntactic sugar for implied-key=val-no-key.
> 
> s/no-key/val-no-key/ ?
> 
>> + *
>> + * TODO support lists
>> + * TODO support key-fragment with __RFQDN_ prefix (downstream extensions)
> 
> Worth another TODO comment for implied values that contain a comma? The
> current restriction feels a bit artificial.

It may be a bit artificial, but at least we can document it: implied
keys are sugar that can only be used for certain values, but you can
always avoid the sugar and explicitly provide the key=value for
problematic values that can't be done with the implied key.


-- 
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 03/24] keyval: New keyval_parse()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/28/2017 09:48 AM, Kevin Wolf wrote:
>> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>>> keyval_parse() parses KEY=VALUE,... into a QDict.  Works like
>>> qemu_opts_parse(), except:
>>>
>
>>> +
>>> +/*
>>> + * KEY=VALUE,... syntax:
>>> + *
>>> + *   key-vals     = [ key-val { ',' key-vals } ]
>
> Just refreshing my memory: in this grammar, [] means optional (0 or 1),
> and {} means repeating (0 or more).

Yes.

> That means an empty string satisfies key-vals (as in "-option ''"),
> intentional?

Yes.  Creates an empty root object.  If we don't want that, or don't
want it now, I can make this case an error.

> I don't see how this permits a trailing comma, but isn't that one of
> your goals to allow "-option key=val," the same as "-option key=val"?

Mistake.  Possible fix:

          key-vals     = [ key-val { ',' key-val } [ ',' ]

>>> + *   key-val      = key '=' val
>>> + *   key          = key-fragment { '.' key-fragment }
>
> Ambiguous.

I'm dense.  Can you give me an example string with two derivations?

>>> + *   key-fragment = / [^=,.]* /
>
> Do you want + instead of * in the regex, so as to require a non-empty
> string for key-fragment?  After all, you want to reject "-option a..b=val".

I like to keep syntactic and semantic analysis conceptually separate.
keyval_parse() looks for the next '.' to extract a key-fragment
(syntactic analysis).  It then rejects key-fragments it doesn't like
(semantic analysis).  Right now, it only dislikes lengths outside
[1,127].  Later on, it'll additionally dislike key-fragments that are
neither valid QAPI names nor digit strings.

Perhaps my comment could explain this better.

>>> + *   val          = { / [^,]* / | ',,' }
>
> Here, * makes sense, since an empty value is permitted in '-option key=".
>
>>> + *
>>> + * Semantics defined by reduction to JSON:
>>> + *
>>> + *   key-vals defines a tree of objects rooted at R
>>> + *   where for each key-val = key-fragment . ... = val in key-vals
>>> + *       R op key-fragment op ... = val'
>>> + *       where (left-associative) op is member reference L.key-fragment
>> 
>> Maybe it's just me, but I can't say that I fully understand what these
>> last two lines are supposed to tell me.
>
> I think it's trying to portray dictionary member lookup semantics (each
> key-fragment represents another member lookup one dictionary deeper,
> before reaching the final lookup to the scalar value) - but yeah, it was
> a confusing read to me as well.

We're in a bit of a time squeeze right now.  I'd like to clarify the
comment on top.

>>> + *             val' is val with ',,' replaced by ','
>>> + *   and only R may be empty.
>>> + *
>>> + *   Duplicate keys are permitted; all but the last one are ignored.
>>> + *
>>> + *   The equations must have a solution.  Counter-example: a.b=1,a=2
>>> + *   doesn't have one, because R.a must be an object to satisfy a.b=1
>>> + *   and a string to satisfy a=2.
>>> + *
>>> + * The length of any key-fragment must be between 1 and 127.
>>> + *
>>> + * Design flaw: there is no way to denote an empty non-root object.
>>> + * While interpreting "key absent" as empty object seems natural
>>> + * (removing a key-val from the input string removes the member when
>>> + * there are more, so why not when it's the last), it doesn't work:
>>> + * "key absent" already means "optional object absent", which isn't
>>> + * the same as "empty object present".
>>> + *
>>> + * Additional syntax for use with an implied key:
>>> + *
>>> + *   key-vals-ik  = val-no-key [ ',' key-vals ]
>>> + *   val-no-key   = / [^,]* /
>
> I think this needs to be [^,=]*, since the presence of an = means you've
> supplied a key, and are not using the implied-key sugar.

You're right.

>>> + *
>>> + * where no-key is syntactic sugar for implied-key=val-no-key.
>> 
>> s/no-key/val-no-key/ ?
>> 
>>> + *
>>> + * TODO support lists
>>> + * TODO support key-fragment with __RFQDN_ prefix (downstream extensions)
>> 
>> Worth another TODO comment for implied values that contain a comma? The
>> current restriction feels a bit artificial.
>
> It may be a bit artificial, but at least we can document it: implied
> keys are sugar that can only be used for certain values, but you can
> always avoid the sugar and explicitly provide the key=value for
> problematic values that can't be done with the implied key.

Re: [Qemu-devel] [PATCH 03/24] keyval: New keyval_parse()

[Eric Blake]

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

On 02/28/2017 12:03 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 02/28/2017 09:48 AM, Kevin Wolf wrote:
>>> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>>>> keyval_parse() parses KEY=VALUE,... into a QDict.  Works like
>>>> qemu_opts_parse(), except:
>>>>
>>
>>>> +
>>>> +/*
>>>> + * KEY=VALUE,... syntax:
>>>> + *
>>>> + *   key-vals     = [ key-val { ',' key-vals } ]
>>
>> Just refreshing my memory: in this grammar, [] means optional (0 or 1),
>> and {} means repeating (0 or more).
> 
> Yes.
> 
>> That means an empty string satisfies key-vals (as in "-option ''"),
>> intentional?
> 
> Yes.  Creates an empty root object.  If we don't want that, or don't
> want it now, I can make this case an error.

Making it an error now, with the ability to relax it later, would be in
line with the fact that dotted notation cannot create an empty
sub-element.  Supporting it now means we're stuck with it even if we
decide the empty string could have usefully meant something else
(although what else is useful besides an empty root object is beyond me).

> 
>> I don't see how this permits a trailing comma, but isn't that one of
>> your goals to allow "-option key=val," the same as "-option key=val"?
> 
> Mistake.  Possible fix:
> 
>           key-vals     = [ key-val { ',' key-val } [ ',' ]

Unbalanced [].  I think you meant:

key-vals = [ key-val { ',' key-val } [ ',' ] ]

which properly rejects "-option ," while allowing "-option key=val,".


> 
>>>> + *   key-val      = key '=' val
>>>> + *   key          = key-fragment { '.' key-fragment }
>>
>> Ambiguous.
> 
> I'm dense.  Can you give me an example string with two derivations?

Sorry, poor editing on my part. (I wrote that before I figured out that
{} meant 0 or more, then forgot to clean it up).  Looks like this one is
well-formed after all.

> 
>>>> + *   key-fragment = / [^=,.]* /
>>
>> Do you want + instead of * in the regex, so as to require a non-empty
>> string for key-fragment?  After all, you want to reject "-option a..b=val".
> 
> I like to keep syntactic and semantic analysis conceptually separate.
> keyval_parse() looks for the next '.' to extract a key-fragment
> (syntactic analysis).  It then rejects key-fragments it doesn't like
> (semantic analysis).  Right now, it only dislikes lengths outside
> [1,127].  Later on, it'll additionally dislike key-fragments that are
> neither valid QAPI names nor digit strings.
> 
> Perhaps my comment could explain this better.

Yes, a comment would help (then keeping the grammar accepting a 0-length
string doesn't hurt, because the semantic analysis kicks in).


>>>> + *
>>>> + * Semantics defined by reduction to JSON:
>>>> + *
>>>> + *   key-vals defines a tree of objects rooted at R
>>>> + *   where for each key-val = key-fragment . ... = val in key-vals
>>>> + *       R op key-fragment op ... = val'
>>>> + *       where (left-associative) op is member reference L.key-fragment
>>>
>>> Maybe it's just me, but I can't say that I fully understand what these
>>> last two lines are supposed to tell me.
>>
>> I think it's trying to portray dictionary member lookup semantics (each
>> key-fragment represents another member lookup one dictionary deeper,
>> before reaching the final lookup to the scalar value) - but yeah, it was
>> a confusing read to me as well.
> 
> We're in a bit of a time squeeze right now.  I'd like to clarify the
> comment on top.

That's fair.  A poor comment isn't code, so fixing it in soft freeze is
fine.  Maybe a FIXME is still appropriate, if we don't want to forget
it, but I trust your judgment on this one.

-- 
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 03/24] keyval: New keyval_parse()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/28/2017 12:03 PM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 02/28/2017 09:48 AM, Kevin Wolf wrote:
>>>> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>>>>> keyval_parse() parses KEY=VALUE,... into a QDict.  Works like
>>>>> qemu_opts_parse(), except:
>>>>>
>>>
>>>>> +
>>>>> +/*
>>>>> + * KEY=VALUE,... syntax:
>>>>> + *
>>>>> + *   key-vals     = [ key-val { ',' key-vals } ]
>>>
>>> Just refreshing my memory: in this grammar, [] means optional (0 or 1),
>>> and {} means repeating (0 or more).
>> 
>> Yes.
>> 
>>> That means an empty string satisfies key-vals (as in "-option ''"),
>>> intentional?
>> 
>> Yes.  Creates an empty root object.  If we don't want that, or don't
>> want it now, I can make this case an error.
>
> Making it an error now, with the ability to relax it later, would be in
> line with the fact that dotted notation cannot create an empty
> sub-element.  Supporting it now means we're stuck with it even if we
> decide the empty string could have usefully meant something else
> (although what else is useful besides an empty root object is beyond me).

We can outlaw "" during the freeze.

>>> I don't see how this permits a trailing comma, but isn't that one of
>>> your goals to allow "-option key=val," the same as "-option key=val"?
>> 
>> Mistake.  Possible fix:
>> 
>>           key-vals     = [ key-val { ',' key-val } [ ',' ]
>
> Unbalanced [].  I think you meant:
>
> key-vals = [ key-val { ',' key-val } [ ',' ] ]
>
> which properly rejects "-option ," while allowing "-option key=val,".

Fixed.

>>>>> + *   key-val      = key '=' val
>>>>> + *   key          = key-fragment { '.' key-fragment }
>>>
>>> Ambiguous.
>> 
>> I'm dense.  Can you give me an example string with two derivations?
>
> Sorry, poor editing on my part. (I wrote that before I figured out that
> {} meant 0 or more, then forgot to clean it up).  Looks like this one is
> well-formed after all.
>
>> 
>>>>> + *   key-fragment = / [^=,.]* /
>>>
>>> Do you want + instead of * in the regex, so as to require a non-empty
>>> string for key-fragment?  After all, you want to reject "-option a..b=val".
>> 
>> I like to keep syntactic and semantic analysis conceptually separate.
>> keyval_parse() looks for the next '.' to extract a key-fragment
>> (syntactic analysis).  It then rejects key-fragments it doesn't like
>> (semantic analysis).  Right now, it only dislikes lengths outside
>> [1,127].  Later on, it'll additionally dislike key-fragments that are
>> neither valid QAPI names nor digit strings.
>> 
>> Perhaps my comment could explain this better.
>
> Yes, a comment would help (then keeping the grammar accepting a 0-length
> string doesn't hurt, because the semantic analysis kicks in).

I'll try to address that when I clarify the comment on top.

>>>>> + *
>>>>> + * Semantics defined by reduction to JSON:
>>>>> + *
>>>>> + *   key-vals defines a tree of objects rooted at R
>>>>> + *   where for each key-val = key-fragment . ... = val in key-vals
>>>>> + *       R op key-fragment op ... = val'
>>>>> + *       where (left-associative) op is member reference L.key-fragment
>>>>
>>>> Maybe it's just me, but I can't say that I fully understand what these
>>>> last two lines are supposed to tell me.
>>>
>>> I think it's trying to portray dictionary member lookup semantics (each
>>> key-fragment represents another member lookup one dictionary deeper,
>>> before reaching the final lookup to the scalar value) - but yeah, it was
>>> a confusing read to me as well.
>> 
>> We're in a bit of a time squeeze right now.  I'd like to clarify the
>> comment on top.
>
> That's fair.  A poor comment isn't code, so fixing it in soft freeze is
> fine.  Maybe a FIXME is still appropriate, if we don't want to forget
> it, but I trust your judgment on this one.

Thanks!

[Qemu-devel] [PATCH 04/24] qapi: qobject input visitor variant for use with keyval_parse()

[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().  autocast replaced by keyval in identifiers,
noautocast replaced by fail in tests.

Fix qobject_input_type_uint64_keyval() not to reject '-', for QemuOpts
compatibility: replace parse_uint_full() by open-coded
parse_option_number().  The next commit will add suitable tests.
Leave out the fancy ERANGE error reporting for now, but add a TODO
comment.  Add it qobject_input_type_int64_keyval() and
qobject_input_type_number_keyval(), too.

Open code parse_option_bool() and parse_option_size() so we have to
call qobject_input_get_name() only when actually needed.  Again, leave
out ERANGE error reporting for now.

QAPI/QMP downstream extension prefixes __RFQDN_ don't work, because
keyval_parse() splits them at '.'.  Add a TODO comment there.

qobject_input_type_int64_keyval(), qobject_input_type_uint64_keyval(),
qobject_input_type_number_keyval() tweaked for style.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/qobject-input-visitor.h |   9 ++
 qapi/qobject-input-visitor.c         | 166 ++++++++++++++++++++++++++++++-
 tests/test-qobject-input-visitor.c   | 188 ++++++++++++++++++++++++++++++++++-
 3 files changed, 358 insertions(+), 5 deletions(-)

diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h
index 0b7633a..282f9d2 100644
--- a/include/qapi/qobject-input-visitor.h
+++ b/include/qapi/qobject-input-visitor.h
@@ -59,4 +59,13 @@ typedef struct QObjectInputVisitor QObjectInputVisitor;
  */
 Visitor *qobject_input_visitor_new(QObject *obj);
 
+/*
+ * Create a QObject input visitor for @obj for use with keyval_parse()
+ *
+ * This is like qobject_input_visitor_new(), except scalars are all
+ * QString, and error messages refer to parts of @obj in the syntax
+ * keyval_parse() uses for KEYs.
+ */
+Visitor *qobject_input_visitor_new_keyval(QObject *obj);
+
 #endif
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 6bb5a80..d53bad3 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -1,7 +1,7 @@
 /*
  * Input Visitor
  *
- * Copyright (C) 2012-2016 Red Hat, Inc.
+ * Copyright (C) 2012-2017 Red Hat, Inc.
  * Copyright IBM, Corp. 2011
  *
  * Authors:
@@ -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 of @obj in its parent, if any */
@@ -336,6 +337,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_keyval(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,
+                   full_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,
+                   full_name(qiv, name), "integer");
+    }
+}
+
 static void qobject_input_type_uint64(Visitor *v, const char *name,
                                       uint64_t *obj, Error **errp)
 {
@@ -357,6 +383,30 @@ static void qobject_input_type_uint64(Visitor *v, const char *name,
     *obj = qint_get_int(qint);
 }
 
+static void qobject_input_type_uint64_keyval(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,
+                   full_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,
+                   full_name(qiv, name), "integer");
+    }
+}
+
 static void qobject_input_type_bool(Visitor *v, const char *name, bool *obj,
                                     Error **errp)
 {
@@ -377,6 +427,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_keyval(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,
+                   full_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,
+                   full_name(qiv, name), "'on' or 'off'");
+    }
+}
+
 static void qobject_input_type_str(Visitor *v, const char *name, char **obj,
                                    Error **errp)
 {
@@ -425,6 +504,35 @@ static void qobject_input_type_number(Visitor *v, const char *name, double *obj,
                full_name(qiv, name), "number");
 }
 
+static void qobject_input_type_number_keyval(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,
+                   full_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,
+                   full_name(qiv, name), "number");
+    }
+}
+
 static void qobject_input_type_any(Visitor *v, const char *name, QObject **obj,
                                    Error **errp)
 {
@@ -455,6 +563,30 @@ static void qobject_input_type_null(Visitor *v, const char *name, Error **errp)
     }
 }
 
+static void qobject_input_type_size_keyval(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,
+                   full_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,
+                   full_name(qiv, name), "size");
+    }
+}
+
 static void qobject_input_optional(Visitor *v, const char *name, bool *present)
 {
     QObjectInputVisitor *qiv = to_qiv(v);
@@ -517,3 +649,35 @@ Visitor *qobject_input_visitor_new(QObject *obj)
 
     return &v->visitor;
 }
+
+Visitor *qobject_input_visitor_new_keyval(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.check_list = qobject_input_check_list;
+    v->visitor.end_list = qobject_input_pop;
+    v->visitor.start_alternate = qobject_input_start_alternate;
+    v->visitor.type_int64 = qobject_input_type_int64_keyval;
+    v->visitor.type_uint64 = qobject_input_type_uint64_keyval;
+    v->visitor.type_bool = qobject_input_type_bool_keyval;
+    v->visitor.type_str = qobject_input_type_str;
+    v->visitor.type_number = qobject_input_type_number_keyval;
+    v->visitor.type_any = qobject_input_type_any;
+    v->visitor.type_null = qobject_input_type_null;
+    v->visitor.type_size = qobject_input_type_size_keyval;
+    v->visitor.optional = qobject_input_optional;
+    v->visitor.free = qobject_input_free;
+
+    v->root = obj;
+    qobject_incref(obj);
+
+    return &v->visitor;
+}
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 8943cab..650637e 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -45,6 +45,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 keyval,
                                                  const char *json_string,
                                                  va_list *ap)
 {
@@ -53,11 +54,29 @@ 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);
+    if (keyval) {
+        data->qiv = qobject_input_visitor_new_keyval(data->obj);
+    } else {
+        data->qiv = qobject_input_visitor_new(data->obj);
+    }
     g_assert(data->qiv);
     return data->qiv;
 }
 
+static GCC_FMT_ATTR(3, 4)
+Visitor *visitor_input_test_init_full(TestInputVisitorData *data,
+                                      bool keyval,
+                                      const char *json_string, ...)
+{
+    Visitor *v;
+    va_list ap;
+
+    va_start(ap, json_string);
+    v = visitor_input_test_init_internal(data, keyval, 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, ...)
@@ -66,7 +85,7 @@ 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, false, json_string, &ap);
     va_end(ap);
     return v;
 }
@@ -81,7 +100,7 @@ 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, false, json_string, NULL);
 }
 
 static void test_visitor_in_int(TestInputVisitorData *data,
@@ -114,6 +133,43 @@ static void test_visitor_in_int_overflow(TestInputVisitorData *data,
     error_free_or_abort(&err);
 }
 
+static void test_visitor_in_int_keyval(TestInputVisitorData *data,
+                                       const void *unused)
+{
+    int64_t res = 0, value = -42;
+    Error *err = NULL;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, "%" PRId64, value);
+    visit_type_int(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+}
+
+static void test_visitor_in_int_str_keyval(TestInputVisitorData *data,
+                                           const void *unused)
+{
+    int64_t res = 0, value = -42;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, "\"-42\"");
+
+    visit_type_int(v, NULL, &res, &error_abort);
+    g_assert_cmpint(res, ==, value);
+}
+
+static void test_visitor_in_int_str_fail(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)
 {
@@ -126,6 +182,44 @@ static void test_visitor_in_bool(TestInputVisitorData *data,
     g_assert_cmpint(res, ==, true);
 }
 
+static void test_visitor_in_bool_keyval(TestInputVisitorData *data,
+                                        const void *unused)
+{
+    bool res = false;
+    Error *err = NULL;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, "true");
+
+    visit_type_bool(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+}
+
+static void test_visitor_in_bool_str_keyval(TestInputVisitorData *data,
+                                            const void *unused)
+{
+    bool res = false;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, "\"on\"");
+
+    visit_type_bool(v, NULL, &res, &error_abort);
+    g_assert_cmpint(res, ==, true);
+}
+
+static void test_visitor_in_bool_str_fail(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)
 {
@@ -138,6 +232,69 @@ static void test_visitor_in_number(TestInputVisitorData *data,
     g_assert_cmpfloat(res, ==, value);
 }
 
+static void test_visitor_in_number_keyval(TestInputVisitorData *data,
+                                          const void *unused)
+{
+    double res = 0, value = 3.14;
+    Error *err = NULL;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, "%f", value);
+
+    visit_type_number(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+}
+
+static void test_visitor_in_number_str_keyval(TestInputVisitorData *data,
+                                              const void *unused)
+{
+    double res = 0, value = 3.14;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, "\"3.14\"");
+
+    visit_type_number(v, NULL, &res, &error_abort);
+    g_assert_cmpfloat(res, ==, value);
+}
+
+static void test_visitor_in_number_str_fail(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_keyval(TestInputVisitorData *data,
+                                            const void *unused)
+{
+    uint64_t res, value = 500 * 1024 * 1024;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, "\"500M\"");
+
+    visit_type_size(v, NULL, &res, &error_abort);
+    g_assert_cmpfloat(res, ==, value);
+}
+
+static void test_visitor_in_size_str_fail(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)
 {
@@ -294,7 +451,8 @@ static void test_visitor_in_null(TestInputVisitorData *data,
      * when input is not null.
      */
 
-    v = visitor_input_test_init(data, "{ 'a': null, 'b': '', 'c': null }");
+    v = visitor_input_test_init_full(data, false,
+                                     "{ 'a': null, 'b': '' }");
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
     visit_type_null(v, "a", &error_abort);
     visit_type_null(v, "b", &err);
@@ -1068,10 +1226,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_keyval",
+                           NULL, test_visitor_in_int_keyval);
+    input_visitor_test_add("/visitor/input/int_str_keyval",
+                           NULL, test_visitor_in_int_str_keyval);
+    input_visitor_test_add("/visitor/input/int_str_fail",
+                           NULL, test_visitor_in_int_str_fail);
     input_visitor_test_add("/visitor/input/bool",
                            NULL, test_visitor_in_bool);
+    input_visitor_test_add("/visitor/input/bool_keyval",
+                           NULL, test_visitor_in_bool_keyval);
+    input_visitor_test_add("/visitor/input/bool_str_keyval",
+                           NULL, test_visitor_in_bool_str_keyval);
+    input_visitor_test_add("/visitor/input/bool_str_fail",
+                           NULL, test_visitor_in_bool_str_fail);
     input_visitor_test_add("/visitor/input/number",
                            NULL, test_visitor_in_number);
+    input_visitor_test_add("/visitor/input/number_keyval",
+                           NULL, test_visitor_in_number_keyval);
+    input_visitor_test_add("/visitor/input/number_str_keyval",
+                           NULL, test_visitor_in_number_str_keyval);
+    input_visitor_test_add("/visitor/input/number_str_fail",
+                           NULL, test_visitor_in_number_str_fail);
+    input_visitor_test_add("/visitor/input/size_str_keyval",
+                           NULL, test_visitor_in_size_str_keyval);
+    input_visitor_test_add("/visitor/input/size_str_fail",
+                           NULL, test_visitor_in_size_str_fail);
     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 04/24] qapi: qobject input visitor variant for use with keyval_parse()

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> 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().  autocast replaced by keyval in identifiers,
> noautocast replaced by fail in tests.
> 
> Fix qobject_input_type_uint64_keyval() not to reject '-', for QemuOpts
> compatibility: replace parse_uint_full() by open-coded
> parse_option_number().  The next commit will add suitable tests.
> Leave out the fancy ERANGE error reporting for now, but add a TODO
> comment.  Add it qobject_input_type_int64_keyval() and
> qobject_input_type_number_keyval(), too.
> 
> Open code parse_option_bool() and parse_option_size() so we have to
> call qobject_input_get_name() only when actually needed.  Again, leave
> out ERANGE error reporting for now.
> 
> QAPI/QMP downstream extension prefixes __RFQDN_ don't work, because
> keyval_parse() splits them at '.'.  Add a TODO comment there.
> 
> qobject_input_type_int64_keyval(), qobject_input_type_uint64_keyval(),
> qobject_input_type_number_keyval() tweaked for style.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 05/24] test-keyval: Cover use with qobject input visitor

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-keyval.c | 312 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 312 insertions(+)

diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index 27f6625..f6496d7 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -12,6 +12,8 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qemu/cutils.h"
 #include "qemu/option.h"
 
 static void test_keyval_parse(void)
@@ -171,10 +173,320 @@ static void test_keyval_parse(void)
     g_assert(!qdict);
 }
 
+static void test_keyval_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_keyval(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_keyval(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_keyval_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_keyval(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_keyval(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_keyval(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_keyval(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_keyval(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_keyval(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_keyval_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_keyval(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_keyval(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_keyval(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_keyval(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_keyval(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_keyval(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_keyval(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_keyval(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);
+}
+
+static void test_keyval_visit_dict(void)
+{
+    Error *err = NULL;
+    Visitor *v;
+    QDict *qdict;
+    int64_t i;
+
+    qdict = keyval_parse("a.b.c=1,a.b.c=2,d=3", NULL, &error_abort);
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_start_struct(v, "a", NULL, 0, &error_abort);
+    visit_start_struct(v, "b", NULL, 0, &error_abort);
+    visit_type_int(v, "c", &i, &error_abort);
+    g_assert_cmpint(i, ==, 2);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_type_int(v, "d", &i, &error_abort);
+    g_assert_cmpint(i, ==, 3);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    qdict = keyval_parse("a.b=", NULL, &error_abort);
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_start_struct(v, "a", NULL, 0, &error_abort);
+    visit_type_int(v, "c", &i, &err);   /* a.c missing */
+    error_free_or_abort(&err);
+    visit_check_struct(v, &err);
+    error_free_or_abort(&err);          /* a.b unexpected */
+    visit_end_struct(v, NULL);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+}
+
+static void test_keyval_visit_optional(void)
+{
+    Visitor *v;
+    QDict *qdict;
+    bool present;
+    int64_t i;
+
+    qdict = keyval_parse("a.b=1", NULL, &error_abort);
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_optional(v, "b", &present);
+    g_assert(!present);         /* b missing */
+    visit_optional(v, "a", &present);
+    g_assert(present);          /* a present */
+    visit_start_struct(v, "a", NULL, 0, &error_abort);
+    visit_optional(v, "b", &present);
+    g_assert(present);          /* a.b present */
+    visit_type_int(v, "b", &i, &error_abort);
+    g_assert_cmpint(i, ==, 1);
+    visit_optional(v, "a", &present);
+    g_assert(!present);         /* a.a missing */
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_check_struct(v, &error_abort);
+    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/visit/bool", test_keyval_visit_bool);
+    g_test_add_func("/keyval/visit/number", test_keyval_visit_number);
+    g_test_add_func("/keyval/visit/size", test_keyval_visit_size);
+    g_test_add_func("/keyval/visit/dict", test_keyval_visit_dict);
+    g_test_add_func("/keyval/visit/optional", test_keyval_visit_optional);
     g_test_run();
     return 0;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 05/24] test-keyval: Cover use with qobject input visitor

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

> +static void test_keyval_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_keyval(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",

That's 2^53+1, not 2^54. With the comment fixed:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 05/24] test-keyval: Cover use with qobject input visitor

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>> +static void test_keyval_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_keyval(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",
>
> That's 2^53+1, not 2^54. With the comment fixed:

D'oh!

> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Thanks!

[Qemu-devel] [PATCH 06/24] qapi: Factor out common part of qobject input visitor creation

[Markus Armbruster]

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

diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index d53bad3..e47615e 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -618,22 +618,34 @@ static void qobject_input_free(Visitor *v)
     g_free(qiv);
 }
 
+static QObjectInputVisitor *qobject_input_visitor_base_new(QObject *obj)
+{
+    QObjectInputVisitor *v = g_malloc0(sizeof(*v));
+
+    assert(obj);
+
+    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.check_list = qobject_input_check_list;
+    v->visitor.end_list = qobject_input_pop;
+    v->visitor.start_alternate = qobject_input_start_alternate;
+    v->visitor.optional = qobject_input_optional;
+    v->visitor.free = qobject_input_free;
+
+    v->root = obj;
+    qobject_incref(obj);
+
+    return v;
+}
+
 Visitor *qobject_input_visitor_new(QObject *obj)
 {
-    QObjectInputVisitor *v;
+    QObjectInputVisitor *v = qobject_input_visitor_base_new(obj);
 
-    assert(obj);
-    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.check_list = qobject_input_check_list;
-    v->visitor.end_list = qobject_input_pop;
-    v->visitor.start_alternate = qobject_input_start_alternate;
     v->visitor.type_int64 = qobject_input_type_int64;
     v->visitor.type_uint64 = qobject_input_type_uint64;
     v->visitor.type_bool = qobject_input_type_bool;
@@ -641,30 +653,14 @@ Visitor *qobject_input_visitor_new(QObject *obj)
     v->visitor.type_number = qobject_input_type_number;
     v->visitor.type_any = qobject_input_type_any;
     v->visitor.type_null = qobject_input_type_null;
-    v->visitor.optional = qobject_input_optional;
-    v->visitor.free = qobject_input_free;
-
-    v->root = obj;
-    qobject_incref(obj);
 
     return &v->visitor;
 }
 
 Visitor *qobject_input_visitor_new_keyval(QObject *obj)
 {
-    QObjectInputVisitor *v;
+    QObjectInputVisitor *v = qobject_input_visitor_base_new(obj);
 
-    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.check_list = qobject_input_check_list;
-    v->visitor.end_list = qobject_input_pop;
-    v->visitor.start_alternate = qobject_input_start_alternate;
     v->visitor.type_int64 = qobject_input_type_int64_keyval;
     v->visitor.type_uint64 = qobject_input_type_uint64_keyval;
     v->visitor.type_bool = qobject_input_type_bool_keyval;
@@ -673,11 +669,6 @@ Visitor *qobject_input_visitor_new_keyval(QObject *obj)
     v->visitor.type_any = qobject_input_type_any;
     v->visitor.type_null = qobject_input_type_null;
     v->visitor.type_size = qobject_input_type_size_keyval;
-    v->visitor.optional = qobject_input_optional;
-    v->visitor.free = qobject_input_free;
-
-    v->root = obj;
-    qobject_incref(obj);
 
     return &v->visitor;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 06/24] qapi: Factor out common part of qobject input visitor creation

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 07/24] qapi: Factor out common qobject_input_get_keyval()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@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 e47615e..3db5850 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -151,6 +151,28 @@ static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
     return obj;
 }
 
+static const char *qobject_input_get_keyval(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,
+                   full_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;
@@ -342,20 +364,13 @@ static void qobject_input_type_int64_keyval(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_keyval(qiv, name, errp);
 
-    if (!qobj) {
-        return;
-    }
-    qstr = qobject_to_qstring(qobj);
-    if (!qstr) {
-        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-                   full_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,
                    full_name(qiv, name), "integer");
@@ -387,20 +402,13 @@ static void qobject_input_type_uint64_keyval(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_keyval(qiv, name, errp);
 
-    if (!qobj) {
-        return;
-    }
-    qstr = qobject_to_qstring(qobj);
-    if (!qstr) {
-        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-                   full_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,
                    full_name(qiv, name), "integer");
@@ -431,21 +439,12 @@ static void qobject_input_type_bool_keyval(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_keyval(qiv, name, errp);
 
-    if (!qobj) {
-        return;
-    }
-    qstr = qobject_to_qstring(qobj);
-    if (!qstr) {
-        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-                   full_name(qiv, name), "string");
+    if (!str) {
         return;
     }
 
-    str = qstring_get_str(qstr);
     if (!strcmp(str, "on")) {
         *obj = true;
     } else if (!strcmp(str, "off")) {
@@ -508,22 +507,13 @@ static void qobject_input_type_number_keyval(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_keyval(qiv, name, errp);
     char *endp;
 
-    if (!qobj) {
-        return;
-    }
-    qstr = qobject_to_qstring(qobj);
-    if (!qstr) {
-        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-                   full_name(qiv, name), "string");
+    if (!str) {
         return;
     }
 
-    str = qstring_get_str(qstr);
     errno = 0;
     *obj = strtod(str, &endp);
     if (errno || endp == str || *endp) {
@@ -567,20 +557,13 @@ static void qobject_input_type_size_keyval(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_keyval(qiv, name, errp);
 
-    if (!qobj) {
-        return;
-    }
-    qstr = qobject_to_qstring(qobj);
-    if (!qstr) {
-        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-                   full_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,
                    full_name(qiv, name), "size");
-- 
2.7.4

[Qemu-devel] [PATCH 08/24] qobject: Propagate parse errors through qobject_from_jsonv()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/qmp/qjson.h           |  3 ++-
 qobject/qjson.c                    | 12 ++++++++----
 tests/libqtest.c                   |  2 +-
 tests/test-qobject-input-visitor.c |  2 +-
 4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index 02b1f2c..6fe42d0 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -19,7 +19,8 @@
 
 QObject *qobject_from_json(const char *string);
 QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
-QObject *qobject_from_jsonv(const char *string, va_list *ap) GCC_FMT_ATTR(1, 0);
+QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
+    GCC_FMT_ATTR(1, 0);
 
 QString *qobject_to_json(const QObject *obj);
 QString *qobject_to_json_pretty(const QObject *obj);
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 9a0de89..339c9f7 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -12,6 +12,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "qapi/qmp/json-lexer.h"
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/json-streamer.h"
@@ -24,15 +25,17 @@ typedef struct JSONParsingState
     JSONMessageParser parser;
     va_list *ap;
     QObject *result;
+    Error *err;
 } JSONParsingState;
 
 static void parse_json(JSONMessageParser *parser, GQueue *tokens)
 {
     JSONParsingState *s = container_of(parser, JSONParsingState, parser);
-    s->result = json_parser_parse(tokens, s->ap);
+
+    s->result = json_parser_parse_err(tokens, s->ap, &s->err);
 }
 
-QObject *qobject_from_jsonv(const char *string, va_list *ap)
+QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
 {
     JSONParsingState state = {};
 
@@ -43,12 +46,13 @@ QObject *qobject_from_jsonv(const char *string, va_list *ap)
     json_message_parser_flush(&state.parser);
     json_message_parser_destroy(&state.parser);
 
+    error_propagate(errp, state.err);
     return state.result;
 }
 
 QObject *qobject_from_json(const char *string)
 {
-    return qobject_from_jsonv(string, NULL);
+    return qobject_from_jsonv(string, NULL, NULL);
 }
 
 /*
@@ -61,7 +65,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
     va_list ap;
 
     va_start(ap, string);
-    obj = qobject_from_jsonv(string, &ap);
+    obj = qobject_from_jsonv(string, &ap, NULL);
     va_end(ap);
 
     assert(obj != NULL);
diff --git a/tests/libqtest.c b/tests/libqtest.c
index cf27afc..683d5e3 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -442,7 +442,7 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
      * is an array type.
      */
     va_copy(ap_copy, ap);
-    qobj = qobject_from_jsonv(fmt, &ap_copy);
+    qobj = qobject_from_jsonv(fmt, &ap_copy, NULL);
     va_end(ap_copy);
 
     /* No need to send anything for an empty QObject.  */
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 650637e..44885ee 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -51,7 +51,7 @@ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data,
 {
     visitor_input_teardown(data, NULL);
 
-    data->obj = qobject_from_jsonv(json_string, ap);
+    data->obj = qobject_from_jsonv(json_string, ap, NULL);
     g_assert(data->obj);
 
     if (keyval) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH 08/24] qobject: Propagate parse errors through qobject_from_jsonv()

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/qmp/qjson.h           |  3 ++-
>  qobject/qjson.c                    | 12 ++++++++----
>  tests/libqtest.c                   |  2 +-
>  tests/test-qobject-input-visitor.c |  2 +-
>  4 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
> index 02b1f2c..6fe42d0 100644
> --- a/include/qapi/qmp/qjson.h
> +++ b/include/qapi/qmp/qjson.h
> @@ -19,7 +19,8 @@
>  
>  QObject *qobject_from_json(const char *string);
>  QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
> -QObject *qobject_from_jsonv(const char *string, va_list *ap) GCC_FMT_ATTR(1, 0);
> +QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
> +    GCC_FMT_ATTR(1, 0);
>  
>  QString *qobject_to_json(const QObject *obj);
>  QString *qobject_to_json_pretty(const QObject *obj);
> diff --git a/qobject/qjson.c b/qobject/qjson.c
> index 9a0de89..339c9f7 100644
> --- a/qobject/qjson.c
> +++ b/qobject/qjson.c
> @@ -12,6 +12,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qapi/error.h"
>  #include "qapi/qmp/json-lexer.h"
>  #include "qapi/qmp/json-parser.h"
>  #include "qapi/qmp/json-streamer.h"
> @@ -24,15 +25,17 @@ typedef struct JSONParsingState
>      JSONMessageParser parser;
>      va_list *ap;
>      QObject *result;
> +    Error *err;
>  } JSONParsingState;
>  
>  static void parse_json(JSONMessageParser *parser, GQueue *tokens)
>  {
>      JSONParsingState *s = container_of(parser, JSONParsingState, parser);
> -    s->result = json_parser_parse(tokens, s->ap);
> +
> +    s->result = json_parser_parse_err(tokens, s->ap, &s->err);
>  }
>  
> -QObject *qobject_from_jsonv(const char *string, va_list *ap)
> +QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
>  {
>      JSONParsingState state = {};
>  
> @@ -43,12 +46,13 @@ QObject *qobject_from_jsonv(const char *string, va_list *ap)
>      json_message_parser_flush(&state.parser);
>      json_message_parser_destroy(&state.parser);
>  
> +    error_propagate(errp, state.err);
>      return state.result;
>  }
>  
>  QObject *qobject_from_json(const char *string)
>  {
> -    return qobject_from_jsonv(string, NULL);
> +    return qobject_from_jsonv(string, NULL, NULL);
>  }
>  
>  /*
> @@ -61,7 +65,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
>      va_list ap;
>  
>      va_start(ap, string);
> -    obj = qobject_from_jsonv(string, &ap);
> +    obj = qobject_from_jsonv(string, &ap, NULL);
>      va_end(ap);
>  
>      assert(obj != NULL);
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index cf27afc..683d5e3 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -442,7 +442,7 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
>       * is an array type.
>       */
>      va_copy(ap_copy, ap);
> -    qobj = qobject_from_jsonv(fmt, &ap_copy);
> +    qobj = qobject_from_jsonv(fmt, &ap_copy, NULL);
>      va_end(ap_copy);

&error_abort might be appropriate here.

>      /* No need to send anything for an empty QObject.  */
> diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
> index 650637e..44885ee 100644
> --- a/tests/test-qobject-input-visitor.c
> +++ b/tests/test-qobject-input-visitor.c
> @@ -51,7 +51,7 @@ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data,
>  {
>      visitor_input_teardown(data, NULL);
>  
> -    data->obj = qobject_from_jsonv(json_string, ap);
> +    data->obj = qobject_from_jsonv(json_string, ap, NULL);
>      g_assert(data->obj);

And here, too.

Changed or unchanged:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 09/24] libqtest: Fix qmp() & friends to abort on JSON parse errors

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/libqtest.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 683d5e3..bb444d5 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -21,6 +21,7 @@
 #include <sys/wait.h>
 #include <sys/un.h>
 
+#include "qapi/error.h"
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/json-streamer.h"
 #include "qapi/qmp/qjson.h"
@@ -442,7 +443,7 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
      * is an array type.
      */
     va_copy(ap_copy, ap);
-    qobj = qobject_from_jsonv(fmt, &ap_copy, NULL);
+    qobj = qobject_from_jsonv(fmt, &ap_copy, &error_abort);
     va_end(ap_copy);
 
     /* No need to send anything for an empty QObject.  */
-- 
2.7.4

Re: [Qemu-devel] [PATCH 09/24] libqtest: Fix qmp() & friends to abort on JSON parse errors

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Aha, I was just too quick! :-)

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 09/24] libqtest: Fix qmp() & friends to abort on JSON parse errors

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Aha, I was just too quick! :-)

I'll add something to the previous commit's message.

> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Thanks!

[Qemu-devel] [PATCH 10/24] qjson: Abort earlier on qobject_from_jsonf() misuse

[Markus Armbruster]

Ignoring errors first, then asserting success is suboptimal.  Pass
&error_abort instead, so we abort earlier, and hopefully get more
useful clues on what's wrong.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qobject/qjson.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qobject/qjson.c b/qobject/qjson.c
index 339c9f7..c98d6a7 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -65,7 +65,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
     va_list ap;
 
     va_start(ap, string);
-    obj = qobject_from_jsonv(string, &ap, NULL);
+    obj = qobject_from_jsonv(string, &ap, &error_abort);
     va_end(ap);
 
     assert(obj != NULL);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 10/24] qjson: Abort earlier on qobject_from_jsonf() misuse

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Ignoring errors first, then asserting success is suboptimal.  Pass
> &error_abort instead, so we abort earlier, and hopefully get more
> useful clues on what's wrong.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 11/24] test-qobject-input-visitor: Abort earlier on bad test input

[Markus Armbruster]

visitor_input_test_init_internal() parses test input with
qobject_from_jsonv(), and asserts it succeeds.  Pass &error_abort for
good measure.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qobject-input-visitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 44885ee..746dfa3 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -51,7 +51,7 @@ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data,
 {
     visitor_input_teardown(data, NULL);
 
-    data->obj = qobject_from_jsonv(json_string, ap, NULL);
+    data->obj = qobject_from_jsonv(json_string, ap, &error_abort);
     g_assert(data->obj);
 
     if (keyval) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH 11/24] test-qobject-input-visitor: Abort earlier on bad test input

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> visitor_input_test_init_internal() parses test input with
> qobject_from_jsonv(), and asserts it succeeds.  Pass &error_abort for
> good measure.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 12/24] qobject: Propagate parse errors through qobject_from_json()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c                            |  2 +-
 include/qapi/qmp/qjson.h           |  5 +--
 monitor.c                          |  2 +-
 qobject/qjson.c                    |  4 +--
 tests/check-qjson.c                | 62 +++++++++++++++++++-------------------
 tests/test-visitor-serialization.c |  2 +-
 6 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/block.c b/block.c
index 3c36af5..aa6790c 100644
--- a/block.c
+++ b/block.c
@@ -1163,7 +1163,7 @@ static QDict *parse_json_filename(const char *filename, Error **errp)
     ret = strstart(filename, "json:", &filename);
     assert(ret);
 
-    options_obj = qobject_from_json(filename);
+    options_obj = qobject_from_json(filename, NULL);
     if (!options_obj) {
         error_setg(errp, "Could not parse the JSON options");
         return NULL;
diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index 6fe42d0..8568f2d 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -17,8 +17,9 @@
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp/qstring.h"
 
-QObject *qobject_from_json(const char *string);
-QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
+QObject *qobject_from_json(const char *string, Error **errp);
+QObject *qobject_from_jsonf(const char *string, ...)
+    GCC_FMT_ATTR(1, 2);
 QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
     GCC_FMT_ATTR(1, 0);
 
diff --git a/monitor.c b/monitor.c
index 97b73ab..858bcda 100644
--- a/monitor.c
+++ b/monitor.c
@@ -950,7 +950,7 @@ EventInfoList *qmp_query_events(Error **errp)
 static void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
                                  Error **errp)
 {
-    *ret_data = qobject_from_json(qmp_schema_json);
+    *ret_data = qobject_from_json(qmp_schema_json, NULL);
 }
 
 /*
diff --git a/qobject/qjson.c b/qobject/qjson.c
index c98d6a7..b2f3bfe 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -50,9 +50,9 @@ QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
     return state.result;
 }
 
-QObject *qobject_from_json(const char *string)
+QObject *qobject_from_json(const char *string, Error **errp)
 {
-    return qobject_from_jsonv(string, NULL, NULL);
+    return qobject_from_jsonv(string, NULL, errp);
 }
 
 /*
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index e6d6935..aa63758 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -53,7 +53,7 @@ static void escaped_string(void)
         QObject *obj;
         QString *str;
 
-        obj = qobject_from_json(test_cases[i].encoded);
+        obj = qobject_from_json(test_cases[i].encoded, NULL);
         str = qobject_to_qstring(obj);
         g_assert(str);
         g_assert_cmpstr(qstring_get_str(str), ==, test_cases[i].decoded);
@@ -85,7 +85,7 @@ static void simple_string(void)
         QObject *obj;
         QString *str;
 
-        obj = qobject_from_json(test_cases[i].encoded);
+        obj = qobject_from_json(test_cases[i].encoded, NULL);
         str = qobject_to_qstring(obj);
         g_assert(str);
         g_assert(strcmp(qstring_get_str(str), test_cases[i].decoded) == 0);
@@ -116,7 +116,7 @@ static void single_quote_string(void)
         QObject *obj;
         QString *str;
 
-        obj = qobject_from_json(test_cases[i].encoded);
+        obj = qobject_from_json(test_cases[i].encoded, NULL);
         str = qobject_to_qstring(obj);
         g_assert(str);
         g_assert(strcmp(qstring_get_str(str), test_cases[i].decoded) == 0);
@@ -809,7 +809,7 @@ static void utf8_string(void)
         utf8_in = test_cases[i].utf8_in ?: test_cases[i].utf8_out;
         json_out = test_cases[i].json_out ?: test_cases[i].json_in;
 
-        obj = qobject_from_json(json_in);
+        obj = qobject_from_json(json_in, NULL);
         if (utf8_out) {
             str = qobject_to_qstring(obj);
             g_assert(str);
@@ -836,7 +836,7 @@ static void utf8_string(void)
          * FIXME Enable once these bugs have been fixed.
          */
         if (0 && json_out != json_in) {
-            obj = qobject_from_json(json_out);
+            obj = qobject_from_json(json_out, NULL);
             str = qobject_to_qstring(obj);
             g_assert(str);
             g_assert_cmpstr(qstring_get_str(str), ==, utf8_out);
@@ -886,7 +886,7 @@ static void simple_number(void)
     for (i = 0; test_cases[i].encoded; i++) {
         QInt *qint;
 
-        qint = qobject_to_qint(qobject_from_json(test_cases[i].encoded));
+        qint = qobject_to_qint(qobject_from_json(test_cases[i].encoded, NULL));
         g_assert(qint);
         g_assert(qint_get_int(qint) == test_cases[i].decoded);
         if (test_cases[i].skip == 0) {
@@ -920,7 +920,7 @@ static void float_number(void)
         QObject *obj;
         QFloat *qfloat;
 
-        obj = qobject_from_json(test_cases[i].encoded);
+        obj = qobject_from_json(test_cases[i].encoded, NULL);
         qfloat = qobject_to_qfloat(obj);
         g_assert(qfloat);
         g_assert(qfloat_get_double(qfloat) == test_cases[i].decoded);
@@ -965,7 +965,7 @@ static void keyword_literal(void)
     QObject *null;
     QString *str;
 
-    obj = qobject_from_json("true");
+    obj = qobject_from_json("true", NULL);
     qbool = qobject_to_qbool(obj);
     g_assert(qbool);
     g_assert(qbool_get_bool(qbool) == true);
@@ -976,7 +976,7 @@ static void keyword_literal(void)
 
     QDECREF(qbool);
 
-    obj = qobject_from_json("false");
+    obj = qobject_from_json("false", NULL);
     qbool = qobject_to_qbool(obj);
     g_assert(qbool);
     g_assert(qbool_get_bool(qbool) == false);
@@ -998,7 +998,7 @@ static void keyword_literal(void)
     g_assert(qbool_get_bool(qbool) == true);
     QDECREF(qbool);
 
-    obj = qobject_from_json("null");
+    obj = qobject_from_json("null", NULL);
     g_assert(obj != NULL);
     g_assert(qobject_type(obj) == QTYPE_QNULL);
 
@@ -1134,13 +1134,13 @@ static void simple_dict(void)
         QObject *obj;
         QString *str;
 
-        obj = qobject_from_json(test_cases[i].encoded);
+        obj = qobject_from_json(test_cases[i].encoded, NULL);
         g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
 
         str = qobject_to_json(obj);
         qobject_decref(obj);
 
-        obj = qobject_from_json(qstring_get_str(str));
+        obj = qobject_from_json(qstring_get_str(str), NULL);
         g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
         qobject_decref(obj);
         QDECREF(str);
@@ -1192,7 +1192,7 @@ static void large_dict(void)
     QObject *obj;
 
     gen_test_json(gstr, 10, 100);
-    obj = qobject_from_json(gstr->str);
+    obj = qobject_from_json(gstr->str, NULL);
     g_assert(obj != NULL);
 
     qobject_decref(obj);
@@ -1243,13 +1243,13 @@ static void simple_list(void)
         QObject *obj;
         QString *str;
 
-        obj = qobject_from_json(test_cases[i].encoded);
+        obj = qobject_from_json(test_cases[i].encoded, NULL);
         g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
 
         str = qobject_to_json(obj);
         qobject_decref(obj);
 
-        obj = qobject_from_json(qstring_get_str(str));
+        obj = qobject_from_json(qstring_get_str(str), NULL);
         g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
         qobject_decref(obj);
         QDECREF(str);
@@ -1305,13 +1305,13 @@ static void simple_whitespace(void)
         QObject *obj;
         QString *str;
 
-        obj = qobject_from_json(test_cases[i].encoded);
+        obj = qobject_from_json(test_cases[i].encoded, NULL);
         g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
 
         str = qobject_to_json(obj);
         qobject_decref(obj);
 
-        obj = qobject_from_json(qstring_get_str(str));
+        obj = qobject_from_json(qstring_get_str(str), NULL);
         g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
 
         qobject_decref(obj);
@@ -1332,7 +1332,7 @@ static void simple_varargs(void)
                         {}})),
             {}}));
 
-    embedded_obj = qobject_from_json("[32, 42]");
+    embedded_obj = qobject_from_json("[32, 42]", NULL);
     g_assert(embedded_obj != NULL);
 
     obj = qobject_from_jsonf("[%d, 2, %p]", 1, embedded_obj);
@@ -1345,67 +1345,67 @@ static void empty_input(void)
 {
     const char *empty = "";
 
-    QObject *obj = qobject_from_json(empty);
+    QObject *obj = qobject_from_json(empty, NULL);
     g_assert(obj == NULL);
 }
 
 static void unterminated_string(void)
 {
-    QObject *obj = qobject_from_json("\"abc");
+    QObject *obj = qobject_from_json("\"abc", NULL);
     g_assert(obj == NULL);
 }
 
 static void unterminated_sq_string(void)
 {
-    QObject *obj = qobject_from_json("'abc");
+    QObject *obj = qobject_from_json("'abc", NULL);
     g_assert(obj == NULL);
 }
 
 static void unterminated_escape(void)
 {
-    QObject *obj = qobject_from_json("\"abc\\\"");
+    QObject *obj = qobject_from_json("\"abc\\\"", NULL);
     g_assert(obj == NULL);
 }
 
 static void unterminated_array(void)
 {
-    QObject *obj = qobject_from_json("[32");
+    QObject *obj = qobject_from_json("[32", NULL);
     g_assert(obj == NULL);
 }
 
 static void unterminated_array_comma(void)
 {
-    QObject *obj = qobject_from_json("[32,");
+    QObject *obj = qobject_from_json("[32,", NULL);
     g_assert(obj == NULL);
 }
 
 static void invalid_array_comma(void)
 {
-    QObject *obj = qobject_from_json("[32,}");
+    QObject *obj = qobject_from_json("[32,}", NULL);
     g_assert(obj == NULL);
 }
 
 static void unterminated_dict(void)
 {
-    QObject *obj = qobject_from_json("{'abc':32");
+    QObject *obj = qobject_from_json("{'abc':32", NULL);
     g_assert(obj == NULL);
 }
 
 static void unterminated_dict_comma(void)
 {
-    QObject *obj = qobject_from_json("{'abc':32,");
+    QObject *obj = qobject_from_json("{'abc':32,", NULL);
     g_assert(obj == NULL);
 }
 
 static void invalid_dict_comma(void)
 {
-    QObject *obj = qobject_from_json("{'abc':32,}");
+    QObject *obj = qobject_from_json("{'abc':32,}", NULL);
     g_assert(obj == NULL);
 }
 
 static void unterminated_literal(void)
 {
-    QObject *obj = qobject_from_json("nul");
+    QObject *obj = qobject_from_json("nul", NULL);
     g_assert(obj == NULL);
 }
 
@@ -1425,11 +1425,11 @@ static void limits_nesting(void)
     char buf[2 * (max_nesting + 1) + 1];
     QObject *obj;
 
-    obj = qobject_from_json(make_nest(buf, max_nesting));
+    obj = qobject_from_json(make_nest(buf, max_nesting), NULL);
     g_assert(obj != NULL);
     qobject_decref(obj);
 
-    obj = qobject_from_json(make_nest(buf, max_nesting + 1));
+    obj = qobject_from_json(make_nest(buf, max_nesting + 1), NULL);
     g_assert(obj == NULL);
 }
 
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index c7e64f0..37dff41 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -1037,7 +1037,7 @@ static void qmp_deserialize(void **native_out, void *datap,
     visit_complete(d->qov, &d->obj);
     obj_orig = d->obj;
     output_json = qobject_to_json(obj_orig);
-    obj = qobject_from_json(qstring_get_str(output_json));
+    obj = qobject_from_json(qstring_get_str(output_json), NULL);
 
     QDECREF(output_json);
     d->qiv = qobject_input_visitor_new(obj);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 12/24] qobject: Propagate parse errors through qobject_from_json()

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 12/24] qobject: Propagate parse errors through qobject_from_json()

[Eric Blake]

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

On 02/27/2017 05:20 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c                            |  2 +-
>  include/qapi/qmp/qjson.h           |  5 +--
>  monitor.c                          |  2 +-
>  qobject/qjson.c                    |  4 +--
>  tests/check-qjson.c                | 62 +++++++++++++++++++-------------------
>  tests/test-visitor-serialization.c |  2 +-
>  6 files changed, 39 insertions(+), 38 deletions(-)
> 

As with 8/24, this would be a good place for the commit message to
mention that this patch adds the parameter and mechanically adjusts
callers with minimal semantic changes, but that later patches will take
advantage of the parameter.

> +++ b/include/qapi/qmp/qjson.h
> @@ -17,8 +17,9 @@
>  #include "qapi/qmp/qobject.h"
>  #include "qapi/qmp/qstring.h"
>  
> -QObject *qobject_from_json(const char *string);
> -QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
> +QObject *qobject_from_json(const char *string, Error **errp);

The real change here, vs.

> +QObject *qobject_from_jsonf(const char *string, ...)
> +    GCC_FMT_ATTR(1, 2);

formatting.  Should the formatting be hoisted earlier in the series,
when you first touch qobject_from_jsonv?

>  QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
>      GCC_FMT_ATTR(1, 0);

This makes qobject_from_jsonf() and qobject_from_jsonv() asymmetric
(only one of the two can report errors); and looks a bit weird to have a
va_list not as the last argument (as it is, a 'va_list *' argument is
already weird).  If symmetry is important, we can put errp prior to the
.../ap argument (then both forms have an errp pointer).  But I don't
think it matters in the context of this series.  If you DO change it,
though, then 8/24 would be the place to tweak it.

At one point, I posted a series that removed all uses of
qobject_from_json[fv] (leaving just qobject_from_json); at the time,
there was not a strong opinion on whether the series was worthwhile, but
if we want it, I'm fine rebasing on top of this.  (One argument in favor
of my series would be getting rid of the weird 'va_list *' argument).

> +++ b/qobject/qjson.c
> @@ -50,9 +50,9 @@ QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
>      return state.result;
>  }
>  
> -QObject *qobject_from_json(const char *string)
> +QObject *qobject_from_json(const char *string, Error **errp)
>  {
> -    return qobject_from_jsonv(string, NULL, NULL);
> +    return qobject_from_jsonv(string, NULL, errp);

Of course, if you rebase the series to rearrange where errp appears in
relation to va_list, then be sure this changes to match (the compiler
may or may not flag it, if va_list looks too much like void*).

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

-- 
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 12/24] qobject: Propagate parse errors through qobject_from_json()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/27/2017 05:20 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block.c                            |  2 +-
>>  include/qapi/qmp/qjson.h           |  5 +--
>>  monitor.c                          |  2 +-
>>  qobject/qjson.c                    |  4 +--
>>  tests/check-qjson.c                | 62 +++++++++++++++++++-------------------
>>  tests/test-visitor-serialization.c |  2 +-
>>  6 files changed, 39 insertions(+), 38 deletions(-)
>> 
>
> As with 8/24, this would be a good place for the commit message to
> mention that this patch adds the parameter and mechanically adjusts
> callers with minimal semantic changes, but that later patches will take
> advantage of the parameter.

Already done :)

>> +++ b/include/qapi/qmp/qjson.h
>> @@ -17,8 +17,9 @@
>>  #include "qapi/qmp/qobject.h"
>>  #include "qapi/qmp/qstring.h"
>>  
>> -QObject *qobject_from_json(const char *string);
>> -QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
>> +QObject *qobject_from_json(const char *string, Error **errp);
>
> The real change here, vs.
>
>> +QObject *qobject_from_jsonf(const char *string, ...)
>> +    GCC_FMT_ATTR(1, 2);
>
> formatting.  Should the formatting be hoisted earlier in the series,
> when you first touch qobject_from_jsonv?

Either that, or simply drop this change.  I'd say drop.

>>  QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
>>      GCC_FMT_ATTR(1, 0);
>
> This makes qobject_from_jsonf() and qobject_from_jsonv() asymmetric
> (only one of the two can report errors);

Yes, but they've always been asymmetric: only qobject_from_jsonv() can
fail, qobject_from_jsonf() asserts instead.

>                                          and looks a bit weird to have a
> va_list not as the last argument (as it is, a 'va_list *' argument is
> already weird).

The weirdness precedes the patch, though.

>                  If symmetry is important, we can put errp prior to the
> .../ap argument (then both forms have an errp pointer).  But I don't
> think it matters in the context of this series.  If you DO change it,
> though, then 8/24 would be the place to tweak it.

I'd prefer not to change it now.  Perhaps we can improve things around
here in separate patches.

> At one point, I posted a series that removed all uses of
> qobject_from_json[fv] (leaving just qobject_from_json); at the time,
> there was not a strong opinion on whether the series was worthwhile, but
> if we want it, I'm fine rebasing on top of this.  (One argument in favor
> of my series would be getting rid of the weird 'va_list *' argument).

I didn't like it at the time.  Perhaps I should have a second look.

>> +++ b/qobject/qjson.c
>> @@ -50,9 +50,9 @@ QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
>>      return state.result;
>>  }
>>  
>> -QObject *qobject_from_json(const char *string)
>> +QObject *qobject_from_json(const char *string, Error **errp)
>>  {
>> -    return qobject_from_jsonv(string, NULL, NULL);
>> +    return qobject_from_jsonv(string, NULL, errp);
>
> Of course, if you rebase the series to rearrange where errp appears in
> relation to va_list, then be sure this changes to match (the compiler
> may or may not flag it, if va_list looks too much like void*).
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

[Qemu-devel] [PATCH 13/24] block: More detailed syntax error reporting for JSON filenames

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index aa6790c..de7d530 100644
--- a/block.c
+++ b/block.c
@@ -1163,9 +1163,14 @@ static QDict *parse_json_filename(const char *filename, Error **errp)
     ret = strstart(filename, "json:", &filename);
     assert(ret);
 
-    options_obj = qobject_from_json(filename, NULL);
+    options_obj = qobject_from_json(filename, errp);
     if (!options_obj) {
-        error_setg(errp, "Could not parse the JSON options");
+        /* Work around qobject_from_json() lossage TODO fix that */
+        if (errp && !*errp) {
+            error_setg(errp, "Could not parse the JSON options");
+            return NULL;
+        }
+        error_prepend(errp, "Could not parse the JSON options: ");
         return NULL;
     }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 13/24] block: More detailed syntax error reporting for JSON filenames

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 14/24] check-qjson: Test errors from qobject_from_json()

[Markus Armbruster]

Pass &error_abort with known-good input.  Else pass &err and check
what comes back.  This demonstrates that the parser fails silently for
many errors.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/check-qjson.c | 88 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 56 insertions(+), 32 deletions(-)

diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index aa63758..963dd46 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -10,8 +10,10 @@
  * See the COPYING.LIB file in the top-level directory.
  *
  */
+
 #include "qemu/osdep.h"
 
+#include "qapi/error.h"
 #include "qapi/qmp/types.h"
 #include "qapi/qmp/qjson.h"
 #include "qemu-common.h"
@@ -53,7 +55,7 @@ static void escaped_string(void)
         QObject *obj;
         QString *str;
 
-        obj = qobject_from_json(test_cases[i].encoded, NULL);
+        obj = qobject_from_json(test_cases[i].encoded, &error_abort);
         str = qobject_to_qstring(obj);
         g_assert(str);
         g_assert_cmpstr(qstring_get_str(str), ==, test_cases[i].decoded);
@@ -85,7 +87,7 @@ static void simple_string(void)
         QObject *obj;
         QString *str;
 
-        obj = qobject_from_json(test_cases[i].encoded, NULL);
+        obj = qobject_from_json(test_cases[i].encoded, &error_abort);
         str = qobject_to_qstring(obj);
         g_assert(str);
         g_assert(strcmp(qstring_get_str(str), test_cases[i].decoded) == 0);
@@ -116,7 +118,7 @@ static void single_quote_string(void)
         QObject *obj;
         QString *str;
 
-        obj = qobject_from_json(test_cases[i].encoded, NULL);
+        obj = qobject_from_json(test_cases[i].encoded, &error_abort);
         str = qobject_to_qstring(obj);
         g_assert(str);
         g_assert(strcmp(qstring_get_str(str), test_cases[i].decoded) == 0);
@@ -809,7 +811,7 @@ static void utf8_string(void)
         utf8_in = test_cases[i].utf8_in ?: test_cases[i].utf8_out;
         json_out = test_cases[i].json_out ?: test_cases[i].json_in;
 
-        obj = qobject_from_json(json_in, NULL);
+        obj = qobject_from_json(json_in, utf8_out ? &error_abort : NULL);
         if (utf8_out) {
             str = qobject_to_qstring(obj);
             g_assert(str);
@@ -836,7 +838,7 @@ static void utf8_string(void)
          * FIXME Enable once these bugs have been fixed.
          */
         if (0 && json_out != json_in) {
-            obj = qobject_from_json(json_out, NULL);
+            obj = qobject_from_json(json_out, &error_abort);
             str = qobject_to_qstring(obj);
             g_assert(str);
             g_assert_cmpstr(qstring_get_str(str), ==, utf8_out);
@@ -886,7 +888,8 @@ static void simple_number(void)
     for (i = 0; test_cases[i].encoded; i++) {
         QInt *qint;
 
-        qint = qobject_to_qint(qobject_from_json(test_cases[i].encoded, NULL));
+        qint = qobject_to_qint(qobject_from_json(test_cases[i].encoded,
+                                                 &error_abort));
         g_assert(qint);
         g_assert(qint_get_int(qint) == test_cases[i].decoded);
         if (test_cases[i].skip == 0) {
@@ -920,7 +923,7 @@ static void float_number(void)
         QObject *obj;
         QFloat *qfloat;
 
-        obj = qobject_from_json(test_cases[i].encoded, NULL);
+        obj = qobject_from_json(test_cases[i].encoded, &error_abort);
         qfloat = qobject_to_qfloat(obj);
         g_assert(qfloat);
         g_assert(qfloat_get_double(qfloat) == test_cases[i].decoded);
@@ -965,7 +968,7 @@ static void keyword_literal(void)
     QObject *null;
     QString *str;
 
-    obj = qobject_from_json("true", NULL);
+    obj = qobject_from_json("true", &error_abort);
     qbool = qobject_to_qbool(obj);
     g_assert(qbool);
     g_assert(qbool_get_bool(qbool) == true);
@@ -976,7 +979,7 @@ static void keyword_literal(void)
 
     QDECREF(qbool);
 
-    obj = qobject_from_json("false", NULL);
+    obj = qobject_from_json("false", &error_abort);
     qbool = qobject_to_qbool(obj);
     g_assert(qbool);
     g_assert(qbool_get_bool(qbool) == false);
@@ -998,7 +1001,7 @@ static void keyword_literal(void)
     g_assert(qbool_get_bool(qbool) == true);
     QDECREF(qbool);
 
-    obj = qobject_from_json("null", NULL);
+    obj = qobject_from_json("null", &error_abort);
     g_assert(obj != NULL);
     g_assert(qobject_type(obj) == QTYPE_QNULL);
 
@@ -1134,13 +1137,13 @@ static void simple_dict(void)
         QObject *obj;
         QString *str;
 
-        obj = qobject_from_json(test_cases[i].encoded, NULL);
+        obj = qobject_from_json(test_cases[i].encoded, &error_abort);
         g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
 
         str = qobject_to_json(obj);
         qobject_decref(obj);
 
-        obj = qobject_from_json(qstring_get_str(str), NULL);
+        obj = qobject_from_json(qstring_get_str(str), &error_abort);
         g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
         qobject_decref(obj);
         QDECREF(str);
@@ -1192,7 +1195,7 @@ static void large_dict(void)
     QObject *obj;
 
     gen_test_json(gstr, 10, 100);
-    obj = qobject_from_json(gstr->str, NULL);
+    obj = qobject_from_json(gstr->str, &error_abort);
     g_assert(obj != NULL);
 
     qobject_decref(obj);
@@ -1243,13 +1246,13 @@ static void simple_list(void)
         QObject *obj;
         QString *str;
 
-        obj = qobject_from_json(test_cases[i].encoded, NULL);
+        obj = qobject_from_json(test_cases[i].encoded, &error_abort);
         g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
 
         str = qobject_to_json(obj);
         qobject_decref(obj);
 
-        obj = qobject_from_json(qstring_get_str(str), NULL);
+        obj = qobject_from_json(qstring_get_str(str), &error_abort);
         g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
         qobject_decref(obj);
         QDECREF(str);
@@ -1305,13 +1308,13 @@ static void simple_whitespace(void)
         QObject *obj;
         QString *str;
 
-        obj = qobject_from_json(test_cases[i].encoded, NULL);
+        obj = qobject_from_json(test_cases[i].encoded, &error_abort);
         g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
 
         str = qobject_to_json(obj);
         qobject_decref(obj);
 
-        obj = qobject_from_json(qstring_get_str(str), NULL);
+        obj = qobject_from_json(qstring_get_str(str), &error_abort);
         g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
 
         qobject_decref(obj);
@@ -1332,7 +1335,7 @@ static void simple_varargs(void)
                         {}})),
             {}}));
 
-    embedded_obj = qobject_from_json("[32, 42]", NULL);
+    embedded_obj = qobject_from_json("[32, 42]", &error_abort);
     g_assert(embedded_obj != NULL);
 
     obj = qobject_from_jsonf("[%d, 2, %p]", 1, embedded_obj);
@@ -1344,68 +1347,87 @@ static void simple_varargs(void)
 static void empty_input(void)
 {
     const char *empty = "";
-
-    QObject *obj = qobject_from_json(empty, NULL);
+    QObject *obj = qobject_from_json(empty, &error_abort);
     g_assert(obj == NULL);
 }
 
 static void unterminated_string(void)
 {
-    QObject *obj = qobject_from_json("\"abc", NULL);
+    Error *err = NULL;
+    QObject *obj = qobject_from_json("\"abc", &err);
+    g_assert(!err);             /* BUG */
     g_assert(obj == NULL);
 }
 
 static void unterminated_sq_string(void)
 {
-    QObject *obj = qobject_from_json("'abc", NULL);
+    Error *err = NULL;
+    QObject *obj = qobject_from_json("'abc", &err);
+    g_assert(!err);             /* BUG */
     g_assert(obj == NULL);
 }
 
 static void unterminated_escape(void)
 {
-    QObject *obj = qobject_from_json("\"abc\\\"", NULL);
+    Error *err = NULL;
+    QObject *obj = qobject_from_json("\"abc\\\"", &err);
+    g_assert(!err);             /* BUG */
     g_assert(obj == NULL);
 }
 
 static void unterminated_array(void)
 {
-    QObject *obj = qobject_from_json("[32", NULL);
+    Error *err = NULL;
+    QObject *obj = qobject_from_json("[32", &err);
+    g_assert(!err);             /* BUG */
     g_assert(obj == NULL);
 }
 
 static void unterminated_array_comma(void)
 {
-    QObject *obj = qobject_from_json("[32,", NULL);
+    Error *err = NULL;
+    QObject *obj = qobject_from_json("[32,", &err);
+    g_assert(!err);             /* BUG */
     g_assert(obj == NULL);
 }
 
 static void invalid_array_comma(void)
 {
-    QObject *obj = qobject_from_json("[32,}", NULL);
+    Error *err = NULL;
+    QObject *obj = qobject_from_json("[32,}", &err);
+    error_free_or_abort(&err);
     g_assert(obj == NULL);
 }
 
 static void unterminated_dict(void)
 {
-    QObject *obj = qobject_from_json("{'abc':32", NULL);
+    Error *err = NULL;
+    QObject *obj = qobject_from_json("{'abc':32", &err);
+    g_assert(!err);             /* BUG */
     g_assert(obj == NULL);
 }
 
 static void unterminated_dict_comma(void)
 {
-    QObject *obj = qobject_from_json("{'abc':32,", NULL);
+    Error *err = NULL;
+    QObject *obj = qobject_from_json("{'abc':32,", &err);
+    g_assert(!err);             /* BUG */
     g_assert(obj == NULL);
 }
 
 static void invalid_dict_comma(void)
 {
-    QObject *obj = qobject_from_json("{'abc':32,}", NULL);
+    Error *err = NULL;
+    QObject *obj = qobject_from_json("{'abc':32,}", &err);
+    error_free_or_abort(&err);
     g_assert(obj == NULL);
 }
 
 static void unterminated_literal(void)
 {
-    QObject *obj = qobject_from_json("nul", NULL);
+    Error *err = NULL;
+    QObject *obj = qobject_from_json("nul", &err);
+    error_free_or_abort(&err);
     g_assert(obj == NULL);
 }
 
@@ -1421,15 +1443,17 @@ static char *make_nest(char *buf, size_t cnt)
 
 static void limits_nesting(void)
 {
+    Error *err = NULL;
     enum { max_nesting = 1024 }; /* see qobject/json-streamer.c */
     char buf[2 * (max_nesting + 1) + 1];
     QObject *obj;
 
-    obj = qobject_from_json(make_nest(buf, max_nesting), NULL);
+    obj = qobject_from_json(make_nest(buf, max_nesting), &error_abort);
     g_assert(obj != NULL);
     qobject_decref(obj);
 
-    obj = qobject_from_json(make_nest(buf, max_nesting + 1), NULL);
+    obj = qobject_from_json(make_nest(buf, max_nesting + 1), &err);
+    error_free_or_abort(&err);
     g_assert(obj == NULL);
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 14/24] check-qjson: Test errors from qobject_from_json()

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Pass &error_abort with known-good input.  Else pass &err and check
> what comes back.  This demonstrates that the parser fails silently for
> many errors.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 14/24] check-qjson: Test errors from qobject_from_json()

[Eric Blake]

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

On 02/27/2017 05:20 AM, Markus Armbruster wrote:
> Pass &error_abort with known-good input.  Else pass &err and check
> what comes back.  This demonstrates that the parser fails silently for
> many errors.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/check-qjson.c | 88 ++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 56 insertions(+), 32 deletions(-)
> 

> @@ -809,7 +811,7 @@ static void utf8_string(void)
>          utf8_in = test_cases[i].utf8_in ?: test_cases[i].utf8_out;
>          json_out = test_cases[i].json_out ?: test_cases[i].json_in;
>  
> -        obj = qobject_from_json(json_in, NULL);
> +        obj = qobject_from_json(json_in, utf8_out ? &error_abort : NULL);

This one ignores errors if utf8_out is not specified; intentional?
Especially when compared to...


>  static void unterminated_string(void)
>  {
> -    QObject *obj = qobject_from_json("\"abc", NULL);
> +    Error *err = NULL;
> +    QObject *obj = qobject_from_json("\"abc", &err);
> +    g_assert(!err);             /* BUG */
>      g_assert(obj == NULL);

change like these.

-- 
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 14/24] check-qjson: Test errors from qobject_from_json()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/27/2017 05:20 AM, Markus Armbruster wrote:
>> Pass &error_abort with known-good input.  Else pass &err and check
>> what comes back.  This demonstrates that the parser fails silently for
>> many errors.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/check-qjson.c | 88 ++++++++++++++++++++++++++++++++++-------------------
>>  1 file changed, 56 insertions(+), 32 deletions(-)
>> 
>
>> @@ -809,7 +811,7 @@ static void utf8_string(void)
>>          utf8_in = test_cases[i].utf8_in ?: test_cases[i].utf8_out;
>>          json_out = test_cases[i].json_out ?: test_cases[i].json_in;
>>  
>> -        obj = qobject_from_json(json_in, NULL);
>> +        obj = qobject_from_json(json_in, utf8_out ? &error_abort : NULL);
>
> This one ignores errors if utf8_out is not specified; intentional?
> Especially when compared to...

            if (utf8_out) {
                str = qobject_to_qstring(obj);
                g_assert(str);
                g_assert_cmpstr(qstring_get_str(str), ==, utf8_out);

If @utf8_out, the conversion is expected to work and produce utf8_out.

            } else {
                g_assert(!obj);

Else, the conversion expected not to work.  These are bugs we've known
for a long time.

            }

>>  static void unterminated_string(void)
>>  {
>> -    QObject *obj = qobject_from_json("\"abc", NULL);
>> +    Error *err = NULL;
>> +    QObject *obj = qobject_from_json("\"abc", &err);
>> +    g_assert(!err);             /* BUG */
>>      g_assert(obj == NULL);
>
> change like these.

These are new bugs: the JSON parser can fail without setting an error.

[Qemu-devel] [PATCH 15/24] test-visitor-serialization: Pass &error_abort to qobject_from_json()

[Markus Armbruster]

qmp_deserialize() calls qobject_from_json() ignoring errors.  It
passes the result to qobject_input_visitor_new(), which asserts it's
not null.  Therefore, we can just as well pass &error_abort to
qobject_from_json().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-visitor-serialization.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index 37dff41..4d47cee 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -1037,7 +1037,7 @@ static void qmp_deserialize(void **native_out, void *datap,
     visit_complete(d->qov, &d->obj);
     obj_orig = d->obj;
     output_json = qobject_to_json(obj_orig);
-    obj = qobject_from_json(qstring_get_str(output_json), NULL);
+    obj = qobject_from_json(qstring_get_str(output_json), &error_abort);
 
     QDECREF(output_json);
     d->qiv = qobject_input_visitor_new(obj);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 15/24] test-visitor-serialization: Pass &error_abort to qobject_from_json()

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> qmp_deserialize() calls qobject_from_json() ignoring errors.  It
> passes the result to qobject_input_visitor_new(), which asserts it's
> not null.  Therefore, we can just as well pass &error_abort to
> qobject_from_json().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 16/24] monitor: Assert qmp_schema_json[] is sane

[Markus Armbruster]

qmp_query_qmp_schema() parses qmp_schema_json[] with
qobject_from_json().  This must not fail, so pass &error_abort.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/monitor.c b/monitor.c
index 858bcda..13f6133 100644
--- a/monitor.c
+++ b/monitor.c
@@ -950,7 +950,7 @@ EventInfoList *qmp_query_events(Error **errp)
 static void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
                                  Error **errp)
 {
-    *ret_data = qobject_from_json(qmp_schema_json, NULL);
+    *ret_data = qobject_from_json(qmp_schema_json, &error_abort);
 }
 
 /*
-- 
2.7.4

Re: [Qemu-devel] [PATCH 16/24] monitor: Assert qmp_schema_json[] is sane

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> qmp_query_qmp_schema() parses qmp_schema_json[] with
> qobject_from_json().  This must not fail, so pass &error_abort.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 17/24] qapi: New qobject_input_visitor_new_str() for convenience

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/qobject-input-visitor.h | 12 ++++++++++++
 qapi/qobject-input-visitor.c         | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h
index 282f9d2..b399285 100644
--- a/include/qapi/qobject-input-visitor.h
+++ b/include/qapi/qobject-input-visitor.h
@@ -68,4 +68,16 @@ Visitor *qobject_input_visitor_new(QObject *obj);
  */
 Visitor *qobject_input_visitor_new_keyval(QObject *obj);
 
+/*
+ * Create a QObject input visitor for parsing @str.
+ *
+ * If @str looks like JSON, parse it as JSON, else as KEY=VALUE,...
+ * @implied_key applies to KEY=VALUE, and works as in keyval_parse().
+ * On failure, store an error through @errp and return NULL.
+ * On success, return a new QObject input visitor for the parse.
+ */
+Visitor *qobject_input_visitor_new_str(const char *str,
+                                       const char *implied_key,
+                                       Error **errp);
+
 #endif
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 3db5850..64a08d3 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -18,9 +18,11 @@
 #include "qapi/visitor-impl.h"
 #include "qemu/queue.h"
 #include "qemu-common.h"
+#include "qapi/qmp/qjson.h"
 #include "qapi/qmp/types.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/cutils.h"
+#include "qemu/option.h"
 
 typedef struct StackObject {
     const char *name;            /* Name of @obj in its parent, if any */
@@ -655,3 +657,34 @@ Visitor *qobject_input_visitor_new_keyval(QObject *obj)
 
     return &v->visitor;
 }
+
+Visitor *qobject_input_visitor_new_str(const char *str,
+                                       const char *implied_key,
+                                       Error **errp)
+{
+    bool is_json = str[0] == '{';
+    QObject *obj;
+    QDict *args;
+    Visitor *v;
+
+    if (is_json) {
+        obj = qobject_from_json(str, errp);
+        if (!obj) {
+            /* Work around qobject_from_json() lossage TODO fix that */
+            if (errp && !*errp) {
+                error_setg(errp, "JSON parse error");
+                return NULL;
+            }
+            return NULL;
+        }
+        args = qobject_to_qdict(obj);
+        assert(args);
+        v = qobject_input_visitor_new(QOBJECT(args));
+    } else {
+        args = keyval_parse(optarg, implied_key, errp);
+        v = qobject_input_visitor_new_keyval(QOBJECT(args));
+    }
+    QDECREF(args);
+
+    return v;
+}
-- 
2.7.4

Re: [Qemu-devel] [PATCH 17/24] qapi: New qobject_input_visitor_new_str() for convenience

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/qobject-input-visitor.h | 12 ++++++++++++
>  qapi/qobject-input-visitor.c         | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h
> index 282f9d2..b399285 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -68,4 +68,16 @@ Visitor *qobject_input_visitor_new(QObject *obj);
>   */
>  Visitor *qobject_input_visitor_new_keyval(QObject *obj);
>  
> +/*
> + * Create a QObject input visitor for parsing @str.
> + *
> + * If @str looks like JSON, parse it as JSON, else as KEY=VALUE,...
> + * @implied_key applies to KEY=VALUE, and works as in keyval_parse().
> + * On failure, store an error through @errp and return NULL.
> + * On success, return a new QObject input visitor for the parse.
> + */
> +Visitor *qobject_input_visitor_new_str(const char *str,
> +                                       const char *implied_key,
> +                                       Error **errp);
> +
>  #endif
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 3db5850..64a08d3 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -18,9 +18,11 @@
>  #include "qapi/visitor-impl.h"
>  #include "qemu/queue.h"
>  #include "qemu-common.h"
> +#include "qapi/qmp/qjson.h"
>  #include "qapi/qmp/types.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/cutils.h"
> +#include "qemu/option.h"
>  
>  typedef struct StackObject {
>      const char *name;            /* Name of @obj in its parent, if any */
> @@ -655,3 +657,34 @@ Visitor *qobject_input_visitor_new_keyval(QObject *obj)
>  
>      return &v->visitor;
>  }
> +
> +Visitor *qobject_input_visitor_new_str(const char *str,
> +                                       const char *implied_key,
> +                                       Error **errp)
> +{
> +    bool is_json = str[0] == '{';

Should the keyval_parse() grammar just forbid { in key names then (and
ideally employ assertions to enforce this) to avoid any ambiguity here?
therwise the behaviour might depend on the order of options in a
key/value string, which could be somewhat confusing.

> +    QObject *obj;
> +    QDict *args;
> +    Visitor *v;
> +
> +    if (is_json) {
> +        obj = qobject_from_json(str, errp);
> +        if (!obj) {
> +            /* Work around qobject_from_json() lossage TODO fix that */
> +            if (errp && !*errp) {
> +                error_setg(errp, "JSON parse error");
> +                return NULL;
> +            }
> +            return NULL;
> +        }
> +        args = qobject_to_qdict(obj);
> +        assert(args);
> +        v = qobject_input_visitor_new(QOBJECT(args));
> +    } else {
> +        args = keyval_parse(optarg, implied_key, errp);
> +        v = qobject_input_visitor_new_keyval(QOBJECT(args));

If keyval_parse() fails and returns args == NULL, won't we run into the
assertion in qobject_input_visitor_base_new() now?

> +    }
> +    QDECREF(args);
> +
> +    return v;
> +}

Kevin

Re: [Qemu-devel] [PATCH 17/24] qapi: New qobject_input_visitor_new_str() for convenience

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/qapi/qobject-input-visitor.h | 12 ++++++++++++
>>  qapi/qobject-input-visitor.c         | 33 +++++++++++++++++++++++++++++++++
>>  2 files changed, 45 insertions(+)
>> 
>> diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h
>> index 282f9d2..b399285 100644
>> --- a/include/qapi/qobject-input-visitor.h
>> +++ b/include/qapi/qobject-input-visitor.h
>> @@ -68,4 +68,16 @@ Visitor *qobject_input_visitor_new(QObject *obj);
>>   */
>>  Visitor *qobject_input_visitor_new_keyval(QObject *obj);
>>  
>> +/*
>> + * Create a QObject input visitor for parsing @str.
>> + *
>> + * If @str looks like JSON, parse it as JSON, else as KEY=VALUE,...
>> + * @implied_key applies to KEY=VALUE, and works as in keyval_parse().
>> + * On failure, store an error through @errp and return NULL.
>> + * On success, return a new QObject input visitor for the parse.
>> + */
>> +Visitor *qobject_input_visitor_new_str(const char *str,
>> +                                       const char *implied_key,
>> +                                       Error **errp);
>> +
>>  #endif
>> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>> index 3db5850..64a08d3 100644
>> --- a/qapi/qobject-input-visitor.c
>> +++ b/qapi/qobject-input-visitor.c
>> @@ -18,9 +18,11 @@
>>  #include "qapi/visitor-impl.h"
>>  #include "qemu/queue.h"
>>  #include "qemu-common.h"
>> +#include "qapi/qmp/qjson.h"
>>  #include "qapi/qmp/types.h"
>>  #include "qapi/qmp/qerror.h"
>>  #include "qemu/cutils.h"
>> +#include "qemu/option.h"
>>  
>>  typedef struct StackObject {
>>      const char *name;            /* Name of @obj in its parent, if any */
>> @@ -655,3 +657,34 @@ Visitor *qobject_input_visitor_new_keyval(QObject *obj)
>>  
>>      return &v->visitor;
>>  }
>> +
>> +Visitor *qobject_input_visitor_new_str(const char *str,
>> +                                       const char *implied_key,
>> +                                       Error **errp)
>> +{
>> +    bool is_json = str[0] == '{';
>
> Should the keyval_parse() grammar just forbid { in key names then (and
> ideally employ assertions to enforce this) to avoid any ambiguity here?
> therwise the behaviour might depend on the order of options in a
> key/value string, which could be somewhat confusing.

You're absolutely right.  Fortunately, PATCH 23 does.  I guess the
easiest fix is to move it before this one.

>> +    QObject *obj;
>> +    QDict *args;
>> +    Visitor *v;
>> +
>> +    if (is_json) {
>> +        obj = qobject_from_json(str, errp);
>> +        if (!obj) {
>> +            /* Work around qobject_from_json() lossage TODO fix that */
>> +            if (errp && !*errp) {
>> +                error_setg(errp, "JSON parse error");
>> +                return NULL;
>> +            }
>> +            return NULL;
>> +        }
>> +        args = qobject_to_qdict(obj);
>> +        assert(args);
>> +        v = qobject_input_visitor_new(QOBJECT(args));
>> +    } else {
>> +        args = keyval_parse(optarg, implied_key, errp);
>> +        v = qobject_input_visitor_new_keyval(QOBJECT(args));
>
> If keyval_parse() fails and returns args == NULL, won't we run into the
> assertion in qobject_input_visitor_base_new() now?

Lacks error checking, will fix.

>> +    }
>> +    QDECREF(args);
>> +
>> +    return v;
>> +}
>
> Kevin

Thanks!

Re: [Qemu-devel] [PATCH 17/24] qapi: New qobject_input_visitor_new_str() for convenience

[Kevin Wolf]

Am 28.02.2017 um 19:48 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  include/qapi/qobject-input-visitor.h | 12 ++++++++++++
> >>  qapi/qobject-input-visitor.c         | 33 +++++++++++++++++++++++++++++++++
> >>  2 files changed, 45 insertions(+)
> >> 
> >> diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h
> >> index 282f9d2..b399285 100644
> >> --- a/include/qapi/qobject-input-visitor.h
> >> +++ b/include/qapi/qobject-input-visitor.h
> >> @@ -68,4 +68,16 @@ Visitor *qobject_input_visitor_new(QObject *obj);
> >>   */
> >>  Visitor *qobject_input_visitor_new_keyval(QObject *obj);
> >>  
> >> +/*
> >> + * Create a QObject input visitor for parsing @str.
> >> + *
> >> + * If @str looks like JSON, parse it as JSON, else as KEY=VALUE,...
> >> + * @implied_key applies to KEY=VALUE, and works as in keyval_parse().
> >> + * On failure, store an error through @errp and return NULL.
> >> + * On success, return a new QObject input visitor for the parse.
> >> + */
> >> +Visitor *qobject_input_visitor_new_str(const char *str,
> >> +                                       const char *implied_key,
> >> +                                       Error **errp);
> >> +
> >>  #endif
> >> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> >> index 3db5850..64a08d3 100644
> >> --- a/qapi/qobject-input-visitor.c
> >> +++ b/qapi/qobject-input-visitor.c
> >> @@ -18,9 +18,11 @@
> >>  #include "qapi/visitor-impl.h"
> >>  #include "qemu/queue.h"
> >>  #include "qemu-common.h"
> >> +#include "qapi/qmp/qjson.h"
> >>  #include "qapi/qmp/types.h"
> >>  #include "qapi/qmp/qerror.h"
> >>  #include "qemu/cutils.h"
> >> +#include "qemu/option.h"
> >>  
> >>  typedef struct StackObject {
> >>      const char *name;            /* Name of @obj in its parent, if any */
> >> @@ -655,3 +657,34 @@ Visitor *qobject_input_visitor_new_keyval(QObject *obj)
> >>  
> >>      return &v->visitor;
> >>  }
> >> +
> >> +Visitor *qobject_input_visitor_new_str(const char *str,
> >> +                                       const char *implied_key,
> >> +                                       Error **errp)
> >> +{
> >> +    bool is_json = str[0] == '{';
> >
> > Should the keyval_parse() grammar just forbid { in key names then (and
> > ideally employ assertions to enforce this) to avoid any ambiguity here?
> > therwise the behaviour might depend on the order of options in a
> > key/value string, which could be somewhat confusing.
> 
> You're absolutely right.  Fortunately, PATCH 23 does.  I guess the
> easiest fix is to move it before this one.

Don't bother. I'm just reviewing the patches in order, so I didn't see
at this point that there is a fix in a later patch.

Kevin

Re: [Qemu-devel] [PATCH 17/24] qapi: New qobject_input_visitor_new_str() for convenience

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/qobject-input-visitor.h | 12 ++++++++++++
>  qapi/qobject-input-visitor.c         | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h
> index 282f9d2..b399285 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -68,4 +68,16 @@ Visitor *qobject_input_visitor_new(QObject *obj);
>   */
>  Visitor *qobject_input_visitor_new_keyval(QObject *obj);
>  
> +/*
> + * Create a QObject input visitor for parsing @str.
> + *
> + * If @str looks like JSON, parse it as JSON, else as KEY=VALUE,...
> + * @implied_key applies to KEY=VALUE, and works as in keyval_parse().
> + * On failure, store an error through @errp and return NULL.
> + * On success, return a new QObject input visitor for the parse.
> + */
> +Visitor *qobject_input_visitor_new_str(const char *str,
> +                                       const char *implied_key,
> +                                       Error **errp);
> +
>  #endif
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 3db5850..64a08d3 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -18,9 +18,11 @@
>  #include "qapi/visitor-impl.h"
>  #include "qemu/queue.h"
>  #include "qemu-common.h"
> +#include "qapi/qmp/qjson.h"
>  #include "qapi/qmp/types.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/cutils.h"
> +#include "qemu/option.h"
>  
>  typedef struct StackObject {
>      const char *name;            /* Name of @obj in its parent, if any */
> @@ -655,3 +657,34 @@ Visitor *qobject_input_visitor_new_keyval(QObject *obj)
>  
>      return &v->visitor;
>  }
> +
> +Visitor *qobject_input_visitor_new_str(const char *str,
> +                                       const char *implied_key,
> +                                       Error **errp)
> +{
> +    bool is_json = str[0] == '{';
> +    QObject *obj;
> +    QDict *args;
> +    Visitor *v;
> +
> +    if (is_json) {
> +        obj = qobject_from_json(str, errp);
> +        if (!obj) {
> +            /* Work around qobject_from_json() lossage TODO fix that */
> +            if (errp && !*errp) {
> +                error_setg(errp, "JSON parse error");
> +                return NULL;
> +            }
> +            return NULL;
> +        }
> +        args = qobject_to_qdict(obj);
> +        assert(args);
> +        v = qobject_input_visitor_new(QOBJECT(args));
> +    } else {
> +        args = keyval_parse(optarg, implied_key, errp);

No, you don't mean optarg.

Makes all attempts to use -blockdev segfault for me.

> +        v = qobject_input_visitor_new_keyval(QOBJECT(args));
> +    }
> +    QDECREF(args);
> +
> +    return v;
> +}

Kevin

Re: [Qemu-devel] [PATCH 17/24] qapi: New qobject_input_visitor_new_str() for convenience

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/qapi/qobject-input-visitor.h | 12 ++++++++++++
>>  qapi/qobject-input-visitor.c         | 33 +++++++++++++++++++++++++++++++++
>>  2 files changed, 45 insertions(+)
>> 
>> diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h
>> index 282f9d2..b399285 100644
>> --- a/include/qapi/qobject-input-visitor.h
>> +++ b/include/qapi/qobject-input-visitor.h
>> @@ -68,4 +68,16 @@ Visitor *qobject_input_visitor_new(QObject *obj);
>>   */
>>  Visitor *qobject_input_visitor_new_keyval(QObject *obj);
>>  
>> +/*
>> + * Create a QObject input visitor for parsing @str.
>> + *
>> + * If @str looks like JSON, parse it as JSON, else as KEY=VALUE,...
>> + * @implied_key applies to KEY=VALUE, and works as in keyval_parse().
>> + * On failure, store an error through @errp and return NULL.
>> + * On success, return a new QObject input visitor for the parse.
>> + */
>> +Visitor *qobject_input_visitor_new_str(const char *str,
>> +                                       const char *implied_key,
>> +                                       Error **errp);
>> +
>>  #endif
>> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>> index 3db5850..64a08d3 100644
>> --- a/qapi/qobject-input-visitor.c
>> +++ b/qapi/qobject-input-visitor.c
>> @@ -18,9 +18,11 @@
>>  #include "qapi/visitor-impl.h"
>>  #include "qemu/queue.h"
>>  #include "qemu-common.h"
>> +#include "qapi/qmp/qjson.h"
>>  #include "qapi/qmp/types.h"
>>  #include "qapi/qmp/qerror.h"
>>  #include "qemu/cutils.h"
>> +#include "qemu/option.h"
>>  
>>  typedef struct StackObject {
>>      const char *name;            /* Name of @obj in its parent, if any */
>> @@ -655,3 +657,34 @@ Visitor *qobject_input_visitor_new_keyval(QObject *obj)
>>  
>>      return &v->visitor;
>>  }
>> +
>> +Visitor *qobject_input_visitor_new_str(const char *str,
>> +                                       const char *implied_key,
>> +                                       Error **errp)
>> +{
>> +    bool is_json = str[0] == '{';
>> +    QObject *obj;
>> +    QDict *args;
>> +    Visitor *v;
>> +
>> +    if (is_json) {
>> +        obj = qobject_from_json(str, errp);
>> +        if (!obj) {
>> +            /* Work around qobject_from_json() lossage TODO fix that */
>> +            if (errp && !*errp) {
>> +                error_setg(errp, "JSON parse error");
>> +                return NULL;
>> +            }
>> +            return NULL;
>> +        }
>> +        args = qobject_to_qdict(obj);
>> +        assert(args);
>> +        v = qobject_input_visitor_new(QOBJECT(args));
>> +    } else {
>> +        args = keyval_parse(optarg, implied_key, errp);
>
> No, you don't mean optarg.
>
> Makes all attempts to use -blockdev segfault for me.

Okay, I guess I get to stay after class and write "Late night last
minute changes are a bad idea" one hundred times.

Including the kitchen sink can hide stupid bugs...

>
>> +        v = qobject_input_visitor_new_keyval(QOBJECT(args));
>> +    }
>> +    QDECREF(args);
>> +
>> +    return v;
>> +}
>
> Kevin

[Qemu-devel] [PATCH 18/24] block: 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

This does not yet support lists or downstream extensions, i.e. keys
with __RFQDN_ prefix, but the next few patches will take care of that.

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 |  7 +++++++
 vl.c            | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index bf458f8..8f02264 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -532,6 +532,13 @@ 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 driver[,node-name=N][,discard=ignore|unmap]\n"
+    "          [,cache.direct=on|off][,cache.no-flush=on|off]\n"
+    "          [,read-only=on|off][,detect-zeroes=on|off|unmap]\n"
+    "          [,driver specific parameters...]\n"
+    "                configure a block backend\n", QEMU_ARCH_ALL)
+
 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 c6020b9..0684d52 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"
@@ -2975,6 +2978,13 @@ 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);
 
@@ -3117,6 +3127,25 @@ 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:
+                {
+                    Visitor *v;
+                    BlockdevOptions_queue *bdo;
+
+                    v = qobject_input_visitor_new_str(optarg, "driver", &err);
+                    if (!v) {
+                        error_report_err(err);
+                        exit(1);
+                    }
+
+                    bdo = g_new(BlockdevOptions_queue, 1);
+                    visit_type_BlockdevOptions(v, NULL, &bdo->bdo,
+                                               &error_fatal);
+                    visit_free(v);
+                    loc_save(&bdo->loc);
+                    QSIMPLEQ_INSERT_TAIL(&bdo_queue, bdo, entry);
+                    break;
+                }
             case QEMU_OPTION_drive:
                 if (drive_def(optarg) == NULL) {
                     exit(1);
@@ -4454,6 +4483,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 18/24] block: Initial implementation of -blockdev

[Eric Blake]

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

On 02/27/2017 05:20 AM, 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
> 
> This does not yet support lists or downstream extensions, i.e. keys
> with __RFQDN_ prefix, but the next few patches will take care of that.
> 
> 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 |  7 +++++++
>  vl.c            | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index bf458f8..8f02264 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -532,6 +532,13 @@ 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 driver[,node-name=N][,discard=ignore|unmap]\n"

Maybe "-blockdev [driver=]DRIVER[,node-name".....

to call out that you use driver as an implied key? But I'm also fine
with what you wrote (it's sufficient enough for a human reading it, and
the help output is not scraped by libvirt).

> +    "          [,cache.direct=on|off][,cache.no-flush=on|off]\n"
> +    "          [,read-only=on|off][,detect-zeroes=on|off|unmap]\n"
> +    "          [,driver specific parameters...]\n"
> +    "                configure a block backend\n", QEMU_ARCH_ALL)
> +
>  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"

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

-- 
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 18/24] block: Initial implementation of -blockdev

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> 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
> 
> This does not yet support lists or downstream extensions, i.e. keys
> with __RFQDN_ prefix, but the next few patches will take care of that.
> 
> 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>

Apparently I forgot to actually reply here after going back to the
previous patch...

> @@ -532,6 +532,13 @@ 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 driver[,node-name=N][,discard=ignore|unmap]\n"

I was going to suggest the same thing as Eric here, simply 'driver'
doesn't look right.

> +    "          [,cache.direct=on|off][,cache.no-flush=on|off]\n"
> +    "          [,read-only=on|off][,detect-zeroes=on|off|unmap]\n"
> +    "          [,driver specific parameters...]\n"
> +    "                configure a block backend\n", QEMU_ARCH_ALL)
> +
>  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"

> @@ -4454,6 +4483,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);

And here I found the placement of the new code odd, it's in the middle
of -drive handling. The current placement means that -drive can
reference -blockdev nodes, but not the other way round. I like this and
wouldn't recommand changing it. So my suggestion for a better placement
would be to move the -blockdev code up a bit.

Kevin

Re: [Qemu-devel] [PATCH 18/24] block: Initial implementation of -blockdev

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>> 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
>> 
>> This does not yet support lists or downstream extensions, i.e. keys
>> with __RFQDN_ prefix, but the next few patches will take care of that.
>> 
>> 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>
>
> Apparently I forgot to actually reply here after going back to the
> previous patch...
>
>> @@ -532,6 +532,13 @@ 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 driver[,node-name=N][,discard=ignore|unmap]\n"
>
> I was going to suggest the same thing as Eric here, simply 'driver'
> doesn't look right.

Matches how we document -chardev, -netdev, -device, ..., which is where
I looked.  But [driver=]driver matches a whole bunch of other options.
I'll change it.

>> +    "          [,cache.direct=on|off][,cache.no-flush=on|off]\n"
>> +    "          [,read-only=on|off][,detect-zeroes=on|off|unmap]\n"
>> +    "          [,driver specific parameters...]\n"
>> +    "                configure a block backend\n", QEMU_ARCH_ALL)
>> +
>>  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"
>
>> @@ -4454,6 +4483,16 @@ int main(int argc, char **argv, char **envp)
        /* open the virtual block devices */
        if (snapshot || replay_mode != REPLAY_MODE_NONE) {
            qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
>>          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);
>
> And here I found the placement of the new code odd, it's in the middle
> of -drive handling. The current placement means that -drive can
> reference -blockdev nodes, but not the other way round. I like this and
> wouldn't recommand changing it. So my suggestion for a better placement
> would be to move the -blockdev code up a bit.

Moving it right after /* open the virtual block devices */

[Qemu-devel] [PATCH 19/24] qapi: Improve how keyval input visitor reports unexpected dicts

[Markus Armbruster]

Incorrect option

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

is rejected with "Invalid parameter type for 'aio', expected: string".
To make sense of this, you almost have to translate it into the
equivalent QMP command

    { "execute": "blockdev-add", "arguments": { "node-name": "foo", "driver": "file", "filename": "foo.img", "aio": { "unmap": true } } }

Improve the error message to "Parameters 'aio.*' are unexpected".
Take care not to confuse the case "unexpected nested parameters"
(i.e. the object is a QDict or QList) with the case "non-string scalar
parameter".  The latter is a misuse of the visitor, and should perhaps
be an assertion.  Note that test-qobject-input-visitor exercises this
misuse in test_visitor_in_int_keyval(), test_visitor_in_bool_keyval()
and test_visitor_in_number_keyval().

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

diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 64a08d3..1d7b420 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -167,9 +167,18 @@ static const char *qobject_input_get_keyval(QObjectInputVisitor *qiv,
 
     qstr = qobject_to_qstring(qobj);
     if (!qstr) {
-        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
-                   full_name(qiv, name), "string");
-        return NULL;
+        switch (qobject_type(qobj)) {
+        case QTYPE_QDICT:
+        case QTYPE_QLIST:
+            error_setg(errp, "Parameters '%s.*' are unexpected",
+                       full_name(qiv, name));
+            return NULL;
+        default:
+            /* Non-string scalar (should this be an assertion?) */
+            error_setg(errp, "Internal error: parameter %s invalid",
+                       full_name(qiv, name));
+            return NULL;
+        }
     }
 
     return qstring_get_str(qstr);
@@ -478,6 +487,15 @@ static void qobject_input_type_str(Visitor *v, const char *name, char **obj,
     *obj = g_strdup(qstring_get_str(qstr));
 }
 
+static void qobject_input_type_str_keyval(Visitor *v, const char *name,
+                                          char **obj, Error **errp)
+{
+    QObjectInputVisitor *qiv = to_qiv(v);
+    const char *str = qobject_input_get_keyval(qiv, name, errp);
+
+    *obj = g_strdup(str);
+}
+
 static void qobject_input_type_number(Visitor *v, const char *name, double *obj,
                                       Error **errp)
 {
@@ -649,7 +667,7 @@ Visitor *qobject_input_visitor_new_keyval(QObject *obj)
     v->visitor.type_int64 = qobject_input_type_int64_keyval;
     v->visitor.type_uint64 = qobject_input_type_uint64_keyval;
     v->visitor.type_bool = qobject_input_type_bool_keyval;
-    v->visitor.type_str = qobject_input_type_str;
+    v->visitor.type_str = qobject_input_type_str_keyval;
     v->visitor.type_number = qobject_input_type_number_keyval;
     v->visitor.type_any = qobject_input_type_any;
     v->visitor.type_null = qobject_input_type_null;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 19/24] qapi: Improve how keyval input visitor reports unexpected dicts

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Incorrect option
> 
>     -blockdev node-name=foo,driver=file,filename=foo.img,aio.unmap
> 
> is rejected with "Invalid parameter type for 'aio', expected: string".
> To make sense of this, you almost have to translate it into the
> equivalent QMP command
> 
>     { "execute": "blockdev-add", "arguments": { "node-name": "foo", "driver": "file", "filename": "foo.img", "aio": { "unmap": true } } }
> 
> Improve the error message to "Parameters 'aio.*' are unexpected".

In fact, what I get (after fixing the previous review comments that
would make it crash) is "Expected '=' after parameter 'aio.unmap'". Only
if I add a value, I get the newly added message.

Doesn't make the patch less correct, though.

> Take care not to confuse the case "unexpected nested parameters"
> (i.e. the object is a QDict or QList) with the case "non-string scalar
> parameter".  The latter is a misuse of the visitor, and should perhaps
> be an assertion.  Note that test-qobject-input-visitor exercises this
> misuse in test_visitor_in_int_keyval(), test_visitor_in_bool_keyval()
> and test_visitor_in_number_keyval().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 19/24] qapi: Improve how keyval input visitor reports unexpected dicts

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>> Incorrect option
>> 
>>     -blockdev node-name=foo,driver=file,filename=foo.img,aio.unmap
>> 
>> is rejected with "Invalid parameter type for 'aio', expected: string".
>> To make sense of this, you almost have to translate it into the
>> equivalent QMP command
>> 
>>     { "execute": "blockdev-add", "arguments": { "node-name": "foo", "driver": "file", "filename": "foo.img", "aio": { "unmap": true } } }
>> 
>> Improve the error message to "Parameters 'aio.*' are unexpected".
>
> In fact, what I get (after fixing the previous review comments that
> would make it crash) is "Expected '=' after parameter 'aio.unmap'". Only
> if I add a value, I get the newly added message.

Fallout from ditching implied value sugar some time after this commit
message was written.  Will fix.

> Doesn't make the patch less correct, though.
>
>> Take care not to confuse the case "unexpected nested parameters"
>> (i.e. the object is a QDict or QList) with the case "non-string scalar
>> parameter".  The latter is a misuse of the visitor, and should perhaps
>> be an assertion.  Note that test-qobject-input-visitor exercises this
>> misuse in test_visitor_in_int_keyval(), test_visitor_in_bool_keyval()
>> and test_visitor_in_number_keyval().
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Thanks!

[Qemu-devel] [PATCH 20/24] docs/qapi-code-gen.txt: Clarify naming rules

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/qapi-code-gen.txt | 61 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 6746c10..9514d93 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -216,33 +216,38 @@ single-dimension array of that type; multi-dimension arrays are not
 directly supported (although an array of a complex struct that
 contains an array member is possible).
 
+All names must begin with a letter, and contain only ASCII letters,
+digits, hyphen, and underscore.  There are two exceptions: enum values
+may start with a digit, and names that are downstream extensions (see
+section Downstream extensions) start with underscore.
+
+Names beginning with 'q_' are reserved for the generator, which uses
+them for munging QMP names that resemble C keywords or other
+problematic strings.  For example, a member named "default" in qapi
+becomes "q_default" in the generated C code.
+
 Types, commands, and events share a common namespace.  Therefore,
 generally speaking, type definitions should always use CamelCase for
-user-defined type names, while built-in types are lowercase. Type
-definitions should not end in 'Kind', as this namespace is used for
-creating implicit C enums for visiting union types, or in 'List', as
-this namespace is used for creating array types.  Command names,
-and member names within a type, should be all lower case with words
-separated by a hyphen.  However, some existing older commands and
-complex types use underscore; when extending such expressions,
-consistency is preferred over blindly avoiding underscore.  Event
-names should be ALL_CAPS with words separated by underscore.  Member
-names cannot start with 'has-' or 'has_', as this is reserved for
-tracking optional members.
+user-defined type names, while built-in types are lowercase.
+
+Type names ending with 'Kind' or 'List' are reserved for the
+generator, which uses them for implicit union enums and array types,
+respectively.
+
+Command names, and member names within a type, should be all lower
+case with words separated by a hyphen.  However, some existing older
+commands and complex types use underscore; when extending such
+expressions, consistency is preferred over blindly avoiding
+underscore.
+
+Event names should be ALL_CAPS with words separated by underscore.
+
+Member names starting with 'has-' or 'has_' are reserved for the
+generator, which uses them for tracking optional members.
 
 Any name (command, event, type, member, or enum value) beginning with
 "x-" is marked experimental, and may be withdrawn or changed
-incompatibly in a future release.  All names must begin with a letter,
-and contain only ASCII letters, digits, dash, and underscore.  There
-are two exceptions: enum values may start with a digit, and any
-extensions added by downstream vendors should start with a prefix
-matching "__RFQDN_" (for the reverse-fully-qualified-domain-name of
-the vendor), even if the rest of the name uses dash (example:
-__com.redhat_drive-mirror).  Names beginning with 'q_' are reserved
-for the generator: QMP names that resemble C keywords or other
-problematic strings will be munged in C to use this prefix.  For
-example, a member named "default" in qapi becomes "q_default" in the
-generated C code.
+incompatibly in a future release.
 
 In the rest of this document, usage lines are given for each
 expression type, with literal strings written in lower case and
@@ -643,6 +648,18 @@ any non-empty complex type (struct, union, or alternate), and a
 pointer to that QAPI type is passed as a single argument.
 
 
+=== Downstream extensions ===
+
+QAPI schema names that are externally visible, say in the Client JSON
+Protocol, need to be managed with care.  Names starting with a
+downstream prefix of the form __RFQDN_ are reserved for the downstream
+who controls the valid, reverse fully qualified domain name RFQDN.
+RFQDN may only contain ASCII letters, digits, hyphen and period.
+
+Example: Red Hat, Inc. controls redhat.com, and may therefore add a
+downstream command __com.redhat_drive-mirror.
+
+
 == Client JSON Protocol introspection ==
 
 Clients of a Client JSON Protocol commonly need to figure out what
-- 
2.7.4

Re: [Qemu-devel] [PATCH 20/24] docs/qapi-code-gen.txt: Clarify naming rules

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 21/24] test-qapi-util: New, covering qapi/qapi-util.c

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/.gitignore       |  1 +
 tests/Makefile.include |  3 +++
 tests/test-qapi-util.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+)
 create mode 100644 tests/test-qapi-util.c

diff --git a/tests/.gitignore b/tests/.gitignore
index 30b7740..a966740 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -53,6 +53,7 @@ test-mul64
 test-opts-visitor
 test-qapi-event.[ch]
 test-qapi-types.[ch]
+test-qapi-util
 test-qapi-visit.[ch]
 test-qdev-global-props
 test-qemu-opts
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 2171e4a..2b8301e 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -128,6 +128,8 @@ gcov-files-check-bufferiszero-y = util/bufferiszero.c
 check-unit-y += tests/test-uuid$(EXESUF)
 check-unit-y += tests/ptimer-test$(EXESUF)
 gcov-files-ptimer-test-y = hw/core/ptimer.c
+check-unit-y += tests/test-qapi-util$(EXESUF)
+gcov-files-test-qapi-util-y = qapi/qapi-util.c
 
 check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
 
@@ -733,6 +735,7 @@ tests/ivshmem-test$(EXESUF): tests/ivshmem-test.o contrib/ivshmem-server/ivshmem
 tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o contrib/libvhost-user/libvhost-user.o $(test-util-obj-y)
 tests/test-uuid$(EXESUF): tests/test-uuid.o $(test-util-obj-y)
 tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
+tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
 
 tests/migration/stress$(EXESUF): tests/migration/stress.o
 	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
diff --git a/tests/test-qapi-util.c b/tests/test-qapi-util.c
new file mode 100644
index 0000000..39db8bf
--- /dev/null
+++ b/tests/test-qapi-util.c
@@ -0,0 +1,51 @@
+/*
+ * Unit tests for QAPI utility functions
+ *
+ * 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/util.h"
+#include "test-qapi-types.h"
+
+static void test_qapi_enum_parse(void)
+{
+    Error *err = NULL;
+    int ret;
+
+    ret = qapi_enum_parse(QType_lookup, NULL, QTYPE__MAX, QTYPE_NONE,
+                          &error_abort);
+    g_assert_cmpint(ret, ==, QTYPE_NONE);
+
+    ret = qapi_enum_parse(QType_lookup, "junk", QTYPE__MAX, -1,
+                          NULL);
+    g_assert_cmpint(ret, ==, -1);
+
+    ret = qapi_enum_parse(QType_lookup, "junk", QTYPE__MAX, -1,
+                          &err);
+    error_free_or_abort(&err);
+
+    ret = qapi_enum_parse(QType_lookup, "none", QTYPE__MAX, -1,
+                          &error_abort);
+    g_assert_cmpint(ret, ==, QTYPE_NONE);
+
+    ret = qapi_enum_parse(QType_lookup, QType_lookup[QTYPE__MAX - 1],
+                          QTYPE__MAX, QTYPE__MAX - 1,
+                          &error_abort);
+    g_assert_cmpint(ret, ==, QTYPE__MAX - 1);
+}
+
+int main(int argc, char *argv[])
+{
+    g_test_init(&argc, &argv, NULL);
+    g_test_add_func("/qapi/util/qapi_enum_parse", test_qapi_enum_parse);
+    g_test_run();
+    return 0;
+}
-- 
2.7.4

Re: [Qemu-devel] [PATCH 21/24] test-qapi-util: New, covering qapi/qapi-util.c

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 22/24] qapi: New parse_qapi_name()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/util.h    |  2 ++
 qapi/qapi-util.c       | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 tests/test-qapi-util.c | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+)

diff --git a/include/qapi/util.h b/include/qapi/util.h
index 7ad26c0..7436ed8 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -14,4 +14,6 @@
 int qapi_enum_parse(const char * const lookup[], const char *buf,
                     int max, int def, Error **errp);
 
+int parse_qapi_name(const char *name, bool complete);
+
 #endif
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 818730a..53b27e2 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -33,3 +33,50 @@ int qapi_enum_parse(const char * const lookup[], const char *buf,
     error_setg(errp, "invalid parameter value: %s", buf);
     return def;
 }
+
+/*
+ * Parse a valid QAPI name from @str.
+ * A valid name consists of letters, digits, hyphen and underscore.
+ * It may be prefixed by __RFQDN_ (downstream extension), where RFQDN
+ * may contain only letters, digits and hyphen.
+ * The special exception for enumeration names is not implemented.
+ * See docs/qapi-code-gen.txt for more on QAPI naming rules.
+ * Keep this consistent with scripts/qapi.py!
+ * If @complete, the parse fails unless it consumes @str completely.
+ * Return its length on success, -1 on failure.
+ */
+int parse_qapi_name(const char *str, bool complete)
+{
+    const char *p = str;
+
+    if (*p == '_') {            /* Downstream __RFQDN_ */
+        p++;
+        if (*p != '_') {
+            return -1;
+        }
+        while (*++p) {
+            if (!qemu_isalnum(*p) && *p != '-' && *p != '.') {
+                break;
+            }
+        }
+
+        if (*p != '_') {
+            return -1;
+        }
+        p++;
+    }
+
+    if (!qemu_isalpha(*p)) {
+        return -1;
+    }
+    while (*++p) {
+        if (!qemu_isalnum(*p) && *p != '-' && *p != '_') {
+            break;
+        }
+    }
+
+    if (complete && *p) {
+        return -1;
+    }
+    return p - str;
+}
diff --git a/tests/test-qapi-util.c b/tests/test-qapi-util.c
index 39db8bf..e869757 100644
--- a/tests/test-qapi-util.c
+++ b/tests/test-qapi-util.c
@@ -42,10 +42,44 @@ static void test_qapi_enum_parse(void)
     g_assert_cmpint(ret, ==, QTYPE__MAX - 1);
 }
 
+static void test_parse_qapi_name(void)
+{
+    int ret;
+
+    /* Must start with a letter */
+    ret = parse_qapi_name("a", true);
+    g_assert(ret == 1);
+    ret = parse_qapi_name("a$", false);
+    g_assert(ret == 1);
+    ret = parse_qapi_name("", false);
+    g_assert(ret == -1);
+    ret = parse_qapi_name("1", false);
+    g_assert(ret == -1);
+
+    /* Only letters, digits, hyphen, underscore */
+    ret = parse_qapi_name("A-Za-z0-9_", true);
+    g_assert(ret == 10);
+    ret = parse_qapi_name("A-Za-z0-9_$", false);
+    g_assert(ret == 10);
+    ret = parse_qapi_name("A-Za-z0-9_$", true);
+    g_assert(ret == -1);
+
+    /* __RFQDN_ */
+    ret = parse_qapi_name("__com.redhat_supports", true);
+    g_assert(ret == 21);
+    ret = parse_qapi_name("_com.example_", false);
+    g_assert(ret == -1);
+    ret = parse_qapi_name("__com.example", false);
+    g_assert(ret == -1);
+    ret = parse_qapi_name("__com.example_", false);
+    g_assert(ret == -1);
+}
+
 int main(int argc, char *argv[])
 {
     g_test_init(&argc, &argv, NULL);
     g_test_add_func("/qapi/util/qapi_enum_parse", test_qapi_enum_parse);
+    g_test_add_func("/qapi/util/parse_qapi_name", test_parse_qapi_name);
     g_test_run();
     return 0;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 22/24] qapi: New parse_qapi_name()

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/util.h    |  2 ++
>  qapi/qapi-util.c       | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  tests/test-qapi-util.c | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 83 insertions(+)
> 
> diff --git a/include/qapi/util.h b/include/qapi/util.h
> index 7ad26c0..7436ed8 100644
> --- a/include/qapi/util.h
> +++ b/include/qapi/util.h
> @@ -14,4 +14,6 @@
>  int qapi_enum_parse(const char * const lookup[], const char *buf,
>                      int max, int def, Error **errp);
>  
> +int parse_qapi_name(const char *name, bool complete);
> +
>  #endif
> diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
> index 818730a..53b27e2 100644
> --- a/qapi/qapi-util.c
> +++ b/qapi/qapi-util.c
> @@ -33,3 +33,50 @@ int qapi_enum_parse(const char * const lookup[], const char *buf,
>      error_setg(errp, "invalid parameter value: %s", buf);
>      return def;
>  }
> +
> +/*
> + * Parse a valid QAPI name from @str.
> + * A valid name consists of letters, digits, hyphen and underscore.
> + * It may be prefixed by __RFQDN_ (downstream extension), where RFQDN
> + * may contain only letters, digits and hyphen.

And '.' according to the code below.

> + * The special exception for enumeration names is not implemented.
> + * See docs/qapi-code-gen.txt for more on QAPI naming rules.
> + * Keep this consistent with scripts/qapi.py!
> + * If @complete, the parse fails unless it consumes @str completely.
> + * Return its length on success, -1 on failure.
> + */

Kevin

Re: [Qemu-devel] [PATCH 22/24] qapi: New parse_qapi_name()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/qapi/util.h    |  2 ++
>>  qapi/qapi-util.c       | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/test-qapi-util.c | 34 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 83 insertions(+)
>> 
>> diff --git a/include/qapi/util.h b/include/qapi/util.h
>> index 7ad26c0..7436ed8 100644
>> --- a/include/qapi/util.h
>> +++ b/include/qapi/util.h
>> @@ -14,4 +14,6 @@
>>  int qapi_enum_parse(const char * const lookup[], const char *buf,
>>                      int max, int def, Error **errp);
>>  
>> +int parse_qapi_name(const char *name, bool complete);
>> +
>>  #endif
>> diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
>> index 818730a..53b27e2 100644
>> --- a/qapi/qapi-util.c
>> +++ b/qapi/qapi-util.c
>> @@ -33,3 +33,50 @@ int qapi_enum_parse(const char * const lookup[], const char *buf,
>>      error_setg(errp, "invalid parameter value: %s", buf);
>>      return def;
>>  }
>> +
>> +/*
>> + * Parse a valid QAPI name from @str.
>> + * A valid name consists of letters, digits, hyphen and underscore.
>> + * It may be prefixed by __RFQDN_ (downstream extension), where RFQDN
>> + * may contain only letters, digits and hyphen.
>
> And '.' according to the code below.

Of course.  Fixing the comment...

>> + * The special exception for enumeration names is not implemented.
>> + * See docs/qapi-code-gen.txt for more on QAPI naming rules.
>> + * Keep this consistent with scripts/qapi.py!
>> + * If @complete, the parse fails unless it consumes @str completely.
>> + * Return its length on success, -1 on failure.
>> + */
>
> Kevin

Re: [Qemu-devel] [PATCH 22/24] qapi: New parse_qapi_name()

[Eric Blake]

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

On 02/27/2017 05:20 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/util.h    |  2 ++
>  qapi/qapi-util.c       | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  tests/test-qapi-util.c | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 83 insertions(+)
> 

> +/*
> + * Parse a valid QAPI name from @str.
> + * A valid name consists of letters, digits, hyphen and underscore.
> + * It may be prefixed by __RFQDN_ (downstream extension), where RFQDN
> + * may contain only letters, digits and hyphen.

With the doc fix you discussed with Kevin,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 23/24] keyval: Restrict key components to valid QAPI names

[Markus Armbruster]

Restricting the key components to something sane leaves us room for
evolving key syntax.  Since they will be commonly used as QAPI member
names by the QObject input visitor, we can just as well borrow the
QAPI naming rules here.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-keyval.c | 10 ++++++++++
 util/keyval.c       | 12 ++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index f6496d7..6eceafb 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -41,6 +41,11 @@ static void test_keyval_parse(void)
     error_free_or_abort(&err);
     g_assert(!qdict);
 
+    /* Invalid non-empty key (qemu_opts_parse() doesn't care) */
+    qdict = keyval_parse("7up=val", NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
     /* Overlong key */
     memset(long_key, 'a', 127);
     long_key[127] = 'z';
@@ -73,6 +78,11 @@ static void test_keyval_parse(void)
     QDECREF(qdict);
     g_free(params);
 
+    /* Crap after valid key */
+    qdict = keyval_parse("key[0]=val", NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
     /* Multiple keys, last one wins */
     qdict = keyval_parse("a=1,b=2,,x,a=3", NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 2);
diff --git a/util/keyval.c b/util/keyval.c
index 3904c39..1170dad 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -34,6 +34,8 @@
  *   doesn't have one, because R.a must be an object to satisfy a.b=1
  *   and a string to satisfy a=2.
  *
+ * Key-fragments must be valid QAPI names.
+ *
  * The length of any key-fragment must be between 1 and 127.
  *
  * Design flaw: there is no way to denote an empty non-root object.
@@ -51,12 +53,12 @@
  * where no-key is syntactic sugar for implied-key=val-no-key.
  *
  * TODO support lists
- * TODO support key-fragment with __RFQDN_ prefix (downstream extensions)
  */
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/util.h"
 #include "qemu/option.h"
 
 /*
@@ -115,6 +117,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
     size_t len;
     char key_in_cur[128];
     QDict *cur;
+    int ret;
     QObject *next;
     QString *val;
 
@@ -134,9 +137,10 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
     cur = qdict;
     s = key;
     for (;;) {
-        for (len = 0; s + len < key_end && s[len] != '.'; len++) {
-        }
-        if (!len) {
+        ret = parse_qapi_name(s, false);
+        len = ret < 0 ? 0 : ret;
+        assert(s + len <= key_end);
+        if (!len || (s + len < key_end && s[len] != '.')) {
             assert(key != implied_key);
             error_setg(errp, "Invalid parameter '%.*s'",
                        (int)(key_end - key), key);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 23/24] keyval: Restrict key components to valid QAPI names

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Restricting the key components to something sane leaves us room for
> evolving key syntax.  Since they will be commonly used as QAPI member
> names by the QObject input visitor, we can just as well borrow the
> QAPI naming rules here.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 24/24] keyval: Support lists

[Markus Armbruster]

Additionally permit non-negative integers as key components.  A
dictionary's keys must either be all integers or none.  If all keys
are integers, convert the dictionary to a list.  The set of keys must
be [0,N].

Examples:

* list.1=goner,list.0=null,list.1=eins,list.2=zwei
  is equivalent to JSON [ "null", "eins", "zwei" ]

* a.b.c=1,a.b.0=2
  is inconsistent: a.b.c clashes with a.b.0

* list.0=null,list.2=eins,list.2=zwei
  has a hole: list.1 is missing

Similar design flaw as for objects: there is now way to denote an
empty list.  While interpreting "key absent" as empty list seems
natural (removing a list member from the input string works when there
are multiple ones, so why not when there's just one), it doesn't work:
"key absent" already means "optional list absent", which isn't the
same as "empty list present".

Update the keyval object visitor to use this a.0 syntax in error
messages rather than the usual a[0].

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/qobject-input-visitor.c |   5 +-
 tests/test-keyval.c          | 117 ++++++++++++++++++++++++++++
 util/keyval.c                | 177 ++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 286 insertions(+), 13 deletions(-)

diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 1d7b420..4c159e0 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -41,6 +41,7 @@ struct QObjectInputVisitor {
 
     /* Root of visit at visitor creation. */
     QObject *root;
+    bool keyval;                /* Assume @root made with keyval_parse() */
 
     /* Stack of objects being visited (all entries will be either
      * QDict or QList). */
@@ -73,7 +74,9 @@ static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
             g_string_prepend(qiv->errname, name ?: "<anonymous>");
             g_string_prepend_c(qiv->errname, '.');
         } else {
-            snprintf(buf, sizeof(buf), "[%u]", so->index);
+            snprintf(buf, sizeof(buf),
+                     qiv->keyval ? ".%u" : "[%u]",
+                     so->index);
             g_string_prepend(qiv->errname, buf);
         }
         name = so->name;
diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index 6eceafb..1ff6035 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -12,6 +12,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qstring.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qemu/cutils.h"
 #include "qemu/option.h"
@@ -183,6 +184,71 @@ static void test_keyval_parse(void)
     g_assert(!qdict);
 }
 
+static void check_list012(QList *qlist)
+{
+    static const char *expected[] = { "null", "eins", "zwei" };
+    int i;
+    QString *qstr;
+
+    g_assert(qlist);
+    for (i = 0; i < ARRAY_SIZE(expected); i++) {
+        qstr = qobject_to_qstring(qlist_pop(qlist));
+        g_assert(qstr);
+        g_assert_cmpstr(qstring_get_str(qstr), ==, expected[i]);
+    }
+    g_assert(qlist_empty(qlist));
+}
+
+static void test_keyval_parse_list(void)
+{
+    Error *err = NULL;
+    QDict *qdict, *sub_qdict;
+
+    /* Root can't be a list */
+    qdict = keyval_parse("0=1", NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* List elements need not be in order */
+    qdict = keyval_parse("list.0=null,list.2=zwei,list.1=eins",
+                         NULL, &error_abort);
+    g_assert_cmpint(qdict_size(qdict), ==, 1);
+    check_list012(qdict_get_qlist(qdict, "list"));
+    QDECREF(qdict);
+
+    /* Multiple indexes, last one wins */
+    qdict = keyval_parse("list.1=goner,list.0=null,list.1=eins,list.2=zwei",
+                         NULL, &error_abort);
+    g_assert_cmpint(qdict_size(qdict), ==, 1);
+    check_list012(qdict_get_qlist(qdict, "list"));
+    QDECREF(qdict);
+
+    /* List at deeper nesting */
+    qdict = keyval_parse("a.list.1=eins,a.list.0=null,a.list.2=zwei",
+                         NULL, &error_abort);
+    g_assert_cmpint(qdict_size(qdict), ==, 1);
+    sub_qdict = qdict_get_qdict(qdict, "a");
+    g_assert_cmpint(qdict_size(sub_qdict), ==, 1);
+    check_list012(qdict_get_qlist(sub_qdict, "list"));
+    QDECREF(qdict);
+
+    /* Inconsistent dotted keys: both list and dictionary */
+    qdict = keyval_parse("a.b.c=1,a.b.0=2", NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+    qdict = keyval_parse("a.0.c=1,a.b.c=2", NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* Missing list indexes */
+    qdict = keyval_parse("list.2=lonely", NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+    qdict = keyval_parse("list.0=null,list.2=eins,list.2=zwei", NULL, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+}
+
 static void test_keyval_visit_bool(void)
 {
     Error *err = NULL;
@@ -459,6 +525,55 @@ static void test_keyval_visit_dict(void)
     visit_free(v);
 }
 
+static void test_keyval_visit_list(void)
+{
+    Error *err = NULL;
+    Visitor *v;
+    QDict *qdict;
+    char *s;
+
+    qdict = keyval_parse("a.0=,a.1=I,a.2.0=II", NULL, &error_abort);
+    /* TODO empty list */
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_start_list(v, "a", NULL, 0, &error_abort);
+    visit_type_str(v, NULL, &s, &error_abort);
+    g_assert_cmpstr(s, ==, "");
+    visit_type_str(v, NULL, &s, &error_abort);
+    g_assert_cmpstr(s, ==, "I");
+    visit_start_list(v, NULL, NULL, 0, &error_abort);
+    visit_type_str(v, NULL, &s, &error_abort);
+    g_assert_cmpstr(s, ==, "II");
+    visit_check_list(v, &error_abort);
+    visit_end_list(v, NULL);
+    visit_check_list(v, &error_abort);
+    visit_end_list(v, NULL);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    qdict = keyval_parse("a.0=,b.0.0=head", NULL, &error_abort);
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_start_list(v, "a", NULL, 0, &error_abort);
+    visit_check_list(v, &err);  /* a[0] unexpected */
+    error_free_or_abort(&err);
+    visit_end_list(v, NULL);
+    visit_start_list(v, "b", NULL, 0, &error_abort);
+    visit_start_list(v, NULL, NULL, 0, &error_abort);
+    visit_type_str(v, NULL, &s, &error_abort);
+    g_assert_cmpstr(s, ==, "head");
+    visit_type_str(v, NULL, &s, &err); /* b[0][1] missing */
+    error_free_or_abort(&err);
+    visit_end_list(v, NULL);
+    visit_end_list(v, NULL);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+}
+
 static void test_keyval_visit_optional(void)
 {
     Visitor *v;
@@ -492,10 +607,12 @@ 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/keyval_parse/list", test_keyval_parse_list);
     g_test_add_func("/keyval/visit/bool", test_keyval_visit_bool);
     g_test_add_func("/keyval/visit/number", test_keyval_visit_number);
     g_test_add_func("/keyval/visit/size", test_keyval_visit_size);
     g_test_add_func("/keyval/visit/dict", test_keyval_visit_dict);
+    g_test_add_func("/keyval/visit/list", test_keyval_visit_list);
     g_test_add_func("/keyval/visit/optional", test_keyval_visit_optional);
     g_test_run();
     return 0;
diff --git a/util/keyval.c b/util/keyval.c
index 1170dad..3621f28 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -21,10 +21,12 @@
  *
  * Semantics defined by reduction to JSON:
  *
- *   key-vals defines a tree of objects rooted at R
+ *   key-vals is a tree of objects and arrays rooted at object R
  *   where for each key-val = key-fragment . ... = val in key-vals
  *       R op key-fragment op ... = val'
- *       where (left-associative) op is member reference L.key-fragment
+ *       where (left-associative) op is
+ *                 array subscript L[key-fragment] for numeric key-fragment
+ *                 member reference L.key-fragment otherwise
  *             val' is val with ',,' replaced by ','
  *   and only R may be empty.
  *
@@ -34,16 +36,16 @@
  *   doesn't have one, because R.a must be an object to satisfy a.b=1
  *   and a string to satisfy a=2.
  *
- * Key-fragments must be valid QAPI names.
+ * Key-fragments must be valid QAPI names or consist only of digits.
  *
  * The length of any key-fragment must be between 1 and 127.
  *
- * Design flaw: there is no way to denote an empty non-root object.
- * While interpreting "key absent" as empty object seems natural
+ * Design flaw: there is no way to denote an empty array or non-root
+ * object.  While interpreting "key absent" as empty seems natural
  * (removing a key-val from the input string removes the member when
  * there are more, so why not when it's the last), it doesn't work:
- * "key absent" already means "optional object absent", which isn't
- * the same as "empty object present".
+ * "key absent" already means "optional object/array absent", which
+ * isn't the same as "empty object/array present".
  *
  * Additional syntax for use with an implied key:
  *
@@ -51,17 +53,43 @@
  *   val-no-key   = / [^,]* /
  *
  * where no-key is syntactic sugar for implied-key=val-no-key.
- *
- * TODO support lists
  */
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qstring.h"
 #include "qapi/util.h"
+#include "qemu/cutils.h"
 #include "qemu/option.h"
 
 /*
+ * Convert @key to a list index.
+ * Convert all leading digits to a (non-negative) number, capped at
+ * INT_MAX.
+ * If @end is non-null, assign a pointer to the first character after
+ * the number to *@end.
+ * Else, fail if any characters follow.
+ * On success, return the converted number.
+ * On failure, return a negative value.
+ * Note: since only digits are converted, no two keys can map to the
+ * same number, except by overflow to INT_MAX.
+ */
+static int key_to_index(const char *key, const char **end)
+{
+    int ret;
+    unsigned long index;
+
+    if (*key < '0' || *key > '9') {
+        return -EINVAL;
+    }
+    ret = qemu_strtoul(key, end, 10, &index);
+    if (ret) {
+        return ret == -ERANGE ? INT_MAX : ret;
+    }
+    return index <= INT_MAX ? index : INT_MAX;
+}
+
+/*
  * Ensure @cur maps @key_in_cur the right way.
  * If @value is null, it needs to map to a QDict, else to this
  * QString.
@@ -113,7 +141,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
                                     const char *implied_key,
                                     Error **errp)
 {
-    const char *key, *key_end, *s;
+    const char *key, *key_end, *s, *end;
     size_t len;
     char key_in_cur[128];
     QDict *cur;
@@ -137,8 +165,13 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
     cur = qdict;
     s = key;
     for (;;) {
-        ret = parse_qapi_name(s, false);
-        len = ret < 0 ? 0 : ret;
+        /* Want a key index (unless it's first) or a QAPI name */
+        if (s != key && key_to_index(s, &end) >= 0) {
+            len = end - s;
+        } else {
+            ret = parse_qapi_name(s, false);
+            len = ret < 0 ? 0 : ret;
+        }
         assert(s + len <= key_end);
         if (!len || (s + len < key_end && s[len] != '.')) {
             assert(key != implied_key);
@@ -205,6 +238,119 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
     return s;
 }
 
+static char *reassemble_key(GSList *key)
+{
+    GString *s = g_string_new("");
+    GSList *p;
+
+    for (p = key; p; p = p->next) {
+        g_string_prepend_c(s, '.');
+        g_string_prepend(s, (char *)p->data);
+    }
+
+    return g_string_free(s, FALSE);
+}
+
+/*
+ * Listify @cur recursively.
+ * Replace QDicts whose keys are all valid list indexes by QLists.
+ * @key_of_cur is the list of key fragments leading up to @cur.
+ * On success, return either @cur or its replacement.
+ * On failure, store an error through @errp and return NULL.
+ */
+static QObject *keyval_listify(QDict *cur, GSList *key_of_cur, Error **errp)
+{
+    GSList key_node;
+    bool has_index, has_member;
+    const QDictEntry *ent;
+    QDict *qdict;
+    QObject *val;
+    char *key;
+    size_t nelt;
+    QObject **elt;
+    int index, i;
+    QList *list;
+
+    key_node.next = key_of_cur;
+
+    /*
+     * Recursively listify @cur's members, and figure out whether @cur
+     * itself is to be listified.
+     */
+    has_index = false;
+    has_member = false;
+    for (ent = qdict_first(cur); ent; ent = qdict_next(cur, ent)) {
+        if (key_to_index(ent->key, NULL) >= 0) {
+            has_index = true;
+        } else {
+            has_member = true;
+        }
+
+        qdict = qobject_to_qdict(ent->value);
+        if (!qdict) {
+            continue;
+        }
+
+        key_node.data = ent->key;
+        val = keyval_listify(qdict, &key_node, errp);
+        if (!val) {
+            return NULL;
+        }
+        if (val != ent->value) {
+            qdict_put_obj(cur, ent->key, val);
+        }
+    }
+
+    if (has_index && has_member) {
+        key = reassemble_key(key_of_cur);
+        error_setg(errp, "Parameters '%s*' used inconsistently", key);
+        g_free(key);
+        return NULL;
+    }
+    if (!has_index) {
+        return QOBJECT(cur);
+    }
+
+    /* Copy @cur's values to @elt[] */
+    nelt = qdict_size(cur);
+    elt = g_new0(QObject *, nelt);
+    for (ent = qdict_first(cur); ent; ent = qdict_next(cur, ent)) {
+        index = key_to_index(ent->key, NULL);
+        assert(index >= 0);
+        /*
+         * We iterate @nelt times.  Because the dictionary keys are
+         * distinct, the indexes are also distinct (key_to_index()
+         * ensures it).  If we get one exceeding @nelt here, we will
+         * leave a hole in @elt[], triggering the error in the next
+         * loop.
+         *
+         * Well, I lied.  key_to_index() can return INT_MAX multiple
+         * times, but INT_MAX surely exceeds @nelt.
+         */
+        if ((size_t)index >= nelt) {
+            continue;
+        }
+        assert(!elt[index]);
+        elt[index] = ent->value;
+        qobject_incref(elt[index]);
+    }
+
+    /* Make a list from @elt[] */
+    list = qlist_new();
+    for (i = 0; i < nelt; i++) {
+        if (!elt[i]) {
+            key = reassemble_key(key_of_cur);
+            error_setg(errp, "Parameter '%s%d' missing", key, i);
+            g_free(key);
+            QDECREF(list);
+            return NULL;
+        }
+        qlist_append_obj(list, elt[i]);
+    }
+
+    return QOBJECT(list);
+}
+
 /*
  * Parse @params in QEMU's traditional KEY=VALUE,... syntax.
  * If @implied_key, the first KEY= can be omitted.  @implied_key is
@@ -216,6 +362,7 @@ QDict *keyval_parse(const char *params, const char *implied_key,
                     Error **errp)
 {
     QDict *qdict = qdict_new();
+    QObject *listified;
     const char *s;
 
     s = params;
@@ -228,5 +375,11 @@ QDict *keyval_parse(const char *params, const char *implied_key,
         implied_key = NULL;
     }
 
+    listified = keyval_listify(qdict, NULL, errp);
+    if (!listified) {
+        QDECREF(qdict);
+        return NULL;
+    }
+    assert(listified == QOBJECT(qdict));
     return qdict;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 24/24] keyval: Support lists

[Kevin Wolf]

Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
> Additionally permit non-negative integers as key components.  A
> dictionary's keys must either be all integers or none.  If all keys
> are integers, convert the dictionary to a list.  The set of keys must
> be [0,N].
> 
> Examples:
> 
> * list.1=goner,list.0=null,list.1=eins,list.2=zwei
>   is equivalent to JSON [ "null", "eins", "zwei" ]
> 
> * a.b.c=1,a.b.0=2
>   is inconsistent: a.b.c clashes with a.b.0
> 
> * list.0=null,list.2=eins,list.2=zwei
>   has a hole: list.1 is missing
> 
> Similar design flaw as for objects: there is now way to denote an

s/now/no/

> empty list.  While interpreting "key absent" as empty list seems
> natural (removing a list member from the input string works when there
> are multiple ones, so why not when there's just one), it doesn't work:
> "key absent" already means "optional list absent", which isn't the
> same as "empty list present".
> 
> Update the keyval object visitor to use this a.0 syntax in error
> messages rather than the usual a[0].
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

valgrind finds a quite a few leaks while running tests/test-keyval. Not
sure if I caught all of them, maybe you want to just run it yourself.

>  qapi/qobject-input-visitor.c |   5 +-
>  tests/test-keyval.c          | 117 ++++++++++++++++++++++++++++
>  util/keyval.c                | 177 ++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 286 insertions(+), 13 deletions(-)
> 
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 1d7b420..4c159e0 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -41,6 +41,7 @@ struct QObjectInputVisitor {
>  
>      /* Root of visit at visitor creation. */
>      QObject *root;
> +    bool keyval;                /* Assume @root made with keyval_parse() */

Who sets this? I can't seem to find it in this patch, and it's the final
patch of the series.

>      /* Stack of objects being visited (all entries will be either
>       * QDict or QList). */
> @@ -73,7 +74,9 @@ static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
>              g_string_prepend(qiv->errname, name ?: "<anonymous>");
>              g_string_prepend_c(qiv->errname, '.');
>          } else {
> -            snprintf(buf, sizeof(buf), "[%u]", so->index);
> +            snprintf(buf, sizeof(buf),
> +                     qiv->keyval ? ".%u" : "[%u]",
> +                     so->index);
>              g_string_prepend(qiv->errname, buf);
>          }
>          name = so->name;

> diff --git a/util/keyval.c b/util/keyval.c
> index 1170dad..3621f28 100644
> --- a/util/keyval.c
> +++ b/util/keyval.c
> @@ -21,10 +21,12 @@
>   *
>   * Semantics defined by reduction to JSON:
>   *
> - *   key-vals defines a tree of objects rooted at R
> + *   key-vals is a tree of objects and arrays rooted at object R
>   *   where for each key-val = key-fragment . ... = val in key-vals
>   *       R op key-fragment op ... = val'
> - *       where (left-associative) op is member reference L.key-fragment
> + *       where (left-associative) op is
> + *                 array subscript L[key-fragment] for numeric key-fragment
> + *                 member reference L.key-fragment otherwise
>   *             val' is val with ',,' replaced by ','
>   *   and only R may be empty.
>   *
> @@ -34,16 +36,16 @@
>   *   doesn't have one, because R.a must be an object to satisfy a.b=1
>   *   and a string to satisfy a=2.
>   *
> - * Key-fragments must be valid QAPI names.
> + * Key-fragments must be valid QAPI names or consist only of digits.
>   *
>   * The length of any key-fragment must be between 1 and 127.
>   *
> - * Design flaw: there is no way to denote an empty non-root object.
> - * While interpreting "key absent" as empty object seems natural
> + * Design flaw: there is no way to denote an empty array or non-root
> + * object.  While interpreting "key absent" as empty seems natural
>   * (removing a key-val from the input string removes the member when
>   * there are more, so why not when it's the last), it doesn't work:
> - * "key absent" already means "optional object absent", which isn't
> - * the same as "empty object present".
> + * "key absent" already means "optional object/array absent", which
> + * isn't the same as "empty object/array present".
>   *
>   * Additional syntax for use with an implied key:
>   *
> @@ -51,17 +53,43 @@
>   *   val-no-key   = / [^,]* /
>   *
>   * where no-key is syntactic sugar for implied-key=val-no-key.
> - *
> - * TODO support lists
>   */
>  
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qstring.h"
>  #include "qapi/util.h"
> +#include "qemu/cutils.h"
>  #include "qemu/option.h"
>  
>  /*
> + * Convert @key to a list index.
> + * Convert all leading digits to a (non-negative) number, capped at
> + * INT_MAX.
> + * If @end is non-null, assign a pointer to the first character after
> + * the number to *@end.
> + * Else, fail if any characters follow.
> + * On success, return the converted number.
> + * On failure, return a negative value.
> + * Note: since only digits are converted, no two keys can map to the
> + * same number, except by overflow to INT_MAX.
> + */
> +static int key_to_index(const char *key, const char **end)
> +{
> +    int ret;
> +    unsigned long index;
> +
> +    if (*key < '0' || *key > '9') {
> +        return -EINVAL;
> +    }
> +    ret = qemu_strtoul(key, end, 10, &index);
> +    if (ret) {
> +        return ret == -ERANGE ? INT_MAX : ret;
> +    }
> +    return index <= INT_MAX ? index : INT_MAX;
> +}
> +
> +/*
>   * Ensure @cur maps @key_in_cur the right way.
>   * If @value is null, it needs to map to a QDict, else to this
>   * QString.
> @@ -113,7 +141,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>                                      const char *implied_key,
>                                      Error **errp)
>  {
> -    const char *key, *key_end, *s;
> +    const char *key, *key_end, *s, *end;
>      size_t len;
>      char key_in_cur[128];
>      QDict *cur;
> @@ -137,8 +165,13 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>      cur = qdict;
>      s = key;
>      for (;;) {
> -        ret = parse_qapi_name(s, false);
> -        len = ret < 0 ? 0 : ret;
> +        /* Want a key index (unless it's first) or a QAPI name */
> +        if (s != key && key_to_index(s, &end) >= 0) {
> +            len = end - s;
> +        } else {
> +            ret = parse_qapi_name(s, false);
> +            len = ret < 0 ? 0 : ret;
> +        }
>          assert(s + len <= key_end);
>          if (!len || (s + len < key_end && s[len] != '.')) {
>              assert(key != implied_key);
> @@ -205,6 +238,119 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>      return s;
>  }
>  
> +static char *reassemble_key(GSList *key)
> +{
> +    GString *s = g_string_new("");
> +    GSList *p;
> +
> +    for (p = key; p; p = p->next) {
> +        g_string_prepend_c(s, '.');
> +        g_string_prepend(s, (char *)p->data);
> +    }
> +
> +    return g_string_free(s, FALSE);
> +}
> +
> +/*
> + * Listify @cur recursively.
> + * Replace QDicts whose keys are all valid list indexes by QLists.
> + * @key_of_cur is the list of key fragments leading up to @cur.
> + * On success, return either @cur or its replacement.
> + * On failure, store an error through @errp and return NULL.
> + */
> +static QObject *keyval_listify(QDict *cur, GSList *key_of_cur, Error **errp)
> +{
> +    GSList key_node;
> +    bool has_index, has_member;
> +    const QDictEntry *ent;
> +    QDict *qdict;
> +    QObject *val;
> +    char *key;
> +    size_t nelt;
> +    QObject **elt;
> +    int index, i;
> +    QList *list;
> +
> +    key_node.next = key_of_cur;
> +
> +    /*
> +     * Recursively listify @cur's members, and figure out whether @cur
> +     * itself is to be listified.
> +     */
> +    has_index = false;
> +    has_member = false;
> +    for (ent = qdict_first(cur); ent; ent = qdict_next(cur, ent)) {
> +        if (key_to_index(ent->key, NULL) >= 0) {
> +            has_index = true;
> +        } else {
> +            has_member = true;
> +        }
> +
> +        qdict = qobject_to_qdict(ent->value);
> +        if (!qdict) {
> +            continue;
> +        }
> +
> +        key_node.data = ent->key;
> +        val = keyval_listify(qdict, &key_node, errp);
> +        if (!val) {
> +            return NULL;
> +        }
> +        if (val != ent->value) {
> +            qdict_put_obj(cur, ent->key, val);
> +        }
> +    }
> +
> +    if (has_index && has_member) {
> +        key = reassemble_key(key_of_cur);
> +        error_setg(errp, "Parameters '%s*' used inconsistently", key);
> +        g_free(key);
> +        return NULL;
> +    }
> +    if (!has_index) {
> +        return QOBJECT(cur);
> +    }
> +
> +    /* Copy @cur's values to @elt[] */
> +    nelt = qdict_size(cur);
> +    elt = g_new0(QObject *, nelt);

This doesn't seem to be freed.

> +    for (ent = qdict_first(cur); ent; ent = qdict_next(cur, ent)) {
> +        index = key_to_index(ent->key, NULL);
> +        assert(index >= 0);
> +        /*
> +         * We iterate @nelt times.  Because the dictionary keys are
> +         * distinct, the indexes are also distinct (key_to_index()
> +         * ensures it).

Really? What about leading zeros?

> +                        If we get one exceeding @nelt here, we will
> +         * leave a hole in @elt[], triggering the error in the next
> +         * loop.
> +         *
> +         * Well, I lied.  key_to_index() can return INT_MAX multiple
> +         * times, but INT_MAX surely exceeds @nelt.
> +         */
> +        if ((size_t)index >= nelt) {
> +            continue;
> +        }
> +        assert(!elt[index]);
> +        elt[index] = ent->value;
> +        qobject_incref(elt[index]);

We forget to decref this in error cases. Maybe it would be better to
only incref immediately before qlist_append_obj().

> +    }
> +
> +    /* Make a list from @elt[] */
> +    list = qlist_new();
> +    for (i = 0; i < nelt; i++) {
> +        if (!elt[i]) {
> +            key = reassemble_key(key_of_cur);
> +            error_setg(errp, "Parameter '%s%d' missing", key, i);
> +            g_free(key);
> +            QDECREF(list);
> +            return NULL;
> +        }
> +        qlist_append_obj(list, elt[i]);
> +    }
> +
> +    return QOBJECT(list);
> +}
> +
>  /*
>   * Parse @params in QEMU's traditional KEY=VALUE,... syntax.
>   * If @implied_key, the first KEY= can be omitted.  @implied_key is
> @@ -216,6 +362,7 @@ QDict *keyval_parse(const char *params, const char *implied_key,
>                      Error **errp)
>  {
>      QDict *qdict = qdict_new();
> +    QObject *listified;
>      const char *s;
>  
>      s = params;
> @@ -228,5 +375,11 @@ QDict *keyval_parse(const char *params, const char *implied_key,
>          implied_key = NULL;
>      }
>  
> +    listified = keyval_listify(qdict, NULL, errp);
> +    if (!listified) {
> +        QDECREF(qdict);
> +        return NULL;
> +    }
> +    assert(listified == QOBJECT(qdict));
>      return qdict;
>  }

Kevin

Re: [Qemu-devel] [PATCH 24/24] keyval: Support lists

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>> Additionally permit non-negative integers as key components.  A
>> dictionary's keys must either be all integers or none.  If all keys
>> are integers, convert the dictionary to a list.  The set of keys must
>> be [0,N].
>> 
>> Examples:
>> 
>> * list.1=goner,list.0=null,list.1=eins,list.2=zwei
>>   is equivalent to JSON [ "null", "eins", "zwei" ]
>> 
>> * a.b.c=1,a.b.0=2
>>   is inconsistent: a.b.c clashes with a.b.0
>> 
>> * list.0=null,list.2=eins,list.2=zwei
>>   has a hole: list.1 is missing
>> 
>> Similar design flaw as for objects: there is now way to denote an
>
> s/now/no/

Fixing...

>> empty list.  While interpreting "key absent" as empty list seems
>> natural (removing a list member from the input string works when there
>> are multiple ones, so why not when there's just one), it doesn't work:
>> "key absent" already means "optional list absent", which isn't the
>> same as "empty list present".
>> 
>> Update the keyval object visitor to use this a.0 syntax in error
>> messages rather than the usual a[0].
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> valgrind finds a quite a few leaks while running tests/test-keyval. Not
> sure if I caught all of them, maybe you want to just run it yourself.

Will do.

>>  qapi/qobject-input-visitor.c |   5 +-
>>  tests/test-keyval.c          | 117 ++++++++++++++++++++++++++++
>>  util/keyval.c                | 177 ++++++++++++++++++++++++++++++++++++++++---
>>  3 files changed, 286 insertions(+), 13 deletions(-)
>> 
>> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>> index 1d7b420..4c159e0 100644
>> --- a/qapi/qobject-input-visitor.c
>> +++ b/qapi/qobject-input-visitor.c
>> @@ -41,6 +41,7 @@ struct QObjectInputVisitor {
>>  
>>      /* Root of visit at visitor creation. */
>>      QObject *root;
>> +    bool keyval;                /* Assume @root made with keyval_parse() */
>
> Who sets this? I can't seem to find it in this patch, and it's the final
> patch of the series.

I either forgot or lost its initialization in
qobject_input_visitor_new_keyval().  Fixing...

>>      /* Stack of objects being visited (all entries will be either
>>       * QDict or QList). */
>> @@ -73,7 +74,9 @@ static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
>>              g_string_prepend(qiv->errname, name ?: "<anonymous>");
>>              g_string_prepend_c(qiv->errname, '.');
>>          } else {
>> -            snprintf(buf, sizeof(buf), "[%u]", so->index);
>> +            snprintf(buf, sizeof(buf),
>> +                     qiv->keyval ? ".%u" : "[%u]",
>> +                     so->index);
>>              g_string_prepend(qiv->errname, buf);
>>          }
>>          name = so->name;
>
>> diff --git a/util/keyval.c b/util/keyval.c
>> index 1170dad..3621f28 100644
>> --- a/util/keyval.c
>> +++ b/util/keyval.c
>> @@ -21,10 +21,12 @@
>>   *
>>   * Semantics defined by reduction to JSON:
>>   *
>> - *   key-vals defines a tree of objects rooted at R
>> + *   key-vals is a tree of objects and arrays rooted at object R
>>   *   where for each key-val = key-fragment . ... = val in key-vals
>>   *       R op key-fragment op ... = val'
>> - *       where (left-associative) op is member reference L.key-fragment
>> + *       where (left-associative) op is
>> + *                 array subscript L[key-fragment] for numeric key-fragment
>> + *                 member reference L.key-fragment otherwise
>>   *             val' is val with ',,' replaced by ','
>>   *   and only R may be empty.
>>   *
>> @@ -34,16 +36,16 @@
>>   *   doesn't have one, because R.a must be an object to satisfy a.b=1
>>   *   and a string to satisfy a=2.
>>   *
>> - * Key-fragments must be valid QAPI names.
>> + * Key-fragments must be valid QAPI names or consist only of digits.
>>   *
>>   * The length of any key-fragment must be between 1 and 127.
>>   *
>> - * Design flaw: there is no way to denote an empty non-root object.
>> - * While interpreting "key absent" as empty object seems natural
>> + * Design flaw: there is no way to denote an empty array or non-root
>> + * object.  While interpreting "key absent" as empty seems natural
>>   * (removing a key-val from the input string removes the member when
>>   * there are more, so why not when it's the last), it doesn't work:
>> - * "key absent" already means "optional object absent", which isn't
>> - * the same as "empty object present".
>> + * "key absent" already means "optional object/array absent", which
>> + * isn't the same as "empty object/array present".
>>   *
>>   * Additional syntax for use with an implied key:
>>   *
>> @@ -51,17 +53,43 @@
>>   *   val-no-key   = / [^,]* /
>>   *
>>   * where no-key is syntactic sugar for implied-key=val-no-key.
>> - *
>> - * TODO support lists
>>   */
>>  
>>  #include "qemu/osdep.h"
>>  #include "qapi/error.h"
>>  #include "qapi/qmp/qstring.h"
>>  #include "qapi/util.h"
>> +#include "qemu/cutils.h"
>>  #include "qemu/option.h"
>>  
>>  /*
>> + * Convert @key to a list index.
>> + * Convert all leading digits to a (non-negative) number, capped at
>> + * INT_MAX.
>> + * If @end is non-null, assign a pointer to the first character after
>> + * the number to *@end.
>> + * Else, fail if any characters follow.
>> + * On success, return the converted number.
>> + * On failure, return a negative value.
>> + * Note: since only digits are converted, no two keys can map to the
>> + * same number, except by overflow to INT_MAX.
>> + */
>> +static int key_to_index(const char *key, const char **end)
>> +{
>> +    int ret;
>> +    unsigned long index;
>> +
>> +    if (*key < '0' || *key > '9') {
>> +        return -EINVAL;
>> +    }
>> +    ret = qemu_strtoul(key, end, 10, &index);
>> +    if (ret) {
>> +        return ret == -ERANGE ? INT_MAX : ret;
>> +    }
>> +    return index <= INT_MAX ? index : INT_MAX;
>> +}
>> +
>> +/*
>>   * Ensure @cur maps @key_in_cur the right way.
>>   * If @value is null, it needs to map to a QDict, else to this
>>   * QString.
>> @@ -113,7 +141,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>>                                      const char *implied_key,
>>                                      Error **errp)
>>  {
>> -    const char *key, *key_end, *s;
>> +    const char *key, *key_end, *s, *end;
>>      size_t len;
>>      char key_in_cur[128];
>>      QDict *cur;
>> @@ -137,8 +165,13 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>>      cur = qdict;
>>      s = key;
>>      for (;;) {
>> -        ret = parse_qapi_name(s, false);
>> -        len = ret < 0 ? 0 : ret;
>> +        /* Want a key index (unless it's first) or a QAPI name */
>> +        if (s != key && key_to_index(s, &end) >= 0) {
>> +            len = end - s;
>> +        } else {
>> +            ret = parse_qapi_name(s, false);
>> +            len = ret < 0 ? 0 : ret;
>> +        }
>>          assert(s + len <= key_end);
>>          if (!len || (s + len < key_end && s[len] != '.')) {
>>              assert(key != implied_key);
>> @@ -205,6 +238,119 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>>      return s;
>>  }
>>  
>> +static char *reassemble_key(GSList *key)
>> +{
>> +    GString *s = g_string_new("");
>> +    GSList *p;
>> +
>> +    for (p = key; p; p = p->next) {
>> +        g_string_prepend_c(s, '.');
>> +        g_string_prepend(s, (char *)p->data);
>> +    }
>> +
>> +    return g_string_free(s, FALSE);
>> +}
>> +
>> +/*
>> + * Listify @cur recursively.
>> + * Replace QDicts whose keys are all valid list indexes by QLists.
>> + * @key_of_cur is the list of key fragments leading up to @cur.
>> + * On success, return either @cur or its replacement.
>> + * On failure, store an error through @errp and return NULL.
>> + */
>> +static QObject *keyval_listify(QDict *cur, GSList *key_of_cur, Error **errp)
>> +{
>> +    GSList key_node;
>> +    bool has_index, has_member;
>> +    const QDictEntry *ent;
>> +    QDict *qdict;
>> +    QObject *val;
>> +    char *key;
>> +    size_t nelt;
>> +    QObject **elt;
>> +    int index, i;
>> +    QList *list;
>> +
>> +    key_node.next = key_of_cur;
>> +
>> +    /*
>> +     * Recursively listify @cur's members, and figure out whether @cur
>> +     * itself is to be listified.
>> +     */
>> +    has_index = false;
>> +    has_member = false;
>> +    for (ent = qdict_first(cur); ent; ent = qdict_next(cur, ent)) {
>> +        if (key_to_index(ent->key, NULL) >= 0) {
>> +            has_index = true;
>> +        } else {
>> +            has_member = true;
>> +        }
>> +
>> +        qdict = qobject_to_qdict(ent->value);
>> +        if (!qdict) {
>> +            continue;
>> +        }
>> +
>> +        key_node.data = ent->key;
>> +        val = keyval_listify(qdict, &key_node, errp);
>> +        if (!val) {
>> +            return NULL;
>> +        }
>> +        if (val != ent->value) {
>> +            qdict_put_obj(cur, ent->key, val);
>> +        }
>> +    }
>> +
>> +    if (has_index && has_member) {
>> +        key = reassemble_key(key_of_cur);
>> +        error_setg(errp, "Parameters '%s*' used inconsistently", key);
>> +        g_free(key);
>> +        return NULL;
>> +    }
>> +    if (!has_index) {
>> +        return QOBJECT(cur);
>> +    }
>> +
>> +    /* Copy @cur's values to @elt[] */
>> +    nelt = qdict_size(cur);
>> +    elt = g_new0(QObject *, nelt);
>
> This doesn't seem to be freed.

Yup.  Fixing...

>> +    for (ent = qdict_first(cur); ent; ent = qdict_next(cur, ent)) {
>> +        index = key_to_index(ent->key, NULL);
>> +        assert(index >= 0);
>> +        /*
>> +         * We iterate @nelt times.  Because the dictionary keys are
>> +         * distinct, the indexes are also distinct (key_to_index()
>> +         * ensures it).
>
> Really? What about leading zeros?

Bummer.

You know, my first version of this code didn't rely on distinct indexes,
but I found it a bit complicated, so I "simplified" it.  I'll dig it out
of git.

>> +                        If we get one exceeding @nelt here, we will
>> +         * leave a hole in @elt[], triggering the error in the next
>> +         * loop.
>> +         *
>> +         * Well, I lied.  key_to_index() can return INT_MAX multiple
>> +         * times, but INT_MAX surely exceeds @nelt.
>> +         */
>> +        if ((size_t)index >= nelt) {
>> +            continue;
>> +        }
>> +        assert(!elt[index]);
>> +        elt[index] = ent->value;
>> +        qobject_incref(elt[index]);
>
> We forget to decref this in error cases. Maybe it would be better to
> only incref immediately before qlist_append_obj().

Will fix one way or another.

>> +    }
>> +
>> +    /* Make a list from @elt[] */
>> +    list = qlist_new();
>> +    for (i = 0; i < nelt; i++) {
>> +        if (!elt[i]) {
>> +            key = reassemble_key(key_of_cur);
>> +            error_setg(errp, "Parameter '%s%d' missing", key, i);
>> +            g_free(key);
>> +            QDECREF(list);
>> +            return NULL;
>> +        }
>> +        qlist_append_obj(list, elt[i]);
>> +    }
>> +
>> +    return QOBJECT(list);
>> +}
>> +
>>  /*
>>   * Parse @params in QEMU's traditional KEY=VALUE,... syntax.
>>   * If @implied_key, the first KEY= can be omitted.  @implied_key is
>> @@ -216,6 +362,7 @@ QDict *keyval_parse(const char *params, const char *implied_key,
>>                      Error **errp)
>>  {
>>      QDict *qdict = qdict_new();
>> +    QObject *listified;
>>      const char *s;
>>  
>>      s = params;
>> @@ -228,5 +375,11 @@ QDict *keyval_parse(const char *params, const char *implied_key,
>>          implied_key = NULL;
>>      }
>>  
>> +    listified = keyval_listify(qdict, NULL, errp);
>> +    if (!listified) {
>> +        QDECREF(qdict);
>> +        return NULL;
>> +    }
>> +    assert(listified == QOBJECT(qdict));
>>      return qdict;
>>  }
>
> Kevin

Re: [Qemu-devel] [PATCH 24/24] keyval: Support lists

[Eric Blake]

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

On 02/28/2017 01:25 PM, Kevin Wolf wrote:
> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>> Additionally permit non-negative integers as key components.  A
>> dictionary's keys must either be all integers or none.  If all keys
>> are integers, convert the dictionary to a list.  The set of keys must
>> be [0,N].
>>

>> @@ -34,16 +36,16 @@
>>   *   doesn't have one, because R.a must be an object to satisfy a.b=1
>>   *   and a string to satisfy a=2.
>>   *
>> - * Key-fragments must be valid QAPI names.
>> + * Key-fragments must be valid QAPI names or consist only of digits.

>>  /*
>> + * Convert @key to a list index.
>> + * Convert all leading digits to a (non-negative) number, capped at
>> + * INT_MAX.
>> + * If @end is non-null, assign a pointer to the first character after
>> + * the number to *@end.
>> + * Else, fail if any characters follow.
>> + * On success, return the converted number.
>> + * On failure, return a negative value.
>> + * Note: since only digits are converted, no two keys can map to the
>> + * same number, except by overflow to INT_MAX.
>> + */
>> +static int key_to_index(const char *key, const char **end)
>> +{
>> +    int ret;
>> +    unsigned long index;
>> +
>> +    if (*key < '0' || *key > '9') {
>> +        return -EINVAL;
>> +    }

So no leading whitespace, '+', or '-', even if strtoul would have
allowed it.  Such names are also invalid as member id names.  (There's
still the question if we want to allow arbitrary whitespace after
between-key-value ',', and maybe even after between-key-segment '.'
after this series, to make it easier to write strategically line-wrapped
command lines - but that's an independent thing to be done on top).

>> @@ -137,8 +165,13 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>>      cur = qdict;
>>      s = key;
>>      for (;;) {
>> -        ret = parse_qapi_name(s, false);
>> -        len = ret < 0 ? 0 : ret;
>> +        /* Want a key index (unless it's first) or a QAPI name */
>> +        if (s != key && key_to_index(s, &end) >= 0) {
>> +            len = end - s;
>> +        } else {
>> +            ret = parse_qapi_name(s, false);
>> +            len = ret < 0 ? 0 : ret;
>> +        }

Does this mishandle keyval_parse(string, "0", err) - where we want to
assert that the caller always passes only a valid id name for an
implicit key?

>>          assert(s + len <= key_end);
>>          if (!len || (s + len < key_end && s[len] != '.')) {
>>              assert(key != implied_key);
>> @@ -205,6 +238,119 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>>      return s;
>>  }
>>  
>> +static char *reassemble_key(GSList *key)
>> +{
>> +    GString *s = g_string_new("");
>> +    GSList *p;
>> +
>> +    for (p = key; p; p = p->next) {
>> +        g_string_prepend_c(s, '.');
>> +        g_string_prepend(s, (char *)p->data);

Should this use the canonical form of an index, even if the user spelled
it with extra bytes like leading zero?


>> +    /* Copy @cur's values to @elt[] */
>> +    nelt = qdict_size(cur);
>> +    elt = g_new0(QObject *, nelt);
> 
> This doesn't seem to be freed.
> 
>> +    for (ent = qdict_first(cur); ent; ent = qdict_next(cur, ent)) {
>> +        index = key_to_index(ent->key, NULL);
>> +        assert(index >= 0);
>> +        /*
>> +         * We iterate @nelt times.  Because the dictionary keys are
>> +         * distinct, the indexes are also distinct (key_to_index()
>> +         * ensures it).
> 
> Really? What about leading zeros?

key_to_index() should be used to convert "01" and "1" into the same key
when first computing the QDict during the initial parse; this post-pass
should thus only see a single "1" key (last-one-wins semantics,
regardless of the difference in spelling of the same index repeated on
the command line).


-- 
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 24/24] keyval: Support lists

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/28/2017 01:25 PM, Kevin Wolf wrote:
>> Am 27.02.2017 um 12:20 hat Markus Armbruster geschrieben:
>>> Additionally permit non-negative integers as key components.  A
>>> dictionary's keys must either be all integers or none.  If all keys
>>> are integers, convert the dictionary to a list.  The set of keys must
>>> be [0,N].
>>>
>
>>> @@ -34,16 +36,16 @@
>>>   *   doesn't have one, because R.a must be an object to satisfy a.b=1
>>>   *   and a string to satisfy a=2.
>>>   *
>>> - * Key-fragments must be valid QAPI names.
>>> + * Key-fragments must be valid QAPI names or consist only of digits.
>
>>>  /*
>>> + * Convert @key to a list index.
>>> + * Convert all leading digits to a (non-negative) number, capped at
>>> + * INT_MAX.
>>> + * If @end is non-null, assign a pointer to the first character after
>>> + * the number to *@end.
>>> + * Else, fail if any characters follow.
>>> + * On success, return the converted number.
>>> + * On failure, return a negative value.
>>> + * Note: since only digits are converted, no two keys can map to the
>>> + * same number, except by overflow to INT_MAX.
>>> + */
>>> +static int key_to_index(const char *key, const char **end)
>>> +{
>>> +    int ret;
>>> +    unsigned long index;
>>> +
>>> +    if (*key < '0' || *key > '9') {
>>> +        return -EINVAL;
>>> +    }
>
> So no leading whitespace, '+', or '-', even if strtoul would have
> allowed it.  Such names are also invalid as member id names.  (There's
> still the question if we want to allow arbitrary whitespace after
> between-key-value ',', and maybe even after between-key-segment '.'
> after this series, to make it easier to write strategically line-wrapped
> command lines - but that's an independent thing to be done on top).

Yes.

>>> @@ -137,8 +165,13 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>>>      cur = qdict;
>>>      s = key;
>>>      for (;;) {
>>> -        ret = parse_qapi_name(s, false);
>>> -        len = ret < 0 ? 0 : ret;
>>> +        /* Want a key index (unless it's first) or a QAPI name */
>>> +        if (s != key && key_to_index(s, &end) >= 0) {
>>> +            len = end - s;
>>> +        } else {
>>> +            ret = parse_qapi_name(s, false);
>>> +            len = ret < 0 ? 0 : ret;
>>> +        }
>
> Does this mishandle keyval_parse(string, "0", err) - where we want to
> assert that the caller always passes only a valid id name for an
> implicit key?

Crashes and burns...

>>>          assert(s + len <= key_end);
>>>          if (!len || (s + len < key_end && s[len] != '.')) {

... right here (I tried):

>>>              assert(key != implied_key);
>>> @@ -205,6 +238,119 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>>>      return s;
>>>  }
>>>  
>>> +static char *reassemble_key(GSList *key)
>>> +{
>>> +    GString *s = g_string_new("");
>>> +    GSList *p;
>>> +
>>> +    for (p = key; p; p = p->next) {
>>> +        g_string_prepend_c(s, '.');
>>> +        g_string_prepend(s, (char *)p->data);
>
> Should this use the canonical form of an index, even if the user spelled
> it with extra bytes like leading zero?

I feel it's best to echo it back to the user exactly how he wrote it, as
far as practical.

>>> +    /* Copy @cur's values to @elt[] */
>>> +    nelt = qdict_size(cur);
>>> +    elt = g_new0(QObject *, nelt);
>> 
>> This doesn't seem to be freed.
>> 
>>> +    for (ent = qdict_first(cur); ent; ent = qdict_next(cur, ent)) {
>>> +        index = key_to_index(ent->key, NULL);
>>> +        assert(index >= 0);
>>> +        /*
>>> +         * We iterate @nelt times.  Because the dictionary keys are
>>> +         * distinct, the indexes are also distinct (key_to_index()
>>> +         * ensures it).
>> 
>> Really? What about leading zeros?
>
> key_to_index() should be used to convert "01" and "1" into the same key
> when first computing the QDict during the initial parse; this post-pass
> should thus only see a single "1" key (last-one-wins semantics,
> regardless of the difference in spelling of the same index repeated on
> the command line).

Re: [Qemu-devel] [PATCH 00/24] block: Command line option -blockdev

[Eric Blake]

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

On 02/27/2017 05:20 AM, Markus Armbruster wrote:
> Actually, the command line option is the least part of this series.
> Its bulk is about building infrastructure and getting errors out of
> the JSON parser[*].  The latter part could be spun out in its own
> series, if that helps.  We'll see.
> 
> The design of the command line interface was discussed here:
> Subject: Non-flat command line option argument syntax
> Message-ID: <87bmukmlau.fsf@dusky.pond.sub.org>
> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00555.html
> 
> This series is based on "[PATCH v2 00/26] qapi: QMP dispatch and input
> visitor work".

which I've now reviewed; it has a couple issues that need fixing before
I can give R-b everywhere, but we should be able to get to a pull
request soon. (Wow - soft freeze comes faster than we want!)

>  Available as branch blockdev-cmdline in my public repo
> <http://repo.or.cz/w/qemu/armbru.git>, except that one still has a
> patch to add implied value sugar, followed by its revert.
> 

It helped. Thanks for posting 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 --]