* [PATCH RFC 1/5] qapi: Enable enum member introspection to show more than name
2021-09-15 19:24 [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members Markus Armbruster
@ 2021-09-15 19:24 ` Markus Armbruster
2021-09-17 13:56 ` Eric Blake
2021-09-17 14:49 ` Peter Krempa
2021-09-15 19:24 ` [PATCH RFC 2/5] qapi: Add feature flags to enum members Markus Armbruster
` (4 subsequent siblings)
5 siblings, 2 replies; 19+ messages in thread
From: Markus Armbruster @ 2021-09-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, vsementsov, berrange, libvir-list, eblake, mdroth,
pkrempa, marcandre.lureau, jsnow, libguestfs
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. We should be able to get rid of it
eventually.
In my testing, output of qemu-system-x86_64's query-qmp-schema
grows by 11% (18.5KiB).
2. Like 1, but omit "boring" elements of @member, and empty @member.
@values does not become redundant. Output of query-qmp-schema
grows only as we make enum members non-boring.
3. Versioned query-qmp-schema.
query-qmp-schema provides either @values or @members. The QMP
client can select which version it wants.
This commit implements 1. simply because it's the solution I thought
of first. I'm prepared to implement one of the others if we decide
that's what we want.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qapi/introspect.json | 20 ++++++++++++++++++--
scripts/qapi/introspect.py | 18 ++++++++++++++----
2 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/qapi/introspect.json b/qapi/introspect.json
index 39bd303778..250748cd95 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -142,14 +142,30 @@
#
# 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.
+#
+# @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.1
+##
+{ '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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH RFC 1/5] qapi: Enable enum member introspection to show more than name
2021-09-15 19:24 ` [PATCH RFC 1/5] qapi: Enable enum member introspection to show more than name Markus Armbruster
@ 2021-09-17 13:56 ` Eric Blake
2021-09-20 8:57 ` Markus Armbruster
2021-09-17 14:49 ` Peter Krempa
1 sibling, 1 reply; 19+ messages in thread
From: Eric Blake @ 2021-09-17 13:56 UTC (permalink / raw)
To: Markus Armbruster
Cc: kwolf, vsementsov, berrange, libvir-list, qemu-devel, mdroth,
pkrempa, marcandre.lureau, jsnow, libguestfs
On Wed, Sep 15, 2021 at 09:24:21PM +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.
>
> 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. We should be able to get rid of it
> eventually.
>
> In my testing, output of qemu-system-x86_64's query-qmp-schema
> grows by 11% (18.5KiB).
This makes sense if we plan to deprecate @values - if so, that
deprecation would make sense as part of this series, although we may
drag our feet for how long before we actually remove it.
>
> 2. Like 1, but omit "boring" elements of @member, and empty @member.
>
> @values does not become redundant. Output of query-qmp-schema
> grows only as we make enum members non-boring.
Does not change whether libvirt would have to learn to query the new
members, but adds a mandatory fallback step for learning about boring
members (although the fallback step will have to be there anyway for
older qemu). Peter probably has a better idea of which version is
nicer.
>
> 3. Versioned query-qmp-schema.
>
> query-qmp-schema provides either @values or @members. The QMP
> client can select which version it wants.
Sounds more complicated to implement. I'm not opposed to it, but am
leaning towards 1 or 2 myself.
>
> This commit implements 1. simply because it's the solution I thought
> of first. I'm prepared to implement one of the others if we decide
> that's what we want.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> qapi/introspect.json | 20 ++++++++++++++++++--
> scripts/qapi/introspect.py | 18 ++++++++++++++----
> 2 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index 39bd303778..250748cd95 100644
> --- a/qapi/introspect.json
> +++ b/qapi/introspect.json
> @@ -142,14 +142,30 @@
> #
> # 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.
Missing a '(since 6.2)' tag.
> +#
> +# @values: the enumeration type's member names, in no particular order.
> +# Redundant with @members. Just for backward compatibility.
Worth marking as deprecated in this patch, or in a followup?
> #
> # 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.1
6.2
> +##
> +{ 'struct': 'SchemaInfoEnumMember',
> + 'data': { 'name': 'str' } }
>
Definitely more flexible.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH RFC 1/5] qapi: Enable enum member introspection to show more than name
2021-09-17 13:56 ` Eric Blake
@ 2021-09-20 8:57 ` Markus Armbruster
0 siblings, 0 replies; 19+ messages in thread
From: Markus Armbruster @ 2021-09-20 8:57 UTC (permalink / raw)
To: Eric Blake
Cc: kwolf, vsementsov, berrange, qemu-devel, mdroth,
Markus Armbruster, libvir-list, pkrempa, marcandre.lureau, jsnow,
libguestfs
Eric Blake <eblake@redhat.com> writes:
> On Wed, Sep 15, 2021 at 09:24:21PM +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.
>>
>> 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. We should be able to get rid of it
>> eventually.
>>
>> In my testing, output of qemu-system-x86_64's query-qmp-schema
>> grows by 11% (18.5KiB).
>
> This makes sense if we plan to deprecate @values - if so, that
> deprecation would make sense as part of this series, although we may
> drag our feet for how long before we actually remove it.
Yes. Changing query-qmp-schema requires extra care, as it is the very
means for coping with change.
>>
>> 2. Like 1, but omit "boring" elements of @member, and empty @member.
>>
>> @values does not become redundant. Output of query-qmp-schema
>> grows only as we make enum members non-boring.
>
> Does not change whether libvirt would have to learn to query the new
> members, but adds a mandatory fallback step for learning about boring
> members (although the fallback step will have to be there anyway for
> older qemu). Peter probably has a better idea of which version is
> nicer.
>
>>
>> 3. Versioned query-qmp-schema.
>>
>> query-qmp-schema provides either @values or @members. The QMP
>> client can select which version it wants.
>
> Sounds more complicated to implement. I'm not opposed to it, but am
> leaning towards 1 or 2 myself.
More on this in my reply to Peter.
>
>>
>> This commit implements 1. simply because it's the solution I thought
>> of first. I'm prepared to implement one of the others if we decide
>> that's what we want.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> qapi/introspect.json | 20 ++++++++++++++++++--
>> scripts/qapi/introspect.py | 18 ++++++++++++++----
>> 2 files changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/qapi/introspect.json b/qapi/introspect.json
>> index 39bd303778..250748cd95 100644
>> --- a/qapi/introspect.json
>> +++ b/qapi/introspect.json
>> @@ -142,14 +142,30 @@
>> #
>> # 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.
>
> Missing a '(since 6.2)' tag.
Yes.
>> +#
>> +# @values: the enumeration type's member names, in no particular order.
>> +# Redundant with @members. Just for backward compatibility.
>
> Worth marking as deprecated in this patch, or in a followup?
If we intend to deprecate, we can just as well do it right away.
>> #
>> # 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.1
>
> 6.2
Whoops!
>> +##
>> +{ 'struct': 'SchemaInfoEnumMember',
>> + 'data': { 'name': 'str' } }
>>
>
> Definitely more flexible.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/5] qapi: Enable enum member introspection to show more than name
2021-09-15 19:24 ` [PATCH RFC 1/5] qapi: Enable enum member introspection to show more than name Markus Armbruster
2021-09-17 13:56 ` Eric Blake
@ 2021-09-17 14:49 ` Peter Krempa
2021-09-20 9:08 ` Markus Armbruster
1 sibling, 1 reply; 19+ messages in thread
From: Peter Krempa @ 2021-09-17 14:49 UTC (permalink / raw)
To: Markus Armbruster
Cc: kwolf, vsementsov, berrange, libvir-list, eblake, mdroth,
qemu-devel, marcandre.lureau, jsnow, libguestfs
On Wed, Sep 15, 2021 at 21:24:21 +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.
>
> 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. We should be able to get rid of it
> eventually.
>
> In my testing, output of qemu-system-x86_64's query-qmp-schema
> grows by 11% (18.5KiB).
I prefer this one. While the schema output grows, nobody is really
reading it manually.
The implementation in libvirt is very straightforward:
https://gitlab.com/pipo.sk/libvirt/-/commit/24f1709cfae72cd765d02dc2124d6c954554ea55
https://gitlab.com/pipo.sk/libvirt/-/commit/5909db9d4994327b3f64d5c329edd4b175495bfe
> 2. Like 1, but omit "boring" elements of @member, and empty @member.
>
> @values does not become redundant. Output of query-qmp-schema
> grows only as we make enum members non-boring.
This has 2 drawbacks:
- we would never get rid of either of those
- clients would have to check both, one for whether the member is
present and second whether it's non-boring.
IMO it's simpler for clients just to prefer the new approach when
present as the old is simply a subset.
> 3. Versioned query-qmp-schema.
>
> query-qmp-schema provides either @values or @members. The QMP
> client can select which version it wants.
At least for libvirt this poses a chicken & egg problem. We'd have to
query the schema to see that it has the switch to do the selection and
then probe with the modern one.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/5] qapi: Enable enum member introspection to show more than name
2021-09-17 14:49 ` Peter Krempa
@ 2021-09-20 9:08 ` Markus Armbruster
2021-09-20 9:57 ` Peter Krempa
0 siblings, 1 reply; 19+ messages in thread
From: Markus Armbruster @ 2021-09-20 9:08 UTC (permalink / raw)
To: Peter Krempa
Cc: kwolf, vsementsov, berrange, libvir-list, eblake, mdroth,
qemu-devel, marcandre.lureau, jsnow, libguestfs
Peter Krempa <pkrempa@redhat.com> writes:
> On Wed, Sep 15, 2021 at 21:24:21 +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.
>>
>> 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. We should be able to get rid of it
>> eventually.
>>
>> In my testing, output of qemu-system-x86_64's query-qmp-schema
>> grows by 11% (18.5KiB).
>
> I prefer this one. While the schema output grows, nobody is really
> reading it manually.
True, but growing schema output can only slow down client startup.
Negligible for libvirt, I presume?
> The implementation in libvirt is very straightforward:
>
> https://gitlab.com/pipo.sk/libvirt/-/commit/24f1709cfae72cd765d02dc2124d6c954554ea55
> https://gitlab.com/pipo.sk/libvirt/-/commit/5909db9d4994327b3f64d5c329edd4b175495bfe
>
>> 2. Like 1, but omit "boring" elements of @member, and empty @member.
>>
>> @values does not become redundant. Output of query-qmp-schema
>> grows only as we make enum members non-boring.
>
> This has 2 drawbacks:
> - we would never get rid of either of those
> - clients would have to check both, one for whether the member is
> present and second whether it's non-boring.
>
> IMO it's simpler for clients just to prefer the new approach when
> present as the old is simply a subset.
Noted.
>> 3. Versioned query-qmp-schema.
>>
>> query-qmp-schema provides either @values or @members. The QMP
>> client can select which version it wants.
>
> At least for libvirt this poses a chicken & egg problem. We'd have to
> query the schema to see that it has the switch to do the selection and
> then probe with the modern one.
The simplest solution is to try the versions the management application
can understand in order of preference (newest to oldest) until one
succeeds. I'd expect the first try to work most of the time. Only when
you combine new libvirt with old QEMU, the fallback has to kick in.
Other parts of the management application should remain oblivous of the
differences.
We could of course try to reduce the number of roundtrips, say by
putting sufficient information into the QMP greeting (one roundtrip), or
the output of query-qmp-schema (try oldest to find the best one, then
try the best one unless it's the oldest). I doubt that's worthwhile.
I'm not trying to talk you into this solution. We're just exploring the
solution space together, and with an open mind.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/5] qapi: Enable enum member introspection to show more than name
2021-09-20 9:08 ` Markus Armbruster
@ 2021-09-20 9:57 ` Peter Krempa
2021-10-09 8:08 ` Markus Armbruster
0 siblings, 1 reply; 19+ messages in thread
From: Peter Krempa @ 2021-09-20 9:57 UTC (permalink / raw)
To: Markus Armbruster
Cc: kwolf, vsementsov, berrange, libvir-list, eblake, mdroth,
qemu-devel, marcandre.lureau, jsnow, libguestfs
On Mon, Sep 20, 2021 at 11:08:59 +0200, Markus Armbruster wrote:
> Peter Krempa <pkrempa@redhat.com> writes:
>
> > On Wed, Sep 15, 2021 at 21:24:21 +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.
> >>
> >> 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. We should be able to get rid of it
> >> eventually.
> >>
> >> In my testing, output of qemu-system-x86_64's query-qmp-schema
> >> grows by 11% (18.5KiB).
> >
> > I prefer this one. While the schema output grows, nobody is really
> > reading it manually.
>
> True, but growing schema output can only slow down client startup.
> Negligible for libvirt, I presume?
Libvirt employs caching, so unless it's the first VM started after a
qemu/libvirt upgrade, the results are already processed and cached.
In fact we don't even keep the full schema around, we just extract
information and store them as capability bits. For now we didn't run
into the need to have the full schema around when starting a VM.
[...]
> >> 3. Versioned query-qmp-schema.
> >>
> >> query-qmp-schema provides either @values or @members. The QMP
> >> client can select which version it wants.
> >
> > At least for libvirt this poses a chicken & egg problem. We'd have to
> > query the schema to see that it has the switch to do the selection and
> > then probe with the modern one.
>
> The simplest solution is to try the versions the management application
> can understand in order of preference (newest to oldest) until one
> succeeds. I'd expect the first try to work most of the time. Only when
> you combine new libvirt with old QEMU, the fallback has to kick in.
>
> Other parts of the management application should remain oblivous of the
> differences.
That would certainly work and be reasonably straightforward for libvirt
to implement, but:
1) libvirt's code for using the QMP schema would be exactly the same as
with approach 1), as we need to handle old clients too and the new
way is simply a superset of what we have
2) qemu's deprecation approach itself wouldn't be any easier in either
of those scenarios
Basically the only thing this would gain us is that if the deprecation
period is over old clients which were not fixed could fail silently:
Assuming that 'query-qmp-schema' gains a boolean option such as
'fancier-enums' and setting that to true returns the new format of
schema, after the deprecation is over you could simply return an error
if a caller omits 'fancier-enums' or sets it to false, which creates a
clean cut for the removal.
With approach 1) itself, clients which were not adapted would start
lacking information based on enum values.
Now for those it depends on how they actually handled it until now. E.g.
old libvirt would report that the QMP schema is broken if 'values' would
be missing.
Whether that's a worthwhile thing to do? I'm not really persuaded. (And
I'm biased since libvirt handles it correctly).
> We could of course try to reduce the number of roundtrips, say by
> putting sufficient information into the QMP greeting (one roundtrip), or
> the output of query-qmp-schema (try oldest to find the best one, then
> try the best one unless it's the oldest). I doubt that's worthwhile.
In this particular scenario, I'd doubt that it's worthwhile as the
change isn't really fundamental and issuing one extra QMP call isn't as
problematic as other cases, e.g probing of CPU features which results in
a QMP call per feature when starting a VM.
Currently libvirt issues 50 + 5 QMP commands(kvm, and non-kvm) for
probing capabilities.
> I'm not trying to talk you into this solution. We're just exploring the
> solution space together, and with an open mind.
The idea of unconditionally trying a newer approach is a good one to
hold onto when we'll need it in the future!
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH RFC 1/5] qapi: Enable enum member introspection to show more than name
2021-09-20 9:57 ` Peter Krempa
@ 2021-10-09 8:08 ` Markus Armbruster
0 siblings, 0 replies; 19+ messages in thread
From: Markus Armbruster @ 2021-10-09 8:08 UTC (permalink / raw)
To: Peter Krempa
Cc: kwolf, vsementsov, berrange, libvir-list, eblake, mdroth,
qemu-devel, marcandre.lureau, jsnow, libguestfs
Peter Krempa <pkrempa@redhat.com> writes:
> On Mon, Sep 20, 2021 at 11:08:59 +0200, Markus Armbruster wrote:
>> Peter Krempa <pkrempa@redhat.com> writes:
>>
>> > On Wed, Sep 15, 2021 at 21:24:21 +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.
>> >>
>> >> 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. We should be able to get rid of it
>> >> eventually.
>> >>
>> >> In my testing, output of qemu-system-x86_64's query-qmp-schema
>> >> grows by 11% (18.5KiB).
>> >
>> > I prefer this one. While the schema output grows, nobody is really
>> > reading it manually.
>>
>> True, but growing schema output can only slow down client startup.
>> Negligible for libvirt, I presume?
>
> Libvirt employs caching, so unless it's the first VM started after a
> qemu/libvirt upgrade, the results are already processed and cached.
Good!
> In fact we don't even keep the full schema around, we just extract
> information and store them as capability bits. For now we didn't run
> into the need to have the full schema around when starting a VM.
>
> [...]
>
>> >> 3. Versioned query-qmp-schema.
>> >>
>> >> query-qmp-schema provides either @values or @members. The QMP
>> >> client can select which version it wants.
>> >
>> > At least for libvirt this poses a chicken & egg problem. We'd have to
>> > query the schema to see that it has the switch to do the selection and
>> > then probe with the modern one.
>>
>> The simplest solution is to try the versions the management application
>> can understand in order of preference (newest to oldest) until one
>> succeeds. I'd expect the first try to work most of the time. Only when
>> you combine new libvirt with old QEMU, the fallback has to kick in.
>>
>> Other parts of the management application should remain oblivous of the
>> differences.
>
> That would certainly work and be reasonably straightforward for libvirt
> to implement, but:
> 1) libvirt's code for using the QMP schema would be exactly the same as
> with approach 1), as we need to handle old clients too and the new
> way is simply a superset of what we have
Yes, libvirt would need the same code for processing old and new. The
only difference would be how it decides which method to use. With 1,
it's "if @members is present, use it, else @values". With 2, it's "if
the version we use is new enough, use @members, else @values".
> 2) qemu's deprecation approach itself wouldn't be any easier in either
> of those scenarios
>
> Basically the only thing this would gain us is that if the deprecation
> period is over old clients which were not fixed could fail silently:
>
> Assuming that 'query-qmp-schema' gains a boolean option such as
> 'fancier-enums' and setting that to true returns the new format of
> schema, after the deprecation is over you could simply return an error
> if a caller omits 'fancier-enums' or sets it to false, which creates a
> clean cut for the removal.
Yes.
> With approach 1) itself, clients which were not adapted would start
> lacking information based on enum values.
>
> Now for those it depends on how they actually handled it until now. E.g.
> old libvirt would report that the QMP schema is broken if 'values' would
> be missing.
Which I consider the sensible thing to do.
> Whether that's a worthwhile thing to do? I'm not really persuaded. (And
> I'm biased since libvirt handles it correctly).
I think 3 has the following advantages over 1:
* As you noted, it ensures outmoded clients fail cleanly. Not much of
an advantage for clients that handle missing @values sensibly.
Perhaps it could enable better error messages.
* It avoids duplicated contents in old an new format. Not much of an
advantage for clients that cache their schema interrogation.
* It can enable more radical introspection changes. Without versioning,
the common rules for compatible evolution apply (section
"Compatibility considerations" in qapi-code-gen.rst). With
versioning, they don't.
I agree this is not really compelling just for the problem at hand. We
can reconsider when we run into more problems.
>> We could of course try to reduce the number of roundtrips, say by
>> putting sufficient information into the QMP greeting (one roundtrip), or
>> the output of query-qmp-schema (try oldest to find the best one, then
>> try the best one unless it's the oldest). I doubt that's worthwhile.
>
> In this particular scenario, I'd doubt that it's worthwhile as the
> change isn't really fundamental and issuing one extra QMP call isn't as
> problematic as other cases, e.g probing of CPU features which results in
> a QMP call per feature when starting a VM.
>
> Currently libvirt issues 50 + 5 QMP commands(kvm, and non-kvm) for
> probing capabilities.
>
>> I'm not trying to talk you into this solution. We're just exploring the
>> solution space together, and with an open mind.
>
> The idea of unconditionally trying a newer approach is a good one to
> hold onto when we'll need it in the future!
Only where the failure modes are simple enough to make misinterpretation
basically impossible.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC 2/5] qapi: Add feature flags to enum members
2021-09-15 19:24 [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members Markus Armbruster
2021-09-15 19:24 ` [PATCH RFC 1/5] qapi: Enable enum member introspection to show more than name Markus Armbruster
@ 2021-09-15 19:24 ` Markus Armbruster
2021-09-17 14:41 ` Eric Blake
2021-09-17 14:53 ` Peter Krempa
2021-09-15 19:24 ` [PATCH RFC 3/5] qapi: Move compat policy from QObject to generic visitor Markus Armbruster
` (3 subsequent siblings)
5 siblings, 2 replies; 19+ messages in thread
From: Markus Armbruster @ 2021-09-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, vsementsov, berrange, libvir-list, eblake, mdroth,
pkrempa, marcandre.lureau, jsnow, libguestfs
This is quite similar to commit 84ab008687 "qapi: Add feature flags to
struct members", only for enums instead of structs.
Signed-off-by: Markus Armbruster <armbru@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 250748cd95..e1219914c9 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -162,10 +162,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.1
##
{ '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 a20acffd8b..ce05bc3eac 100644
--- a/tests/qapi-schema/doc-good.json
+++ b/tests/qapi-schema/doc-good.json
@@ -57,11 +57,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 b10490ccc6..c30b9ab94c 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 0b49dc3044..6de54507e8 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 73cffae2b6..46b41baa3c 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH RFC 2/5] qapi: Add feature flags to enum members
2021-09-15 19:24 ` [PATCH RFC 2/5] qapi: Add feature flags to enum members Markus Armbruster
@ 2021-09-17 14:41 ` Eric Blake
2021-09-17 14:53 ` Peter Krempa
1 sibling, 0 replies; 19+ messages in thread
From: Eric Blake @ 2021-09-17 14:41 UTC (permalink / raw)
To: Markus Armbruster
Cc: kwolf, vsementsov, berrange, libvir-list, qemu-devel, mdroth,
pkrempa, marcandre.lureau, jsnow, libguestfs
On Wed, Sep 15, 2021 at 09:24:22PM +0200, Markus Armbruster wrote:
> This is quite similar to commit 84ab008687 "qapi: Add feature flags to
> struct members", only for enums instead of structs.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 2/5] qapi: Add feature flags to enum members
2021-09-15 19:24 ` [PATCH RFC 2/5] qapi: Add feature flags to enum members Markus Armbruster
2021-09-17 14:41 ` Eric Blake
@ 2021-09-17 14:53 ` Peter Krempa
1 sibling, 0 replies; 19+ messages in thread
From: Peter Krempa @ 2021-09-17 14:53 UTC (permalink / raw)
To: Markus Armbruster
Cc: kwolf, vsementsov, berrange, libvir-list, eblake, mdroth,
qemu-devel, marcandre.lureau, jsnow, libguestfs
On Wed, Sep 15, 2021 at 21:24:22 +0200, Markus Armbruster wrote:
> This is quite similar to commit 84ab008687 "qapi: Add feature flags to
> struct members", only for enums instead of structs.
For now in libvirt I've implemented the validator for the 'deprecated'
flag:
https://gitlab.com/pipo.sk/libvirt/-/commit/1fc69fd78fbca8b93287e368f622abfcf1464b6c
I'm not sure for now whether we'll have a use case for probing
non-deprecation features, but extending the query mechanism in libvirt
will be simple if we'll do need to do that.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC 3/5] qapi: Move compat policy from QObject to generic visitor
2021-09-15 19:24 [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members Markus Armbruster
2021-09-15 19:24 ` [PATCH RFC 1/5] qapi: Enable enum member introspection to show more than name Markus Armbruster
2021-09-15 19:24 ` [PATCH RFC 2/5] qapi: Add feature flags to enum members Markus Armbruster
@ 2021-09-15 19:24 ` Markus Armbruster
2021-09-17 14:45 ` Eric Blake
2021-09-15 19:24 ` [PATCH RFC 4/5] qapi: Implement deprecated-input={reject, crash} for enum values Markus Armbruster
` (2 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Markus Armbruster @ 2021-09-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, vsementsov, berrange, libvir-list, eblake, mdroth,
pkrempa, marcandre.lureau, jsnow, libguestfs
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>
---
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH RFC 3/5] qapi: Move compat policy from QObject to generic visitor
2021-09-15 19:24 ` [PATCH RFC 3/5] qapi: Move compat policy from QObject to generic visitor Markus Armbruster
@ 2021-09-17 14:45 ` Eric Blake
0 siblings, 0 replies; 19+ messages in thread
From: Eric Blake @ 2021-09-17 14:45 UTC (permalink / raw)
To: Markus Armbruster
Cc: kwolf, vsementsov, berrange, libvir-list, qemu-devel, mdroth,
pkrempa, marcandre.lureau, jsnow, libguestfs
On Wed, Sep 15, 2021 at 09:24:23PM +0200, Markus Armbruster wrote:
> 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>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC 4/5] qapi: Implement deprecated-input={reject, crash} for enum values
2021-09-15 19:24 [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members Markus Armbruster
` (2 preceding siblings ...)
2021-09-15 19:24 ` [PATCH RFC 3/5] qapi: Move compat policy from QObject to generic visitor Markus Armbruster
@ 2021-09-15 19:24 ` Markus Armbruster
2021-09-15 19:24 ` [PATCH RFC 5/5] block: Deprecate transaction type drive-backup Markus Armbruster
2021-09-16 7:18 ` [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members Vladimir Sementsov-Ogievskiy
5 siblings, 0 replies; 19+ messages in thread
From: Markus Armbruster @ 2021-09-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, vsementsov, berrange, libvir-list, eblake, mdroth,
pkrempa, marcandre.lureau, jsnow, libguestfs
This copies the code implementing the policy from qapi/qmp-dispatch.c
to qapi/qobject-input-visitor.c. I hope to avoid that in a future
revision.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qapi/compat.json | 3 ++-
include/qapi/util.h | 6 +++++-
qapi/qapi-visit-core.c | 18 +++++++++++++++---
scripts/qapi/types.py | 17 ++++++++++++++++-
4 files changed, 38 insertions(+), 6 deletions(-)
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH RFC 5/5] block: Deprecate transaction type drive-backup
2021-09-15 19:24 [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members Markus Armbruster
` (3 preceding siblings ...)
2021-09-15 19:24 ` [PATCH RFC 4/5] qapi: Implement deprecated-input={reject, crash} for enum values Markus Armbruster
@ 2021-09-15 19:24 ` Markus Armbruster
2021-09-16 7:18 ` [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members Vladimir Sementsov-Ogievskiy
5 siblings, 0 replies; 19+ messages in thread
From: Markus Armbruster @ 2021-09-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, vsementsov, berrange, libvir-list, eblake, mdroth,
pkrempa, marcandre.lureau, jsnow, libguestfs
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 d7fc73d7df..0be6f83f6d 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -41,6 +41,9 @@
##
# @TransactionActionKind:
#
+# Features:
+# @deprecated: Member @drive-backup is deprecated. Use FIXME instead.
+#
# Since: 6.1
##
{ 'enum': 'TransactionActionKind',
@@ -49,7 +52,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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members
2021-09-15 19:24 [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members Markus Armbruster
` (4 preceding siblings ...)
2021-09-15 19:24 ` [PATCH RFC 5/5] block: Deprecate transaction type drive-backup Markus Armbruster
@ 2021-09-16 7:18 ` Vladimir Sementsov-Ogievskiy
2021-09-16 12:57 ` Markus Armbruster
5 siblings, 1 reply; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-16 7:18 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: eblake, jsnow, marcandre.lureau, mdroth, libguestfs, libvir-list,
berrange, pkrempa, kwolf
Great! Thanks for working on this!
15.09.2021 22:24, Markus Armbruster wrote:
> PATCH 1+2 add feature flags to enum members. Awkward due to an
> introspection design mistake; see PATCH 1 for details. Feedback
> welcome, in particular from management application guys.
>
> 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=crash to help us catch
> unwanted use of deprecated enum values. Thoughts?
I think crash policy for output is doubtful. If we have query-* command that returns a lot of things and some of them are deprecated the whole command will always crash.. Deprecated is "use" of the deprecated field, but we can't control does user use a specific field or not.
If some deprecated output is a consequence of deprecated input, we'd better always show it.. Or in other words, we shouldn't deprecate such output, deprecating of the corresponding input is enough.
So, we are saying about showing some internal state to the user. And part of this state becomes deprecated because internal implementation changed. I think the only correct thing to do is handle deprecated=hide by hand, in the place where we set this deprecated field. Only at this place we can decide, should we simulate old deprecated output value or not. For this we'll need a possibility to get current policy at any place, but that doesn't seem to be a problem, I see, it's just a global variable compat_policy.
>
> PATCH 5 puts the new feature flags to use. It makes sense only on top
> of Vladimir's deprecation of drive-backup. See its commit message for
> a reference.
>
> Based on my "[PATCH 00/22] qapi: Remove simple unions from the schema
> language".
>
> Based-on: Message-Id: <20210913123932.3306639-1-armbru@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
>
> docs/devel/qapi-code-gen.rst | 4 ++-
> qapi/compat.json | 3 +++
> qapi/introspect.json | 23 ++++++++++++++--
> 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, 144 insertions(+), 55 deletions(-)
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members
2021-09-16 7:18 ` [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members Vladimir Sementsov-Ogievskiy
@ 2021-09-16 12:57 ` Markus Armbruster
2021-09-16 13:28 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 19+ messages in thread
From: Markus Armbruster @ 2021-09-16 12:57 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, pkrempa, berrange, libvir-list, eblake, mdroth,
qemu-devel, marcandre.lureau, jsnow, libguestfs
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> Great! Thanks for working on this!
>
> 15.09.2021 22:24, Markus Armbruster wrote:
>> PATCH 1+2 add feature flags to enum members. Awkward due to an
>> introspection design mistake; see PATCH 1 for details. Feedback
>> welcome, in particular from management application guys.
>> 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=crash to help us catch
>> unwanted use of deprecated enum values. Thoughts?
>
> I think crash policy for output is doubtful. If we have query-*
> command that returns a lot of things and some of them are deprecated
> the whole command will always crash..
Policy "crash" is not quite as crazy as it may look :)
The non-default policies for handling deprecated interfaces are intended
for testing users of these interfaces.
Input policy reject and output policy hide enable "testing the future".
Input policy crash is a robust way to double-check "management
application does not send deprecated input". This is not quite the same
as "testing the future". A management application may choose to send
deprecated input, detect the failure and recover. Testing that should
pass with input policy reject, but not with input policy crash.
Output policy crash could be a way to double-check "the management
application does not make QEMU send deprecated output".
Example: Say we deprecate BlockdevDriver member 'vvfat'. We know that
output of query-named-block-nodes can contain 'vvfat' only if something
creates such nodes. Input policy reject will reject attempts to use
this driver with blockdev-add. But what if the management application
uses some other way to create such nodes, not (yet) covered by input
policy? Output policy crash could be used to double-check it doesn't.
Except it doesn't actually work, because as you said, testing will
likely produce *other* deprecated output that should *not* crash the
test.
Perhaps a policy hide-or-else-crash could work.
> Deprecated is "use" of the
> deprecated field, but we can't control does user use a specific field
> or not.
>
> If some deprecated output is a consequence of deprecated input, we'd
> better always show it.. Or in other words, we shouldn't deprecate such
> output, deprecating of the corresponding input is enough.
Deprecating only input may require duplicating definitions used both for
input and output, unless we replace today's feature flags by something
more sophisticated.
Example: BlockdevDriver is used both as input of blockdev-add and as
output of query-named-block-nodes. Deprecate member 'vvfat' affects
both input and output.
> So, we are saying about showing some internal state to the user. And
> part of this state becomes deprecated because internal implementation
> changed. I think the only correct thing to do is handle
> deprecated=hide by hand, in the place where we set this deprecated
> field. Only at this place we can decide, should we simulate old
> deprecated output value or not. For this we'll need a possibility to
> get current policy at any place, but that doesn't seem to be a
> problem, I see, it's just a global variable compat_policy.
I'm not sure I fully got this.
Compat policies are not about optionally providing older versions of an
interface ("simulate old deprecated output value"). They try to help
with testing users of interfaces. One aspect of that is optionally
approximating expected future interfaces.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members
2021-09-16 12:57 ` Markus Armbruster
@ 2021-09-16 13:28 ` Vladimir Sementsov-Ogievskiy
2021-09-16 14:12 ` Markus Armbruster
0 siblings, 1 reply; 19+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-16 13:28 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, eblake, jsnow, marcandre.lureau, mdroth, libguestfs,
libvir-list, berrange, pkrempa, kwolf
16.09.2021 15:57, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> Great! Thanks for working on this!
>>
>> 15.09.2021 22:24, Markus Armbruster wrote:
>>> PATCH 1+2 add feature flags to enum members. Awkward due to an
>>> introspection design mistake; see PATCH 1 for details. Feedback
>>> welcome, in particular from management application guys.
>>> 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=crash to help us catch
>>> unwanted use of deprecated enum values. Thoughts?
>>
>> I think crash policy for output is doubtful. If we have query-*
>> command that returns a lot of things and some of them are deprecated
>> the whole command will always crash..
>
> Policy "crash" is not quite as crazy as it may look :)
>
> The non-default policies for handling deprecated interfaces are intended
> for testing users of these interfaces.
>
> Input policy reject and output policy hide enable "testing the future".
>
> Input policy crash is a robust way to double-check "management
> application does not send deprecated input". This is not quite the same
> as "testing the future". A management application may choose to send
> deprecated input, detect the failure and recover. Testing that should
> pass with input policy reject, but not with input policy crash.
>
> Output policy crash could be a way to double-check "the management
> application does not make QEMU send deprecated output".
>
> Example: Say we deprecate BlockdevDriver member 'vvfat'. We know that
> output of query-named-block-nodes can contain 'vvfat' only if something
> creates such nodes. Input policy reject will reject attempts to use
> this driver with blockdev-add. But what if the management application
> uses some other way to create such nodes, not (yet) covered by input
> policy? Output policy crash could be used to double-check it doesn't.
>
> Except it doesn't actually work, because as you said, testing will
> likely produce *other* deprecated output that should *not* crash the
> test.
>
> Perhaps a policy hide-or-else-crash could work.
>
>> Deprecated is "use" of the
>> deprecated field, but we can't control does user use a specific field
>> or not.
>>
>> If some deprecated output is a consequence of deprecated input, we'd
>> better always show it.. Or in other words, we shouldn't deprecate such
>> output, deprecating of the corresponding input is enough.
>
> Deprecating only input may require duplicating definitions used both for
> input and output, unless we replace today's feature flags by something
> more sophisticated.
>
> Example: BlockdevDriver is used both as input of blockdev-add and as
> output of query-named-block-nodes. Deprecate member 'vvfat' affects
> both input and output.
If we deprecate a vvfat, but still have some not deprecated ways to create vvfat node, that's a kind of bug[1] in deprecation system: if vvfat is deprecated, all ways to create vvfat should go through input compat policy.
So, making qemu crash on trying to output "vvfat" for me looks like a workaround for that bug[1]. And it's strange to create and interface to workaround the internal bug..
May be, we can just enable hide-or-else-crash behavior automatically, when user choose input=crash and output=hide?
>
>> So, we are saying about showing some internal state to the user. And
>> part of this state becomes deprecated because internal implementation
>> changed. I think the only correct thing to do is handle
>> deprecated=hide by hand, in the place where we set this deprecated
>> field. Only at this place we can decide, should we simulate old
>> deprecated output value or not. For this we'll need a possibility to
>> get current policy at any place, but that doesn't seem to be a
>> problem, I see, it's just a global variable compat_policy.
>
> I'm not sure I fully got this.
>
> Compat policies are not about optionally providing older versions of an
> interface ("simulate old deprecated output value"). They try to help
> with testing users of interfaces. One aspect of that is optionally
> approximating expected future interfaces.
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members
2021-09-16 13:28 ` Vladimir Sementsov-Ogievskiy
@ 2021-09-16 14:12 ` Markus Armbruster
0 siblings, 0 replies; 19+ messages in thread
From: Markus Armbruster @ 2021-09-16 14:12 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, pkrempa, berrange, libvir-list, eblake, mdroth,
qemu-devel, marcandre.lureau, jsnow, libguestfs
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 16.09.2021 15:57, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> Great! Thanks for working on this!
>>>
>>> 15.09.2021 22:24, Markus Armbruster wrote:
>>>> PATCH 1+2 add feature flags to enum members. Awkward due to an
>>>> introspection design mistake; see PATCH 1 for details. Feedback
>>>> welcome, in particular from management application guys.
>>>> 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=crash to help us catch
>>>> unwanted use of deprecated enum values. Thoughts?
>>>
>>> I think crash policy for output is doubtful. If we have query-*
>>> command that returns a lot of things and some of them are deprecated
>>> the whole command will always crash..
>>
>> Policy "crash" is not quite as crazy as it may look :)
>>
>> The non-default policies for handling deprecated interfaces are intended
>> for testing users of these interfaces.
>>
>> Input policy reject and output policy hide enable "testing the future".
>>
>> Input policy crash is a robust way to double-check "management
>> application does not send deprecated input". This is not quite the same
>> as "testing the future". A management application may choose to send
>> deprecated input, detect the failure and recover. Testing that should
>> pass with input policy reject, but not with input policy crash.
>>
>> Output policy crash could be a way to double-check "the management
>> application does not make QEMU send deprecated output".
>>
>> Example: Say we deprecate BlockdevDriver member 'vvfat'. We know that
>> output of query-named-block-nodes can contain 'vvfat' only if something
>> creates such nodes. Input policy reject will reject attempts to use
>> this driver with blockdev-add. But what if the management application
>> uses some other way to create such nodes, not (yet) covered by input
>> policy? Output policy crash could be used to double-check it doesn't.
>>
>> Except it doesn't actually work, because as you said, testing will
>> likely produce *other* deprecated output that should *not* crash the
>> test.
>>
>> Perhaps a policy hide-or-else-crash could work.
>>
>>> Deprecated is "use" of the
>>> deprecated field, but we can't control does user use a specific field
>>> or not.
>>>
>>> If some deprecated output is a consequence of deprecated input, we'd
>>> better always show it.. Or in other words, we shouldn't deprecate such
>>> output, deprecating of the corresponding input is enough.
>>
>> Deprecating only input may require duplicating definitions used both for
>> input and output, unless we replace today's feature flags by something
>> more sophisticated.
>>
>> Example: BlockdevDriver is used both as input of blockdev-add and as
>> output of query-named-block-nodes. Deprecate member 'vvfat' affects
>> both input and output.
>
>
> If we deprecate a vvfat, but still have some not deprecated ways to
> create vvfat node, that's a kind of bug[1] in deprecation system: if
> vvfat is deprecated, all ways to create vvfat should go through input
> compat policy.
We need to distinguish between three separate things:
(1) Deprecating certain interface usage
(2) QAPI feature flag 'deprecated'
(3) Policy for handling deprecated interface usage
Note that (2) cannot cover all of (1) for two reasons, only one of them
fixable:
* Some interfaces still aren't QAPI-based. QAPIfying them all is hard.
* Feature flags only apply to *syntactic* elements, such as a command
argument.
Example for a deprecation where they don't apply: we deprecated use of
chardev-add argument "wait" together with "server": false in v4.0
(it's an error since v6.0). Use without "server": false remains okay.
Note further that (3) may cover both less and more than (2).
Before this series, it covers exactly (2). Afterwards, there is a hole:
# Limitation: deprecated-output policy @hide is not implemented for
# enumeration values. They behave the same as with policy @accept.
But it could also go beyond (2) in the future:
# Limitation: covers only syntactic aspects of QMP, i.e. stuff tagged
# with feature 'deprecated'. We may want to extend it to cover
# semantic aspects, CLI, and experimental features.
> So, making qemu crash on trying to output "vvfat" for me looks like a
> workaround for that bug[1]. And it's strange to create and interface
> to workaround the internal bug..
>
> May be, we can just enable hide-or-else-crash behavior automatically,
> when user choose input=crash and output=hide?
Hmm, interesting idea. Needs to be documented, obviously.
[...]
^ permalink raw reply [flat|nested] 19+ messages in thread