[PATCH v2 0/5] qapi: Add feature flags to enum members

[PATCH v2 0/5] qapi: Add feature flags to enum members

[Markus Armbruster]

PATCH 1+2 add feature flags to enum members.  Awkward due to an
introspection design mistake; see PATCH 1 for details.

PATCH 3+4 implement policy deprecated-input={reject,crash} for enum
values.

Policy deprecated-output=hide is not implemented, because we can't
hide a value without hiding the entire member, which is almost
certainly more than the requester of this policy bargained for.
Perhaps we want a new policy deprecated-output=hide-or-else-crash to
help us catch unwanted use of deprecated enum values.  Perhaps we want
deprecated-output=hide to behave that way together with
deprecated-input=crash.  Or even always.  Thoughts?

PATCH 5 puts the new feature flags to use.  It's RFC because it makes
sense only on top of Vladimir's deprecation of drive-backup.  See its
commit message for a reference.

I prefer to commit new features together with a use outside tests/.
PATCH 5 adds such a use, but it's RFC, because it depends on
Vladimir's work.  Perhaps another use pops up.  I can delay this work
in the hope of a use becoming ready, but the feature flags work I have
in the pipeline will eventually force my hand.

v2:
* Rebased with straightforward conflicts.
* PATCH 1-4: No longer RFC.
* PATCH 1: "Since" information fixed [Eric].  Commit message updated
  to reflect feedback.
* PATCH 2: Commit message amended to point out special feature flag
 'deprecated' is ignored at this stage.
* PATCH 4: Documentation updated.  Commit message tweaked.

Markus Armbruster (5):
  qapi: Enable enum member introspection to show more than name
  qapi: Add feature flags to enum members
  qapi: Move compat policy from QObject to generic visitor
  qapi: Implement deprecated-input={reject,crash} for enum values
  block: Deprecate transaction type drive-backup

 docs/devel/qapi-code-gen.rst                  | 10 ++++---
 qapi/compat.json                              |  3 +++
 qapi/introspect.json                          | 24 +++++++++++++++--
 qapi/transaction.json                         |  5 +++-
 include/qapi/qobject-input-visitor.h          |  4 ---
 include/qapi/qobject-output-visitor.h         |  4 ---
 include/qapi/util.h                           |  6 ++++-
 include/qapi/visitor-impl.h                   |  3 +++
 include/qapi/visitor.h                        |  9 +++++++
 qapi/qapi-visit-core.c                        | 27 ++++++++++++++++---
 qapi/qmp-dispatch.c                           |  4 +--
 qapi/qobject-input-visitor.c                  | 14 +---------
 qapi/qobject-output-visitor.c                 | 14 +---------
 scripts/qapi/expr.py                          |  3 ++-
 scripts/qapi/introspect.py                    | 19 ++++++++++---
 scripts/qapi/schema.py                        | 22 +++++++++++++--
 scripts/qapi/types.py                         | 17 +++++++++++-
 tests/qapi-schema/doc-good.json               |  5 +++-
 tests/qapi-schema/doc-good.out                |  3 +++
 tests/qapi-schema/doc-good.txt                |  3 +++
 .../qapi-schema/enum-dict-member-unknown.err  |  2 +-
 tests/qapi-schema/qapi-schema-test.json       |  3 ++-
 tests/qapi-schema/qapi-schema-test.out        |  1 +
 tests/qapi-schema/test-qapi.py                |  1 +
 24 files changed, 149 insertions(+), 57 deletions(-)

-- 
2.31.1

[PATCH v2 1/5] qapi: Enable enum member introspection to show more than name

[Markus Armbruster]

The next commit will add feature flags to enum members.  There's a
problem, though: query-qmp-schema shows an enum type's members as an
array of member names (SchemaInfoEnum member @values).  If it showed
an array of objects with a name member, we could simply add more
members to these objects.  Since it's just strings, we can't.

I can see three ways to correct this design mistake:

1. Do it the way we should have done it, plus compatibility goo.

   We want a ['SchemaInfoEnumMember'] member in SchemaInfoEnum.  Since
   changing @values would be a compatibility break, add a new member
   @members instead.

   @values is now redundant.  In my testing, output of
   qemu-system-x86_64's query-qmp-schema grows by 11% (18.5KiB).

   We can deprecate @values now and drop it later.  This will break
   outmoded clients.  Well-behaved clients such as libvirt are
   expected to break cleanly.

2. Like 1, but omit "boring" elements of @member, and empty @member.

   @values does not become redundant.  @members augments it.  Somewhat
   cumbersome, but output of query-qmp-schema grows only as we make
   enum members non-boring.

   There is nothing to deprecate here.

3. Versioned query-qmp-schema.

   query-qmp-schema provides either @values or @members.  The QMP
   client can select which version it wants.  There is no redundant
   output.

   We can deprecate old versions and eventually drop them.  This will
   break outmoded clients.  Breaking cleanly is easier than for 1.

   While 1 and 2 operate within the common rules for compatible
   evolution apply (section "Compatibility considerations" in
   docs/devel/qapi-code-gen.rst), 3 bypasses them.  Attractive when
   operating within the rules is just too awkward.  Not the case here.

This commit implements 1.  Libvirt developers prefer it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/introspect.json       | 21 +++++++++++++++++++--
 scripts/qapi/introspect.py | 18 ++++++++++++++----
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/qapi/introspect.json b/qapi/introspect.json
index 39bd303778..f806bd7281 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -142,14 +142,31 @@
 #
 # Additional SchemaInfo members for meta-type 'enum'.
 #
-# @values: the enumeration type's values, in no particular order.
+# @members: the enum type's members, in no particular order
+#           (since 6.2).
+#
+# @values: the enumeration type's member names, in no particular order.
+#          Redundant with @members.  Just for backward compatibility.
 #
 # Values of this type are JSON string on the wire.
 #
 # Since: 2.5
 ##
 { 'struct': 'SchemaInfoEnum',
-  'data': { 'values': ['str'] } }
+  'data': { 'members': [ 'SchemaInfoEnumMember' ],
+            'values': ['str'] } }
+
+##
+# @SchemaInfoEnumMember:
+#
+# An object member.
+#
+# @name: the member's name, as defined in the QAPI schema.
+#
+# Since: 6.2
+##
+{ 'struct': 'SchemaInfoEnumMember',
+  'data': { 'name': 'str' } }
 
 ##
 # @SchemaInfoArray:
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 4c079ee627..6334546363 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -68,6 +68,7 @@
 # TypedDict constructs, so they are broadly typed here as simple
 # Python Dicts.
 SchemaInfo = Dict[str, object]
+SchemaInfoEnumMember = Dict[str, object]
 SchemaInfoObject = Dict[str, object]
 SchemaInfoObjectVariant = Dict[str, object]
 SchemaInfoObjectMember = Dict[str, object]
@@ -274,8 +275,16 @@ def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
             obj['features'] = self._gen_features(features)
         self._trees.append(Annotated(obj, ifcond, comment))
 
-    def _gen_member(self, member: QAPISchemaObjectTypeMember
-                    ) -> Annotated[SchemaInfoObjectMember]:
+    @staticmethod
+    def _gen_enum_member(member: QAPISchemaEnumMember
+                         ) -> Annotated[SchemaInfoEnumMember]:
+        obj: SchemaInfoEnumMember = {
+            'name': member.name,
+        }
+        return Annotated(obj, member.ifcond)
+
+    def _gen_object_member(self, member: QAPISchemaObjectTypeMember
+                           ) -> Annotated[SchemaInfoObjectMember]:
         obj: SchemaInfoObjectMember = {
             'name': member.name,
             'type': self._use_type(member.type)
@@ -305,7 +314,8 @@ def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
                         prefix: Optional[str]) -> None:
         self._gen_tree(
             name, 'enum',
-            {'values': [Annotated(m.name, m.ifcond) for m in members]},
+            {'members': [self._gen_enum_member(m) for m in members],
+             'values': [Annotated(m.name, m.ifcond) for m in members]},
             ifcond, features
         )
 
@@ -322,7 +332,7 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
                                members: List[QAPISchemaObjectTypeMember],
                                variants: Optional[QAPISchemaVariants]) -> None:
         obj: SchemaInfoObject = {
-            'members': [self._gen_member(m) for m in members]
+            'members': [self._gen_object_member(m) for m in members]
         }
         if variants:
             obj['tag'] = variants.tag_member.name
-- 
2.31.1

Re: [PATCH v2 1/5] qapi: Enable enum member introspection to show more than name

[Eric Blake]

On Sat, Oct 09, 2021 at 02:09:40PM +0200, Markus Armbruster wrote:
> The next commit will add feature flags to enum members.  There's a
> problem, though: query-qmp-schema shows an enum type's members as an
> array of member names (SchemaInfoEnum member @values).  If it showed
> an array of objects with a name member, we could simply add more
> members to these objects.  Since it's just strings, we can't.
> 
> I can see three ways to correct this design mistake:
> 
> 1. Do it the way we should have done it, plus compatibility goo.
...
> 2. Like 1, but omit "boring" elements of @member, and empty @member.

> 3. Versioned query-qmp-schema.

> This commit implements 1.  Libvirt developers prefer it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi/introspect.json       | 21 +++++++++++++++++++--
>  scripts/qapi/introspect.py | 18 ++++++++++++++----
>  2 files changed, 33 insertions(+), 6 deletions(-)

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

> 
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index 39bd303778..f806bd7281 100644
> --- a/qapi/introspect.json
> +++ b/qapi/introspect.json
> @@ -142,14 +142,31 @@
>  #
>  # Additional SchemaInfo members for meta-type 'enum'.
>  #
> -# @values: the enumeration type's values, in no particular order.
> +# @members: the enum type's members, in no particular order
> +#           (since 6.2).
> +#
> +# @values: the enumeration type's member names, in no particular order.
> +#          Redundant with @members.  Just for backward compatibility.
>  #
>  # Values of this type are JSON string on the wire.
>  #
>  # Since: 2.5
>  ##
>  { 'struct': 'SchemaInfoEnum',
> -  'data': { 'values': ['str'] } }
> +  'data': { 'members': [ 'SchemaInfoEnumMember' ],
> +            'values': ['str'] } }

Not deprecated at this time, I agree with your choice to make the
actual deprecation of 'values' to be in a later patch (if at all).

> +
> +##
> +# @SchemaInfoEnumMember:
> +#
> +# An object member.
> +#
> +# @name: the member's name, as defined in the QAPI schema.
> +#
> +# Since: 6.2
> +##
> +{ 'struct': 'SchemaInfoEnumMember',
> +  'data': { 'name': 'str' } }

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 1/5] qapi: Enable enum member introspection to show more than name

[Kevin Wolf]

Am 09.10.2021 um 14:09 hat Markus Armbruster geschrieben:
> The next commit will add feature flags to enum members.  There's a
> problem, though: query-qmp-schema shows an enum type's members as an
> array of member names (SchemaInfoEnum member @values).  If it showed
> an array of objects with a name member, we could simply add more
> members to these objects.  Since it's just strings, we can't.
> 
> I can see three ways to correct this design mistake:
> 
> 1. Do it the way we should have done it, plus compatibility goo.
> 
>    We want a ['SchemaInfoEnumMember'] member in SchemaInfoEnum.  Since
>    changing @values would be a compatibility break, add a new member
>    @members instead.
> 
>    @values is now redundant.  In my testing, output of
>    qemu-system-x86_64's query-qmp-schema grows by 11% (18.5KiB).
> 
>    We can deprecate @values now and drop it later.  This will break
>    outmoded clients.  Well-behaved clients such as libvirt are
>    expected to break cleanly.
> 
> 2. Like 1, but omit "boring" elements of @member, and empty @member.
> 
>    @values does not become redundant.  @members augments it.  Somewhat
>    cumbersome, but output of query-qmp-schema grows only as we make
>    enum members non-boring.
> 
>    There is nothing to deprecate here.
> 
> 3. Versioned query-qmp-schema.
> 
>    query-qmp-schema provides either @values or @members.  The QMP
>    client can select which version it wants.  There is no redundant
>    output.
> 
>    We can deprecate old versions and eventually drop them.  This will
>    break outmoded clients.  Breaking cleanly is easier than for 1.
> 
>    While 1 and 2 operate within the common rules for compatible
>    evolution apply (section "Compatibility considerations" in
>    docs/devel/qapi-code-gen.rst), 3 bypasses them.  Attractive when
>    operating within the rules is just too awkward.  Not the case here.
> 
> This commit implements 1.  Libvirt developers prefer it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi/introspect.json       | 21 +++++++++++++++++++--
>  scripts/qapi/introspect.py | 18 ++++++++++++++----
>  2 files changed, 33 insertions(+), 6 deletions(-)

docs/devel/qapi-code-gen.rst still says:

  The SchemaInfo for an enumeration type has meta-type "enum" and
  variant member "values".  The values are listed in no particular
  order; clients must search the entire enum when learning whether a
  particular value is supported.

  Example: the SchemaInfo for MyEnum from section `Enumeration types`_ ::

      { "name": "MyEnum", "meta-type": "enum",
        "values": [ "value1", "value2", "value3" ] }

It probably needs an update.

Kevin

Re: [PATCH v2 1/5] qapi: Enable enum member introspection to show more than name

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 09.10.2021 um 14:09 hat Markus Armbruster geschrieben:
>> The next commit will add feature flags to enum members.  There's a
>> problem, though: query-qmp-schema shows an enum type's members as an
>> array of member names (SchemaInfoEnum member @values).  If it showed
>> an array of objects with a name member, we could simply add more
>> members to these objects.  Since it's just strings, we can't.
>> 
>> I can see three ways to correct this design mistake:
>> 
>> 1. Do it the way we should have done it, plus compatibility goo.
>> 
>>    We want a ['SchemaInfoEnumMember'] member in SchemaInfoEnum.  Since
>>    changing @values would be a compatibility break, add a new member
>>    @members instead.
>> 
>>    @values is now redundant.  In my testing, output of
>>    qemu-system-x86_64's query-qmp-schema grows by 11% (18.5KiB).
>> 
>>    We can deprecate @values now and drop it later.  This will break
>>    outmoded clients.  Well-behaved clients such as libvirt are
>>    expected to break cleanly.
>> 
>> 2. Like 1, but omit "boring" elements of @member, and empty @member.
>> 
>>    @values does not become redundant.  @members augments it.  Somewhat
>>    cumbersome, but output of query-qmp-schema grows only as we make
>>    enum members non-boring.
>> 
>>    There is nothing to deprecate here.
>> 
>> 3. Versioned query-qmp-schema.
>> 
>>    query-qmp-schema provides either @values or @members.  The QMP
>>    client can select which version it wants.  There is no redundant
>>    output.
>> 
>>    We can deprecate old versions and eventually drop them.  This will
>>    break outmoded clients.  Breaking cleanly is easier than for 1.
>> 
>>    While 1 and 2 operate within the common rules for compatible
>>    evolution apply (section "Compatibility considerations" in
>>    docs/devel/qapi-code-gen.rst), 3 bypasses them.  Attractive when
>>    operating within the rules is just too awkward.  Not the case here.
>> 
>> This commit implements 1.  Libvirt developers prefer it.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  qapi/introspect.json       | 21 +++++++++++++++++++--
>>  scripts/qapi/introspect.py | 18 ++++++++++++++----
>>  2 files changed, 33 insertions(+), 6 deletions(-)
>
> docs/devel/qapi-code-gen.rst still says:
>
>   The SchemaInfo for an enumeration type has meta-type "enum" and
>   variant member "values".  The values are listed in no particular
>   order; clients must search the entire enum when learning whether a
>   particular value is supported.
>
>   Example: the SchemaInfo for MyEnum from section `Enumeration types`_ ::
>
>       { "name": "MyEnum", "meta-type": "enum",
>         "values": [ "value1", "value2", "value3" ] }
>
> It probably needs an update.

It sure does.  Thanks!

[PATCH v2 2/5] qapi: Add feature flags to enum members

[Markus Armbruster]

This is quite similar to commit 84ab008687 "qapi: Add feature flags to
struct members", only for enums instead of structs.

Special feature flag 'deprecated' is silently ignored there.  This is
okay only because it will be implemented shortly.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 docs/devel/qapi-code-gen.rst                  |  4 +++-
 qapi/compat.json                              |  2 ++
 qapi/introspect.json                          |  5 ++++-
 scripts/qapi/expr.py                          |  3 ++-
 scripts/qapi/introspect.py                    |  5 +++--
 scripts/qapi/schema.py                        | 22 +++++++++++++++++--
 tests/qapi-schema/doc-good.json               |  5 ++++-
 tests/qapi-schema/doc-good.out                |  3 +++
 tests/qapi-schema/doc-good.txt                |  3 +++
 .../qapi-schema/enum-dict-member-unknown.err  |  2 +-
 tests/qapi-schema/qapi-schema-test.json       |  3 ++-
 tests/qapi-schema/qapi-schema-test.out        |  1 +
 tests/qapi-schema/test-qapi.py                |  1 +
 13 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index b2569de486..00334e9fb8 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -200,7 +200,9 @@ Syntax::
              '*if': COND,
              '*features': FEATURES }
     ENUM-VALUE = STRING
-               | { 'name': STRING, '*if': COND }
+               | { 'name': STRING,
+                   '*if': COND,
+                   '*features': FEATURES }
 
 Member 'enum' names the enum type.
 
diff --git a/qapi/compat.json b/qapi/compat.json
index ae3afc22df..1d2b76f00c 100644
--- a/qapi/compat.json
+++ b/qapi/compat.json
@@ -42,6 +42,8 @@
 # with feature 'deprecated'.  We may want to extend it to cover
 # semantic aspects, CLI, and experimental features.
 #
+# Limitation: not implemented for deprecated enumeration values.
+#
 # @deprecated-input: how to handle deprecated input (default 'accept')
 # @deprecated-output: how to handle deprecated output (default 'accept')
 #
diff --git a/qapi/introspect.json b/qapi/introspect.json
index f806bd7281..4a3b76464e 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -163,10 +163,13 @@
 #
 # @name: the member's name, as defined in the QAPI schema.
 #
+# @features: names of features associated with the member, in no
+#            particular order.
+#
 # Since: 6.2
 ##
 { 'struct': 'SchemaInfoEnumMember',
-  'data': { 'name': 'str' } }
+  'data': { 'name': 'str', '*features': [ 'str' ] } }
 
 ##
 # @SchemaInfoArray:
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 819ea6ad97..3cb389e875 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -472,7 +472,7 @@ def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None:
                   for m in members]
     for member in members:
         source = "'data' member"
-        check_keys(member, info, source, ['name'], ['if'])
+        check_keys(member, info, source, ['name'], ['if', 'features'])
         member_name = member['name']
         check_name_is_str(member_name, info, source)
         source = "%s '%s'" % (source, member_name)
@@ -483,6 +483,7 @@ def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None:
                          permit_upper=permissive,
                          permit_underscore=permissive)
         check_if(member, info, source)
+        check_features(member.get('features'), info)
 
 
 def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None:
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 6334546363..67c7d89aae 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -275,12 +275,13 @@ def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
             obj['features'] = self._gen_features(features)
         self._trees.append(Annotated(obj, ifcond, comment))
 
-    @staticmethod
-    def _gen_enum_member(member: QAPISchemaEnumMember
+    def _gen_enum_member(self, member: QAPISchemaEnumMember
                          ) -> Annotated[SchemaInfoEnumMember]:
         obj: SchemaInfoEnumMember = {
             'name': member.name,
         }
+        if member.features:
+            obj['features'] = self._gen_features(member.features)
         return Annotated(obj, member.ifcond)
 
     def _gen_object_member(self, member: QAPISchemaObjectTypeMember
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 004d7095ff..6d5f46509a 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -708,6 +708,19 @@ def describe(self, info):
 class QAPISchemaEnumMember(QAPISchemaMember):
     role = 'value'
 
+    def __init__(self, name, info, ifcond=None, features=None):
+        super().__init__(name, info, ifcond)
+        for f in features or []:
+            assert isinstance(f, QAPISchemaFeature)
+            f.set_defined_in(name)
+        self.features = features or []
+
+    def connect_doc(self, doc):
+        super().connect_doc(doc)
+        if doc:
+            for f in self.features:
+                doc.connect_feature(f)
+
 
 class QAPISchemaFeature(QAPISchemaMember):
     role = 'feature'
@@ -980,9 +993,14 @@ def _make_features(self, features, info):
                                   QAPISchemaIfCond(f.get('if')))
                 for f in features]
 
+    def _make_enum_member(self, name, ifcond, features, info):
+        return QAPISchemaEnumMember(name, info,
+                                    QAPISchemaIfCond(ifcond),
+                                    self._make_features(features, info))
+
     def _make_enum_members(self, values, info):
-        return [QAPISchemaEnumMember(v['name'], info,
-                                     QAPISchemaIfCond(v.get('if')))
+        return [self._make_enum_member(v['name'], v.get('if'),
+                                       v.get('features'), info)
                 for v in values]
 
     def _make_array_type(self, element_type, info):
diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
index 86dc25d2bd..74745fb405 100644
--- a/tests/qapi-schema/doc-good.json
+++ b/tests/qapi-schema/doc-good.json
@@ -58,11 +58,14 @@
 #
 # Features:
 # @enum-feat: Also _one_ {and only}
+# @enum-member-feat: a member feature
 #
 # @two is undocumented
 ##
 { 'enum': 'Enum',
-  'data': [ { 'name': 'one', 'if': 'IFONE' }, 'two' ],
+  'data': [ { 'name': 'one', 'if': 'IFONE',
+              'features': [ 'enum-member-feat' ] },
+            'two' ],
   'features': [ 'enum-feat' ],
   'if': 'IFCOND' }
 
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 5a324e2627..9dd65b9d92 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -13,6 +13,7 @@ module doc-good.json
 enum Enum
     member one
         if IFONE
+        feature enum-member-feat
     member two
     if IFCOND
     feature enum-feat
@@ -108,6 +109,8 @@ The _one_ {and only}
 
     feature=enum-feat
 Also _one_ {and only}
+    feature=enum-member-feat
+a member feature
     section=None
 @two is undocumented
 doc symbol=Base
diff --git a/tests/qapi-schema/doc-good.txt b/tests/qapi-schema/doc-good.txt
index 701402ee5e..b3b76bd43f 100644
--- a/tests/qapi-schema/doc-good.txt
+++ b/tests/qapi-schema/doc-good.txt
@@ -56,6 +56,9 @@ Features
 "enum-feat"
    Also _one_ {and only}
 
+"enum-member-feat"
+   a member feature
+
 "two" is undocumented
 
 
diff --git a/tests/qapi-schema/enum-dict-member-unknown.err b/tests/qapi-schema/enum-dict-member-unknown.err
index f8617ea179..235cde0c49 100644
--- a/tests/qapi-schema/enum-dict-member-unknown.err
+++ b/tests/qapi-schema/enum-dict-member-unknown.err
@@ -1,3 +1,3 @@
 enum-dict-member-unknown.json: In enum 'MyEnum':
 enum-dict-member-unknown.json:2: 'data' member has unknown key 'bad-key'
-Valid keys are 'if', 'name'.
+Valid keys are 'features', 'if', 'name'.
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 2ec50109cb..b677ab861d 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -301,7 +301,8 @@
                                  'TEST_IF_COND_2'] } } ] }
 
 { 'enum': 'FeatureEnum1',
-  'data': [ 'eins', 'zwei', 'drei' ],
+  'data': [ 'eins', 'zwei',
+            { 'name': 'drei', 'features': [ 'deprecated' ] } ],
   'features': [ 'feature1' ] }
 
 { 'union': 'FeatureUnion1',
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 9337adc9ea..16846dbeb8 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -341,6 +341,7 @@ enum FeatureEnum1
     member eins
     member zwei
     member drei
+        feature deprecated
     feature feature1
 object q_obj_FeatureUnion1-base
     member tag: FeatureEnum1 optional=False
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index c717a7a90b..2160cef082 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -37,6 +37,7 @@ def visit_enum_type(self, name, info, ifcond, features, members, prefix):
         for m in members:
             print('    member %s' % m.name)
             self._print_if(m.ifcond, indent=8)
+            self._print_features(m.features, indent=8)
         self._print_if(ifcond)
         self._print_features(features)
 
-- 
2.31.1

Re: [PATCH v2 2/5] qapi: Add feature flags to enum members

[Kevin Wolf]

Am 09.10.2021 um 14:09 hat Markus Armbruster geschrieben:
> This is quite similar to commit 84ab008687 "qapi: Add feature flags to
> struct members", only for enums instead of structs.
> 
> Special feature flag 'deprecated' is silently ignored there.  This is
> okay only because it will be implemented shortly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Should we have a test case for an invalid value for 'features'?

Kevin

Re: [PATCH v2 2/5] qapi: Add feature flags to enum members

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 09.10.2021 um 14:09 hat Markus Armbruster geschrieben:
>> This is quite similar to commit 84ab008687 "qapi: Add feature flags to
>> struct members", only for enums instead of structs.
>> 
>> Special feature flag 'deprecated' is silently ignored there.  This is
>> okay only because it will be implemented shortly.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Should we have a test case for an invalid value for 'features'?

We have coverage, just not in every context.

struct context:

    tests/qapi-schema/features-bad-type.json
    tests/qapi-schema/features-deprecated-type.json
    tests/qapi-schema/features-duplicate-name.json
    tests/qapi-schema/features-if-invalid.json
    tests/qapi-schema/features-missing-name.json
    tests/qapi-schema/features-name-bad-type.json
    tests/qapi-schema/features-no-list.json
    tests/qapi-schema/features-unknown-key.json

struct member context:

    tests/qapi-schema/features-member-bad-type.json
    tests/qapi-schema/features-member-duplicate-name.json
    tests/qapi-schema/features-member-if-invalid.json
    tests/qapi-schema/features-member-missing-name.json
    tests/qapi-schema/features-member-name-bad-type.json
    tests/qapi-schema/features-member-no-list.json
    tests/qapi-schema/features-member-unknown-key.json

These are basically the same, except for features-deprecated-type.json,
which makes sense only in struct context.

The other contexts are enum, union, alternate, command, event, and now
enum member.

My excuse for skipping contexts is that the errors come from
check_features() for all them.

[PATCH v2 3/5] qapi: Move compat policy from QObject to generic visitor

[Markus Armbruster]

The next commit needs to access compat policy from the generic visitor
core.  Move it there from qobject input and output visitor.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/qobject-input-visitor.h  |  4 ----
 include/qapi/qobject-output-visitor.h |  4 ----
 include/qapi/visitor-impl.h           |  3 +++
 include/qapi/visitor.h                |  9 +++++++++
 qapi/qapi-visit-core.c                |  9 +++++++++
 qapi/qmp-dispatch.c                   |  4 ++--
 qapi/qobject-input-visitor.c          | 14 +-------------
 qapi/qobject-output-visitor.c         | 14 +-------------
 8 files changed, 25 insertions(+), 36 deletions(-)

diff --git a/include/qapi/qobject-input-visitor.h b/include/qapi/qobject-input-visitor.h
index 8d69388810..95985e25e5 100644
--- a/include/qapi/qobject-input-visitor.h
+++ b/include/qapi/qobject-input-visitor.h
@@ -15,7 +15,6 @@
 #ifndef QOBJECT_INPUT_VISITOR_H
 #define QOBJECT_INPUT_VISITOR_H
 
-#include "qapi/qapi-types-compat.h"
 #include "qapi/visitor.h"
 
 typedef struct QObjectInputVisitor QObjectInputVisitor;
@@ -59,9 +58,6 @@ typedef struct QObjectInputVisitor QObjectInputVisitor;
  */
 Visitor *qobject_input_visitor_new(QObject *obj);
 
-void qobject_input_visitor_set_policy(Visitor *v,
-                                      CompatPolicyInput deprecated);
-
 /*
  * Create a QObject input visitor for @obj for use with keyval_parse()
  *
diff --git a/include/qapi/qobject-output-visitor.h b/include/qapi/qobject-output-visitor.h
index f2a2f92a00..2b1726baf5 100644
--- a/include/qapi/qobject-output-visitor.h
+++ b/include/qapi/qobject-output-visitor.h
@@ -15,7 +15,6 @@
 #define QOBJECT_OUTPUT_VISITOR_H
 
 #include "qapi/visitor.h"
-#include "qapi/qapi-types-compat.h"
 
 typedef struct QObjectOutputVisitor QObjectOutputVisitor;
 
@@ -54,7 +53,4 @@ typedef struct QObjectOutputVisitor QObjectOutputVisitor;
  */
 Visitor *qobject_output_visitor_new(QObject **result);
 
-void qobject_output_visitor_set_policy(Visitor *v,
-                                       CompatPolicyOutput deprecated);
-
 #endif
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 3b950f6e3d..72b6537bef 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -122,6 +122,9 @@ struct Visitor
     /* Must be set */
     VisitorType type;
 
+    /* Optional */
+    struct CompatPolicy compat_policy;
+
     /* Must be set for output visitors, optional otherwise. */
     void (*complete)(Visitor *v, void *opaque);
 
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index b3c9ef7a81..dcb96018a9 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -16,6 +16,7 @@
 #define QAPI_VISITOR_H
 
 #include "qapi/qapi-builtin-types.h"
+#include "qapi/qapi-types-compat.h"
 
 /*
  * The QAPI schema defines both a set of C data types, and a QMP wire
@@ -477,6 +478,14 @@ bool visit_deprecated_accept(Visitor *v, const char *name, Error **errp);
  */
 bool visit_deprecated(Visitor *v, const char *name);
 
+/*
+ * Set policy for handling deprecated management interfaces.
+ *
+ * Intended use: call visit_set_policy(v, &compat_policy) when
+ * visiting management interface input or output.
+ */
+void visit_set_policy(Visitor *v, CompatPolicy *policy);
+
 /*
  * Visit an enum value.
  *
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index a641adec51..066f77a26d 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -19,6 +19,10 @@
 #include "qapi/visitor-impl.h"
 #include "trace.h"
 
+/* Zero-initialization must result in default policy */
+QEMU_BUILD_BUG_ON(COMPAT_POLICY_INPUT_ACCEPT || COMPAT_POLICY_OUTPUT_ACCEPT);
+
+
 void visit_complete(Visitor *v, void *opaque)
 {
     assert(v->type != VISITOR_OUTPUT || v->complete);
@@ -153,6 +157,11 @@ bool visit_deprecated(Visitor *v, const char *name)
     return true;
 }
 
+void visit_set_policy(Visitor *v, CompatPolicy *policy)
+{
+    v->compat_policy = *policy;
+}
+
 bool visit_is_input(Visitor *v)
 {
     return v->type == VISITOR_INPUT;
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 59600210ce..7e943a0af5 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -32,7 +32,7 @@ Visitor *qobject_input_visitor_new_qmp(QObject *obj)
 {
     Visitor *v = qobject_input_visitor_new(obj);
 
-    qobject_input_visitor_set_policy(v, compat_policy.deprecated_input);
+    visit_set_policy(v, &compat_policy);
     return v;
 }
 
@@ -40,7 +40,7 @@ Visitor *qobject_output_visitor_new_qmp(QObject **result)
 {
     Visitor *v = qobject_output_visitor_new(result);
 
-    qobject_output_visitor_set_policy(v, compat_policy.deprecated_output);
+    visit_set_policy(v, &compat_policy);
     return v;
 }
 
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 04b790412e..71b24a4429 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -14,7 +14,6 @@
 
 #include "qemu/osdep.h"
 #include <math.h>
-#include "qapi/compat-policy.h"
 #include "qapi/error.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/visitor-impl.h"
@@ -44,7 +43,6 @@ typedef struct StackObject {
 
 struct QObjectInputVisitor {
     Visitor visitor;
-    CompatPolicyInput deprecated_policy;
 
     /* Root of visit at visitor creation. */
     QObject *root;
@@ -667,9 +665,7 @@ static void qobject_input_optional(Visitor *v, const char *name, bool *present)
 static bool qobject_input_deprecated_accept(Visitor *v, const char *name,
                                             Error **errp)
 {
-    QObjectInputVisitor *qiv = to_qiv(v);
-
-    switch (qiv->deprecated_policy) {
+    switch (v->compat_policy.deprecated_input) {
     case COMPAT_POLICY_INPUT_ACCEPT:
         return true;
     case COMPAT_POLICY_INPUT_REJECT:
@@ -739,14 +735,6 @@ Visitor *qobject_input_visitor_new(QObject *obj)
     return &v->visitor;
 }
 
-void qobject_input_visitor_set_policy(Visitor *v,
-                                       CompatPolicyInput deprecated)
-{
-    QObjectInputVisitor *qiv = to_qiv(v);
-
-    qiv->deprecated_policy = deprecated;
-}
-
 Visitor *qobject_input_visitor_new_keyval(QObject *obj)
 {
     QObjectInputVisitor *v = qobject_input_visitor_base_new(obj);
diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c
index e4873308d4..9b7f510036 100644
--- a/qapi/qobject-output-visitor.c
+++ b/qapi/qobject-output-visitor.c
@@ -13,7 +13,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "qapi/compat-policy.h"
 #include "qapi/qobject-output-visitor.h"
 #include "qapi/visitor-impl.h"
 #include "qemu/queue.h"
@@ -32,7 +31,6 @@ typedef struct QStackEntry {
 
 struct QObjectOutputVisitor {
     Visitor visitor;
-    CompatPolicyOutput deprecated_policy;
 
     QSLIST_HEAD(, QStackEntry) stack; /* Stack of unfinished containers */
     QObject *root; /* Root of the output visit */
@@ -212,9 +210,7 @@ static bool qobject_output_type_null(Visitor *v, const char *name,
 
 static bool qobject_output_deprecated(Visitor *v, const char *name)
 {
-    QObjectOutputVisitor *qov = to_qov(v);
-
-    return qov->deprecated_policy != COMPAT_POLICY_OUTPUT_HIDE;
+    return v->compat_policy.deprecated_output != COMPAT_POLICY_OUTPUT_HIDE;
 }
 
 /* Finish building, and return the root object.
@@ -275,11 +271,3 @@ Visitor *qobject_output_visitor_new(QObject **result)
 
     return &v->visitor;
 }
-
-void qobject_output_visitor_set_policy(Visitor *v,
-                                       CompatPolicyOutput deprecated)
-{
-    QObjectOutputVisitor *qov = to_qov(v);
-
-    qov->deprecated_policy = deprecated;
-}
-- 
2.31.1

[PATCH v2 4/5] qapi: Implement deprecated-input={reject, crash} for enum values

[Markus Armbruster]

This copies the code implementing the policy from qapi/qmp-dispatch.c
to qapi/qobject-input-visitor.c.  Tolerable, but if we acquire more
copes, we should look into factoring them out.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/devel/qapi-code-gen.rst |  6 ++++--
 qapi/compat.json             |  3 ++-
 include/qapi/util.h          |  6 +++++-
 qapi/qapi-visit-core.c       | 18 +++++++++++++++---
 scripts/qapi/types.py        | 17 ++++++++++++++++-
 5 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index 00334e9fb8..006a6f4a9a 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -708,8 +708,10 @@ QEMU shows a certain behaviour.
 Special features
 ~~~~~~~~~~~~~~~~
 
-Feature "deprecated" marks a command, event, or struct member as
-deprecated.  It is not supported elsewhere so far.
+Feature "deprecated" marks a command, event, struct or enum member as
+deprecated.  It is not supported elsewhere so far.  Interfaces so
+marked may be withdrawn in future releases in accordance with QEMU's
+deprecation policy.
 
 
 Naming rules and reserved names
diff --git a/qapi/compat.json b/qapi/compat.json
index 1d2b76f00c..74a8493d3d 100644
--- a/qapi/compat.json
+++ b/qapi/compat.json
@@ -42,7 +42,8 @@
 # with feature 'deprecated'.  We may want to extend it to cover
 # semantic aspects, CLI, and experimental features.
 #
-# Limitation: not implemented for deprecated enumeration values.
+# Limitation: deprecated-output policy @hide is not implemented for
+# enumeration values.  They behave the same as with policy @accept.
 #
 # @deprecated-input: how to handle deprecated input (default 'accept')
 # @deprecated-output: how to handle deprecated output (default 'accept')
diff --git a/include/qapi/util.h b/include/qapi/util.h
index d7bfb30e25..257c600f99 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -11,9 +11,13 @@
 #ifndef QAPI_UTIL_H
 #define QAPI_UTIL_H
 
+/* QEnumLookup flags */
+#define QAPI_ENUM_DEPRECATED 1
+
 typedef struct QEnumLookup {
     const char *const *array;
-    int size;
+    const unsigned char *const flags;
+    const int size;
 } QEnumLookup;
 
 const char *qapi_enum_lookup(const QEnumLookup *lookup, int val);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 066f77a26d..49136ae88e 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -393,7 +393,7 @@ static bool input_type_enum(Visitor *v, const char *name, int *obj,
                             const QEnumLookup *lookup, Error **errp)
 {
     int64_t value;
-    char *enum_str;
+    g_autofree char *enum_str = NULL;
 
     if (!visit_type_str(v, name, &enum_str, errp)) {
         return false;
@@ -402,11 +402,23 @@ static bool input_type_enum(Visitor *v, const char *name, int *obj,
     value = qapi_enum_parse(lookup, enum_str, -1, NULL);
     if (value < 0) {
         error_setg(errp, QERR_INVALID_PARAMETER, enum_str);
-        g_free(enum_str);
         return false;
     }
 
-    g_free(enum_str);
+    if (lookup->flags && (lookup->flags[value] & QAPI_ENUM_DEPRECATED)) {
+        switch (v->compat_policy.deprecated_input) {
+        case COMPAT_POLICY_INPUT_ACCEPT:
+            break;
+        case COMPAT_POLICY_INPUT_REJECT:
+            error_setg(errp, "Deprecated value '%s' disabled by policy",
+                       enum_str);
+            return false;
+        case COMPAT_POLICY_INPUT_CRASH:
+        default:
+            abort();
+        }
+    }
+
     *obj = value;
     return true;
 }
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 831294fe42..ab2441adc9 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -38,6 +38,8 @@
 def gen_enum_lookup(name: str,
                     members: List[QAPISchemaEnumMember],
                     prefix: Optional[str] = None) -> str:
+    max_index = c_enum_const(name, '_MAX', prefix)
+    flags = ''
     ret = mcgen('''
 
 const QEnumLookup %(c_name)s_lookup = {
@@ -52,13 +54,26 @@ def gen_enum_lookup(name: str,
 ''',
                      index=index, name=memb.name)
         ret += memb.ifcond.gen_endif()
+        if 'deprecated' in (f.name for f in memb.features):
+            flags += mcgen('''
+        [%(index)s] = QAPI_ENUM_DEPRECATED,
+''',
+                           index=index)
+
+    if flags:
+        ret += mcgen('''
+    },
+    .flags = (const unsigned char[%(max_index)s]) {
+''',
+                     max_index=max_index)
+        ret += flags
 
     ret += mcgen('''
     },
     .size = %(max_index)s
 };
 ''',
-                 max_index=c_enum_const(name, '_MAX', prefix))
+                 max_index=max_index)
     return ret
 
 
-- 
2.31.1

Re: [PATCH v2 4/5] qapi: Implement deprecated-input={reject,crash} for enum values

[Eric Blake]

On Sat, Oct 09, 2021 at 02:09:43PM +0200, Markus Armbruster wrote:
> This copies the code implementing the policy from qapi/qmp-dispatch.c
> to qapi/qobject-input-visitor.c.  Tolerable, but if we acquire more
> copes, we should look into factoring them out.

copies

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/devel/qapi-code-gen.rst |  6 ++++--
>  qapi/compat.json             |  3 ++-
>  include/qapi/util.h          |  6 +++++-
>  qapi/qapi-visit-core.c       | 18 +++++++++++++++---
>  scripts/qapi/types.py        | 17 ++++++++++++++++-
>  5 files changed, 42 insertions(+), 8 deletions(-)
> 
> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
> index 00334e9fb8..006a6f4a9a 100644
> --- a/docs/devel/qapi-code-gen.rst
> +++ b/docs/devel/qapi-code-gen.rst
> @@ -708,8 +708,10 @@ QEMU shows a certain behaviour.
>  Special features
>  ~~~~~~~~~~~~~~~~
>  
> -Feature "deprecated" marks a command, event, or struct member as
> -deprecated.  It is not supported elsewhere so far.
> +Feature "deprecated" marks a command, event, struct or enum member as

Do we want the comma before the conjunction?  (I've seen style guides
that recommend it, and style guides that discourage it; while I tend
to write by the former style, I usually don't care about the latter.
Rather, switching styles mid-patch caught my attention).

> +deprecated.  It is not supported elsewhere so far.  Interfaces so
> +marked may be withdrawn in future releases in accordance with QEMU's
> +deprecation policy.
>  
>  
> +++ b/qapi/qapi-visit-core.c
> @@ -393,7 +393,7 @@ static bool input_type_enum(Visitor *v, const char *name, int *obj,
>                              const QEnumLookup *lookup, Error **errp)
>  {
>      int64_t value;
> -    char *enum_str;
> +    g_autofree char *enum_str = NULL;

Nice change while touching the code.  Is it worth mentioning in the
commit message?

>  
>      if (!visit_type_str(v, name, &enum_str, errp)) {
>          return false;
> @@ -402,11 +402,23 @@ static bool input_type_enum(Visitor *v, const char *name, int *obj,
>      value = qapi_enum_parse(lookup, enum_str, -1, NULL);
>      if (value < 0) {
>          error_setg(errp, QERR_INVALID_PARAMETER, enum_str);
> -        g_free(enum_str);
>          return false;
>      }
>  
> -    g_free(enum_str);
> +    if (lookup->flags && (lookup->flags[value] & QAPI_ENUM_DEPRECATED)) {
> +        switch (v->compat_policy.deprecated_input) {
> +        case COMPAT_POLICY_INPUT_ACCEPT:
> +            break;
> +        case COMPAT_POLICY_INPUT_REJECT:
> +            error_setg(errp, "Deprecated value '%s' disabled by policy",
> +                       enum_str);
> +            return false;
> +        case COMPAT_POLICY_INPUT_CRASH:
> +        default:
> +            abort();
> +        }
> +    }
> +
>      *obj = value;
>      return true;
>  }

Grammar fixes are minor, so:

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 4/5] qapi: Implement deprecated-input={reject,crash} for enum values

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On Sat, Oct 09, 2021 at 02:09:43PM +0200, Markus Armbruster wrote:
>> This copies the code implementing the policy from qapi/qmp-dispatch.c
>> to qapi/qobject-input-visitor.c.  Tolerable, but if we acquire more
>> copes, we should look into factoring them out.
>
> copies

Fixing, thanks!

>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  docs/devel/qapi-code-gen.rst |  6 ++++--
>>  qapi/compat.json             |  3 ++-
>>  include/qapi/util.h          |  6 +++++-
>>  qapi/qapi-visit-core.c       | 18 +++++++++++++++---
>>  scripts/qapi/types.py        | 17 ++++++++++++++++-
>>  5 files changed, 42 insertions(+), 8 deletions(-)
>> 
>> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
>> index 00334e9fb8..006a6f4a9a 100644
>> --- a/docs/devel/qapi-code-gen.rst
>> +++ b/docs/devel/qapi-code-gen.rst
>> @@ -708,8 +708,10 @@ QEMU shows a certain behaviour.
>>  Special features
>>  ~~~~~~~~~~~~~~~~
>>  
>> -Feature "deprecated" marks a command, event, or struct member as
>> -deprecated.  It is not supported elsewhere so far.
>> +Feature "deprecated" marks a command, event, struct or enum member as
>
> Do we want the comma before the conjunction?  (I've seen style guides
> that recommend it, and style guides that discourage it; while I tend
> to write by the former style, I usually don't care about the latter.
> Rather, switching styles mid-patch caught my attention).

With a comma there, we claim structs can be marked, which is actually
wrong.  Correct is "command, event, struct member, or enum member".

I'll rephrase to "marks a command, event, enum value, or struct member
deprecated."

>> +deprecated.  It is not supported elsewhere so far.  Interfaces so
>> +marked may be withdrawn in future releases in accordance with QEMU's
>> +deprecation policy.
>>  
>>  
>> +++ b/qapi/qapi-visit-core.c
>> @@ -393,7 +393,7 @@ static bool input_type_enum(Visitor *v, const char *name, int *obj,
>>                              const QEnumLookup *lookup, Error **errp)
>>  {
>>      int64_t value;
>> -    char *enum_str;
>> +    g_autofree char *enum_str = NULL;
>
> Nice change while touching the code.  Is it worth mentioning in the
> commit message?

I figure it would be more distracting than useful.

>>  
>>      if (!visit_type_str(v, name, &enum_str, errp)) {
>>          return false;
>> @@ -402,11 +402,23 @@ static bool input_type_enum(Visitor *v, const char *name, int *obj,
>>      value = qapi_enum_parse(lookup, enum_str, -1, NULL);
>>      if (value < 0) {
>>          error_setg(errp, QERR_INVALID_PARAMETER, enum_str);
>> -        g_free(enum_str);
>>          return false;
>>      }
>>  
>> -    g_free(enum_str);
>> +    if (lookup->flags && (lookup->flags[value] & QAPI_ENUM_DEPRECATED)) {
>> +        switch (v->compat_policy.deprecated_input) {
>> +        case COMPAT_POLICY_INPUT_ACCEPT:
>> +            break;
>> +        case COMPAT_POLICY_INPUT_REJECT:
>> +            error_setg(errp, "Deprecated value '%s' disabled by policy",
>> +                       enum_str);
>> +            return false;
>> +        case COMPAT_POLICY_INPUT_CRASH:
>> +        default:
>> +            abort();
>> +        }
>> +    }
>> +
>>      *obj = value;
>>      return true;
>>  }
>
> Grammar fixes are minor, so:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

[PATCH RFC v2 5/5] block: Deprecate transaction type drive-backup

[Markus Armbruster]

Several moons ago, Vladimir posted

    Subject: [PATCH v2 3/3] qapi: deprecate drive-backup
    Date: Wed,  5 May 2021 16:58:03 +0300
    Message-Id: <20210505135803.67896-4-vsementsov@virtuozzo.com>
    https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01394.html

with this

    TODO: We also need to deprecate drive-backup transaction action..
    But union members in QAPI doesn't support 'deprecated' feature. I tried
    to dig a bit, but failed :/ Markus, could you please help with it? At
    least by advice?

This is one way to resolve it.  Sorry it took so long.

John explored another way, namely adding feature flags to union
branches.  Could also be useful, say to add different features to
branches in multiple unions sharing the same tag enum.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/transaction.json | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/qapi/transaction.json b/qapi/transaction.json
index d175b5f863..0564a893b3 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -54,6 +54,9 @@
 # @blockdev-snapshot-sync: since 1.1
 # @drive-backup: Since 1.6
 #
+# Features:
+# @deprecated: Member @drive-backup is deprecated.  Use FIXME instead.
+#
 # Since: 1.1
 ##
 { 'enum': 'TransactionActionKind',
@@ -62,7 +65,7 @@
             'block-dirty-bitmap-disable', 'block-dirty-bitmap-merge',
             'blockdev-backup', 'blockdev-snapshot',
             'blockdev-snapshot-internal-sync', 'blockdev-snapshot-sync',
-            'drive-backup' ] }
+            { 'name': 'drive-backup', 'features': [ 'deprecated' ] } ] }
 
 ##
 # @AbortWrapper:
-- 
2.31.1

Re: [PATCH RFC v2 5/5] block: Deprecate transaction type drive-backup

[Eric Blake]

On Sat, Oct 09, 2021 at 02:09:44PM +0200, Markus Armbruster wrote:
> Several moons ago, Vladimir posted
> 
>     Subject: [PATCH v2 3/3] qapi: deprecate drive-backup
>     Date: Wed,  5 May 2021 16:58:03 +0300
>     Message-Id: <20210505135803.67896-4-vsementsov@virtuozzo.com>
>     https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01394.html
> 
> with this
> 
>     TODO: We also need to deprecate drive-backup transaction action..
>     But union members in QAPI doesn't support 'deprecated' feature. I tried
>     to dig a bit, but failed :/ Markus, could you please help with it? At
>     least by advice?
> 
> This is one way to resolve it.  Sorry it took so long.
> 
> John explored another way, namely adding feature flags to union
> branches.  Could also be useful, say to add different features to
> branches in multiple unions sharing the same tag enum.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi/transaction.json | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/transaction.json b/qapi/transaction.json
> index d175b5f863..0564a893b3 100644
> --- a/qapi/transaction.json
> +++ b/qapi/transaction.json
> @@ -54,6 +54,9 @@
>  # @blockdev-snapshot-sync: since 1.1
>  # @drive-backup: Since 1.6
>  #
> +# Features:
> +# @deprecated: Member @drive-backup is deprecated.  Use FIXME instead.

Obviously, we'd need to flesh this out ("'blockdev-backup' with proper
node names"? something else?) before dropping RFC on this patch.

And we'd want to edit docs/about/deprecated.rst to match.

> +#
>  # Since: 1.1
>  ##
>  { 'enum': 'TransactionActionKind',
> @@ -62,7 +65,7 @@
>              'block-dirty-bitmap-disable', 'block-dirty-bitmap-merge',
>              'blockdev-backup', 'blockdev-snapshot',
>              'blockdev-snapshot-internal-sync', 'blockdev-snapshot-sync',
> -            'drive-backup' ] }
> +            { 'name': 'drive-backup', 'features': [ 'deprecated' ] } ] }
>  
>  ##
>  # @AbortWrapper:
> -- 
> 2.31.1
>

But the idea is reasonable, and I'm not sure if we're any closer to
John's idea of feature flags on union branches.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH RFC v2 5/5] block: Deprecate transaction type drive-backup

[Kevin Wolf]

Am 11.10.2021 um 20:58 hat Eric Blake geschrieben:
> On Sat, Oct 09, 2021 at 02:09:44PM +0200, Markus Armbruster wrote:
> > Several moons ago, Vladimir posted
> > 
> >     Subject: [PATCH v2 3/3] qapi: deprecate drive-backup
> >     Date: Wed,  5 May 2021 16:58:03 +0300
> >     Message-Id: <20210505135803.67896-4-vsementsov@virtuozzo.com>
> >     https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01394.html
> > 
> > with this
> > 
> >     TODO: We also need to deprecate drive-backup transaction action..
> >     But union members in QAPI doesn't support 'deprecated' feature. I tried
> >     to dig a bit, but failed :/ Markus, could you please help with it? At
> >     least by advice?
> > 
> > This is one way to resolve it.  Sorry it took so long.
> > 
> > John explored another way, namely adding feature flags to union
> > branches.  Could also be useful, say to add different features to
> > branches in multiple unions sharing the same tag enum.
> > 
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >  qapi/transaction.json | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi/transaction.json b/qapi/transaction.json
> > index d175b5f863..0564a893b3 100644
> > --- a/qapi/transaction.json
> > +++ b/qapi/transaction.json
> > @@ -54,6 +54,9 @@
> >  # @blockdev-snapshot-sync: since 1.1
> >  # @drive-backup: Since 1.6
> >  #
> > +# Features:
> > +# @deprecated: Member @drive-backup is deprecated.  Use FIXME instead.
> 
> Obviously, we'd need to flesh this out ("'blockdev-backup' with proper
> node names"? something else?) before dropping RFC on this patch.

What does 'blockdev-backup' with improper node names look like?

I think it's sufficient to say "Use @blockdev-backup instead", which is
already documented to take a node/device name instead of a file name.

> And we'd want to edit docs/about/deprecated.rst to match.

Yes.

Kevin

Re: [PATCH RFC v2 5/5] block: Deprecate transaction type drive-backup

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 11.10.2021 um 20:58 hat Eric Blake geschrieben:
>> On Sat, Oct 09, 2021 at 02:09:44PM +0200, Markus Armbruster wrote:
>> > Several moons ago, Vladimir posted
>> > 
>> >     Subject: [PATCH v2 3/3] qapi: deprecate drive-backup
>> >     Date: Wed,  5 May 2021 16:58:03 +0300
>> >     Message-Id: <20210505135803.67896-4-vsementsov@virtuozzo.com>
>> >     https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01394.html
>> > 
>> > with this
>> > 
>> >     TODO: We also need to deprecate drive-backup transaction action..
>> >     But union members in QAPI doesn't support 'deprecated' feature. I tried
>> >     to dig a bit, but failed :/ Markus, could you please help with it? At
>> >     least by advice?
>> > 
>> > This is one way to resolve it.  Sorry it took so long.
>> > 
>> > John explored another way, namely adding feature flags to union
>> > branches.  Could also be useful, say to add different features to
>> > branches in multiple unions sharing the same tag enum.
>> > 
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> > ---
>> >  qapi/transaction.json | 5 ++++-
>> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/qapi/transaction.json b/qapi/transaction.json
>> > index d175b5f863..0564a893b3 100644
>> > --- a/qapi/transaction.json
>> > +++ b/qapi/transaction.json
>> > @@ -54,6 +54,9 @@
>> >  # @blockdev-snapshot-sync: since 1.1
>> >  # @drive-backup: Since 1.6
>> >  #
>> > +# Features:
>> > +# @deprecated: Member @drive-backup is deprecated.  Use FIXME instead.
>> 
>> Obviously, we'd need to flesh this out ("'blockdev-backup' with proper
>> node names"? something else?) before dropping RFC on this patch.
>
> What does 'blockdev-backup' with improper node names look like?
>
> I think it's sufficient to say "Use @blockdev-backup instead", which is
> already documented to take a node/device name instead of a file name.

Sold!

>> And we'd want to edit docs/about/deprecated.rst to match.
>
> Yes.

My patch makes sense only together with Vladimir's patch to deprecate
the drive-backup command.  That one updates deprecated.rst.  The
combination of the two might need further tweaking, but let's not worry
about that now.

Re: [PATCH v2 0/5] qapi: Add feature flags to enum members

[Peter Krempa]

On Sat, Oct 09, 2021 at 14:09:39 +0200, Markus Armbruster wrote:

I've tested this with my libvirt patches which make use of the new way
enum members are returned as well as code which validates whether
libvirt uses deprecated values and everything works as expected, thus

Series:

Tested-by: Peter Krempa <pkrempa@redhat.com>

> Markus Armbruster (5):
>   qapi: Enable enum member introspection to show more than name
>   qapi: Add feature flags to enum members
>   qapi: Move compat policy from QObject to generic visitor
>   qapi: Implement deprecated-input={reject,crash} for enum values
>   block: Deprecate transaction type drive-backup

In addition, libvirt never used 'drive-backup' so an additional

ACKed-by: Peter Krempa <pkrempa@redhat.com>

for the last patch.