On Tue, Oct 26, 2021 at 09:15:30AM +0200, Markus Armbruster wrote:

Kashyap Chamarthy <kchamart@redhat.com> writes:

...

On Mon, Oct 25, 2021 at 07:25:24AM +0200, Markus Armbruster wrote:

[...]

...

Looks like there's another stable property with an "x-" prefix: "x-remote-object", part of QOM type @RemoteObjectProperties.

The union branch 'x-remote-object' isn't flagged 'unstable' (because union branches can't have feature flags), but the enumeration value 'x-remote-object' is. Sufficient, because you can't use the branch without using the enumeration value. Admittedly subtle.

Ah, thanks for the explanation. I missed the union branch part, which I now notice in your "[PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'".

I wrote a bit of code (appended) to make sure I don't miss names.

Thanks; looks good to me.

...

Given the above "x-" properties are now stable, I take it that they cannot be renamed now, as they might break any tools using them? My guess is the tedious way is not worth it: deprecate them, and add the non-x variants as "synonyms".

"x-use-canonical-path-for-ramblock-id" goes back to commit fa0cb34d22 "hostmem: use object id for memory region name with >= 4.0" (v4.0). It may have been intended to be internal back then. It wasn't anymore when commit 8db0b20415 "machine: add missing doc for memory-backend option" (v6.0) documented it:

And document that x-use-canonical-path-for-ramblock-id, is considered to be stable to make sure it won't go away by accident.

x- was intended for unstable/iternal properties, and not supposed to be stable option. However it's too late to rename (drop x-) it as it would mean that users will have to mantain both x-use-canonical-path-for-ramblock-id (for QEMU 5.0-5.2) versions and prefix-less for later versions.

Igor's reasoning still applies.

"x-origin" has always been stable. Same argument.

Yep, fair enough. [...]

commit 415b71a9f6e5bc37e84895d2e767cf4cfacd279b (HEAD) Author: Markus Armbruster <armbru@redhat.com> Date: Sat Oct 9 09:01:21 2021 +0200

qapi: Find x- without feature unstable DBG

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index b7b3fc0ce4..f2af1d7eea 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -14,6 +14,8 @@

# TODO catching name collisions in generated code would be nice

+import sys + from collections import OrderedDict import os import re @@ -118,6 +120,11 @@ def describe(self): return "%s '%s'" % (self.meta, self.name)

+def check_have_feature_unstable(name, info, features): + if name.startswith('x-') and 'unstable' not in (f.name for f in features): + print(QAPISemError(info, f"XXX %{name} %{features}"), file=sys.stderr) + + class QAPISchemaVisitor: def visit_begin(self, schema): pass @@ -718,6 +725,7 @@ def __init__(self, name, info, ifcond=None, features=None): self.features = features or []

def connect_doc(self, doc): + check_have_feature_unstable(self.name, self.info, self.features) super().connect_doc(doc) if doc: for f in self.features: @@ -745,6 +753,7 @@ def __init__(self, name, info, typ, optional, ifcond=None, features=None): self.features = features or []

def check(self, schema): + check_have_feature_unstable(self.name, self.info, self.features) assert self.defined_in self.type = schema.resolve_type(self._type_name, self.info, self.describe) @@ -789,6 +798,7 @@ def __init__(self, name, info, doc, ifcond, features,

def check(self, schema): super().check(schema) + check_have_feature_unstable(self.name, self.info, self.features) if self._arg_type_name: self.arg_type = schema.resolve_type( self._arg_type_name, self.info, "command's 'data'") @@ -844,6 +854,7 @@ def __init__(self, name, info, doc, ifcond, features, arg_type, boxed):

def check(self, schema): super().check(schema) + check_have_feature_unstable(self.name, self.info, self.features) if self._arg_type_name: self.arg_type = schema.resolve_type( self._arg_type_name, self.info, "event's 'data'")

TIL: the error class QAPISemError() Reviewed-by: Kashyap Chamarthy <kchamart@redhat.com> -- /kashyap

On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster <armbru@redhat.com> wrote:

Add special feature 'unstable' everywhere the name starts with 'x-', except for InputBarrierProperties member x-origin and MemoryBackendProperties member x-use-canonical-path-for-ramblock-id, because these two are actually stable.

Signed-off-by: Markus Armbruster <armbru@redhat.com> --- qapi/block-core.json | 123 +++++++++++++++++++++++++++++++------------ qapi/migration.json | 35 +++++++++--- qapi/misc.json | 6 ++- qapi/qom.json | 11 ++-- 4 files changed, 130 insertions(+), 45 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json index 6d3217abb6..ce2c1352cb 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1438,6 +1438,9 @@ # # @x-perf: Performance options. (Since 6.0) # +# Features: +# @unstable: Member @x-perf is experimental. +#

It'd be a lot cooler if we could annotate the unstable member directly instead of confusing it with the syntax that might describe the entire struct/union/command/etc, but ... eh, it's just a doc field, so I'm not gonna press on this. I don't have the energy to get into a doc formatting standard discussion right now, so: sure, why not?

# Note: @on-source-error and @on-target-error only affect background # I/O. If an error occurs during a guest write request, the device's # rerror/werror actions will be used. @@ -1452,7 +1455,9 @@ '*on-source-error': 'BlockdevOnError', '*on-target-error': 'BlockdevOnError', '*auto-finalize': 'bool', '*auto-dismiss': 'bool', - '*filter-node-name': 'str', '*x-perf': 'BackupPerf' } } + '*filter-node-name': 'str', + '*x-perf': { 'type': 'BackupPerf', + 'features': [ 'unstable' ] } } }

## # @DriveBackup: @@ -1916,9 +1921,13 @@ # # Get the block graph. # +# Features: +# @unstable: This command is meant for debugging. +# # Since: 4.0 ## -{ 'command': 'x-debug-query-block-graph', 'returns': 'XDbgBlockGraph' } +{ 'command': 'x-debug-query-block-graph', 'returns': 'XDbgBlockGraph', + 'features': [ 'unstable' ] }

## # @drive-mirror: @@ -2257,6 +2266,9 @@ # # Get bitmap SHA256. # +# Features: +# @unstable: This command is meant for debugging. +# # Returns: - BlockDirtyBitmapSha256 on success # - If @node is not a valid block device, DeviceNotFound # - If @name is not found or if hashing has failed, GenericError with an @@ -2265,7 +2277,8 @@ # Since: 2.10 ## { 'command': 'x-debug-block-dirty-bitmap-sha256', - 'data': 'BlockDirtyBitmap', 'returns': 'BlockDirtyBitmapSha256' } + 'data': 'BlockDirtyBitmap', 'returns': 'BlockDirtyBitmapSha256', + 'features': [ 'unstable' ] }

## # @blockdev-mirror: @@ -2495,27 +2508,57 @@ # # Properties for throttle-group objects. # -# The options starting with x- are aliases for the same key without x- in -# the @limits object. As indicated by the x- prefix, this is not a stable -# interface and may be removed or changed incompatibly in the future. Use -# @limits for a supported stable interface. -# # @limits: limits to apply for this throttle group # +# Features: +# @unstable: All members starting with x- are aliases for the same key +# without x- in the @limits object. This is not a stable +# interface and may be removed or changed incompatibly in +# the future. Use @limits for a supported stable +# interface. +# # Since: 2.11 ## { 'struct': 'ThrottleGroupProperties', 'data': { '*limits': 'ThrottleLimits', - '*x-iops-total' : 'int', '*x-iops-total-max' : 'int', - '*x-iops-total-max-length' : 'int', '*x-iops-read' : 'int', - '*x-iops-read-max' : 'int', '*x-iops-read-max-length' : 'int', - '*x-iops-write' : 'int', '*x-iops-write-max' : 'int', - '*x-iops-write-max-length' : 'int', '*x-bps-total' : 'int', - '*x-bps-total-max' : 'int', '*x-bps-total-max-length' : 'int', - '*x-bps-read' : 'int', '*x-bps-read-max' : 'int', - '*x-bps-read-max-length' : 'int', '*x-bps-write' : 'int', - '*x-bps-write-max' : 'int', '*x-bps-write-max-length' : 'int', - '*x-iops-size' : 'int' } } + '*x-iops-total': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-iops-total-max': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-iops-total-max-length': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-iops-read': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-iops-read-max': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-iops-read-max-length': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-iops-write': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-iops-write-max': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-iops-write-max-length': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-bps-total': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-bps-total-max': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-bps-total-max-length': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-bps-read': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-bps-read-max': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-bps-read-max-length': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-bps-write': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-bps-write-max': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-bps-write-max-length': { 'type': 'int', + 'features': [ 'unstable' ] }, + '*x-iops-size': { 'type': 'int', + 'features': [ 'unstable' ] } } }

## # @block-stream: @@ -2916,6 +2959,7 @@ # read-only when the last writer is detached. This # allows giving QEMU write permissions only on demand # when an operation actually needs write access. +# @unstable: Member x-check-cache-dropped is meant for debugging. # # Since: 2.9 ## @@ -2926,7 +2970,8 @@ '*aio': 'BlockdevAioOptions', '*drop-cache': {'type': 'bool', 'if': 'CONFIG_LINUX'}, - '*x-check-cache-dropped': 'bool' }, + '*x-check-cache-dropped': { 'type': 'bool', + 'features': [ 'unstable' ] } }, 'features': [ { 'name': 'dynamic-auto-read-only', 'if': 'CONFIG_POSIX' } ] }

@@ -4041,13 +4086,16 @@ # future requests before a successful reconnect will # immediately fail. Default 0 (Since 4.2) # +# Features: +# @unstable: Member @x-dirty-bitmap is experimental. +# # Since: 2.9 ## { 'struct': 'BlockdevOptionsNbd', 'data': { 'server': 'SocketAddress', '*export': 'str', '*tls-creds': 'str', - '*x-dirty-bitmap': 'str', + '*x-dirty-bitmap': { 'type': 'str', 'features': [ 'unstable' ] }, '*reconnect-delay': 'uint32' } }

## @@ -4865,13 +4913,17 @@ # and replacement of an active keyslot # (possible loss of data if IO error happens) # +# Features: +# @unstable: This command is experimental. +# # Since: 5.1 ## { 'command': 'x-blockdev-amend', 'data': { 'job-id': 'str', 'node-name': 'str', 'options': 'BlockdevAmendOptions', - '*force': 'bool' } } + '*force': 'bool' }, + 'features': [ 'unstable' ] }

## # @BlockErrorAction: @@ -5242,16 +5294,18 @@ # # @node: the name of the node that will be added. # -# Note: this command is experimental, and its API is not stable. It -# does not support all kinds of operations, all kinds of children, nor -# all block drivers. +# Features: +# @unstable: This command is experimental, and its API is not stable. It +# does not support all kinds of operations, all kinds of +# children, nor all block drivers. # -# FIXME Removing children from a quorum node means introducing gaps in the -# child indices. This cannot be represented in the 'children' list of -# BlockdevOptionsQuorum, as returned by .bdrv_refresh_filename(). +# FIXME Removing children from a quorum node means introducing +# gaps in the child indices. This cannot be represented in the +# 'children' list of BlockdevOptionsQuorum, as returned by +# .bdrv_refresh_filename(). # -# Warning: The data in a new quorum child MUST be consistent with that of -# the rest of the array. +# Warning: The data in a new quorum child MUST be consistent +# with that of the rest of the array. # # Since: 2.7 # @@ -5280,7 +5334,8 @@ { 'command': 'x-blockdev-change', 'data' : { 'parent': 'str', '*child': 'str', - '*node': 'str' } } + '*node': 'str' }, + 'features': [ 'unstable' ] }

## # @x-blockdev-set-iothread: @@ -5297,8 +5352,9 @@ # @force: true if the node and its children should be moved when a BlockBackend # is already attached # -# Note: this command is experimental and intended for test cases that need -# control over IOThreads only. +# Features: +# @unstable: This command is experimental and intended for test cases that +# need control over IOThreads only. # # Since: 2.12 # @@ -5320,7 +5376,8 @@ { 'command': 'x-blockdev-set-iothread', 'data' : { 'node-name': 'str', 'iothread': 'StrOrNull', - '*force': 'bool' } } + '*force': 'bool' }, + 'features': [ 'unstable' ] }

## # @QuorumOpType: diff --git a/qapi/migration.json b/qapi/migration.json index 88f07baedd..9aa8bc5759 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -452,14 +452,20 @@ # procedure starts. The VM RAM is saved with running VM. # (since 6.0) # +# Features: +# @unstable: Members @x-colo and @x-ignore-shared are experimental. +# # Since: 1.2 ## { 'enum': 'MigrationCapability', 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', - 'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram', + 'compress', 'events', 'postcopy-ram', + { 'name': 'x-colo', 'features': [ 'unstable' ] }, + 'release-ram', 'block', 'return-path', 'pause-before-switchover', 'multifd', 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate', - 'x-ignore-shared', 'validate-uuid', 'background-snapshot'] } + { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] }, + 'validate-uuid', 'background-snapshot'] }

## # @MigrationCapabilityStatus: @@ -743,6 +749,9 @@ # block device name if there is one, and to their node name # otherwise. (Since 5.2) # +# Features: +# @unstable: Member @x-checkpoint-delay is experimental. +# # Since: 2.4 ## { 'enum': 'MigrationParameter', @@ -753,7 +762,9 @@ 'cpu-throttle-initial', 'cpu-throttle-increment', 'cpu-throttle-tailslow', 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', - 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', + 'downtime-limit', + { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] }, + 'block-incremental', 'multifd-channels', 'xbzrle-cache-size', 'max-postcopy-bandwidth', 'max-cpu-throttle', 'multifd-compression', @@ -903,6 +914,9 @@ # block device name if there is one, and to their node name # otherwise. (Since 5.2) # +# Features: +# @unstable: Member @x-checkpoint-delay is experimental. +# # Since: 2.4 ## # TODO either fuse back into MigrationParameters, or make @@ -925,7 +939,8 @@ '*tls-authz': 'StrOrNull', '*max-bandwidth': 'size', '*downtime-limit': 'uint64', - '*x-checkpoint-delay': 'uint32', + '*x-checkpoint-delay': { 'type': 'uint32', + 'features': [ 'unstable' ] }, '*block-incremental': 'bool', '*multifd-channels': 'uint8', '*xbzrle-cache-size': 'size', @@ -1099,6 +1114,9 @@ # block device name if there is one, and to their node name # otherwise. (Since 5.2) # +# Features: +# @unstable: Member @x-checkpoint-delay is experimental. +# # Since: 2.4 ## { 'struct': 'MigrationParameters', @@ -1119,7 +1137,8 @@ '*tls-authz': 'str', '*max-bandwidth': 'size', '*downtime-limit': 'uint64', - '*x-checkpoint-delay': 'uint32', + '*x-checkpoint-delay': { 'type': 'uint32', + 'features': [ 'unstable' ] }, '*block-incremental': 'bool', '*multifd-channels': 'uint8', '*xbzrle-cache-size': 'size', @@ -1351,6 +1370,9 @@ # If sent to the Secondary, the Secondary side will run failover work, # then takes over server operation to become the service VM. # +# Features: +# @unstable: This command is experimental. +# # Since: 2.8 # # Example: @@ -1359,7 +1381,8 @@ # <- { "return": {} } # ## -{ 'command': 'x-colo-lost-heartbeat' } +{ 'command': 'x-colo-lost-heartbeat', + 'features': [ 'unstable' ] }

## # @migrate_cancel: diff --git a/qapi/misc.json b/qapi/misc.json index 5c2ca3b556..358548abe1 100644 --- a/qapi/misc.json +++ b/qapi/misc.json @@ -185,6 +185,9 @@ # available during the preconfig state (i.e. when the --preconfig command # line option was in use). # +# Features: +# @unstable: This command is experimental. +# # Since 3.0 # # Returns: nothing @@ -195,7 +198,8 @@ # <- { "return": {} } # ## -{ 'command': 'x-exit-preconfig', 'allow-preconfig': true } +{ 'command': 'x-exit-preconfig', 'allow-preconfig': true, + 'features': [ 'unstable' ] }

## # @human-monitor-command: diff --git a/qapi/qom.json b/qapi/qom.json index 7231ac3f34..ccd1167808 100644 --- a/qapi/qom.json +++ b/qapi/qom.json @@ -559,10 +559,8 @@ # for ramblock-id. Disable this for 4.0 # machine types or older to allow # migration with newer QEMU versions. -# This option is considered stable -# despite the x- prefix. (default: -# false generally, but true for machine -# types <= 4.0) +# (default: false generally, +# but true for machine types <= 4.0) # # Note: prealloc=true and reserve=false cannot be set at the same time. With # reserve=true, the behavior depends on the operating system: for example, @@ -785,6 +783,9 @@ ## # @ObjectType: # +# Features: +# @unstable: Member @x-remote-object is experimental. +# # Since: 6.0 ## { 'enum': 'ObjectType', @@ -836,7 +837,7 @@ 'tls-creds-psk', 'tls-creds-x509', 'tls-cipher-suites', - 'x-remote-object' + { 'name': 'x-remote-object', 'features': [ 'unstable' ] } ] }

## -- 2.31.1

Seems OK, but I didn't audit for false positives/negatives. Trusting your judgment here. (It looks like Phil started to audit this in his reply to your previous commit, so I'll trust that.) Acked-by: John Snow <jsnow@redhat.com>

On Tue, Oct 26, 2021 at 3:56 AM Markus Armbruster <armbru@redhat.com> wrote:

John Snow <jsnow@redhat.com> writes:

...

On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster <armbru@redhat.com> wrote:

...

Add special feature 'unstable' everywhere the name starts with 'x-', except for InputBarrierProperties member x-origin and MemoryBackendProperties member x-use-canonical-path-for-ramblock-id, because these two are actually stable.

Signed-off-by: Markus Armbruster <armbru@redhat.com> --- qapi/block-core.json | 123 +++++++++++++++++++++++++++++++------------ qapi/migration.json | 35 +++++++++--- qapi/misc.json | 6 ++- qapi/qom.json | 11 ++-- 4 files changed, 130 insertions(+), 45 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json index 6d3217abb6..ce2c1352cb 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1438,6 +1438,9 @@ # # @x-perf: Performance options. (Since 6.0) # +# Features: +# @unstable: Member @x-perf is experimental. +#

It'd be a lot cooler if we could annotate the unstable member directly instead of confusing it with the syntax that might describe the entire struct/union/command/etc, but ... eh, it's just a doc field, so I'm not gonna press on this. I don't have the energy to get into a doc formatting standard discussion right now, so: sure, why not?

By design, we have a single doc comment block for the entire definition.

When Kevin invented feature flags (merge commit 4747524f9f2), he added them just to struct types. He added "feature sections" for documenting features. It mirrors the "argument sections" for documenting members. Makes sense.

Aside: he neglected to update qapi-code-gen.rst section "Definition documentation", and I failed to catch it. I'll make up for it.

Peter and I then added feature flags to the remaining definitions (commit 23394b4c39 and 013b4efc9b). Feature sections make sense there, too.

I then added them to struct members (commit 84ab008687). Instead of doing something fancy for documenting feature flags of members, I simply used the existing feature sections. This conflates member features with struct features.

Yeah, that's the part I don't like. If this isn't the first instance of breaking the seal, though, this is the wrong time for me to comment on it. Don't worry about it for this series.

What could "something fancy" be? All we have for members is "argument sections", which are basically a name plus descriptive text. We'd have to add structure to that, say nest feature sections into argument sections. I have no appetite for that right now.

(Tangent below, unrelated to acceptance of this series) Yeah, I don't have an appetite for it at the moment either. I'll have to read Marc-Andre's recent sphinx patches and see if I want to do more work -- I had a bigger prototype a few months back I didn't bring all the way home, but I am still thinking about reworking our QAPIDoc format. It's ... well. I don't really want to, but I am not sure how else to bring some of the features I want home, and I think I need some more time to think carefully through exactly what I want to do and why. I wouldn't mind chatting about it with you sometime soon -- there's a few things to balance here, such as: (1) Reworking the doc format would be an obnoxious amount of churn, ... (2) ...but the Sphinx internals are really not meant to be used the way we're using them right now, ... (3) ...but I also don't want to write our QAPI docstrings in a way that *targets* Sphinx. Not that I think we'll be dropping it any time soon, but it still feels like the wrong idea to tie them so closely together. I'm hoping there's an opportunity to make the parsing nicer with minimal changes to the parsing and format, though. It just might require enforcing a *pinch* more structure. I could see how I feel about per-field annotations at that point. I consider them interesting for things like the Python SDK where I may want to enable/disable warnings for using deprecated stuff at the client-level. (e.g., let's say you're using Python SDK 6.2 to talk to a 6.1 client. Nothing stops you from doing this, but some commands will simply be rejected by QEMU as it won't know what you're talking about. Using deprecated fields or commands to talk to an older client will produce no visible warning from QEMU either, as it wasn't deprecated at that point in time. Still, the client may wish to know that they're asking for future trouble -- so it's a thought that client-side warnings have some play here.) Ehhhhhhhhhhhhhhhh don't worry about it for now, I guess I'll plow forward a bit with the Sphinx stuff I'm thinking of at some point Soon:tm: and worry about where the immovable objects are when I get there. This is foot-based landmine-detection, and it works 100% of the time.

...

...

# Note: @on-source-error and @on-target-error only affect background # I/O. If an error occurs during a guest write request, the device's # rerror/werror actions will be used. @@ -1452,7 +1455,9 @@ '*on-source-error': 'BlockdevOnError', '*on-target-error': 'BlockdevOnError', '*auto-finalize': 'bool', '*auto-dismiss': 'bool', - '*filter-node-name': 'str', '*x-perf': 'BackupPerf' } } + '*filter-node-name': 'str', + '*x-perf': { 'type': 'BackupPerf', + 'features': [ 'unstable' ] } } }

## # @DriveBackup:

[...]

...

Seems OK, but I didn't audit for false positives/negatives. Trusting your judgment here. (It looks like Phil started to audit this in his reply to your previous commit, so I'll trust that.)

I'm pretty sure the x- names that don't get feature 'unstable' are actually stable; see my reply to Kashyap.

I did check git history for each that does get feature 'unstable'. Double-checking is of course welcome.

Yeh, just explaining why it's not an R-B. I'm trying to be a bit better about reviews by not forcing myself to do "all or nothing". I trust your work, of course -- I just also did not double check it :) I need to change the way in which I read and discuss code so that I can be more responsive.

...

Acked-by: John Snow <jsnow@redhat.com>

Thanks!

On Mon, Oct 25, 2021 at 07:25:25AM +0200, Markus Armbruster wrote:

Add special feature 'unstable' everywhere the name starts with 'x-', except for InputBarrierProperties member x-origin and MemoryBackendProperties member x-use-canonical-path-for-ramblock-id, because these two are actually stable.

Signed-off-by: Markus Armbruster <armbru@redhat.com> --- @@ -2495,27 +2508,57 @@ # # Properties for throttle-group objects. # -# The options starting with x- are aliases for the same key without x- in -# the @limits object. As indicated by the x- prefix, this is not a stable -# interface and may be removed or changed incompatibly in the future. Use -# @limits for a supported stable interface. -# # @limits: limits to apply for this throttle group # +# Features: +# @unstable: All members starting with x- are aliases for the same key +# without x- in the @limits object. This is not a stable +# interface and may be removed or changed incompatibly in +# the future. Use @limits for a supported stable +# interface. +# # Since: 2.11 ## { 'struct': 'ThrottleGroupProperties', 'data': { '*limits': 'ThrottleLimits', - '*x-iops-total' : 'int', '*x-iops-total-max' : 'int',

+ '*x-iops-total': { 'type': 'int', + 'features': [ 'unstable' ] },

This struct has been around since 381bd74 (v6.0); but was not listed as deprecated at the time. Do we still need it in 6.2, or have we gone enough release cycles with the saner naming without x- that we could drop this? But that is a question independent of this patch. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster <armbru@redhat.com> wrote:

Signed-off-by: Markus Armbruster <armbru@redhat.com> --- include/qapi/qmp/dispatch.h | 1 - monitor/misc.c | 3 +-- scripts/qapi/commands.py | 5 +---- 3 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 075203dc67..0ce88200b9 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -21,7 +21,6 @@ typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);

typedef enum QmpCommandOptions { - QCO_NO_OPTIONS = 0x0, QCO_NO_SUCCESS_RESP = (1U << 0), QCO_ALLOW_OOB = (1U << 1), QCO_ALLOW_PRECONFIG = (1U << 2), diff --git a/monitor/misc.c b/monitor/misc.c index ffe7966870..3556b177f6 100644 --- a/monitor/misc.c +++ b/monitor/misc.c @@ -230,8 +230,7 @@ static void monitor_init_qmp_commands(void)

qmp_init_marshal(&qmp_commands);

- qmp_register_command(&qmp_commands, "device_add", qmp_device_add, - QCO_NO_OPTIONS); + qmp_register_command(&qmp_commands, "device_add", qmp_device_add, 0);

QTAILQ_INIT(&qmp_cap_negotiation_commands); qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities", diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py index 3654825968..c8a975528f 100644 --- a/scripts/qapi/commands.py +++ b/scripts/qapi/commands.py @@ -229,15 +229,12 @@ def gen_register_command(name: str, if coroutine: options += ['QCO_COROUTINE']

- if not options: - options = ['QCO_NO_OPTIONS'] - ret = mcgen(''' qmp_register_command(cmds, "%(name)s", qmp_marshal_%(c_name)s, %(opts)s); ''', name=name, c_name=c_name(name), - opts=" | ".join(options)) + opts=' | '.join(options) or 0) return ret

I'm not a big fan of naked constants on the C side, but the generator simplification is nice. I suppose it's worth the trade-off if you like it better this way. "eh". Reviewed-by: John Snow <jsnow@redhat.com>

John Snow <jsnow@redhat.com> writes:

On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster <armbru@redhat.com> wrote:

...

New enum QapiSpecialFeature enumerates the special feature flags.

New helper gen_special_features() returns code to represent a collection of special feature flags as a bitset.

The next few commits will put them to use.

Signed-off-by: Markus Armbruster <armbru@redhat.com> --- include/qapi/util.h | 4 ++++ scripts/qapi/gen.py | 13 +++++++++++++ scripts/qapi/schema.py | 3 +++ 3 files changed, 20 insertions(+)

diff --git a/include/qapi/util.h b/include/qapi/util.h index 257c600f99..7a8d5c7d72 100644 --- a/include/qapi/util.h +++ b/include/qapi/util.h @@ -11,6 +11,10 @@ #ifndef QAPI_UTIL_H #define QAPI_UTIL_H

+typedef enum { + QAPI_DEPRECATED, +} QapiSpecialFeature; + /* QEnumLookup flags */ #define QAPI_ENUM_DEPRECATED 1

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index 2ec1e7b3b6..9d07b88cf6 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -29,6 +29,7 @@ mcgen, ) from .schema import ( + QAPISchemaFeature, QAPISchemaIfCond, QAPISchemaModule, QAPISchemaObjectType, @@ -37,6 +38,18 @@ from .source import QAPISourceInfo

+def gen_special_features(features: QAPISchemaFeature): + ret = '' + sep = '' + + for feat in features: + if feat.is_special(): + ret += ('%s1u << QAPI_%s' % (sep, feat.name.upper()))

Building the constant name here "feels" fragile, but I'll trust that the test suite and/or the compiler will catch us if we accidentally goof up this mapping. In the interest of simplicity, then, "sure, why not."

It relies on .is_special() remaining consistent with enum QapiSpecialFeature. The C compiler should catch any screwups.

...

+ sep = ' | ' +

+ return ret or '0'

...

+

Subjectively more pythonic:

special_features = [f"1u << QAPI_{feat.name.upper()}" for feat in features if feat.is_special()] ret = ' | '.join(special_features) return ret or '0'

A bit more dense, but more functional. Up to you, but I find join() easier to read and reason about for the presence of separators.

Sold!

...

+ class QAPIGen: def __init__(self, fname: str): self.fname = fname diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 6d5f46509a..55f82d7389 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -725,6 +725,9 @@ def connect_doc(self, doc): class QAPISchemaFeature(QAPISchemaMember): role = 'feature'

+ def is_special(self): + return self.name in ('deprecated') +

alrighty.

(Briefly wondered: is it worth naming special features as a property of the class, but with only two names, it's probably fine enough to leave it embedded in the method logic. Only a style thing and doesn't have any actual impact that I can imagine, so ... nevermind.)

Let's keep it simple.

...

class QAPISchemaObjectTypeMember(QAPISchemaMember): def __init__(self, name, info, typ, optional, ifcond=None, features=None): -- 2.31.1

Well, either way:

Reviewed-by: John Snow <jsnow@redhat.com>

Thanks!

On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster <armbru@redhat.com> wrote:

The generated visitor functions call visit_deprecated_accept() and visit_deprecated() when visiting a struct member with special feature flag 'deprecated'. This makes the feature flag visible to the actual visitors. I want to make feature flag 'unstable' visible there as well, so I can add policy for it.

To let me make it visible, replace these functions by visit_policy_reject() and visit_policy_skip(), which take the member's special features as an argument. Note that the new functions have the opposite sense, i.e. the return value flips.

Signed-off-by: Markus Armbruster <armbru@redhat.com> --- include/qapi/visitor-impl.h | 6 ++++-- include/qapi/visitor.h | 17 +++++++++++++---- qapi/qapi-forward-visitor.c | 16 +++++++++------- qapi/qapi-visit-core.c | 22 ++++++++++++---------- qapi/qobject-input-visitor.c | 15 ++++++++++----- qapi/qobject-output-visitor.c | 9 ++++++--- qapi/trace-events | 4 ++-- scripts/qapi/visit.py | 14 +++++++------- 8 files changed, 63 insertions(+), 40 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index 72b6537bef..2badec5ba4 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -114,10 +114,12 @@ struct Visitor void (*optional)(Visitor *v, const char *name, bool *present);

/* Optional */ - bool (*deprecated_accept)(Visitor *v, const char *name, Error **errp); + bool (*policy_reject)(Visitor *v, const char *name, + unsigned special_features, Error **errp);

/* Optional */ - bool (*deprecated)(Visitor *v, const char *name); + bool (*policy_skip)(Visitor *v, const char *name, + unsigned special_features);

/* Must be set */ VisitorType type; diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index dcb96018a9..d53a84c9ba 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -461,22 +461,31 @@ void visit_end_alternate(Visitor *v, void **obj); bool visit_optional(Visitor *v, const char *name, bool *present);

/* - * Should we reject deprecated member @name? + * Should we reject member @name due to policy? + * + * @special_features is the member's special features encoded as a + * bitset of QapiSpecialFeature. * * @name must not be NULL. This function is only useful between * visit_start_struct() and visit_end_struct(), since only objects * have deprecated members. */ -bool visit_deprecated_accept(Visitor *v, const char *name, Error **errp); +bool visit_policy_reject(Visitor *v, const char *name, + unsigned special_features, Error **errp);

/* - * Should we visit deprecated member @name? + * + * Should we skip member @name due to policy? + * + * @special_features is the member's special features encoded as a + * bitset of QapiSpecialFeature. * * @name must not be NULL. This function is only useful between * visit_start_struct() and visit_end_struct(), since only objects * have deprecated members. */ -bool visit_deprecated(Visitor *v, const char *name); +bool visit_policy_skip(Visitor *v, const char *name, + unsigned special_features);

/* * Set policy for handling deprecated management interfaces. diff --git a/qapi/qapi-forward-visitor.c b/qapi/qapi-forward-visitor.c index a4b111e22a..25d098aa8a 100644 --- a/qapi/qapi-forward-visitor.c +++ b/qapi/qapi-forward-visitor.c @@ -246,25 +246,27 @@ static void forward_field_optional(Visitor *v, const char *name, bool *present) visit_optional(ffv->target, name, present); }

-static bool forward_field_deprecated_accept(Visitor *v, const char *name, - Error **errp) +static bool forward_field_policy_reject(Visitor *v, const char *name, + unsigned special_features, + Error **errp) { ForwardFieldVisitor *ffv = to_ffv(v);

if (!forward_field_translate_name(ffv, &name, errp)) { return false; } - return visit_deprecated_accept(ffv->target, name, errp); + return visit_policy_reject(ffv->target, name, special_features, errp); }

-static bool forward_field_deprecated(Visitor *v, const char *name) +static bool forward_field_policy_skip(Visitor *v, const char *name, + unsigned special_features) { ForwardFieldVisitor *ffv = to_ffv(v);

if (!forward_field_translate_name(ffv, &name, NULL)) { return false; } - return visit_deprecated(ffv->target, name); + return visit_policy_skip(ffv->target, name, special_features); }

static void forward_field_complete(Visitor *v, void *opaque) @@ -313,8 +315,8 @@ Visitor *visitor_forward_field(Visitor *target, const char *from, const char *to v->visitor.type_any = forward_field_type_any; v->visitor.type_null = forward_field_type_null; v->visitor.optional = forward_field_optional; - v->visitor.deprecated_accept = forward_field_deprecated_accept; - v->visitor.deprecated = forward_field_deprecated; + v->visitor.policy_reject = forward_field_policy_reject; + v->visitor.policy_skip = forward_field_policy_skip; v->visitor.complete = forward_field_complete; v->visitor.free = forward_field_free;

diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index 49136ae88e..b4a81f1757 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -139,22 +139,24 @@ bool visit_optional(Visitor *v, const char *name, bool *present) return *present; }

-bool visit_deprecated_accept(Visitor *v, const char *name, Error **errp) +bool visit_policy_reject(Visitor *v, const char *name, + unsigned special_features, Error **errp) { - trace_visit_deprecated_accept(v, name); - if (v->deprecated_accept) { - return v->deprecated_accept(v, name, errp); + trace_visit_policy_reject(v, name); + if (v->policy_reject) { + return v->policy_reject(v, name, special_features, errp); } - return true; + return false; }

-bool visit_deprecated(Visitor *v, const char *name) +bool visit_policy_skip(Visitor *v, const char *name, + unsigned special_features) { - trace_visit_deprecated(v, name); - if (v->deprecated) { - return v->deprecated(v, name); + trace_visit_policy_skip(v, name); + if (v->policy_skip) { + return v->policy_skip(v, name, special_features); } - return true; + return false; }

void visit_set_policy(Visitor *v, CompatPolicy *policy) diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c index 71b24a4429..fda485614b 100644 --- a/qapi/qobject-input-visitor.c +++ b/qapi/qobject-input-visitor.c @@ -662,16 +662,21 @@ static void qobject_input_optional(Visitor *v, const char *name, bool *present) *present = true; }

-static bool qobject_input_deprecated_accept(Visitor *v, const char *name, - Error **errp) +static bool qobject_input_policy_reject(Visitor *v, const char *name, + unsigned special_features, + Error **errp) { + if (!(special_features && 1u << QAPI_DEPRECATED)) { + return false; + } + switch (v->compat_policy.deprecated_input) { case COMPAT_POLICY_INPUT_ACCEPT: - return true; + return false; case COMPAT_POLICY_INPUT_REJECT: error_setg(errp, "Deprecated parameter '%s' disabled by policy", name); - return false; + return true; case COMPAT_POLICY_INPUT_CRASH: default: abort(); @@ -712,7 +717,7 @@ static QObjectInputVisitor *qobject_input_visitor_base_new(QObject *obj) v->visitor.end_list = qobject_input_end_list; v->visitor.start_alternate = qobject_input_start_alternate; v->visitor.optional = qobject_input_optional; - v->visitor.deprecated_accept = qobject_input_deprecated_accept; + v->visitor.policy_reject = qobject_input_policy_reject; v->visitor.free = qobject_input_free;

v->root = qobject_ref(obj); diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c index 9b7f510036..b5c6564cbb 100644 --- a/qapi/qobject-output-visitor.c +++ b/qapi/qobject-output-visitor.c @@ -13,6 +13,7 @@ */

#include "qemu/osdep.h" +#include "qapi/compat-policy.h" #include "qapi/qobject-output-visitor.h" #include "qapi/visitor-impl.h" #include "qemu/queue.h" @@ -208,9 +209,11 @@ static bool qobject_output_type_null(Visitor *v, const char *name, return true; }

-static bool qobject_output_deprecated(Visitor *v, const char *name) +static bool qobject_output_policy_skip(Visitor *v, const char *name, + unsigned special_features) { - return v->compat_policy.deprecated_output != COMPAT_POLICY_OUTPUT_HIDE; + return !(special_features && 1u << QAPI_DEPRECATED) + || v->compat_policy.deprecated_output == COMPAT_POLICY_OUTPUT_HIDE; }

/* Finish building, and return the root object. @@ -262,7 +265,7 @@ Visitor *qobject_output_visitor_new(QObject **result) v->visitor.type_number = qobject_output_type_number; v->visitor.type_any = qobject_output_type_any; v->visitor.type_null = qobject_output_type_null; - v->visitor.deprecated = qobject_output_deprecated; + v->visitor.policy_skip = qobject_output_policy_skip; v->visitor.complete = qobject_output_complete; v->visitor.free = qobject_output_free;

diff --git a/qapi/trace-events b/qapi/trace-events index cccafc07e5..ab108c4f0e 100644 --- a/qapi/trace-events +++ b/qapi/trace-events @@ -17,8 +17,8 @@ visit_start_alternate(void *v, const char *name, void *obj, size_t size) "v=%p n visit_end_alternate(void *v, void *obj) "v=%p obj=%p"

visit_optional(void *v, const char *name, bool *present) "v=%p name=%s present=%p" -visit_deprecated_accept(void *v, const char *name) "v=%p name=%s" -visit_deprecated(void *v, const char *name) "v=%p name=%s" +visit_policy_reject(void *v, const char *name) "v=%p name=%s" +visit_policy_skip(void *v, const char *name) "v=%p name=%s"

visit_type_enum(void *v, const char *name, int *obj) "v=%p name=%s obj=%p" visit_type_int(void *v, const char *name, int64_t *obj) "v=%p name=%s obj=%p" diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py index 9d9196a143..e13bbe4292 100644 --- a/scripts/qapi/visit.py +++ b/scripts/qapi/visit.py @@ -21,7 +21,7 @@ indent, mcgen, ) -from .gen import QAPISchemaModularCVisitor, ifcontext +from .gen import QAPISchemaModularCVisitor, gen_special_features, ifcontext from .schema import ( QAPISchema, QAPISchemaEnumMember, @@ -76,7 +76,6 @@ def gen_visit_object_members(name: str, c_type=base.c_name())

for memb in members: - deprecated = 'deprecated' in [f.name for f in memb.features] ret += memb.ifcond.gen_if() if memb.optional: ret += mcgen(''' @@ -84,14 +83,15 @@ def gen_visit_object_members(name: str, ''', name=memb.name, c_name=c_name(memb.name)) indent.increase() - if deprecated: + special_features = gen_special_features(memb.features) + if special_features != '0':

Would it be possible for gen_special_features to return something false-y instead of '0'? Do we actually *use* the '0' return anywhere other than to test it to see if we should include additional code? If you actually use the '0' anywhere: Go ahead and treat this as an ack. If you don't, can we clean this up? (Sorry, I find the mcgen stuff hard to read in patch form and I am trying to give you a quick review instead of NO review.) --js

ret += mcgen(''' - if (!visit_deprecated_accept(v, "%(name)s", errp)) { + if (visit_policy_reject(v, "%(name)s", %(special_features)s, errp)) { return false; } - if (visit_deprecated(v, "%(name)s")) { + if (!visit_policy_skip(v, "%(name)s", %(special_features)s)) { ''', - name=memb.name) + name=memb.name, special_features=special_features) indent.increase() ret += mcgen(''' if (!visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, errp)) { @@ -100,7 +100,7 @@ def gen_visit_object_members(name: str, ''', c_type=memb.type.c_name(), name=memb.name, c_name=c_name(memb.name)) - if deprecated: + if special_features != '0': indent.decrease() ret += mcgen(''' } -- 2.31.1

On 10/25/21 07:25, Markus Armbruster wrote:

The generated visitor functions call visit_deprecated_accept() and visit_deprecated() when visiting a struct member with special feature flag 'deprecated'. This makes the feature flag visible to the actual visitors. I want to make feature flag 'unstable' visible there as well, so I can add policy for it.

To let me make it visible, replace these functions by visit_policy_reject() and visit_policy_skip(), which take the member's special features as an argument. Note that the new functions have the opposite sense, i.e. the return value flips.

Signed-off-by: Markus Armbruster <armbru@redhat.com> --- include/qapi/visitor-impl.h | 6 ++++-- include/qapi/visitor.h | 17 +++++++++++++---- qapi/qapi-forward-visitor.c | 16 +++++++++------- qapi/qapi-visit-core.c | 22 ++++++++++++---------- qapi/qobject-input-visitor.c | 15 ++++++++++----- qapi/qobject-output-visitor.c | 9 ++++++--- qapi/trace-events | 4 ++-- scripts/qapi/visit.py | 14 +++++++------- 8 files changed, 63 insertions(+), 40 deletions(-)

diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c index 71b24a4429..fda485614b 100644 --- a/qapi/qobject-input-visitor.c +++ b/qapi/qobject-input-visitor.c @@ -662,16 +662,21 @@ static void qobject_input_optional(Visitor *v, const char *name, bool *present) *present = true; }

-static bool qobject_input_deprecated_accept(Visitor *v, const char *name, - Error **errp) +static bool qobject_input_policy_reject(Visitor *v, const char *name, + unsigned special_features, + Error **errp) { + if (!(special_features && 1u << QAPI_DEPRECATED)) {

Unreachable =) Proof than extract() is safer :P

+ return false; + }

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

On 10/25/21 07:25, Markus Armbruster wrote:

...

The code to check command policy can see special feature flag 'deprecated' as command flag QCO_DEPRECATED. I want to make feature flag 'unstable' visible there as well, so I can add policy for it.

To let me make it visible, add member @special_features (a bitset of QapiSpecialFeature) to QmpCommand, and adjust the generator to pass it through qmp_register_command(). Then replace "QCO_DEPRECATED in @flags" by QAPI_DEPRECATED in @special_features", and drop QCO_DEPRECATED.

Signed-off-by: Markus Armbruster <armbru@redhat.com> --- include/qapi/qmp/dispatch.h | 5 +++-- monitor/misc.c | 6 ++++-- qapi/qmp-dispatch.c | 2 +- qapi/qmp-registry.c | 4 +++- storage-daemon/qemu-storage-daemon.c | 3 ++- scripts/qapi/commands.py | 9 ++++----- 6 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 0ce88200b9..1e4240fd0d 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -25,7 +25,6 @@ typedef enum QmpCommandOptions QCO_ALLOW_OOB = (1U << 1), QCO_ALLOW_PRECONFIG = (1U << 2), QCO_COROUTINE = (1U << 3), - QCO_DEPRECATED = (1U << 4), } QmpCommandOptions;

typedef struct QmpCommand @@ -34,6 +33,7 @@ typedef struct QmpCommand /* Runs in coroutine context if QCO_COROUTINE is set */ QmpCommandFunc *fn; QmpCommandOptions options; + unsigned special_features; QTAILQ_ENTRY(QmpCommand) node; bool enabled; const char *disable_reason; @@ -42,7 +42,8 @@ typedef struct QmpCommand typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;

void qmp_register_command(QmpCommandList *cmds, const char *name, - QmpCommandFunc *fn, QmpCommandOptions options); + QmpCommandFunc *fn, QmpCommandOptions options, + unsigned special_features);

What about:

typedef unsigned QapiFeatureMask;

?

I think the name @special_features makes the connection to QapiSpecialFeature clear enough.

Otherwise: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!

John Snow <jsnow@redhat.com> writes:

On Mon, Oct 25, 2021 at 1:26 AM Markus Armbruster <armbru@redhat.com> wrote:

...

The code to check enumeration value policy can see special feature flag 'deprecated' in QEnumLookup member flags[value]. I want to make feature flag 'unstable' visible there as well, so I can add policy for it.

Instead of extending flags[], replace it by @special_features (a bitset of QapiSpecialFeature), because that's how special features get passed around elsewhere.

Signed-off-by: Markus Armbruster <armbru@redhat.com> --- include/qapi/util.h | 5 +---- qapi/qapi-visit-core.c | 3 ++- scripts/qapi/types.py | 22 ++++++++++++---------- 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/qapi/util.h b/include/qapi/util.h index 7a8d5c7d72..0cc98db9f9 100644 --- a/include/qapi/util.h +++ b/include/qapi/util.h @@ -15,12 +15,9 @@ typedef enum { QAPI_DEPRECATED, } QapiSpecialFeature;

-/* QEnumLookup flags */ -#define QAPI_ENUM_DEPRECATED 1 - typedef struct QEnumLookup { const char *const *array; - const unsigned char *const flags; + const unsigned char *const special_features; const int size; } QEnumLookup;

diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index b4a81f1757..5572d90efb 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -407,7 +407,8 @@ static bool input_type_enum(Visitor *v, const char *name, int *obj, return false; }

- if (lookup->flags && (lookup->flags[value] & QAPI_ENUM_DEPRECATED)) { + if (lookup->special_features + && (lookup->special_features[value] & QAPI_DEPRECATED)) { switch (v->compat_policy.deprecated_input) { case COMPAT_POLICY_INPUT_ACCEPT: break; diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index ab2441adc9..3013329c24 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -16,7 +16,7 @@ from typing import List, Optional

from .common import c_enum_const, c_name, mcgen -from .gen import QAPISchemaModularCVisitor, ifcontext +from .gen import QAPISchemaModularCVisitor, gen_special_features, ifcontext from .schema import ( QAPISchema, QAPISchemaEnumMember, @@ -39,7 +39,7 @@ def gen_enum_lookup(name: str, members: List[QAPISchemaEnumMember], prefix: Optional[str] = None) -> str: max_index = c_enum_const(name, '_MAX', prefix) - flags = '' + feats = '' ret = mcgen('''

const QEnumLookup %(c_name)s_lookup = { @@ -54,19 +54,21 @@ 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: + special_features = gen_special_features(memb.features) + if special_features != '0':

Though, I have to admit the common reoccurrence of this pattern suggests to me that gen_special_features really ought to be returning something false-y in these cases. Something about testing for the empty case with something that represents, but isn't empty, gives me a brief pause.

Not willing to wage war over it.

I habitually start stupid, and when stupid confuses or annoys me later, I smarten it up some. Let's see how this instance of "stupid" ages, okay?

...

+ feats += mcgen(''' + [%(index)s] = %(special_features)s, +''', + index=index, special_features=special_features) + + if feats: ret += mcgen(''' }, - .flags = (const unsigned char[%(max_index)s]) { + .special_features = (const unsigned char[%(max_index)s]) { ''', max_index=max_index) - ret += flags + ret += feats

ret += mcgen(''' }, -- 2.31.1

Python bits: Acked-by: John Snow <jsnow@redhat.com>

Thanks!

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

On 10/25/21 07:25, Markus Armbruster wrote:

...

The code to check policy for handling deprecated input is triplicated. Factor it out into compat_policy_input_ok() before I mess with it in the next commit.

Signed-off-by: Markus Armbruster <armbru@redhat.com> --- include/qapi/compat-policy.h | 7 +++++ qapi/qapi-visit-core.c | 18 +++++-------- qapi/qmp-dispatch.c | 51 +++++++++++++++++++++++++++--------- qapi/qobject-input-visitor.c | 19 +++----------- 4 files changed, 55 insertions(+), 40 deletions(-)

...

diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 8cca18c891..e29ade134c 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -28,6 +28,40 @@

CompatPolicy compat_policy;

+static bool compat_policy_input_ok1(const char *adjective, + CompatPolicyInput policy, + ErrorClass error_class, + const char *kind, const char *name, + Error **errp) +{ + switch (policy) { + case COMPAT_POLICY_INPUT_ACCEPT: + return true; + case COMPAT_POLICY_INPUT_REJECT: + error_set(errp, error_class, "%s %s %s disabled by policy", + adjective, kind, name); + return false; + case COMPAT_POLICY_INPUT_CRASH: + default: + abort();

g_assert_not_reached() provides a nicer user experience.

I find it hard to care for making the experience of a crash that should never ever happen nicer :)

...

+ } +} + +bool compat_policy_input_ok(unsigned special_features, + const CompatPolicy *policy, + ErrorClass error_class, + const char *kind, const char *name, + Error **errp) +{ + if ((special_features & 1u << QAPI_DEPRECATED)

Matter of taste, I find code using extract() easier to review:

extract64(special_features, QAPI_DEPRECATED, 1)

I agree for width > 1.

...

+ && !compat_policy_input_ok1("Deprecated", + policy->deprecated_input, + error_class, kind, name, errp)) { + return false; + } + return true; +}

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

On 10/26/21 11:46, Markus Armbruster wrote:

...

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

...

On 10/25/21 07:25, Markus Armbruster wrote:

...

The code to check policy for handling deprecated input is triplicated. Factor it out into compat_policy_input_ok() before I mess with it in the next commit.

Signed-off-by: Markus Armbruster <armbru@redhat.com> --- include/qapi/compat-policy.h | 7 +++++ qapi/qapi-visit-core.c | 18 +++++-------- qapi/qmp-dispatch.c | 51 +++++++++++++++++++++++++++--------- qapi/qobject-input-visitor.c | 19 +++----------- 4 files changed, 55 insertions(+), 40 deletions(-)

...

diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 8cca18c891..e29ade134c 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -28,6 +28,40 @@

CompatPolicy compat_policy;

+static bool compat_policy_input_ok1(const char *adjective, + CompatPolicyInput policy, + ErrorClass error_class, + const char *kind, const char *name, + Error **errp) +{ + switch (policy) { + case COMPAT_POLICY_INPUT_ACCEPT: + return true; + case COMPAT_POLICY_INPUT_REJECT: + error_set(errp, error_class, "%s %s %s disabled by policy", + adjective, kind, name); + return false; + case COMPAT_POLICY_INPUT_CRASH: + default: + abort();

g_assert_not_reached() provides a nicer user experience.

I find it hard to care for making the experience of a crash that should never ever happen nicer :)

Well COMPAT_POLICY_INPUT_CRASH can happen... What about:

Point.

case COMPAT_POLICY_INPUT_CRASH: error_printf("%s %s %s disabled by policy", adjective, kind, name); abort(); default: g_assert_not_reached();

Separate patch. I'd prefer to delay it a bit, to avoid rocking the boat so close to the soft freeze.

* Markus Armbruster (armbru@redhat.com) wrote:

New option parameters unstable-input and unstable-output set policy for unstable interfaces just like deprecated-input and deprecated-output set policy for deprecated interfaces (see commit 6dd75472d5 "qemu-options: New -compat to set policy for deprecated interfaces"). This is intended for testing users of the management interfaces. It is experimental.

So is there no way to mark the option as unstable? Dave

For now, this covers only syntactic aspects of QMP, i.e. stuff tagged with feature 'unstable'. We may want to extend it to cover semantic aspects, or the command line.

Note that there is no good way for management application to detect presence of these new option parameters: they are not visible output of query-qmp-schema or query-command-line-options. Tolerable, because it's meant for testing. If running with -compat fails, skip the test.

Signed-off-by: Markus Armbruster <armbru@redhat.com> --- qapi/compat.json | 6 +++++- include/qapi/util.h | 1 + qapi/qmp-dispatch.c | 6 ++++++ qapi/qobject-output-visitor.c | 8 ++++++-- qemu-options.hx | 20 +++++++++++++++++++- scripts/qapi/events.py | 10 ++++++---- scripts/qapi/schema.py | 10 ++++++---- 7 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/qapi/compat.json b/qapi/compat.json index 74a8493d3d..9bc9804abb 100644 --- a/qapi/compat.json +++ b/qapi/compat.json @@ -47,9 +47,13 @@ # # @deprecated-input: how to handle deprecated input (default 'accept') # @deprecated-output: how to handle deprecated output (default 'accept') +# @unstable-input: how to handle unstable input (default 'accept') +# @unstable-output: how to handle unstable output (default 'accept') # # Since: 6.0 ## { 'struct': 'CompatPolicy', 'data': { '*deprecated-input': 'CompatPolicyInput', - '*deprecated-output': 'CompatPolicyOutput' } } + '*deprecated-output': 'CompatPolicyOutput', + '*unstable-input': 'CompatPolicyInput', + '*unstable-output': 'CompatPolicyOutput' } } diff --git a/include/qapi/util.h b/include/qapi/util.h index 0cc98db9f9..81a2b13a33 100644 --- a/include/qapi/util.h +++ b/include/qapi/util.h @@ -13,6 +13,7 @@

typedef enum { QAPI_DEPRECATED, + QAPI_UNSTABLE, } QapiSpecialFeature;

typedef struct QEnumLookup { diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index e29ade134c..c5c6e521a2 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -59,6 +59,12 @@ bool compat_policy_input_ok(unsigned special_features, error_class, kind, name, errp)) { return false; } + if ((special_features & (1u << QAPI_UNSTABLE)) + && !compat_policy_input_ok1("Unstable", + policy->unstable_input, + error_class, kind, name, errp)) { + return false; + } return true; }

diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c index b5c6564cbb..74770edd73 100644 --- a/qapi/qobject-output-visitor.c +++ b/qapi/qobject-output-visitor.c @@ -212,8 +212,12 @@ static bool qobject_output_type_null(Visitor *v, const char *name, static bool qobject_output_policy_skip(Visitor *v, const char *name, unsigned special_features) { - return !(special_features && 1u << QAPI_DEPRECATED) - || v->compat_policy.deprecated_output == COMPAT_POLICY_OUTPUT_HIDE; + CompatPolicy *pol = &v->compat_policy; + + return ((special_features & 1u << QAPI_DEPRECATED) + && pol->deprecated_output == COMPAT_POLICY_OUTPUT_HIDE) + || ((special_features & 1u << QAPI_UNSTABLE) + && pol->unstable_output == COMPAT_POLICY_OUTPUT_HIDE); }

/* Finish building, and return the root object. diff --git a/qemu-options.hx b/qemu-options.hx index 5f375bbfa6..f051536b63 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3641,7 +3641,9 @@ DEFHEADING(Debug/Expert options:)

DEF("compat", HAS_ARG, QEMU_OPTION_compat, "-compat [deprecated-input=accept|reject|crash][,deprecated-output=accept|hide]\n" - " Policy for handling deprecated management interfaces\n", + " Policy for handling deprecated management interfaces\n" + "-compat [unstable-input=accept|reject|crash][,unstable-output=accept|hide]\n" + " Policy for handling unstable management interfaces\n", QEMU_ARCH_ALL) SRST ``-compat [deprecated-input=@var{input-policy}][,deprecated-output=@var{output-policy}]`` @@ -3659,6 +3661,22 @@ SRST Suppress deprecated command results and events

Limitation: covers only syntactic aspects of QMP. + +``-compat [unstable-input=@var{input-policy}][,unstable-output=@var{output-policy}]`` + Set policy for handling unstable management interfaces (experimental): + + ``unstable-input=accept`` (default) + Accept unstable commands and arguments + ``unstable-input=reject`` + Reject unstable commands and arguments + ``unstable-input=crash`` + Crash on unstable commands and arguments + ``unstable-output=accept`` (default) + Emit unstable command results and events + ``unstable-output=hide`` + Suppress unstable command results and events + + Limitation: covers only syntactic aspects of QMP. ERST

DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg, diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py index 82475e84ec..27b44c49f5 100644 --- a/scripts/qapi/events.py +++ b/scripts/qapi/events.py @@ -109,13 +109,15 @@ def gen_event_send(name: str, if not boxed: ret += gen_param_var(arg_type)

- if 'deprecated' in [f.name for f in features]: - ret += mcgen(''' + for f in features: + if f.is_special(): + ret += mcgen('''

- if (compat_policy.deprecated_output == COMPAT_POLICY_OUTPUT_HIDE) { + if (compat_policy.%(feat)s_output == COMPAT_POLICY_OUTPUT_HIDE) { return; } -''') +''', + feat=f.name)

ret += mcgen('''

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 55f82d7389..b7b3fc0ce4 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -254,9 +254,11 @@ def doc_type(self):

def check(self, schema): QAPISchemaEntity.check(self, schema) - if 'deprecated' in [f.name for f in self.features]: - raise QAPISemError( - self.info, "feature 'deprecated' is not supported for types") + for feat in self.features: + if feat.is_special(): + raise QAPISemError( + self.info, + f"feature '{feat.name}' is not supported for types")

def describe(self): assert self.meta @@ -726,7 +728,7 @@ class QAPISchemaFeature(QAPISchemaMember): role = 'feature'

def is_special(self): - return self.name in ('deprecated') + return self.name in ('deprecated', 'unstable')

class QAPISchemaObjectTypeMember(QAPISchemaMember): -- 2.31.1

-- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK