[Qemu-devel] [PATCH for-2.9 0/5] keyval: Doc improvements and additional tests

[Qemu-devel] [PATCH for-2.9 0/5] keyval: Doc improvements and additional tests

[Markus Armbruster]

These are basically leftovers from review of "block: Command line
option -blockdev".  Non-test code is unchanged.  Nice to have in 2.9.

Markus Armbruster (5):
  test-keyval: Tweaks to improve list coverage
  keyval: Improve some comments
  test-keyval: Cover alternate and 'any' type
  keyval: Document issues with 'any' and alternate types
  MAINTAINERS: Add myself for files I touched recently

 MAINTAINERS            | 11 ++++++++++
 tests/Makefile.include |  2 +-
 tests/test-keyval.c    | 59 +++++++++++++++++++++++++++++++++++++++++++++++---
 util/keyval.c          | 57 ++++++++++++++++++++++++++++++++++--------------
 4 files changed, 109 insertions(+), 20 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH for-2.9 1/5] test-keyval: Tweaks to improve list coverage

[Markus Armbruster]

We have a negative test case for a list index with leading zero.  Add
positive ones.

Tweak the test case for list index greater or equal the number of
elements: test "equal" instead of "greater" to guard against
off-by-one mistakes.

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

diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index 71288b0..e97f6d5 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -218,14 +218,14 @@ static void test_keyval_parse_list(void)
     QDECREF(qdict);
 
     /* Multiple indexes, last one wins */
-    qdict = keyval_parse("list.1=goner,list.0=null,list.1=eins,list.2=zwei",
+    qdict = keyval_parse("list.1=goner,list.0=null,list.01=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",
+    qdict = keyval_parse("a.list.1=eins,a.list.00=null,a.list.2=zwei",
                          NULL, &error_abort);
     g_assert_cmpint(qdict_size(qdict), ==, 1);
     sub_qdict = qdict_get_qdict(qdict, "a");
@@ -242,7 +242,7 @@ static void test_keyval_parse_list(void)
     g_assert(!qdict);
 
     /* Missing list indexes */
-    qdict = keyval_parse("list.2=lonely", NULL, &err);
+    qdict = keyval_parse("list.1=lonely", NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
     qdict = keyval_parse("list.0=null,list.2=eins,list.02=zwei", NULL, &err);
-- 
2.7.4

[Qemu-devel] [PATCH for-2.9 2/5] keyval: Improve some comments

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/keyval.c | 47 +++++++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/util/keyval.c b/util/keyval.c
index f646b36..46cd540 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -21,22 +21,36 @@
  *
  * Semantics defined by reduction to JSON:
  *
- *   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
- *                 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.
+ *   key-vals specifies a JSON object, i.e. a tree whose root is an
+ *   object, inner nodes other than the root are objects or arrays,
+ *   and leaves are strings.
  *
- *   Duplicate keys are permitted; all but the last one are ignored.
+ *   Each key-val = key-fragment '.' ... '=' val specifies a path from
+ *   root to a leaf (left of '='), and the leaf's value (right of
+ *   '=').
  *
- *   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.
+ *   A path from the root is defined recursively:
+ *       L '.' key-fragment is a child of the node denoted by path L
+ *       key-fragment is a child of the tree root
+ *   If key-fragment is numeric, the parent is an array and the child
+ *   is its key-fragment-th member, counting from zero.
+ *   Else, the parent is an object, and the child is its member named
+ *   key-fragment.
  *
- * Key-fragments must be valid QAPI names or consist only of digits.
+ *   This constrains inner nodes to be either array or object.  The
+ *   constraints must be satisfiable.  Counter-example: a.b=1,a=2 is
+ *   not, because root.a must be an object to satisfy a.b=1 and a
+ *   string to satisfy a=2.
+ *
+ *   Array subscripts can occur in any order, but the set of
+ *   subscripts must not have gaps.  For instance, a.1=v is not okay,
+ *   because root.a[0] is missing.
+ *
+ *   If multiple key-val denote the same leaf, the last one determines
+ *   the value.
+ *
+ * Key-fragments must be valid QAPI names or consist only of decimal
+ * digits.
  *
  * The length of any key-fragment must be between 1 and 127.
  *
@@ -64,8 +78,8 @@
 
 /*
  * Convert @key to a list index.
- * Convert all leading digits to a (non-negative) number, capped at
- * INT_MAX.
+ * Convert all leading decimal 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.
@@ -337,7 +351,8 @@ static QObject *keyval_listify(QDict *cur, GSList *key_of_cur, Error **errp)
     }
 
     /*
-     * Make a list from @elt[], reporting any missing elements.
+     * Make a list from @elt[], reporting the first missing element,
+     * if any.
      * If we dropped an index >= nelt in the previous loop, this loop
      * will run into the sentinel and report index @nelt missing.
      */
-- 
2.7.4

[Qemu-devel] [PATCH for-2.9 3/5] test-keyval: Cover alternate and 'any' type

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/Makefile.include |  2 +-
 tests/test-keyval.c    | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 402e71c..86f9490 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -736,7 +736,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-keyval$(EXESUF): tests/test-keyval.o $(test-util-obj-y) $(test-qapi-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
index e97f6d5..ba19560 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -14,6 +14,7 @@
 #include "qapi/error.h"
 #include "qapi/qmp/qstring.h"
 #include "qapi/qobject-input-visitor.h"
+#include "test-qapi-visit.h"
 #include "qemu/cutils.h"
 #include "qemu/option.h"
 
@@ -608,6 +609,56 @@ static void test_keyval_visit_optional(void)
     visit_free(v);
 }
 
+static void test_keyval_visit_alternate(void)
+{
+    Error *err = NULL;
+    Visitor *v;
+    QDict *qdict;
+    AltNumStr *ans;
+    AltNumInt *ani;
+
+    /*
+     * Can't do scalar alternate variants other than string.  You get
+     * the string variant if there is one, else an error.
+     */
+    qdict = keyval_parse("a=1,b=2", NULL, &error_abort);
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    QDECREF(qdict);
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+    visit_type_AltNumStr(v, "a", &ans, &error_abort);
+    g_assert_cmpint(ans->type, ==, QTYPE_QSTRING);
+    g_assert_cmpstr(ans->u.s, ==, "1");
+    visit_type_AltNumInt(v, "a", &ani, &err);
+    error_free_or_abort(&err);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+}
+
+static void test_keyval_visit_any(void)
+{
+    Visitor *v;
+    QDict *qdict;
+    QObject *any;
+    QList *qlist;
+    QString *qstr;
+
+    qdict = keyval_parse("a.0=null,a.1=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_any(v, "a", &any, &error_abort);
+    qlist = qobject_to_qlist(any);
+    g_assert(qlist);
+    qstr = qobject_to_qstring(qlist_pop(qlist));
+    g_assert_cmpstr(qstring_get_str(qstr), ==, "null");
+    qstr = qobject_to_qstring(qlist_pop(qlist));
+    g_assert_cmpstr(qstring_get_str(qstr), ==, "1");
+    g_assert(qlist_empty(qlist));
+    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);
@@ -619,6 +670,8 @@ int main(int argc, char *argv[])
     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_add_func("/keyval/visit/alternate", test_keyval_visit_alternate);
+    g_test_add_func("/keyval/visit/any", test_keyval_visit_any);
     g_test_run();
     return 0;
 }
-- 
2.7.4

[Qemu-devel] [PATCH for-2.9 4/5] keyval: Document issues with 'any' and alternate types

[Markus Armbruster]

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

diff --git a/util/keyval.c b/util/keyval.c
index 46cd540..93d5db6 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -61,6 +61,16 @@
  * "key absent" already means "optional object/array absent", which
  * isn't the same as "empty object/array present".
  *
+ * Design flaw: scalar values can only be strings; there is no way to
+ * denote numbers, true, false or null.  The special QObject input
+ * visitor returned by qobject_input_visitor_new_keyval() mostly hides
+ * this by automatically converting strings to the type the visitor
+ * expects.  Breaks down for alternate types and type 'any', where the
+ * visitor's expectation isn't clear.  Code visiting such types needs
+ * to do the conversion itself, but only when using this keyval
+ * visitor.  Awkward.  Alternate types without a string member don't
+ * work at all.
+ *
  * Additional syntax for use with an implied key:
  *
  *   key-vals-ik  = val-no-key [ ',' key-vals ]
-- 
2.7.4

[Qemu-devel] [PATCH for-2.9 5/5] MAINTAINERS: Add myself for files I touched recently

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 MAINTAINERS | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index bf1aafb..3f20288 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1218,6 +1218,15 @@ M: Samuel Thibault <samuel.thibault@ens-lyon.org>
 S: Maintained
 F: backends/baum.c
 
+Command line option argument parsing
+M: Markus Armbruster <armbru@redhat.com>
+S: Supported
+F: include/qemu/option.h
+F: tests/test-keyval.c
+F: tests/test-qemu-opts.c
+F: util/keyval.c
+F: util/qemu-option.c
+
 Coverity model
 M: Markus Armbruster <armbru@redhat.com>
 S: Supported
@@ -1352,7 +1361,9 @@ X: include/qapi/qmp/
 F: include/qapi/qmp/dispatch.h
 F: tests/qapi-schema/
 F: tests/test-*-visitor.c
+F: tests/test-qapi-*.c
 F: tests/test-qmp-*.c
+F: tests/test-visitor-serialization.c
 F: scripts/qapi*
 F: docs/qapi*
 T: git git://repo.or.cz/qemu/armbru.git qapi-next
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.9 1/5] test-keyval: Tweaks to improve list coverage

[Eric Blake]

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

On 03/20/2017 07:55 AM, Markus Armbruster wrote:
> We have a negative test case for a list index with leading zero.  Add
> positive ones.
> 
> Tweak the test case for list index greater or equal the number of
> elements: test "equal" instead of "greater" to guard against
> off-by-one mistakes.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/test-keyval.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 

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 for-2.9 2/5] keyval: Improve some comments

[Eric Blake]

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

On 03/20/2017 07:55 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  util/keyval.c | 47 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 31 insertions(+), 16 deletions(-)
> 

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 for-2.9 3/5] test-keyval: Cover alternate and 'any' type

[Eric Blake]

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

On 03/20/2017 07:55 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/Makefile.include |  2 +-
>  tests/test-keyval.c    | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+), 1 deletion(-)
> 

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 for-2.9 4/5] keyval: Document issues with 'any' and alternate types

[Eric Blake]

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

On 03/20/2017 07:55 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  util/keyval.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/util/keyval.c b/util/keyval.c
> index 46cd540..93d5db6 100644
> --- a/util/keyval.c
> +++ b/util/keyval.c
> @@ -61,6 +61,16 @@
>   * "key absent" already means "optional object/array absent", which
>   * isn't the same as "empty object/array present".
>   *
> + * Design flaw: scalar values can only be strings; there is no way to
> + * denote numbers, true, false or null.  The special QObject input
> + * visitor returned by qobject_input_visitor_new_keyval() mostly hides
> + * this by automatically converting strings to the type the visitor
> + * expects.  Breaks down for alternate types and type 'any', where the
> + * visitor's expectation isn't clear.  Code visiting such types needs
> + * to do the conversion itself, but only when using this keyval
> + * visitor.  Awkward.  Alternate types without a string member don't
> + * work at all.

We've toyed with the idea of making socket parsing accept an alternate
between string and integer (right now it's string due to named port
support, even though most users end up accessing an integer causing a
lot of in-tree parsing/formatting between integers and strings) - that
will be one case that is particularly impacted by this design flaw, if
we ever go through with that idea.  But enough speculation about the
future - your patch as written is accurate for the present.

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 for-2.9 5/5] MAINTAINERS: Add myself for files I touched recently

[Eric Blake]

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

On 03/20/2017 07:55 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  MAINTAINERS | 11 +++++++++++
>  1 file changed, 11 insertions(+)

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

By the way, where do we stand on the idea of having checkpatch.pl reject
patches that introduce new files without mentioning a maintainer?

-- 
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 for-2.9 4/5] keyval: Document issues with 'any' and alternate types

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/20/2017 07:55 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  util/keyval.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>> 
>> diff --git a/util/keyval.c b/util/keyval.c
>> index 46cd540..93d5db6 100644
>> --- a/util/keyval.c
>> +++ b/util/keyval.c
>> @@ -61,6 +61,16 @@
>>   * "key absent" already means "optional object/array absent", which
>>   * isn't the same as "empty object/array present".
>>   *
>> + * Design flaw: scalar values can only be strings; there is no way to
>> + * denote numbers, true, false or null.  The special QObject input
>> + * visitor returned by qobject_input_visitor_new_keyval() mostly hides
>> + * this by automatically converting strings to the type the visitor
>> + * expects.  Breaks down for alternate types and type 'any', where the
>> + * visitor's expectation isn't clear.  Code visiting such types needs
>> + * to do the conversion itself, but only when using this keyval
>> + * visitor.  Awkward.  Alternate types without a string member don't
>> + * work at all.
>
> We've toyed with the idea of making socket parsing accept an alternate
> between string and integer (right now it's string due to named port
> support, even though most users end up accessing an integer causing a
> lot of in-tree parsing/formatting between integers and strings) - that
> will be one case that is particularly impacted by this design flaw, if
> we ever go through with that idea.

I'll reply to this in "Re: Non-flat command line option argument
syntax".

>                                     But enough speculation about the
> future - your patch as written is accurate for the present.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH for-2.9 5/5] MAINTAINERS: Add myself for files I touched recently

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/20/2017 07:55 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  MAINTAINERS | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> By the way, where do we stand on the idea of having checkpatch.pl reject
> patches that introduce new files without mentioning a maintainer?

Stuck.  Thomas hasn't followed up on his RFC PATCH because he's afraid
of false positives.  I encouraged him to rescue at least "[RFC PATCH
4/5] checkpatch: emit a reminder about MAINTAINERS on file
add/move/delete", and I'm now encouraging him again.

Re: [Qemu-devel] [PATCH for-2.9 5/5] MAINTAINERS: Add myself for files I touched recently

[Thomas Huth]

On 21.03.2017 08:17, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 03/20/2017 07:55 AM, Markus Armbruster wrote:
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  MAINTAINERS | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> By the way, where do we stand on the idea of having checkpatch.pl reject
>> patches that introduce new files without mentioning a maintainer?
> 
> Stuck.  Thomas hasn't followed up on his RFC PATCH because he's afraid
> of false positives.  I encouraged him to rescue at least "[RFC PATCH
> 4/5] checkpatch: emit a reminder about MAINTAINERS on file
> add/move/delete", and I'm now encouraging him again.

Well, the patch series is out there, and there haven't been any (valid)
requests to rework it, so if you like, feel free to merge it:

https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05740.html

But as mentioned here:

 https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05753.html

there will be quite a bunch of false-positives. So we'd either have to
live with those or we have to come up with a smarter approach to handle
this issue (e.g. by running get_maintainers.pl on the affected files).

 Thomas