* [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature
@ 2019-04-08 14:35 Kevin Wolf
2019-04-08 14:35 ` Kevin Wolf
` (7 more replies)
0 siblings, 8 replies; 25+ messages in thread
From: Kevin Wolf @ 2019-04-08 14:35 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, armbru, eblake, pkrempa, qemu-devel
This series adds optional feature lists to struct definitions in the
QAPI schema and makes use of them to advertise the new behaviour of
auto-read-only=on in file-posix.
Kevin Wolf (4):
qapi: Support features for structs
tests/qapi-schema: Test for good feature lists in structs
tests/qapi-schema: Error case tests for features in structs
file-posix: Add dynamic-auto-read-only QAPI feature
qapi/block-core.json | 11 ++++-
qapi/introspect.json | 8 ++-
tests/qapi-schema/features-bad-type.json | 3 ++
tests/qapi-schema/features-missing-name.json | 3 ++
tests/qapi-schema/features-name-bad-type.json | 3 ++
tests/qapi-schema/features-no-list.json | 3 ++
tests/qapi-schema/features-unknown-key.json | 3 ++
tests/qapi-schema/qapi-schema-test.json | 27 ++++++++++
docs/devel/qapi-code-gen.txt | 38 ++++++++++++++
scripts/qapi/common.py | 49 +++++++++++++++----
scripts/qapi/doc.py | 3 +-
scripts/qapi/introspect.py | 6 ++-
scripts/qapi/types.py | 3 +-
scripts/qapi/visit.py | 3 +-
tests/Makefile.include | 5 ++
tests/qapi-schema/double-type.err | 2 +-
tests/qapi-schema/features-bad-type.err | 1 +
tests/qapi-schema/features-bad-type.exit | 1 +
tests/qapi-schema/features-bad-type.out | 0
tests/qapi-schema/features-missing-name.err | 1 +
tests/qapi-schema/features-missing-name.exit | 1 +
tests/qapi-schema/features-missing-name.out | 0
tests/qapi-schema/features-name-bad-type.err | 1 +
tests/qapi-schema/features-name-bad-type.exit | 1 +
tests/qapi-schema/features-name-bad-type.out | 0
tests/qapi-schema/features-no-list.err | 1 +
tests/qapi-schema/features-no-list.exit | 1 +
tests/qapi-schema/features-no-list.out | 0
tests/qapi-schema/features-unknown-key.err | 2 +
tests/qapi-schema/features-unknown-key.exit | 1 +
tests/qapi-schema/features-unknown-key.out | 0
tests/qapi-schema/qapi-schema-test.out | 26 ++++++++++
tests/qapi-schema/test-qapi.py | 7 ++-
tests/qapi-schema/unknown-expr-key.err | 2 +-
34 files changed, 197 insertions(+), 19 deletions(-)
create mode 100644 tests/qapi-schema/features-bad-type.json
create mode 100644 tests/qapi-schema/features-missing-name.json
create mode 100644 tests/qapi-schema/features-name-bad-type.json
create mode 100644 tests/qapi-schema/features-no-list.json
create mode 100644 tests/qapi-schema/features-unknown-key.json
create mode 100644 tests/qapi-schema/features-bad-type.err
create mode 100644 tests/qapi-schema/features-bad-type.exit
create mode 100644 tests/qapi-schema/features-bad-type.out
create mode 100644 tests/qapi-schema/features-missing-name.err
create mode 100644 tests/qapi-schema/features-missing-name.exit
create mode 100644 tests/qapi-schema/features-missing-name.out
create mode 100644 tests/qapi-schema/features-name-bad-type.err
create mode 100644 tests/qapi-schema/features-name-bad-type.exit
create mode 100644 tests/qapi-schema/features-name-bad-type.out
create mode 100644 tests/qapi-schema/features-no-list.err
create mode 100644 tests/qapi-schema/features-no-list.exit
create mode 100644 tests/qapi-schema/features-no-list.out
create mode 100644 tests/qapi-schema/features-unknown-key.err
create mode 100644 tests/qapi-schema/features-unknown-key.exit
create mode 100644 tests/qapi-schema/features-unknown-key.out
--
2.20.1
^ permalink raw reply [flat|nested] 25+ messages in thread* [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature 2019-04-08 14:35 [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature Kevin Wolf @ 2019-04-08 14:35 ` Kevin Wolf 2019-04-08 14:35 ` [Qemu-devel] [PATCH 1/4] qapi: Support features for structs Kevin Wolf ` (6 subsequent siblings) 7 siblings, 0 replies; 25+ messages in thread From: Kevin Wolf @ 2019-04-08 14:35 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, pkrempa, armbru, qemu-devel This series adds optional feature lists to struct definitions in the QAPI schema and makes use of them to advertise the new behaviour of auto-read-only=on in file-posix. Kevin Wolf (4): qapi: Support features for structs tests/qapi-schema: Test for good feature lists in structs tests/qapi-schema: Error case tests for features in structs file-posix: Add dynamic-auto-read-only QAPI feature qapi/block-core.json | 11 ++++- qapi/introspect.json | 8 ++- tests/qapi-schema/features-bad-type.json | 3 ++ tests/qapi-schema/features-missing-name.json | 3 ++ tests/qapi-schema/features-name-bad-type.json | 3 ++ tests/qapi-schema/features-no-list.json | 3 ++ tests/qapi-schema/features-unknown-key.json | 3 ++ tests/qapi-schema/qapi-schema-test.json | 27 ++++++++++ docs/devel/qapi-code-gen.txt | 38 ++++++++++++++ scripts/qapi/common.py | 49 +++++++++++++++---- scripts/qapi/doc.py | 3 +- scripts/qapi/introspect.py | 6 ++- scripts/qapi/types.py | 3 +- scripts/qapi/visit.py | 3 +- tests/Makefile.include | 5 ++ tests/qapi-schema/double-type.err | 2 +- tests/qapi-schema/features-bad-type.err | 1 + tests/qapi-schema/features-bad-type.exit | 1 + tests/qapi-schema/features-bad-type.out | 0 tests/qapi-schema/features-missing-name.err | 1 + tests/qapi-schema/features-missing-name.exit | 1 + tests/qapi-schema/features-missing-name.out | 0 tests/qapi-schema/features-name-bad-type.err | 1 + tests/qapi-schema/features-name-bad-type.exit | 1 + tests/qapi-schema/features-name-bad-type.out | 0 tests/qapi-schema/features-no-list.err | 1 + tests/qapi-schema/features-no-list.exit | 1 + tests/qapi-schema/features-no-list.out | 0 tests/qapi-schema/features-unknown-key.err | 2 + tests/qapi-schema/features-unknown-key.exit | 1 + tests/qapi-schema/features-unknown-key.out | 0 tests/qapi-schema/qapi-schema-test.out | 26 ++++++++++ tests/qapi-schema/test-qapi.py | 7 ++- tests/qapi-schema/unknown-expr-key.err | 2 +- 34 files changed, 197 insertions(+), 19 deletions(-) create mode 100644 tests/qapi-schema/features-bad-type.json create mode 100644 tests/qapi-schema/features-missing-name.json create mode 100644 tests/qapi-schema/features-name-bad-type.json create mode 100644 tests/qapi-schema/features-no-list.json create mode 100644 tests/qapi-schema/features-unknown-key.json create mode 100644 tests/qapi-schema/features-bad-type.err create mode 100644 tests/qapi-schema/features-bad-type.exit create mode 100644 tests/qapi-schema/features-bad-type.out create mode 100644 tests/qapi-schema/features-missing-name.err create mode 100644 tests/qapi-schema/features-missing-name.exit create mode 100644 tests/qapi-schema/features-missing-name.out create mode 100644 tests/qapi-schema/features-name-bad-type.err create mode 100644 tests/qapi-schema/features-name-bad-type.exit create mode 100644 tests/qapi-schema/features-name-bad-type.out create mode 100644 tests/qapi-schema/features-no-list.err create mode 100644 tests/qapi-schema/features-no-list.exit create mode 100644 tests/qapi-schema/features-no-list.out create mode 100644 tests/qapi-schema/features-unknown-key.err create mode 100644 tests/qapi-schema/features-unknown-key.exit create mode 100644 tests/qapi-schema/features-unknown-key.out -- 2.20.1 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 1/4] qapi: Support features for structs 2019-04-08 14:35 [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature Kevin Wolf 2019-04-08 14:35 ` Kevin Wolf @ 2019-04-08 14:35 ` Kevin Wolf 2019-04-08 14:35 ` Kevin Wolf 2019-04-18 20:03 ` Markus Armbruster 2019-04-08 14:35 ` [Qemu-devel] [PATCH 2/4] tests/qapi-schema: Test for good feature lists in structs Kevin Wolf ` (5 subsequent siblings) 7 siblings, 2 replies; 25+ messages in thread From: Kevin Wolf @ 2019-04-08 14:35 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, armbru, eblake, pkrempa, qemu-devel Sometimes, the behaviour of QEMU changes compatibly, but without a change in the QMP syntax (usually by allowing values or operations that previously resulted in an error). QMP clients may still need to know whether the extension is available. This allows to add a list of features to struct definitions that will be made visible to QMP clients through schema introspection. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/introspect.json | 8 ++++- docs/devel/qapi-code-gen.txt | 38 ++++++++++++++++++++ scripts/qapi/common.py | 49 ++++++++++++++++++++------ scripts/qapi/doc.py | 3 +- scripts/qapi/introspect.py | 6 +++- scripts/qapi/types.py | 3 +- scripts/qapi/visit.py | 3 +- tests/qapi-schema/double-type.err | 2 +- tests/qapi-schema/test-qapi.py | 3 +- tests/qapi-schema/unknown-expr-key.err | 2 +- 10 files changed, 99 insertions(+), 18 deletions(-) diff --git a/qapi/introspect.json b/qapi/introspect.json index 3d22166b2b..3cb6c1aca4 100644 --- a/qapi/introspect.json +++ b/qapi/introspect.json @@ -174,6 +174,11 @@ # and may even differ from the order of the values of the # enum type of the @tag. # +# @features: names of features that are supported by this version and build and +# aren't othervise visible through schema introspection (e.g. change +# of behaviour related to an object type that didn't come with a +# syntactic change in the schema of the object type) (since: 4.1) +# # Values of this type are JSON object on the wire. # # Since: 2.5 @@ -181,7 +186,8 @@ { 'struct': 'SchemaInfoObject', 'data': { 'members': [ 'SchemaInfoObjectMember' ], '*tag': 'str', - '*variants': [ 'SchemaInfoObjectVariant' ] } } + '*variants': [ 'SchemaInfoObjectVariant' ], + '*features': [ 'str' ] } } ## # @SchemaInfoObjectMember: diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt index b517b0cfbf..e8ec8ac1de 100644 --- a/docs/devel/qapi-code-gen.txt +++ b/docs/devel/qapi-code-gen.txt @@ -719,6 +719,34 @@ any non-empty complex type (struct, union, or alternate), and a pointer to that QAPI type is passed as a single argument. +=== Features === + +Sometimes, the behaviour of QEMU changes compatibly, but without a +change in the QMP syntax (usually by allowing values or operations that +previously resulted in an error). QMP clients may still need to know +whether the extension is available. + +For this purpose, a list of features can be specified for a struct type. +This is exposed to the client as a list of string, where each string +signals that this build of QEMU shows a certain behaviour. + +In the schema, features can be specified as simple strings, for example: + +{ 'struct': 'TestType', + 'data': { 'number': 'int' }, + 'features': [ 'allow-negative-numbers' ] } + +Another option is to specify features as dictionaries, where the key +'name' specifies the feature string to be exposed to clients: + +{ 'struct': 'TestType', + 'data': { 'number': 'int' }, + 'features': [ { 'name': 'allow-negative-numbers' } ] } + +This expanded form is necessary if you want to make the feature +conditional (see below in "Configuring the schema"). + + === Downstream extensions === QAPI schema names that are externally visible, say in the Client JSON @@ -771,6 +799,16 @@ Example: a conditional 'bar' enum member. [ 'foo', { 'name' : 'bar', 'if': 'defined(IFCOND)' } ] } +Similarly, features can be specified as a dictionary with a 'name' and +an 'if' key. + +Example: a conditional 'allow-negative-numbers' feature + +{ 'struct': 'TestType', + 'data': { 'number': 'int' }, + 'features': [ { 'name': 'allow-negative-numbers', + 'if' 'defined(IFCOND)' } ] } + Please note that you are responsible to ensure that the C code will compile with an arbitrary combination of conditions, since the generators are unable to check it at this point. diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index f07869ec73..692d787f04 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -886,12 +886,29 @@ def check_enum(expr, info): def check_struct(expr, info): name = expr['struct'] members = expr['data'] + features = expr.get('features') check_type(info, "'data' for struct '%s'" % name, members, allow_dict=True, allow_optional=True) check_type(info, "'base' for struct '%s'" % name, expr.get('base'), allow_metas=['struct']) + if features: + if not isinstance(features, list): + raise QAPISemError(info, + "Struct '%s' requires an array for 'features'" % + name) + for f in features: + assert isinstance(f, dict) + check_known_keys(info, "feature of struct %s" % (name), f, + ['name'], ['if']) + + check_if(f, info) + f['if'] = listify_cond(f.get('if')) + + if not isinstance(f['name'], str): + raise QAPISemError(info, "Feature names for struct '%s' must " + "be strings" % name) def check_known_keys(info, source, keys, required, optional): @@ -947,6 +964,10 @@ def normalize_members(members): continue members[key] = {'type': arg} +def normalize_features(features): + if isinstance(features, list): + features[:] = [f if isinstance(f, dict) else {'name': f} + for f in features] def check_exprs(exprs): global all_names @@ -986,8 +1007,10 @@ def check_exprs(exprs): normalize_members(expr['data']) elif 'struct' in expr: meta = 'struct' - check_keys(expr_elem, 'struct', ['data'], ['base', 'if']) + check_keys(expr_elem, 'struct', ['data'], + ['base', 'if', 'features']) normalize_members(expr['data']) + normalize_features(expr.get('features')) struct_types[expr[meta]] = expr elif 'command' in expr: meta = 'command' @@ -1126,10 +1149,12 @@ class QAPISchemaVisitor(object): def visit_array_type(self, name, info, ifcond, element_type): pass - def visit_object_type(self, name, info, ifcond, base, members, variants): + def visit_object_type(self, name, info, ifcond, base, members, variants, + features): pass - def visit_object_type_flat(self, name, info, ifcond, members, variants): + def visit_object_type_flat(self, name, info, ifcond, members, variants, + features): pass def visit_alternate_type(self, name, info, ifcond, variants): @@ -1290,7 +1315,7 @@ class QAPISchemaArrayType(QAPISchemaType): class QAPISchemaObjectType(QAPISchemaType): def __init__(self, name, info, doc, ifcond, - base, local_members, variants): + base, local_members, variants, features): # struct has local_members, optional base, and no variants # flat union has base, variants, and no local_members # simple union has local_members, variants, and no base @@ -1307,6 +1332,7 @@ class QAPISchemaObjectType(QAPISchemaType): self.local_members = local_members self.variants = variants self.members = None + self.features = features def check(self, schema): QAPISchemaType.check(self, schema) @@ -1368,9 +1394,11 @@ class QAPISchemaObjectType(QAPISchemaType): def visit(self, visitor): visitor.visit_object_type(self.name, self.info, self.ifcond, - self.base, self.local_members, self.variants) + self.base, self.local_members, self.variants, + self.features) visitor.visit_object_type_flat(self.name, self.info, self.ifcond, - self.members, self.variants) + self.members, self.variants, + self.features) class QAPISchemaMember(object): @@ -1675,7 +1703,7 @@ class QAPISchema(object): ('null', 'null', 'QNull' + pointer_suffix)]: self._def_builtin_type(*t) self.the_empty_object_type = QAPISchemaObjectType( - 'q_empty', None, None, None, None, [], None) + 'q_empty', None, None, None, None, [], None, []) self._def_entity(self.the_empty_object_type) qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', @@ -1721,7 +1749,7 @@ class QAPISchema(object): assert ifcond == typ._ifcond # pylint: disable=protected-access else: self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, - None, members, None)) + None, members, None, [])) return name def _def_enum_type(self, expr, info, doc): @@ -1752,9 +1780,10 @@ class QAPISchema(object): base = expr.get('base') data = expr['data'] ifcond = expr.get('if') + features = expr.get('features') self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, base, self._make_members(data, info), - None)) + None, features)) def _make_variant(self, case, typ, ifcond): return QAPISchemaObjectTypeVariant(case, typ, ifcond) @@ -1795,7 +1824,7 @@ class QAPISchema(object): QAPISchemaObjectType(name, info, doc, ifcond, base, members, QAPISchemaObjectTypeVariants(tag_name, tag_member, - variants))) + variants), [])) def _def_alternate_type(self, expr, info, doc): name = expr['alternate'] diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py index 5c8c136899..433e9fcbfb 100755 --- a/scripts/qapi/doc.py +++ b/scripts/qapi/doc.py @@ -220,7 +220,8 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor): body=texi_entity(doc, 'Values', ifcond, member_func=texi_enum_value))) - def visit_object_type(self, name, info, ifcond, base, members, variants): + def visit_object_type(self, name, info, ifcond, base, members, variants, + features): doc = self.cur_doc if base and base.is_implicit(): base = None diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index f7f2ca07e4..8909cecde4 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -188,11 +188,15 @@ const QLitObject %(c_name)s = %(c_string)s; self._gen_qlit('[' + element + ']', 'array', {'element-type': element}, ifcond) - def visit_object_type_flat(self, name, info, ifcond, members, variants): + def visit_object_type_flat(self, name, info, ifcond, members, variants, + features): obj = {'members': [self._gen_member(m) for m in members]} if variants: obj.update(self._gen_variants(variants.tag_member.name, variants.variants)) + if features: + obj['features'] = [ (f['name'], f) for f in features ] + self._gen_qlit(name, 'object', obj, ifcond) def visit_alternate_type(self, name, info, ifcond, variants): diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index 2bd6fcd44f..3edd9374aa 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -227,7 +227,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor): self._genh.add(gen_array(name, element_type)) self._gen_type_cleanup(name) - def visit_object_type(self, name, info, ifcond, base, members, variants): + def visit_object_type(self, name, info, ifcond, base, members, variants, + features): # Nothing to do for the special empty builtin if name == 'q_empty': return diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py index 826b8066e1..c1cd675c95 100644 --- a/scripts/qapi/visit.py +++ b/scripts/qapi/visit.py @@ -326,7 +326,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor): self._genh.add(gen_visit_decl(name)) self._genc.add(gen_visit_list(name, element_type)) - def visit_object_type(self, name, info, ifcond, base, members, variants): + def visit_object_type(self, name, info, ifcond, base, members, variants, + features): # Nothing to do for the special empty builtin if name == 'q_empty': return diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err index 799193dba1..69457173a7 100644 --- a/tests/qapi-schema/double-type.err +++ b/tests/qapi-schema/double-type.err @@ -1,2 +1,2 @@ tests/qapi-schema/double-type.json:2: Unknown key 'command' in struct 'bar' -Valid keys are 'base', 'data', 'if', 'struct'. +Valid keys are 'base', 'data', 'features', 'if', 'struct'. diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py index d21fca01fc..f2d6815c86 100644 --- a/tests/qapi-schema/test-qapi.py +++ b/tests/qapi-schema/test-qapi.py @@ -38,7 +38,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): print('array %s %s' % (name, element_type.name)) self._print_if(ifcond) - def visit_object_type(self, name, info, ifcond, base, members, variants): + def visit_object_type(self, name, info, ifcond, base, members, variants, + features): print('object %s' % name) if base: print(' base %s' % base.name) diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err index 6ff8bb99c5..4340eaf894 100644 --- a/tests/qapi-schema/unknown-expr-key.err +++ b/tests/qapi-schema/unknown-expr-key.err @@ -1,2 +1,2 @@ tests/qapi-schema/unknown-expr-key.json:2: Unknown keys 'bogus', 'phony' in struct 'bar' -Valid keys are 'base', 'data', 'if', 'struct'. +Valid keys are 'base', 'data', 'features', 'if', 'struct'. -- 2.20.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 1/4] qapi: Support features for structs 2019-04-08 14:35 ` [Qemu-devel] [PATCH 1/4] qapi: Support features for structs Kevin Wolf @ 2019-04-08 14:35 ` Kevin Wolf 2019-04-18 20:03 ` Markus Armbruster 1 sibling, 0 replies; 25+ messages in thread From: Kevin Wolf @ 2019-04-08 14:35 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, pkrempa, armbru, qemu-devel Sometimes, the behaviour of QEMU changes compatibly, but without a change in the QMP syntax (usually by allowing values or operations that previously resulted in an error). QMP clients may still need to know whether the extension is available. This allows to add a list of features to struct definitions that will be made visible to QMP clients through schema introspection. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/introspect.json | 8 ++++- docs/devel/qapi-code-gen.txt | 38 ++++++++++++++++++++ scripts/qapi/common.py | 49 ++++++++++++++++++++------ scripts/qapi/doc.py | 3 +- scripts/qapi/introspect.py | 6 +++- scripts/qapi/types.py | 3 +- scripts/qapi/visit.py | 3 +- tests/qapi-schema/double-type.err | 2 +- tests/qapi-schema/test-qapi.py | 3 +- tests/qapi-schema/unknown-expr-key.err | 2 +- 10 files changed, 99 insertions(+), 18 deletions(-) diff --git a/qapi/introspect.json b/qapi/introspect.json index 3d22166b2b..3cb6c1aca4 100644 --- a/qapi/introspect.json +++ b/qapi/introspect.json @@ -174,6 +174,11 @@ # and may even differ from the order of the values of the # enum type of the @tag. # +# @features: names of features that are supported by this version and build and +# aren't othervise visible through schema introspection (e.g. change +# of behaviour related to an object type that didn't come with a +# syntactic change in the schema of the object type) (since: 4.1) +# # Values of this type are JSON object on the wire. # # Since: 2.5 @@ -181,7 +186,8 @@ { 'struct': 'SchemaInfoObject', 'data': { 'members': [ 'SchemaInfoObjectMember' ], '*tag': 'str', - '*variants': [ 'SchemaInfoObjectVariant' ] } } + '*variants': [ 'SchemaInfoObjectVariant' ], + '*features': [ 'str' ] } } ## # @SchemaInfoObjectMember: diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt index b517b0cfbf..e8ec8ac1de 100644 --- a/docs/devel/qapi-code-gen.txt +++ b/docs/devel/qapi-code-gen.txt @@ -719,6 +719,34 @@ any non-empty complex type (struct, union, or alternate), and a pointer to that QAPI type is passed as a single argument. +=== Features === + +Sometimes, the behaviour of QEMU changes compatibly, but without a +change in the QMP syntax (usually by allowing values or operations that +previously resulted in an error). QMP clients may still need to know +whether the extension is available. + +For this purpose, a list of features can be specified for a struct type. +This is exposed to the client as a list of string, where each string +signals that this build of QEMU shows a certain behaviour. + +In the schema, features can be specified as simple strings, for example: + +{ 'struct': 'TestType', + 'data': { 'number': 'int' }, + 'features': [ 'allow-negative-numbers' ] } + +Another option is to specify features as dictionaries, where the key +'name' specifies the feature string to be exposed to clients: + +{ 'struct': 'TestType', + 'data': { 'number': 'int' }, + 'features': [ { 'name': 'allow-negative-numbers' } ] } + +This expanded form is necessary if you want to make the feature +conditional (see below in "Configuring the schema"). + + === Downstream extensions === QAPI schema names that are externally visible, say in the Client JSON @@ -771,6 +799,16 @@ Example: a conditional 'bar' enum member. [ 'foo', { 'name' : 'bar', 'if': 'defined(IFCOND)' } ] } +Similarly, features can be specified as a dictionary with a 'name' and +an 'if' key. + +Example: a conditional 'allow-negative-numbers' feature + +{ 'struct': 'TestType', + 'data': { 'number': 'int' }, + 'features': [ { 'name': 'allow-negative-numbers', + 'if' 'defined(IFCOND)' } ] } + Please note that you are responsible to ensure that the C code will compile with an arbitrary combination of conditions, since the generators are unable to check it at this point. diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index f07869ec73..692d787f04 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -886,12 +886,29 @@ def check_enum(expr, info): def check_struct(expr, info): name = expr['struct'] members = expr['data'] + features = expr.get('features') check_type(info, "'data' for struct '%s'" % name, members, allow_dict=True, allow_optional=True) check_type(info, "'base' for struct '%s'" % name, expr.get('base'), allow_metas=['struct']) + if features: + if not isinstance(features, list): + raise QAPISemError(info, + "Struct '%s' requires an array for 'features'" % + name) + for f in features: + assert isinstance(f, dict) + check_known_keys(info, "feature of struct %s" % (name), f, + ['name'], ['if']) + + check_if(f, info) + f['if'] = listify_cond(f.get('if')) + + if not isinstance(f['name'], str): + raise QAPISemError(info, "Feature names for struct '%s' must " + "be strings" % name) def check_known_keys(info, source, keys, required, optional): @@ -947,6 +964,10 @@ def normalize_members(members): continue members[key] = {'type': arg} +def normalize_features(features): + if isinstance(features, list): + features[:] = [f if isinstance(f, dict) else {'name': f} + for f in features] def check_exprs(exprs): global all_names @@ -986,8 +1007,10 @@ def check_exprs(exprs): normalize_members(expr['data']) elif 'struct' in expr: meta = 'struct' - check_keys(expr_elem, 'struct', ['data'], ['base', 'if']) + check_keys(expr_elem, 'struct', ['data'], + ['base', 'if', 'features']) normalize_members(expr['data']) + normalize_features(expr.get('features')) struct_types[expr[meta]] = expr elif 'command' in expr: meta = 'command' @@ -1126,10 +1149,12 @@ class QAPISchemaVisitor(object): def visit_array_type(self, name, info, ifcond, element_type): pass - def visit_object_type(self, name, info, ifcond, base, members, variants): + def visit_object_type(self, name, info, ifcond, base, members, variants, + features): pass - def visit_object_type_flat(self, name, info, ifcond, members, variants): + def visit_object_type_flat(self, name, info, ifcond, members, variants, + features): pass def visit_alternate_type(self, name, info, ifcond, variants): @@ -1290,7 +1315,7 @@ class QAPISchemaArrayType(QAPISchemaType): class QAPISchemaObjectType(QAPISchemaType): def __init__(self, name, info, doc, ifcond, - base, local_members, variants): + base, local_members, variants, features): # struct has local_members, optional base, and no variants # flat union has base, variants, and no local_members # simple union has local_members, variants, and no base @@ -1307,6 +1332,7 @@ class QAPISchemaObjectType(QAPISchemaType): self.local_members = local_members self.variants = variants self.members = None + self.features = features def check(self, schema): QAPISchemaType.check(self, schema) @@ -1368,9 +1394,11 @@ class QAPISchemaObjectType(QAPISchemaType): def visit(self, visitor): visitor.visit_object_type(self.name, self.info, self.ifcond, - self.base, self.local_members, self.variants) + self.base, self.local_members, self.variants, + self.features) visitor.visit_object_type_flat(self.name, self.info, self.ifcond, - self.members, self.variants) + self.members, self.variants, + self.features) class QAPISchemaMember(object): @@ -1675,7 +1703,7 @@ class QAPISchema(object): ('null', 'null', 'QNull' + pointer_suffix)]: self._def_builtin_type(*t) self.the_empty_object_type = QAPISchemaObjectType( - 'q_empty', None, None, None, None, [], None) + 'q_empty', None, None, None, None, [], None, []) self._def_entity(self.the_empty_object_type) qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', @@ -1721,7 +1749,7 @@ class QAPISchema(object): assert ifcond == typ._ifcond # pylint: disable=protected-access else: self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, - None, members, None)) + None, members, None, [])) return name def _def_enum_type(self, expr, info, doc): @@ -1752,9 +1780,10 @@ class QAPISchema(object): base = expr.get('base') data = expr['data'] ifcond = expr.get('if') + features = expr.get('features') self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, base, self._make_members(data, info), - None)) + None, features)) def _make_variant(self, case, typ, ifcond): return QAPISchemaObjectTypeVariant(case, typ, ifcond) @@ -1795,7 +1824,7 @@ class QAPISchema(object): QAPISchemaObjectType(name, info, doc, ifcond, base, members, QAPISchemaObjectTypeVariants(tag_name, tag_member, - variants))) + variants), [])) def _def_alternate_type(self, expr, info, doc): name = expr['alternate'] diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py index 5c8c136899..433e9fcbfb 100755 --- a/scripts/qapi/doc.py +++ b/scripts/qapi/doc.py @@ -220,7 +220,8 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor): body=texi_entity(doc, 'Values', ifcond, member_func=texi_enum_value))) - def visit_object_type(self, name, info, ifcond, base, members, variants): + def visit_object_type(self, name, info, ifcond, base, members, variants, + features): doc = self.cur_doc if base and base.is_implicit(): base = None diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index f7f2ca07e4..8909cecde4 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -188,11 +188,15 @@ const QLitObject %(c_name)s = %(c_string)s; self._gen_qlit('[' + element + ']', 'array', {'element-type': element}, ifcond) - def visit_object_type_flat(self, name, info, ifcond, members, variants): + def visit_object_type_flat(self, name, info, ifcond, members, variants, + features): obj = {'members': [self._gen_member(m) for m in members]} if variants: obj.update(self._gen_variants(variants.tag_member.name, variants.variants)) + if features: + obj['features'] = [ (f['name'], f) for f in features ] + self._gen_qlit(name, 'object', obj, ifcond) def visit_alternate_type(self, name, info, ifcond, variants): diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index 2bd6fcd44f..3edd9374aa 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -227,7 +227,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor): self._genh.add(gen_array(name, element_type)) self._gen_type_cleanup(name) - def visit_object_type(self, name, info, ifcond, base, members, variants): + def visit_object_type(self, name, info, ifcond, base, members, variants, + features): # Nothing to do for the special empty builtin if name == 'q_empty': return diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py index 826b8066e1..c1cd675c95 100644 --- a/scripts/qapi/visit.py +++ b/scripts/qapi/visit.py @@ -326,7 +326,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor): self._genh.add(gen_visit_decl(name)) self._genc.add(gen_visit_list(name, element_type)) - def visit_object_type(self, name, info, ifcond, base, members, variants): + def visit_object_type(self, name, info, ifcond, base, members, variants, + features): # Nothing to do for the special empty builtin if name == 'q_empty': return diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err index 799193dba1..69457173a7 100644 --- a/tests/qapi-schema/double-type.err +++ b/tests/qapi-schema/double-type.err @@ -1,2 +1,2 @@ tests/qapi-schema/double-type.json:2: Unknown key 'command' in struct 'bar' -Valid keys are 'base', 'data', 'if', 'struct'. +Valid keys are 'base', 'data', 'features', 'if', 'struct'. diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py index d21fca01fc..f2d6815c86 100644 --- a/tests/qapi-schema/test-qapi.py +++ b/tests/qapi-schema/test-qapi.py @@ -38,7 +38,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): print('array %s %s' % (name, element_type.name)) self._print_if(ifcond) - def visit_object_type(self, name, info, ifcond, base, members, variants): + def visit_object_type(self, name, info, ifcond, base, members, variants, + features): print('object %s' % name) if base: print(' base %s' % base.name) diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err index 6ff8bb99c5..4340eaf894 100644 --- a/tests/qapi-schema/unknown-expr-key.err +++ b/tests/qapi-schema/unknown-expr-key.err @@ -1,2 +1,2 @@ tests/qapi-schema/unknown-expr-key.json:2: Unknown keys 'bogus', 'phony' in struct 'bar' -Valid keys are 'base', 'data', 'if', 'struct'. +Valid keys are 'base', 'data', 'features', 'if', 'struct'. -- 2.20.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] qapi: Support features for structs 2019-04-08 14:35 ` [Qemu-devel] [PATCH 1/4] qapi: Support features for structs Kevin Wolf 2019-04-08 14:35 ` Kevin Wolf @ 2019-04-18 20:03 ` Markus Armbruster 2019-04-18 20:03 ` Markus Armbruster 2019-05-15 10:58 ` Kevin Wolf 1 sibling, 2 replies; 25+ messages in thread From: Markus Armbruster @ 2019-04-18 20:03 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-block, pkrempa, qemu-devel Kevin Wolf <kwolf@redhat.com> writes: > Sometimes, the behaviour of QEMU changes compatibly, but without a > change in the QMP syntax (usually by allowing values or operations that > previously resulted in an error). QMP clients may still need to know > whether the extension is available. > > This allows to add a list of features to struct definitions that will be > made visible to QMP clients through schema introspection. Only a first step, but that's okay. I think we want to be able to tack features to all user-defined types, commands, and events. Ideally even to members of enum and object types. Use case: feature 'deprecated'. We talked about ways to communicate deprecation to management applications. Introspection was proposed. Feature 'deprecated' delivers it. > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > qapi/introspect.json | 8 ++++- > docs/devel/qapi-code-gen.txt | 38 ++++++++++++++++++++ > scripts/qapi/common.py | 49 ++++++++++++++++++++------ > scripts/qapi/doc.py | 3 +- > scripts/qapi/introspect.py | 6 +++- > scripts/qapi/types.py | 3 +- > scripts/qapi/visit.py | 3 +- > tests/qapi-schema/double-type.err | 2 +- > tests/qapi-schema/test-qapi.py | 3 +- > tests/qapi-schema/unknown-expr-key.err | 2 +- > 10 files changed, 99 insertions(+), 18 deletions(-) > > diff --git a/qapi/introspect.json b/qapi/introspect.json > index 3d22166b2b..3cb6c1aca4 100644 > --- a/qapi/introspect.json > +++ b/qapi/introspect.json > @@ -174,6 +174,11 @@ > # and may even differ from the order of the values of the > # enum type of the @tag. > # > +# @features: names of features that are supported by this version and build and > +# aren't othervise visible through schema introspection (e.g. change > +# of behaviour related to an object type that didn't come with a > +# syntactic change in the schema of the object type) (since: 4.1) > +# > # Values of this type are JSON object on the wire. > # > # Since: 2.5 > @@ -181,7 +186,8 @@ > { 'struct': 'SchemaInfoObject', > 'data': { 'members': [ 'SchemaInfoObjectMember' ], > '*tag': 'str', > - '*variants': [ 'SchemaInfoObjectVariant' ] } } > + '*variants': [ 'SchemaInfoObjectVariant' ], > + '*features': [ 'str' ] } } > > ## > # @SchemaInfoObjectMember: > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt > index b517b0cfbf..e8ec8ac1de 100644 > --- a/docs/devel/qapi-code-gen.txt > +++ b/docs/devel/qapi-code-gen.txt > @@ -719,6 +719,34 @@ any non-empty complex type (struct, union, or alternate), and a > pointer to that QAPI type is passed as a single argument. > > > +=== Features === This bolts on features similar to how we bolted on conditionals. Minimally invasive, but the result is suboptimal. It'll do for now. > + > +Sometimes, the behaviour of QEMU changes compatibly, but without a > +change in the QMP syntax (usually by allowing values or operations that > +previously resulted in an error). QMP clients may still need to know > +whether the extension is available. > + > +For this purpose, a list of features can be specified for a struct type. > +This is exposed to the client as a list of string, where each string > +signals that this build of QEMU shows a certain behaviour. > + > +In the schema, features can be specified as simple strings, for example: > + > +{ 'struct': 'TestType', > + 'data': { 'number': 'int' }, > + 'features': [ 'allow-negative-numbers' ] } > + > +Another option is to specify features as dictionaries, where the key > +'name' specifies the feature string to be exposed to clients: > + > +{ 'struct': 'TestType', > + 'data': { 'number': 'int' }, > + 'features': [ { 'name': 'allow-negative-numbers' } ] } > + > +This expanded form is necessary if you want to make the feature > +conditional (see below in "Configuring the schema"). You justify the general form with a forward reference. I guess that's the best you can do without reorganizing the document. Let's worry about that later. > + > + > === Downstream extensions === > > QAPI schema names that are externally visible, say in the Client JSON > @@ -771,6 +799,16 @@ Example: a conditional 'bar' enum member. Example: a conditional 'bar' enum member. { 'enum': 'IfEnum', 'data': > [ 'foo', > { 'name' : 'bar', 'if': 'defined(IFCOND)' } ] } > > +Similarly, features can be specified as a dictionary with a 'name' and > +an 'if' key. > + > +Example: a conditional 'allow-negative-numbers' feature > + > +{ 'struct': 'TestType', > + 'data': { 'number': 'int' }, > + 'features': [ { 'name': 'allow-negative-numbers', > + 'if' 'defined(IFCOND)' } ] } > + Note the structure of enum's 'data' is identical to struct's 'features'. We'll get back to that below. > Please note that you are responsible to ensure that the C code will > compile with an arbitrary combination of conditions, since the > generators are unable to check it at this point. > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index f07869ec73..692d787f04 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -886,12 +886,29 @@ def check_enum(expr, info): > def check_struct(expr, info): > name = expr['struct'] > members = expr['data'] > + features = expr.get('features') > > check_type(info, "'data' for struct '%s'" % name, members, > allow_dict=True, allow_optional=True) > check_type(info, "'base' for struct '%s'" % name, expr.get('base'), > allow_metas=['struct']) > > + if features: > + if not isinstance(features, list): > + raise QAPISemError(info, > + "Struct '%s' requires an array for 'features'" % > + name) > + for f in features: > + assert isinstance(f, dict) > + check_known_keys(info, "feature of struct %s" % (name), f, > + ['name'], ['if']) > + > + check_if(f, info) > + f['if'] = listify_cond(f.get('if')) > + > + if not isinstance(f['name'], str): > + raise QAPISemError(info, "Feature names for struct '%s' must " > + "be strings" % name) Duplicates much of check_enum(). That's because the list of features has the same structure as the list of enumeration values. We can worry about avoiding duplication later. Let's examine the differences to check_enum(). check_enum() additionally checks prefix, which we don't have here. check_enum() checks member names with check_name(). Here, we check only "feature name is str". I think requiring feature names to conform to the common rules for QAPI names would be prudent. Let's call check_name(). check_enum() doesn't call listify_cond(). Only QAPISchemaFOO methods do. I think it should be called in QAPISchemaObjectType.__init__() instead. See there. > > def check_known_keys(info, source, keys, required, optional): > > @@ -947,6 +964,10 @@ def normalize_members(members): > continue > members[key] = {'type': arg} > > +def normalize_features(features): > + if isinstance(features, list): > + features[:] = [f if isinstance(f, dict) else {'name': f} > + for f in features] Duplicates much of normalize_enum() for the same reason. > > def check_exprs(exprs): > global all_names > @@ -986,8 +1007,10 @@ def check_exprs(exprs): > normalize_members(expr['data']) > elif 'struct' in expr: > meta = 'struct' > - check_keys(expr_elem, 'struct', ['data'], ['base', 'if']) > + check_keys(expr_elem, 'struct', ['data'], > + ['base', 'if', 'features']) > normalize_members(expr['data']) > + normalize_features(expr.get('features')) > struct_types[expr[meta]] = expr > elif 'command' in expr: > meta = 'command' > @@ -1126,10 +1149,12 @@ class QAPISchemaVisitor(object): > def visit_array_type(self, name, info, ifcond, element_type): > pass > > - def visit_object_type(self, name, info, ifcond, base, members, variants): > + def visit_object_type(self, name, info, ifcond, base, members, variants, > + features): > pass > > - def visit_object_type_flat(self, name, info, ifcond, members, variants): > + def visit_object_type_flat(self, name, info, ifcond, members, variants, > + features): > pass > > def visit_alternate_type(self, name, info, ifcond, variants): > @@ -1290,7 +1315,7 @@ class QAPISchemaArrayType(QAPISchemaType): > > class QAPISchemaObjectType(QAPISchemaType): > def __init__(self, name, info, doc, ifcond, > - base, local_members, variants): > + base, local_members, variants, features): > # struct has local_members, optional base, and no variants > # flat union has base, variants, and no local_members > # simple union has local_members, variants, and no base > @@ -1307,6 +1332,7 @@ class QAPISchemaObjectType(QAPISchemaType): > self.local_members = local_members > self.variants = variants > self.members = None > + self.features = features I think this should be self.features = listify_cond(features) > > def check(self, schema): > QAPISchemaType.check(self, schema) > @@ -1368,9 +1394,11 @@ class QAPISchemaObjectType(QAPISchemaType): > > def visit(self, visitor): > visitor.visit_object_type(self.name, self.info, self.ifcond, > - self.base, self.local_members, self.variants) > + self.base, self.local_members, self.variants, > + self.features) > visitor.visit_object_type_flat(self.name, self.info, self.ifcond, > - self.members, self.variants) > + self.members, self.variants, > + self.features) > > > class QAPISchemaMember(object): > @@ -1675,7 +1703,7 @@ class QAPISchema(object): > ('null', 'null', 'QNull' + pointer_suffix)]: > self._def_builtin_type(*t) > self.the_empty_object_type = QAPISchemaObjectType( > - 'q_empty', None, None, None, None, [], None) > + 'q_empty', None, None, None, None, [], None, []) The built-in empty object has no features. Good. > self._def_entity(self.the_empty_object_type) > > qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', > @@ -1721,7 +1749,7 @@ class QAPISchema(object): > assert ifcond == typ._ifcond # pylint: disable=protected-access > else: > self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, > - None, members, None)) > + None, members, None, [])) Implicit object types have no features. These are: * Simple union variant wrapper types The wrapped type can carry the feature * Flat union's anonymous base type If you want features, use a named type. * Command's anonymous parameter type Likewise. * Event's anonymous parameter type Likewise. Okay. > return name > > def _def_enum_type(self, expr, info, doc): > @@ -1752,9 +1780,10 @@ class QAPISchema(object): > base = expr.get('base') > data = expr['data'] > ifcond = expr.get('if') > + features = expr.get('features') > self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, base, > self._make_members(data, info), > - None)) > + None, features)) This is struct. Okay. For enum, we do: self._def_entity(QAPISchemaEnumType( name, info, doc, ifcond, self._make_enum_members(data), prefix)) where def _make_enum_members(self, values): return [QAPISchemaMember(v['name'], v.get('if')) for v in values] i.e. we represent the enum members as a list of QAPISchemaMember. Features remain a list of dicts. Any particular reason for not making them QAPISchemaMember? > > def _make_variant(self, case, typ, ifcond): > return QAPISchemaObjectTypeVariant(case, typ, ifcond) > @@ -1795,7 +1824,7 @@ class QAPISchema(object): > QAPISchemaObjectType(name, info, doc, ifcond, base, members, > QAPISchemaObjectTypeVariants(tag_name, > tag_member, > - variants))) > + variants), [])) This is union. We clearly want to support features there. This patch only implements them for struct. That's okay, we don't need to implement them everywhere we want them in one go. > > def _def_alternate_type(self, expr, info, doc): > name = expr['alternate'] > diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py > index 5c8c136899..433e9fcbfb 100755 > --- a/scripts/qapi/doc.py > +++ b/scripts/qapi/doc.py > @@ -220,7 +220,8 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor): > body=texi_entity(doc, 'Values', ifcond, > member_func=texi_enum_value))) > > - def visit_object_type(self, name, info, ifcond, base, members, variants): > + def visit_object_type(self, name, info, ifcond, base, members, variants, > + features): > doc = self.cur_doc > if base and base.is_implicit(): > base = None > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index f7f2ca07e4..8909cecde4 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -188,11 +188,15 @@ const QLitObject %(c_name)s = %(c_string)s; > self._gen_qlit('[' + element + ']', 'array', {'element-type': element}, > ifcond) > > - def visit_object_type_flat(self, name, info, ifcond, members, variants): > + def visit_object_type_flat(self, name, info, ifcond, members, variants, > + features): > obj = {'members': [self._gen_member(m) for m in members]} > if variants: > obj.update(self._gen_variants(variants.tag_member.name, > variants.variants)) > + if features: > + obj['features'] = [ (f['name'], f) for f in features ] Compare: def visit_enum_type(self, name, info, ifcond, members, prefix): self._gen_qlit(name, 'enum', {'values': [(m.name, {'if': m.ifcond}) for m in members]}, ifcond) Since f is a dict, not an object, we need to use f['name']. Since f is a dict, abusing it as the tuple's second element works. Cleaner: (f['name'], {'if': f['if']}) > + > self._gen_qlit(name, 'object', obj, ifcond) > > def visit_alternate_type(self, name, info, ifcond, variants): > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py > index 2bd6fcd44f..3edd9374aa 100644 > --- a/scripts/qapi/types.py > +++ b/scripts/qapi/types.py > @@ -227,7 +227,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor): > self._genh.add(gen_array(name, element_type)) > self._gen_type_cleanup(name) > > - def visit_object_type(self, name, info, ifcond, base, members, variants): > + def visit_object_type(self, name, info, ifcond, base, members, variants, > + features): > # Nothing to do for the special empty builtin > if name == 'q_empty': > return > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py > index 826b8066e1..c1cd675c95 100644 > --- a/scripts/qapi/visit.py > +++ b/scripts/qapi/visit.py > @@ -326,7 +326,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor): > self._genh.add(gen_visit_decl(name)) > self._genc.add(gen_visit_list(name, element_type)) > > - def visit_object_type(self, name, info, ifcond, base, members, variants): > + def visit_object_type(self, name, info, ifcond, base, members, variants, > + features): > # Nothing to do for the special empty builtin > if name == 'q_empty': > return > diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err > index 799193dba1..69457173a7 100644 > --- a/tests/qapi-schema/double-type.err > +++ b/tests/qapi-schema/double-type.err > @@ -1,2 +1,2 @@ > tests/qapi-schema/double-type.json:2: Unknown key 'command' in struct 'bar' > -Valid keys are 'base', 'data', 'if', 'struct'. > +Valid keys are 'base', 'data', 'features', 'if', 'struct'. > diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py > index d21fca01fc..f2d6815c86 100644 > --- a/tests/qapi-schema/test-qapi.py > +++ b/tests/qapi-schema/test-qapi.py > @@ -38,7 +38,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): > print('array %s %s' % (name, element_type.name)) > self._print_if(ifcond) > > - def visit_object_type(self, name, info, ifcond, base, members, variants): > + def visit_object_type(self, name, info, ifcond, base, members, variants, > + features): > print('object %s' % name) > if base: > print(' base %s' % base.name) Print the features, please. Steal the code from visit_enum_type(). > diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err > index 6ff8bb99c5..4340eaf894 100644 > --- a/tests/qapi-schema/unknown-expr-key.err > +++ b/tests/qapi-schema/unknown-expr-key.err > @@ -1,2 +1,2 @@ > tests/qapi-schema/unknown-expr-key.json:2: Unknown keys 'bogus', 'phony' in struct 'bar' > -Valid keys are 'base', 'data', 'if', 'struct'. > +Valid keys are 'base', 'data', 'features', 'if', 'struct'. Missing tests. Ah, I see these are in the next patches, nevermind. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] qapi: Support features for structs 2019-04-18 20:03 ` Markus Armbruster @ 2019-04-18 20:03 ` Markus Armbruster 2019-05-15 10:58 ` Kevin Wolf 1 sibling, 0 replies; 25+ messages in thread From: Markus Armbruster @ 2019-04-18 20:03 UTC (permalink / raw) To: Kevin Wolf; +Cc: pkrempa, qemu-devel, qemu-block Kevin Wolf <kwolf@redhat.com> writes: > Sometimes, the behaviour of QEMU changes compatibly, but without a > change in the QMP syntax (usually by allowing values or operations that > previously resulted in an error). QMP clients may still need to know > whether the extension is available. > > This allows to add a list of features to struct definitions that will be > made visible to QMP clients through schema introspection. Only a first step, but that's okay. I think we want to be able to tack features to all user-defined types, commands, and events. Ideally even to members of enum and object types. Use case: feature 'deprecated'. We talked about ways to communicate deprecation to management applications. Introspection was proposed. Feature 'deprecated' delivers it. > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > qapi/introspect.json | 8 ++++- > docs/devel/qapi-code-gen.txt | 38 ++++++++++++++++++++ > scripts/qapi/common.py | 49 ++++++++++++++++++++------ > scripts/qapi/doc.py | 3 +- > scripts/qapi/introspect.py | 6 +++- > scripts/qapi/types.py | 3 +- > scripts/qapi/visit.py | 3 +- > tests/qapi-schema/double-type.err | 2 +- > tests/qapi-schema/test-qapi.py | 3 +- > tests/qapi-schema/unknown-expr-key.err | 2 +- > 10 files changed, 99 insertions(+), 18 deletions(-) > > diff --git a/qapi/introspect.json b/qapi/introspect.json > index 3d22166b2b..3cb6c1aca4 100644 > --- a/qapi/introspect.json > +++ b/qapi/introspect.json > @@ -174,6 +174,11 @@ > # and may even differ from the order of the values of the > # enum type of the @tag. > # > +# @features: names of features that are supported by this version and build and > +# aren't othervise visible through schema introspection (e.g. change > +# of behaviour related to an object type that didn't come with a > +# syntactic change in the schema of the object type) (since: 4.1) > +# > # Values of this type are JSON object on the wire. > # > # Since: 2.5 > @@ -181,7 +186,8 @@ > { 'struct': 'SchemaInfoObject', > 'data': { 'members': [ 'SchemaInfoObjectMember' ], > '*tag': 'str', > - '*variants': [ 'SchemaInfoObjectVariant' ] } } > + '*variants': [ 'SchemaInfoObjectVariant' ], > + '*features': [ 'str' ] } } > > ## > # @SchemaInfoObjectMember: > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt > index b517b0cfbf..e8ec8ac1de 100644 > --- a/docs/devel/qapi-code-gen.txt > +++ b/docs/devel/qapi-code-gen.txt > @@ -719,6 +719,34 @@ any non-empty complex type (struct, union, or alternate), and a > pointer to that QAPI type is passed as a single argument. > > > +=== Features === This bolts on features similar to how we bolted on conditionals. Minimally invasive, but the result is suboptimal. It'll do for now. > + > +Sometimes, the behaviour of QEMU changes compatibly, but without a > +change in the QMP syntax (usually by allowing values or operations that > +previously resulted in an error). QMP clients may still need to know > +whether the extension is available. > + > +For this purpose, a list of features can be specified for a struct type. > +This is exposed to the client as a list of string, where each string > +signals that this build of QEMU shows a certain behaviour. > + > +In the schema, features can be specified as simple strings, for example: > + > +{ 'struct': 'TestType', > + 'data': { 'number': 'int' }, > + 'features': [ 'allow-negative-numbers' ] } > + > +Another option is to specify features as dictionaries, where the key > +'name' specifies the feature string to be exposed to clients: > + > +{ 'struct': 'TestType', > + 'data': { 'number': 'int' }, > + 'features': [ { 'name': 'allow-negative-numbers' } ] } > + > +This expanded form is necessary if you want to make the feature > +conditional (see below in "Configuring the schema"). You justify the general form with a forward reference. I guess that's the best you can do without reorganizing the document. Let's worry about that later. > + > + > === Downstream extensions === > > QAPI schema names that are externally visible, say in the Client JSON > @@ -771,6 +799,16 @@ Example: a conditional 'bar' enum member. Example: a conditional 'bar' enum member. { 'enum': 'IfEnum', 'data': > [ 'foo', > { 'name' : 'bar', 'if': 'defined(IFCOND)' } ] } > > +Similarly, features can be specified as a dictionary with a 'name' and > +an 'if' key. > + > +Example: a conditional 'allow-negative-numbers' feature > + > +{ 'struct': 'TestType', > + 'data': { 'number': 'int' }, > + 'features': [ { 'name': 'allow-negative-numbers', > + 'if' 'defined(IFCOND)' } ] } > + Note the structure of enum's 'data' is identical to struct's 'features'. We'll get back to that below. > Please note that you are responsible to ensure that the C code will > compile with an arbitrary combination of conditions, since the > generators are unable to check it at this point. > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index f07869ec73..692d787f04 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -886,12 +886,29 @@ def check_enum(expr, info): > def check_struct(expr, info): > name = expr['struct'] > members = expr['data'] > + features = expr.get('features') > > check_type(info, "'data' for struct '%s'" % name, members, > allow_dict=True, allow_optional=True) > check_type(info, "'base' for struct '%s'" % name, expr.get('base'), > allow_metas=['struct']) > > + if features: > + if not isinstance(features, list): > + raise QAPISemError(info, > + "Struct '%s' requires an array for 'features'" % > + name) > + for f in features: > + assert isinstance(f, dict) > + check_known_keys(info, "feature of struct %s" % (name), f, > + ['name'], ['if']) > + > + check_if(f, info) > + f['if'] = listify_cond(f.get('if')) > + > + if not isinstance(f['name'], str): > + raise QAPISemError(info, "Feature names for struct '%s' must " > + "be strings" % name) Duplicates much of check_enum(). That's because the list of features has the same structure as the list of enumeration values. We can worry about avoiding duplication later. Let's examine the differences to check_enum(). check_enum() additionally checks prefix, which we don't have here. check_enum() checks member names with check_name(). Here, we check only "feature name is str". I think requiring feature names to conform to the common rules for QAPI names would be prudent. Let's call check_name(). check_enum() doesn't call listify_cond(). Only QAPISchemaFOO methods do. I think it should be called in QAPISchemaObjectType.__init__() instead. See there. > > def check_known_keys(info, source, keys, required, optional): > > @@ -947,6 +964,10 @@ def normalize_members(members): > continue > members[key] = {'type': arg} > > +def normalize_features(features): > + if isinstance(features, list): > + features[:] = [f if isinstance(f, dict) else {'name': f} > + for f in features] Duplicates much of normalize_enum() for the same reason. > > def check_exprs(exprs): > global all_names > @@ -986,8 +1007,10 @@ def check_exprs(exprs): > normalize_members(expr['data']) > elif 'struct' in expr: > meta = 'struct' > - check_keys(expr_elem, 'struct', ['data'], ['base', 'if']) > + check_keys(expr_elem, 'struct', ['data'], > + ['base', 'if', 'features']) > normalize_members(expr['data']) > + normalize_features(expr.get('features')) > struct_types[expr[meta]] = expr > elif 'command' in expr: > meta = 'command' > @@ -1126,10 +1149,12 @@ class QAPISchemaVisitor(object): > def visit_array_type(self, name, info, ifcond, element_type): > pass > > - def visit_object_type(self, name, info, ifcond, base, members, variants): > + def visit_object_type(self, name, info, ifcond, base, members, variants, > + features): > pass > > - def visit_object_type_flat(self, name, info, ifcond, members, variants): > + def visit_object_type_flat(self, name, info, ifcond, members, variants, > + features): > pass > > def visit_alternate_type(self, name, info, ifcond, variants): > @@ -1290,7 +1315,7 @@ class QAPISchemaArrayType(QAPISchemaType): > > class QAPISchemaObjectType(QAPISchemaType): > def __init__(self, name, info, doc, ifcond, > - base, local_members, variants): > + base, local_members, variants, features): > # struct has local_members, optional base, and no variants > # flat union has base, variants, and no local_members > # simple union has local_members, variants, and no base > @@ -1307,6 +1332,7 @@ class QAPISchemaObjectType(QAPISchemaType): > self.local_members = local_members > self.variants = variants > self.members = None > + self.features = features I think this should be self.features = listify_cond(features) > > def check(self, schema): > QAPISchemaType.check(self, schema) > @@ -1368,9 +1394,11 @@ class QAPISchemaObjectType(QAPISchemaType): > > def visit(self, visitor): > visitor.visit_object_type(self.name, self.info, self.ifcond, > - self.base, self.local_members, self.variants) > + self.base, self.local_members, self.variants, > + self.features) > visitor.visit_object_type_flat(self.name, self.info, self.ifcond, > - self.members, self.variants) > + self.members, self.variants, > + self.features) > > > class QAPISchemaMember(object): > @@ -1675,7 +1703,7 @@ class QAPISchema(object): > ('null', 'null', 'QNull' + pointer_suffix)]: > self._def_builtin_type(*t) > self.the_empty_object_type = QAPISchemaObjectType( > - 'q_empty', None, None, None, None, [], None) > + 'q_empty', None, None, None, None, [], None, []) The built-in empty object has no features. Good. > self._def_entity(self.the_empty_object_type) > > qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', > @@ -1721,7 +1749,7 @@ class QAPISchema(object): > assert ifcond == typ._ifcond # pylint: disable=protected-access > else: > self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, > - None, members, None)) > + None, members, None, [])) Implicit object types have no features. These are: * Simple union variant wrapper types The wrapped type can carry the feature * Flat union's anonymous base type If you want features, use a named type. * Command's anonymous parameter type Likewise. * Event's anonymous parameter type Likewise. Okay. > return name > > def _def_enum_type(self, expr, info, doc): > @@ -1752,9 +1780,10 @@ class QAPISchema(object): > base = expr.get('base') > data = expr['data'] > ifcond = expr.get('if') > + features = expr.get('features') > self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, base, > self._make_members(data, info), > - None)) > + None, features)) This is struct. Okay. For enum, we do: self._def_entity(QAPISchemaEnumType( name, info, doc, ifcond, self._make_enum_members(data), prefix)) where def _make_enum_members(self, values): return [QAPISchemaMember(v['name'], v.get('if')) for v in values] i.e. we represent the enum members as a list of QAPISchemaMember. Features remain a list of dicts. Any particular reason for not making them QAPISchemaMember? > > def _make_variant(self, case, typ, ifcond): > return QAPISchemaObjectTypeVariant(case, typ, ifcond) > @@ -1795,7 +1824,7 @@ class QAPISchema(object): > QAPISchemaObjectType(name, info, doc, ifcond, base, members, > QAPISchemaObjectTypeVariants(tag_name, > tag_member, > - variants))) > + variants), [])) This is union. We clearly want to support features there. This patch only implements them for struct. That's okay, we don't need to implement them everywhere we want them in one go. > > def _def_alternate_type(self, expr, info, doc): > name = expr['alternate'] > diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py > index 5c8c136899..433e9fcbfb 100755 > --- a/scripts/qapi/doc.py > +++ b/scripts/qapi/doc.py > @@ -220,7 +220,8 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor): > body=texi_entity(doc, 'Values', ifcond, > member_func=texi_enum_value))) > > - def visit_object_type(self, name, info, ifcond, base, members, variants): > + def visit_object_type(self, name, info, ifcond, base, members, variants, > + features): > doc = self.cur_doc > if base and base.is_implicit(): > base = None > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index f7f2ca07e4..8909cecde4 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -188,11 +188,15 @@ const QLitObject %(c_name)s = %(c_string)s; > self._gen_qlit('[' + element + ']', 'array', {'element-type': element}, > ifcond) > > - def visit_object_type_flat(self, name, info, ifcond, members, variants): > + def visit_object_type_flat(self, name, info, ifcond, members, variants, > + features): > obj = {'members': [self._gen_member(m) for m in members]} > if variants: > obj.update(self._gen_variants(variants.tag_member.name, > variants.variants)) > + if features: > + obj['features'] = [ (f['name'], f) for f in features ] Compare: def visit_enum_type(self, name, info, ifcond, members, prefix): self._gen_qlit(name, 'enum', {'values': [(m.name, {'if': m.ifcond}) for m in members]}, ifcond) Since f is a dict, not an object, we need to use f['name']. Since f is a dict, abusing it as the tuple's second element works. Cleaner: (f['name'], {'if': f['if']}) > + > self._gen_qlit(name, 'object', obj, ifcond) > > def visit_alternate_type(self, name, info, ifcond, variants): > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py > index 2bd6fcd44f..3edd9374aa 100644 > --- a/scripts/qapi/types.py > +++ b/scripts/qapi/types.py > @@ -227,7 +227,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor): > self._genh.add(gen_array(name, element_type)) > self._gen_type_cleanup(name) > > - def visit_object_type(self, name, info, ifcond, base, members, variants): > + def visit_object_type(self, name, info, ifcond, base, members, variants, > + features): > # Nothing to do for the special empty builtin > if name == 'q_empty': > return > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py > index 826b8066e1..c1cd675c95 100644 > --- a/scripts/qapi/visit.py > +++ b/scripts/qapi/visit.py > @@ -326,7 +326,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor): > self._genh.add(gen_visit_decl(name)) > self._genc.add(gen_visit_list(name, element_type)) > > - def visit_object_type(self, name, info, ifcond, base, members, variants): > + def visit_object_type(self, name, info, ifcond, base, members, variants, > + features): > # Nothing to do for the special empty builtin > if name == 'q_empty': > return > diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err > index 799193dba1..69457173a7 100644 > --- a/tests/qapi-schema/double-type.err > +++ b/tests/qapi-schema/double-type.err > @@ -1,2 +1,2 @@ > tests/qapi-schema/double-type.json:2: Unknown key 'command' in struct 'bar' > -Valid keys are 'base', 'data', 'if', 'struct'. > +Valid keys are 'base', 'data', 'features', 'if', 'struct'. > diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py > index d21fca01fc..f2d6815c86 100644 > --- a/tests/qapi-schema/test-qapi.py > +++ b/tests/qapi-schema/test-qapi.py > @@ -38,7 +38,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): > print('array %s %s' % (name, element_type.name)) > self._print_if(ifcond) > > - def visit_object_type(self, name, info, ifcond, base, members, variants): > + def visit_object_type(self, name, info, ifcond, base, members, variants, > + features): > print('object %s' % name) > if base: > print(' base %s' % base.name) Print the features, please. Steal the code from visit_enum_type(). > diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err > index 6ff8bb99c5..4340eaf894 100644 > --- a/tests/qapi-schema/unknown-expr-key.err > +++ b/tests/qapi-schema/unknown-expr-key.err > @@ -1,2 +1,2 @@ > tests/qapi-schema/unknown-expr-key.json:2: Unknown keys 'bogus', 'phony' in struct 'bar' > -Valid keys are 'base', 'data', 'if', 'struct'. > +Valid keys are 'base', 'data', 'features', 'if', 'struct'. Missing tests. Ah, I see these are in the next patches, nevermind. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] qapi: Support features for structs 2019-04-18 20:03 ` Markus Armbruster 2019-04-18 20:03 ` Markus Armbruster @ 2019-05-15 10:58 ` Kevin Wolf 2019-05-15 11:22 ` Peter Krempa 2019-05-15 13:48 ` Markus Armbruster 1 sibling, 2 replies; 25+ messages in thread From: Kevin Wolf @ 2019-05-15 10:58 UTC (permalink / raw) To: Markus Armbruster; +Cc: pkrempa, qemu-devel, qemu-block Am 18.04.2019 um 22:03 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Sometimes, the behaviour of QEMU changes compatibly, but without a > > change in the QMP syntax (usually by allowing values or operations that > > previously resulted in an error). QMP clients may still need to know > > whether the extension is available. > > > > This allows to add a list of features to struct definitions that will be > > made visible to QMP clients through schema introspection. > > Only a first step, but that's okay. I think we want to be able to tack > features to all user-defined types, commands, and events. Ideally even > to members of enum and object types. > > Use case: feature 'deprecated'. We talked about ways to communicate > deprecation to management applications. Introspection was proposed. > Feature 'deprecated' delivers it. How does introspection solve the problem? It requires the client to actively look for a deprecation instead of notifying it that it is using something deprecated. Do you expect libvirt to check a full list of all QMP commands, types, etc. it ever uses against the schema after starting a VM or something like that? > > def check_exprs(exprs): > > global all_names > > @@ -986,8 +1007,10 @@ def check_exprs(exprs): > > normalize_members(expr['data']) > > elif 'struct' in expr: > > meta = 'struct' > > - check_keys(expr_elem, 'struct', ['data'], ['base', 'if']) > > + check_keys(expr_elem, 'struct', ['data'], > > + ['base', 'if', 'features']) > > normalize_members(expr['data']) > > + normalize_features(expr.get('features')) > > struct_types[expr[meta]] = expr > > elif 'command' in expr: > > meta = 'command' > > @@ -1126,10 +1149,12 @@ class QAPISchemaVisitor(object): > > def visit_array_type(self, name, info, ifcond, element_type): > > pass > > > > - def visit_object_type(self, name, info, ifcond, base, members, variants): > > + def visit_object_type(self, name, info, ifcond, base, members, variants, > > + features): > > pass > > > > - def visit_object_type_flat(self, name, info, ifcond, members, variants): > > + def visit_object_type_flat(self, name, info, ifcond, members, variants, > > + features): > > pass > > > > def visit_alternate_type(self, name, info, ifcond, variants): > > @@ -1290,7 +1315,7 @@ class QAPISchemaArrayType(QAPISchemaType): > > > > class QAPISchemaObjectType(QAPISchemaType): > > def __init__(self, name, info, doc, ifcond, > > - base, local_members, variants): > > + base, local_members, variants, features): > > # struct has local_members, optional base, and no variants > > # flat union has base, variants, and no local_members > > # simple union has local_members, variants, and no base > > @@ -1307,6 +1332,7 @@ class QAPISchemaObjectType(QAPISchemaType): > > self.local_members = local_members > > self.variants = variants > > self.members = None > > + self.features = features > > I think this should be > > self.features = listify_cond(features) Not quite, listify_cond() takes the condition it, not the entity that contains it. But I can do this: self.features = features for f in self.features or []: f['if'] = listify_cond(f.get('if')) > > @@ -1752,9 +1780,10 @@ class QAPISchema(object): > > base = expr.get('base') > > data = expr['data'] > > ifcond = expr.get('if') > > + features = expr.get('features') > > self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, base, > > self._make_members(data, info), > > - None)) > > + None, features)) > > This is struct. Okay. > > For enum, we do: > > self._def_entity(QAPISchemaEnumType( > name, info, doc, ifcond, > self._make_enum_members(data), prefix)) > > where > > def _make_enum_members(self, values): > return [QAPISchemaMember(v['name'], v.get('if')) for v in values] > > i.e. we represent the enum members as a list of QAPISchemaMember. > > Features remain a list of dicts. > > Any particular reason for not making them QAPISchemaMember? I don't know what QAPISchemaMember is, and the documentation doesn't tell me either. Its code looks related to generated C code, but features don't generate C code. I guess I could use it anyway and just never call any of its methods but just use it to store things in attributes rather than dict entries, but somehow creating an object and not making use of anything the class provides feels like abuse. Maybe I could be convinced otherwise if you suggest a docstring to add to the QAPISchemaMember class that explains for which cases it is meant. > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > > index f7f2ca07e4..8909cecde4 100644 > > --- a/scripts/qapi/introspect.py > > +++ b/scripts/qapi/introspect.py > > @@ -188,11 +188,15 @@ const QLitObject %(c_name)s = %(c_string)s; > > self._gen_qlit('[' + element + ']', 'array', {'element-type': element}, > > ifcond) > > > > - def visit_object_type_flat(self, name, info, ifcond, members, variants): > > + def visit_object_type_flat(self, name, info, ifcond, members, variants, > > + features): > > obj = {'members': [self._gen_member(m) for m in members]} > > if variants: > > obj.update(self._gen_variants(variants.tag_member.name, > > variants.variants)) > > + if features: > > + obj['features'] = [ (f['name'], f) for f in features ] > > Compare: > > def visit_enum_type(self, name, info, ifcond, members, prefix): > self._gen_qlit(name, 'enum', > {'values': > [(m.name, {'if': m.ifcond}) for m in members]}, > ifcond) > > Since f is a dict, not an object, we need to use f['name']. > > Since f is a dict, abusing it as the tuple's second element works. > Cleaner: (f['name'], {'if': f['if']}) to_qlit() doesn't document what it expects for obj. From the code, if it's a tuple, the first item is the content to generate and the second item is a dict 'extra' that contains additional information. The 'if' and 'comment' keys in it are used, the rest is silently ignored. f is already a dict that satisfies the expectations of the to_qlit() interface, so it feels unnecessary to duplicate it. But if we really do want to duplicate it, it would look like this: if features: obj['features'] = [ (f['name'], {'if': f['if']} if 'if' in f else {}) for f in features ] Or we could pull the condition one level up: if features: obj['features'] = [ (f['name'], {'if': f['if']}) if 'if' in f else f['name'] for f in features ] I don't consider either way very readable compared to what I currently have, but I'll make the change if you insist on it. Kevin ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] qapi: Support features for structs 2019-05-15 10:58 ` Kevin Wolf @ 2019-05-15 11:22 ` Peter Krempa 2019-05-15 13:48 ` Markus Armbruster 1 sibling, 0 replies; 25+ messages in thread From: Peter Krempa @ 2019-05-15 11:22 UTC (permalink / raw) To: Kevin Wolf; +Cc: Markus Armbruster, qemu-block, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2056 bytes --] On Wed, May 15, 2019 at 12:58:46 +0200, Kevin Wolf wrote: > Am 18.04.2019 um 22:03 hat Markus Armbruster geschrieben: > > Kevin Wolf <kwolf@redhat.com> writes: > > > > > Sometimes, the behaviour of QEMU changes compatibly, but without a > > > change in the QMP syntax (usually by allowing values or operations that > > > previously resulted in an error). QMP clients may still need to know > > > whether the extension is available. > > > > > > This allows to add a list of features to struct definitions that will be > > > made visible to QMP clients through schema introspection. > > > > Only a first step, but that's okay. I think we want to be able to tack > > features to all user-defined types, commands, and events. Ideally even > > to members of enum and object types. > > > > Use case: feature 'deprecated'. We talked about ways to communicate > > deprecation to management applications. Introspection was proposed. > > Feature 'deprecated' delivers it. > > How does introspection solve the problem? It requires the client to > actively look for a deprecation instead of notifying it that it is using > something deprecated. Well, we can actively poll .. > Do you expect libvirt to check a full list of all QMP commands, types, > etc. it ever uses against the schema after starting a VM or something > like that? .. similarly to how we asked to be put on the reviewer list for the deprecated. We an use our test data which we gather from qemu periodically which include the QMP schema to extract the list of deprecated features and store them in git. If we bump the test data and a new deprecated entry appears the developers will need to asses whether it's used or not and take the appropriate action. The appropriate action may also include deriving a capability information from it and use it to alter libvirt's behaviour which would then be queried at startup of the VM, but mostly we want to use the new approach which will replace given deprecated feature as soon as it's available. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] qapi: Support features for structs 2019-05-15 10:58 ` Kevin Wolf 2019-05-15 11:22 ` Peter Krempa @ 2019-05-15 13:48 ` Markus Armbruster 2019-05-17 13:43 ` Peter Krempa 1 sibling, 1 reply; 25+ messages in thread From: Markus Armbruster @ 2019-05-15 13:48 UTC (permalink / raw) To: Kevin Wolf; +Cc: pkrempa, qemu-devel, qemu-block Kevin Wolf <kwolf@redhat.com> writes: > Am 18.04.2019 um 22:03 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Sometimes, the behaviour of QEMU changes compatibly, but without a >> > change in the QMP syntax (usually by allowing values or operations that >> > previously resulted in an error). QMP clients may still need to know >> > whether the extension is available. >> > >> > This allows to add a list of features to struct definitions that will be >> > made visible to QMP clients through schema introspection. >> >> Only a first step, but that's okay. I think we want to be able to tack >> features to all user-defined types, commands, and events. Ideally even >> to members of enum and object types. >> >> Use case: feature 'deprecated'. We talked about ways to communicate >> deprecation to management applications. Introspection was proposed. >> Feature 'deprecated' delivers it. > > How does introspection solve the problem? It requires the client to > actively look for a deprecation instead of notifying it that it is using > something deprecated. > > Do you expect libvirt to check a full list of all QMP commands, types, > etc. it ever uses against the schema after starting a VM or something > like that? I'm merely responding to demand. Subject: Minutes of KVM Forum BoF on deprecating stuff Message-ID: <87mur0ls8o.fsf@dusky.pond.sub.org> * We need to communicate "you're using something that is deprecated". How? Right now, we print a deprecation message. Okay when humans use QEMU directly in a shell. However, when QEMU sits at the bottom of a software stack, the message will likely end up in a log file that is effectively write-only. - The one way to get people read log files is crashing their application. A command line option --future could make QEMU crash right after printing a deprecation message. This could help with finding use of deprecated features in a testing environment. - A less destructive way to grab people's attention is to make things run really, really slow: have QEMU go to sleep for a while after printing a deprecation message. - We can also pass the buck to the next layer up: emit a QMP event. Sadly, by the time the next layer connects to QMP, plenty of stuff already happened. We'd have to buffer deprecation events somehow. What would libvirt do with such an event? Log it, taint the domain, emit a (libvirt) event to pass it on to the next layer up. - A completely different idea is to have a configuratin linter. To support doing this at the libvirt level, QEMU could expose "is deprecated" in interface introspection. Feels feasible for QMP, where we already have sufficiently expressive introspection. For CLI, we'd first have to provide that (but we want that anyway). - We might also want to dispay deprecation messages in QEMU's GUI somehow, or on serial consoles. Second to last item. >> > def check_exprs(exprs): >> > global all_names >> > @@ -986,8 +1007,10 @@ def check_exprs(exprs): >> > normalize_members(expr['data']) >> > elif 'struct' in expr: >> > meta = 'struct' >> > - check_keys(expr_elem, 'struct', ['data'], ['base', 'if']) >> > + check_keys(expr_elem, 'struct', ['data'], >> > + ['base', 'if', 'features']) >> > normalize_members(expr['data']) >> > + normalize_features(expr.get('features')) >> > struct_types[expr[meta]] = expr >> > elif 'command' in expr: >> > meta = 'command' >> > @@ -1126,10 +1149,12 @@ class QAPISchemaVisitor(object): >> > def visit_array_type(self, name, info, ifcond, element_type): >> > pass >> > >> > - def visit_object_type(self, name, info, ifcond, base, members, variants): >> > + def visit_object_type(self, name, info, ifcond, base, members, variants, >> > + features): >> > pass >> > >> > - def visit_object_type_flat(self, name, info, ifcond, members, variants): >> > + def visit_object_type_flat(self, name, info, ifcond, members, variants, >> > + features): >> > pass >> > >> > def visit_alternate_type(self, name, info, ifcond, variants): >> > @@ -1290,7 +1315,7 @@ class QAPISchemaArrayType(QAPISchemaType): >> > >> > class QAPISchemaObjectType(QAPISchemaType): >> > def __init__(self, name, info, doc, ifcond, >> > - base, local_members, variants): >> > + base, local_members, variants, features): >> > # struct has local_members, optional base, and no variants >> > # flat union has base, variants, and no local_members >> > # simple union has local_members, variants, and no base >> > @@ -1307,6 +1332,7 @@ class QAPISchemaObjectType(QAPISchemaType): >> > self.local_members = local_members >> > self.variants = variants >> > self.members = None >> > + self.features = features >> >> I think this should be >> >> self.features = listify_cond(features) > > Not quite, listify_cond() takes the condition it, not the entity that > contains it. But I can do this: > > self.features = features > for f in self.features or []: > f['if'] = listify_cond(f.get('if')) > >> > @@ -1752,9 +1780,10 @@ class QAPISchema(object): >> > base = expr.get('base') >> > data = expr['data'] >> > ifcond = expr.get('if') >> > + features = expr.get('features') >> > self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, base, >> > self._make_members(data, info), >> > - None)) >> > + None, features)) >> >> This is struct. Okay. >> >> For enum, we do: >> >> self._def_entity(QAPISchemaEnumType( >> name, info, doc, ifcond, >> self._make_enum_members(data), prefix)) >> >> where >> >> def _make_enum_members(self, values): >> return [QAPISchemaMember(v['name'], v.get('if')) for v in values] >> >> i.e. we represent the enum members as a list of QAPISchemaMember. >> >> Features remain a list of dicts. >> >> Any particular reason for not making them QAPISchemaMember? > > I don't know what QAPISchemaMember is, and the documentation doesn't > tell me either. Its code looks related to generated C code, but features > don't generate C code. QAPISchemaMember (and all of the QAPISchemaFOO) are not about generating C code. QAPISchema is the intermediate representation. It uses the QAPISchemaFOO to represent schema entities. QAPISchemaMember is what enum members and object members have in common. For Enum members, the common stuff is all there is, and that's why they're represented as QAPISchemaMember. Object members are more complex, and that's why they're represented as QAPISchemaObjectTypeMember, subtype of QAPISchemaMember. QAPISchema provides three services: * Looking up schema identities by name (public methods .lookup_entity(), .lookup_type()). * Semantic checker (public method .check()). Delegates checking the parts to the QAPISchemaFOO. QAPISchemaMember.check_clash() is part of this. * Walking the IR (public method .visit()). Delegates walking parts to the QAPISchemaFOO. QAPISchemaMember has no code for this purpose, because it has no parts to walk. QAPISchema is related to generating C only by .visit() getting used by the code generating C. Some QAPISchemaFOO are additionally related by providing it helpers such as QAPISchemaType.c_type(). QAPISchemaMember provides no such helper. So, QAPISchemaMember serves just two purposes: * Represent an enum member or (the common part of) an object type member Method __init__(). * Check this enum member or (the common part of) this object type member All the other code. What is checked? Naming rules and name clashes with sibling members. The simplest case is actually the one closest to features: rejecting duplicate enum members (as in tests/qapi-schema/enum-clash-member.json). See use of QAPISchemaMember.check_clash() in QAPISchemaEnumType.check(). By the way, that case is missing from your tests :) > I guess I could use it anyway and just never call any of its methods but > just use it to store things in attributes rather than dict entries, but > somehow creating an object and not making use of anything the class > provides feels like abuse. Add the test case, and you'll likely want to use its .check_clash() method :) > Maybe I could be convinced otherwise if you suggest a docstring to add > to the QAPISchemaMember class that explains for which cases it is meant. Yes, scripts/qapi/ is under-documented. Technical debt. We all try to pay it back when we can, and avoid incurring more, but we don't always succeed. >> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py >> > index f7f2ca07e4..8909cecde4 100644 >> > --- a/scripts/qapi/introspect.py >> > +++ b/scripts/qapi/introspect.py >> > @@ -188,11 +188,15 @@ const QLitObject %(c_name)s = %(c_string)s; >> > self._gen_qlit('[' + element + ']', 'array', {'element-type': element}, >> > ifcond) >> > >> > - def visit_object_type_flat(self, name, info, ifcond, members, variants): >> > + def visit_object_type_flat(self, name, info, ifcond, members, variants, >> > + features): >> > obj = {'members': [self._gen_member(m) for m in members]} >> > if variants: >> > obj.update(self._gen_variants(variants.tag_member.name, >> > variants.variants)) >> > + if features: >> > + obj['features'] = [ (f['name'], f) for f in features ] >> >> Compare: >> >> def visit_enum_type(self, name, info, ifcond, members, prefix): >> self._gen_qlit(name, 'enum', >> {'values': >> [(m.name, {'if': m.ifcond}) for m in members]}, >> ifcond) >> >> Since f is a dict, not an object, we need to use f['name']. >> >> Since f is a dict, abusing it as the tuple's second element works. >> Cleaner: (f['name'], {'if': f['if']}) > > to_qlit() doesn't document what it expects for obj. From the code, if > it's a tuple, the first item is the content to generate and the second > item is a dict 'extra' that contains additional information. The 'if' > and 'comment' keys in it are used, the rest is silently ignored. to_qlit()'s tuple case was added in commit d626b6c1ae7 to make it generate #if. qapi-introspect: add preprocessor conditions to generated QLit This commit adds 'ifcond' conditions to top-level QLit objects. Future work will add them to object and enum type members, i.e. within QLit objects. Extend the QLit generator to_qlit() to accept (@obj, @cond) tuples in addition to just @obj. The tuple causes the QLit generated for objects for @obj with #if/#endif conditions for @cond. Simple, but ugly. The tuple's second element was the if condition then. Commit 8c643361eeb turned the tuple's second element into a dict with keys 'if' and 'comment', for generating comments. No other dict keys occur. Still simple, and still ugly. Your patch adds other dict keys. I'd prefer not to do that. Your tacit criticism that code that's this ugly should at least explain itself in comments is valid. > f is already a dict that satisfies the expectations of the to_qlit() > interface, so it feels unnecessary to duplicate it. But if we really > do want to duplicate it, it would look like this: > > if features: > obj['features'] = [ (f['name'], > {'if': f['if']} if 'if' in f else {}) > for f in features ] > > Or we could pull the condition one level up: > > if features: > obj['features'] = [ (f['name'], {'if': f['if']}) > if 'if' in f else f['name'] > for f in features ] > > I don't consider either way very readable compared to what I currently > have, but I'll make the change if you insist on it. Since to_qlit() treats (obj, {'if': None}) (obj, {}) obj the same, the following should work: obj['features'] = [(f['name'], {'if': f.get('if')}) for f in features] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] qapi: Support features for structs 2019-05-15 13:48 ` Markus Armbruster @ 2019-05-17 13:43 ` Peter Krempa 2019-05-17 18:03 ` Markus Armbruster 0 siblings, 1 reply; 25+ messages in thread From: Peter Krempa @ 2019-05-17 13:43 UTC (permalink / raw) To: Markus Armbruster; +Cc: Kevin Wolf, qemu-devel, qemu-block [-- Attachment #1: Type: text/plain, Size: 3968 bytes --] On Wed, May 15, 2019 at 15:48:29 +0200, Markus Armbruster wrote: > Kevin Wolf <kwolf@redhat.com> writes: > > Am 18.04.2019 um 22:03 hat Markus Armbruster geschrieben: > >> Kevin Wolf <kwolf@redhat.com> writes: [...] > > Do you expect libvirt to check a full list of all QMP commands, types, > > etc. it ever uses against the schema after starting a VM or something > > like that? > > I'm merely responding to demand. > > Subject: Minutes of KVM Forum BoF on deprecating stuff > Message-ID: <87mur0ls8o.fsf@dusky.pond.sub.org> > > * We need to communicate "you're using something that is deprecated". > How? Right now, we print a deprecation message. Okay when humans use > QEMU directly in a shell. However, when QEMU sits at the bottom of a > software stack, the message will likely end up in a log file that is > effectively write-only. > > - The one way to get people read log files is crashing their > application. A command line option --future could make QEMU crash > right after printing a deprecation message. This could help with > finding use of deprecated features in a testing environment. > > - A less destructive way to grab people's attention is to make things > run really, really slow: have QEMU go to sleep for a while after > printing a deprecation message. > > - We can also pass the buck to the next layer up: emit a QMP event. > > Sadly, by the time the next layer connects to QMP, plenty of stuff > already happened. We'd have to buffer deprecation events somehow. > > What would libvirt do with such an event? Log it, taint the domain, > emit a (libvirt) event to pass it on to the next layer up. > > - A completely different idea is to have a configuratin linter. To > support doing this at the libvirt level, QEMU could expose "is > deprecated" in interface introspection. Feels feasible for QMP, > where we already have sufficiently expressive introspection. For > CLI, we'd first have to provide that (but we want that anyway). From all of the above, the most important bit is still that the libvirt developers at first identify sooner rather than later that something is deprecated. That way we can either make sure to not use it any longer if there's a compatible replacement or perhaps add the aforementioned linter to print a warning that the config may not be supported in some time. The linter will still require us seeing what is deprecated, so marking that in the schema is useful. There are two dimensions to this though. QMP is the first, where introspection is awesome. Then there is command line and it's various commands which don't have QMP counterparts and that doesn't work that well there. In normal operation there's not much we can do here as refusing to use deprecated attributes or commands would cause regressions. In the test suite we are already validating the output of some of our tests against the QMP schema so making those fail when they are deprecated is certainly possible but not very likely to ever catch something as our QMP tests are exremely basic. It would be much more potent to have something like this to allow validating the command line automatically, but this would require using some structured format first. Without that it's really up to us implementing a check for every unsupported feature as we figure out it's deprecated rather than having it done automatically. Things got way better after we got CC'd on the deprecation document, since we can make sure that we don't use the particular removed thing. There are still a few things that are not addressed which were present before this was happening. E.g. we don't specify the full NUMA topology on the commandline and get an angry message back on the command line. This is going on for almost a year now and nobody complained. Stderr messages are ineffective. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] qapi: Support features for structs 2019-05-17 13:43 ` Peter Krempa @ 2019-05-17 18:03 ` Markus Armbruster 0 siblings, 0 replies; 25+ messages in thread From: Markus Armbruster @ 2019-05-17 18:03 UTC (permalink / raw) To: Peter Krempa; +Cc: Kevin Wolf, qemu-devel, qemu-block Peter Krempa <pkrempa@redhat.com> writes: > On Wed, May 15, 2019 at 15:48:29 +0200, Markus Armbruster wrote: >> Kevin Wolf <kwolf@redhat.com> writes: >> > Am 18.04.2019 um 22:03 hat Markus Armbruster geschrieben: >> >> Kevin Wolf <kwolf@redhat.com> writes: > > [...] > >> > Do you expect libvirt to check a full list of all QMP commands, types, >> > etc. it ever uses against the schema after starting a VM or something >> > like that? >> >> I'm merely responding to demand. >> >> Subject: Minutes of KVM Forum BoF on deprecating stuff >> Message-ID: <87mur0ls8o.fsf@dusky.pond.sub.org> >> >> * We need to communicate "you're using something that is deprecated". >> How? Right now, we print a deprecation message. Okay when humans use >> QEMU directly in a shell. However, when QEMU sits at the bottom of a >> software stack, the message will likely end up in a log file that is >> effectively write-only. >> >> - The one way to get people read log files is crashing their >> application. A command line option --future could make QEMU crash >> right after printing a deprecation message. This could help with >> finding use of deprecated features in a testing environment. >> >> - A less destructive way to grab people's attention is to make things >> run really, really slow: have QEMU go to sleep for a while after >> printing a deprecation message. >> >> - We can also pass the buck to the next layer up: emit a QMP event. >> >> Sadly, by the time the next layer connects to QMP, plenty of stuff >> already happened. We'd have to buffer deprecation events somehow. >> >> What would libvirt do with such an event? Log it, taint the domain, >> emit a (libvirt) event to pass it on to the next layer up. >> >> - A completely different idea is to have a configuratin linter. To >> support doing this at the libvirt level, QEMU could expose "is >> deprecated" in interface introspection. Feels feasible for QMP, >> where we already have sufficiently expressive introspection. For >> CLI, we'd first have to provide that (but we want that anyway). > > From all of the above, the most important bit is still that the libvirt > developers at first identify sooner rather than later that something is > deprecated. Yes, that's the goal. The above lists the ideas we had on how to get closer to the goal. > That way we can either make sure to not use it any longer if > there's a compatible replacement or perhaps add the aforementioned > linter to print a warning that the config may not be supported in some > time. > > The linter will still require us seeing what is deprecated, so marking > that in the schema is useful. There are two dimensions to this though. > QMP is the first, where introspection is awesome. Then there is command > line and it's various commands which don't have QMP counterparts and > that doesn't work that well there. > > In normal operation there's not much we can do here as refusing to use > deprecated attributes or commands would cause regressions. In the test > suite we are already validating the output of some of our tests against > the QMP schema so making those fail when they are deprecated is > certainly possible but not very likely to ever catch something as our > QMP tests are exremely basic. > > It would be much more potent to have something like this to allow > validating the command line automatically, but this would require using > some structured format first. You're asking for command line QAPIfication. > Without that it's really up to us > implementing a check for every unsupported feature as we figure out it's > deprecated rather than having it done automatically. > > Things got way better after we got CC'd on the deprecation document, > since we can make sure that we don't use the particular removed thing. > > There are still a few things that are not addressed which were present > before this was happening. E.g. we don't specify the full NUMA topology > on the commandline and get an angry message back on the command line. > This is going on for almost a year now and nobody complained. Stderr > messages are ineffective. I figure they work as well as anything when a human runs QEMU directly. They're ineffective when a management application runs QEMU. It won't be able to make sense of deprecation messages it doesn't already know (and therefore doesn't actually need), so all it can do is bury them in logs nobody reads until after things have gone south. Exposing deprecation in introspection is just one idea of several. It might be the least immediately useful one. We'll likely want to do more. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 2/4] tests/qapi-schema: Test for good feature lists in structs 2019-04-08 14:35 [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature Kevin Wolf 2019-04-08 14:35 ` Kevin Wolf 2019-04-08 14:35 ` [Qemu-devel] [PATCH 1/4] qapi: Support features for structs Kevin Wolf @ 2019-04-08 14:35 ` Kevin Wolf 2019-04-08 14:35 ` Kevin Wolf 2019-04-08 14:35 ` [Qemu-devel] [PATCH 3/4] tests/qapi-schema: Error case tests for features " Kevin Wolf ` (4 subsequent siblings) 7 siblings, 1 reply; 25+ messages in thread From: Kevin Wolf @ 2019-04-08 14:35 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, armbru, eblake, pkrempa, qemu-devel Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qapi-schema/qapi-schema-test.json | 27 +++++++++++++++++++++++++ tests/qapi-schema/qapi-schema-test.out | 26 ++++++++++++++++++++++++ tests/qapi-schema/test-qapi.py | 4 ++++ 3 files changed, 57 insertions(+) diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 0952c68734..263514b448 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -242,3 +242,30 @@ { 'foo': 'TestIfStruct', 'bar': { 'type': ['TestIfEnum'], 'if': 'defined(TEST_IF_EVT_BAR)' } }, 'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' } + +# test 'features' for structs + +{ 'struct': 'FeatureStruct0', + 'data': { 'foo': 'int' }, + 'features': [] } +{ 'struct': 'FeatureStruct1', + 'data': { 'foo': 'int' }, + 'features': [ 'feature1' ] } +{ 'struct': 'FeatureStruct2', + 'data': { 'foo': 'int' }, + 'features': [ { 'name': 'feature1' } ] } +{ 'struct': 'FeatureStruct3', + 'data': { 'foo': 'int' }, + 'features': [ 'feature1', 'feature2' ] } + +{ 'struct': 'CondFeatureStruct1', + 'data': { 'foo': 'int' }, + 'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'} ] } +{ 'struct': 'CondFeatureStruct2', + 'data': { 'foo': 'int' }, + 'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'}, + { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] } +{ 'struct': 'CondFeatureStruct3', + 'data': { 'foo': 'int' }, + 'features': [ { 'name': 'feature1', 'if': [ 'defined(TEST_IF_COND_1)', + 'defined(TEST_IF_COND_2)'] } ] } diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 77fb1e1aa9..490e202f65 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -354,3 +354,29 @@ object q_obj_TestIfEvent-arg event TestIfEvent q_obj_TestIfEvent-arg boxed=False if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)'] +object FeatureStruct0 + member foo: int optional=False +object FeatureStruct1 + member foo: int optional=False + feature feature1 +object FeatureStruct2 + member foo: int optional=False + feature feature1 +object FeatureStruct3 + member foo: int optional=False + feature feature1 + feature feature2 +object CondFeatureStruct1 + member foo: int optional=False + feature feature1 + if ['defined(TEST_IF_FEATURE_1)'] +object CondFeatureStruct2 + member foo: int optional=False + feature feature1 + if ['defined(TEST_IF_FEATURE_1)'] + feature feature2 + if ['defined(TEST_IF_FEATURE_2)'] +object CondFeatureStruct3 + member foo: int optional=False + feature feature1 + if ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'] diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py index f2d6815c86..08e29c23a6 100644 --- a/tests/qapi-schema/test-qapi.py +++ b/tests/qapi-schema/test-qapi.py @@ -49,6 +49,10 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): self._print_if(m.ifcond, 8) self._print_variants(variants) self._print_if(ifcond) + if features: + for f in features: + print(' feature %s' % f['name']) + self._print_if(f.get('if'), 8) def visit_alternate_type(self, name, info, ifcond, variants): print('alternate %s' % name) -- 2.20.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 2/4] tests/qapi-schema: Test for good feature lists in structs 2019-04-08 14:35 ` [Qemu-devel] [PATCH 2/4] tests/qapi-schema: Test for good feature lists in structs Kevin Wolf @ 2019-04-08 14:35 ` Kevin Wolf 0 siblings, 0 replies; 25+ messages in thread From: Kevin Wolf @ 2019-04-08 14:35 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, pkrempa, armbru, qemu-devel Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qapi-schema/qapi-schema-test.json | 27 +++++++++++++++++++++++++ tests/qapi-schema/qapi-schema-test.out | 26 ++++++++++++++++++++++++ tests/qapi-schema/test-qapi.py | 4 ++++ 3 files changed, 57 insertions(+) diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 0952c68734..263514b448 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -242,3 +242,30 @@ { 'foo': 'TestIfStruct', 'bar': { 'type': ['TestIfEnum'], 'if': 'defined(TEST_IF_EVT_BAR)' } }, 'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' } + +# test 'features' for structs + +{ 'struct': 'FeatureStruct0', + 'data': { 'foo': 'int' }, + 'features': [] } +{ 'struct': 'FeatureStruct1', + 'data': { 'foo': 'int' }, + 'features': [ 'feature1' ] } +{ 'struct': 'FeatureStruct2', + 'data': { 'foo': 'int' }, + 'features': [ { 'name': 'feature1' } ] } +{ 'struct': 'FeatureStruct3', + 'data': { 'foo': 'int' }, + 'features': [ 'feature1', 'feature2' ] } + +{ 'struct': 'CondFeatureStruct1', + 'data': { 'foo': 'int' }, + 'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'} ] } +{ 'struct': 'CondFeatureStruct2', + 'data': { 'foo': 'int' }, + 'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'}, + { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] } +{ 'struct': 'CondFeatureStruct3', + 'data': { 'foo': 'int' }, + 'features': [ { 'name': 'feature1', 'if': [ 'defined(TEST_IF_COND_1)', + 'defined(TEST_IF_COND_2)'] } ] } diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 77fb1e1aa9..490e202f65 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -354,3 +354,29 @@ object q_obj_TestIfEvent-arg event TestIfEvent q_obj_TestIfEvent-arg boxed=False if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)'] +object FeatureStruct0 + member foo: int optional=False +object FeatureStruct1 + member foo: int optional=False + feature feature1 +object FeatureStruct2 + member foo: int optional=False + feature feature1 +object FeatureStruct3 + member foo: int optional=False + feature feature1 + feature feature2 +object CondFeatureStruct1 + member foo: int optional=False + feature feature1 + if ['defined(TEST_IF_FEATURE_1)'] +object CondFeatureStruct2 + member foo: int optional=False + feature feature1 + if ['defined(TEST_IF_FEATURE_1)'] + feature feature2 + if ['defined(TEST_IF_FEATURE_2)'] +object CondFeatureStruct3 + member foo: int optional=False + feature feature1 + if ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'] diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py index f2d6815c86..08e29c23a6 100644 --- a/tests/qapi-schema/test-qapi.py +++ b/tests/qapi-schema/test-qapi.py @@ -49,6 +49,10 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): self._print_if(m.ifcond, 8) self._print_variants(variants) self._print_if(ifcond) + if features: + for f in features: + print(' feature %s' % f['name']) + self._print_if(f.get('if'), 8) def visit_alternate_type(self, name, info, ifcond, variants): print('alternate %s' % name) -- 2.20.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 3/4] tests/qapi-schema: Error case tests for features in structs 2019-04-08 14:35 [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature Kevin Wolf ` (2 preceding siblings ...) 2019-04-08 14:35 ` [Qemu-devel] [PATCH 2/4] tests/qapi-schema: Test for good feature lists in structs Kevin Wolf @ 2019-04-08 14:35 ` Kevin Wolf 2019-04-08 14:35 ` Kevin Wolf 2019-04-08 14:35 ` [Qemu-devel] [PATCH 4/4] file-posix: Add dynamic-auto-read-only QAPI feature Kevin Wolf ` (3 subsequent siblings) 7 siblings, 1 reply; 25+ messages in thread From: Kevin Wolf @ 2019-04-08 14:35 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, armbru, eblake, pkrempa, qemu-devel Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qapi-schema/features-bad-type.json | 3 +++ tests/qapi-schema/features-missing-name.json | 3 +++ tests/qapi-schema/features-name-bad-type.json | 3 +++ tests/qapi-schema/features-no-list.json | 3 +++ tests/qapi-schema/features-unknown-key.json | 3 +++ tests/Makefile.include | 5 +++++ tests/qapi-schema/features-bad-type.err | 1 + tests/qapi-schema/features-bad-type.exit | 1 + tests/qapi-schema/features-bad-type.out | 0 tests/qapi-schema/features-missing-name.err | 1 + tests/qapi-schema/features-missing-name.exit | 1 + tests/qapi-schema/features-missing-name.out | 0 tests/qapi-schema/features-name-bad-type.err | 1 + tests/qapi-schema/features-name-bad-type.exit | 1 + tests/qapi-schema/features-name-bad-type.out | 0 tests/qapi-schema/features-no-list.err | 1 + tests/qapi-schema/features-no-list.exit | 1 + tests/qapi-schema/features-no-list.out | 0 tests/qapi-schema/features-unknown-key.err | 2 ++ tests/qapi-schema/features-unknown-key.exit | 1 + tests/qapi-schema/features-unknown-key.out | 0 21 files changed, 31 insertions(+) create mode 100644 tests/qapi-schema/features-bad-type.json create mode 100644 tests/qapi-schema/features-missing-name.json create mode 100644 tests/qapi-schema/features-name-bad-type.json create mode 100644 tests/qapi-schema/features-no-list.json create mode 100644 tests/qapi-schema/features-unknown-key.json create mode 100644 tests/qapi-schema/features-bad-type.err create mode 100644 tests/qapi-schema/features-bad-type.exit create mode 100644 tests/qapi-schema/features-bad-type.out create mode 100644 tests/qapi-schema/features-missing-name.err create mode 100644 tests/qapi-schema/features-missing-name.exit create mode 100644 tests/qapi-schema/features-missing-name.out create mode 100644 tests/qapi-schema/features-name-bad-type.err create mode 100644 tests/qapi-schema/features-name-bad-type.exit create mode 100644 tests/qapi-schema/features-name-bad-type.out create mode 100644 tests/qapi-schema/features-no-list.err create mode 100644 tests/qapi-schema/features-no-list.exit create mode 100644 tests/qapi-schema/features-no-list.out create mode 100644 tests/qapi-schema/features-unknown-key.err create mode 100644 tests/qapi-schema/features-unknown-key.exit create mode 100644 tests/qapi-schema/features-unknown-key.out diff --git a/tests/qapi-schema/features-bad-type.json b/tests/qapi-schema/features-bad-type.json new file mode 100644 index 0000000000..57db5540e7 --- /dev/null +++ b/tests/qapi-schema/features-bad-type.json @@ -0,0 +1,3 @@ +{ 'struct': 'FeatureStruct0', + 'data': { 'foo': 'int' }, + 'features': [ [ 'a feature cannot be an array' ] ] } diff --git a/tests/qapi-schema/features-missing-name.json b/tests/qapi-schema/features-missing-name.json new file mode 100644 index 0000000000..2314f97c00 --- /dev/null +++ b/tests/qapi-schema/features-missing-name.json @@ -0,0 +1,3 @@ +{ 'struct': 'FeatureStruct0', + 'data': { 'foo': 'int' }, + 'features': [ { 'if': 'defined(NAMELESS_FEATURES)' } ] } diff --git a/tests/qapi-schema/features-name-bad-type.json b/tests/qapi-schema/features-name-bad-type.json new file mode 100644 index 0000000000..b07139978a --- /dev/null +++ b/tests/qapi-schema/features-name-bad-type.json @@ -0,0 +1,3 @@ +{ 'struct': 'FeatureStruct0', + 'data': { 'foo': 'int' }, + 'features': [ { 'name': { 'feature-type': 'object' } } ] } diff --git a/tests/qapi-schema/features-no-list.json b/tests/qapi-schema/features-no-list.json new file mode 100644 index 0000000000..9484fd94fc --- /dev/null +++ b/tests/qapi-schema/features-no-list.json @@ -0,0 +1,3 @@ +{ 'struct': 'FeatureStruct0', + 'data': { 'foo': 'int' }, + 'features': 'bar' } diff --git a/tests/qapi-schema/features-unknown-key.json b/tests/qapi-schema/features-unknown-key.json new file mode 100644 index 0000000000..134df3b503 --- /dev/null +++ b/tests/qapi-schema/features-unknown-key.json @@ -0,0 +1,3 @@ +{ 'struct': 'FeatureStruct0', + 'data': { 'foo': 'int' }, + 'features': [ { 'name': 'bar', 'colour': 'red' } ] } diff --git a/tests/Makefile.include b/tests/Makefile.include index 6b904d7430..d116ad770c 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -377,6 +377,11 @@ qapi-schema += event-boxed-empty.json qapi-schema += event-case.json qapi-schema += event-member-invalid-dict.json qapi-schema += event-nest-struct.json +qapi-schema += features-bad-type.json +qapi-schema += features-missing-name.json +qapi-schema += features-name-bad-type.json +qapi-schema += features-no-list.json +qapi-schema += features-unknown-key.json qapi-schema += flat-union-array-branch.json qapi-schema += flat-union-bad-base.json qapi-schema += flat-union-bad-discriminator.json diff --git a/tests/qapi-schema/features-bad-type.err b/tests/qapi-schema/features-bad-type.err new file mode 100644 index 0000000000..7ca7a08ab0 --- /dev/null +++ b/tests/qapi-schema/features-bad-type.err @@ -0,0 +1 @@ +tests/qapi-schema/features-bad-type.json:1: Feature names for struct 'FeatureStruct0' must be strings diff --git a/tests/qapi-schema/features-bad-type.exit b/tests/qapi-schema/features-bad-type.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/features-bad-type.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/features-bad-type.out b/tests/qapi-schema/features-bad-type.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/features-missing-name.err b/tests/qapi-schema/features-missing-name.err new file mode 100644 index 0000000000..4f1d2715aa --- /dev/null +++ b/tests/qapi-schema/features-missing-name.err @@ -0,0 +1 @@ +tests/qapi-schema/features-missing-name.json:1: Key 'name' is missing from feature of struct FeatureStruct0 diff --git a/tests/qapi-schema/features-missing-name.exit b/tests/qapi-schema/features-missing-name.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/features-missing-name.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/features-missing-name.out b/tests/qapi-schema/features-missing-name.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/features-name-bad-type.err b/tests/qapi-schema/features-name-bad-type.err new file mode 100644 index 0000000000..8a68d7cb73 --- /dev/null +++ b/tests/qapi-schema/features-name-bad-type.err @@ -0,0 +1 @@ +tests/qapi-schema/features-name-bad-type.json:1: Feature names for struct 'FeatureStruct0' must be strings diff --git a/tests/qapi-schema/features-name-bad-type.exit b/tests/qapi-schema/features-name-bad-type.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/features-name-bad-type.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/features-name-bad-type.out b/tests/qapi-schema/features-name-bad-type.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/features-no-list.err b/tests/qapi-schema/features-no-list.err new file mode 100644 index 0000000000..61ed68612b --- /dev/null +++ b/tests/qapi-schema/features-no-list.err @@ -0,0 +1 @@ +tests/qapi-schema/features-no-list.json:1: Struct 'FeatureStruct0' requires an array for 'features' diff --git a/tests/qapi-schema/features-no-list.exit b/tests/qapi-schema/features-no-list.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/features-no-list.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/features-no-list.out b/tests/qapi-schema/features-no-list.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/features-unknown-key.err b/tests/qapi-schema/features-unknown-key.err new file mode 100644 index 0000000000..a1d693030d --- /dev/null +++ b/tests/qapi-schema/features-unknown-key.err @@ -0,0 +1,2 @@ +tests/qapi-schema/features-unknown-key.json:1: Unknown key 'colour' in feature of struct FeatureStruct0 +Valid keys are 'if', 'name'. diff --git a/tests/qapi-schema/features-unknown-key.exit b/tests/qapi-schema/features-unknown-key.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/features-unknown-key.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/features-unknown-key.out b/tests/qapi-schema/features-unknown-key.out new file mode 100644 index 0000000000..e69de29bb2 -- 2.20.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 3/4] tests/qapi-schema: Error case tests for features in structs 2019-04-08 14:35 ` [Qemu-devel] [PATCH 3/4] tests/qapi-schema: Error case tests for features " Kevin Wolf @ 2019-04-08 14:35 ` Kevin Wolf 0 siblings, 0 replies; 25+ messages in thread From: Kevin Wolf @ 2019-04-08 14:35 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, pkrempa, armbru, qemu-devel Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qapi-schema/features-bad-type.json | 3 +++ tests/qapi-schema/features-missing-name.json | 3 +++ tests/qapi-schema/features-name-bad-type.json | 3 +++ tests/qapi-schema/features-no-list.json | 3 +++ tests/qapi-schema/features-unknown-key.json | 3 +++ tests/Makefile.include | 5 +++++ tests/qapi-schema/features-bad-type.err | 1 + tests/qapi-schema/features-bad-type.exit | 1 + tests/qapi-schema/features-bad-type.out | 0 tests/qapi-schema/features-missing-name.err | 1 + tests/qapi-schema/features-missing-name.exit | 1 + tests/qapi-schema/features-missing-name.out | 0 tests/qapi-schema/features-name-bad-type.err | 1 + tests/qapi-schema/features-name-bad-type.exit | 1 + tests/qapi-schema/features-name-bad-type.out | 0 tests/qapi-schema/features-no-list.err | 1 + tests/qapi-schema/features-no-list.exit | 1 + tests/qapi-schema/features-no-list.out | 0 tests/qapi-schema/features-unknown-key.err | 2 ++ tests/qapi-schema/features-unknown-key.exit | 1 + tests/qapi-schema/features-unknown-key.out | 0 21 files changed, 31 insertions(+) create mode 100644 tests/qapi-schema/features-bad-type.json create mode 100644 tests/qapi-schema/features-missing-name.json create mode 100644 tests/qapi-schema/features-name-bad-type.json create mode 100644 tests/qapi-schema/features-no-list.json create mode 100644 tests/qapi-schema/features-unknown-key.json create mode 100644 tests/qapi-schema/features-bad-type.err create mode 100644 tests/qapi-schema/features-bad-type.exit create mode 100644 tests/qapi-schema/features-bad-type.out create mode 100644 tests/qapi-schema/features-missing-name.err create mode 100644 tests/qapi-schema/features-missing-name.exit create mode 100644 tests/qapi-schema/features-missing-name.out create mode 100644 tests/qapi-schema/features-name-bad-type.err create mode 100644 tests/qapi-schema/features-name-bad-type.exit create mode 100644 tests/qapi-schema/features-name-bad-type.out create mode 100644 tests/qapi-schema/features-no-list.err create mode 100644 tests/qapi-schema/features-no-list.exit create mode 100644 tests/qapi-schema/features-no-list.out create mode 100644 tests/qapi-schema/features-unknown-key.err create mode 100644 tests/qapi-schema/features-unknown-key.exit create mode 100644 tests/qapi-schema/features-unknown-key.out diff --git a/tests/qapi-schema/features-bad-type.json b/tests/qapi-schema/features-bad-type.json new file mode 100644 index 0000000000..57db5540e7 --- /dev/null +++ b/tests/qapi-schema/features-bad-type.json @@ -0,0 +1,3 @@ +{ 'struct': 'FeatureStruct0', + 'data': { 'foo': 'int' }, + 'features': [ [ 'a feature cannot be an array' ] ] } diff --git a/tests/qapi-schema/features-missing-name.json b/tests/qapi-schema/features-missing-name.json new file mode 100644 index 0000000000..2314f97c00 --- /dev/null +++ b/tests/qapi-schema/features-missing-name.json @@ -0,0 +1,3 @@ +{ 'struct': 'FeatureStruct0', + 'data': { 'foo': 'int' }, + 'features': [ { 'if': 'defined(NAMELESS_FEATURES)' } ] } diff --git a/tests/qapi-schema/features-name-bad-type.json b/tests/qapi-schema/features-name-bad-type.json new file mode 100644 index 0000000000..b07139978a --- /dev/null +++ b/tests/qapi-schema/features-name-bad-type.json @@ -0,0 +1,3 @@ +{ 'struct': 'FeatureStruct0', + 'data': { 'foo': 'int' }, + 'features': [ { 'name': { 'feature-type': 'object' } } ] } diff --git a/tests/qapi-schema/features-no-list.json b/tests/qapi-schema/features-no-list.json new file mode 100644 index 0000000000..9484fd94fc --- /dev/null +++ b/tests/qapi-schema/features-no-list.json @@ -0,0 +1,3 @@ +{ 'struct': 'FeatureStruct0', + 'data': { 'foo': 'int' }, + 'features': 'bar' } diff --git a/tests/qapi-schema/features-unknown-key.json b/tests/qapi-schema/features-unknown-key.json new file mode 100644 index 0000000000..134df3b503 --- /dev/null +++ b/tests/qapi-schema/features-unknown-key.json @@ -0,0 +1,3 @@ +{ 'struct': 'FeatureStruct0', + 'data': { 'foo': 'int' }, + 'features': [ { 'name': 'bar', 'colour': 'red' } ] } diff --git a/tests/Makefile.include b/tests/Makefile.include index 6b904d7430..d116ad770c 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -377,6 +377,11 @@ qapi-schema += event-boxed-empty.json qapi-schema += event-case.json qapi-schema += event-member-invalid-dict.json qapi-schema += event-nest-struct.json +qapi-schema += features-bad-type.json +qapi-schema += features-missing-name.json +qapi-schema += features-name-bad-type.json +qapi-schema += features-no-list.json +qapi-schema += features-unknown-key.json qapi-schema += flat-union-array-branch.json qapi-schema += flat-union-bad-base.json qapi-schema += flat-union-bad-discriminator.json diff --git a/tests/qapi-schema/features-bad-type.err b/tests/qapi-schema/features-bad-type.err new file mode 100644 index 0000000000..7ca7a08ab0 --- /dev/null +++ b/tests/qapi-schema/features-bad-type.err @@ -0,0 +1 @@ +tests/qapi-schema/features-bad-type.json:1: Feature names for struct 'FeatureStruct0' must be strings diff --git a/tests/qapi-schema/features-bad-type.exit b/tests/qapi-schema/features-bad-type.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/features-bad-type.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/features-bad-type.out b/tests/qapi-schema/features-bad-type.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/features-missing-name.err b/tests/qapi-schema/features-missing-name.err new file mode 100644 index 0000000000..4f1d2715aa --- /dev/null +++ b/tests/qapi-schema/features-missing-name.err @@ -0,0 +1 @@ +tests/qapi-schema/features-missing-name.json:1: Key 'name' is missing from feature of struct FeatureStruct0 diff --git a/tests/qapi-schema/features-missing-name.exit b/tests/qapi-schema/features-missing-name.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/features-missing-name.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/features-missing-name.out b/tests/qapi-schema/features-missing-name.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/features-name-bad-type.err b/tests/qapi-schema/features-name-bad-type.err new file mode 100644 index 0000000000..8a68d7cb73 --- /dev/null +++ b/tests/qapi-schema/features-name-bad-type.err @@ -0,0 +1 @@ +tests/qapi-schema/features-name-bad-type.json:1: Feature names for struct 'FeatureStruct0' must be strings diff --git a/tests/qapi-schema/features-name-bad-type.exit b/tests/qapi-schema/features-name-bad-type.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/features-name-bad-type.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/features-name-bad-type.out b/tests/qapi-schema/features-name-bad-type.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/features-no-list.err b/tests/qapi-schema/features-no-list.err new file mode 100644 index 0000000000..61ed68612b --- /dev/null +++ b/tests/qapi-schema/features-no-list.err @@ -0,0 +1 @@ +tests/qapi-schema/features-no-list.json:1: Struct 'FeatureStruct0' requires an array for 'features' diff --git a/tests/qapi-schema/features-no-list.exit b/tests/qapi-schema/features-no-list.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/features-no-list.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/features-no-list.out b/tests/qapi-schema/features-no-list.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/features-unknown-key.err b/tests/qapi-schema/features-unknown-key.err new file mode 100644 index 0000000000..a1d693030d --- /dev/null +++ b/tests/qapi-schema/features-unknown-key.err @@ -0,0 +1,2 @@ +tests/qapi-schema/features-unknown-key.json:1: Unknown key 'colour' in feature of struct FeatureStruct0 +Valid keys are 'if', 'name'. diff --git a/tests/qapi-schema/features-unknown-key.exit b/tests/qapi-schema/features-unknown-key.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/features-unknown-key.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/features-unknown-key.out b/tests/qapi-schema/features-unknown-key.out new file mode 100644 index 0000000000..e69de29bb2 -- 2.20.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 4/4] file-posix: Add dynamic-auto-read-only QAPI feature 2019-04-08 14:35 [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature Kevin Wolf ` (3 preceding siblings ...) 2019-04-08 14:35 ` [Qemu-devel] [PATCH 3/4] tests/qapi-schema: Error case tests for features " Kevin Wolf @ 2019-04-08 14:35 ` Kevin Wolf 2019-04-08 14:35 ` Kevin Wolf 2019-04-18 20:13 ` Markus Armbruster 2019-04-08 16:25 ` [Qemu-devel] [PATCH 0/4] " Peter Krempa ` (2 subsequent siblings) 7 siblings, 2 replies; 25+ messages in thread From: Kevin Wolf @ 2019-04-08 14:35 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, armbru, eblake, pkrempa, qemu-devel In commit 23dece19da4 ('file-posix: Make auto-read-only dynamic') , auto-read-only=on changed its behaviour in file-posix for the 4.0 release. This change cannot be detected through the usual mechanisms like schema introspection. Add a new feature flag to the schema to allow libvirt to detect the presence of the new behaviour. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index 7ccbfff9d0..4f96d5846b 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2843,6 +2843,13 @@ # file is large, do not use in production. # (default: off) (since: 3.0) # +# If the feature 'dynamic-auto-read-only' is present, enabled auto-read-only +# means that the driver will open the image read-only at first, dynamically +# reopen the image file read-write when the first writer is attached to the +# node and reopen read-only when the last writer is detached. This allows to +# give QEMU write permissions only on demand when an operation actually needs +# write access. +# # Since: 2.9 ## { 'struct': 'BlockdevOptionsFile', @@ -2852,7 +2859,9 @@ '*aio': 'BlockdevAioOptions', '*drop-cache': {'type': 'bool', 'if': 'defined(CONFIG_LINUX)'}, - '*x-check-cache-dropped': 'bool' } } + '*x-check-cache-dropped': 'bool' }, + 'features': [ { 'name': 'dynamic-auto-read-only', + 'if': 'defined(CONFIG_POSIX)' } ] } ## # @BlockdevOptionsNull: -- 2.20.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 4/4] file-posix: Add dynamic-auto-read-only QAPI feature 2019-04-08 14:35 ` [Qemu-devel] [PATCH 4/4] file-posix: Add dynamic-auto-read-only QAPI feature Kevin Wolf @ 2019-04-08 14:35 ` Kevin Wolf 2019-04-18 20:13 ` Markus Armbruster 1 sibling, 0 replies; 25+ messages in thread From: Kevin Wolf @ 2019-04-08 14:35 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, pkrempa, armbru, qemu-devel In commit 23dece19da4 ('file-posix: Make auto-read-only dynamic') , auto-read-only=on changed its behaviour in file-posix for the 4.0 release. This change cannot be detected through the usual mechanisms like schema introspection. Add a new feature flag to the schema to allow libvirt to detect the presence of the new behaviour. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index 7ccbfff9d0..4f96d5846b 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2843,6 +2843,13 @@ # file is large, do not use in production. # (default: off) (since: 3.0) # +# If the feature 'dynamic-auto-read-only' is present, enabled auto-read-only +# means that the driver will open the image read-only at first, dynamically +# reopen the image file read-write when the first writer is attached to the +# node and reopen read-only when the last writer is detached. This allows to +# give QEMU write permissions only on demand when an operation actually needs +# write access. +# # Since: 2.9 ## { 'struct': 'BlockdevOptionsFile', @@ -2852,7 +2859,9 @@ '*aio': 'BlockdevAioOptions', '*drop-cache': {'type': 'bool', 'if': 'defined(CONFIG_LINUX)'}, - '*x-check-cache-dropped': 'bool' } } + '*x-check-cache-dropped': 'bool' }, + 'features': [ { 'name': 'dynamic-auto-read-only', + 'if': 'defined(CONFIG_POSIX)' } ] } ## # @BlockdevOptionsNull: -- 2.20.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] file-posix: Add dynamic-auto-read-only QAPI feature 2019-04-08 14:35 ` [Qemu-devel] [PATCH 4/4] file-posix: Add dynamic-auto-read-only QAPI feature Kevin Wolf 2019-04-08 14:35 ` Kevin Wolf @ 2019-04-18 20:13 ` Markus Armbruster 2019-04-18 20:13 ` Markus Armbruster 1 sibling, 1 reply; 25+ messages in thread From: Markus Armbruster @ 2019-04-18 20:13 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-block, pkrempa, qemu-devel Kevin Wolf <kwolf@redhat.com> writes: > In commit 23dece19da4 ('file-posix: Make auto-read-only dynamic') , > auto-read-only=on changed its behaviour in file-posix for the 4.0 > release. This change cannot be detected through the usual mechanisms > like schema introspection. Add a new feature flag to the schema to > allow libvirt to detect the presence of the new behaviour. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > qapi/block-core.json | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 7ccbfff9d0..4f96d5846b 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -2843,6 +2843,13 @@ ## # @BlockdevOptionsFile: # # Driver specific block device options for the file backend. # # @filename: path to the image file # @pr-manager: the id for the object that will handle persistent reservations # for this device (default: none, forward the commands via SG_IO; # since 2.11) # @aio: AIO backend (default: threads) (since: 2.8) # @locking: whether to enable file locking. If set to 'auto', only enable # when Open File Descriptor (OFD) locking API is available # (default: auto, since 2.10) # @drop-cache: invalidate page cache during live migration. This prevents # stale data on the migration destination with cache.direct=off. # Currently only supported on Linux hosts. # (default: on, since: 4.0) # @x-check-cache-dropped: whether to check that page cache was dropped on live # migration. May cause noticeable delays if the image > # file is large, do not use in production. > # (default: off) (since: 3.0) > # > +# If the feature 'dynamic-auto-read-only' is present, enabled auto-read-only > +# means that the driver will open the image read-only at first, dynamically > +# reopen the image file read-write when the first writer is attached to the > +# node and reopen read-only when the last writer is detached. This allows to > +# give QEMU write permissions only on demand when an operation actually needs > +# write access. > +# > # Since: 2.9 > ## > { 'struct': 'BlockdevOptionsFile', > @@ -2852,7 +2859,9 @@ > '*aio': 'BlockdevAioOptions', > '*drop-cache': {'type': 'bool', > 'if': 'defined(CONFIG_LINUX)'}, > - '*x-check-cache-dropped': 'bool' } } > + '*x-check-cache-dropped': 'bool' }, > + 'features': [ { 'name': 'dynamic-auto-read-only', > + 'if': 'defined(CONFIG_POSIX)' } ] } > > ## > # @BlockdevOptionsNull: This uses free-form text to document features. That means the doc generator can't flag missing documentation, like it could for members (it doesn't only because we still got doc work to do there). A more structured approach could look like this: ## # @BlockdevOptionsFile: # # Driver specific block device options for the file backend. # # @filename: path to the image file [More members...] # # Features: # @dynamic-read-only: if present, enabled auto-read-only ... # # Since: 2.9 ## ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] file-posix: Add dynamic-auto-read-only QAPI feature 2019-04-18 20:13 ` Markus Armbruster @ 2019-04-18 20:13 ` Markus Armbruster 0 siblings, 0 replies; 25+ messages in thread From: Markus Armbruster @ 2019-04-18 20:13 UTC (permalink / raw) To: Kevin Wolf; +Cc: pkrempa, qemu-devel, qemu-block Kevin Wolf <kwolf@redhat.com> writes: > In commit 23dece19da4 ('file-posix: Make auto-read-only dynamic') , > auto-read-only=on changed its behaviour in file-posix for the 4.0 > release. This change cannot be detected through the usual mechanisms > like schema introspection. Add a new feature flag to the schema to > allow libvirt to detect the presence of the new behaviour. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > qapi/block-core.json | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 7ccbfff9d0..4f96d5846b 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -2843,6 +2843,13 @@ ## # @BlockdevOptionsFile: # # Driver specific block device options for the file backend. # # @filename: path to the image file # @pr-manager: the id for the object that will handle persistent reservations # for this device (default: none, forward the commands via SG_IO; # since 2.11) # @aio: AIO backend (default: threads) (since: 2.8) # @locking: whether to enable file locking. If set to 'auto', only enable # when Open File Descriptor (OFD) locking API is available # (default: auto, since 2.10) # @drop-cache: invalidate page cache during live migration. This prevents # stale data on the migration destination with cache.direct=off. # Currently only supported on Linux hosts. # (default: on, since: 4.0) # @x-check-cache-dropped: whether to check that page cache was dropped on live # migration. May cause noticeable delays if the image > # file is large, do not use in production. > # (default: off) (since: 3.0) > # > +# If the feature 'dynamic-auto-read-only' is present, enabled auto-read-only > +# means that the driver will open the image read-only at first, dynamically > +# reopen the image file read-write when the first writer is attached to the > +# node and reopen read-only when the last writer is detached. This allows to > +# give QEMU write permissions only on demand when an operation actually needs > +# write access. > +# > # Since: 2.9 > ## > { 'struct': 'BlockdevOptionsFile', > @@ -2852,7 +2859,9 @@ > '*aio': 'BlockdevAioOptions', > '*drop-cache': {'type': 'bool', > 'if': 'defined(CONFIG_LINUX)'}, > - '*x-check-cache-dropped': 'bool' } } > + '*x-check-cache-dropped': 'bool' }, > + 'features': [ { 'name': 'dynamic-auto-read-only', > + 'if': 'defined(CONFIG_POSIX)' } ] } > > ## > # @BlockdevOptionsNull: This uses free-form text to document features. That means the doc generator can't flag missing documentation, like it could for members (it doesn't only because we still got doc work to do there). A more structured approach could look like this: ## # @BlockdevOptionsFile: # # Driver specific block device options for the file backend. # # @filename: path to the image file [More members...] # # Features: # @dynamic-read-only: if present, enabled auto-read-only ... # # Since: 2.9 ## ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature 2019-04-08 14:35 [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature Kevin Wolf ` (4 preceding siblings ...) 2019-04-08 14:35 ` [Qemu-devel] [PATCH 4/4] file-posix: Add dynamic-auto-read-only QAPI feature Kevin Wolf @ 2019-04-08 16:25 ` Peter Krempa 2019-04-08 16:25 ` Peter Krempa 2019-04-18 7:22 ` Kevin Wolf 2019-04-18 20:18 ` Markus Armbruster 7 siblings, 1 reply; 25+ messages in thread From: Peter Krempa @ 2019-04-08 16:25 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-block, armbru, eblake, qemu-devel [-- Attachment #1: Type: text/plain, Size: 688 bytes --] On Mon, Apr 08, 2019 at 16:35:39 +0200, Kevin Wolf wrote: > This series adds optional feature lists to struct definitions in the > QAPI schema and makes use of them to advertise the new behaviour of > auto-read-only=on in file-posix. > > Kevin Wolf (4): > qapi: Support features for structs > tests/qapi-schema: Test for good feature lists in structs > tests/qapi-schema: Error case tests for features in structs > file-posix: Add dynamic-auto-read-only QAPI feature I was able to implement this in libvirt fairly easy. My work-in-progress (needs some docs and polishing) patches can be found at: https://github.com/pipo/libvirt/commits/qapi-features-introspect [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature 2019-04-08 16:25 ` [Qemu-devel] [PATCH 0/4] " Peter Krempa @ 2019-04-08 16:25 ` Peter Krempa 0 siblings, 0 replies; 25+ messages in thread From: Peter Krempa @ 2019-04-08 16:25 UTC (permalink / raw) To: Kevin Wolf; +Cc: armbru, qemu-block, qemu-devel [-- Attachment #1: Type: text/plain, Size: 688 bytes --] On Mon, Apr 08, 2019 at 16:35:39 +0200, Kevin Wolf wrote: > This series adds optional feature lists to struct definitions in the > QAPI schema and makes use of them to advertise the new behaviour of > auto-read-only=on in file-posix. > > Kevin Wolf (4): > qapi: Support features for structs > tests/qapi-schema: Test for good feature lists in structs > tests/qapi-schema: Error case tests for features in structs > file-posix: Add dynamic-auto-read-only QAPI feature I was able to implement this in libvirt fairly easy. My work-in-progress (needs some docs and polishing) patches can be found at: https://github.com/pipo/libvirt/commits/qapi-features-introspect [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature 2019-04-08 14:35 [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature Kevin Wolf ` (5 preceding siblings ...) 2019-04-08 16:25 ` [Qemu-devel] [PATCH 0/4] " Peter Krempa @ 2019-04-18 7:22 ` Kevin Wolf 2019-04-18 7:22 ` Kevin Wolf 2019-04-18 20:18 ` Markus Armbruster 7 siblings, 1 reply; 25+ messages in thread From: Kevin Wolf @ 2019-04-18 7:22 UTC (permalink / raw) To: qemu-block; +Cc: armbru, eblake, pkrempa, qemu-devel Am 08.04.2019 um 16:35 hat Kevin Wolf geschrieben: > This series adds optional feature lists to struct definitions in the > QAPI schema and makes use of them to advertise the new behaviour of > auto-read-only=on in file-posix. Ping? ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature 2019-04-18 7:22 ` Kevin Wolf @ 2019-04-18 7:22 ` Kevin Wolf 0 siblings, 0 replies; 25+ messages in thread From: Kevin Wolf @ 2019-04-18 7:22 UTC (permalink / raw) To: qemu-block; +Cc: pkrempa, armbru, qemu-devel Am 08.04.2019 um 16:35 hat Kevin Wolf geschrieben: > This series adds optional feature lists to struct definitions in the > QAPI schema and makes use of them to advertise the new behaviour of > auto-read-only=on in file-posix. Ping? ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature 2019-04-08 14:35 [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature Kevin Wolf ` (6 preceding siblings ...) 2019-04-18 7:22 ` Kevin Wolf @ 2019-04-18 20:18 ` Markus Armbruster 2019-04-18 20:18 ` Markus Armbruster 7 siblings, 1 reply; 25+ messages in thread From: Markus Armbruster @ 2019-04-18 20:18 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-block, pkrempa, qemu-devel Kevin Wolf <kwolf@redhat.com> writes: > This series adds optional feature lists to struct definitions in the > QAPI schema and makes use of them to advertise the new behaviour of > auto-read-only=on in file-posix. General approach looks good to me. It's just a first step, however. One that is already useful. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature 2019-04-18 20:18 ` Markus Armbruster @ 2019-04-18 20:18 ` Markus Armbruster 0 siblings, 0 replies; 25+ messages in thread From: Markus Armbruster @ 2019-04-18 20:18 UTC (permalink / raw) To: Kevin Wolf; +Cc: pkrempa, qemu-devel, qemu-block Kevin Wolf <kwolf@redhat.com> writes: > This series adds optional feature lists to struct definitions in the > QAPI schema and makes use of them to advertise the new behaviour of > auto-read-only=on in file-posix. General approach looks good to me. It's just a first step, however. One that is already useful. ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2019-05-17 18:05 UTC | newest] Thread overview: 25+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-04-08 14:35 [Qemu-devel] [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI feature Kevin Wolf 2019-04-08 14:35 ` Kevin Wolf 2019-04-08 14:35 ` [Qemu-devel] [PATCH 1/4] qapi: Support features for structs Kevin Wolf 2019-04-08 14:35 ` Kevin Wolf 2019-04-18 20:03 ` Markus Armbruster 2019-04-18 20:03 ` Markus Armbruster 2019-05-15 10:58 ` Kevin Wolf 2019-05-15 11:22 ` Peter Krempa 2019-05-15 13:48 ` Markus Armbruster 2019-05-17 13:43 ` Peter Krempa 2019-05-17 18:03 ` Markus Armbruster 2019-04-08 14:35 ` [Qemu-devel] [PATCH 2/4] tests/qapi-schema: Test for good feature lists in structs Kevin Wolf 2019-04-08 14:35 ` Kevin Wolf 2019-04-08 14:35 ` [Qemu-devel] [PATCH 3/4] tests/qapi-schema: Error case tests for features " Kevin Wolf 2019-04-08 14:35 ` Kevin Wolf 2019-04-08 14:35 ` [Qemu-devel] [PATCH 4/4] file-posix: Add dynamic-auto-read-only QAPI feature Kevin Wolf 2019-04-08 14:35 ` Kevin Wolf 2019-04-18 20:13 ` Markus Armbruster 2019-04-18 20:13 ` Markus Armbruster 2019-04-08 16:25 ` [Qemu-devel] [PATCH 0/4] " Peter Krempa 2019-04-08 16:25 ` Peter Krempa 2019-04-18 7:22 ` Kevin Wolf 2019-04-18 7:22 ` Kevin Wolf 2019-04-18 20:18 ` Markus Armbruster 2019-04-18 20:18 ` Markus Armbruster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).