[Qemu-devel] [PATCH RFC 0/3] qapi: more introspection tweaks

[Qemu-devel] [PATCH RFC 0/3] qapi: more introspection tweaks

[Eric Blake]

RFC because it depends on:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02580.html
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg01599.html

Eric Blake (3):
  qapi: Improve 'include' error message
  qapi: Sort qapi-schema tests
  qapi-introspect: Guarantee particular sorting

 docs/qapi-code-gen.txt                  |  21 +++-
 qapi/introspect.json                    |  22 ++++-
 scripts/qapi-introspect.py              |   9 +-
 scripts/qapi.py                         |   4 +-
 tests/Makefile                          | 164 +++++++++++++++++++++++---------
 tests/qapi-schema/include-non-file.err  |   2 +-
 tests/qapi-schema/include-non-file.json |   2 +-
 7 files changed, 163 insertions(+), 61 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH RFC 1/3] qapi: Improve 'include' error message

[Eric Blake]

Use of '"...%s" % include' to print non-strings can lead to
ugly messages, such as this (if the .json change is applied
without the qapi.py change):
 Expected a file name (string), got: OrderedDict()

Better is to just omit the actual non-string value in the
message.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi.py                         | 4 ++--
 tests/qapi-schema/include-non-file.err  | 2 +-
 tests/qapi-schema/include-non-file.json | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index a38862d..df06421 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -138,8 +138,8 @@ class QAPISchemaParser(object):
                                         "Invalid 'include' directive")
                 include = expr["include"]
                 if not isinstance(include, str):
-                    raise QAPIExprError(expr_info, 'Expected a file name '
-                                        '(string), got: %s' % include)
+                    raise QAPIExprError(expr_info,
+                                        "Expected a string for 'include'")
                 incl_abs_fname = os.path.join(os.path.dirname(abs_fname),
                                               include)
                 # catch inclusion cycle
diff --git a/tests/qapi-schema/include-non-file.err b/tests/qapi-schema/include-non-file.err
index 9658c78..079120b 100644
--- a/tests/qapi-schema/include-non-file.err
+++ b/tests/qapi-schema/include-non-file.err
@@ -1 +1 @@
-tests/qapi-schema/include-non-file.json:1: Expected a file name (string), got: ['foo', 'bar']
+tests/qapi-schema/include-non-file.json:1: Expected a string for 'include'
diff --git a/tests/qapi-schema/include-non-file.json b/tests/qapi-schema/include-non-file.json
index cd43c3f..4711aa4 100644
--- a/tests/qapi-schema/include-non-file.json
+++ b/tests/qapi-schema/include-non-file.json
@@ -1 +1 @@
-{ 'include': [ 'foo', 'bar' ] }
+{ 'include': {} }
-- 
2.4.3

[Qemu-devel] [PATCH RFC 2/3] qapi: Sort qapi-schema tests

[Eric Blake]

Recent changes to qapi have provided quite a bit of churn in
the makefile, because we are inconsistent on what order test
names appear in, and on whether to re-wrap the list of tests or
just add arbitrary line lengths.  Writing the list in a sorted
fashion, one test per line, will make future patches easier
to see what tests are being added or removed by a patch.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/Makefile | 164 +++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 119 insertions(+), 45 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index ebc6809..6b87679 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -217,51 +217,125 @@ $(foreach target,$(SYSEMU_TARGET_LIST), \
 	$(if $(findstring tests/qom-test$(EXESUF), $(check-qtest-$(target)-y)),, \
 		$(eval check-qtest-$(target)-y += tests/qom-test$(EXESUF))))

-check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
-	comments.json empty.json enum-empty.json enum-missing-data.json \
-	enum-wrong-data.json enum-int-member.json enum-dict-member.json \
-	enum-clash-member.json enum-max-member.json enum-union-clash.json \
-	enum-bad-name.json funny-char.json indented-expr.json \
-	missing-type.json bad-ident.json ident-with-escape.json \
-	escape-outside-string.json unknown-escape.json \
-	escape-too-short.json escape-too-big.json unicode-str.json \
-	double-type.json bad-base.json bad-type-bool.json bad-type-int.json \
-	bad-type-dict.json double-data.json unknown-expr-key.json \
-	redefined-type.json redefined-command.json redefined-builtin.json \
-	redefined-event.json command-int.json bad-data.json event-max.json \
-	type-bypass-bad-gen.json args-invalid.json args-bad-box.json \
-	args-array-empty.json args-array-unknown.json args-int.json \
-	args-unknown.json args-member-unknown.json args-member-array.json \
-	args-member-array-bad.json args-alternate.json args-union.json \
-	args-any.json args-box-anon.json args-box-empty.json \
-	returns-array-bad.json returns-int.json returns-dict.json \
-	returns-unknown.json returns-alternate.json returns-whitelist.json \
-	missing-colon.json missing-comma-list.json missing-comma-object.json \
-	struct-data-invalid.json struct-member-invalid.json \
-	nested-struct-data.json non-objects.json \
-	qapi-schema-test.json quoted-structural-chars.json \
-	leading-comma-list.json leading-comma-object.json \
-	trailing-comma-list.json trailing-comma-object.json \
-	unclosed-list.json unclosed-object.json unclosed-string.json \
-	duplicate-key.json union-invalid-base.json union-bad-branch.json \
-	union-optional-branch.json union-unknown.json union-max.json \
-	union-empty.json flat-union-empty.json alternate-empty.json \
-	flat-union-optional-discriminator.json flat-union-no-base.json \
-	flat-union-invalid-discriminator.json flat-union-inline.json \
-	flat-union-invalid-branch-key.json flat-union-reverse-define.json \
-	flat-union-string-discriminator.json union-base-no-discriminator.json \
-	flat-union-bad-discriminator.json flat-union-bad-base.json \
-	flat-union-base-any.json \
-	flat-union-array-branch.json flat-union-int-branch.json \
-	flat-union-base-union.json flat-union-branch-clash.json \
-	alternate-nested.json alternate-unknown.json alternate-clash.json \
-	alternate-good.json alternate-base.json alternate-array.json \
-	alternate-conflict-string.json alternate-conflict-dict.json \
-	include-simple.json include-relpath.json include-format-err.json \
-	include-non-file.json include-no-file.json include-before-err.json \
-	include-nested-err.json include-self-cycle.json include-cycle.json \
-	include-repetition.json event-nest-struct.json event-case.json \
-	struct-base-clash.json struct-base-clash-deep.json )
+qapi-schema += alternate-array.json
+qapi-schema += alternate-base.json
+qapi-schema += alternate-clash.json
+qapi-schema += alternate-conflict-dict.json
+qapi-schema += alternate-conflict-string.json
+qapi-schema += alternate-empty.json
+qapi-schema += alternate-good.json
+qapi-schema += alternate-nested.json
+qapi-schema += alternate-unknown.json
+qapi-schema += args-alternate.json
+qapi-schema += args-any.json
+qapi-schema += args-array-empty.json
+qapi-schema += args-array-unknown.json
+qapi-schema += args-bad-box.json
+qapi-schema += args-box-anon.json
+qapi-schema += args-box-empty.json
+qapi-schema += args-int.json
+qapi-schema += args-invalid.json
+qapi-schema += args-member-array-bad.json
+qapi-schema += args-member-array.json
+qapi-schema += args-member-unknown.json
+qapi-schema += args-union.json
+qapi-schema += args-unknown.json
+qapi-schema += bad-base.json
+qapi-schema += bad-data.json
+qapi-schema += bad-ident.json
+qapi-schema += bad-type-bool.json
+qapi-schema += bad-type-dict.json
+qapi-schema += bad-type-int.json
+qapi-schema += command-int.json
+qapi-schema += comments.json
+qapi-schema += double-data.json
+qapi-schema += double-type.json
+qapi-schema += duplicate-key.json
+qapi-schema += empty.json
+qapi-schema += enum-bad-name.json
+qapi-schema += enum-clash-member.json
+qapi-schema += enum-dict-member.json
+qapi-schema += enum-empty.json
+qapi-schema += enum-int-member.json
+qapi-schema += enum-max-member.json
+qapi-schema += enum-missing-data.json
+qapi-schema += enum-union-clash.json
+qapi-schema += enum-wrong-data.json
+qapi-schema += escape-outside-string.json
+qapi-schema += escape-too-big.json
+qapi-schema += escape-too-short.json
+qapi-schema += event-case.json
+qapi-schema += event-max.json
+qapi-schema += event-nest-struct.json
+qapi-schema += flat-union-array-branch.json
+qapi-schema += flat-union-bad-base.json
+qapi-schema += flat-union-bad-discriminator.json
+qapi-schema += flat-union-base-any.json
+qapi-schema += flat-union-base-union.json
+qapi-schema += flat-union-branch-clash.json
+qapi-schema += flat-union-empty.json
+qapi-schema += flat-union-inline.json
+qapi-schema += flat-union-int-branch.json
+qapi-schema += flat-union-invalid-branch-key.json
+qapi-schema += flat-union-invalid-discriminator.json
+qapi-schema += flat-union-no-base.json
+qapi-schema += flat-union-optional-discriminator.json
+qapi-schema += flat-union-reverse-define.json
+qapi-schema += flat-union-string-discriminator.json
+qapi-schema += funny-char.json
+qapi-schema += ident-with-escape.json
+qapi-schema += include-before-err.json
+qapi-schema += include-cycle.json
+qapi-schema += include-format-err.json
+qapi-schema += include-nested-err.json
+qapi-schema += include-no-file.json
+qapi-schema += include-non-file.json
+qapi-schema += include-relpath.json
+qapi-schema += include-repetition.json
+qapi-schema += include-self-cycle.json
+qapi-schema += include-simple.json
+qapi-schema += indented-expr.json
+qapi-schema += leading-comma-list.json
+qapi-schema += leading-comma-object.json
+qapi-schema += missing-colon.json
+qapi-schema += missing-comma-list.json
+qapi-schema += missing-comma-object.json
+qapi-schema += missing-type.json
+qapi-schema += nested-struct-data.json
+qapi-schema += non-objects.json
+qapi-schema += qapi-schema-test.json
+qapi-schema += quoted-structural-chars.json
+qapi-schema += redefined-builtin.json
+qapi-schema += redefined-command.json
+qapi-schema += redefined-event.json
+qapi-schema += redefined-type.json
+qapi-schema += returns-alternate.json
+qapi-schema += returns-array-bad.json
+qapi-schema += returns-dict.json
+qapi-schema += returns-int.json
+qapi-schema += returns-unknown.json
+qapi-schema += returns-whitelist.json
+qapi-schema += struct-base-clash-deep.json
+qapi-schema += struct-base-clash.json
+qapi-schema += struct-data-invalid.json
+qapi-schema += struct-member-invalid.json
+qapi-schema += trailing-comma-list.json
+qapi-schema += trailing-comma-object.json
+qapi-schema += type-bypass-bad-gen.json
+qapi-schema += unclosed-list.json
+qapi-schema += unclosed-object.json
+qapi-schema += unclosed-string.json
+qapi-schema += unicode-str.json
+qapi-schema += union-bad-branch.json
+qapi-schema += union-base-no-discriminator.json
+qapi-schema += union-empty.json
+qapi-schema += union-invalid-base.json
+qapi-schema += union-max.json
+qapi-schema += union-optional-branch.json
+qapi-schema += union-unknown.json
+qapi-schema += unknown-escape.json
+qapi-schema += unknown-expr-key.json
+check-qapi-schema-y := $(addprefix tests/qapi-schema/, $(qapi-schema))

 GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h \
 	tests/test-qmp-commands.h tests/test-qapi-event.h \
-- 
2.4.3

[Qemu-devel] [PATCH RFC 3/3] qapi-introspect: Guarantee particular sorting

[Eric Blake]

Sorting the values of an enum makes it easier to look up whether
a particular value is present by binary rather than linear search
(probably most visible with QKeyCode, which has grown over
several releases).  Additionally, QMP clients need not know which
C value is associated with an enum name, so sorting is an
effective way to hide that non-ABI aspect of qapi.

While we are at it, it is also easy to sort the members and
variants of objects, to allow for a similar binary search, and
equally valid since JSON objects have no specific order in which
keys must appear.  There is no trivial or obvious way to sort
the types of an alternate, so that is left unchanged.

However, the overall SchemaInfo array remains unsorted.  It might
make sense to sort with 'meta-type' as a primary key and 'name'
as a secondary key, but it is not obvious that this will provide
benefits to end-user clients (we allow mutually recursive types,
so there is no posible topological sorting where a single pass
over the array could resolve all types, and while binary searches
could be made possible by sorting, it would be even more efficient
for clients to read the array into a hashtable for O(1) rather
than O(log n) random-access lookups, at which point pre-sorting is
wasted effort).

Document these conventions, so that clients will know what can
and cannot be relied on.

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

---
TODO: should the documentation mention that the sorting is done
in the C locale? Is there anything required to ensure that python
sorts sanely (ie. that the choice of locale while building
doesn't cause inadvertent sorting differences such as turning on
case-insensitivity)?
---
 docs/qapi-code-gen.txt     | 21 +++++++++++++++++----
 qapi/introspect.json       | 22 +++++++++++++++++-----
 scripts/qapi-introspect.py |  9 ++++++---
 3 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index a12a7f6..8991fba 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -522,6 +522,13 @@ query-qmp-schema.  QGA currently doesn't support introspection.

 query-qmp-schema returns a JSON array of SchemaInfo objects.  These
 objects together describe the wire ABI, as defined in the QAPI schema.
+There is no specified order to the SchemaInfo objects returned; a
+client must search for a particular name and meta-type throughout the
+entire array to learn more about that name.  For now, the name for
+each SchemaInfo is unique thanks to qapi naming conventions; however
+this may be changed in the future (such as allowing a command and an
+event with the same name), so it is important that the client check
+for the desired meta-type.

 However, the SchemaInfo can't reflect all the rules and restrictions
 that apply to QMP.  It's interface introspection (figuring out what's
@@ -602,7 +609,8 @@ any.  Each element is a JSON object with members "name" (the member's
 name), "type" (the name of its type), and optionally "default".  The
 member is optional if "default" is present.  Currently, "default" can
 only have value null.  Other values are reserved for future
-extensions.
+extensions.  The "members" array is sorted by "name", so that clients
+can use a binary search to see if a particular member is supported.

 Example: the SchemaInfo for MyType from section Struct types

@@ -616,7 +624,9 @@ Example: the SchemaInfo for MyType from section Struct types
 "variants" is a JSON array describing the object's variant members.
 Each element is a JSON object with members "case" (the value of type
 tag this element applies to) and "type" (the name of an object type
-that provides the variant members for this type tag value).
+that provides the variant members for this type tag value).  The
+"variants" array is sorted by "case", so it appears in the same
+order as the enum type matching "tag".

 Example: the SchemaInfo for flat union BlockdevOptions from section
 Union types
@@ -657,7 +667,8 @@ Union types
 The SchemaInfo for an alternate type has meta-type "alternate", and
 variant member "members".  "members" is a JSON array.  Each element is
 a JSON object with member "type", which names a type.  Values of the
-alternate type conform to exactly one of its member types.
+alternate type conform to exactly one of its member types.  There is
+no guarantee on the order in which "members" will be listed.

 Example: the SchemaInfo for BlockRef from section Alternate types

@@ -679,7 +690,9 @@ Example: the SchemaInfo for ['str']
       "element-type": "str" }

 The SchemaInfo for an enumeration type has meta-type "enum" and
-variant member "values".
+variant member "values".  The values are listed in sorted order,
+so clients can use a binary search to see if a particular value
+is present.

 Example: the SchemaInfo for MyEnum from section Enumeration types

diff --git a/qapi/introspect.json b/qapi/introspect.json
index cc50dc6..71632af 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -25,6 +25,11 @@
 # Returns: array of @SchemaInfo, where each element describes an
 # entity in the ABI: command, event, type, ...
 #
+# The order of the various SchemaInfo is unspecified.  For now, the
+# name of each SchemaInfo is unique regardless of meta-type, but to be
+# safe, clients should check that a given name has the expected
+# meta-type.
+#
 # Note: the QAPI schema is also used to help define *internal*
 # interfaces, by defining QAPI types.  These are not part of the QMP
 # wire ABI, and therefore not returned by this command.
@@ -78,7 +83,8 @@
 #        Commands and events have the name defined in the QAPI schema.
 #        Unlike command and event names, type names are not part of
 #        the wire ABI.  Consequently, type names are meaningless
-#        strings here.
+#        strings here.  Although all names are currently unique
+#        regardless of @meta-type, clients should not rely on this.
 #
 # All references to other SchemaInfo are by name.
 #
@@ -130,7 +136,9 @@
 #
 # Additional SchemaInfo members for meta-type 'enum'.
 #
-# @values: the enumeration type's values.
+# @values: the enumeration type's values.  The values are sorted, so
+#          clients can use a binary search to see if a particular value
+#          is present.
 #
 # Values of this type are JSON string on the wire.
 #
@@ -158,14 +166,18 @@
 #
 # Additional SchemaInfo members for meta-type 'object'.
 #
-# @members: the object type's (non-variant) members.
+# @members: the object type's (non-variant) members.  The members are
+#           sorted by name, so clients can use a binary search to see
+#           if a given member is present.
 #
 # @tag: #optional the name of the member serving as type tag.
 #       An element of @members with this name must exist.
 #
 # @variants: #optional variant members, i.e. additional members that
 #            depend on the type tag's value.  Present exactly when
-#            @tag is present.
+#            @tag is present.  The variants are sorted by case, which
+#            means they appear in the same order as the values of the
+#            enum type of the @tag.
 #
 # Values of this type are JSON object on the wire.
 #
@@ -219,7 +231,7 @@
 #
 # Additional SchemaInfo members for meta-type 'alternate'.
 #
-# @members: the alternate type's members.
+# @members: the alternate type's members, in no particular order.
 #           The members' wire encoding is distinct, see
 #           docs/qapi-code-gen.txt section Alternate types.
 #
diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
index 64069ad..37bac3c 100644
--- a/scripts/qapi-introspect.py
+++ b/scripts/qapi-introspect.py
@@ -10,6 +10,7 @@
 # See the COPYING file in the top-level directory.

 from qapi import *
+from operator import attrgetter


 # Caveman's json.dumps() replacement (we're stuck at Python 2.4)
@@ -123,7 +124,8 @@ const char %(c_name)s[] = %(c_string)s;

     def _gen_variants(self, tag_name, variants):
         return {'tag': tag_name,
-                'variants': [self._gen_variant(v) for v in variants]}
+                'variants': [self._gen_variant(v) for v in
+                             sorted(variants, key=attrgetter('name'))]}

     def _gen_variant(self, variant):
         return {'case': variant.name, 'type': self._use_type(variant.type)}
@@ -132,14 +134,15 @@ const char %(c_name)s[] = %(c_string)s;
         self._gen_json(name, 'builtin', {'json-type': json_type})

     def visit_enum_type(self, name, info, values):
-        self._gen_json(name, 'enum', {'values': values})
+        self._gen_json(name, 'enum', {'values': sorted(values)})

     def visit_array_type(self, name, info, element_type):
         element = self._use_type(element_type)
         self._gen_json('[' + element + ']', 'array', {'element-type': element})

     def visit_object_type_flat(self, name, info, members, variants):
-        obj = {'members': [self._gen_member(m) for m in members]}
+        obj = {'members': [self._gen_member(m) for m in
+                           sorted(members, key=attrgetter('name'))]}
         if variants:
             obj.update(self._gen_variants(variants.tag_member.name,
                                           variants.variants))
-- 
2.4.3

Re: [Qemu-devel] [PATCH RFC 0/3] qapi: more introspection tweaks

[Eric Blake]

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

On 09/10/2015 04:25 PM, Eric Blake wrote:
> RFC because it depends on:
> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02580.html
> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg01599.html
> 
> Eric Blake (3):
>   qapi: Improve 'include' error message
>   qapi: Sort qapi-schema tests
>   qapi-introspect: Guarantee particular sorting

If it helps review, I just created a tracking repo where I'll keep my
pending work:
http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi
git fetch git://repo.or.cz/qemu/ericb.git qapi

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


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

Re: [Qemu-devel] [PATCH RFC 0/3] qapi: more introspection tweaks

[Marc-André Lureau]

Hi

On Fri, Sep 11, 2015 at 12:59 AM, Eric Blake <eblake@redhat.com> wrote:
> http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi
> git fetch git://repo.or.cz/qemu/ericb.git qapi


My pending qapi patches are no rebased on your branch
(https://github.com/elmarco/qemu qapi branch). I dropped the varargs
patch, as it was now only for device_add, and monitor can just
register the command manually instead.

-- 
Marc-André Lureau