[PATCH 0/9] Configurable policy for handling unstable interfaces

[PATCH 0/9] Configurable policy for handling unstable interfaces

[Markus Armbruster]

Option -compat lets you configure what to do when deprecated
interfaces get used.  This series extends this to unstable interfaces.
Works the same way.  Intended for testing users of the management
interfaces.  It is experimental.

To make it possible, I replace the "x-" naming convention by special
feature flag "unstable".  See PATCH 1 for rationale.

Based on my "[PATCH v4 0/5] qapi: Add feature flags to enum members"

Based-on: Message-Id: <20211025042405.3762351-1-armbru@redhat.com>

Markus Armbruster (9):
  qapi: New special feature flag "unstable"
  qapi: Mark unstable QMP parts with feature 'unstable'
  qapi: Eliminate QCO_NO_OPTIONS for a slight simplification
  qapi: Tools for sets of special feature flags in generated code
  qapi: Generalize struct member policy checking
  qapi: Generalize command policy checking
  qapi: Generalize enum member policy checking
  qapi: Factor out compat_policy_input_ok()
  qapi: Extend -compat to set policy for unstable interfaces

 docs/devel/qapi-code-gen.rst            |   9 +-
 qapi/block-core.json                    | 123 +++++++++++++++++-------
 qapi/compat.json                        |   6 +-
 qapi/migration.json                     |  35 +++++--
 qapi/misc.json                          |   6 +-
 qapi/qom.json                           |  11 ++-
 include/qapi/compat-policy.h            |   7 ++
 include/qapi/qmp/dispatch.h             |   6 +-
 include/qapi/util.h                     |   8 +-
 include/qapi/visitor-impl.h             |   6 +-
 include/qapi/visitor.h                  |  17 +++-
 monitor/misc.c                          |   7 +-
 qapi/qapi-forward-visitor.c             |  16 +--
 qapi/qapi-visit-core.c                  |  41 ++++----
 qapi/qmp-dispatch.c                     |  57 ++++++++---
 qapi/qmp-registry.c                     |   4 +-
 qapi/qobject-input-visitor.c            |  22 ++---
 qapi/qobject-output-visitor.c           |  13 ++-
 storage-daemon/qemu-storage-daemon.c    |   3 +-
 qapi/trace-events                       |   4 +-
 qemu-options.hx                         |  20 +++-
 scripts/qapi/commands.py                |  12 +--
 scripts/qapi/events.py                  |  10 +-
 scripts/qapi/gen.py                     |  13 +++
 scripts/qapi/schema.py                  |  11 ++-
 scripts/qapi/types.py                   |  22 +++--
 scripts/qapi/visit.py                   |  14 +--
 tests/qapi-schema/qapi-schema-test.json |   7 +-
 tests/qapi-schema/qapi-schema-test.out  |   5 +
 29 files changed, 353 insertions(+), 162 deletions(-)

-- 
2.31.1

[PATCH 1/9] qapi: New special feature flag "unstable"

[Markus Armbruster]

By convention, names starting with "x-" are experimental.  The parts
of external interfaces so named may be withdrawn or changed
incompatibly in future releases.

Drawback: promoting something from experimental to stable involves a
name change.  Client code needs to be updated.

Moreover, the convention is not universally observed:

* QOM type "input-barrier" has properties "x-origin", "y-origin".
  Looks accidental, but it's ABI since 4.2.

* QOM types "memory-backend-file", "memory-backend-memfd",
  "memory-backend-ram", and "memory-backend-epc" have a property
  "x-use-canonical-path-for-ramblock-id" that is documented to be
  stable despite its name.

We could document these exceptions, but documentation helps only
humans.  We want to recognize "unstable" in code, like "deprecated".

Replace the convention by a new special feature flag "unstable".  It
will be recognized by the QAPI generator, like the existing feature
flag "deprecated", and unlike regular feature flags.

This commit updates documentation and prepares tests.  The next commit
updates the QAPI schema.  The remaining patches update the QAPI
generator and wire up -compat policy checking.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/devel/qapi-code-gen.rst            | 9 ++++++---
 tests/qapi-schema/qapi-schema-test.json | 7 +++++--
 tests/qapi-schema/qapi-schema-test.out  | 5 +++++
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index 4071c9074a..38f2d7aad3 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -713,6 +713,10 @@ member as deprecated.  It is not supported elsewhere so far.
 Interfaces so marked may be withdrawn in future releases in accordance
 with QEMU's deprecation policy.
 
+Feature "unstable" marks a command, event, enum value, or struct
+member as unstable.  It is not supported elsewhere so far.  Interfaces
+so marked may be withdrawn or changed incompatibly in future releases.
+
 
 Naming rules and reserved names
 -------------------------------
@@ -746,9 +750,8 @@ Member name ``u`` and names starting with ``has-`` or ``has_`` are reserved
 for the generator, which uses them for unions and for tracking
 optional members.
 
-Any name (command, event, type, member, or enum value) beginning with
-``x-`` is marked experimental, and may be withdrawn or changed
-incompatibly in a future release.
+Names beginning with ``x-`` used to signify "experimental".  This
+convention has been replaced by special feature "unstable".
 
 Pragmas ``command-name-exceptions`` and ``member-name-exceptions`` let
 you violate naming rules.  Use for new code is strongly discouraged. See
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index b677ab861d..43b8697002 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -273,7 +273,7 @@
   'data': { 'foo': { 'type': 'int', 'features': [ 'deprecated' ] } },
   'features': [ 'feature1' ] }
 { 'struct': 'FeatureStruct2',
-  'data': { 'foo': 'int' },
+  'data': { 'foo': { 'type': 'int', 'features': [ 'unstable' ] } },
   'features': [ { 'name': 'feature1' } ] }
 { 'struct': 'FeatureStruct3',
   'data': { 'foo': 'int' },
@@ -331,7 +331,7 @@
 { 'command': 'test-command-features1',
   'features': [ 'deprecated' ] }
 { 'command': 'test-command-features3',
-  'features': [ 'feature1', 'feature2' ] }
+  'features': [ 'unstable', 'feature1', 'feature2' ] }
 
 { 'command': 'test-command-cond-features1',
   'features': [ { 'name': 'feature1', 'if': 'TEST_IF_FEATURE_1'} ] }
@@ -348,3 +348,6 @@
 
 { 'event': 'TEST_EVENT_FEATURES1',
   'features': [ 'deprecated' ] }
+
+{ 'event': 'TEST_EVENT_FEATURES2',
+  'features': [ 'unstable' ] }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 16846dbeb8..1f9585fa9b 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -308,6 +308,7 @@ object FeatureStruct1
     feature feature1
 object FeatureStruct2
     member foo: int optional=False
+        feature unstable
     feature feature1
 object FeatureStruct3
     member foo: int optional=False
@@ -373,6 +374,7 @@ command test-command-features1 None -> None
     feature deprecated
 command test-command-features3 None -> None
     gen=True success_response=True boxed=False oob=False preconfig=False
+    feature unstable
     feature feature1
     feature feature2
 command test-command-cond-features1 None -> None
@@ -394,6 +396,9 @@ event TEST_EVENT_FEATURES0 FeatureStruct1
 event TEST_EVENT_FEATURES1 None
     boxed=False
     feature deprecated
+event TEST_EVENT_FEATURES2 None
+    boxed=False
+    feature unstable
 module include/sub-module.json
 include sub-sub-module.json
 object SecondArrayRef
-- 
2.31.1

[PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'

[Markus Armbruster]

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.
+#
 # 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

[PATCH 3/9] qapi: Eliminate QCO_NO_OPTIONS for a slight simplification

[Markus Armbruster]

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
 
 
-- 
2.31.1

[PATCH 4/9] qapi: Tools for sets of special feature flags in generated code

[Markus Armbruster]

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()))
+            sep = ' | '
+
+    return ret or '0'
+
+
 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')
+
 
 class QAPISchemaObjectTypeMember(QAPISchemaMember):
     def __init__(self, name, info, typ, optional, ifcond=None, features=None):
-- 
2.31.1

[PATCH 5/9] qapi: Generalize struct member policy checking

[Markus Armbruster]

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':
             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

[PATCH 6/9] qapi: Generalize command policy checking

[Markus Armbruster]

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);
 const QmpCommand *qmp_find_command(const QmpCommandList *cmds,
                                    const char *name);
 void qmp_disable_command(QmpCommandList *cmds, const char *name,
diff --git a/monitor/misc.c b/monitor/misc.c
index 3556b177f6..c2d227a07c 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -230,11 +230,13 @@ static void monitor_init_qmp_commands(void)
 
     qmp_init_marshal(&qmp_commands);
 
-    qmp_register_command(&qmp_commands, "device_add", qmp_device_add, 0);
+    qmp_register_command(&qmp_commands, "device_add",
+                         qmp_device_add, 0, 0);
 
     QTAILQ_INIT(&qmp_cap_negotiation_commands);
     qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
-                         qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
+                         qmp_marshal_qmp_capabilities,
+                         QCO_ALLOW_PRECONFIG, 0);
 }
 
 /* Set the current CPU defined by the user. Callers must hold BQL. */
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 7e943a0af5..8cca18c891 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -176,7 +176,7 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
                   "The command %s has not been found", command);
         goto out;
     }
-    if (cmd->options & QCO_DEPRECATED) {
+    if (cmd->special_features & 1u << QAPI_DEPRECATED) {
         switch (compat_policy.deprecated_input) {
         case COMPAT_POLICY_INPUT_ACCEPT:
             break;
diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
index f78c064aae..485bc5e6fc 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -16,7 +16,8 @@
 #include "qapi/qmp/dispatch.h"
 
 void qmp_register_command(QmpCommandList *cmds, const char *name,
-                          QmpCommandFunc *fn, QmpCommandOptions options)
+                          QmpCommandFunc *fn, QmpCommandOptions options,
+                          unsigned special_features)
 {
     QmpCommand *cmd = g_malloc0(sizeof(*cmd));
 
@@ -27,6 +28,7 @@ void qmp_register_command(QmpCommandList *cmds, const char *name,
     cmd->fn = fn;
     cmd->enabled = true;
     cmd->options = options;
+    cmd->special_features = special_features;
     QTAILQ_INSERT_TAIL(cmds, cmd, node);
 }
 
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index 10a1a33761..52cf17e8ac 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -146,7 +146,8 @@ static void init_qmp_commands(void)
 
     QTAILQ_INIT(&qmp_cap_negotiation_commands);
     qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
-                         qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
+                         qmp_marshal_qmp_capabilities,
+                         QCO_ALLOW_PRECONFIG, 0);
 }
 
 static int getopt_set_loc(int argc, char **argv, const char *optstring,
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index c8a975528f..21001bbd6b 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -26,6 +26,7 @@
     QAPISchemaModularCVisitor,
     build_params,
     ifcontext,
+    gen_special_features,
 )
 from .schema import (
     QAPISchema,
@@ -217,9 +218,6 @@ def gen_register_command(name: str,
                          coroutine: bool) -> str:
     options = []
 
-    if 'deprecated' in [f.name for f in features]:
-        options += ['QCO_DEPRECATED']
-
     if not success_response:
         options += ['QCO_NO_SUCCESS_RESP']
     if allow_oob:
@@ -231,10 +229,11 @@ def gen_register_command(name: str,
 
     ret = mcgen('''
     qmp_register_command(cmds, "%(name)s",
-                         qmp_marshal_%(c_name)s, %(opts)s);
+                         qmp_marshal_%(c_name)s, %(opts)s, %(feats)s);
 ''',
                 name=name, c_name=c_name(name),
-                opts=' | '.join(options) or 0)
+                opts=' | '.join(options) or 0,
+                feats=gen_special_features(features))
     return ret
 
 
-- 
2.31.1

[PATCH 7/9] qapi: Generalize enum member policy checking

[Markus Armbruster]

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':
+            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

[PATCH 8/9] qapi: Factor out compat_policy_input_ok()

[Markus Armbruster]

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/include/qapi/compat-policy.h b/include/qapi/compat-policy.h
index 1083f95122..8b7b25c0b5 100644
--- a/include/qapi/compat-policy.h
+++ b/include/qapi/compat-policy.h
@@ -13,10 +13,17 @@
 #ifndef QAPI_COMPAT_POLICY_H
 #define QAPI_COMPAT_POLICY_H
 
+#include "qapi/error.h"
 #include "qapi/qapi-types-compat.h"
 
 extern CompatPolicy compat_policy;
 
+bool compat_policy_input_ok(unsigned special_features,
+                            const CompatPolicy *policy,
+                            ErrorClass error_class,
+                            const char *kind, const char *name,
+                            Error **errp);
+
 /*
  * Create a QObject input visitor for @obj for use with QMP
  *
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 5572d90efb..a1ddfe8831 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -13,6 +13,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/compat-policy.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/visitor.h"
@@ -408,18 +409,11 @@ static bool input_type_enum(Visitor *v, const char *name, int *obj,
     }
 
     if (lookup->special_features
-        && (lookup->special_features[value] & QAPI_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();
-        }
+        && !compat_policy_input_ok(lookup->special_features[value],
+                                   &v->compat_policy,
+                                   ERROR_CLASS_GENERIC_ERROR,
+                                   "value", enum_str, errp)) {
+        return false;
     }
 
     *obj = value;
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();
+    }
+}
+
+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)
+        && !compat_policy_input_ok1("Deprecated",
+                                    policy->deprecated_input,
+                                    error_class, kind, name, errp)) {
+        return false;
+    }
+    return true;
+}
+
 Visitor *qobject_input_visitor_new_qmp(QObject *obj)
 {
     Visitor *v = qobject_input_visitor_new(obj);
@@ -176,19 +210,10 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
                   "The command %s has not been found", command);
         goto out;
     }
-    if (cmd->special_features & 1u << QAPI_DEPRECATED) {
-        switch (compat_policy.deprecated_input) {
-        case COMPAT_POLICY_INPUT_ACCEPT:
-            break;
-        case COMPAT_POLICY_INPUT_REJECT:
-            error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
-                      "Deprecated command %s disabled by policy",
-                      command);
-            goto out;
-        case COMPAT_POLICY_INPUT_CRASH:
-        default:
-            abort();
-        }
+    if (!compat_policy_input_ok(cmd->special_features, &compat_policy,
+                                ERROR_CLASS_COMMAND_NOT_FOUND,
+                                "command", command, &err)) {
+        goto out;
     }
     if (!cmd->enabled) {
         error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index fda485614b..f0b4c7ca9d 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -14,6 +14,7 @@
 
 #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"
@@ -666,21 +667,9 @@ 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 false;
-    case COMPAT_POLICY_INPUT_REJECT:
-        error_setg(errp, "Deprecated parameter '%s' disabled by policy",
-                   name);
-        return true;
-    case COMPAT_POLICY_INPUT_CRASH:
-    default:
-        abort();
-    }
+    return !compat_policy_input_ok(special_features, &v->compat_policy,
+                                   ERROR_CLASS_GENERIC_ERROR,
+                                   "parameter", name, errp);
 }
 
 static void qobject_input_free(Visitor *v)
-- 
2.31.1

[PATCH 9/9] qapi: Extend -compat to set policy for unstable interfaces

[Markus Armbruster]

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.

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

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> By convention, names starting with "x-" are experimental.  The parts
> of external interfaces so named may be withdrawn or changed
> incompatibly in future releases.
>
> Drawback: promoting something from experimental to stable involves a
> name change.  Client code needs to be updated.
>
> Moreover, the convention is not universally observed:
>
> * QOM type "input-barrier" has properties "x-origin", "y-origin".
>   Looks accidental, but it's ABI since 4.2.
>
> * QOM types "memory-backend-file", "memory-backend-memfd",
>   "memory-backend-ram", and "memory-backend-epc" have a property
>   "x-use-canonical-path-for-ramblock-id" that is documented to be
>   stable despite its name.
>
> We could document these exceptions, but documentation helps only
> humans.  We want to recognize "unstable" in code, like "deprecated".
>
> Replace the convention by a new special feature flag "unstable".  It
> will be recognized by the QAPI generator, like the existing feature
> flag "deprecated", and unlike regular feature flags.
>
> This commit updates documentation and prepares tests.  The next commit
> updates the QAPI schema.  The remaining patches update the QAPI
> generator and wire up -compat policy checking.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'

[Juan Quintela]

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>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH 3/9] qapi: Eliminate QCO_NO_OPTIONS for a slight simplification

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH 6/9] qapi: Generalize command policy checking

[Philippe Mathieu-Daudé]

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;

?

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

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Kashyap Chamarthy]

On Mon, Oct 25, 2021 at 07:25:24AM +0200, Markus Armbruster wrote:
> By convention, names starting with "x-" are experimental.  The parts
> of external interfaces so named may be withdrawn or changed
> incompatibly in future releases.
> 
> Drawback: promoting something from experimental to stable involves a
> name change.  Client code needs to be updated.
> 
> Moreover, the convention is not universally observed:
> 
> * QOM type "input-barrier" has properties "x-origin", "y-origin".
>   Looks accidental, but it's ABI since 4.2.
> 
> * QOM types "memory-backend-file", "memory-backend-memfd",
>   "memory-backend-ram", and "memory-backend-epc" have a property
>   "x-use-canonical-path-for-ramblock-id" that is documented to be
>   stable despite its name.

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

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".  
 
> We could document these exceptions, but documentation helps only
> humans.  We want to recognize "unstable" in code, like "deprecated".
>
> Replace the convention by a new special feature flag "unstable".  It
> will be recognized by the QAPI generator, like the existing feature
> flag "deprecated", and unlike regular feature flags.

FWIW, sounds good to me.

> This commit updates documentation and prepares tests.  The next commit
> updates the QAPI schema.  The remaining patches update the QAPI
> generator and wire up -compat policy checking.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/devel/qapi-code-gen.rst            | 9 ++++++---
>  tests/qapi-schema/qapi-schema-test.json | 7 +++++--
>  tests/qapi-schema/qapi-schema-test.out  | 5 +++++
>  3 files changed, 16 insertions(+), 5 deletions(-)

[...]

-- 
/kashyap

Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()

[Philippe Mathieu-Daudé]

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.

> +    }
> +}
> +
> +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)

> +        && !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>

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Philippe Mathieu-Daudé]

On 10/25/21 14:05, Kashyap Chamarthy wrote:
> On Mon, Oct 25, 2021 at 07:25:24AM +0200, Markus Armbruster wrote:
>> By convention, names starting with "x-" are experimental.  The parts
>> of external interfaces so named may be withdrawn or changed
>> incompatibly in future releases.
>>
>> Drawback: promoting something from experimental to stable involves a
>> name change.  Client code needs to be updated.
>>
>> Moreover, the convention is not universally observed:
>>
>> * QOM type "input-barrier" has properties "x-origin", "y-origin".
>>   Looks accidental, but it's ABI since 4.2.
>>
>> * QOM types "memory-backend-file", "memory-backend-memfd",
>>   "memory-backend-ram", and "memory-backend-epc" have a property
>>   "x-use-canonical-path-for-ramblock-id" that is documented to be
>>   stable despite its name.
> 
> Looks like there's another stable property with an "x-" prefix:
> "x-remote-object", part of QOM type @RemoteObjectProperties.

IIRC "x-remote-object" and RemoteObjectProperties are not stable.

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 5311 bytes --]

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

> By convention, names starting with "x-" are experimental.  The parts
> of external interfaces so named may be withdrawn or changed
> incompatibly in future releases.
>
> Drawback: promoting something from experimental to stable involves a
> name change.  Client code needs to be updated.
>
> Moreover, the convention is not universally observed:
>
> * QOM type "input-barrier" has properties "x-origin", "y-origin".
>   Looks accidental, but it's ABI since 4.2.
>
> * QOM types "memory-backend-file", "memory-backend-memfd",
>   "memory-backend-ram", and "memory-backend-epc" have a property
>   "x-use-canonical-path-for-ramblock-id" that is documented to be
>   stable despite its name.
>
> We could document these exceptions, but documentation helps only
> humans.  We want to recognize "unstable" in code, like "deprecated".
>
> Replace the convention by a new special feature flag "unstable".  It
> will be recognized by the QAPI generator, like the existing feature
> flag "deprecated", and unlike regular feature flags.
>
> This commit updates documentation and prepares tests.  The next commit
> updates the QAPI schema.  The remaining patches update the QAPI
> generator and wire up -compat policy checking.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/devel/qapi-code-gen.rst            | 9 ++++++---
>  tests/qapi-schema/qapi-schema-test.json | 7 +++++--
>  tests/qapi-schema/qapi-schema-test.out  | 5 +++++
>  3 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
> index 4071c9074a..38f2d7aad3 100644
> --- a/docs/devel/qapi-code-gen.rst
> +++ b/docs/devel/qapi-code-gen.rst
> @@ -713,6 +713,10 @@ member as deprecated.  It is not supported elsewhere
> so far.
>  Interfaces so marked may be withdrawn in future releases in accordance
>  with QEMU's deprecation policy.
>
> +Feature "unstable" marks a command, event, enum value, or struct
> +member as unstable.  It is not supported elsewhere so far.  Interfaces
> +so marked may be withdrawn or changed incompatibly in future releases.
> +
>
>  Naming rules and reserved names
>  -------------------------------
> @@ -746,9 +750,8 @@ Member name ``u`` and names starting with ``has-`` or
> ``has_`` are reserved
>  for the generator, which uses them for unions and for tracking
>  optional members.
>
> -Any name (command, event, type, member, or enum value) beginning with
> -``x-`` is marked experimental, and may be withdrawn or changed
> -incompatibly in a future release.
> +Names beginning with ``x-`` used to signify "experimental".  This
> +convention has been replaced by special feature "unstable".
>
>  Pragmas ``command-name-exceptions`` and ``member-name-exceptions`` let
>  you violate naming rules.  Use for new code is strongly discouraged. See
> diff --git a/tests/qapi-schema/qapi-schema-test.json
> b/tests/qapi-schema/qapi-schema-test.json
> index b677ab861d..43b8697002 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -273,7 +273,7 @@
>    'data': { 'foo': { 'type': 'int', 'features': [ 'deprecated' ] } },
>    'features': [ 'feature1' ] }
>  { 'struct': 'FeatureStruct2',
> -  'data': { 'foo': 'int' },
> +  'data': { 'foo': { 'type': 'int', 'features': [ 'unstable' ] } },
>    'features': [ { 'name': 'feature1' } ] }
>  { 'struct': 'FeatureStruct3',
>    'data': { 'foo': 'int' },
> @@ -331,7 +331,7 @@
>  { 'command': 'test-command-features1',
>    'features': [ 'deprecated' ] }
>  { 'command': 'test-command-features3',
> -  'features': [ 'feature1', 'feature2' ] }
> +  'features': [ 'unstable', 'feature1', 'feature2' ] }
>
>  { 'command': 'test-command-cond-features1',
>    'features': [ { 'name': 'feature1', 'if': 'TEST_IF_FEATURE_1'} ] }
> @@ -348,3 +348,6 @@
>
>  { 'event': 'TEST_EVENT_FEATURES1',
>    'features': [ 'deprecated' ] }
> +
> +{ 'event': 'TEST_EVENT_FEATURES2',
> +  'features': [ 'unstable' ] }
> diff --git a/tests/qapi-schema/qapi-schema-test.out
> b/tests/qapi-schema/qapi-schema-test.out
> index 16846dbeb8..1f9585fa9b 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -308,6 +308,7 @@ object FeatureStruct1
>      feature feature1
>  object FeatureStruct2
>      member foo: int optional=False
> +        feature unstable
>      feature feature1
>  object FeatureStruct3
>      member foo: int optional=False
> @@ -373,6 +374,7 @@ command test-command-features1 None -> None
>      feature deprecated
>  command test-command-features3 None -> None
>      gen=True success_response=True boxed=False oob=False preconfig=False
> +    feature unstable
>      feature feature1
>      feature feature2
>  command test-command-cond-features1 None -> None
> @@ -394,6 +396,9 @@ event TEST_EVENT_FEATURES0 FeatureStruct1
>  event TEST_EVENT_FEATURES1 None
>      boxed=False
>      feature deprecated
> +event TEST_EVENT_FEATURES2 None
> +    boxed=False
> +    feature unstable
>  module include/sub-module.json
>  include sub-sub-module.json
>  object SecondArrayRef
> --
> 2.31.1
>
>
Feels odd to combine the doc update *and* test prep, but eh, whatever.

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

[-- Attachment #2: Type: text/html, Size: 6954 bytes --]

Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 17342 bytes --]

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>

[-- Attachment #2: Type: text/html, Size: 24058 bytes --]

Re: [PATCH 3/9] qapi: Eliminate QCO_NO_OPTIONS for a slight simplification

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 2252 bytes --]

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>

[-- Attachment #2: Type: text/html, Size: 3348 bytes --]

Re: [PATCH 4/9] qapi: Tools for sets of special feature flags in generated code

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 3045 bytes --]

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."


> +            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.


> +
>  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.)


>
>  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>

[-- Attachment #2: Type: text/html, Size: 4762 bytes --]

Re: [PATCH 5/9] qapi: Generalize struct member policy checking

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 13366 bytes --]

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
>
>

[-- Attachment #2: Type: text/html, Size: 16680 bytes --]

Re: [PATCH 6/9] qapi: Generalize command policy checking

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 6636 bytes --]

On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster <armbru@redhat.com> 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);
>  const QmpCommand *qmp_find_command(const QmpCommandList *cmds,
>                                     const char *name);
>  void qmp_disable_command(QmpCommandList *cmds, const char *name,
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 3556b177f6..c2d227a07c 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -230,11 +230,13 @@ static void monitor_init_qmp_commands(void)
>
>      qmp_init_marshal(&qmp_commands);
>
> -    qmp_register_command(&qmp_commands, "device_add", qmp_device_add, 0);
> +    qmp_register_command(&qmp_commands, "device_add",
> +                         qmp_device_add, 0, 0);
>
>      QTAILQ_INIT(&qmp_cap_negotiation_commands);
>      qmp_register_command(&qmp_cap_negotiation_commands,
> "qmp_capabilities",
> -                         qmp_marshal_qmp_capabilities,
> QCO_ALLOW_PRECONFIG);
> +                         qmp_marshal_qmp_capabilities,
> +                         QCO_ALLOW_PRECONFIG, 0);
>  }
>
>  /* Set the current CPU defined by the user. Callers must hold BQL. */
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 7e943a0af5..8cca18c891 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -176,7 +176,7 @@ QDict *qmp_dispatch(const QmpCommandList *cmds,
> QObject *request,
>                    "The command %s has not been found", command);
>          goto out;
>      }
> -    if (cmd->options & QCO_DEPRECATED) {
> +    if (cmd->special_features & 1u << QAPI_DEPRECATED) {
>          switch (compat_policy.deprecated_input) {
>          case COMPAT_POLICY_INPUT_ACCEPT:
>              break;
> diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
> index f78c064aae..485bc5e6fc 100644
> --- a/qapi/qmp-registry.c
> +++ b/qapi/qmp-registry.c
> @@ -16,7 +16,8 @@
>  #include "qapi/qmp/dispatch.h"
>
>  void qmp_register_command(QmpCommandList *cmds, const char *name,
> -                          QmpCommandFunc *fn, QmpCommandOptions options)
> +                          QmpCommandFunc *fn, QmpCommandOptions options,
> +                          unsigned special_features)
>  {
>      QmpCommand *cmd = g_malloc0(sizeof(*cmd));
>
> @@ -27,6 +28,7 @@ void qmp_register_command(QmpCommandList *cmds, const
> char *name,
>      cmd->fn = fn;
>      cmd->enabled = true;
>      cmd->options = options;
> +    cmd->special_features = special_features;
>      QTAILQ_INSERT_TAIL(cmds, cmd, node);
>  }
>
> diff --git a/storage-daemon/qemu-storage-daemon.c
> b/storage-daemon/qemu-storage-daemon.c
> index 10a1a33761..52cf17e8ac 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -146,7 +146,8 @@ static void init_qmp_commands(void)
>
>      QTAILQ_INIT(&qmp_cap_negotiation_commands);
>      qmp_register_command(&qmp_cap_negotiation_commands,
> "qmp_capabilities",
> -                         qmp_marshal_qmp_capabilities,
> QCO_ALLOW_PRECONFIG);
> +                         qmp_marshal_qmp_capabilities,
> +                         QCO_ALLOW_PRECONFIG, 0);
>  }
>
>  static int getopt_set_loc(int argc, char **argv, const char *optstring,
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index c8a975528f..21001bbd6b 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -26,6 +26,7 @@
>      QAPISchemaModularCVisitor,
>      build_params,
>      ifcontext,
> +    gen_special_features,
>  )
>  from .schema import (
>      QAPISchema,
> @@ -217,9 +218,6 @@ def gen_register_command(name: str,
>                           coroutine: bool) -> str:
>      options = []
>
> -    if 'deprecated' in [f.name for f in features]:
> -        options += ['QCO_DEPRECATED']
> -
>      if not success_response:
>          options += ['QCO_NO_SUCCESS_RESP']
>      if allow_oob:
> @@ -231,10 +229,11 @@ def gen_register_command(name: str,
>
>      ret = mcgen('''
>      qmp_register_command(cmds, "%(name)s",
> -                         qmp_marshal_%(c_name)s, %(opts)s);
> +                         qmp_marshal_%(c_name)s, %(opts)s, %(feats)s);
>  ''',
>                  name=name, c_name=c_name(name),
> -                opts=' | '.join(options) or 0)
> +                opts=' | '.join(options) or 0,
> +                feats=gen_special_features(features))
>

Ah, you use the '0' return here. Alright then.


>      return ret
>
>
> --
> 2.31.1
>
>
Python bits: Acked-by: John Snow <jsnow@redhat.com>
C bits: "I believe in my heart that they probably work."

(for this and previous patch.)

[-- Attachment #2: Type: text/html, Size: 8594 bytes --]

Re: [PATCH 7/9] qapi: Generalize enum member policy checking

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 3973 bytes --]

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.


> +            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>

[-- Attachment #2: Type: text/html, Size: 5490 bytes --]

Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 377 bytes --]

On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster <armbru@redhat.com> 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>
>


(Skipping C-only patches for quick review. I'll trust you on these.)

--js

[-- Attachment #2: Type: text/html, Size: 824 bytes --]

Re: [PATCH 9/9] qapi: Extend -compat to set policy for unstable interfaces

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 7657 bytes --]

On Mon, Oct 25, 2021 at 1:26 AM 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.
>
> 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
>
>
Python bits: Acked-by: John Snow <jsnow@redhat.com>

Looks good overall from what I can see, minor style quibbles that I'd
probably fold on if you frowned at me.

--js

[-- Attachment #2: Type: text/html, Size: 10130 bytes --]

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Mon, Oct 25, 2021 at 1:26 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> By convention, names starting with "x-" are experimental.  The parts
>> of external interfaces so named may be withdrawn or changed
>> incompatibly in future releases.
>>
>> Drawback: promoting something from experimental to stable involves a
>> name change.  Client code needs to be updated.
>>
>> Moreover, the convention is not universally observed:
>>
>> * QOM type "input-barrier" has properties "x-origin", "y-origin".
>>   Looks accidental, but it's ABI since 4.2.
>>
>> * QOM types "memory-backend-file", "memory-backend-memfd",
>>   "memory-backend-ram", and "memory-backend-epc" have a property
>>   "x-use-canonical-path-for-ramblock-id" that is documented to be
>>   stable despite its name.
>>
>> We could document these exceptions, but documentation helps only
>> humans.  We want to recognize "unstable" in code, like "deprecated".
>>
>> Replace the convention by a new special feature flag "unstable".  It
>> will be recognized by the QAPI generator, like the existing feature
>> flag "deprecated", and unlike regular feature flags.
>>
>> This commit updates documentation and prepares tests.  The next commit
>> updates the QAPI schema.  The remaining patches update the QAPI
>> generator and wire up -compat policy checking.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>

[...]

> Feels odd to combine the doc update *and* test prep, but eh, whatever.

I admit this is what was left after patch splitting and reshuffling.

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

Thanks!

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Markus Armbruster]

Kashyap Chamarthy <kchamart@redhat.com> writes:

> On Mon, Oct 25, 2021 at 07:25:24AM +0200, Markus Armbruster wrote:
>> By convention, names starting with "x-" are experimental.  The parts
>> of external interfaces so named may be withdrawn or changed
>> incompatibly in future releases.
>> 
>> Drawback: promoting something from experimental to stable involves a
>> name change.  Client code needs to be updated.
>> 
>> Moreover, the convention is not universally observed:
>> 
>> * QOM type "input-barrier" has properties "x-origin", "y-origin".
>>   Looks accidental, but it's ABI since 4.2.
>> 
>> * QOM types "memory-backend-file", "memory-backend-memfd",
>>   "memory-backend-ram", and "memory-backend-epc" have a property
>>   "x-use-canonical-path-for-ramblock-id" that is documented to be
>>   stable despite its name.
>
> 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.

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

> 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.

>> We could document these exceptions, but documentation helps only
>> humans.  We want to recognize "unstable" in code, like "deprecated".
>>
>> Replace the convention by a new special feature flag "unstable".  It
>> will be recognized by the QAPI generator, like the existing feature
>> flag "deprecated", and unlike regular feature flags.
>
> FWIW, sounds good to me.

Thanks!

>> This commit updates documentation and prepares tests.  The next commit
>> updates the QAPI schema.  The remaining patches update the QAPI
>> generator and wire up -compat policy checking.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  docs/devel/qapi-code-gen.rst            | 9 ++++++---
>>  tests/qapi-schema/qapi-schema-test.json | 7 +++++--
>>  tests/qapi-schema/qapi-schema-test.out  | 5 +++++
>>  3 files changed, 16 insertions(+), 5 deletions(-)
>
> [...]


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'")

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Kevin Wolf]

Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben:
> By convention, names starting with "x-" are experimental.  The parts
> of external interfaces so named may be withdrawn or changed
> incompatibly in future releases.
> 
> Drawback: promoting something from experimental to stable involves a
> name change.  Client code needs to be updated.
> 
> Moreover, the convention is not universally observed:
> 
> * QOM type "input-barrier" has properties "x-origin", "y-origin".
>   Looks accidental, but it's ABI since 4.2.
> 
> * QOM types "memory-backend-file", "memory-backend-memfd",
>   "memory-backend-ram", and "memory-backend-epc" have a property
>   "x-use-canonical-path-for-ramblock-id" that is documented to be
>   stable despite its name.
> 
> We could document these exceptions, but documentation helps only
> humans.  We want to recognize "unstable" in code, like "deprecated".
> 
> Replace the convention by a new special feature flag "unstable".  It
> will be recognized by the QAPI generator, like the existing feature
> flag "deprecated", and unlike regular feature flags.
> 
> This commit updates documentation and prepares tests.  The next commit
> updates the QAPI schema.  The remaining patches update the QAPI
> generator and wire up -compat policy checking.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Obviously, replacing the old convention gets rid of the old drawbacks,
but adds a new one: While using x- makes it very obvious for a human
user that this is an unstable feature, a feature flag in the schema will
almost certainly go unnoticed in manual use.

Kevin

Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'

[Markus Armbruster]

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.

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@redhat.com>

Thanks!

Re: [PATCH 3/9] qapi: Eliminate QCO_NO_OPTIONS for a slight simplification

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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".

I think 0 is an okay way to say "no flags, nothing to see here, move
on".

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

Thanks!

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Kashyap Chamarthy]

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

Re: [Libguestfs] [PATCH 1/9] qapi: New special feature flag "unstable"

[Kashyap Chamarthy]

On Tue, Oct 26, 2021 at 09:37:29AM +0200, Kevin Wolf wrote:
> Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben:

[...]

> > This commit updates documentation and prepares tests.  The next commit
> > updates the QAPI schema.  The remaining patches update the QAPI
> > generator and wire up -compat policy checking.
> > 
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> Obviously, replacing the old convention gets rid of the old drawbacks,
> but adds a new one: While using x- makes it very obvious for a human
> user that this is an unstable feature, a feature flag in the schema will
> almost certainly go unnoticed in manual use.

FWIW, I wondered about it too, as I like the visual reminder of the "x-"
prefix.  Then I thought, "how many people besides QEMU and related
tooling developers -- who would read QAPI docs anyway :) -- launch
experimental QEMU commands manually?"  Maybe that's too big of a leap.

-- 
/kashyap

Re: [PATCH 4/9] qapi: Tools for sets of special feature flags in generated code

[Markus Armbruster]

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!

Re: [PATCH 5/9] qapi: Generalize struct member policy checking

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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>

[...]

>> 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?

gen_special_features() returns a string holding C code for a special
features bitset.  The empty bitset is 0.

The "natural" use is interpolating this value into C code.  Two
instances are visible below.

The use right here is for testing "have we got special features", so we
can skip generating code that is of no use when we don't have any.  It's
admittedly slightly unclean.

> (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.)

Any kind of review is appreciated :)

> --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)

These are the "natural" uses I mentioned.

If gen_special_features() returned something other than '0', we'd have
to replace it by '0' here.

>>              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
>>
>>

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Dr. David Alan Gilbert]

* Kevin Wolf (kwolf@redhat.com) wrote:
> Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben:
> > By convention, names starting with "x-" are experimental.  The parts
> > of external interfaces so named may be withdrawn or changed
> > incompatibly in future releases.
> > 
> > Drawback: promoting something from experimental to stable involves a
> > name change.  Client code needs to be updated.
> > 
> > Moreover, the convention is not universally observed:
> > 
> > * QOM type "input-barrier" has properties "x-origin", "y-origin".
> >   Looks accidental, but it's ABI since 4.2.
> > 
> > * QOM types "memory-backend-file", "memory-backend-memfd",
> >   "memory-backend-ram", and "memory-backend-epc" have a property
> >   "x-use-canonical-path-for-ramblock-id" that is documented to be
> >   stable despite its name.
> > 
> > We could document these exceptions, but documentation helps only
> > humans.  We want to recognize "unstable" in code, like "deprecated".
> > 
> > Replace the convention by a new special feature flag "unstable".  It
> > will be recognized by the QAPI generator, like the existing feature
> > flag "deprecated", and unlike regular feature flags.
> > 
> > This commit updates documentation and prepares tests.  The next commit
> > updates the QAPI schema.  The remaining patches update the QAPI
> > generator and wire up -compat policy checking.
> > 
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> Obviously, replacing the old convention gets rid of the old drawbacks,
> but adds a new one: While using x- makes it very obvious for a human
> user that this is an unstable feature, a feature flag in the schema will
> almost certainly go unnoticed in manual use.

Agreed, I'd keep the x- as well.

Having said that, the x- represents a few different things (that we
don't currently distinguish):
  - experimental
  - for internal use
  - for debugging/human use

Dave

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

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Daniel P. Berrangé]

On Tue, Oct 26, 2021 at 10:22:15AM +0100, Dr. David Alan Gilbert wrote:
> * Kevin Wolf (kwolf@redhat.com) wrote:
> > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben:
> > > By convention, names starting with "x-" are experimental.  The parts
> > > of external interfaces so named may be withdrawn or changed
> > > incompatibly in future releases.
> > > 
> > > Drawback: promoting something from experimental to stable involves a
> > > name change.  Client code needs to be updated.
> > > 
> > > Moreover, the convention is not universally observed:
> > > 
> > > * QOM type "input-barrier" has properties "x-origin", "y-origin".
> > >   Looks accidental, but it's ABI since 4.2.
> > > 
> > > * QOM types "memory-backend-file", "memory-backend-memfd",
> > >   "memory-backend-ram", and "memory-backend-epc" have a property
> > >   "x-use-canonical-path-for-ramblock-id" that is documented to be
> > >   stable despite its name.
> > > 
> > > We could document these exceptions, but documentation helps only
> > > humans.  We want to recognize "unstable" in code, like "deprecated".
> > > 
> > > Replace the convention by a new special feature flag "unstable".  It
> > > will be recognized by the QAPI generator, like the existing feature
> > > flag "deprecated", and unlike regular feature flags.
> > > 
> > > This commit updates documentation and prepares tests.  The next commit
> > > updates the QAPI schema.  The remaining patches update the QAPI
> > > generator and wire up -compat policy checking.
> > > 
> > > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > 
> > Obviously, replacing the old convention gets rid of the old drawbacks,
> > but adds a new one: While using x- makes it very obvious for a human
> > user that this is an unstable feature, a feature flag in the schema will
> > almost certainly go unnoticed in manual use.
> 
> Agreed, I'd keep the x- as well.
> 
> Having said that, the x- represents a few different things (that we
> don't currently distinguish):
>   - experimental
>   - for internal use
>   - for debugging/human use

All of those usage scenarios have the same implication though:

   Command/data format is liable to change in incompatible ways,
   or be deleted, with no prior warning.

I don't think we need to distinguish the use cases - some commands
may belong to two or three of those use cases. All that matters is
that they're considered "unstable" from an API compat POV.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben:
>> By convention, names starting with "x-" are experimental.  The parts
>> of external interfaces so named may be withdrawn or changed
>> incompatibly in future releases.
>> 
>> Drawback: promoting something from experimental to stable involves a
>> name change.  Client code needs to be updated.
>> 
>> Moreover, the convention is not universally observed:
>> 
>> * QOM type "input-barrier" has properties "x-origin", "y-origin".
>>   Looks accidental, but it's ABI since 4.2.
>> 
>> * QOM types "memory-backend-file", "memory-backend-memfd",
>>   "memory-backend-ram", and "memory-backend-epc" have a property
>>   "x-use-canonical-path-for-ramblock-id" that is documented to be
>>   stable despite its name.
>> 
>> We could document these exceptions, but documentation helps only
>> humans.  We want to recognize "unstable" in code, like "deprecated".
>> 
>> Replace the convention by a new special feature flag "unstable".  It
>> will be recognized by the QAPI generator, like the existing feature
>> flag "deprecated", and unlike regular feature flags.
>> 
>> This commit updates documentation and prepares tests.  The next commit
>> updates the QAPI schema.  The remaining patches update the QAPI
>> generator and wire up -compat policy checking.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Obviously, replacing the old convention gets rid of the old drawbacks,
> but adds a new one: While using x- makes it very obvious for a human
> user that this is an unstable feature, a feature flag in the schema will
> almost certainly go unnoticed in manual use.

I thought about this, but neglected to put it in writing.  My bad.

Manual use of unstable interfaces is mostly fine.  Human users can adapt
to changing interfaces.  HMP works that way.

Management applications are better off with a feature flag than with a
naming convention we sometimes ignore.

The most potential for trouble is in between: programs that aren't
full-fledged management applications.

If we want to keep "unstable" obvious to the humans who write such
programs, we can continue to require "x-", in addition to the feature
flag.  We pay for it with renames, and the risk of forgetting to rename
in time (which is what got us the awkward stable
"x-use-canonical-path-for-ramblock-id").  Tradeoff.  I chose not to, but
if y'all think we should...

What we can't do, at least not easily, is to use *only* the "x-"
convention: it is not reliable.  We'd have to add a way to say 'this is
stable even though the name starts with "x-"'.

Re: [PATCH 6/9] qapi: Generalize command policy checking

[Markus Armbruster]

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!

Re: [PATCH 7/9] qapi: Generalize enum member policy checking

[Markus Armbruster]

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!

Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()

[Markus Armbruster]

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!

Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()

[Philippe Mathieu-Daudé]

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:

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

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Kevin Wolf]

Am 26.10.2021 um 11:37 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben:
> >> By convention, names starting with "x-" are experimental.  The parts
> >> of external interfaces so named may be withdrawn or changed
> >> incompatibly in future releases.
> >> 
> >> Drawback: promoting something from experimental to stable involves a
> >> name change.  Client code needs to be updated.
> >> 
> >> Moreover, the convention is not universally observed:
> >> 
> >> * QOM type "input-barrier" has properties "x-origin", "y-origin".
> >>   Looks accidental, but it's ABI since 4.2.
> >> 
> >> * QOM types "memory-backend-file", "memory-backend-memfd",
> >>   "memory-backend-ram", and "memory-backend-epc" have a property
> >>   "x-use-canonical-path-for-ramblock-id" that is documented to be
> >>   stable despite its name.
> >> 
> >> We could document these exceptions, but documentation helps only
> >> humans.  We want to recognize "unstable" in code, like "deprecated".
> >> 
> >> Replace the convention by a new special feature flag "unstable".  It
> >> will be recognized by the QAPI generator, like the existing feature
> >> flag "deprecated", and unlike regular feature flags.
> >> 
> >> This commit updates documentation and prepares tests.  The next commit
> >> updates the QAPI schema.  The remaining patches update the QAPI
> >> generator and wire up -compat policy checking.
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >
> > Obviously, replacing the old convention gets rid of the old drawbacks,
> > but adds a new one: While using x- makes it very obvious for a human
> > user that this is an unstable feature, a feature flag in the schema will
> > almost certainly go unnoticed in manual use.
> 
> I thought about this, but neglected to put it in writing.  My bad.
> 
> Manual use of unstable interfaces is mostly fine.  Human users can adapt
> to changing interfaces.  HMP works that way.
> 
> Management applications are better off with a feature flag than with a
> naming convention we sometimes ignore.
> 
> The most potential for trouble is in between: programs that aren't
> full-fledged management applications.
> 
> If we want to keep "unstable" obvious to the humans who write such
> programs, we can continue to require "x-", in addition to the feature
> flag.  We pay for it with renames, and the risk of forgetting to rename
> in time (which is what got us the awkward stable
> "x-use-canonical-path-for-ramblock-id").  Tradeoff.  I chose not to, but
> if y'all think we should...

Just to clarify, I'm not implying that we should keep it. I'm merely
pointing out that there is a tradeoff that requires us to make a choice.
The decision for one of the options should be explicit rather than just
happening as a side effect. Documenting that it was a conscious decision
is probably best done by adding the reasoning for it to the commit
message.

> What we can't do, at least not easily, is to use *only* the "x-"
> convention: it is not reliable.  We'd have to add a way to say 'this is
> stable even though the name starts with "x-"'.

No question.

Kevin

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Daniel P. Berrangé]

On Tue, Oct 26, 2021 at 11:37:19AM +0200, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben:
> >> By convention, names starting with "x-" are experimental.  The parts
> >> of external interfaces so named may be withdrawn or changed
> >> incompatibly in future releases.
> >> 
> >> Drawback: promoting something from experimental to stable involves a
> >> name change.  Client code needs to be updated.
> >> 
> >> Moreover, the convention is not universally observed:
> >> 
> >> * QOM type "input-barrier" has properties "x-origin", "y-origin".
> >>   Looks accidental, but it's ABI since 4.2.
> >> 
> >> * QOM types "memory-backend-file", "memory-backend-memfd",
> >>   "memory-backend-ram", and "memory-backend-epc" have a property
> >>   "x-use-canonical-path-for-ramblock-id" that is documented to be
> >>   stable despite its name.
> >> 
> >> We could document these exceptions, but documentation helps only
> >> humans.  We want to recognize "unstable" in code, like "deprecated".
> >> 
> >> Replace the convention by a new special feature flag "unstable".  It
> >> will be recognized by the QAPI generator, like the existing feature
> >> flag "deprecated", and unlike regular feature flags.
> >> 
> >> This commit updates documentation and prepares tests.  The next commit
> >> updates the QAPI schema.  The remaining patches update the QAPI
> >> generator and wire up -compat policy checking.
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >
> > Obviously, replacing the old convention gets rid of the old drawbacks,
> > but adds a new one: While using x- makes it very obvious for a human
> > user that this is an unstable feature, a feature flag in the schema will
> > almost certainly go unnoticed in manual use.
> 
> I thought about this, but neglected to put it in writing.  My bad.
> 
> Manual use of unstable interfaces is mostly fine.  Human users can adapt
> to changing interfaces.  HMP works that way.
> 
> Management applications are better off with a feature flag than with a
> naming convention we sometimes ignore.

We will sometimes ignore/forget the feature flag too though, so I'm
not convinced there's much difference there.

> If we want to keep "unstable" obvious to the humans who write such
> programs, we can continue to require "x-", in addition to the feature
> flag.  We pay for it with renames, and the risk of forgetting to rename
> in time (which is what got us the awkward stable
> "x-use-canonical-path-for-ramblock-id").  Tradeoff.  I chose not to, but
> if y'all think we should...

IMHO the renames arguably make life easier for mgmt apps, as they
only need to check for the name without the x- prefix, and they
know they won't be accidentally using the fature from an older
QEMU where it was unstable because the older QEMU had a different
name they won't be checking for.

We can just as easily forget to remove the "unstable" feature
flag, as forget to rename.

The plus point about the feature flag is that it is aligned with
the "deprecated" feature flag, and potentially aligned with a
future "insecure" feature flag.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Oct 26, 2021 at 11:37:19AM +0200, Markus Armbruster wrote:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben:
>> >> By convention, names starting with "x-" are experimental.  The parts
>> >> of external interfaces so named may be withdrawn or changed
>> >> incompatibly in future releases.
>> >> 
>> >> Drawback: promoting something from experimental to stable involves a
>> >> name change.  Client code needs to be updated.
>> >> 
>> >> Moreover, the convention is not universally observed:
>> >> 
>> >> * QOM type "input-barrier" has properties "x-origin", "y-origin".
>> >>   Looks accidental, but it's ABI since 4.2.
>> >> 
>> >> * QOM types "memory-backend-file", "memory-backend-memfd",
>> >>   "memory-backend-ram", and "memory-backend-epc" have a property
>> >>   "x-use-canonical-path-for-ramblock-id" that is documented to be
>> >>   stable despite its name.
>> >> 
>> >> We could document these exceptions, but documentation helps only
>> >> humans.  We want to recognize "unstable" in code, like "deprecated".
>> >> 
>> >> Replace the convention by a new special feature flag "unstable".  It
>> >> will be recognized by the QAPI generator, like the existing feature
>> >> flag "deprecated", and unlike regular feature flags.
>> >> 
>> >> This commit updates documentation and prepares tests.  The next commit
>> >> updates the QAPI schema.  The remaining patches update the QAPI
>> >> generator and wire up -compat policy checking.
>> >> 
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >
>> > Obviously, replacing the old convention gets rid of the old drawbacks,
>> > but adds a new one: While using x- makes it very obvious for a human
>> > user that this is an unstable feature, a feature flag in the schema will
>> > almost certainly go unnoticed in manual use.
>> 
>> I thought about this, but neglected to put it in writing.  My bad.
>> 
>> Manual use of unstable interfaces is mostly fine.  Human users can adapt
>> to changing interfaces.  HMP works that way.
>> 
>> Management applications are better off with a feature flag than with a
>> naming convention we sometimes ignore.
>
> We will sometimes ignore/forget the feature flag too though, so I'm
> not convinced there's much difference there.

-compat unstable-input=reject,unstable-output=hide should help you stay
on the straight & narrow :)

>> If we want to keep "unstable" obvious to the humans who write such
>> programs, we can continue to require "x-", in addition to the feature
>> flag.  We pay for it with renames, and the risk of forgetting to rename
>> in time (which is what got us the awkward stable
>> "x-use-canonical-path-for-ramblock-id").  Tradeoff.  I chose not to, but
>> if y'all think we should...
>
> IMHO the renames arguably make life easier for mgmt apps, as they
> only need to check for the name without the x- prefix, and they
> know they won't be accidentally using the fature from an older
> QEMU where it was unstable because the older QEMU had a different
> name they won't be checking for.
>
> We can just as easily forget to remove the "unstable" feature
> flag, as forget to rename.
>
> The plus point about the feature flag is that it is aligned with
> the "deprecated" feature flag, and potentially aligned with a
> future "insecure" feature flag.

Yup.

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Daniel P. Berrangé]

On Tue, Oct 26, 2021 at 05:15:10PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Tue, Oct 26, 2021 at 11:37:19AM +0200, Markus Armbruster wrote:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >> > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben:
> >> >> By convention, names starting with "x-" are experimental.  The parts
> >> >> of external interfaces so named may be withdrawn or changed
> >> >> incompatibly in future releases.
> >> >> 
> >> >> Drawback: promoting something from experimental to stable involves a
> >> >> name change.  Client code needs to be updated.
> >> >> 
> >> >> Moreover, the convention is not universally observed:
> >> >> 
> >> >> * QOM type "input-barrier" has properties "x-origin", "y-origin".
> >> >>   Looks accidental, but it's ABI since 4.2.
> >> >> 
> >> >> * QOM types "memory-backend-file", "memory-backend-memfd",
> >> >>   "memory-backend-ram", and "memory-backend-epc" have a property
> >> >>   "x-use-canonical-path-for-ramblock-id" that is documented to be
> >> >>   stable despite its name.
> >> >> 
> >> >> We could document these exceptions, but documentation helps only
> >> >> humans.  We want to recognize "unstable" in code, like "deprecated".
> >> >> 
> >> >> Replace the convention by a new special feature flag "unstable".  It
> >> >> will be recognized by the QAPI generator, like the existing feature
> >> >> flag "deprecated", and unlike regular feature flags.
> >> >> 
> >> >> This commit updates documentation and prepares tests.  The next commit
> >> >> updates the QAPI schema.  The remaining patches update the QAPI
> >> >> generator and wire up -compat policy checking.
> >> >> 
> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> >
> >> > Obviously, replacing the old convention gets rid of the old drawbacks,
> >> > but adds a new one: While using x- makes it very obvious for a human
> >> > user that this is an unstable feature, a feature flag in the schema will
> >> > almost certainly go unnoticed in manual use.
> >> 
> >> I thought about this, but neglected to put it in writing.  My bad.
> >> 
> >> Manual use of unstable interfaces is mostly fine.  Human users can adapt
> >> to changing interfaces.  HMP works that way.
> >> 
> >> Management applications are better off with a feature flag than with a
> >> naming convention we sometimes ignore.
> >
> > We will sometimes ignore/forget the feature flag too though, so I'm
> > not convinced there's much difference there.
> 
> -compat unstable-input=reject,unstable-output=hide should help you stay
> on the straight & narrow :)

That's from the pov of the mgmt app. I meant from the POV of QEMU
maintainers forgetting to add "unstable" flag, just as they might
forget to add a "x-" prefix.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 5/9] qapi: Generalize struct member policy checking

[Philippe Mathieu-Daudé]

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;
> +    }

Re: [PATCH 7/9] qapi: Generalize enum member policy checking

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 4703 bytes --]

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

> 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?
>
>
"I see what you're saying, but mehhhhh" is a relatable feeling ;)

>
> >> +            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!
>
>

[-- Attachment #2: Type: text/html, Size: 7119 bytes --]

Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 6525 bytes --]

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!
>
>

[-- Attachment #2: Type: text/html, Size: 8872 bytes --]

Re: [PATCH 9/9] qapi: Extend -compat to set policy for unstable interfaces

[Dr. David Alan Gilbert]

* 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

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Oct 26, 2021 at 05:15:10PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Tue, Oct 26, 2021 at 11:37:19AM +0200, Markus Armbruster wrote:

[...]

>> >> Management applications are better off with a feature flag than with a
>> >> naming convention we sometimes ignore.
>> >
>> > We will sometimes ignore/forget the feature flag too though, so I'm
>> > not convinced there's much difference there.
>> 
>> -compat unstable-input=reject,unstable-output=hide should help you stay
>> on the straight & narrow :)
>
> That's from the pov of the mgmt app. I meant from the POV of QEMU
> maintainers forgetting to add "unstable" flag, just as they might
> forget to add a "x-" prefix.

Got it.

My point was that feature flag "unstable" is an unequivocal signal for
"this thing is unstable", while a name starting with "x-" isn't: there
are exceptions.

The converse is a wash: we can forget to mark something unstable no
matter how the mark works.

Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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.

Okay.

>> 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, ...

Yes.  But that need not be the end of the argument, it may be the
beginning of one.

> (2) ...but the Sphinx internals are really not meant to be used the way
> we're using them right now, ...

Happy to trust you on this one.

> (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.

Maybe.  Depends on what we get for it.

> 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.

The doc comment language is the result of a series of pragmatic
decisions that got us from semi-accidental comment conventions to where
we are now.  Each of the decisions made sense at the time.  The result
is messy to describe and to parse.  It's limiting and hard to grow
further.

>                            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.

Soldier, hand me the binoculars before you enter the minefield.  Good
luck!

Back to serious: I feel we need to wrap the typing hints work before we
start the next big QAPI schema language project.

>> >>  # 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 :)

You stating the limits of your review is useful.

Me stating the limits of my own research is also useful :)

> 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!
>>
>>

Re: [PATCH 5/9] qapi: Generalize struct member policy checking

[Markus Armbruster]

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

> 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

Good eyes, thank you!

I actually like extract & desposit macros when the width is greater than
one.  Then, the longhand C code is illegible anyway, and having to
remember what the macros mean is no worse.

For width 1 it feels like a wash.  Universal use of the macros could
build familiarity and thus tip the balance.

I count more than a thousand instances of '& (1 <<'.

I wasn't even aware the macros existed in QEMU[*].

>
>> +        return false;
>> +    }


[*] I may well have seen them before, but my memory is limited and
lossy.

Re: [PATCH 9/9] qapi: Extend -compat to set policy for unstable interfaces

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * 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?

The closest we have to marking command line options as unstable is
putting them under "Debug/Expert options" in -help.

For option *parameters*, we sometimes use the x- convention.

QAPIfication will give us better tools.

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 26.10.2021 um 11:37 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben:
>> >> By convention, names starting with "x-" are experimental.  The parts
>> >> of external interfaces so named may be withdrawn or changed
>> >> incompatibly in future releases.
>> >> 
>> >> Drawback: promoting something from experimental to stable involves a
>> >> name change.  Client code needs to be updated.
>> >> 
>> >> Moreover, the convention is not universally observed:
>> >> 
>> >> * QOM type "input-barrier" has properties "x-origin", "y-origin".
>> >>   Looks accidental, but it's ABI since 4.2.
>> >> 
>> >> * QOM types "memory-backend-file", "memory-backend-memfd",
>> >>   "memory-backend-ram", and "memory-backend-epc" have a property
>> >>   "x-use-canonical-path-for-ramblock-id" that is documented to be
>> >>   stable despite its name.
>> >> 
>> >> We could document these exceptions, but documentation helps only
>> >> humans.  We want to recognize "unstable" in code, like "deprecated".
>> >> 
>> >> Replace the convention by a new special feature flag "unstable".  It
>> >> will be recognized by the QAPI generator, like the existing feature
>> >> flag "deprecated", and unlike regular feature flags.
>> >> 
>> >> This commit updates documentation and prepares tests.  The next commit
>> >> updates the QAPI schema.  The remaining patches update the QAPI
>> >> generator and wire up -compat policy checking.
>> >> 
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >
>> > Obviously, replacing the old convention gets rid of the old drawbacks,
>> > but adds a new one: While using x- makes it very obvious for a human
>> > user that this is an unstable feature, a feature flag in the schema will
>> > almost certainly go unnoticed in manual use.
>> 
>> I thought about this, but neglected to put it in writing.  My bad.
>> 
>> Manual use of unstable interfaces is mostly fine.  Human users can adapt
>> to changing interfaces.  HMP works that way.
>> 
>> Management applications are better off with a feature flag than with a
>> naming convention we sometimes ignore.
>> 
>> The most potential for trouble is in between: programs that aren't
>> full-fledged management applications.
>> 
>> If we want to keep "unstable" obvious to the humans who write such
>> programs, we can continue to require "x-", in addition to the feature
>> flag.  We pay for it with renames, and the risk of forgetting to rename
>> in time (which is what got us the awkward stable
>> "x-use-canonical-path-for-ramblock-id").  Tradeoff.  I chose not to, but
>> if y'all think we should...
>
> Just to clarify, I'm not implying that we should keep it. I'm merely
> pointing out that there is a tradeoff that requires us to make a choice.
> The decision for one of the options should be explicit rather than just
> happening as a side effect. Documenting that it was a conscious decision
> is probably best done by adding the reasoning for it to the commit
> message.

I rewrote the commit message for v2.

Thanks!

[...]

Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'

[Eric Blake]

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

Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()

[Markus Armbruster]

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.

Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'

[Kevin Wolf]

Am 29.10.2021 um 15:07 hat Eric Blake geschrieben:
> 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.

There is no reason any more to use the x- options, and I think libvirt
never used them anyway.

I actually have a commit in my QAPI object branch that removes these
properties, but I think it still broke some tests.

Anyway, something for a separate patch.

Kevin