John Snow
2021-Oct-25 19:05 UTC
[Libguestfs] [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'
On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster <armbru at 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 at 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 at redhat.com> -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://listman.redhat.com/archives/libguestfs/attachments/20211025/8af6c7e6/attachment.htm>
Markus Armbruster
2021-Oct-26 07:56 UTC
[Libguestfs] [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'
John Snow <jsnow at redhat.com> writes:> On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster <armbru at 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 at 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. 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.> > >> # 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.> Acked-by: John Snow <jsnow at redhat.com>Thanks!