[PATCH 00/18] qapi/qom: QAPIfy object-add

[PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

This series adds a QAPI type for the properties of all user creatable
QOM types and finally makes QMP object-add use the new ObjectOptions
union so that QAPI introspection can be used for user creatable objects.

If you are in the CC list and didn't expect this series, it's probably
because you're the maintainer of one of the objects for which I'm adding
a QAPI schema description. Please just have a look at the specific patch
for your object and check whether the schema and its documentation make
sense to you. You can ignore all other patches.


After this series, there is least one obvious next step that needs to be
done: Change HMP and all of the command line parser to use
ObjectOptions, too, so that the QAPI schema is consistently enforced in
all external interfaces. I am planning to send another series to address
this.

In a third step, we can try to start deduplicating and integrating things
better between QAPI and the QOM implementation, e.g. by generating parts
of the QOM boilerplate from the QAPI schema.

Kevin Wolf (18):
  qapi/qom: Add ObjectOptions for iothread
  qapi/qom: Add ObjectOptions for authz-*
  qapi/qom: Add ObjectOptions for cryptodev-*
  qapi/qom: Add ObjectOptions for dbus-vmstate
  qapi/qom: Add ObjectOptions for memory-backend-*
  qapi/qom: Add ObjectOptions for rng-*, deprecate 'opened'
  qapi/qom: Add ObjectOptions for throttle-group
  qapi/qom: Add ObjectOptions for secret*, deprecate 'loaded'
  qapi/qom: Add ObjectOptions for tls-*, deprecate 'loaded'
  qapi/qom: Add ObjectOptions for can-*
  qapi/qom: Add ObjectOptions for colo-compare
  qapi/qom: Add ObjectOptions for filter-*
  qapi/qom: Add ObjectOptions for pr-manager-helper
  qapi/qom: Add ObjectOptions for sev-guest
  qapi/qom: Add ObjectOptions for input-*
  tests: Drop 'props' from object-add calls
  qapi/qom: Drop deprecated 'props' from object-add
  qapi/qom: QAPIfy object-add

 qapi/authz.json                      |  62 +++
 qapi/block-core.json                 |  12 +
 qapi/common.json                     |  52 +++
 qapi/crypto.json                     | 159 +++++++
 qapi/machine.json                    |  22 +-
 qapi/net.json                        |  20 -
 qapi/qom.json                        | 609 ++++++++++++++++++++++++++-
 qapi/ui.json                         |  13 +-
 docs/system/deprecated.rst           |  29 +-
 include/qom/object_interfaces.h      |   7 -
 hw/block/xen-block.c                 |  16 +-
 monitor/misc.c                       |   2 -
 qom/qom-qmp-cmds.c                   |  44 +-
 tests/qtest/qmp-cmd-test.c           |  16 +-
 tests/qtest/test-netfilter.c         |  54 ++-
 storage-daemon/qapi/qapi-schema.json |   1 +
 tests/qemu-iotests/087               |   8 +-
 tests/qemu-iotests/184               |  18 +-
 tests/qemu-iotests/218               |   2 +-
 tests/qemu-iotests/235               |   2 +-
 tests/qemu-iotests/245               |   4 +-
 tests/qemu-iotests/258               |   6 +-
 tests/qemu-iotests/258.out           |   4 +-
 tests/qemu-iotests/295               |   2 +-
 tests/qemu-iotests/296               |   2 +-
 25 files changed, 993 insertions(+), 173 deletions(-)

-- 
2.28.0

[PATCH 01/18] qapi/qom: Add ObjectOptions for iothread

[Kevin Wolf]

Add an ObjectOptions union that will eventually describe the options of
all user creatable object types. As unions can't exist without any
branches, also add the first object type.

This adds a QAPI schema for the properties of the iothread object.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qom.json | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index 0b0b92944b..57d1386758 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -202,6 +202,59 @@
   'returns': [ 'ObjectPropertyInfo' ],
   'allow-preconfig': true }
 
+##
+# @IothreadProperties:
+#
+# Properties for iothread objects.
+#
+# @poll-max-ns: the maximum number of nanoseconds to busy wait for events.
+#               0 means polling is disabled (default: 32768 on POSIX hosts,
+#               0 otherwise)
+#
+# @poll-grow: the multiplier used to increase the polling time when the
+#             algorithm detects it is missing events due to not polling long
+#             enough. 0 selects a default behaviour (default: 0)
+#
+# @poll-shrink: the divisor used to decrease the polling time when the
+#               algorithm detects it is spending too long polling without
+#               encountering events. 0 selects a default behaviour (default: 0)
+#
+# Since: 6.0
+##
+{ 'struct': 'IothreadProperties',
+  'data': { '*poll-max-ns': 'int',
+            '*poll-grow': 'int',
+            '*poll-shrink': 'int' } }
+
+##
+# @ObjectType:
+#
+# Since: 6.0
+##
+{ 'enum': 'ObjectType',
+  'data': [
+    'iothread'
+  ] }
+
+##
+# @ObjectOptions:
+#
+# Describes the options of a user creatable QOM object.
+#
+# @qom-type: the class name for the object to be created
+#
+# @id: the name of the new object
+#
+# Since: 6.0
+##
+{ 'union': 'ObjectOptions',
+  'base': { 'qom-type': 'ObjectType',
+            'id': 'str' },
+  'discriminator': 'qom-type',
+  'data': {
+      'iothread':                   'IothreadProperties'
+  } }
+
 ##
 # @object-add:
 #
-- 
2.28.0

Re: [PATCH 01/18] qapi/qom: Add ObjectOptions for iothread

[Paolo Bonzini]

On 30/11/20 13:25, Kevin Wolf wrote:
> +##
> +# @IothreadProperties:
> +#
> +# Properties for iothread objects.
> +#
> +# @poll-max-ns: the maximum number of nanoseconds to busy wait for events.
> +#               0 means polling is disabled (default: 32768 on POSIX hosts,
> +#               0 otherwise)
> +#
> +# @poll-grow: the multiplier used to increase the polling time when the
> +#             algorithm detects it is missing events due to not polling long
> +#             enough. 0 selects a default behaviour (default: 0)
> +#
> +# @poll-shrink: the divisor used to decrease the polling time when the
> +#               algorithm detects it is spending too long polling without
> +#               encountering events. 0 selects a default behaviour (default: 0)
> +#
> +# Since: 6.0
> +##
> +{ 'struct': 'IothreadProperties',
> +  'data': { '*poll-max-ns': 'int',
> +            '*poll-grow': 'int',
> +            '*poll-shrink': 'int' } }
> +

Documentation is the main advantage of the ObjectOptions concept. 
However, please use the version where each object and property was 
introduced for the "since" value.  Otherwise the documentation will 
appear to show that none of these objects was available before 6.0.

Yes, there is no documentation at all right now for QOM objects. 
However, wrong documentation sometimes is worse than non-existing 
documentation.

Paolo

Re: [PATCH 01/18] qapi/qom: Add ObjectOptions for iothread

[Kevin Wolf]

Am 30.11.2020 um 16:00 hat Paolo Bonzini geschrieben:
> On 30/11/20 13:25, Kevin Wolf wrote:
> > +##
> > +# @IothreadProperties:
> > +#
> > +# Properties for iothread objects.
> > +#
> > +# @poll-max-ns: the maximum number of nanoseconds to busy wait for events.
> > +#               0 means polling is disabled (default: 32768 on POSIX hosts,
> > +#               0 otherwise)
> > +#
> > +# @poll-grow: the multiplier used to increase the polling time when the
> > +#             algorithm detects it is missing events due to not polling long
> > +#             enough. 0 selects a default behaviour (default: 0)
> > +#
> > +# @poll-shrink: the divisor used to decrease the polling time when the
> > +#               algorithm detects it is spending too long polling without
> > +#               encountering events. 0 selects a default behaviour (default: 0)
> > +#
> > +# Since: 6.0
> > +##
> > +{ 'struct': 'IothreadProperties',
> > +  'data': { '*poll-max-ns': 'int',
> > +            '*poll-grow': 'int',
> > +            '*poll-shrink': 'int' } }
> > +
> 
> Documentation is the main advantage of the ObjectOptions concept. However,
> please use the version where each object and property was introduced for the
> "since" value.  Otherwise the documentation will appear to show that none of
> these objects was available before 6.0.
> 
> Yes, there is no documentation at all right now for QOM objects. However,
> wrong documentation sometimes is worse than non-existing documentation.

I think we generally use the version when the schema was introduced (so
blockdev-add has 2.9 for most things even if they existed before in
-drive and drive_add), but I agree that your suggestion is more useful.
And object-add isn't a new command, we're just giving it a schema now.

So let's first discuss the general direction, but if we agree on that,
using the version numbers where objects and properties were first
introduced makes sense.

Maybe if maintainers can include the right version numbers in the review
of the patch for "their" object, that would help me updating the
patches.

Kevin

[PATCH 02/18] qapi/qom: Add ObjectOptions for authz-*

[Kevin Wolf]

This adds a QAPI schema for the properties of the authz-* objects.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/authz.json                      | 62 ++++++++++++++++++++++++++++
 qapi/qom.json                        | 10 +++++
 storage-daemon/qapi/qapi-schema.json |  1 +
 3 files changed, 73 insertions(+)

diff --git a/qapi/authz.json b/qapi/authz.json
index 42afe752d1..ac72ad5c9e 100644
--- a/qapi/authz.json
+++ b/qapi/authz.json
@@ -59,3 +59,65 @@
 ##
 { 'struct': 'QAuthZListRuleListHack',
   'data': { 'unused': ['QAuthZListRule'] } }
+
+##
+# @AuthZListProperties:
+#
+# Properties for authz-list objects.
+#
+# @policy: Default policy to apply when no rule matches (default: deny)
+#
+# @rules: Authorization rules based on matching user
+#
+# Since: 6.0
+##
+{ 'struct': 'AuthZListProperties',
+  'data': { '*policy': 'QAuthZListPolicy',
+            '*rules': ['QAuthZListRule'] } }
+
+##
+# @AuthZListFileProperties:
+#
+# Properties for authz-listfile objects.
+#
+# @filename: File name to load the configuration from. The file must
+#            contain valid JSON for AuthZListProperties.
+#
+# @refresh: If true, inotify is used to monitor the file, automatically
+#           reloading changes. If an error occurs during reloading, all
+#           authorizations will fail until the file is next successfully
+#           loaded. (default: true if the binary was built with
+#           CONFIG_INOTIFY1, false otherwise)
+#
+# Since: 6.0
+##
+{ 'struct': 'AuthZListFileProperties',
+  'data': { 'filename': 'str',
+            '*refresh': 'bool' } }
+
+##
+# @AuthZPAMProperties:
+#
+# Properties for authz-pam objects.
+#
+# @service: PAM service name to use for authorization
+#
+# Since: 6.0
+##
+{ 'struct': 'AuthZPAMProperties',
+  'data': { 'service': 'str' } }
+
+##
+# @AuthZSimpleProperties:
+#
+# Properties for authz-simple objects.
+#
+# @identity: Identifies the allowed user. Its format depends on the network
+#            service that authorization object is associated with. For
+#            authorizing based on TLS x509 certificates, the identity must be
+#            the x509 distinguished name.
+#
+# Since: 6.0
+##
+{ 'struct': 'AuthZSimpleProperties',
+  'data': { 'identity': 'str' } }
diff --git a/qapi/qom.json b/qapi/qom.json
index 57d1386758..0ac4b1c9fb 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -4,6 +4,8 @@
 # This work is licensed under the terms of the GNU GPL, version 2 or later.
 # See the COPYING file in the top-level directory.
 
+{ 'include': 'authz.json' }
+
 ##
 # = QEMU Object Model (QOM)
 ##
@@ -233,6 +235,10 @@
 ##
 { 'enum': 'ObjectType',
   'data': [
+    'authz-list',
+    'authz-listfile',
+    'authz-pam',
+    'authz-simple',
     'iothread'
   ] }
 
@@ -252,6 +258,10 @@
             'id': 'str' },
   'discriminator': 'qom-type',
   'data': {
+      'authz-list':                 'AuthZListProperties',
+      'authz-listfile':             'AuthZListFileProperties',
+      'authz-pam':                  'AuthZPAMProperties',
+      'authz-simple':               'AuthZSimpleProperties',
       'iothread':                   'IothreadProperties'
   } }
 
diff --git a/storage-daemon/qapi/qapi-schema.json b/storage-daemon/qapi/qapi-schema.json
index c6ad5ae1e3..39982d8cac 100644
--- a/storage-daemon/qapi/qapi-schema.json
+++ b/storage-daemon/qapi/qapi-schema.json
@@ -23,6 +23,7 @@
 { 'include': '../../qapi/crypto.json' }
 { 'include': '../../qapi/introspect.json' }
 { 'include': '../../qapi/job.json' }
+{ 'include': '../../qapi/authz.json' }
 { 'include': '../../qapi/qom.json' }
 { 'include': '../../qapi/sockets.json' }
 { 'include': '../../qapi/transaction.json' }
-- 
2.28.0

[PATCH 03/18] qapi/qom: Add ObjectOptions for cryptodev-*

[Kevin Wolf]

This adds a QAPI schema for the properties of the cryptodev-* objects.

These interfaces have some questionable aspects (cryptodev-backend is
really an abstract base class without function, and the queues option
only makes sense for cryptodev-vhost-user), but as the goal is to
represent the existing interface in QAPI, leave these things in place.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qom.json | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index 0ac4b1c9fb..7cbc0a3c54 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -204,6 +204,34 @@
   'returns': [ 'ObjectPropertyInfo' ],
   'allow-preconfig': true }
 
+##
+# @CryptodevBackendProperties:
+#
+# Properties for cryptodev-backend and cryptodev-backend-builtin objects.
+#
+# @queues: the number of queues for the cryptodev backend. Ignored for
+#          cryptodev-backend and must be 1 for cryptodev-backend-builtin.
+#          (default: 1)
+#
+# Since: 6.0
+##
+{ 'struct': 'CryptodevBackendProperties',
+  'data': { '*queues': 'uint32' } }
+
+##
+# @CryptodevVhostUserProperties:
+#
+# Properties for cryptodev-vhost-user objects.
+#
+# @chardev: the name of a unix domain socket character device that connects to
+#           the vhost-user server
+#
+# Since: 6.0
+##
+{ 'struct': 'CryptodevVhostUserProperties',
+  'base': 'CryptodevBackendProperties',
+  'data': { 'chardev': 'str' } }
+
 ##
 # @IothreadProperties:
 #
@@ -239,6 +267,9 @@
     'authz-listfile',
     'authz-pam',
     'authz-simple',
+    'cryptodev-backend',
+    'cryptodev-backend-builtin',
+    'cryptodev-vhost-user',
     'iothread'
   ] }
 
@@ -262,6 +293,9 @@
       'authz-listfile':             'AuthZListFileProperties',
       'authz-pam':                  'AuthZPAMProperties',
       'authz-simple':               'AuthZSimpleProperties',
+      'cryptodev-backend':          'CryptodevBackendProperties',
+      'cryptodev-backend-builtin':  'CryptodevBackendProperties',
+      'cryptodev-vhost-user':       'CryptodevVhostUserProperties',
       'iothread':                   'IothreadProperties'
   } }
 
-- 
2.28.0

[PATCH 04/18] qapi/qom: Add ObjectOptions for dbus-vmstate

[Kevin Wolf]

This adds a QAPI schema for the properties of the dbus-vmstate object.

A list represented as a comma separated string is clearly not very
QAPI-like, but for now just describe the existing interface.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qom.json | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index 7cbc0a3c54..2319c9bad6 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -232,6 +232,22 @@
   'base': 'CryptodevBackendProperties',
   'data': { 'chardev': 'str' } }
 
+##
+# @DBusVMStateProperties:
+#
+# Properties for dbus-vmstate objects.
+#
+# @addr: the name of the DBus bus to connect to
+#
+# @id-list: a comma separated list of DBus IDs of helpers whose data should be
+#           included in the VM state on migration
+#
+# Since: 6.0
+##
+{ 'struct': 'DBusVMStateProperties',
+  'data': { 'addr': 'str' ,
+            '*id-list': 'str' } }
+
 ##
 # @IothreadProperties:
 #
@@ -270,6 +286,7 @@
     'cryptodev-backend',
     'cryptodev-backend-builtin',
     'cryptodev-vhost-user',
+    'dbus-vmstate',
     'iothread'
   ] }
 
@@ -296,6 +313,7 @@
       'cryptodev-backend':          'CryptodevBackendProperties',
       'cryptodev-backend-builtin':  'CryptodevBackendProperties',
       'cryptodev-vhost-user':       'CryptodevVhostUserProperties',
+      'dbus-vmstate':               'DBusVMStateProperties',
       'iothread':                   'IothreadProperties'
   } }
 
-- 
2.28.0

[PATCH 05/18] qapi/qom: Add ObjectOptions for memory-backend-*

[Kevin Wolf]

This adds a QAPI schema for the properties of the memory-backend-*
objects.

HostMemPolicy has to be moved to an include file that can be used by the
storage daemon, too, because ObjectOptions must be the same in all
binaries if we don't want to compile the whole code multiple times.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/common.json  |  20 +++++++++
 qapi/machine.json |  22 +---------
 qapi/qom.json     | 106 +++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 126 insertions(+), 22 deletions(-)

diff --git a/qapi/common.json b/qapi/common.json
index 716712d4b3..2dad4fadc3 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -145,3 +145,23 @@
 ##
 { 'enum': 'PCIELinkWidth',
   'data': [ '1', '2', '4', '8', '12', '16', '32' ] }
+
+##
+# @HostMemPolicy:
+#
+# Host memory policy types
+#
+# @default: restore default policy, remove any nondefault policy
+#
+# @preferred: set the preferred host nodes for allocation
+#
+# @bind: a strict policy that restricts memory allocation to the
+#        host nodes specified
+#
+# @interleave: memory allocations are interleaved across the set
+#              of host nodes specified
+#
+# Since: 2.1
+##
+{ 'enum': 'HostMemPolicy',
+  'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
diff --git a/qapi/machine.json b/qapi/machine.json
index 7c9a263778..75b9737213 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -8,6 +8,8 @@
 # = Machines
 ##
 
+{ 'include': 'common.json' }
+
 ##
 # @SysEmuTarget:
 #
@@ -897,26 +899,6 @@
    'policy': 'HmatCacheWritePolicy',
    'line': 'uint16' }}
 
-##
-# @HostMemPolicy:
-#
-# Host memory policy types
-#
-# @default: restore default policy, remove any nondefault policy
-#
-# @preferred: set the preferred host nodes for allocation
-#
-# @bind: a strict policy that restricts memory allocation to the
-#        host nodes specified
-#
-# @interleave: memory allocations are interleaved across the set
-#              of host nodes specified
-#
-# Since: 2.1
-##
-{ 'enum': 'HostMemPolicy',
-  'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
-
 ##
 # @memsave:
 #
diff --git a/qapi/qom.json b/qapi/qom.json
index 2319c9bad6..f8a1da43ad 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -5,6 +5,7 @@
 # See the COPYING file in the top-level directory.
 
 { 'include': 'authz.json' }
+{ 'include': 'common.json' }
 
 ##
 # = QEMU Object Model (QOM)
@@ -272,6 +273,101 @@
             '*poll-grow': 'int',
             '*poll-shrink': 'int' } }
 
+##
+# @MemoryBackendProperties:
+#
+# Properties for objects of classes derived from memory-backend.
+#
+# @merge: if true, mark the memory as mergeable (default depends on the machine
+#         type)
+#
+# @dump: if true, include the memory in core dumps (default depends on the
+#        machine type)
+#
+# @host-nodes: the list of NUMA host nodes to bind the memory to
+#
+# @policy: the NUMA policy (default: 'default')
+#
+# @prealloc: if true, preallocate memory (default: false)
+#
+# @prealloc-threads: number of CPU threads to use for prealloc (default: 1)
+#
+# @share: if false, the memory is private to QEMU; if true, it is shared
+#         (default: false)
+#
+# @size: size of the memory region in bytes
+#
+# Since: 6.0
+##
+{ 'struct': 'MemoryBackendProperties',
+  'data': { '*dump': 'bool',
+            '*host-nodes': ['uint16'],
+            '*merge': 'bool',
+            '*policy': 'HostMemPolicy',
+            '*prealloc': 'bool',
+            '*prealloc-threads': 'uint32',
+            '*share': 'bool',
+            'size': 'size' } }
+
+##
+# @MemoryBackendFileProperties:
+#
+# Properties for memory-backend-file objects.
+#
+# @align: the base address alignment when QEMU mmap(2) @mem-path. Some
+#         backend store specified by @mem-path requires an alignment different
+#         than the default one used by QEMU, e.g. the device DAX /dev/dax0.0
+#         requires 2M alignment rather than 4K. In such cases, users can
+#         specify the required alignment via this option.
+#         0 selects a default alignment (currently the page size). (default: 0)
+#
+# @discard-data: if true, the file contents can be destroyed when QEMU exits,
+#                to avoid unnecessarily flushing data to the backing file. Note
+#                that ``discard-data`` is only an optimization, and QEMU might
+#                not discard file contents if it aborts unexpectedly or is
+#                terminated using SIGKILL. (default: false)
+#
+# @mem-path: the path to either a shared memory or huge page filesystem mount
+#
+# @pmem: specifies whether the backing file specified by @mem-path is in
+#        host persistent memory that can be accessed using the SNIA NVM
+#        programming model (e.g. Intel NVDIMM).
+#
+# Since: 6.0
+##
+{ 'struct': 'MemoryBackendFileProperties',
+  'base': 'MemoryBackendProperties',
+  'data': { '*align': 'size',
+            '*discard-data': 'bool',
+            'mem-path': 'str',
+            '*pmem': 'bool' } }
+
+##
+# @MemoryBackendMemfdProperties:
+#
+# Properties for memory-backend-memfd objects.
+#
+# The @share boolean option is true by default with memfd.
+#
+# @hugetlb: if true, the file to be created resides in the hugetlbfs filesystem
+#           (default: false)
+#
+# @hugetlbsize: the hugetlb page size on systems that support multiple hugetlb
+#               page sizes (it must be a power of 2 value supported by the
+#               system). 0 selects a default page size. This option is ignored
+#               if @hugetlb is false. (default: 0)
+#
+# @seal: if true, create a sealed-file, which will block further resizing of
+#        the memory (default: true)
+#
+# Since: 6.0
+##
+{ 'struct': 'MemoryBackendMemfdProperties',
+  'base': 'MemoryBackendProperties',
+  'data': { '*hugetlb': 'bool',
+            '*hugetlbsize': 'size',
+            '*seal': 'bool' } }
+
 ##
 # @ObjectType:
 #
@@ -287,7 +383,10 @@
     'cryptodev-backend-builtin',
     'cryptodev-vhost-user',
     'dbus-vmstate',
-    'iothread'
+    'iothread',
+    'memory-backend-file',
+    'memory-backend-memfd',
+    'memory-backend-ram'
   ] }
 
 ##
@@ -314,7 +413,10 @@
       'cryptodev-backend-builtin':  'CryptodevBackendProperties',
       'cryptodev-vhost-user':       'CryptodevVhostUserProperties',
       'dbus-vmstate':               'DBusVMStateProperties',
-      'iothread':                   'IothreadProperties'
+      'iothread':                   'IothreadProperties',
+      'memory-backend-file':        'MemoryBackendFileProperties',
+      'memory-backend-memfd':       'MemoryBackendMemfdProperties',
+      'memory-backend-ram':         'MemoryBackendProperties'
   } }
 
 ##
-- 
2.28.0

[PATCH 06/18] qapi/qom: Add ObjectOptions for rng-*, deprecate 'opened'

[Kevin Wolf]

This adds a QAPI schema for the properties of the rng-* objects.

The 'opened' property doesn't seem to make sense as an external
interface: It is automatically set to true in ucc->complete, and
explicitly setting it to true earlier just means that trying to set
additional options will result in an error. After the property has once
been set to true (i.e. when the object construction has completed), it
can never be reset to false. In other words, the 'opened' property is
useless. Mark it as deprecated in the schema from the start.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qom.json              | 56 ++++++++++++++++++++++++++++++++++++--
 docs/system/deprecated.rst |  9 ++++++
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index f8a1da43ad..f3d1a55cb8 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -368,6 +368,52 @@
             '*hugetlbsize': 'size',
             '*seal': 'bool' } }
 
+##
+# @RngProperties:
+#
+# Properties for objects of classes derived from rng.
+#
+# @opened: if true, the device is opened immediately when applying this option
+#          and will probably fail when processing the next option. Don't use;
+#          only provided for compatibility. (default: false)
+#
+# Features:
+# @deprecated: Member @opened is deprecated.  Setting true doesn't make sense,
+#              and false is already the default.
+#
+# Since: 6.0
+##
+{ 'struct': 'RngProperties',
+  'data': { '*opened': { 'type': 'bool', 'features': ['deprecated'] } } }
+
+##
+# @RngEgdProperties:
+#
+# Properties for rng-egd objects.
+#
+# @chardev: the name of a character device backend that provides the connection
+#           to the RNG daemon
+#
+# Since: 6.0
+##
+{ 'struct': 'RngEgdProperties',
+  'base': 'RngProperties',
+  'data': { 'chardev': 'str' } }
+
+##
+# @RngRandomProperties:
+#
+# Properties for rng-random objects.
+#
+# @filename: the filename of the device on the host to obtain entropy from
+#            (default: "/dev/urandom")
+#
+# Since: 6.0
+##
+{ 'struct': 'RngRandomProperties',
+  'base': 'RngProperties',
+  'data': { '*filename': 'str' } }
+
 ##
 # @ObjectType:
 #
@@ -386,7 +432,10 @@
     'iothread',
     'memory-backend-file',
     'memory-backend-memfd',
-    'memory-backend-ram'
+    'memory-backend-ram',
+    'rng-builtin',
+    'rng-egd',
+    'rng-random'
   ] }
 
 ##
@@ -416,7 +465,10 @@
       'iothread':                   'IothreadProperties',
       'memory-backend-file':        'MemoryBackendFileProperties',
       'memory-backend-memfd':       'MemoryBackendMemfdProperties',
-      'memory-backend-ram':         'MemoryBackendProperties'
+      'memory-backend-ram':         'MemoryBackendProperties',
+      'rng-builtin':                'RngProperties',
+      'rng-egd':                    'RngEgdProperties',
+      'rng-random':                 'RngRandomProperties'
   } }
 
 ##
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 565389697e..30a694a39a 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -146,6 +146,15 @@ Drives with interface types other than ``if=none`` are for onboard
 devices.  It is possible to use drives the board doesn't pick up with
 -device.  This usage is now deprecated.  Use ``if=none`` instead.
 
+``opened```property of ``rng-*```objects (since 6.0.0)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+The only effect of specifying ``opened=on`` in the command line or QMP
+``object-add`` is that the device is opened immediately, possibly before all
+other options have been processed.  This will either have no effect (if
+``opened`` was the last option) or cause errors.  The property is therefore
+useless and should not be specified.
+
 
 QEMU Machine Protocol (QMP) commands
 ------------------------------------
-- 
2.28.0

[PATCH 07/18] qapi/qom: Add ObjectOptions for throttle-group

[Kevin Wolf]

This adds a QAPI schema for the properties of the throttle-group object.

The x-* properties are not represented in the schema. Their only purpose
is to make the nested options in 'limits' available for a command line
parser that doesn't support structs. Any parser that will use the QAPI
schema will supports structs, though, so they will not be needed in the
schema.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json | 12 ++++++++++++
 qapi/qom.json        |  7 +++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 04ad80bc1e..d7a4cdc11c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2478,6 +2478,18 @@
             '*bps-write-max' : 'int', '*bps-write-max-length' : 'int',
             '*iops-size' : 'int' } }
 
+##
+# @ThrottleGroupProperties:
+#
+# Properties for throttle-group objects.
+#
+# @limits: limits to apply for this throttle group
+#
+# Since: 6.0
+##
+{ 'struct': 'ThrottleGroupProperties',
+  'data': { '*limits': 'ThrottleLimits' } }
+
 ##
 # @block-stream:
 #
diff --git a/qapi/qom.json b/qapi/qom.json
index f3d1a55cb8..8c9785d2dd 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -5,6 +5,7 @@
 # See the COPYING file in the top-level directory.
 
 { 'include': 'authz.json' }
+{ 'include': 'block-core.json' }
 { 'include': 'common.json' }
 
 ##
@@ -435,7 +436,8 @@
     'memory-backend-ram',
     'rng-builtin',
     'rng-egd',
-    'rng-random'
+    'rng-random',
+    'throttle-group'
   ] }
 
 ##
@@ -468,7 +470,8 @@
       'memory-backend-ram':         'MemoryBackendProperties',
       'rng-builtin':                'RngProperties',
       'rng-egd':                    'RngEgdProperties',
-      'rng-random':                 'RngRandomProperties'
+      'rng-random':                 'RngRandomProperties',
+      'throttle-group':             'ThrottleGroupProperties'
   } }
 
 ##
-- 
2.28.0

[PATCH 08/18] qapi/qom: Add ObjectOptions for secret*, deprecate 'loaded'

[Kevin Wolf]

This adds a QAPI schema for the properties of the secret* objects.

The 'loaded' property doesn't seem to make sense as an external
interface: It is automatically set to true in ucc->complete, and
explicitly setting it to true earlier just means that additional options
will be silently ignored.

In other words, the 'loaded' property is useless. Mark it as deprecated
in the schema from the start.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/crypto.json           | 61 ++++++++++++++++++++++++++++++++++++++
 qapi/qom.json              |  5 ++++
 docs/system/deprecated.rst | 10 +++++++
 3 files changed, 76 insertions(+)

diff --git a/qapi/crypto.json b/qapi/crypto.json
index 2aebe6fa20..4cb6bb00ed 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -381,3 +381,64 @@
   'discriminator': 'format',
   'data': {
           'luks': 'QCryptoBlockAmendOptionsLUKS' } }
+
+##
+# @SecretCommonProperties:
+#
+# Properties for objects of classes derived from secret-common.
+#
+# @loaded: if true, the secret is loaded immediately when applying this option
+#          and will probably fail when processing the next option. Don't use;
+#          only provided for compatibility. (default: false)
+#
+# @format: the data format that the secret is provided in (default: raw)
+#
+# @keyid: the name of another secret that should be used to decrypt the
+#         provided data. If not present, the data is assumed to be unencrypted.
+#
+# @iv: the random initialization vector used for encryption of this particular
+#      secret. Should be a base64 encrypted string of the 16-byte IV. Mandatory
+#      if @keyid is given. Ignored if @keyid is absent.
+#
+# Features:
+# @deprecated: Member @loaded is deprecated.  Setting true doesn't make sense,
+#              and false is already the default.
+#
+# Since: 6.0
+##
+{ 'struct': 'SecretCommonProperties',
+  'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
+            '*format': 'QCryptoSecretFormat',
+            '*keyid': 'str',
+            '*iv': 'str' } }
+
+##
+# @SecretProperties:
+#
+# Properties for secret objects.
+#
+# Either @data or @file must be provided, but not both.
+#
+# @data: the associated with the secret from
+#
+# @file: the filename to load the data associated with the secret from
+#
+# Since: 6.0
+##
+{ 'struct': 'SecretProperties',
+  'base': 'SecretCommonProperties',
+  'data': { '*data': 'str',
+            '*file': 'str' } }
+
+##
+# @SecretKeyringProperties:
+#
+# Properties for secret_keyring objects.
+#
+# @serial: serial number that identifies a key to get from the kernel
+#
+# Since: 6.0
+##
+{ 'struct': 'SecretKeyringProperties',
+  'base': 'SecretCommonProperties',
+  'data': { 'serial': 'int32' } }
diff --git a/qapi/qom.json b/qapi/qom.json
index 8c9785d2dd..05ec67b0d1 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -7,6 +7,7 @@
 { 'include': 'authz.json' }
 { 'include': 'block-core.json' }
 { 'include': 'common.json' }
+{ 'include': 'crypto.json' }
 
 ##
 # = QEMU Object Model (QOM)
@@ -437,6 +438,8 @@
     'rng-builtin',
     'rng-egd',
     'rng-random',
+    'secret',
+    'secret_keyring',
     'throttle-group'
   ] }
 
@@ -471,6 +474,8 @@
       'rng-builtin':                'RngProperties',
       'rng-egd':                    'RngEgdProperties',
       'rng-random':                 'RngRandomProperties',
+      'secret':                     'SecretProperties',
+      'secret_keyring':             'SecretKeyringProperties',
       'throttle-group':             'ThrottleGroupProperties'
   } }
 
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 30a694a39a..04e41254f9 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -155,6 +155,16 @@ other options have been processed.  This will either have no effect (if
 ``opened`` was the last option) or cause errors.  The property is therefore
 useless and should not be specified.
 
+``loaded```property of ``secret`` and ``secret_keyring`` objects (since 6.0.0)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+The only effect of specifying ``loaded=on`` in the command line or QMP
+``object-add`` is that the secret is loaded immediately, possibly before all
+other options have been processed.  This will either have no effect (if
+``loaded`` was the last option) or cause options to be effectively ignored as
+if they were not given.  The property is therefore useless and should not be
+specified.
+
 
 QEMU Machine Protocol (QMP) commands
 ------------------------------------
-- 
2.28.0

[PATCH 09/18] qapi/qom: Add ObjectOptions for tls-*, deprecate 'loaded'

[Kevin Wolf]

This adds a QAPI schema for the properties of the tls-* objects.

The 'loaded' property doesn't seem to make sense as an external
interface: It is automatically set to true in ucc->complete, and
explicitly setting it to true earlier just means that additional options
will be silently ignored.

In other words, the 'loaded' property is useless. Mark it as deprecated
in the schema from the start.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/crypto.json | 98 ++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/qom.json    | 12 +++++-
 2 files changed, 108 insertions(+), 2 deletions(-)

diff --git a/qapi/crypto.json b/qapi/crypto.json
index 4cb6bb00ed..a32dfa320a 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -442,3 +442,101 @@
 { 'struct': 'SecretKeyringProperties',
   'base': 'SecretCommonProperties',
   'data': { 'serial': 'int32' } }
+
+##
+# @TlsCredsProperties:
+#
+# Properties for objects of classes derived from tls-creds.
+#
+# @verify-peer: if true the peer credentials will be verified once the
+#               handshake is completed.  This is a no-op for anonymous
+#               credentials. (default: true)
+#
+# @dir: the path of the directory that contains the credential files
+#
+# @endpoint: whether the QEMU network backend that uses the credentials will be
+#            acting as a client or as a server (default: client)
+#
+# @priority: a gnutls priority string as described at
+#            https://gnutls.org/manual/html_node/Priority-Strings.html
+#
+# Since: 6.0
+##
+{ 'struct': 'TlsCredsProperties',
+  'data': { '*verify-peer': 'bool',
+            '*dir': 'str',
+            '*endpoint': 'QCryptoTLSCredsEndpoint',
+            '*priority': 'str' } }
+
+##
+# @TlsCredsAnonProperties:
+#
+# Properties for tls-creds-anon objects.
+#
+# @loaded: if true, the credentials are loaded immediately when applying this
+#          option and will ignore options that are processed later. Don't use;
+#          only provided for compatibility. (default: false)
+#
+# Features:
+# @deprecated: Member @loaded is deprecated.  Setting true doesn't make sense,
+#              and false is already the default.
+#
+# Since: 6.0
+##
+{ 'struct': 'TlsCredsAnonProperties',
+  'base': 'TlsCredsProperties',
+  'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] } } }
+
+##
+# @TlsCredsPskProperties:
+#
+# Properties for tls-creds-psk objects.
+#
+# @loaded: if true, the credentials are loaded immediately when applying this
+#          option and will ignore options that are processed later. Don't use;
+#          only provided for compatibility. (default: false)
+#
+# @username: the username which will be sent to the server.  For clients only.
+#            If absent, "qemu" is sent and the property will read back as an
+#            empty string.
+#
+# Features:
+# @deprecated: Member @loaded is deprecated.  Setting true doesn't make sense,
+#              and false is already the default.
+#
+# Since: 6.0
+##
+{ 'struct': 'TlsCredsPskProperties',
+  'base': 'TlsCredsProperties',
+  'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
+            '*username': 'str' } }
+
+##
+# @TlsCredsX509Properties:
+#
+# Properties for tls-creds-x509 objects.
+#
+# @loaded: if true, the credentials are loaded immediately when applying this
+#          option and will ignore options that are processed later. Don't use;
+#          only provided for compatibility. (default: false)
+#
+# @sanity-check: if true, perform some sanity checks before using the
+#                credentials (default: true)
+#
+# @passwordid: For the server-key.pem and client-key.pem files which contain
+#              sensitive private keys, it is possible to use an encrypted
+#              version by providing the @passwordid parameter.  This provides
+#              the ID of a previously created secret object containing the
+#              password for decryption.
+#
+# Features:
+# @deprecated: Member @loaded is deprecated.  Setting true doesn't make sense,
+#              and false is already the default.
+#
+# Since: 6.0
+##
+{ 'struct': 'TlsCredsX509Properties',
+  'base': 'TlsCredsProperties',
+  'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
+            '*sanity-check': 'bool',
+            '*passwordid': 'str' } }
diff --git a/qapi/qom.json b/qapi/qom.json
index 05ec67b0d1..31e381b3bb 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -440,7 +440,11 @@
     'rng-random',
     'secret',
     'secret_keyring',
-    'throttle-group'
+    'throttle-group',
+    'tls-creds-anon',
+    'tls-creds-psk',
+    'tls-creds-x509',
+    'tls-cipher-suites'
   ] }
 
 ##
@@ -476,7 +480,11 @@
       'rng-random':                 'RngRandomProperties',
       'secret':                     'SecretProperties',
       'secret_keyring':             'SecretKeyringProperties',
-      'throttle-group':             'ThrottleGroupProperties'
+      'throttle-group':             'ThrottleGroupProperties',
+      'tls-creds-anon':             'TlsCredsAnonProperties',
+      'tls-creds-psk':              'TlsCredsPskProperties',
+      'tls-creds-x509':             'TlsCredsX509Properties',
+      'tls-cipher-suites':          'TlsCredsProperties'
   } }
 
 ##
-- 
2.28.0

[PATCH 10/18] qapi/qom: Add ObjectOptions for can-*

[Kevin Wolf]

This adds a QAPI schema for the properties of the can-* objects.

can-bus doesn't have any properties, so it only needs to be added to the
ObjectType enum without adding a new branch to ObjectOptions.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qom.json | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index 31e381b3bb..ff9e51ee19 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -207,6 +207,21 @@
   'returns': [ 'ObjectPropertyInfo' ],
   'allow-preconfig': true }
 
+##
+# @CanHostSocketcanProperties:
+#
+# Properties for can-host-socketcan objects.
+#
+# @if: interface name of the host system CAN bus to connect to
+#
+# @canbus: object ID of the can-bus object to connect to the host interface
+#
+# Since: 6.0
+##
+{ 'struct': 'CanHostSocketcanProperties',
+  'data': { 'if': 'str',
+            'canbus': 'str' } }
+
 ##
 # @CryptodevBackendProperties:
 #
@@ -427,6 +442,8 @@
     'authz-listfile',
     'authz-pam',
     'authz-simple',
+    'can-bus',
+    'can-host-socketcan',
     'cryptodev-backend',
     'cryptodev-backend-builtin',
     'cryptodev-vhost-user',
@@ -467,6 +484,7 @@
       'authz-listfile':             'AuthZListFileProperties',
       'authz-pam':                  'AuthZPAMProperties',
       'authz-simple':               'AuthZSimpleProperties',
+      'can-host-socketcan':         'CanHostSocketcanProperties',
       'cryptodev-backend':          'CryptodevBackendProperties',
       'cryptodev-backend-builtin':  'CryptodevBackendProperties',
       'cryptodev-vhost-user':       'CryptodevVhostUserProperties',
-- 
2.28.0

[PATCH 11/18] qapi/qom: Add ObjectOptions for colo-compare

[Kevin Wolf]

This adds a QAPI schema for the properties of the colo-compare object.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qom.json | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index ff9e51ee19..4c4f2841c3 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -222,6 +222,53 @@
   'data': { 'if': 'str',
             'canbus': 'str' } }
 
+##
+# @ColoCompareProperties:
+#
+# Properties for colo-compare objects.
+#
+# @primary_in: name of the character device backend to use for the primary
+#              input (incoming packets are redirected to @outdev)
+#
+# @secondary_in: name of the character device backend to use for secondary
+#                input (incoming packets are only compared to the input on
+#                @primary_in and then dropped)
+#
+# @outdev: name of the character device backend to use for output
+#
+# @iothread: name of the iothread to run in
+#
+# @notify_dev: name of the character device backend to be used to communicate
+#              with the remote colo-frame (only for Xen COLO)
+#
+# @compare_timeout: the maximum time to hold a packet from @primary_in for
+#                   comparison with an incoming packet on @secondary_in in
+#                   milliseconds (default: 3000)
+#
+# @expired_scan_cycle: the interval at which colo-compare checks whether
+#                      packets from @primary have timed out, in milliseconds
+#                      (default: 3000)
+#
+# @max_queue_size: the maximum number of packets to keep in the queue for
+#                  comparing with incoming packets from @secondary_in.  If the
+#                  queue is full and addtional packets are received, the
+#                  addtional packets are dropped. (default: 1024)
+#
+# @vnet_hdr_support: if true, vnet header support is enabled (default: false)
+#
+# Since: 6.0
+##
+{ 'struct': 'ColoCompareProperties',
+  'data': { 'primary_in': 'str',
+            'secondary_in': 'str',
+            'outdev': 'str',
+            'iothread': 'str',
+            '*notify_dev': 'str',
+            '*compare_timeout': 'uint64',
+            '*expired_scan_cycle': 'uint32',
+            '*max_queue_size': 'uint32',
+            '*vnet_hdr_support': 'bool' } }
+
 ##
 # @CryptodevBackendProperties:
 #
@@ -444,6 +491,7 @@
     'authz-simple',
     'can-bus',
     'can-host-socketcan',
+    'colo-compare',
     'cryptodev-backend',
     'cryptodev-backend-builtin',
     'cryptodev-vhost-user',
@@ -485,6 +533,7 @@
       'authz-pam':                  'AuthZPAMProperties',
       'authz-simple':               'AuthZSimpleProperties',
       'can-host-socketcan':         'CanHostSocketcanProperties',
+      'colo-compare':               'ColoCompareProperties',
       'cryptodev-backend':          'CryptodevBackendProperties',
       'cryptodev-backend-builtin':  'CryptodevBackendProperties',
       'cryptodev-vhost-user':       'CryptodevVhostUserProperties',
-- 
2.28.0

[PATCH 12/18] qapi/qom: Add ObjectOptions for filter-*

[Kevin Wolf]

This adds a QAPI schema for the properties of the filter-* objects.

Some parts of the interface (in particular NetfilterProperties.position)
are very unusual for QAPI, but for now just describe the existing
interface.

net.json can't be included in qom.json because the storage daemon
doesn't have it. NetFilterDirection is still required in the new object
property definitions in qom.json, so move this enum to common.json.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/common.json |  20 +++++++
 qapi/net.json    |  20 -------
 qapi/qom.json    | 143 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 163 insertions(+), 20 deletions(-)

diff --git a/qapi/common.json b/qapi/common.json
index 2dad4fadc3..b87e7f9039 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -165,3 +165,23 @@
 ##
 { 'enum': 'HostMemPolicy',
   'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
+
+##
+# @NetFilterDirection:
+#
+# Indicates whether a netfilter is attached to a netdev's transmit queue or
+# receive queue or both.
+#
+# @all: the filter is attached both to the receive and the transmit
+#       queue of the netdev (default).
+#
+# @rx: the filter is attached to the receive queue of the netdev,
+#      where it will receive packets sent to the netdev.
+#
+# @tx: the filter is attached to the transmit queue of the netdev,
+#      where it will receive packets sent by the netdev.
+#
+# Since: 2.5
+##
+{ 'enum': 'NetFilterDirection',
+  'data': [ 'all', 'rx', 'tx' ] }
diff --git a/qapi/net.json b/qapi/net.json
index a3a1336001..e19f6df468 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -492,26 +492,6 @@
     'vhost-user': 'NetdevVhostUserOptions',
     'vhost-vdpa': 'NetdevVhostVDPAOptions' } }
 
-##
-# @NetFilterDirection:
-#
-# Indicates whether a netfilter is attached to a netdev's transmit queue or
-# receive queue or both.
-#
-# @all: the filter is attached both to the receive and the transmit
-#       queue of the netdev (default).
-#
-# @rx: the filter is attached to the receive queue of the netdev,
-#      where it will receive packets sent to the netdev.
-#
-# @tx: the filter is attached to the transmit queue of the netdev,
-#      where it will receive packets sent by the netdev.
-#
-# Since: 2.5
-##
-{ 'enum': 'NetFilterDirection',
-  'data': [ 'all', 'rx', 'tx' ] }
-
 ##
 # @RxState:
 #
diff --git a/qapi/qom.json b/qapi/qom.json
index 4c4f2841c3..c8ee02081c 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -313,6 +313,137 @@
   'data': { 'addr': 'str' ,
             '*id-list': 'str' } }
 
+##
+# @NetfilterInsert:
+#
+# Indicates where to insert a netfilter relative to a given other filter.
+#
+# @before: insert before the specified filter
+#
+# @behind: insert behind the specified filter
+#
+# Since: 6.0
+##
+{ 'enum': 'NetfilterInsert',
+  'data': [ 'before', 'behind' ] }
+
+##
+# @NetfilterProperties:
+#
+# Properties for objects of classes derived from netfilter.
+#
+# @netdev: id of the network device backend to filter
+#
+# @queue: indicates which queue(s) to filter (default: all)
+#
+# @status: indicates whether the filter is enabled ("on") or disabled ("off")
+#          (default: "on")
+#
+# @position: specifies where the filter should be inserted in the filter list.
+#            "head" means the filter is inserted at the head of the filter list,
+#            before any existing filters.
+#            "tail" means the filter is inserted at the tail of the filter list,
+#            behind any existing filters (default).
+#            "id=<id>" means the filter is inserted before or behind the filter
+#            specified by <id>, depending on the @insert property.
+#            (default: "tail")
+#
+# @insert: where to insert the filter relative to the filter given in @position.
+#          Ignored if @position is "head" or "tail". (default: behind)
+#
+# Since: 6.0
+##
+{ 'struct': 'NetfilterProperties',
+  'data': { 'netdev': 'str',
+            '*queue': 'NetFilterDirection',
+            '*status': 'str',
+            '*position': 'str',
+            '*insert': 'NetfilterInsert' } }
+
+##
+# @FilterBufferProperties:
+#
+# Properties for filter-buffer objects.
+#
+# @interval: a non-zero interval in microseconds.  All packets arriving in the
+#            given interval are delayed until the end of the interval.
+#
+# Since: 6.0
+##
+{ 'struct': 'FilterBufferProperties',
+  'base': 'NetfilterProperties',
+  'data': { 'interval': 'uint32' } }
+
+##
+# @FilterDumpProperties:
+#
+# Properties for filter-dump objects.
+#
+# @file: the filename where the dumped packets should be stored
+#
+# @maxlen: maximum number of bytes in a packet that are stored (default: 65536)
+#
+# Since: 6.0
+##
+{ 'struct': 'FilterDumpProperties',
+  'base': 'NetfilterProperties',
+  'data': { 'file': 'str',
+            '*maxlen': 'uint32' } }
+
+##
+# @FilterMirrorProperties:
+#
+# Properties for filter-mirror objects.
+#
+# @outdev: the name of a character device backend to which all incoming packets
+#          are mirrored
+#
+# @vnet_hdr_support: if true, vnet header support is enabled (default: false)
+#
+# Since: 6.0
+##
+{ 'struct': 'FilterMirrorProperties',
+  'base': 'NetfilterProperties',
+  'data': { 'outdev': 'str',
+            '*vnet_hdr_support': 'bool' } }
+
+##
+# @FilterRedirectorProperties:
+#
+# Properties for filter-redirector objects.
+#
+# At least one of @indev or @outdev must be present.  If both are present, they
+# must not refer to the same character device backend.
+#
+# @indev: the name of a character device backend from which packets are
+#         received and redirected to the filtered network device
+#
+# @outdev: the name of a character device backend to which all incoming packets
+#          are redirected
+#
+# @vnet_hdr_support: if true, vnet header support is enabled (default: false)
+#
+# Since: 6.0
+##
+{ 'struct': 'FilterRedirectorProperties',
+  'base': 'NetfilterProperties',
+  'data': { '*indev': 'str',
+            '*outdev': 'str',
+            '*vnet_hdr_support': 'bool' } }
+
+##
+# @FilterRewriterProperties:
+#
+# Properties for filter-rewriter objects.
+#
+# @vnet_hdr_support: if true, vnet header support is enabled (default: false)
+#
+# Since: 6.0
+##
+{ 'struct': 'FilterRewriterProperties',
+  'base': 'NetfilterProperties',
+  'data': { '*vnet_hdr_support': 'bool' } }
+
 ##
 # @IothreadProperties:
 #
@@ -496,6 +627,12 @@
     'cryptodev-backend-builtin',
     'cryptodev-vhost-user',
     'dbus-vmstate',
+    'filter-buffer',
+    'filter-dump',
+    'filter-mirror',
+    'filter-redirector',
+    'filter-replay',
+    'filter-rewriter',
     'iothread',
     'memory-backend-file',
     'memory-backend-memfd',
@@ -538,6 +675,12 @@
       'cryptodev-backend-builtin':  'CryptodevBackendProperties',
       'cryptodev-vhost-user':       'CryptodevVhostUserProperties',
       'dbus-vmstate':               'DBusVMStateProperties',
+      'filter-buffer':              'FilterBufferProperties',
+      'filter-dump':                'FilterDumpProperties',
+      'filter-mirror':              'FilterMirrorProperties',
+      'filter-redirector':          'FilterRedirectorProperties',
+      'filter-replay':              'NetfilterProperties',
+      'filter-rewriter':            'FilterRewriterProperties',
       'iothread':                   'IothreadProperties',
       'memory-backend-file':        'MemoryBackendFileProperties',
       'memory-backend-memfd':       'MemoryBackendMemfdProperties',
-- 
2.28.0

[PATCH 13/18] qapi/qom: Add ObjectOptions for pr-manager-helper

[Kevin Wolf]

This adds a QAPI schema for the properties of the pr-manager-helper
object.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qom.json | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index c8ee02081c..24bfa83af5 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -563,6 +563,18 @@
             '*hugetlbsize': 'size',
             '*seal': 'bool' } }
 
+##
+# @PrManagerHelperProperties:
+#
+# Properties for pr-manager-helper objects.
+#
+# @path: the path to a Unix domain socket for connecting to the external helper
+#
+# Since: 6.0
+##
+{ 'struct': 'PrManagerHelperProperties',
+  'data': { 'path': 'str' } }
+
 ##
 # @RngProperties:
 #
@@ -637,6 +649,7 @@
     'memory-backend-file',
     'memory-backend-memfd',
     'memory-backend-ram',
+    'pr-manager-helper',
     'rng-builtin',
     'rng-egd',
     'rng-random',
@@ -685,6 +698,7 @@
       'memory-backend-file':        'MemoryBackendFileProperties',
       'memory-backend-memfd':       'MemoryBackendMemfdProperties',
       'memory-backend-ram':         'MemoryBackendProperties',
+      'pr-manager-helper':          'PrManagerHelperProperties',
       'rng-builtin':                'RngProperties',
       'rng-egd':                    'RngEgdProperties',
       'rng-random':                 'RngRandomProperties',
-- 
2.28.0

[PATCH 14/18] qapi/qom: Add ObjectOptions for sev-guest

[Kevin Wolf]

This adds a QAPI schema for the properties of the sev-guest object.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qom.json | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index 24bfa83af5..43d081cb42 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -621,6 +621,38 @@
   'base': 'RngProperties',
   'data': { '*filename': 'str' } }
 
+##
+# @SevGuestProperties:
+#
+# Properties for sev-guest objects.
+#
+# @sev-device: SEV device to use (default: "/dev/sev")
+#
+# @dh-cert-file: guest owners DH certificate (encoded with base64)
+#
+# @session-file: guest owners session parameters (encoded with base64)
+#
+# @policy: SEV policy value (default: 0x1)
+#
+# @handle: SEV firmware handle (default: 0)
+#
+# @cbitpos: C-bit location in page table entry (default: 0)
+#
+# @reduced-phys-bits: number of bits in physical addresses that become
+#                     unavailable when SEV is enabled
+#
+# Since: 6.0
+##
+{ 'struct': 'SevGuestProperties',
+  'data': { '*sev-device': 'str',
+            '*dh-cert-file': 'str',
+            '*session-file': 'str',
+            '*policy': 'uint32',
+            '*handle': 'uint32',
+            '*cbitpos': 'uint32',
+            'reduced-phys-bits': 'uint32' },
+  'if': 'defined(CONFIG_SEV)' }
+
 ##
 # @ObjectType:
 #
@@ -655,6 +687,7 @@
     'rng-random',
     'secret',
     'secret_keyring',
+    {'name': 'sev-guest', 'if': 'defined(CONFIG_SEV)' },
     'throttle-group',
     'tls-creds-anon',
     'tls-creds-psk',
@@ -704,6 +737,8 @@
       'rng-random':                 'RngRandomProperties',
       'secret':                     'SecretProperties',
       'secret_keyring':             'SecretKeyringProperties',
+      'sev-guest':                  { 'type': 'SevGuestProperties',
+                                      'if': 'defined(CONFIG_SEV)' },
       'throttle-group':             'ThrottleGroupProperties',
       'tls-creds-anon':             'TlsCredsAnonProperties',
       'tls-creds-psk':              'TlsCredsPskProperties',
-- 
2.28.0

[PATCH 15/18] qapi/qom: Add ObjectOptions for input-*

[Kevin Wolf]

This adds a QAPI schema for the properties of the input-* objects.

ui.json cannot be included in qom.json because the storage daemon can't
use it, so move GrabToggleKeys to common.json.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/common.json | 12 ++++++++++
 qapi/qom.json    | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/ui.json     | 13 +----------
 3 files changed, 71 insertions(+), 12 deletions(-)

diff --git a/qapi/common.json b/qapi/common.json
index b87e7f9039..7c976296f0 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -185,3 +185,15 @@
 ##
 { 'enum': 'NetFilterDirection',
   'data': [ 'all', 'rx', 'tx' ] }
+
+##
+# @GrabToggleKeys:
+#
+# Keys to toggle input-linux between host and guest.
+#
+# Since: 4.0
+#
+##
+{ 'enum': 'GrabToggleKeys',
+  'data': [ 'ctrl-ctrl', 'alt-alt', 'shift-shift','meta-meta', 'scrolllock',
+            'ctrl-scrolllock' ] }
diff --git a/qapi/qom.json b/qapi/qom.json
index 43d081cb42..29b229394e 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -444,6 +444,60 @@
   'base': 'NetfilterProperties',
   'data': { '*vnet_hdr_support': 'bool' } }
 
+##
+# @InputBarrierProperties:
+#
+# Properties for input-barrier objects.
+#
+# @name: the screen name as declared in the screens section of barrier.conf
+#
+# @server: hostname of the Barrier server (default: "localhost")
+#
+# @port: TCP port of the Barrier server (default: "24800")
+#
+# @x-origin: x coordinate of the leftmost pixel on the guest screen
+#            (default: "0")
+#
+# @y-origin: y coordinate of he topmost pixel on the guest screen (default: "0")
+#
+# @width: the width of secondary screen in pixels (default: "1920")
+#
+# @height: the height of secondary screen in pixels (default: "1080")
+#
+# Since: 6.0
+##
+{ 'struct': 'InputBarrierProperties',
+  'data': { 'name': 'str',
+            '*server': 'str',
+            '*port': 'str',
+            '*x-origin': 'str',
+            '*y-origin': 'str',
+            '*width': 'str',
+            '*height': 'str' } }
+
+##
+# @InputLinuxProperties:
+#
+# Properties for input-linux objects.
+#
+# @evdev: the path of the host evdev device to use
+#
+# @grab_all: if true, grab is toggled for all devices (e.g. both keyboard and
+#            mouse) instead of just one device (default: false)
+#
+# @repeat: enables auto-repeat events (default: false)
+#
+# @grab-toggle: the key or key combination that toggles device grab
+#               (default: ctrl-ctrl)
+#
+# Since: 6.0
+##
+{ 'struct': 'InputLinuxProperties',
+  'data': { 'evdev': 'str',
+            '*grab_all': 'bool',
+            '*repeat': 'bool',
+            '*grab-toggle': 'GrabToggleKeys' } }
+
 ##
 # @IothreadProperties:
 #
@@ -677,6 +731,8 @@
     'filter-redirector',
     'filter-replay',
     'filter-rewriter',
+    'input-barrier',
+    'input-linux',
     'iothread',
     'memory-backend-file',
     'memory-backend-memfd',
@@ -727,6 +783,8 @@
       'filter-redirector':          'FilterRedirectorProperties',
       'filter-replay':              'NetfilterProperties',
       'filter-rewriter':            'FilterRewriterProperties',
+      'input-barrier':              'InputBarrierProperties',
+      'input-linux':                'InputLinuxProperties',
       'iothread':                   'IothreadProperties',
       'memory-backend-file':        'MemoryBackendFileProperties',
       'memory-backend-memfd':       'MemoryBackendMemfdProperties',
diff --git a/qapi/ui.json b/qapi/ui.json
index 6c7b33cb72..bdbf6d076d 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -6,6 +6,7 @@
 # = Remote desktop
 ##
 
+{ 'include': 'common.json' }
 { 'include': 'sockets.json' }
 
 ##
@@ -1021,18 +1022,6 @@
             '*head'  : 'int',
             'events' : [ 'InputEvent' ] } }
 
-##
-# @GrabToggleKeys:
-#
-# Keys to toggle input-linux between host and guest.
-#
-# Since: 4.0
-#
-##
-{ 'enum': 'GrabToggleKeys',
-  'data': [ 'ctrl-ctrl', 'alt-alt', 'shift-shift','meta-meta', 'scrolllock',
-            'ctrl-scrolllock' ] }
-
 ##
 # @DisplayGTK:
 #
-- 
2.28.0

[PATCH 16/18] tests: Drop 'props' from object-add calls

[Kevin Wolf]

The 'props' option has been deprecated in 5.0 in favour of a flattened
object-add command. Time to change our test cases to drop the deprecated
option.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qtest/qmp-cmd-test.c   | 16 +++++------
 tests/qtest/test-netfilter.c | 54 ++++++++++++++++--------------------
 tests/qemu-iotests/087       |  8 ++----
 tests/qemu-iotests/184       | 18 ++++--------
 tests/qemu-iotests/218       |  2 +-
 tests/qemu-iotests/235       |  2 +-
 tests/qemu-iotests/245       |  4 +--
 tests/qemu-iotests/258       |  6 ++--
 tests/qemu-iotests/258.out   |  4 +--
 tests/qemu-iotests/295       |  2 +-
 tests/qemu-iotests/296       |  2 +-
 11 files changed, 51 insertions(+), 67 deletions(-)

diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index 1c7186e53c..c98b78d033 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -230,14 +230,14 @@ static void test_object_add_failure_modes(void)
     /* attempt to create 2 objects with duplicate id */
     resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
                      " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
-                     " 'props': {'size': 1048576 } } }");
+                     " 'size': 1048576 } }");
     g_assert_nonnull(resp);
     g_assert(qdict_haskey(resp, "return"));
     qobject_unref(resp);
 
     resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
                      " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
-                     " 'props': {'size': 1048576 } } }");
+                     " 'size': 1048576 } }");
     g_assert_nonnull(resp);
     qmp_expect_error_and_unref(resp, "GenericError");
 
@@ -251,14 +251,14 @@ static void test_object_add_failure_modes(void)
     /* attempt to create an object with a property of a wrong type */
     resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
                      " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
-                     " 'props': {'size': '1048576' } } }");
+                     " 'size': '1048576' } }");
     g_assert_nonnull(resp);
     /* now do it right */
     qmp_expect_error_and_unref(resp, "GenericError");
 
     resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
                      " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
-                     " 'props': {'size': 1048576 } } }");
+                     " 'size': 1048576 } }");
     g_assert_nonnull(resp);
     g_assert(qdict_haskey(resp, "return"));
     qobject_unref(resp);
@@ -273,14 +273,14 @@ static void test_object_add_failure_modes(void)
     /* attempt to create an object without the id */
     resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
                      " {'qom-type': 'memory-backend-ram',"
-                     " 'props': {'size': 1048576 } } }");
+                     " 'size': 1048576 } }");
     g_assert_nonnull(resp);
     qmp_expect_error_and_unref(resp, "GenericError");
 
     /* now do it right */
     resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
                      " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
-                     " 'props': {'size': 1048576 } } }");
+                     " 'size': 1048576 } }");
     g_assert_nonnull(resp);
     g_assert(qdict_haskey(resp, "return"));
     qobject_unref(resp);
@@ -295,14 +295,14 @@ static void test_object_add_failure_modes(void)
     /* attempt to set a non existing property */
     resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
                      " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
-                     " 'props': {'sized': 1048576 } } }");
+                     " 'sized': 1048576 } }");
     g_assert_nonnull(resp);
     qmp_expect_error_and_unref(resp, "GenericError");
 
     /* now do it right */
     resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
                      " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
-                     " 'props': {'size': 1048576 } } }");
+                     " 'size': 1048576 } }");
     g_assert_nonnull(resp);
     g_assert(qdict_haskey(resp, "return"));
     qobject_unref(resp);
diff --git a/tests/qtest/test-netfilter.c b/tests/qtest/test-netfilter.c
index 22927ee6ab..785b6f3226 100644
--- a/tests/qtest/test-netfilter.c
+++ b/tests/qtest/test-netfilter.c
@@ -21,11 +21,10 @@ static void add_one_netfilter(void)
                    " 'arguments': {"
                    "   'qom-type': 'filter-buffer',"
                    "   'id': 'qtest-f0',"
-                   "   'props': {"
-                   "     'netdev': 'qtest-bn0',"
-                   "     'queue': 'rx',"
-                   "     'interval': 1000"
-                   "}}}");
+                   "   'netdev': 'qtest-bn0',"
+                   "   'queue': 'rx',"
+                   "   'interval': 1000"
+                   "}}");
 
     g_assert(response);
     g_assert(!qdict_haskey(response, "error"));
@@ -49,11 +48,10 @@ static void remove_netdev_with_one_netfilter(void)
                    " 'arguments': {"
                    "   'qom-type': 'filter-buffer',"
                    "   'id': 'qtest-f0',"
-                   "   'props': {"
-                   "     'netdev': 'qtest-bn0',"
-                   "     'queue': 'rx',"
-                   "     'interval': 1000"
-                   "}}}");
+                   "   'netdev': 'qtest-bn0',"
+                   "   'queue': 'rx',"
+                   "   'interval': 1000"
+                   "}}");
 
     g_assert(response);
     g_assert(!qdict_haskey(response, "error"));
@@ -87,11 +85,10 @@ static void add_multi_netfilter(void)
                    " 'arguments': {"
                    "   'qom-type': 'filter-buffer',"
                    "   'id': 'qtest-f0',"
-                   "   'props': {"
-                   "     'netdev': 'qtest-bn0',"
-                   "     'queue': 'rx',"
-                   "     'interval': 1000"
-                   "}}}");
+                   "   'netdev': 'qtest-bn0',"
+                   "   'queue': 'rx',"
+                   "   'interval': 1000"
+                   "}}");
 
     g_assert(response);
     g_assert(!qdict_haskey(response, "error"));
@@ -101,11 +98,10 @@ static void add_multi_netfilter(void)
                    " 'arguments': {"
                    "   'qom-type': 'filter-buffer',"
                    "   'id': 'qtest-f1',"
-                   "   'props': {"
-                   "     'netdev': 'qtest-bn0',"
-                   "     'queue': 'rx',"
-                   "     'interval': 1000"
-                   "}}}");
+                   "   'netdev': 'qtest-bn0',"
+                   "   'queue': 'rx',"
+                   "   'interval': 1000"
+                   "}}");
 
     g_assert(response);
     g_assert(!qdict_haskey(response, "error"));
@@ -137,11 +133,10 @@ static void remove_netdev_with_multi_netfilter(void)
                    " 'arguments': {"
                    "   'qom-type': 'filter-buffer',"
                    "   'id': 'qtest-f0',"
-                   "   'props': {"
-                   "     'netdev': 'qtest-bn0',"
-                   "     'queue': 'rx',"
-                   "     'interval': 1000"
-                   "}}}");
+                   "   'netdev': 'qtest-bn0',"
+                   "   'queue': 'rx',"
+                   "   'interval': 1000"
+                   "}}");
 
     g_assert(response);
     g_assert(!qdict_haskey(response, "error"));
@@ -151,11 +146,10 @@ static void remove_netdev_with_multi_netfilter(void)
                    " 'arguments': {"
                    "   'qom-type': 'filter-buffer',"
                    "   'id': 'qtest-f1',"
-                   "   'props': {"
-                   "     'netdev': 'qtest-bn0',"
-                   "     'queue': 'rx',"
-                   "     'interval': 1000"
-                   "}}}");
+                   "   'netdev': 'qtest-bn0',"
+                   "   'queue': 'rx',"
+                   "   'interval': 1000"
+                   "}}");
 
     g_assert(response);
     g_assert(!qdict_haskey(response, "error"));
diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index 678e748c58..65a6244ff1 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -142,9 +142,7 @@ run_qemu <<EOF
   "arguments": {
       "qom-type": "secret",
       "id": "sec0",
-      "props": {
-          "data": "123456"
-      }
+      "data": "123456"
   }
 }
 { "execute": "blockdev-add",
@@ -175,9 +173,7 @@ run_qemu <<EOF
   "arguments": {
       "qom-type": "secret",
       "id": "sec0",
-      "props": {
-          "data": "123456"
-      }
+      "data": "123456"
   }
 }
 { "execute": "blockdev-add",
diff --git a/tests/qemu-iotests/184 b/tests/qemu-iotests/184
index eebb53faed..5c5d2ba6e2 100755
--- a/tests/qemu-iotests/184
+++ b/tests/qemu-iotests/184
@@ -66,10 +66,8 @@ run_qemu <<EOF
   "arguments": {
     "qom-type": "throttle-group",
     "id": "group0",
-    "props": {
-      "limits" : {
-        "iops-total": 1000
-      }
+    "limits" : {
+      "iops-total": 1000
     }
   }
 }
@@ -95,10 +93,8 @@ run_qemu <<EOF
   "arguments": {
     "qom-type": "throttle-group",
     "id": "group0",
-    "props" : {
-      "limits": {
-          "iops-total": 1000
-      }
+    "limits": {
+        "iops-total": 1000
     }
   }
 }
@@ -135,10 +131,8 @@ run_qemu <<EOF
   "arguments": {
     "qom-type": "throttle-group",
     "id": "group0",
-    "props" : {
-      "limits": {
-          "iops-total": 1000
-      }
+    "limits": {
+        "iops-total": 1000
     }
   }
 }
diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
index 5586870456..c524d4b062 100755
--- a/tests/qemu-iotests/218
+++ b/tests/qemu-iotests/218
@@ -151,7 +151,7 @@ with iotests.VM() as vm, \
     vm.launch()
 
     ret = vm.qmp('object-add', qom_type='throttle-group', id='tg',
-                 props={'x-bps-read': 4096})
+                 limits={'bps-read': 4096})
     assert ret['return'] == {}
 
     ret = vm.qmp('blockdev-add',
diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
index d1b10ac36b..2765561ada 100755
--- a/tests/qemu-iotests/235
+++ b/tests/qemu-iotests/235
@@ -56,7 +56,7 @@ vm.add_args('-drive', 'id=src,file=' + disk)
 vm.launch()
 
 log(vm.qmp('object-add', qom_type='throttle-group', id='tg0',
-           props={ 'x-bps-total': size }))
+           limits={ 'bps-total': size }))
 
 log(vm.qmp('blockdev-add',
            **{ 'node-name': 'target',
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index e60c8326d3..ce82a8e117 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -643,12 +643,12 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         ###### throttle ######
         ######################
         opts = { 'qom-type': 'throttle-group', 'id': 'group0',
-                 'props': { 'limits': { 'iops-total': 1000 } } }
+                 'limits': { 'iops-total': 1000 } }
         result = self.vm.qmp('object-add', conv_keys = False, **opts)
         self.assert_qmp(result, 'return', {})
 
         opts = { 'qom-type': 'throttle-group', 'id': 'group1',
-                 'props': { 'limits': { 'iops-total': 2000 } } }
+                 'limits': { 'iops-total': 2000 } }
         result = self.vm.qmp('object-add', conv_keys = False, **opts)
         self.assert_qmp(result, 'return', {})
 
diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258
index e305a1502f..5be428d232 100755
--- a/tests/qemu-iotests/258
+++ b/tests/qemu-iotests/258
@@ -102,9 +102,9 @@ def test_concurrent_finish(write_to_stream_node):
         vm.qmp_log('object-add',
                    qom_type='throttle-group',
                    id='tg',
-                   props={
-                       'x-iops-write': 1,
-                       'x-iops-write-max': 1
+                   limits={
+                       'iops-write': 1,
+                       'iops-write-max': 1
                    })
 
         vm.qmp_log('blockdev-add',
diff --git a/tests/qemu-iotests/258.out b/tests/qemu-iotests/258.out
index ce6e9ba3e5..c3a003d3e3 100644
--- a/tests/qemu-iotests/258.out
+++ b/tests/qemu-iotests/258.out
@@ -2,7 +2,7 @@ Running tests:
 
 === Commit and stream finish concurrently (letting stream write) ===
 
-{"execute": "object-add", "arguments": {"id": "tg", "props": {"x-iops-write": 1, "x-iops-write-max": 1}, "qom-type": "throttle-group"}}
+{"execute": "object-add", "arguments": {"id": "tg", "limits": {"iops-write": 1, "iops-write-max": 1}, "qom-type": "throttle-group"}}
 {"return": {}}
 {"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"backing": {"driver": "raw", "file": {"driver": "file", "filename": "TEST_DIR/PID-node0.img"}, "node-name": "node0"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node1.img"}, "node-name": "node1"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node2.img"}, "node-name": "node2"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node3.img"}, "node-name": "node3"}, "driver": "IMGFMT", "file": {"driver": "throttle", "file": {"driver": "file", "filename": "TEST_DIR/PID-node4.img"}, "throttle-group": "tg"}, "node-name": "node4"}}
 {"return": {}}
@@ -18,7 +18,7 @@ Running tests:
 
 === Commit and stream finish concurrently (letting commit write) ===
 
-{"execute": "object-add", "arguments": {"id": "tg", "props": {"x-iops-write": 1, "x-iops-write-max": 1}, "qom-type": "throttle-group"}}
+{"execute": "object-add", "arguments": {"id": "tg", "limits": {"iops-write": 1, "iops-write-max": 1}, "qom-type": "throttle-group"}}
 {"return": {}}
 {"execute": "blockdev-add", "arguments": {"backing": {"backing": {"backing": {"backing": {"driver": "raw", "file": {"driver": "throttle", "file": {"driver": "file", "filename": "TEST_DIR/PID-node0.img"}, "throttle-group": "tg"}, "node-name": "node0"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node1.img"}, "node-name": "node1"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node2.img"}, "node-name": "node2"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node3.img"}, "node-name": "node3"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-node4.img"}, "node-name": "node4"}}
 {"return": {}}
diff --git a/tests/qemu-iotests/295 b/tests/qemu-iotests/295
index 59e674fa85..f0c0d2a50f 100755
--- a/tests/qemu-iotests/295
+++ b/tests/qemu-iotests/295
@@ -42,7 +42,7 @@ class Secret:
 
     def to_qmp_object(self):
         return { "qom_type" : "secret", "id": self.id(),
-                 "props": { "data": self.secret() } }
+                 "data": self.secret() }
 
 ################################################################################
 class EncryptionSetupTestCase(iotests.QMPTestCase):
diff --git a/tests/qemu-iotests/296 b/tests/qemu-iotests/296
index fb7dec88aa..74d0bb44d4 100755
--- a/tests/qemu-iotests/296
+++ b/tests/qemu-iotests/296
@@ -42,7 +42,7 @@ class Secret:
 
     def to_qmp_object(self):
         return { "qom_type" : "secret", "id": self.id(),
-                 "props": { "data": self.secret() } }
+                 "data": self.secret() }
 
 ################################################################################
 
-- 
2.28.0

[PATCH 17/18] qapi/qom: Drop deprecated 'props' from object-add

[Kevin Wolf]

The option has been deprecated in QEMU 5.0, remove it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qom.json              |  6 +-----
 docs/system/deprecated.rst | 10 +++++-----
 qom/qom-qmp-cmds.c         | 21 ---------------------
 3 files changed, 6 insertions(+), 31 deletions(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index 29b229394e..7e0d26a728 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -813,10 +813,6 @@
 #
 # @id: the name of the new object
 #
-# @props: a dictionary of properties to be passed to the backend. Deprecated
-#         since 5.0, specify the properties on the top level instead. It is an
-#         error to specify the same option both on the top level and in @props.
-#
 # Additional arguments depend on qom-type and are passed to the backend
 # unchanged.
 #
@@ -834,7 +830,7 @@
 #
 ##
 { 'command': 'object-add',
-  'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'},
+  'data': {'qom-type': 'str', 'id': 'str'},
   'gen': false } # so we can get the additional arguments
 
 ##
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 04e41254f9..e6043799a4 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -224,11 +224,6 @@ Use ``migrate-set-parameters`` and ``query-migrate-parameters`` instead.
 
 Use arguments ``base-node`` and ``top-node`` instead.
 
-``object-add`` option ``props`` (since 5.0)
-'''''''''''''''''''''''''''''''''''''''''''
-
-Specify the properties for the object as top-level arguments instead.
-
 ``query-named-block-nodes`` and ``query-block`` result dirty-bitmaps[i].status (since 4.0)
 ''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
 
@@ -558,6 +553,11 @@ are automatically loaded from qcow2 images.
 Use ``device_add`` for hotplugging vCPUs instead of ``cpu-add``.  See
 documentation of ``query-hotpluggable-cpus`` for additional details.
 
+``object-add`` option ``props`` (removed in 6.0)
+''''''''''''''''''''''''''''''''''''''''''''''''
+
+Specify the properties for the object as top-level arguments instead.
+
 Human Monitor Protocol (HMP) commands
 -------------------------------------
 
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 310ab2d048..0e7d7247fc 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -243,27 +243,6 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename,
 
 void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
 {
-    QObject *props;
-    QDict *pdict;
-
-    props = qdict_get(qdict, "props");
-    if (props) {
-        pdict = qobject_to(QDict, props);
-        if (!pdict) {
-            error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
-            return;
-        }
-        qobject_ref(pdict);
-        qdict_del(qdict, "props");
-        qdict_join(qdict, pdict, false);
-        if (qdict_size(pdict) != 0) {
-            error_setg(errp, "Option in 'props' conflicts with top level");
-            qobject_unref(pdict);
-            return;
-        }
-        qobject_unref(pdict);
-    }
-
     user_creatable_add_dict(qdict, false, errp);
 }
 
-- 
2.28.0

[PATCH 18/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

This converts object-add from 'gen': false to the ObjectOptions QAPI
type. As an immediate benefit, clients can now use QAPI schema
introspection for user creatable QOM objects.

It is also the first step towards making the QAPI schema the only
external interface for the creation of user creatable objects. Once all
other places (HMP and command lines of the system emulator and all
tools) go through QAPI, too, some object implementations can be
simplified because some checks (e.g. that mandatory options are set) are
already performed by QAPI, and in another step, QOM boilerplate code
could be generated from the schema.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qom.json                   | 11 +----------
 include/qom/object_interfaces.h |  7 -------
 hw/block/xen-block.c            | 16 ++++++++--------
 monitor/misc.c                  |  2 --
 qom/qom-qmp-cmds.c              | 25 +++++++++++++++++++++++--
 5 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index 7e0d26a728..f6c6ba6748 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -809,13 +809,6 @@
 #
 # Create a QOM object.
 #
-# @qom-type: the class name for the object to be created
-#
-# @id: the name of the new object
-#
-# Additional arguments depend on qom-type and are passed to the backend
-# unchanged.
-#
 # Returns: Nothing on success
 #          Error if @qom-type is not a valid class name
 #
@@ -829,9 +822,7 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'object-add',
-  'data': {'qom-type': 'str', 'id': 'str'},
-  'gen': false } # so we can get the additional arguments
+{ 'command': 'object-add', 'data': 'ObjectOptions', 'boxed': true }
 
 ##
 # @object-del:
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 07d5cc8832..9b9938b8c0 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -196,11 +196,4 @@ bool user_creatable_del(const char *id, Error **errp);
  */
 void user_creatable_cleanup(void);
 
-/**
- * qmp_object_add:
- *
- * QMP command handler for object-add. See the QAPI schema for documentation.
- */
-void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp);
-
 #endif
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 8a7a3f5452..d8960ff55b 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -842,17 +842,17 @@ static XenBlockIOThread *xen_block_iothread_create(const char *id,
 {
     ERRP_GUARD();
     XenBlockIOThread *iothread = g_new(XenBlockIOThread, 1);
-    QDict *opts;
-    QObject *ret_data = NULL;
+    ObjectOptions *opts;
 
     iothread->id = g_strdup(id);
 
-    opts = qdict_new();
-    qdict_put_str(opts, "qom-type", TYPE_IOTHREAD);
-    qdict_put_str(opts, "id", id);
-    qmp_object_add(opts, &ret_data, errp);
-    qobject_unref(opts);
-    qobject_unref(ret_data);
+    opts = g_new(ObjectOptions, 1);
+    *opts = (ObjectOptions) {
+        .qom_type = OBJECT_TYPE_IOTHREAD,
+        .id = g_strdup(id),
+    };
+    qmp_object_add(opts, errp);
+    qapi_free_ObjectOptions(opts);
 
     if (*errp) {
         g_free(iothread->id);
diff --git a/monitor/misc.c b/monitor/misc.c
index 398211a034..0586ee1f4f 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -240,8 +240,6 @@ static void monitor_init_qmp_commands(void)
                          qmp_query_qmp_schema, QCO_ALLOW_PRECONFIG);
     qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
                          QCO_NO_OPTIONS);
-    qmp_register_command(&qmp_commands, "object-add", qmp_object_add,
-                         QCO_NO_OPTIONS);
 
     QTAILQ_INIT(&qmp_cap_negotiation_commands);
     qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 0e7d7247fc..5c73f04913 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -19,8 +19,11 @@
 #include "qapi/error.h"
 #include "qapi/qapi-commands-qdev.h"
 #include "qapi/qapi-commands-qom.h"
+#include "qapi/qapi-visit-qom.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
 #include "qemu/cutils.h"
 #include "qom/object_interfaces.h"
 #include "qom/qom-qobject.h"
@@ -241,9 +244,27 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename,
     return prop_list;
 }
 
-void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
+void qmp_object_add(ObjectOptions *options, Error **errp)
 {
-    user_creatable_add_dict(qdict, false, errp);
+    Visitor *v;
+    QObject *qobj;
+    QDict *props;
+    Object *obj;
+
+    v = qobject_output_visitor_new(&qobj);
+    visit_type_ObjectOptions(v, NULL, &options, &error_abort);
+    visit_complete(v, &qobj);
+    visit_free(v);
+
+    props = qobject_to(QDict, qobj);
+    qdict_del(props, "qom-type");
+    qdict_del(props, "id");
+
+    v = qobject_input_visitor_new(QOBJECT(props));
+    obj = user_creatable_add_type(ObjectType_str(options->qom_type),
+                                  options->id, props, v, errp);
+    object_unref(obj);
+    visit_free(v);
 }
 
 void qmp_object_del(const char *id, Error **errp)
-- 
2.28.0

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 30/11/20 13:25, Kevin Wolf wrote:
> This series adds a QAPI type for the properties of all user creatable
> QOM types and finally makes QMP object-add use the new ObjectOptions
> union so that QAPI introspection can be used for user creatable objects.
> 
> After this series, there is least one obvious next step that needs to be
> done: Change HMP and all of the command line parser to use
> ObjectOptions, too, so that the QAPI schema is consistently enforced in
> all external interfaces. I am planning to send another series to address
> this.
> 
> In a third step, we can try to start deduplicating and integrating things
> better between QAPI and the QOM implementation, e.g. by generating parts
> of the QOM boilerplate from the QAPI schema.

With this series it's basically pointless to have QOM properties at all. 
  Instead, you are basically having half of QEMU's backend data model 
into a single struct.

So the question is, are we okay with shoveling half of QEMU's backend 
data model into a single struct?  If so, there are important consequences.

1) QOM basically does not need properties anymore except for devices and 
machines (accelerators could be converted to QAPI as well). All 
user-creatable objects can be changed to something like chardev's "get a 
struct and use it fill in the fields", and only leave properties to 
devices and machines.

2) User-creatable objects can have a much more flexible schema.  This 
means there's no reason to have block device creation as its own command 
and struct for example.

The problem with this series is that you are fine with deduplicating 
things as a third step, but you cannot be sure that such deduplication 
is possible at all.  So while I don't have any problems in principle 
with the ObjectOptions concept, I don't think it should be committed 
without a clear idea of how to do the third step.

In the meanwhile, of course I have no problem with deprecating the 
opened and loaded properties.

Paolo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Daniel P. Berrangé]

On Mon, Nov 30, 2020 at 03:58:23PM +0100, Paolo Bonzini wrote:
> On 30/11/20 13:25, Kevin Wolf wrote:
> > This series adds a QAPI type for the properties of all user creatable
> > QOM types and finally makes QMP object-add use the new ObjectOptions
> > union so that QAPI introspection can be used for user creatable objects.
> > 
> > After this series, there is least one obvious next step that needs to be
> > done: Change HMP and all of the command line parser to use
> > ObjectOptions, too, so that the QAPI schema is consistently enforced in
> > all external interfaces. I am planning to send another series to address
> > this.
> > 
> > In a third step, we can try to start deduplicating and integrating things
> > better between QAPI and the QOM implementation, e.g. by generating parts
> > of the QOM boilerplate from the QAPI schema.
> 
> With this series it's basically pointless to have QOM properties at all.
> Instead, you are basically having half of QEMU's backend data model into a
> single struct.
> 
> So the question is, are we okay with shoveling half of QEMU's backend data
> model into a single struct?  If so, there are important consequences.


In theory they should have the same set of options, but nothing in
this series will enforce that. So we're introducing the danger that
QMP object-add will miss some property, and thus be less functional
than the CLI -object.  If we convert CLI -object  to use the QAPI
parser too, we eliminate that danger, but we still have the struct
duplication.

> 1) QOM basically does not need properties anymore except for devices and
> machines (accelerators could be converted to QAPI as well). All
> user-creatable objects can be changed to something like chardev's "get a
> struct and use it fill in the fields", and only leave properties to devices
> and machines.
> 
> 2) User-creatable objects can have a much more flexible schema.  This means
> there's no reason to have block device creation as its own command and
> struct for example.
> 
> The problem with this series is that you are fine with deduplicating things
> as a third step, but you cannot be sure that such deduplication is possible
> at all.  So while I don't have any problems in principle with the
> ObjectOptions concept, I don't think it should be committed without a clear
> idea of how to do the third step.

I feel like we should at least aim to kill the struct duplication, even if
we ignore the bigger QOM stuff like setters/getters/constructors/etc. The
generated structs are not far off being usable.

eg for the secret object we have the QAPI schema

{ 'struct': 'SecretCommonProperties',
  'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
            '*format': 'QCryptoSecretFormat',
            '*keyid': 'str',
            '*iv': 'str' } }

{ 'struct': 'SecretProperties',
  'base': 'SecretCommonProperties',
  'data': { '*data': 'str',
            '*file': 'str' } }

IIUC this will resulting in a QAPI generated flattened struct:

  struct SecretProperties {
    bool loaded;
    QCryptoSecretFormat format;
    char *keyid;
    char *iv;
    char *data;
    char *file;
  };

vs the QOM manually written structs

  struct QCryptoSecretCommon {
    Object parent_obj;
    uint8_t *rawdata;
    size_t rawlen;
    QCryptoSecretFormat format;
    char *keyid;
    char *iv;
  };

  struct QCryptoSecret {
    QCryptoSecretCommon parent_obj;
    char *data;
    char *file;
  };

The key differences

 - The parent struct is embedded, rather than flattened
 - The "loaded" property doesn't need to exist
 - Some extra fields are live state (rawdata, rawlen)

Lets pretend we just kill "loaded" entirely, so ignore that.

We could simply make QOM "Object" a well known built-in type, so
we can reference it as a "parent". Then any struct with "Object"
as a parent could use struct embedding rather flattening and thus
just work.

Can we invent a "state" field for fields that are internal
only, separate from the public "data" fields.

eg the secret QAPI def would only need a couple of changes:

{ 'struct': 'QCryptoSecretCommon',
  'base': 'Object',
  'state': { 'rawdata': '*uint8_t',
             'rawlen': 'size_t' },
  'data': { '*format': 'QCryptoSecretFormat',
            '*keyid': 'str',
            '*iv': 'str' } }

{ 'struct': 'QCryptoSecret',
  'base': 'QCryptoSecretCommon',
  'data': { '*data': 'str',
            '*file': 'str' } }

There would need to be a

   void QCryptoSecretCommonFreeState(QCryptoSecretCommon *obj)

method defined manually by the programmer to take care of free'ing any
pointers in the "state".

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 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 30.11.2020 um 16:30 hat Daniel P. Berrangé geschrieben:
> On Mon, Nov 30, 2020 at 03:58:23PM +0100, Paolo Bonzini wrote:
> > On 30/11/20 13:25, Kevin Wolf wrote:
> > > This series adds a QAPI type for the properties of all user creatable
> > > QOM types and finally makes QMP object-add use the new ObjectOptions
> > > union so that QAPI introspection can be used for user creatable objects.
> > > 
> > > After this series, there is least one obvious next step that needs to be
> > > done: Change HMP and all of the command line parser to use
> > > ObjectOptions, too, so that the QAPI schema is consistently enforced in
> > > all external interfaces. I am planning to send another series to address
> > > this.
> > > 
> > > In a third step, we can try to start deduplicating and integrating things
> > > better between QAPI and the QOM implementation, e.g. by generating parts
> > > of the QOM boilerplate from the QAPI schema.
> > 
> > With this series it's basically pointless to have QOM properties at all.
> > Instead, you are basically having half of QEMU's backend data model into a
> > single struct.
> > 
> > So the question is, are we okay with shoveling half of QEMU's backend data
> > model into a single struct?  If so, there are important consequences.
> 
> In theory they should have the same set of options, but nothing in
> this series will enforce that. So we're introducing the danger that
> QMP object-add will miss some property, and thus be less functional
> than the CLI -object.  If we convert CLI -object  to use the QAPI
> parser too, we eliminate that danger, but we still have the struct
> duplication.

I think converting the CLI is doable in the short term. I already have
the patch for qemu-storage-daemon, but decided to keep it for a separate
series.

The most difficult part is probably -readconfig, but with Paolo's RFC
patches to move it away from QemuOpts, even that shouldn't be very hard.

> > 1) QOM basically does not need properties anymore except for devices and
> > machines (accelerators could be converted to QAPI as well). All
> > user-creatable objects can be changed to something like chardev's "get a
> > struct and use it fill in the fields", and only leave properties to devices
> > and machines.
> > 
> > 2) User-creatable objects can have a much more flexible schema.  This means
> > there's no reason to have block device creation as its own command and
> > struct for example.
> > 
> > The problem with this series is that you are fine with deduplicating things
> > as a third step, but you cannot be sure that such deduplication is possible
> > at all.  So while I don't have any problems in principle with the
> > ObjectOptions concept, I don't think it should be committed without a clear
> > idea of how to do the third step.
> 
> I feel like we should at least aim to kill the struct duplication, even if
> we ignore the bigger QOM stuff like setters/getters/constructors/etc. The
> generated structs are not far off being usable.
> 
> eg for the secret object we have the QAPI schema
> 
> { 'struct': 'SecretCommonProperties',
>   'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
>             '*format': 'QCryptoSecretFormat',
>             '*keyid': 'str',
>             '*iv': 'str' } }
> 
> { 'struct': 'SecretProperties',
>   'base': 'SecretCommonProperties',
>   'data': { '*data': 'str',
>             '*file': 'str' } }
> 
> IIUC this will resulting in a QAPI generated flattened struct:
> 
>   struct SecretProperties {
>     bool loaded;
>     QCryptoSecretFormat format;
>     char *keyid;
>     char *iv;
>     char *data;
>     char *file;
>   };
> 
> vs the QOM manually written structs
> 
>   struct QCryptoSecretCommon {
>     Object parent_obj;
>     uint8_t *rawdata;
>     size_t rawlen;
>     QCryptoSecretFormat format;
>     char *keyid;
>     char *iv;
>   };
> 
>   struct QCryptoSecret {
>     QCryptoSecretCommon parent_obj;
>     char *data;
>     char *file;
>   };
> 
> The key differences
> 
>  - The parent struct is embedded, rather than flattened
>  - The "loaded" property doesn't need to exist
>  - Some extra fields are live state (rawdata, rawlen)
> 
> Lets pretend we just kill "loaded" entirely, so ignore that.
> 
> We could simply make QOM "Object" a well known built-in type, so
> we can reference it as a "parent". Then any struct with "Object"
> as a parent could use struct embedding rather flattening and thus
> just work.
> 
> Can we invent a "state" field for fields that are internal
> only, separate from the public "data" fields.
> 
> eg the secret QAPI def would only need a couple of changes:
> 
> { 'struct': 'QCryptoSecretCommon',
>   'base': 'Object',
>   'state': { 'rawdata': '*uint8_t',
>              'rawlen': 'size_t' },
>   'data': { '*format': 'QCryptoSecretFormat',
>             '*keyid': 'str',
>             '*iv': 'str' } }
> 
> { 'struct': 'QCryptoSecret',
>   'base': 'QCryptoSecretCommon',
>   'data': { '*data': 'str',
>             '*file': 'str' } }

I haven't given much though to the details yet, but I was thinking of
introducing a new QAPI entity type for objects. We could include
additional fields there, where the type would just directly be a C type
rather than being interpreted by QAPI.

Maybe like this:

{ 'object': 'secret-common',
  'abstract': true,
  'properties': 'SecretCommonProperties',
  'state': { 'rawdata': '*uint8_t',
             'rawlen': 'size_t' } }

{ 'object': 'secret',
  'parent': 'secret-common',
  'properties': 'SecretProperties' } }

Maybe it would actually be nicer to have 'state' just as a string
property that contains the C type name of the state struct and then QAPI
just adds a pointer to it.

Either way, there is some duplication there because we have a
parent-child relationship both between the object types themselves and
between their property classes. Either we remove the base from
SecretProperties (which would make object-add and the CLI more
complicated) or we just let the QAPI generator check that they are
consistent.

secret doesn't need writable properties, but for those objects that do
need it, I would include a list that defines some properties as having
setters and then the QAPI generated property code would call a manually
written callback.

> There would need to be a
> 
>    void QCryptoSecretCommonFreeState(QCryptoSecretCommon *obj)
> 
> method defined manually by the programmer to take care of free'ing any
> pointers in the "state".

Isn't this the job of the normal .instance_finalize method?

Kevin

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Daniel P. Berrangé]

On Mon, Nov 30, 2020 at 05:13:57PM +0100, Kevin Wolf wrote:
> Am 30.11.2020 um 16:30 hat Daniel P. Berrangé geschrieben:
> > On Mon, Nov 30, 2020 at 03:58:23PM +0100, Paolo Bonzini wrote:
> > > On 30/11/20 13:25, Kevin Wolf wrote:
> > > > This series adds a QAPI type for the properties of all user creatable
> > > > QOM types and finally makes QMP object-add use the new ObjectOptions
> > > > union so that QAPI introspection can be used for user creatable objects.
> > > > 
> > > > After this series, there is least one obvious next step that needs to be
> > > > done: Change HMP and all of the command line parser to use
> > > > ObjectOptions, too, so that the QAPI schema is consistently enforced in
> > > > all external interfaces. I am planning to send another series to address
> > > > this.
> > > > 
> > > > In a third step, we can try to start deduplicating and integrating things
> > > > better between QAPI and the QOM implementation, e.g. by generating parts
> > > > of the QOM boilerplate from the QAPI schema.
> > > 
> > > With this series it's basically pointless to have QOM properties at all.
> > > Instead, you are basically having half of QEMU's backend data model into a
> > > single struct.
> > > 
> > > So the question is, are we okay with shoveling half of QEMU's backend data
> > > model into a single struct?  If so, there are important consequences.
> > 
> > In theory they should have the same set of options, but nothing in
> > this series will enforce that. So we're introducing the danger that
> > QMP object-add will miss some property, and thus be less functional
> > than the CLI -object.  If we convert CLI -object  to use the QAPI
> > parser too, we eliminate that danger, but we still have the struct
> > duplication.
> 
> I think converting the CLI is doable in the short term. I already have
> the patch for qemu-storage-daemon, but decided to keep it for a separate
> series.
> 
> The most difficult part is probably -readconfig, but with Paolo's RFC
> patches to move it away from QemuOpts, even that shouldn't be very hard.
> 
> > > 1) QOM basically does not need properties anymore except for devices and
> > > machines (accelerators could be converted to QAPI as well). All
> > > user-creatable objects can be changed to something like chardev's "get a
> > > struct and use it fill in the fields", and only leave properties to devices
> > > and machines.
> > > 
> > > 2) User-creatable objects can have a much more flexible schema.  This means
> > > there's no reason to have block device creation as its own command and
> > > struct for example.
> > > 
> > > The problem with this series is that you are fine with deduplicating things
> > > as a third step, but you cannot be sure that such deduplication is possible
> > > at all.  So while I don't have any problems in principle with the
> > > ObjectOptions concept, I don't think it should be committed without a clear
> > > idea of how to do the third step.
> > 
> > I feel like we should at least aim to kill the struct duplication, even if
> > we ignore the bigger QOM stuff like setters/getters/constructors/etc. The
> > generated structs are not far off being usable.
> > 
> > eg for the secret object we have the QAPI schema
> > 
> > { 'struct': 'SecretCommonProperties',
> >   'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
> >             '*format': 'QCryptoSecretFormat',
> >             '*keyid': 'str',
> >             '*iv': 'str' } }
> > 
> > { 'struct': 'SecretProperties',
> >   'base': 'SecretCommonProperties',
> >   'data': { '*data': 'str',
> >             '*file': 'str' } }
> > 
> > IIUC this will resulting in a QAPI generated flattened struct:
> > 
> >   struct SecretProperties {
> >     bool loaded;
> >     QCryptoSecretFormat format;
> >     char *keyid;
> >     char *iv;
> >     char *data;
> >     char *file;
> >   };
> > 
> > vs the QOM manually written structs
> > 
> >   struct QCryptoSecretCommon {
> >     Object parent_obj;
> >     uint8_t *rawdata;
> >     size_t rawlen;
> >     QCryptoSecretFormat format;
> >     char *keyid;
> >     char *iv;
> >   };
> > 
> >   struct QCryptoSecret {
> >     QCryptoSecretCommon parent_obj;
> >     char *data;
> >     char *file;
> >   };
> > 
> > The key differences
> > 
> >  - The parent struct is embedded, rather than flattened
> >  - The "loaded" property doesn't need to exist
> >  - Some extra fields are live state (rawdata, rawlen)
> > 
> > Lets pretend we just kill "loaded" entirely, so ignore that.
> > 
> > We could simply make QOM "Object" a well known built-in type, so
> > we can reference it as a "parent". Then any struct with "Object"
> > as a parent could use struct embedding rather flattening and thus
> > just work.
> > 
> > Can we invent a "state" field for fields that are internal
> > only, separate from the public "data" fields.
> > 
> > eg the secret QAPI def would only need a couple of changes:
> > 
> > { 'struct': 'QCryptoSecretCommon',
> >   'base': 'Object',
> >   'state': { 'rawdata': '*uint8_t',
> >              'rawlen': 'size_t' },
> >   'data': { '*format': 'QCryptoSecretFormat',
> >             '*keyid': 'str',
> >             '*iv': 'str' } }
> > 
> > { 'struct': 'QCryptoSecret',
> >   'base': 'QCryptoSecretCommon',
> >   'data': { '*data': 'str',
> >             '*file': 'str' } }
> 
> I haven't given much though to the details yet, but I was thinking of
> introducing a new QAPI entity type for objects. We could include
> additional fields there, where the type would just directly be a C type
> rather than being interpreted by QAPI.
> 
> Maybe like this:
> 
> { 'object': 'secret-common',
>   'abstract': true,
>   'properties': 'SecretCommonProperties',
>   'state': { 'rawdata': '*uint8_t',
>              'rawlen': 'size_t' } }
> 
> { 'object': 'secret',
>   'parent': 'secret-common',
>   'properties': 'SecretProperties' } }
> 
> Maybe it would actually be nicer to have 'state' just as a string
> property that contains the C type name of the state struct and then QAPI
> just adds a pointer to it.

Yep, it would be nice to have clear separation of the "state" from
the "config", as that also makes it more obvious what is derived
info. 

> 
> Either way, there is some duplication there because we have a
> parent-child relationship both between the object types themselves and
> between their property classes. Either we remove the base from
> SecretProperties (which would make object-add and the CLI more
> complicated) or we just let the QAPI generator check that they are
> consistent.

I don't really like the duplicate hierarchies there either. I did
consider, a new 'object' entity instead of 'struct', but if we go
that way we should exclusively use "object" for the QOM types.
eg an "object" entity type would be a specialization of the
"struct" entity type, rather than something bolted onto the
side.

Basically I think the QOM struct and the properties struct should
remain one & the same thing.

If we think of "object" as a specialization of 'struct' then and
have "state" as a separate struct, we avoid the duplicate hierarchies


  { 'object': 'QCryptoSecretCommon',
    'state': 'QCryptoSecretCommonState',
    'data': { '*format': 'QCryptoSecretFormat',
              '*keyid': 'str',
              '*iv': 'str' } }
 
  { 'object': 'QCryptoSecret',
    'base': 'QCryptoSecretCommon',
    'data': { '*data': 'str',
              '*file': 'str' } }

there's not really much difference to this than just carrying
on using "struct" entity type though, and having the special
"Object" parent type as a built-in type.

> > There would need to be a
> > 
> >    void QCryptoSecretCommonFreeState(QCryptoSecretCommon *obj)
> > 
> > method defined manually by the programmer to take care of free'ing any
> > pointers in the "state".
> 
> Isn't this the job of the normal .instance_finalize method?

Yes, but I was thinking the fact how to wire into the free methods that
QAPI generates. 


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 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 30/11/20 16:30, Daniel P. Berrangé wrote:
> { 'struct': 'QCryptoSecretCommon',
>    'base': 'Object',
>    'state': { 'rawdata': '*uint8_t',
>               'rawlen': 'size_t' },
>    'data': { '*format': 'QCryptoSecretFormat',
>              '*keyid': 'str',
>              '*iv': 'str' } }
> 
> { 'struct': 'QCryptoSecret',
>    'base': 'QCryptoSecretCommon',
>    'data': { '*data': 'str',
>              '*file': 'str' } }

No, please don't do this.  I want to keep writing C code, not a weird 
JSON syntax for C.

I'd much rather have a FooOptions field in my struct so that I can just 
do visit_FooOptions in the UserCreatable.complete() method, that's feasible.

Paolo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 30/11/20 16:30, Daniel P. Berrangé wrote:
>> { 'struct': 'QCryptoSecretCommon',
>>    'base': 'Object',
>>    'state': { 'rawdata': '*uint8_t',
>>               'rawlen': 'size_t' },
>>    'data': { '*format': 'QCryptoSecretFormat',
>>              '*keyid': 'str',
>>              '*iv': 'str' } }
>> { 'struct': 'QCryptoSecret',
>>    'base': 'QCryptoSecretCommon',
>>    'data': { '*data': 'str',
>>              '*file': 'str' } }
>
> No, please don't do this.  I want to keep writing C code, not a weird
> JSON syntax for C.
>
> I'd much rather have a FooOptions field in my struct so that I can
> just do visit_FooOptions in the UserCreatable.complete() method,
> that's feasible.

It should be, because it's what we've done elsewhere, isn't it?

Yes, we can extend QAPI to let us embed opaques outside the QAPI
schema's scope ("state"), along with means to create, destroy, and
clone.  This is new.

But we can also invert: put the QAPI-generated C type ("config") in a
handwritten C type that marries it to "state".

Code to create and destroy is handwritten, and uses QAPI-generated code
for the "config" part.

A generic interface to handwritten creation is possible: Take the
QAPI-generated "config" type as argument, extract enough information to
call the appropriate constructor, return its value.

Generic destruction is also possible: all it needs is a map from
instance to destructor function.

None of this is new in QEMU.

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 30.11.2020 um 15:58 hat Paolo Bonzini geschrieben:
> On 30/11/20 13:25, Kevin Wolf wrote:
> > This series adds a QAPI type for the properties of all user creatable
> > QOM types and finally makes QMP object-add use the new ObjectOptions
> > union so that QAPI introspection can be used for user creatable objects.
> > 
> > After this series, there is least one obvious next step that needs to be
> > done: Change HMP and all of the command line parser to use
> > ObjectOptions, too, so that the QAPI schema is consistently enforced in
> > all external interfaces. I am planning to send another series to address
> > this.
> > 
> > In a third step, we can try to start deduplicating and integrating things
> > better between QAPI and the QOM implementation, e.g. by generating parts
> > of the QOM boilerplate from the QAPI schema.
> 
> With this series it's basically pointless to have QOM properties at
> all.

Not entirely, because there are still some writable properties that can
be changed later on.

After working through all the user creatable objects, I would say that
separating these from the creation-time options is actually a good thing
because there are basically two types of property setters in the
existing implementations:

1. It starts with something like 'if (completed)' and takes two different
   paths, so they are already separated. Often one path is just
   returning an error, but sometimes we actually make an effort to
   update the internal state according to the new value.

2. No distinction is made. Usually the result is inconsistent state
   because the property values themselves are updated, but they have
   been interpreted once in ucc->complete and are ignored afterwards. Or
   maybe even worse, they are still used, but no care is taken that they
   are consistent with the rest of the internal state.

   Unfortunately my impression is that this is the more common type.

So with this in mind, I think I'm in favour of completely leaving the
initialisation of properties on object creation to QAPI, and only
providing individual setters if we actually intend to allow property
changes after creation.

> Instead, you are basically having half of QEMU's backend data model
> into a single struct.
> 
> So the question is, are we okay with shoveling half of QEMU's backend data
> model into a single struct?  If so, there are important consequences.

Yeah, the single struct bothers me a bit, both in the QAPI schema and in
the C source.

We probably need to have it present in the schema in some way so we can
actually check input against the schema. Maybe we can have it
automatically compiled by the QAPI generator so that we don't need to
manually update the enum and the union each time.

In the C source, I guess the other option would be to have pointers
rather than directly embedding all struct types. In the long run this
might make more sense. As long as it's only user-creatable objects, it's
no worse than BlockdevOptions.

> 1) QOM basically does not need properties anymore except for devices and
> machines (accelerators could be converted to QAPI as well). All
> user-creatable objects can be changed to something like chardev's "get a
> struct and use it fill in the fields", and only leave properties to devices
> and machines.

True for those properties that don't support updates after object
creation. For these, leaving the work to QAPI simplifies things a lot.

> 2) User-creatable objects can have a much more flexible schema.  This means
> there's no reason to have block device creation as its own command and
> struct for example.

In theory yes. The block layer isn't really QAPIfied, though, it just
has a QAPI wrapper (similar to how this series doesn't QAPIfy QOM, but
justs wraps it). But for the long term vision, I think it's a reasonable
goal to have block nodes represented as QOM-with-QAPI objects.

> The problem with this series is that you are fine with deduplicating things
> as a third step, but you cannot be sure that such deduplication is possible
> at all.  So while I don't have any problems in principle with the
> ObjectOptions concept, I don't think it should be committed without a clear
> idea of how to do the third step.

Do you have any specific concerns why the deduplication might not
possible, or just because it's uncharted territory?

The only reason why I wouldn't like to wait too long with merging this
is because of merge conflicts (the list of properties or their details
might change and this could go unnoticed).

Maybe if we don't want to commit to keeping the ObjectOptions schema,
the part that should wait is object-add and I should do the command line
options first? Then the schema remains an implementation detail for now
that is invisible in introspection.

> In the meanwhile, of course I have no problem with deprecating the opened
> and loaded properties.

If we decide that we don't want to have the schema at all (which I hope
we won't decide), I can split the deprecation into separate patches.

Kevin

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 30/11/20 16:46, Kevin Wolf wrote:
> Am 30.11.2020 um 15:58 hat Paolo Bonzini geschrieben:
>> With this series it's basically pointless to have QOM properties at
>> all.
> 
> Not entirely, because there are still some writable properties that can
> be changed later on.

Are there really any (that are not bugs like opened/loaded)? That's also 
why Eduardo and I discussed a class-wide allow_set function for his 
field properties series.

> So with this in mind, I think I'm in favour of completely leaving the
> initialisation of properties on object creation to QAPI, and only
> providing individual setters if we actually intend to allow property
> changes after creation.

The main problem is that it wouldn't extend well, if at all, to machines 
and devices.  So those would still not be integrated into the QAPI schema.

>> So the question is, are we okay with shoveling half of QEMU's backend data
>> model into a single struct?  If so, there are important consequences.
> 
> Yeah, the single struct bothers me a bit, both in the QAPI schema and in
> the C source.

The single struct doesn't bother me _too much_ actually.  What bothers 
me is that it won't be a single source of all QOM objects, only those 
that happen to be created by object-add.  So I start to wonder if QOM as 
it exists now is the right solution for all kind of objects:

- backends and other object-add types (not per-target, relatively few 
classes and even fewer hierarchies)

- machine (per-target, many classes, no hierarchy)

- device (can be treated as not per-target, many many classes, very few 
hierarchies)

- accelerator (per-target, few classes, no hierarchy)

- chardev (ok those are the same as the first category)

If QOM is the right solution, this patch goes in the wrong direction.

If QOM is the wrong solution, this patch is okay but then we have 
another problem to solve. :)

>> The problem with this series is that you are fine with deduplicating things
>> as a third step, but you cannot be sure that such deduplication is possible
>> at all.  So while I don't have any problems in principle with the
>> ObjectOptions concept, I don't think it should be committed without a clear
>> idea of how to do the third step.
> 
> Do you have any specific concerns why the deduplication might not
> possible, or just because it's uncharted territory?

Mostly because it's more or less the same issue that you have with 
BlockdevOptions, with the extra complication that this series only deals 
with the easy one of the four above categories.

> Maybe if we don't want to commit to keeping the ObjectOptions schema,
> the part that should wait is object-add and I should do the command line
> options first? Then the schema remains an implementation detail for now
> that is invisible in introspection.

I don't see much benefit in converting _any_ of the three actually.  The 
only good reason I see for QAPIfying this is the documentation, and the 
promise of deduplicating QOM boilerplate.  The latter is only a promise, 
but documentation alone is a damn good reason and it's enough to get 
this work into a mergeable shape as soon as possible!

But maybe, we could start in the opposite direction: start with the use 
QAPI to eliminate QOM boilerplate.  Basing your work on Eduardo's field 
properties series, you could add a new 'object' "kind" to QAPI that 
would create an array of field properties (e.g. a macro expanding to a 
compound literal?)
.  Something like


+{ 'object': 'InputBarrier',
+  'data': { 'name': 'str',
+            'x-origin': 'int16',
+            'y-origin': 'int16',
+            'width': 'int16',
+            'height': 'int16' },
+  'properties': { 'server': 'str',
+                  'port': 'str' } }

would create a macro QOM_InputBarrier_FIELDS defining properties for the 
following fields of the InputBarrier struct:

     gchar *name;
     int16_t x_origin, y_origin;
     int16_t width, height;

while server and port would only appear in the documentation (or 
alternatively you could leave them out completely, as you wish).
The advantages would be noticeable:

1) the information would be moved in the QAPI schema JSON from the 
beginning, decoupling the conflict-heavy part from the complex question 
of how to expose the QOM schema in the introspection data

2) there would not be any more duplication than before (there would be 
duplication between structs and QAPI schema, but not between structs and 
C code that defines properties).

3) it would be opt-in, so it doesn't put on you the burden of keeping 
the series in sync with new objects that are added (I have one for the 
qtest server for example).  At the same time it would be quite appealing 
for owners of QOM code to convert their objects to field properties and 
get documentation for free.

4) we could special-case 'object' definitions and generate them in the 
system manual as well, since they are also useful to document -object.

Yes it's a huge change but you have the boring part already done.  What 
do you think?

Paolo

>> In the meanwhile, of course I have no problem with deprecating the opened
>> and loaded properties.
> 
> If we decide that we don't want to have the schema at all (which I hope
> we won't decide), I can split the deprecation into separate patches.
> 
> Kevin
> 

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 30.11.2020 um 17:57 hat Paolo Bonzini geschrieben:
> On 30/11/20 16:46, Kevin Wolf wrote:
> > Am 30.11.2020 um 15:58 hat Paolo Bonzini geschrieben:
> > > With this series it's basically pointless to have QOM properties at
> > > all.
> > 
> > Not entirely, because there are still some writable properties that can
> > be changed later on.
> 
> Are there really any (that are not bugs like opened/loaded)? That's also why
> Eduardo and I discussed a class-wide allow_set function for his field
> properties series.

Yes. I don't really know most objects apart from what I had to learn for
this series, but I know at least two that actually allow this
intentionally: iothread and throttle-group.

> > So with this in mind, I think I'm in favour of completely leaving the
> > initialisation of properties on object creation to QAPI, and only
> > providing individual setters if we actually intend to allow property
> > changes after creation.
> 
> The main problem is that it wouldn't extend well, if at all, to
> machines and devices.  So those would still not be integrated into the
> QAPI schema.

What do you think is the biggest difference there? Don't devices work
the same as user creatable objects in that you first set a bunch of
properties (which would now be done through QAPI instead) and then call
the completion/realize method that actually makes use of them?

I must admit that I don't know how machine types work.

> > > So the question is, are we okay with shoveling half of QEMU's backend data
> > > model into a single struct?  If so, there are important consequences.
> > 
> > Yeah, the single struct bothers me a bit, both in the QAPI schema and in
> > the C source.
> 
> The single struct doesn't bother me _too much_ actually.  What bothers me is
> that it won't be a single source of all QOM objects, only those that happen
> to be created by object-add.

But isn't it only natural that a list of these objects will exist in
some way, implicitly or explicitly? object-add must somehow decide which
object types it allows the user to create.

Once we describe the object types in the schema (rather than only their
properties), which is required if we want to generate the QOM
boilerplate, this can possibly become implicit because then QAPI can
know which objects implement USER_CREATABLE.

> So I start to wonder if QOM as it exists now is the right solution for
> all kind of objects:
> 
> - backends and other object-add types (not per-target, relatively few
> classes and even fewer hierarchies)
> 
> - machine (per-target, many classes, no hierarchy)
> 
> - device (can be treated as not per-target, many many classes, very few
> hierarchies)
> 
> - accelerator (per-target, few classes, no hierarchy)
> 
> - chardev (ok those are the same as the first category)
> 
> If QOM is the right solution, this patch goes in the wrong direction.
> 
> If QOM is the wrong solution, this patch is okay but then we have another
> problem to solve. :)

I think the requirements for all of them are probably similar enough
that they can potentially be covered by a single thing.

I'm also pretty sure that QOM as it exists now is not the right solution
for any of them because it has some major shortcomings. It's too easy to
get things wrong (like the writable properties after creation), its
introspection is rather weak and separated from the QAPI introspection,
it doesn't encourage documentation, and it involves quite a bit of
boilerplate and duplicated code between class implementations.

A modified QOM might be the right solution, though. I would like to
bring QAPI and QOM together because most of these weaknesses are
strengths of QAPI.

I guess the real question is what aspects of QOM need to be changed to
make it the right solution.

> > > The problem with this series is that you are fine with deduplicating things
> > > as a third step, but you cannot be sure that such deduplication is possible
> > > at all.  So while I don't have any problems in principle with the
> > > ObjectOptions concept, I don't think it should be committed without a clear
> > > idea of how to do the third step.
> > 
> > Do you have any specific concerns why the deduplication might not
> > possible, or just because it's uncharted territory?
> 
> Mostly because it's more or less the same issue that you have with
> BlockdevOptions, with the extra complication that this series only deals
> with the easy one of the four above categories.

I'm not sure which exact problem with BlockdevOptions you mean. The
reason why the block layer doesn't use BlockdevOptions everywhere is
-drive support.

> > Maybe if we don't want to commit to keeping the ObjectOptions schema,
> > the part that should wait is object-add and I should do the command line
> > options first? Then the schema remains an implementation detail for now
> > that is invisible in introspection.
> 
> I don't see much benefit in converting _any_ of the three actually.  The
> only good reason I see for QAPIfying this is the documentation, and the
> promise of deduplicating QOM boilerplate.  The latter is only a promise, but
> documentation alone is a damn good reason and it's enough to get this work
> into a mergeable shape as soon as possible!

I think having some input validation done by QAPI instead of in each QOM
object individually is nice, too. You get it after CLI, QMP and HMP all
go through QAPI.

> But maybe, we could start in the opposite direction: start with the use QAPI
> to eliminate QOM boilerplate.  Basing your work on Eduardo's field
> properties series, you could add a new 'object' "kind" to QAPI that would
> create an array of field properties (e.g. a macro expanding to a compound
> literal?)

There is a very simple reason why I don't want to start with the QAPI
generator: John has multiple series pending that touch basically every
part of the QAPI generator. This means not only that I need to be
careful about merge conflict (or base my work on top of five other
series, which feels adventurous), but also that I would be competing
with John for Markus' reviewer capacity, further slowing things down.

Well, two reasons: Also because this series for the external interface
of the objects already exists and it's an incremental step towards your
proposal: The types for 'properties' will already exist then and I won't
have to convert both internal state and external interfaces at the same
time.

> .  Something like
> 
> 
> +{ 'object': 'InputBarrier',
> +  'data': { 'name': 'str',
> +            'x-origin': 'int16',
> +            'y-origin': 'int16',
> +            'width': 'int16',
> +            'height': 'int16' },
> +  'properties': { 'server': 'str',
> +                  'port': 'str' } }

I think we have similar ideas there (see my reply to Dan), just that I
see it as an incremental step on top of this one.

> would create a macro QOM_InputBarrier_FIELDS defining properties for the
> following fields of the InputBarrier struct:
> 
>     gchar *name;
>     int16_t x_origin, y_origin;
>     int16_t width, height;
> 
> while server and port would only appear in the documentation (or
> alternatively you could leave them out completely, as you wish).
> The advantages would be noticeable:
> 
> 1) the information would be moved in the QAPI schema JSON from the
> beginning, decoupling the conflict-heavy part from the complex question of
> how to expose the QOM schema in the introspection data
> 
> 2) there would not be any more duplication than before (there would be
> duplication between structs and QAPI schema, but not between structs and C
> code that defines properties).
> 
> 3) it would be opt-in, so it doesn't put on you the burden of keeping the
> series in sync with new objects that are added (I have one for the qtest
> server for example).  At the same time it would be quite appealing for
> owners of QOM code to convert their objects to field properties and get
> documentation for free.
> 
> 4) we could special-case 'object' definitions and generate them in the
> system manual as well, since they are also useful to document -object.
> 
> Yes it's a huge change but you have the boring part already done.  What do
> you think?

Yes, I would be happy to work on something like that. Just not as the
first step, because I think it's nothing that would help us get this
series in a mergable state as soon as possible.

Kevi

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 30/11/20 19:10, Kevin Wolf wrote:
> Am 30.11.2020 um 17:57 hat Paolo Bonzini geschrieben:
>> The main problem is that it wouldn't extend well, if at all, to
>> machines and devices.  So those would still not be integrated into the
>> QAPI schema.
> 
> What do you think is the biggest difference there? Don't devices work
> the same as user creatable objects in that you first set a bunch of
> properties (which would now be done through QAPI instead) and then call
> the completion/realize method that actually makes use of them?

For devices it's just the practical issue that there are too many to 
have something like this series.  For machine types the main issue is 
that the version-specific machine types would have to be defined in one 
more place.

>> The single struct doesn't bother me _too much_ actually.  What bothers me is
>> that it won't be a single source of all QOM objects, only those that happen
>> to be created by object-add.
> 
> But isn't it only natural that a list of these objects will exist in
> some way, implicitly or explicitly? object-add must somehow decide which
> object types it allows the user to create.

There's already one, it's objects that implement user creatable.  I 
don't think having it as a QAPI enum (or discriminator) is worthwhile, 
and it's one more thing that can go out of sync between the QAPI schema 
and the C code.

> I'm also pretty sure that QOM as it exists now is not the right solution
> for any of them because it has some major shortcomings. It's too easy to
> get things wrong (like the writable properties after creation), its
> introspection is rather weak and separated from the QAPI introspection,
> it doesn't encourage documentation, and it involves quite a bit of
> boilerplate and duplicated code between class implementations.
> 
> A modified QOM might be the right solution, though. I would like to
> bring QAPI and QOM together because most of these weaknesses are
> strengths of QAPI.

I agree wholeheartedly.  But your series goes to the opposite excess. 
Eduardo is doing work in QOM to mitigate the issues you point out, and 
you have to meet in the middle with him.  Starting with the user-visible 
parts focuses on the irrelevant parts.

>> Mostly because it's more or less the same issue that you have with
>> BlockdevOptions, with the extra complication that this series only deals
>> with the easy one of the four above categories.
> 
> I'm not sure which exact problem with BlockdevOptions you mean. The
> reason why the block layer doesn't use BlockdevOptions everywhere is
> -drive support.

You don't have a single BlockdevOptions* field in the structs of block/. 
  My interpretation of this is that BlockdevOptions* is a good schema 
for configuring things but not for run-time state.

>>> Maybe if we don't want to commit to keeping the ObjectOptions schema,
>>> the part that should wait is object-add and I should do the command line
>>> options first? Then the schema remains an implementation detail for now
>>> that is invisible in introspection.
>>
>> I don't see much benefit in converting _any_ of the three actually.  The
>> only good reason I see for QAPIfying this is the documentation, and the
>> promise of deduplicating QOM boilerplate.  The latter is only a promise, but
>> documentation alone is a damn good reason and it's enough to get this work
>> into a mergeable shape as soon as possible!
> 
> I think having some input validation done by QAPI instead of in each QOM
> object individually is nice, too. You get it after CLI, QMP and HMP all
> go through QAPI.

But the right way to do that is to use Eduardo's field properties: they 
make QOM do the right thing, adding another layer of parsing is just 
adding more complexity.  Are there any validation bugs that you're 
fixing?  Is that something that cannot be fixed elsewhere, or are you 
papering over bad QOM coding?  (Again, I'm not debating that QOM 
properties are hard to write correctly).

>> But maybe, we could start in the opposite direction: start with the use QAPI
>> to eliminate QOM boilerplate.  Basing your work on Eduardo's field
>> properties series, you could add a new 'object' "kind" to QAPI that would
>> create an array of field properties (e.g. a macro expanding to a compound
>> literal?)
> 
> There is a very simple reason why I don't want to start with the QAPI
> generator: John has multiple series pending that touch basically every
> part of the QAPI generator. This means not only that I need to be
> careful about merge conflict (or base my work on top of five other
> series, which feels adventurous), but also that I would be competing
> with John for Markus' reviewer capacity, further slowing things down.

That's something that you have to discuss with John and Markus.  It may 
be that John has to shelve part of his work or rebase on top of yours 
instead.

By adding an extra parsing layer you're adding complexity that may not 
be needed in the end, and we're very bad of removing it later.  So I'm 
worried that this series will become technical debt in just a few months.

> Well, two reasons: Also because this series for the external interface
> of the objects already exists and it's an incremental step towards your
> proposal: The types for 'properties' will already exist then and I won't
> have to convert both internal state and external interfaces at the same
> time.

I don't think converting internal state to QAPI is useful.  QAPI and QOM 
are external interfaces and that's what they should remain; anything 
that is not an external interface should (must) remain plain C code.

Paolo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 30.11.2020 um 20:35 hat Paolo Bonzini geschrieben:
> On 30/11/20 19:10, Kevin Wolf wrote:
> > Am 30.11.2020 um 17:57 hat Paolo Bonzini geschrieben:
> > > The main problem is that it wouldn't extend well, if at all, to
> > > machines and devices.  So those would still not be integrated into the
> > > QAPI schema.
> > 
> > What do you think is the biggest difference there? Don't devices work
> > the same as user creatable objects in that you first set a bunch of
> > properties (which would now be done through QAPI instead) and then call
> > the completion/realize method that actually makes use of them?
> 
> For devices it's just the practical issue that there are too many to have
> something like this series.  For machine types the main issue is that the
> version-specific machine types would have to be defined in one more place.

Sure, the number of devices means that we can't convert all of them at
once. And we don't need to, we can make the change incrementally.

The only thing we can't do during this incremental phase is switching
device_add from 'gen': false to actually using QAPI types. Doing that
would be the very last step in the conversion.

> > > The single struct doesn't bother me _too much_ actually.  What bothers me is
> > > that it won't be a single source of all QOM objects, only those that happen
> > > to be created by object-add.
> > 
> > But isn't it only natural that a list of these objects will exist in
> > some way, implicitly or explicitly? object-add must somehow decide which
> > object types it allows the user to create.
> 
> There's already one, it's objects that implement user creatable.  I don't
> think having it as a QAPI enum (or discriminator) is worthwhile, and it's
> one more thing that can go out of sync between the QAPI schema and the C
> code.

Well, we all know that this series duplicates things. But at the same
time, we also know that this isn't going to be the final state.

Once QAPI learns about QOM inheritance (which it has to if it should
generate the boilerplate), it will know which objects are user creatable
without a an explicitly defined separate enum.

I think it will still need to have the enum internally, but as long as
it's automatically generated, that shouldn't be a big deal.

> > I'm also pretty sure that QOM as it exists now is not the right solution
> > for any of them because it has some major shortcomings. It's too easy to
> > get things wrong (like the writable properties after creation), its
> > introspection is rather weak and separated from the QAPI introspection,
> > it doesn't encourage documentation, and it involves quite a bit of
> > boilerplate and duplicated code between class implementations.
> > 
> > A modified QOM might be the right solution, though. I would like to
> > bring QAPI and QOM together because most of these weaknesses are
> > strengths of QAPI.
> 
> I agree wholeheartedly.  But your series goes to the opposite excess.
> Eduardo is doing work in QOM to mitigate the issues you point out, and you
> have to meet in the middle with him.  Starting with the user-visible parts
> focuses on the irrelevant parts.

QAPI is first and foremost about user-visible parts, and in fact most of
the problems I listed are about external interfaces. It seems to make a
lot of sense to me to start there.

> > > Mostly because it's more or less the same issue that you have with
> > > BlockdevOptions, with the extra complication that this series only deals
> > > with the easy one of the four above categories.
> > 
> > I'm not sure which exact problem with BlockdevOptions you mean. The
> > reason why the block layer doesn't use BlockdevOptions everywhere is
> > -drive support.
> 
> You don't have a single BlockdevOptions* field in the structs of block/.  My
> interpretation of this is that BlockdevOptions* is a good schema for
> configuring things but not for run-time state.

I'm not sure what you mean with run-time state. Yes, BlockdevOptions is
about external interfaces, not about implementation details. Same thing
as QOM properties are external interfaces, not implementation details.
There may be fields in the internal state that correspond 1:1 to the
externally visible QAPI type/QOM property, but it's not necessarily the
case.

> > > > Maybe if we don't want to commit to keeping the ObjectOptions schema,
> > > > the part that should wait is object-add and I should do the command line
> > > > options first? Then the schema remains an implementation detail for now
> > > > that is invisible in introspection.
> > > 
> > > I don't see much benefit in converting _any_ of the three actually.  The
> > > only good reason I see for QAPIfying this is the documentation, and the
> > > promise of deduplicating QOM boilerplate.  The latter is only a promise, but
> > > documentation alone is a damn good reason and it's enough to get this work
> > > into a mergeable shape as soon as possible!
> > 
> > I think having some input validation done by QAPI instead of in each QOM
> > object individually is nice, too. You get it after CLI, QMP and HMP all
> > go through QAPI.
> 
> But the right way to do that is to use Eduardo's field properties: they make
> QOM do the right thing, adding another layer of parsing is just adding more
> complexity.

QAPI is already here and it's going to stay. QOM doesn't have to
duplicate input validation that existing code can already perform.

I'm not sure which complexity you think I'm introducing: QAPI is already
there. I'm adding the schema, which you agree is valuable documentation,
so we want to have it either case. The actual change to QOM that we have
in this series is this:

 include/qom/object_interfaces.h |  7 -------
 qom/qom-qmp-cmds.c              | 44 ++++++++++++++++++++++----------------------
 2 files changed, 22 insertions(+), 29 deletions(-)

Arguably some more runtime complexity on object-add because we do the
QAPI validation and build an ObjectOptions object just to feed it back
to a visitor. Do we really care?

> Are there any validation bugs that you're fixing?  Is that
> something that cannot be fixed elsewhere, or are you papering over bad QOM
> coding?  (Again, I'm not debating that QOM properties are hard to write
> correctly).

Yes, I found bugs that the QAPI schema would have prevented. They were
generally about not checking whether mandatory propertes are actually
set.

Of course you can fix broken QOM property implementations manually. And
I sent separate patches to fix them manually because we don't go through
QAPI for user creatable types yet (even after this series, -object would
still be missing).

But if a property is declared mandatory in the schema, I shouldn't have
to bother with manual checks in ucc->complete.

> > > But maybe, we could start in the opposite direction: start with the use QAPI
> > > to eliminate QOM boilerplate.  Basing your work on Eduardo's field
> > > properties series, you could add a new 'object' "kind" to QAPI that would
> > > create an array of field properties (e.g. a macro expanding to a compound
> > > literal?)
> > 
> > There is a very simple reason why I don't want to start with the QAPI
> > generator: John has multiple series pending that touch basically every
> > part of the QAPI generator. This means not only that I need to be
> > careful about merge conflict (or base my work on top of five other
> > series, which feels adventurous), but also that I would be competing
> > with John for Markus' reviewer capacity, further slowing things down.
> 
> That's something that you have to discuss with John and Markus.  It may be
> that John has to shelve part of his work or rebase on top of yours instead.
> 
> By adding an extra parsing layer you're adding complexity that may not be
> needed in the end, and we're very bad of removing it later.  So I'm worried
> that this series will become technical debt in just a few months.

Again: Which complexity?

> > Well, two reasons: Also because this series for the external interface
> > of the objects already exists and it's an incremental step towards your
> > proposal: The types for 'properties' will already exist then and I won't
> > have to convert both internal state and external interfaces at the same
> > time.
> 
> I don't think converting internal state to QAPI is useful.  QAPI and QOM are
> external interfaces and that's what they should remain; anything that is not
> an external interface should (must) remain plain C code.

Yes, I agree. What I mean is changing property declarations and probably
replacing most existing property getters or setters, which in turn
access the internal state. That's just different work from describing
the external interface.

Kevin

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 01/12/20 17:20, Kevin Wolf wrote:
> Am 30.11.2020 um 20:35 hat Paolo Bonzini geschrieben:
>> For devices it's just the practical issue that there are too many to have
>> something like this series.  For machine types the main issue is that the
>> version-specific machine types would have to be defined in one more place.
>
> Sure, the number of devices means that we can't convert all of them at
> once. And we don't need to, we can make the change incrementally.

There's also the question that most devices are not present in all 
binaries.  So QAPI introspection would tell you what properties are 
supported but not what devices are.  Also the marshaling/unmarshaling 
code for hundreds of devices would bloat the QEMU executables 
unnecessarily (it'd all be reachable from visit_DeviceOptions so it'd 
not be possible to compile it out, even with LTO).

Plus the above issue with machine types.

>>>> The single struct doesn't bother me _too much_ actually.  What bothers me is
>>>> that it won't be a single source of all QOM objects, only those that happen
>>>> to be created by object-add.
>>>
>>> But isn't it only natural that a list of these objects will exist in
>>> some way, implicitly or explicitly? object-add must somehow decide which
>>> object types it allows the user to create.
>>
>> There's already one, it's objects that implement user creatable.  I don't
>> think having it as a QAPI enum (or discriminator) is worthwhile, and it's
>> one more thing that can go out of sync between the QAPI schema and the C
>> code.
> 
> Well, we all know that this series duplicates things. But at the same
> time, we also know that this isn't going to be the final state.
> 
> Once QAPI learns about QOM inheritance (which it has to if it should
> generate the boilerplate), it will know which objects are user creatable
> without a an explicitly defined separate enum.
> 
> I think it will still need to have the enum internally, but as long as
> it's automatically generated, that shouldn't be a big deal.

Right, I don't want to have the final state now but I'd like to have a 
rough idea of the plan:

1) whether to generate _all_ boilerplate or only properties

2) whether we want to introduce a separation between configuration 
schema and run-time state

3) in the latter case, whether properties will survive at all---iothread 
and throttle-groups don't really need them even if they're writable 
after creation.

These questions have a substantial effect on how to handle this series. 
  Without answers to this questions I cannot understand the long term 
and its interaction with other long term plans for QOM.

>>> A modified QOM might be the right solution, though. I would like to
>>> bring QAPI and QOM together because most of these weaknesses are
>>> strengths of QAPI.
>>
>> I agree wholeheartedly.  But your series goes to the opposite excess.
>> Eduardo is doing work in QOM to mitigate the issues you point out, and you
>> have to meet in the middle with him.  Starting with the user-visible parts
>> focuses on the irrelevant parts.
> 
> QAPI is first and foremost about user-visible parts, and in fact most of
> the problems I listed are about external interfaces.

Yes, but QAPI is also about interfacing with existing code.  Also, QAPI 
does not generate only structs, it also generate C function prototypes. 
I'm not sure whether a QOM object's more similar to the struct case 
(what you do here) or to the QMP command case:

- there's a 1:1 correspondence between ObjectOptions cases and 
ucc->complete implementations

- there's a registry of object types just like there's one for QMP commands.

So another possibility could be that the QAPI generator essentially 
generated the user_creatable_add_type function that called out to 
user-provided functions qom_scsi_pr_helper_complete, 
qom_throttle_group_complete, etc.  The arguments to those functions 
would be the configuration.  That is a very interesting prospect (though 
one that would require changes to the QAPI code generator).

> BlockdevOptions is about external interfaces, not about
> implementation details. Same thing as QOM properties are external
> interfaces, not implementation details. There may be fields in the
> internal state that correspond 1:1 to the externally visible QAPI
> type/QOM property, but it's not necessarily the case.

Right.  It may well be that we decide that, as a result of this series, 
QOM's property interface is declared essentially a failed experiment.  I 
wouldn't be surprised, and that's why I want to understand better where 
we want to go.

For example, Eduardo is focusing specifically on external interfaces 
that correspond 1:1 to the internal implementation.  If we decide that 
non-1:1-mappings and checks on mandatory properties are an important 
part of QOM, that may make large parts of his work moot.  If we decide 
that most QOM objects need no properties at all, then we don't want to 
move more qdev-specific stuff from to QOM.

> QAPI is already here and it's going to stay. QOM doesn't have to
> duplicate input validation that existing code can already perform.
> 
> I'm not sure which complexity you think I'm introducing: QAPI is already
> there. I'm adding the schema, which you agree is valuable documentation,
> so we want to have it either case. The actual change to QOM that we have
> in this series is this:

The complexity is that properties used to be split in two places, and 
now they're split in three places.

It may very well be that this is a good first step to at least have 
classes described in the QAPI schema.  But since _in the short term_ 
there are things that the series makes worse (and has a risk of bringing 
things out of sync), I'd like to understand the long term plan and 
ensure that the QAPI maintainers are on board with it.

Can you at least add a comment to all UserCreatable classes that says 
"if you add a property, remember to modify ... as well in the QAPI schema"?

>> Are there any validation bugs that you're fixing?  Is that
>> something that cannot be fixed elsewhere, or are you papering over bad QOM
>> coding?  (Again, I'm not debating that QOM properties are hard to write
>> correctly).
> 
> Yes, I found bugs that the QAPI schema would have prevented. They were
> generally about not checking whether mandatory propertes are actually
> set.

Okay, I found your series at 
https://patchew.org/QEMU/20201130105615.21799-1-kwolf@redhat.com/ too, 
good to know.

So that's another useful thing that can be chalked to this series at 
least if -object and object_add are converted (and also, another thing 
against QOM properties and 1:1 mappings between configuration schema and 
run-time state).

Paolo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Eduardo Habkost]

On Tue, Dec 01, 2020 at 06:16:14PM +0100, Paolo Bonzini wrote:
> On 01/12/20 17:20, Kevin Wolf wrote:
[...]
> > BlockdevOptions is about external interfaces, not about
> > implementation details. Same thing as QOM properties are external
> > interfaces, not implementation details. There may be fields in the
> > internal state that correspond 1:1 to the externally visible QAPI
> > type/QOM property, but it's not necessarily the case.
> 
> Right.  It may well be that we decide that, as a result of this series,
> QOM's property interface is declared essentially a failed experiment.  I
> wouldn't be surprised, and that's why I want to understand better where we
> want to go.
> 
> For example, Eduardo is focusing specifically on external interfaces that
> correspond 1:1 to the internal implementation.  If we decide that
> non-1:1-mappings and checks on mandatory properties are an important part of
> QOM, that may make large parts of his work moot.  [...]

Whatever we decide, my first and biggest worry is to have a
reasonable migration path for any new API.

We could describe in detail what's the API we _want_, but we also
need a reasonable migration path for the code we already _have_.
If a new API that requires manual conversion of existing devices,
it will probably coexist with the existing qdev property API for
years.

(Note that I haven't read this whole thread yet.)

>                                            [...]  If we decide that most QOM
> objects need no properties at all, then we don't want to move more
> qdev-specific stuff from to QOM.

I don't understand what "move more qdev-specific stuff to QOM"
means.  I consider the QOM field property API valuable even if we
decide the only user of the API will be legacy qdev code, because
it separates the core mechanism (that's code that already
existed) from qdev-specific policies.

> 
> > QAPI is already here and it's going to stay. QOM doesn't have to
> > duplicate input validation that existing code can already perform.
> > 
> > I'm not sure which complexity you think I'm introducing: QAPI is already
> > there. I'm adding the schema, which you agree is valuable documentation,
> > so we want to have it either case. The actual change to QOM that we have
> > in this series is this:
> 
> The complexity is that properties used to be split in two places, and now
> they're split in three places.
> 
> It may very well be that this is a good first step to at least have classes
> described in the QAPI schema.  But since _in the short term_ there are
> things that the series makes worse (and has a risk of bringing things out of
> sync), I'd like to understand the long term plan and ensure that the QAPI
> maintainers are on board with it.
> 
> Can you at least add a comment to all UserCreatable classes that says "if
> you add a property, remember to modify ... as well in the QAPI schema"?
> 
> > > Are there any validation bugs that you're fixing?  Is that
> > > something that cannot be fixed elsewhere, or are you papering over bad QOM
> > > coding?  (Again, I'm not debating that QOM properties are hard to write
> > > correctly).
> > 
> > Yes, I found bugs that the QAPI schema would have prevented. They were
> > generally about not checking whether mandatory propertes are actually
> > set.
> 
> Okay, I found your series at
> https://patchew.org/QEMU/20201130105615.21799-1-kwolf@redhat.com/ too, good
> to know.
> 
> So that's another useful thing that can be chalked to this series at least
> if -object and object_add are converted (and also, another thing against QOM
> properties and 1:1 mappings between configuration schema and run-time
> state).
> 
> Paolo
> 

-- 
Eduardo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben:
> On 01/12/20 17:20, Kevin Wolf wrote:
> > Am 30.11.2020 um 20:35 hat Paolo Bonzini geschrieben:
> > > For devices it's just the practical issue that there are too many to have
> > > something like this series.  For machine types the main issue is that the
> > > version-specific machine types would have to be defined in one more place.
> > 
> > Sure, the number of devices means that we can't convert all of them at
> > once. And we don't need to, we can make the change incrementally.
> 
> There's also the question that most devices are not present in all binaries.
> So QAPI introspection would tell you what properties are supported but not
> what devices are.  Also the marshaling/unmarshaling code for hundreds of
> devices would bloat the QEMU executables unnecessarily (it'd all be
> reachable from visit_DeviceOptions so it'd not be possible to compile it
> out, even with LTO).

I don't think this is actually a new things. We already have types and
commands declared with things like 'if': 'defined(TARGET_S390X)'.
As far as I understand, QAPI generated files are already built per
target, so not compiling things into all binaries should be entirely
possible.

What probably needs a solution is that an explicit 'if' in the schema
would duplicate information that is actually defined in the build system
configuration. So we would somehow need a common source for both.

> Plus the above issue with machine types.

As I said, I don't know the machine type code well, so I'm not really
sure about the best way there.

But maybe QAPI isn't for version-specific machine types. I think they
never have additional properties, but only different init functions. So
their description in QAPI would be essentially empty anyway.

So maybe only the abstract base class that actually defines the machine
properties (like generic-pc-machine) should be described in QAPI, and
then the concrete machine types would inherit from it without being
described in QAPI themselves?

> > > > > The single struct doesn't bother me _too much_ actually.  What bothers me is
> > > > > that it won't be a single source of all QOM objects, only those that happen
> > > > > to be created by object-add.
> > > > 
> > > > But isn't it only natural that a list of these objects will exist in
> > > > some way, implicitly or explicitly? object-add must somehow decide which
> > > > object types it allows the user to create.
> > > 
> > > There's already one, it's objects that implement user creatable.  I don't
> > > think having it as a QAPI enum (or discriminator) is worthwhile, and it's
> > > one more thing that can go out of sync between the QAPI schema and the C
> > > code.
> > 
> > Well, we all know that this series duplicates things. But at the same
> > time, we also know that this isn't going to be the final state.
> > 
> > Once QAPI learns about QOM inheritance (which it has to if it should
> > generate the boilerplate), it will know which objects are user creatable
> > without a an explicitly defined separate enum.
> > 
> > I think it will still need to have the enum internally, but as long as
> > it's automatically generated, that shouldn't be a big deal.
> 
> Right, I don't want to have the final state now but I'd like to have a
> rough idea of the plan:
> 
> 1) whether to generate _all_ boilerplate or only properties

I would like to generate as much boilerplate as possible. That is, I
don't want to constrain us to only properties, but at the same time, I'm
not sure if it's possible to get rid of all boilerplate.

Basically, the vision I have in mind is that QAPI would generate code
that is the same for most instances, and then provide an option that
prevents code generation for a specific part for more complicated cases,
so that the respective function can (and must) be provided in the C
source.

> 2) whether we want to introduce a separation between configuration
> schema and run-time state

You mean the internal run-time state? How is this separation not already
present with getter/setter functions for each property? In many cases
they just directly access the run-time state, but there are other cases
where they actually do things.

Or do you mean between create-time options and properties that can be
written at runtime? In that case, yes, I think that would be very
desirable because mashing them together has resulted in lots of bugs.

> 3) in the latter case, whether properties will survive at all---iothread and
> throttle-groups don't really need them even if they're writable after
> creation.

How do you define properties, i.e. at which point would they stop
existing and what would be a non-property alternative?

Outside of QOM, usually have a query-* command that returns the whole
state rather than a single property, and possibly individual QMP
commands to update the state.


blockdev-reopen takes a new configuration (the same structure as
blockdev-add) and then applies that whole new configuration to the
existing block node. I guess this would be an alternative, though it is
somewhat inconvenient because you have to repeat all the options that
you don't want to change.

The blockdev-reopen way has one important advantage: You can change
multiple options at the same time when there are dependencies between
the options so that individually changing each option would be invalid,
but together it's possible.

throttle-group needs this in QOM, and it's easily implemented: It just
has a single 'limits' property, which is a struct and is always updated
as a single unit.


So in summary, it seems to me that the QOM way is more flexible because
you can get both models out of it. Whether we actually need this
flexibility I can't say.

But there is another reason why I didn't intend to remove properties
entirely, which is that things are a lot easier as long as you don't
have to break external interfaces, which qom-set/get/list are.


The way I imagined it so far is roughly like this:

* Configuration options are described in the QAPI schema. This is mainly
  for object creation, but runtime modifiable properties are a subset of
  this.

* Properties are generated for each option. By default, the getter
  just returns the value from the configuration at creation time, though
  generation of it can be disabled so that it can be overridden. Also,
  setters just return an error by default.

* Property setters aren't called for object creation. Instead, the
  relevant ObjectOptions branch is made available to some init method.

* Runtime modifiable properties (declared as such in the schema) don't
  get the default setter, so you have to provide an implementation for
  them.

> These questions have a substantial effect on how to handle this series.
> Without answers to this questions I cannot understand the long term and its
> interaction with other long term plans for QOM.
> 
> > > > A modified QOM might be the right solution, though. I would like to
> > > > bring QAPI and QOM together because most of these weaknesses are
> > > > strengths of QAPI.
> > > 
> > > I agree wholeheartedly.  But your series goes to the opposite excess.
> > > Eduardo is doing work in QOM to mitigate the issues you point out, and you
> > > have to meet in the middle with him.  Starting with the user-visible parts
> > > focuses on the irrelevant parts.
> > 
> > QAPI is first and foremost about user-visible parts, and in fact most of
> > the problems I listed are about external interfaces.
> 
> Yes, but QAPI is also about interfacing with existing code.  Also, QAPI does
> not generate only structs, it also generate C function prototypes. I'm not
> sure whether a QOM object's more similar to the struct case (what you do
> here) or to the QMP command case:
> 
> - there's a 1:1 correspondence between ObjectOptions cases and ucc->complete
> implementations
> 
> - there's a registry of object types just like there's one for QMP commands.

This is an interesting comparison. I never thought of likening objects
to commands, but there is some truth to it. I certainly did envision
objects as separate type of QAPI entities, which I guess is why I didn't
really like Dan's suggestion to make objects an extension of structs,
but I couldn't really express why. I think your comparison gives a good
answer.

However, this series is still not doing the wrong thing: If you think
about commands, they don't exist without an arguments struct. Similarly,
objects don't exist without a properties struct.

So while this series is doing only one part of the whole solution, that
the second part is missing doesn't make the first part wrong.

> So another possibility could be that the QAPI generator essentially
> generated the user_creatable_add_type function that called out to
> user-provided functions qom_scsi_pr_helper_complete,
> qom_throttle_group_complete, etc.  The arguments to those functions would be
> the configuration.  That is a very interesting prospect (though one that
> would require changes to the QAPI code generator).

I was thinking of using .instance_init because it's more generic, but
using ucc->complete (and in the long run realize for qdev) might require
less modifications of non-user-creatable objects.

One possibly nasty detail to consider there is that we sometimes declare
the USER_CREATABLE interface in the base class, so ucc->complete is for
the base class rather than the actually instantiated class. If we only
instantiate leaf classes (I think this is true), we can move
USER_CREATABLE there.

I also had in mind just passing the whole configuration struct
(essentially always 'boxed': true), but you're right that individual
parameters like for commands would be possible. I'm not entirely
convinced that they would be better (there was a reason why we
introduced 'boxed': true), but it's an option.

> > BlockdevOptions is about external interfaces, not about
> > implementation details. Same thing as QOM properties are external
> > interfaces, not implementation details. There may be fields in the
> > internal state that correspond 1:1 to the externally visible QAPI
> > type/QOM property, but it's not necessarily the case.
> 
> Right.  It may well be that we decide that, as a result of this series,
> QOM's property interface is declared essentially a failed experiment.  I
> wouldn't be surprised, and that's why I want to understand better where we
> want to go.
> 
> For example, Eduardo is focusing specifically on external interfaces that
> correspond 1:1 to the internal implementation.  If we decide that
> non-1:1-mappings and checks on mandatory properties are an important part of
> QOM, that may make large parts of his work moot.  If we decide that most QOM
> objects need no properties at all, then we don't want to move more
> qdev-specific stuff from to QOM.

Hm, it's a good point. 1:1 properties would actually end up in generated
code anyway, so simplifying them is maybe not that useful in the end. It
would probably result in nicer generated code, but that's it.

> > QAPI is already here and it's going to stay. QOM doesn't have to
> > duplicate input validation that existing code can already perform.
> > 
> > I'm not sure which complexity you think I'm introducing: QAPI is already
> > there. I'm adding the schema, which you agree is valuable documentation,
> > so we want to have it either case. The actual change to QOM that we have
> > in this series is this:
> 
> The complexity is that properties used to be split in two places, and now
> they're split in three places.
> 
> It may very well be that this is a good first step to at least have classes
> described in the QAPI schema.  But since _in the short term_ there are
> things that the series makes worse (and has a risk of bringing things out of
> sync), I'd like to understand the long term plan and ensure that the QAPI
> maintainers are on board with it.
> 
> Can you at least add a comment to all UserCreatable classes that says "if
> you add a property, remember to modify ... as well in the QAPI schema"?

I was hoping that by converting object-add in this series, and the CLI
options soon afterwards, it would be very obvious if you forget to
change the schema because your new property just wouldn't work (at least
not during creation).

But I can add comments if you think this is still worthwhile.

> > > Are there any validation bugs that you're fixing?  Is that
> > > something that cannot be fixed elsewhere, or are you papering over bad QOM
> > > coding?  (Again, I'm not debating that QOM properties are hard to write
> > > correctly).
> > 
> > Yes, I found bugs that the QAPI schema would have prevented. They were
> > generally about not checking whether mandatory propertes are actually
> > set.
> 
> Okay, I found your series at
> https://patchew.org/QEMU/20201130105615.21799-1-kwolf@redhat.com/ too, good
> to know.

And two more patches of the same type here:

https://patchew.org/QEMU/20201117163045.307451-1-kwolf@redhat.com/

> So that's another useful thing that can be chalked to this series at least
> if -object and object_add are converted (and also, another thing against QOM
> properties and 1:1 mappings between configuration schema and run-time
> state).

Yes. object-add is already covered by this series and -object should be
easy, so we can have this more or less now. Getting the design right for
QOM code generation will certainly take more time, discussion and also
experimentation.

Kevin

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 01/12/20 20:35, Kevin Wolf wrote:
> Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben:
> I don't think this is actually a new things. We already have types and
> commands declared with things like 'if': 'defined(TARGET_S390X)'.
> As far as I understand, QAPI generated files are already built per
> target, so not compiling things into all binaries should be entirely
> possible.

There is some complication due to having different discriminators per 
target.  So yes it should be possible.  But probably best left after 
objects because it's so much bigger a task and because objects have a 
bit more freedom for experimentation (less ties to other qdev-specific 
concepts, e.g. the magic "bus" property).

> So maybe only the abstract base class that actually defines the machine
> properties (like generic-pc-machine) should be described in QAPI, and
> then the concrete machine types would inherit from it without being
> described in QAPI themselves?

Yes, maybe.

>> 1) whether to generate _all_ boilerplate or only properties
> 
> I would like to generate as much boilerplate as possible. That is, I
> don't want to constrain us to only properties, but at the same time, I'm
> not sure if it's possible to get rid of all boilerplate.
> 
> Basically, the vision I have in mind is that QAPI would generate code
> that is the same for most instances, and then provide an option that
> prevents code generation for a specific part for more complicated cases,
> so that the respective function can (and must) be provided in the C
> source.

Ok, so that's a bit different from what I am thinking of.  I don't care 
very much about the internal boilerplate, only the external interface 
for configuration.  So I don't care about type registration, dynamic 
cast macros etc., only essentially the part that leads to ucc->complete.

>> 2) whether we want to introduce a separation between configuration
>> schema and run-time state
> 
> You mean the internal run-time state? How is this separation not already
> present with getter/setter functions for each property? In many cases
> they just directly access the run-time state, but there are other cases
> where they actually do things.

I mean moving more towards the blockdev/chardev way of doing things, 
increasing the separation very much by having separate configuration 
structs that have (potentially) no link to the run-time state struct.

>> 3) in the latter case, whether properties will survive at all---iothread and
>> throttle-groups don't really need them even if they're writable after
>> creation.
> 
> How do you define properties, i.e. at which point would they stop
> existing and what would be a non-property alternative?

Properties are only a useful concept if they have a use.  If 
-object/object_add/object-add can do the same job without properties, 
properties are not needed anymore.

Right now QOM is all about exposing properties, and having multiple 
interfaces to set them (by picking a different visitor).  But in 
practice most QOM objects have a lifetime that consists of 1) set 
properties 2) flip a switch (realized/complete/open) 3) let the object 
live on its own.  1+2 are a single monitor command or CLI option; during 
3 you access the object through monitor commands, not properties.

> So in summary, it seems to me that the QOM way is more flexible because
> you can get both models out of it. Whether we actually need this
> flexibility I can't say.

I'm thinking there's no need for it, but maybe I'm overly optimistic.

> * Configuration options are described in the QAPI schema. This is mainly
>    for object creation, but runtime modifiable properties are a subset of
>    this.
> 
> * Properties are generated for each option. By default, the getter
>    just returns the value from the configuration at creation time, though
>    generation of it can be disabled so that it can be overridden. Also,
>    setters just return an error by default.
> 
> * Property setters aren't called for object creation. Instead, the
>    relevant ObjectOptions branch is made available to some init method.
> 
> * Runtime modifiable properties (declared as such in the schema) don't
>    get the default setter, so you have to provide an implementation for
>    them.

I wouldn't bother with properties at all in the QAPI schema.  Just do 
the first and third bullet.  Declaring read-only QOM properties is trivial.

> So while this series is doing only one part of the whole solution, that
> the second part is missing doesn't make the first part wrong.

Yeah, I think it's clear that for the long term we're not really 
disagreeing (or perhaps I'm even more radical than you :)).  I'm just 
worried about having yet another incomplete transition.

> One possibly nasty detail to consider there is that we sometimes declare
> the USER_CREATABLE interface in the base class, so ucc->complete is for
> the base class rather than the actually instantiated class. If we only
> instantiate leaf classes (I think this is true), we can move
> USER_CREATABLE there.

You can also use a while loop covering each superclass to decide how to 
dispatch ucc->complete.  I don't care much about these details, they're 
Simple Matter Of Programming. :)

> I also had in mind just passing the whole configuration struct
> (essentially always 'boxed': true), but you're right that individual
> parameters like for commands would be possible. I'm not entirely
> convinced that they would be better (there was a reason why we
> introduced 'boxed': true), but it's an option.

Having 'boxed': true by default would be just an implementation choice, 
nothing to worry about.  (When I said the arguments would be the 
configuration, having a boxed struct as the argument would fit the 
description just as well).

> I was hoping that by converting object-add in this series, and the CLI
> options soon afterwards, it would be very obvious if you forget to
> change the schema because your new property just wouldn't work (at least
> not during creation).

Converting the CLI options is not entirely trivial due to -readconfig 
and friends, so I was expecting that to last until that part of my 6.0 
keyval work goes in.  (It's almost ready for posting BTW, 
https://gitlab.com/bonzini/qemu/-/commit/b59288c86c).

As soon as we have an idea of what we want UserCreatable to look in the 
end, on both the QAPI side and the object implementation side.  That's 
also the part where we have the biggest need to document the schema. 
With that part at least roughly sketched out (no code needed), I'm okay 
with this series going in.

I still don't like the triplication, but as George Michael puts it I 
just gotta have faith---because I must admit, I'm positively surprised 
at the ideas that came out of the discussion.

Paolo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Eduardo Habkost]

On Tue, Dec 01, 2020 at 10:23:57PM +0100, Paolo Bonzini wrote:
> On 01/12/20 20:35, Kevin Wolf wrote:
> > Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben:
> > I don't think this is actually a new things. We already have types and
> > commands declared with things like 'if': 'defined(TARGET_S390X)'.
> > As far as I understand, QAPI generated files are already built per
> > target, so not compiling things into all binaries should be entirely
> > possible.
> 
> There is some complication due to having different discriminators per
> target.  So yes it should be possible.  But probably best left after objects
> because it's so much bigger a task and because objects have a bit more
> freedom for experimentation (less ties to other qdev-specific concepts, e.g.
> the magic "bus" property).
> 
> > So maybe only the abstract base class that actually defines the machine
> > properties (like generic-pc-machine) should be described in QAPI, and
> > then the concrete machine types would inherit from it without being
> > described in QAPI themselves?
> 
> Yes, maybe.
> 
> > > 1) whether to generate _all_ boilerplate or only properties
> > 
> > I would like to generate as much boilerplate as possible. That is, I
> > don't want to constrain us to only properties, but at the same time, I'm
> > not sure if it's possible to get rid of all boilerplate.
> > 
> > Basically, the vision I have in mind is that QAPI would generate code
> > that is the same for most instances, and then provide an option that
> > prevents code generation for a specific part for more complicated cases,
> > so that the respective function can (and must) be provided in the C
> > source.
> 
> Ok, so that's a bit different from what I am thinking of.  I don't care very
> much about the internal boilerplate, only the external interface for
> configuration.  So I don't care about type registration, dynamic cast macros
> etc., only essentially the part that leads to ucc->complete.
> 
> > > 2) whether we want to introduce a separation between configuration
> > > schema and run-time state
> > 
> > You mean the internal run-time state? How is this separation not already
> > present with getter/setter functions for each property? In many cases
> > they just directly access the run-time state, but there are other cases
> > where they actually do things.
> 
> I mean moving more towards the blockdev/chardev way of doing things,
> increasing the separation very much by having separate configuration structs
> that have (potentially) no link to the run-time state struct.
> 
> > > 3) in the latter case, whether properties will survive at all---iothread and
> > > throttle-groups don't really need them even if they're writable after
> > > creation.
> > 
> > How do you define properties, i.e. at which point would they stop
> > existing and what would be a non-property alternative?
> 
> Properties are only a useful concept if they have a use.  If
> -object/object_add/object-add can do the same job without properties,
> properties are not needed anymore.

Do you mean "not needed for -object anymore"?  Properties are
still used by internal C code (esp. board code),
-device/device_add, -machine, -cpu, and debugging commands (like
"info qtree" and qom-list/qom-get/qom-set).

> 
> Right now QOM is all about exposing properties, and having multiple
> interfaces to set them (by picking a different visitor).  But in practice
> most QOM objects have a lifetime that consists of 1) set properties 2) flip
> a switch (realized/complete/open) 3) let the object live on its own.  1+2
> are a single monitor command or CLI option; during 3 you access the object
> through monitor commands, not properties.

I agree with this, except for the word "all" in "QOM is all
about".  QOM is also an extensively used internal QEMU API,
including internal usage of the QOM property system.

> 
> > So in summary, it seems to me that the QOM way is more flexible because
> > you can get both models out of it. Whether we actually need this
> > flexibility I can't say.
> 
> I'm thinking there's no need for it, but maybe I'm overly optimistic.
> 
> > * Configuration options are described in the QAPI schema. This is mainly
> >    for object creation, but runtime modifiable properties are a subset of
> >    this.
> > 
> > * Properties are generated for each option. By default, the getter
> >    just returns the value from the configuration at creation time, though
> >    generation of it can be disabled so that it can be overridden. Also,
> >    setters just return an error by default.
> > 
> > * Property setters aren't called for object creation. Instead, the
> >    relevant ObjectOptions branch is made available to some init method.
> > 
> > * Runtime modifiable properties (declared as such in the schema) don't
> >    get the default setter, so you have to provide an implementation for
> >    them.
> 
> I wouldn't bother with properties at all in the QAPI schema.  Just do the
> first and third bullet.  Declaring read-only QOM properties is trivial.

I'm liking the direction this is taking.  However, I would still
like to have a clearer and feasible plan that would work for
-device, -machine, and -cpu.

> 
> > So while this series is doing only one part of the whole solution, that
> > the second part is missing doesn't make the first part wrong.
> 
> Yeah, I think it's clear that for the long term we're not really disagreeing
> (or perhaps I'm even more radical than you :)).  I'm just worried about
> having yet another incomplete transition.
> 
> > One possibly nasty detail to consider there is that we sometimes declare
> > the USER_CREATABLE interface in the base class, so ucc->complete is for
> > the base class rather than the actually instantiated class. If we only
> > instantiate leaf classes (I think this is true), we can move
> > USER_CREATABLE there.
> 
> You can also use a while loop covering each superclass to decide how to
> dispatch ucc->complete.  I don't care much about these details, they're
> Simple Matter Of Programming. :)
> 
> > I also had in mind just passing the whole configuration struct
> > (essentially always 'boxed': true), but you're right that individual
> > parameters like for commands would be possible. I'm not entirely
> > convinced that they would be better (there was a reason why we
> > introduced 'boxed': true), but it's an option.
> 
> Having 'boxed': true by default would be just an implementation choice,
> nothing to worry about.  (When I said the arguments would be the
> configuration, having a boxed struct as the argument would fit the
> description just as well).
> 
> > I was hoping that by converting object-add in this series, and the CLI
> > options soon afterwards, it would be very obvious if you forget to
> > change the schema because your new property just wouldn't work (at least
> > not during creation).
> 
> Converting the CLI options is not entirely trivial due to -readconfig and
> friends, so I was expecting that to last until that part of my 6.0 keyval
> work goes in.  (It's almost ready for posting BTW,
> https://gitlab.com/bonzini/qemu/-/commit/b59288c86c).
> 
> As soon as we have an idea of what we want UserCreatable to look in the end,
> on both the QAPI side and the object implementation side.  That's also the
> part where we have the biggest need to document the schema. With that part
> at least roughly sketched out (no code needed), I'm okay with this series
> going in.
> 
> I still don't like the triplication, but as George Michael puts it I just
> gotta have faith---because I must admit, I'm positively surprised at the
> ideas that came out of the discussion.
> 
> Paolo
> 

-- 
Eduardo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 01/12/20 23:08, Eduardo Habkost wrote:
>> Properties are only a useful concept if they have a use.  If
>> -object/object_add/object-add can do the same job without properties,
>> properties are not needed anymore.
>
> Do you mean "not needed for -object anymore"?  Properties are
> still used by internal C code (esp. board code),
> -device/device_add, -machine, -cpu, and debugging commands (like
> "info qtree" and qom-list/qom-get/qom-set).

Yes.

>> Right now QOM is all about exposing properties, and having multiple
>> interfaces to set them (by picking a different visitor).  But in practice
>> most QOM objects have a lifetime that consists of 1) set properties 2) flip
>> a switch (realized/complete/open) 3) let the object live on its own.  1+2
>> are a single monitor command or CLI option; during 3 you access the object
>> through monitor commands, not properties.
>
> I agree with this, except for the word "all" in "QOM is all
> about".  QOM is also an extensively used internal QEMU API,
> including internal usage of the QOM property system.

Yeah, "all about exposing properties" includes internal usage.  And 
you're right that some "phase 3" monitor commands do work at the 
property level (mostly "info qtree", but also "qom-get" because there 
are some cases of public run-time properties).

> I'm liking the direction this is taking.  However, I would still
> like to have a clearer and feasible plan that would work for
> -device, -machine, and -cpu.

-cpu is not a problem since it's generally created with a static 
configuration (now done with global properties, in the future it could 
be a struct).

-machine and -device in principle could be done the same way as -object, 
just through a different registry (_not_ a huge struct; that's an 
acceptable stopgap for -object but that's it).  The static aka field 
properties would remain as read-only, with defaults moved to 
instance_init or realize.  But there would be again "triplication" with 
a trivial conversion:

1) in the QAPI schema, e.g. 'num_queues': 'int16'

2) in the struct, "int16_t num_queues;"

3) in the realize function,

     s->num_queues = cfg->has_num_queues ? cfg->num_queues : 8;

So having a mechanism for defaults in the QAPI schema would be good. 
Maybe 'num_queues': { 'type': 'int16', 'default': '8' }?

I also need to review more the part of this code with respect to the 
application of global properties.  I wonder if there are visitor tricks 
that we can do, so that global properties keep working but correspond to 
QAPI fields instead of QOM properties.

Paolo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 02.12.2020 um 10:30 hat Paolo Bonzini geschrieben:
> On 01/12/20 23:08, Eduardo Habkost wrote:
> > > Properties are only a useful concept if they have a use.  If
> > > -object/object_add/object-add can do the same job without properties,
> > > properties are not needed anymore.
> > 
> > Do you mean "not needed for -object anymore"?  Properties are
> > still used by internal C code (esp. board code),
> > -device/device_add, -machine, -cpu, and debugging commands (like
> > "info qtree" and qom-list/qom-get/qom-set).
> 
> Yes.

Are internal uses mostly just right after object creation, or do we make
a lot of use of them during runtime?

> > > Right now QOM is all about exposing properties, and having multiple
> > > interfaces to set them (by picking a different visitor).  But in practice
> > > most QOM objects have a lifetime that consists of 1) set properties 2) flip
> > > a switch (realized/complete/open) 3) let the object live on its own.  1+2
> > > are a single monitor command or CLI option; during 3 you access the object
> > > through monitor commands, not properties.
> > 
> > I agree with this, except for the word "all" in "QOM is all
> > about".  QOM is also an extensively used internal QEMU API,
> > including internal usage of the QOM property system.
> 
> Yeah, "all about exposing properties" includes internal usage.  And you're
> right that some "phase 3" monitor commands do work at the property level
> (mostly "info qtree", but also "qom-get" because there are some cases of
> public run-time properties).
> 
> > I'm liking the direction this is taking.  However, I would still
> > like to have a clearer and feasible plan that would work for
> > -device, -machine, and -cpu.
> 
> -cpu is not a problem since it's generally created with a static
> configuration (now done with global properties, in the future it could be a
> struct).
> 
> -machine and -device in principle could be done the same way as -object,
> just through a different registry (_not_ a huge struct; that's an acceptable
> stopgap for -object but that's it).  The static aka field properties would
> remain as read-only, with defaults moved to instance_init or realize.  But
> there would be again "triplication" with a trivial conversion:
> 
> 1) in the QAPI schema, e.g. 'num_queues': 'int16'
> 
> 2) in the struct, "int16_t num_queues;"

This one is optional, you can use the QAPI type even in the run-time
state. I guess this goes back to how much separation you want between
the configuration and the internal state.

> 3) in the realize function,
> 
>     s->num_queues = cfg->has_num_queues ? cfg->num_queues : 8;
> 
> So having a mechanism for defaults in the QAPI schema would be good. Maybe
> 'num_queues': { 'type': 'int16', 'default': '8' }?

Defaults have been on the QAPI wishlist for a long time, and everyone
agrees that it would be nice to have them. Maybe it's time to finally
implement them.

> I also need to review more the part of this code with respect to the
> application of global properties.  I wonder if there are visitor tricks that
> we can do, so that global properties keep working but correspond to QAPI
> fields instead of QOM properties.

Kevin

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 02/12/20 11:38, Kevin Wolf wrote:
> Am 02.12.2020 um 10:30 hat Paolo Bonzini geschrieben:
>> On 01/12/20 23:08, Eduardo Habkost wrote:
>>>> Properties are only a useful concept if they have a use.  If
>>>> -object/object_add/object-add can do the same job without properties,
>>>> properties are not needed anymore.
>>>
>>> Do you mean "not needed for -object anymore"?  Properties are
>>> still used by internal C code (esp. board code),
>>> -device/device_add, -machine, -cpu, and debugging commands (like
>>> "info qtree" and qom-list/qom-get/qom-set).
>>
>> Yes.
> 
> Are internal uses mostly just right after object creation, or do we make
> a lot of use of them during runtime?

Not "a lot" but enough to be nasty.  There are a couple uses of 
"well-known" QOM properties (e.g. to access the RTC time or the balloon 
stats), plus "info qtree".

Paolo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Eduardo Habkost]

On Wed, Dec 02, 2020 at 10:30:11AM +0100, Paolo Bonzini wrote:
> On 01/12/20 23:08, Eduardo Habkost wrote:
> > > Properties are only a useful concept if they have a use.  If
> > > -object/object_add/object-add can do the same job without properties,
> > > properties are not needed anymore.
> > 
> > Do you mean "not needed for -object anymore"?  Properties are
> > still used by internal C code (esp. board code),
> > -device/device_add, -machine, -cpu, and debugging commands (like
> > "info qtree" and qom-list/qom-get/qom-set).
> 
> Yes.
> 
> > > Right now QOM is all about exposing properties, and having multiple
> > > interfaces to set them (by picking a different visitor).  But in practice
> > > most QOM objects have a lifetime that consists of 1) set properties 2) flip
> > > a switch (realized/complete/open) 3) let the object live on its own.  1+2
> > > are a single monitor command or CLI option; during 3 you access the object
> > > through monitor commands, not properties.
> > 
> > I agree with this, except for the word "all" in "QOM is all
> > about".  QOM is also an extensively used internal QEMU API,
> > including internal usage of the QOM property system.
> 
> Yeah, "all about exposing properties" includes internal usage.  And you're
> right that some "phase 3" monitor commands do work at the property level
> (mostly "info qtree", but also "qom-get" because there are some cases of
> public run-time properties).

I still disagree on the "all about" part even for internal usage.
But this shouldn't really matter for this discussion, I guess.

> 
> > I'm liking the direction this is taking.  However, I would still
> > like to have a clearer and feasible plan that would work for
> > -device, -machine, and -cpu.
> 
> -cpu is not a problem since it's generally created with a static
> configuration (now done with global properties, in the future it could be a
> struct).

It is a problem if it requires manually converting existing code
defining CPU properties and we don't have a transition plan.

> 
> -machine and -device in principle could be done the same way as -object,
> just through a different registry (_not_ a huge struct; that's an acceptable
> stopgap for -object but that's it).  The static aka field properties would
> remain as read-only, with defaults moved to instance_init or realize.  But
> there would be again "triplication" with a trivial conversion:

> 
> 1) in the QAPI schema, e.g. 'num_queues': 'int16'
> 
> 2) in the struct, "int16_t num_queues;"
> 
> 3) in the realize function,
> 
>     s->num_queues = cfg->has_num_queues ? cfg->num_queues : 8;
> 
> So having a mechanism for defaults in the QAPI schema would be good. Maybe
> 'num_queues': { 'type': 'int16', 'default': '8' }?
> 

Would a -device conversion also involve non-user-creatable
devices, or would we keep existing internal usage of QOM
properties?

Even if it's just for user-creatable devices, getting rid of QOM
property usage in devices sounds like a very ambitious goal.  I'd
like us to have a good transition plan, in addition to declaring
what's our ideal end goal.


> I also need to review more the part of this code with respect to the
> application of global properties.  I wonder if there are visitor tricks that
> we can do, so that global properties keep working but correspond to QAPI
> fields instead of QOM properties.
> 
> Paolo
> 

-- 
Eduardo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 02/12/20 13:51, Eduardo Habkost wrote:
>>> I'm liking the direction this is taking.  However, I would still
>>> like to have a clearer and feasible plan that would work for
>>> -device, -machine, and -cpu.
>>
>> -cpu is not a problem since it's generally created with a static
>> configuration (now done with global properties, in the future it could be a
>> struct).
> 
> It is a problem if it requires manually converting existing code
> defining CPU properties and we don't have a transition plan.

We do not have to convert everything _if_ for some objects there are 
good reasons to do programmatically-generated properties.  CPUs might be 
one of those cases (or if we decide to convert them, they might endure 
some more code duplication than other devices because they have so many 
properties).

> Would a -device conversion also involve non-user-creatable
> devices, or would we keep existing internal usage of QOM
> properties?
> 
> Even if it's just for user-creatable devices, getting rid of QOM
> property usage in devices sounds like a very ambitious goal.  I'd
> like us to have a good transition plan, in addition to declaring
> what's our ideal end goal.

For user-creatable objects Kevin is doing work in lockstep on all 
classes; but once we have the infrastructure for QAPI object 
configuration schemas we can proceed in the other direction and operate 
on one device at a time.

With some handwaving, something like (see create_unimplemented_device)

     DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);

     qdev_prop_set_string(dev, "name", name);
     qdev_prop_set_uint64(dev, "size", size);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);

might become something like

     { 'object': 'unimplemented-device',
       'config': {
          'name': 'str',
          'size': 'size'
       },
     }

     DeviceState *dev = qapi_Unimplemented_new(&(
          (UnimplementedDeviceOptions) {
              .name = name,
              .size = size
          }, &error_fatal);
     object_unref(dev);

(i.e. all typesafe!) and the underlying code would be something like

     void (*init_properties)(Object *obj, Visitor *v, Error **errp);
     ...

     // QAPI generated constructor
     UnimplementedDevice *qapi_Unimplemented_new(
         UnimplementedDeviceOptions *opt, Error **errp)
     {
         Object *obj = object_new(TYPE_UNIMPLEMENTED_DEVICE);
         if (!qapi_Unimplemented_init_properties(obj, opt, errp)) {
             object_unref(obj);
             return NULL;
         }
         return obj;
     }

     // QAPI generated Visitor->struct adapter
     bool qapi_Unimplemented_init_properties(
         Object *obj, Visitor *v, Error **errp)
     {
         UnimplementedDeviceOptions opt;
         Error *local_err = NULL;
         visit_type_UnimplementedDeviceOptions(v, NULL,
                                               &opt, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             return false;
         }
         unimplemented_init_properties(UNIMPLEMENTED_DEVICE(obj),
                                       &opt, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             return false;
         }
         return true;
     }

     // Hand-written (?) initializer:
     void unimplemented_init_properties(Object *obj,
          UnimplementedDeviceOptions *opt, Error **errp)
     {
          obj->name = name;
          obj->size = size;
          device_realize(obj, errp);
     }

     // class_init code:
     oc->init_properties = qapi_Unimplemented_init_properties;

and all read-only-after-realize QOM properties could be made *really* 
read-only.

Paolo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Eduardo Habkost]

On Wed, Dec 02, 2020 at 02:26:44PM +0100, Paolo Bonzini wrote:
> On 02/12/20 13:51, Eduardo Habkost wrote:
> > > > I'm liking the direction this is taking.  However, I would still
> > > > like to have a clearer and feasible plan that would work for
> > > > -device, -machine, and -cpu.
> > > 
> > > -cpu is not a problem since it's generally created with a static
> > > configuration (now done with global properties, in the future it could be a
> > > struct).
> > 
> > It is a problem if it requires manually converting existing code
> > defining CPU properties and we don't have a transition plan.
> 
> We do not have to convert everything _if_ for some objects there are good
> reasons to do programmatically-generated properties.  CPUs might be one of
> those cases (or if we decide to convert them, they might endure some more
> code duplication than other devices because they have so many properties).

OK, we just need to agree on what the transition will look like
when we do it.  I think we should put as much care into
transition/glue infrastructure as we put into the new
infrastructure.


> 
> > Would a -device conversion also involve non-user-creatable
> > devices, or would we keep existing internal usage of QOM
> > properties?
> > 
> > Even if it's just for user-creatable devices, getting rid of QOM
> > property usage in devices sounds like a very ambitious goal.  I'd
> > like us to have a good transition plan, in addition to declaring
> > what's our ideal end goal.
> 
> For user-creatable objects Kevin is doing work in lockstep on all classes;
> but once we have the infrastructure for QAPI object configuration schemas we
> can proceed in the other direction and operate on one device at a time.
> 
> With some handwaving, something like (see create_unimplemented_device)
> 
>     DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
> 
>     qdev_prop_set_string(dev, "name", name);
>     qdev_prop_set_uint64(dev, "size", size);
>     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> 
> might become something like
> 
>     { 'object': 'unimplemented-device',
>       'config': {
>          'name': 'str',
>          'size': 'size'
>       },
>     }
> 
>     DeviceState *dev = qapi_Unimplemented_new(&(
>          (UnimplementedDeviceOptions) {
>              .name = name,
>              .size = size
>          }, &error_fatal);
>     object_unref(dev);
> 
> (i.e. all typesafe!) and the underlying code would be something like
[...]
> 

Looks nice as end goal.  Then, these are a few questions I would
have about the transition plan:

Would it require changing both device implementation and device
users in lockstep?  Should we have a compatibility layer to allow
existing qdev_new()+qdev_prop_set_*() code to keep working after
the devices are converted to the new system?  If not, why not?

If we add a compatibility layer, is the end goal to convert all
existing qdev_new() users to the new system?  If yes, why?  If
not, why not?

What about subclasses?  Would base classes need to be converted
in lockstep with all subclasses?  How would the transition
process of (e.g.) PCI devices look like?

-- 
Eduardo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 02.12.2020 um 14:54 hat Eduardo Habkost geschrieben:
> On Wed, Dec 02, 2020 at 02:26:44PM +0100, Paolo Bonzini wrote:
> > On 02/12/20 13:51, Eduardo Habkost wrote:
> > > > > I'm liking the direction this is taking.  However, I would still
> > > > > like to have a clearer and feasible plan that would work for
> > > > > -device, -machine, and -cpu.
> > > > 
> > > > -cpu is not a problem since it's generally created with a static
> > > > configuration (now done with global properties, in the future it could be a
> > > > struct).
> > > 
> > > It is a problem if it requires manually converting existing code
> > > defining CPU properties and we don't have a transition plan.
> > 
> > We do not have to convert everything _if_ for some objects there are good
> > reasons to do programmatically-generated properties.  CPUs might be one of
> > those cases (or if we decide to convert them, they might endure some more
> > code duplication than other devices because they have so many properties).
> 
> OK, we just need to agree on what the transition will look like
> when we do it.  I think we should put as much care into
> transition/glue infrastructure as we put into the new
> infrastructure.
> 
> 
> > 
> > > Would a -device conversion also involve non-user-creatable
> > > devices, or would we keep existing internal usage of QOM
> > > properties?
> > > 
> > > Even if it's just for user-creatable devices, getting rid of QOM
> > > property usage in devices sounds like a very ambitious goal.  I'd
> > > like us to have a good transition plan, in addition to declaring
> > > what's our ideal end goal.
> > 
> > For user-creatable objects Kevin is doing work in lockstep on all classes;
> > but once we have the infrastructure for QAPI object configuration schemas we
> > can proceed in the other direction and operate on one device at a time.
> > 
> > With some handwaving, something like (see create_unimplemented_device)
> > 
> >     DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
> > 
> >     qdev_prop_set_string(dev, "name", name);
> >     qdev_prop_set_uint64(dev, "size", size);
> >     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > 
> > might become something like
> > 
> >     { 'object': 'unimplemented-device',
> >       'config': {
> >          'name': 'str',
> >          'size': 'size'
> >       },
> >     }
> > 
> >     DeviceState *dev = qapi_Unimplemented_new(&(
> >          (UnimplementedDeviceOptions) {
> >              .name = name,
> >              .size = size
> >          }, &error_fatal);
> >     object_unref(dev);
> > 
> > (i.e. all typesafe!) and the underlying code would be something like
> [...]
> > 
> 
> Looks nice as end goal.  Then, these are a few questions I would
> have about the transition plan:
> 
> Would it require changing both device implementation and device
> users in lockstep?  Should we have a compatibility layer to allow
> existing qdev_new()+qdev_prop_set_*() code to keep working after
> the devices are converted to the new system?  If not, why not?

Technically, it doesn't strictly require a lockstep update. You can have
two code paths leading to a fully initialised device, one being the
traditional way of setting properties and finally calling dc->realize,
the other being a new method that takes the configuration in its
parameters and also sets dev->realized = true at the end.

If at all possible, I would however prefer a lockstep update because
having two paths is a weird intermediate state and the code paths could
easily diverge. Keeping the old way around for a device also means that
property setters are still doing two different jobs (initial
configuration and updates at runtime).

> If we add a compatibility layer, is the end goal to convert all
> existing qdev_new() users to the new system?  If yes, why?  If
> not, why not?

My personal goal is covering -object and -device, i.e. the external
interfaces. Converting purely internally created devices is not as
interesting (especially as long as we focus only on object creation),
but might be desirable for consistency.

> What about subclasses?  Would base classes need to be converted
> in lockstep with all subclasses?  How would the transition
> process of (e.g.) PCI devices look like?

I don't think so.

If you want to convert base classes first, you may need to take the
path shown above where both initialisation paths coexist while the
children are converted because instantiation of a child class involves
setting properties of the base class. So you can only restrict these
properties to runtime-only after all children have been converted.

The other way around might be easier: You will need to describe the
properties of base classes in the QAPI schema from the beginning, but
that doesn't mean that their initialisation code has to change just yet.
The child classes will need to forward the part of their configuration
that belongs to the base class. The downside is that this code will have
to be changed again when the base class is finally converted.

So we have options there, and we can decide case by case which one is
most appropriate for the specific class to be converted (depending on
how many child classes exist, how many properties are inherited, etc.)

Kevin

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Eduardo Habkost]

On Wed, Dec 02, 2020 at 04:17:13PM +0100, Kevin Wolf wrote:
> Am 02.12.2020 um 14:54 hat Eduardo Habkost geschrieben:
> > On Wed, Dec 02, 2020 at 02:26:44PM +0100, Paolo Bonzini wrote:
> > > On 02/12/20 13:51, Eduardo Habkost wrote:
> > > > > > I'm liking the direction this is taking.  However, I would still
> > > > > > like to have a clearer and feasible plan that would work for
> > > > > > -device, -machine, and -cpu.
> > > > > 
> > > > > -cpu is not a problem since it's generally created with a static
> > > > > configuration (now done with global properties, in the future it could be a
> > > > > struct).
> > > > 
> > > > It is a problem if it requires manually converting existing code
> > > > defining CPU properties and we don't have a transition plan.
> > > 
> > > We do not have to convert everything _if_ for some objects there are good
> > > reasons to do programmatically-generated properties.  CPUs might be one of
> > > those cases (or if we decide to convert them, they might endure some more
> > > code duplication than other devices because they have so many properties).
> > 
> > OK, we just need to agree on what the transition will look like
> > when we do it.  I think we should put as much care into
> > transition/glue infrastructure as we put into the new
> > infrastructure.
> > 
> > 
> > > 
> > > > Would a -device conversion also involve non-user-creatable
> > > > devices, or would we keep existing internal usage of QOM
> > > > properties?
> > > > 
> > > > Even if it's just for user-creatable devices, getting rid of QOM
> > > > property usage in devices sounds like a very ambitious goal.  I'd
> > > > like us to have a good transition plan, in addition to declaring
> > > > what's our ideal end goal.
> > > 
> > > For user-creatable objects Kevin is doing work in lockstep on all classes;
> > > but once we have the infrastructure for QAPI object configuration schemas we
> > > can proceed in the other direction and operate on one device at a time.
> > > 
> > > With some handwaving, something like (see create_unimplemented_device)
> > > 
> > >     DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
> > > 
> > >     qdev_prop_set_string(dev, "name", name);
> > >     qdev_prop_set_uint64(dev, "size", size);
> > >     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > > 
> > > might become something like
> > > 
> > >     { 'object': 'unimplemented-device',
> > >       'config': {
> > >          'name': 'str',
> > >          'size': 'size'
> > >       },
> > >     }
> > > 
> > >     DeviceState *dev = qapi_Unimplemented_new(&(
> > >          (UnimplementedDeviceOptions) {
> > >              .name = name,
> > >              .size = size
> > >          }, &error_fatal);
> > >     object_unref(dev);
> > > 
> > > (i.e. all typesafe!) and the underlying code would be something like
> > [...]
> > > 
> > 
> > Looks nice as end goal.  Then, these are a few questions I would
> > have about the transition plan:
> > 
> > Would it require changing both device implementation and device
> > users in lockstep?  Should we have a compatibility layer to allow
> > existing qdev_new()+qdev_prop_set_*() code to keep working after
> > the devices are converted to the new system?  If not, why not?
> 
> Technically, it doesn't strictly require a lockstep update. You can have
> two code paths leading to a fully initialised device, one being the
> traditional way of setting properties and finally calling dc->realize,
> the other being a new method that takes the configuration in its
> parameters and also sets dev->realized = true at the end.
> 
> If at all possible, I would however prefer a lockstep update because
> having two paths is a weird intermediate state and the code paths could
> easily diverge. Keeping the old way around for a device also means that
> property setters are still doing two different jobs (initial
> configuration and updates at runtime).

I'd like to understand better how that intermediate state would
look like and why there's risk of separate code paths diverging.

Could we have an intermediate state that doesn't require any
duplication and thus have no separate code paths that could
diverge?


> 
> > If we add a compatibility layer, is the end goal to convert all
> > existing qdev_new() users to the new system?  If yes, why?  If
> > not, why not?
> 
> My personal goal is covering -object and -device, i.e. the external
> interfaces. Converting purely internally created devices is not as
> interesting (especially as long as we focus only on object creation),
> but might be desirable for consistency.

I wonder how much consistency we will lose and how much confusion
we'll cause if we end up with two completely separate methods for
creating devices.

> 
> > What about subclasses?  Would base classes need to be converted
> > in lockstep with all subclasses?  How would the transition
> > process of (e.g.) PCI devices look like?
> 
> I don't think so.
> 
> If you want to convert base classes first, you may need to take the
> path shown above where both initialisation paths coexist while the
> children are converted because instantiation of a child class involves
> setting properties of the base class. So you can only restrict these
> properties to runtime-only after all children have been converted.
> 
> The other way around might be easier: You will need to describe the
> properties of base classes in the QAPI schema from the beginning, but
> that doesn't mean that their initialisation code has to change just yet.
> The child classes will need to forward the part of their configuration
> that belongs to the base class. The downside is that this code will have
> to be changed again when the base class is finally converted.
> 
> So we have options there, and we can decide case by case which one is
> most appropriate for the specific class to be converted (depending on
> how many child classes exist, how many properties are inherited, etc.)

Right now it's hard for me to understand what those intermediate
states would look like.  It sounds like it requires too many
complicated manual changes to be done by humans, and lots of room
for mistakes when maintaining two parallel code paths.  I'd
prefer to delegate the translation job to a machine.

In other words, I'd prefer to have compatibility layer(s) that
would make the same implementation work with the both old-style
and new-style APIs for creating devices.  Maybe this would
involve generating QAPI code from QOM/qdev property lists, and/or
generating qdev property registration code from the QAPI schema,
I don't know.

Providing a compatibility layer would also help us be more
confident that there are no gaps in the abstractions provided by
the two systems (QOM properties and QAPI schema) that we still
need to address.

-- 
Eduardo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 02.12.2020 um 17:05 hat Eduardo Habkost geschrieben:
> > > Looks nice as end goal.  Then, these are a few questions I would
> > > have about the transition plan:
> > > 
> > > Would it require changing both device implementation and device
> > > users in lockstep?  Should we have a compatibility layer to allow
> > > existing qdev_new()+qdev_prop_set_*() code to keep working after
> > > the devices are converted to the new system?  If not, why not?
> > 
> > Technically, it doesn't strictly require a lockstep update. You can have
> > two code paths leading to a fully initialised device, one being the
> > traditional way of setting properties and finally calling dc->realize,
> > the other being a new method that takes the configuration in its
> > parameters and also sets dev->realized = true at the end.
> > 
> > If at all possible, I would however prefer a lockstep update because
> > having two paths is a weird intermediate state and the code paths could
> > easily diverge. Keeping the old way around for a device also means that
> > property setters are still doing two different jobs (initial
> > configuration and updates at runtime).
> 
> I'd like to understand better how that intermediate state would
> look like and why there's risk of separate code paths diverging.
>
> Could we have an intermediate state that doesn't require any
> duplication and thus have no separate code paths that could
> diverge?

The one requirement we have for an intermediate state is that it
supports both interfaces: The well-know create/set properties/realize
dance, and a new DeviceClass method, say .create(), that takes the
configuration in parameters instead of relying on previously set
properties.

I assumed two separate implementations of transferring the configuration
into the internal state. On second thought, this assumption is maybe
wrong.

You can implement the new method as wrapper around the old way: It could
just set all the properties and call realize. Of course, you don't win
much in terms of improving the class implementation this way, but just
support the new interface, but I guess it can be a reasonable
intermediate step to resolve complicated dependencies etc.

It would be much nicer to do the wrapper the other way round, i.e.
setting properties before the device is realized would update a
configuration struct and realize would then call .create() with that
struct. To me, this sounds much harder, though also a more useful state.

As you have worked a lot with properties recently, maybe you have a good
idea how we could get an intermediate state closer to this?

> > > If we add a compatibility layer, is the end goal to convert all
> > > existing qdev_new() users to the new system?  If yes, why?  If
> > > not, why not?
> > 
> > My personal goal is covering -object and -device, i.e. the external
> > interfaces. Converting purely internally created devices is not as
> > interesting (especially as long as we focus only on object creation),
> > but might be desirable for consistency.
> 
> I wonder how much consistency we will lose and how much confusion
> we'll cause if we end up with two completely separate methods for
> creating devices.

I do think we should follow through and convert everything. It's just
not my main motivation, and if the people who work more with qdev think
it's better to leave that part unchanged (or that it won't make much of
a difference), I won't insist.

> > > What about subclasses?  Would base classes need to be converted
> > > in lockstep with all subclasses?  How would the transition
> > > process of (e.g.) PCI devices look like?
> > 
> > I don't think so.
> > 
> > If you want to convert base classes first, you may need to take the
> > path shown above where both initialisation paths coexist while the
> > children are converted because instantiation of a child class involves
> > setting properties of the base class. So you can only restrict these
> > properties to runtime-only after all children have been converted.
> > 
> > The other way around might be easier: You will need to describe the
> > properties of base classes in the QAPI schema from the beginning, but
> > that doesn't mean that their initialisation code has to change just yet.
> > The child classes will need to forward the part of their configuration
> > that belongs to the base class. The downside is that this code will have
> > to be changed again when the base class is finally converted.
> > 
> > So we have options there, and we can decide case by case which one is
> > most appropriate for the specific class to be converted (depending on
> > how many child classes exist, how many properties are inherited, etc.)
> 
> Right now it's hard for me to understand what those intermediate
> states would look like.  It sounds like it requires too many
> complicated manual changes to be done by humans, and lots of room
> for mistakes when maintaining two parallel code paths.  I'd
> prefer to delegate the translation job to a machine.

Maybe devices are in a better shape, but my conclusion from user
creatable objects is that it needs to be done by a human.

Even just writing a schema for an existing object without actually
changing its code (i.e. what this series does for user creatable
objects) involves:

* Figuring out which properties even exist.

  I guess if you know your way around QOM, this could be automatically
  be generated for the common case. If property definitions are
  conditional or dynamic, you'll probably miss them.

* Finding the right data type for each property.

  The const char *type passed to object_(class_)property_add() is often
  enough wrong that it becomes useless. If object_property_add_<type> is
  used, chances are that you know the right type, but strings might
  actually be hidden enums. If not, figuring out the type involves
  analysing the setter and getter callbacks.

* Finding out which properties are mandatory and which are optional.

  This is usually some handwritten check in complete/realize that
  returns an error. Sometimes it's just a segfault that happens sooner
  or later if the property hasn't been set.

* Finding the default for documentation.

  There are multiple ways to do this. It's code, not data.

* Writing (preferably good) documentation for the property.

I see very little opportunity for automating a significant part of this.

Once you have this information, going to the intermediate state where
.create() is just a wrapper that sets properties and calls realize is
fairly easy. Maybe we can have QAPI support for this so that you can
request generation of the wrapper function in the schema. Then you would
just have to set the pointer to it in .class_init.

> In other words, I'd prefer to have compatibility layer(s) that
> would make the same implementation work with the both old-style
> and new-style APIs for creating devices.  Maybe this would
> involve generating QAPI code from QOM/qdev property lists, and/or
> generating qdev property registration code from the QAPI schema,
> I don't know.
> 
> Providing a compatibility layer would also help us be more
> confident that there are no gaps in the abstractions provided by
> the two systems (QOM properties and QAPI schema) that we still
> need to address.

qdev properties could be more useful to generate at least a skeleton
schema from than generic QOM properties. But there will still be large
parts that a human needs to do.

My concern with the compatibility layer is just that it could happen
more easily that we're stuck with it forever.

Kevin

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Eduardo Habkost]

On Wed, Dec 02, 2020 at 06:35:06PM +0100, Kevin Wolf wrote:
> Am 02.12.2020 um 17:05 hat Eduardo Habkost geschrieben:
> > > > Looks nice as end goal.  Then, these are a few questions I would
> > > > have about the transition plan:
> > > > 
> > > > Would it require changing both device implementation and device
> > > > users in lockstep?  Should we have a compatibility layer to allow
> > > > existing qdev_new()+qdev_prop_set_*() code to keep working after
> > > > the devices are converted to the new system?  If not, why not?
> > > 
> > > Technically, it doesn't strictly require a lockstep update. You can have
> > > two code paths leading to a fully initialised device, one being the
> > > traditional way of setting properties and finally calling dc->realize,
> > > the other being a new method that takes the configuration in its
> > > parameters and also sets dev->realized = true at the end.
> > > 
> > > If at all possible, I would however prefer a lockstep update because
> > > having two paths is a weird intermediate state and the code paths could
> > > easily diverge. Keeping the old way around for a device also means that
> > > property setters are still doing two different jobs (initial
> > > configuration and updates at runtime).
> > 
> > I'd like to understand better how that intermediate state would
> > look like and why there's risk of separate code paths diverging.
> >
> > Could we have an intermediate state that doesn't require any
> > duplication and thus have no separate code paths that could
> > diverge?
> 
> The one requirement we have for an intermediate state is that it
> supports both interfaces: The well-know create/set properties/realize
> dance, and a new DeviceClass method, say .create(), that takes the
> configuration in parameters instead of relying on previously set
> properties.

I agree completely.

> 
> I assumed two separate implementations of transferring the configuration
> into the internal state. On second thought, this assumption is maybe
> wrong.
> 
> You can implement the new method as wrapper around the old way: It could
> just set all the properties and call realize. Of course, you don't win
> much in terms of improving the class implementation this way, but just
> support the new interface, but I guess it can be a reasonable
> intermediate step to resolve complicated dependencies etc.
> 
> It would be much nicer to do the wrapper the other way round, i.e.
> setting properties before the device is realized would update a
> configuration struct and realize would then call .create() with that
> struct. To me, this sounds much harder, though also a more useful state.

Comment about this below (look for [1]).

> 
> As you have worked a lot with properties recently, maybe you have a good
> idea how we could get an intermediate state closer to this?

I'd have to re-read this whole thread and think about it.

> 
> > > > If we add a compatibility layer, is the end goal to convert all
> > > > existing qdev_new() users to the new system?  If yes, why?  If
> > > > not, why not?
> > > 
> > > My personal goal is covering -object and -device, i.e. the external
> > > interfaces. Converting purely internally created devices is not as
> > > interesting (especially as long as we focus only on object creation),
> > > but might be desirable for consistency.
> > 
> > I wonder how much consistency we will lose and how much confusion
> > we'll cause if we end up with two completely separate methods for
> > creating devices.
> 
> I do think we should follow through and convert everything. It's just
> not my main motivation, and if the people who work more with qdev think
> it's better to leave that part unchanged (or that it won't make much of
> a difference), I won't insist.

This worries me.  Converting thousands of lines of code that
don't involve user-visible interfaces seems complicated and maybe
pointless.  On the other hand, having two separate APIs for
creating objects internally would cause confusion.

Maybe we should accept the fact that the 2 APIs will exist, and
address the confusion part: we should guarantee the two APIs to
be 100% equivalent, except for the fact that the newer one gives
us type safety in the C code.

I'd like to avoid a case like qdev vs QOM APIs, where they have
similar but slightly different features, and nobody knows which
one to use.

> 
> > > > What about subclasses?  Would base classes need to be converted
> > > > in lockstep with all subclasses?  How would the transition
> > > > process of (e.g.) PCI devices look like?
> > > 
> > > I don't think so.
> > > 
> > > If you want to convert base classes first, you may need to take the
> > > path shown above where both initialisation paths coexist while the
> > > children are converted because instantiation of a child class involves
> > > setting properties of the base class. So you can only restrict these
> > > properties to runtime-only after all children have been converted.
> > > 
> > > The other way around might be easier: You will need to describe the
> > > properties of base classes in the QAPI schema from the beginning, but
> > > that doesn't mean that their initialisation code has to change just yet.
> > > The child classes will need to forward the part of their configuration
> > > that belongs to the base class. The downside is that this code will have
> > > to be changed again when the base class is finally converted.
> > > 
> > > So we have options there, and we can decide case by case which one is
> > > most appropriate for the specific class to be converted (depending on
> > > how many child classes exist, how many properties are inherited, etc.)
> > 
> > Right now it's hard for me to understand what those intermediate
> > states would look like.  It sounds like it requires too many
> > complicated manual changes to be done by humans, and lots of room
> > for mistakes when maintaining two parallel code paths.  I'd
> > prefer to delegate the translation job to a machine.
> 
> Maybe devices are in a better shape, but my conclusion from user
> creatable objects is that it needs to be done by a human.

I agree with the "done by a human part", but I prefer a different
approach: making the APIs converge (using human work), and then
making an automated transition.

More on this below:

> 
> Even just writing a schema for an existing object without actually
> changing its code (i.e. what this series does for user creatable
> objects) involves:
> 
> * Figuring out which properties even exist.
> 
>   I guess if you know your way around QOM, this could be automatically
>   be generated for the common case. If property definitions are
>   conditional or dynamic, you'll probably miss them.

With qdev static properties and class-level properties, this
stage is easy.

> 
> * Finding the right data type for each property.
> 
>   The const char *type passed to object_(class_)property_add() is often
>   enough wrong that it becomes useless. If object_property_add_<type> is
>   used, chances are that you know the right type, but strings might
>   actually be hidden enums. If not, figuring out the type involves
>   analysing the setter and getter callbacks.

With object_property_add_*() and object_class_property_add_*(),
this part is very difficult.

With qdev static properties (now known as "field properties" in
QOM), this stage is easy.

Luckily, most devices use qdev static properties.


> 
> * Finding out which properties are mandatory and which are optional.
> 
>   This is usually some handwritten check in complete/realize that
>   returns an error. Sometimes it's just a segfault that happens sooner
>   or later if the property hasn't been set.

This stage is tricky.  But we can start by:
- Making all of them optional by default (which is true);
- Adding a flag to allow us to declare properties as mandatory.

> 
> * Finding the default for documentation.
> 
>   There are multiple ways to do this. It's code, not data.

In this case, qdev static properties / field properties make that
job possible.

> 
> * Writing (preferably good) documentation for the property.

This part will require manual work.

> 
> I see very little opportunity for automating a significant part of this.

I see a big opportunity to automate this if the device is already
using the field property system (which is true for most of the
devices).

The remaining work that needs to be done by humans is converting
existing dynamic properties to field properties.


About doing the compatibility layer the other way around[1]: if
conversion from qdev properties to QAPI schema is automated, we
can simply choose a point in time where the results of the
conversion would be saved in the git tree, and the original
property list would be deleted from the C source.  Then, the
compatibility layer would work the other way around.


> 
> Once you have this information, going to the intermediate state where
> .create() is just a wrapper that sets properties and calls realize is
> fairly easy. Maybe we can have QAPI support for this so that you can
> request generation of the wrapper function in the schema. Then you would
> just have to set the pointer to it in .class_init.

Agreed.

> 
> > In other words, I'd prefer to have compatibility layer(s) that
> > would make the same implementation work with the both old-style
> > and new-style APIs for creating devices.  Maybe this would
> > involve generating QAPI code from QOM/qdev property lists, and/or
> > generating qdev property registration code from the QAPI schema,
> > I don't know.
> > 
> > Providing a compatibility layer would also help us be more
> > confident that there are no gaps in the abstractions provided by
> > the two systems (QOM properties and QAPI schema) that we still
> > need to address.
> 
> qdev properties could be more useful to generate at least a skeleton
> schema from than generic QOM properties. But there will still be large
> parts that a human needs to do.

I agree there's lot of manual work to do.  My proposal is doing
the manual work during in the conversion of dynamic properties to
field properties.  Conversion from field properties to the new
API then would be automated.

> 
> My concern with the compatibility layer is just that it could happen
> more easily that we're stuck with it forever.

I hear you.

I'm a pessimistic regarding this.  I believe we _will_ be stuck
in the transition forever (or for so long that it will feel like
forever).  But with a good transition/compatibility layer, that
transition stage will be less painful.

I also believe that writing compatibility layer(s) would be less
work than writing and reviewing patches doing manual conversion
of device code directly to the new API.

-- 
Eduardo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Gerd Hoffmann]

  Hi,

> It would be much nicer to do the wrapper the other way round, i.e.
> setting properties before the device is realized would update a
> configuration struct and realize would then call .create() with that
> struct. To me, this sounds much harder, though also a more useful state.

Well, in some places we already have separate config structs.  We have
NICConf for example, which is typically used like this:

	struct USBNetState {
	   USBDevice dev;
	   [ ... ]
	   NICConf conf;
	   [ ... ]
	};

and

	static Property net_properties[] = {
	    DEFINE_NIC_PROPERTIES(USBNetState, conf),
	    DEFINE_PROP_END_OF_LIST(),
	};

So I think we could:

  (1) move *all* properties into structs.
  (2) generate those structs from qapi schemas.
  (3) generate Property lists (or functions with
      object_class_property_add_*() calls) from qapi
      schema.

We could then convert devices one-by-one without breaking anything
or needing two code paths essentially doing the same thing in two
different ways.

take care,
  Gerd

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Eduardo Habkost]

On Thu, Dec 03, 2020 at 07:46:29AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > It would be much nicer to do the wrapper the other way round, i.e.
> > setting properties before the device is realized would update a
> > configuration struct and realize would then call .create() with that
> > struct. To me, this sounds much harder, though also a more useful state.
> 
> Well, in some places we already have separate config structs.  We have
> NICConf for example, which is typically used like this:
> 
> 	struct USBNetState {
> 	   USBDevice dev;
> 	   [ ... ]
> 	   NICConf conf;
> 	   [ ... ]
> 	};
> 
> and
> 
> 	static Property net_properties[] = {
> 	    DEFINE_NIC_PROPERTIES(USBNetState, conf),
> 	    DEFINE_PROP_END_OF_LIST(),
> 	};
> 
> So I think we could:
> 
>   (1) move *all* properties into structs.
>   (2) generate those structs from qapi schemas.
>   (3) generate Property lists (or functions with
>       object_class_property_add_*() calls) from qapi
>       schema.
> 
> We could then convert devices one-by-one without breaking anything
> or needing two code paths essentially doing the same thing in two
> different ways.

Sounds great to me.

This can also work the other way around for devices that weren't
converted yet: we should be able to generate a QAPI schema from
the property lists.

-- 
Eduardo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 02/12/20 18:35, Kevin Wolf wrote:
>> Could we have an intermediate state that doesn't require any
>> duplication and thus have no separate code paths that could
>> diverge?
> 
> The one requirement we have for an intermediate state is that it
> supports both interfaces: The well-know create/set properties/realize
> dance, and a new DeviceClass method, say .create(), that takes the
> configuration in parameters instead of relying on previously set
> properties.
> 
> I assumed two separate implementations of transferring the configuration
> into the internal state. On second thought, this assumption is maybe
> wrong.
> 
> You can implement the new method as wrapper around the old way: It could
> just set all the properties and call realize. Of course, you don't win
> much in terms of improving the class implementation this way, but just
> support the new interface, but I guess it can be a reasonable
> intermediate step to resolve complicated dependencies etc.

I sketched something at https://wiki.qemu.org/Features/QOM-QAPI_integration.

The main difference with what we discussed so far is that it doesn't 
subsume the complete/realize step, only the setting of properties.  The 
idea is that user_creatable_add_type does the following:

- calls an oc->configure method on every superclass of the object

- takes what's left of the input visitor and uses it to set properties

- then calls ucc->complete

So in the end the only new step is the first.  If the first two steps 
are bundled in a new function object_configure, they can be reused for 
devices, machines and accelerators.

The QAPI code generator can also easily wrap them into a C API for QOM 
object creation.

I glossed completely over the generation of properties within the QAPI 
code generator.  Making properties read-only (or, in the 
field-properties world, having a default allow_set of "return false") is 
already a nice improvement over

> It would be much nicer to do the wrapper the other way round, i.e.
> setting properties before the device is realized would update a
> configuration struct and realize would then call .create() with that
> struct. To me, this sounds much harder, though also a more useful state.

This sounds much harder.  However, based on the RngEgd sample, going 
from a basic QAPI struct to a full conversion is not too hard and makes 
code shorter.  So it's win-win even though it's human work.

Paolo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 03.12.2020 um 12:11 hat Paolo Bonzini geschrieben:
> On 02/12/20 18:35, Kevin Wolf wrote:
> > > Could we have an intermediate state that doesn't require any
> > > duplication and thus have no separate code paths that could
> > > diverge?
> > 
> > The one requirement we have for an intermediate state is that it
> > supports both interfaces: The well-know create/set properties/realize
> > dance, and a new DeviceClass method, say .create(), that takes the
> > configuration in parameters instead of relying on previously set
> > properties.
> > 
> > I assumed two separate implementations of transferring the configuration
> > into the internal state. On second thought, this assumption is maybe
> > wrong.
> > 
> > You can implement the new method as wrapper around the old way: It could
> > just set all the properties and call realize. Of course, you don't win
> > much in terms of improving the class implementation this way, but just
> > support the new interface, but I guess it can be a reasonable
> > intermediate step to resolve complicated dependencies etc.
> 
> I sketched something at https://wiki.qemu.org/Features/QOM-QAPI_integration.
> 
> The main difference with what we discussed so far is that it doesn't subsume
> the complete/realize step, only the setting of properties.  The idea is that
> user_creatable_add_type does the following:
> 
> - calls an oc->configure method on every superclass of the object
> 
> - takes what's left of the input visitor and uses it to set properties
> 
> - then calls ucc->complete
> 
> So in the end the only new step is the first.  If the first two steps are
> bundled in a new function object_configure, they can be reused for devices,
> machines and accelerators.
> 
> The QAPI code generator can also easily wrap them into a C API for QOM
> object creation.
> 
> I glossed completely over the generation of properties within the QAPI code
> generator.  Making properties read-only (or, in the field-properties world,
> having a default allow_set of "return false") is already a nice improvement
> over

I don't think this is an intermediate state like Eduardo wants to have.
Creating the object, then setting properties, then realize [1] will fail
after your change. But keeping it working was the whole point of the
exercise.

I'm also not really sure why you go from RngEgdOptions to QObject to a
visitor, only to reconstruct RngEgdOptions at the end. I think the class
implementations should have a normal C interface without visitors and we
should be able to just pass the existing RngEgdOptions object (or the
individual values for its fields for 'boxed': false).

Kevin

[1] Or complete for ucc, but the number of these is small enough that we
    don't really need any intermediate state, except maybe as a proof of
    concept for the later qdev conversion.

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 03/12/20 16:15, Kevin Wolf wrote:
> I don't think this is an intermediate state like Eduardo wants to have.
> Creating the object, then setting properties, then realize [1] will fail
> after your change. But keeping it working was the whole point of the
> exercise.

With the sample code, you must remove object_class_property_set calls at 
the same time as you remove the setters.  Usually that'd be when you 
convert to QAPI and oc->configure, but it doesn't have to be that way if 
there are good reasons not to do so.

Also, it still allows you to do so one class at a time, and I *think* 
the presence of subclasses or superclasses doesn't matter (only whether 
properties are still writable).  We can use chardevs (see ChardevCommon 
in qapi/char.json) to validate that before tackling devices.

(In fact, this means that your series---plus -object and object_add 
conversion---would be good, pretty much unchanged, as a first step.  The 
second would be adding oc->configure and object_configure, and 
converting all user-creatable objects to oc->configure.  The third would 
involve QAPI code generation).

> I'm also not really sure why you go from RngEgdOptions to QObject to a
> visitor, only to reconstruct RngEgdOptions at the end.

The two visits are just because you cannot create an input visitor 
directly on C data. I stole that from your patch 18/18 actually, just 
with object_new+object_configure instead of user_creatable_add_type.

But I wouldn't read too much in the automatically-generated *_new 
functions since they are already in QAPI code generator territory. 
Instead the basic object_configure idea can be applied even without 
having automatic code generation.

> I think the class
> implementations should have a normal C interface without visitors and we
> should be able to just pass the existing RngEgdOptions object (or the
> individual values for its fields for 'boxed': false).

Sure, however that requires changes to the QAPI code generator which was 
only item (3) in your list list.  Until then you can already work with a 
visitor interface:

   void rng_egd_configure(Object *obj, Visitor *v, Error **errp)
   {
       RngEgd *s = RNG_EGD(obj);
       s->config = g_new0(MemoryBackendOptions, 1);
       visit_type_MemoryBackendOptions(v, NULL, &s->config, errp);

       s->config->share = (s->config->has_share
                           ? s->config->share : false);
       ...
   }

but if you had a QAPI description

   { 'object': 'RngEgd',
     'qom-type': 'rng-egd',
     'configuration': 'RngEgdOptions',
     'boxed': true
   }

the QAPI generator could produce the oc->configure implementation. 
Similar to commands, that implementation would be an unmarshaling 
wrapper that calls out to the natural C interface:

   void qapi_RngEgd_configure(Object *obj, Visitor *v, Error **errp);
   {
       Error *local_err = NULL;
       g_autoptr(MemoryBackendOptions) *config =
           g_new0(MemoryBackendOptions, 1);
       visit_type_MemoryBackendOptions(v, NULL, &s->config, &local_err);
       if (local_err) {
           error_propagate(errp, local_err);
           return;
       }
       qom_rng_egd_configure(RNG_EGD(obj), config, errp);
   }

   void qom_rng_egd_configure(RngEng *s,
                              RngEgdOptions *config,
                              Error **errp)
   {
       config->share = (config->has_share
                        ? config->share : false);
       ...
       s->config = QAPI_CLONE(RngEgdOptions, config);
   }

Paolo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 03.12.2020 um 17:50 hat Paolo Bonzini geschrieben:
> On 03/12/20 16:15, Kevin Wolf wrote:
> > I don't think this is an intermediate state like Eduardo wants to have.
> > Creating the object, then setting properties, then realize [1] will fail
> > after your change. But keeping it working was the whole point of the
> > exercise.
> 
> With the sample code, you must remove object_class_property_set calls at the
> same time as you remove the setters.  Usually that'd be when you convert to
> QAPI and oc->configure, but it doesn't have to be that way if there are good
> reasons not to do so.

Okay, thanks, I think I understand now.

So I assume that in the common case, we'll never have the state that you
describe, but we'll want to directly skip to QAPI generated code. But
it's good to know that we can make smaller steps if we need to in more
complicated cases.

> Also, it still allows you to do so one class at a time, and I *think* the
> presence of subclasses or superclasses doesn't matter (only whether
> properties are still writable).  We can use chardevs (see ChardevCommon in
> qapi/char.json) to validate that before tackling devices.

Yes, it looks like it should be working.

> (In fact, this means that your series---plus -object and object_add
> conversion---would be good, pretty much unchanged, as a first step.  The
> second would be adding oc->configure and object_configure, and converting
> all user-creatable objects to oc->configure.  The third would involve QAPI
> code generation).

I think I'd want to do step 2 and 3 combined, because converting
user-creatable objects to oc->configure means manually writing the
configure function that will be generated from QAPI in step 3. Writing
code just to throw it away isn't my favourite pastime.

> > I'm also not really sure why you go from RngEgdOptions to QObject to a
> > visitor, only to reconstruct RngEgdOptions at the end.
> 
> The two visits are just because you cannot create an input visitor directly
> on C data. I stole that from your patch 18/18 actually, just with
> object_new+object_configure instead of user_creatable_add_type.
> 
> But I wouldn't read too much in the automatically-generated *_new functions
> since they are already in QAPI code generator territory. Instead the basic
> object_configure idea can be applied even without having automatic code
> generation.

Yes, I was just wondering why we're going through visitors at all. But
this is what provides the compatibility with the old property system, so
it makes sense if you need an intermediate step.

> > I think the class
> > implementations should have a normal C interface without visitors and we
> > should be able to just pass the existing RngEgdOptions object (or the
> > individual values for its fields for 'boxed': false).
> 
> Sure, however that requires changes to the QAPI code generator which was
> only item (3) in your list list.  Until then you can already work with a
> visitor interface:
> 
>   void rng_egd_configure(Object *obj, Visitor *v, Error **errp)
>   {
>       RngEgd *s = RNG_EGD(obj);
>       s->config = g_new0(MemoryBackendOptions, 1);
>       visit_type_MemoryBackendOptions(v, NULL, &s->config, errp);
> 
>       s->config->share = (s->config->has_share
>                           ? s->config->share : false);
>       ...
>   }
> 
> but if you had a QAPI description
> 
>   { 'object': 'RngEgd',
>     'qom-type': 'rng-egd',
>     'configuration': 'RngEgdOptions',
>     'boxed': true
>   }
> 
> the QAPI generator could produce the oc->configure implementation. Similar
> to commands, that implementation would be an unmarshaling wrapper that calls
> out to the natural C interface:
> 
>   void qapi_RngEgd_configure(Object *obj, Visitor *v, Error **errp);
>   {
>       Error *local_err = NULL;
>       g_autoptr(MemoryBackendOptions) *config =
>           g_new0(MemoryBackendOptions, 1);
>       visit_type_MemoryBackendOptions(v, NULL, &s->config, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
>           return;
>       }
>       qom_rng_egd_configure(RNG_EGD(obj), config, errp);
>   }
> 
>   void qom_rng_egd_configure(RngEng *s,
>                              RngEgdOptions *config,
>                              Error **errp)
>   {
>       config->share = (config->has_share
>                        ? config->share : false);
>       ...
>       s->config = QAPI_CLONE(RngEgdOptions, config);
>   }

Yes, exactly.

Kevin

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 03/12/20 18:43, Kevin Wolf wrote:
> I think I'd want to do step 2 and 3 combined, because converting
> user-creatable objects to oc->configure means manually writing the
> configure function that will be generated from QAPI in step 3. Writing
> code just to throw it away isn't my favourite pastime.

It would only be a couple lines of extra code, but yeah you don't have 
to do it.  It still is clearer to show the steps one by one as they are 
logically separate and it shows what (modulo inlining) the generated 
code ends up doing.

That said having no setter might simplify the introduction of field 
properties too (no allow_set to worry about); perhaps that's a good 
reason to do the oc->configure conversion earlier rather than later, 
especially if QAPI code generation ends up taking a bit longer.

Another good reason is to make sure the API is stable before moving to 
generated code, especially with respect to inheritance.

Paolo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Eduardo Habkost]

On Thu, Dec 03, 2020 at 05:50:46PM +0100, Paolo Bonzini wrote:
> On 03/12/20 16:15, Kevin Wolf wrote:
> > I don't think this is an intermediate state like Eduardo wants to have.
> > Creating the object, then setting properties, then realize [1] will fail
> > after your change. But keeping it working was the whole point of the
> > exercise.
> 
> With the sample code, you must remove object_class_property_set calls at the

Do you mean object_property_set()?

> same time as you remove the setters.  Usually that'd be when you convert to
> QAPI and oc->configure, but it doesn't have to be that way if there are good
> reasons not to do so.

Having two (or more) similar but incompatible APIs to do exactly
the same thing is a mistake we did before, and I wouldn't like us
to repeat it.

If we can keep qdev_new() + object_property_set() + realize
working after the device is converted, we should.  I believe we
can.

If we can make object_new_configure() work with all (or most)
device types before we manually convert them to the new system,
we should.  I believe we can.

We may be able avoid these questions with -object because
converting all backends at the same time is doable.  With
devices, API usability and maintainability during the transition
period (which could be very long) needs to be taken into account.

-- 
Eduardo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 03/12/20 18:52, Eduardo Habkost wrote:
> On Thu, Dec 03, 2020 at 05:50:46PM +0100, Paolo Bonzini wrote:
>> On 03/12/20 16:15, Kevin Wolf wrote:
>>> I don't think this is an intermediate state like Eduardo wants to have.
>>> Creating the object, then setting properties, then realize [1] will fail
>>> after your change. But keeping it working was the whole point of the
>>> exercise.
>>
>> With the sample code, you must remove object_class_property_set calls at the
> 
> Do you mean object_property_set()?

Yes.

>> same time as you remove the setters.  Usually that'd be when you convert to
>> QAPI and oc->configure, but it doesn't have to be that way if there are good
>> reasons not to do so.
> 
> Having two (or more) similar but incompatible APIs to do exactly
> the same thing is a mistake we did before, and I wouldn't like us
> to repeat it.
> 
> If we can keep qdev_new() + object_property_set() + realize
> working after the device is converted, we should.  I believe we
> can.

You can.  If you want to do that, you have to give up on removing the 
setters; but that's not so beneficial for devices because they already 
use static properties anyway.  They have much less boilerplate than 
-object objects.

> If we can make object_new_configure() work with all (or most)
> device types before we manually convert them to the new system,
> we should.  I believe we can.

Yup, object_new_configure() is the low-level visitor-based API and 
therefore it supports both properties and oc->configure.

> We may be able avoid these questions with -object because
> converting all backends at the same time is doable.  With
> devices, API usability and maintainability during the transition
> period (which could be very long) needs to be taken into account.

I think we're in violent agreement. :)

Paolo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Eduardo Habkost]

On Thu, Dec 03, 2020 at 07:10:37PM +0100, Paolo Bonzini wrote:
> On 03/12/20 18:52, Eduardo Habkost wrote:
> > On Thu, Dec 03, 2020 at 05:50:46PM +0100, Paolo Bonzini wrote:
> > > On 03/12/20 16:15, Kevin Wolf wrote:
> > > > I don't think this is an intermediate state like Eduardo wants to have.
> > > > Creating the object, then setting properties, then realize [1] will fail
> > > > after your change. But keeping it working was the whole point of the
> > > > exercise.
> > > 
> > > With the sample code, you must remove object_class_property_set calls at the
> > 
> > Do you mean object_property_set()?
> 
> Yes.
> 
> > > same time as you remove the setters.  Usually that'd be when you convert to
> > > QAPI and oc->configure, but it doesn't have to be that way if there are good
> > > reasons not to do so.
> > 
> > Having two (or more) similar but incompatible APIs to do exactly
> > the same thing is a mistake we did before, and I wouldn't like us
> > to repeat it.
> > 
> > If we can keep qdev_new() + object_property_set() + realize
> > working after the device is converted, we should.  I believe we
> > can.
> 
> You can.  If you want to do that, you have to give up on removing the
> setters; but that's not so beneficial for devices because they already use
> static properties anyway.  They have much less boilerplate than -object
> objects.

Understood.

We can also get rid of most setters in -object backends using
field properties.  Maybe not a necessary step, but a useful
intermediate step in case the new API takes time to be ready.

> 
> > If we can make object_new_configure() work with all (or most)
> > device types before we manually convert them to the new system,
> > we should.  I believe we can.
> 
> Yup, object_new_configure() is the low-level visitor-based API and therefore
> it supports both properties and oc->configure.

Perfect.  That part was not clear yet to me (I just skimmed to
the example code you posted on the wiki).

> 
> > We may be able avoid these questions with -object because
> > converting all backends at the same time is doable.  With
> > devices, API usability and maintainability during the transition
> > period (which could be very long) needs to be taken into account.
> 
> I think we're in violent agreement. :)
> 
> Paolo
> 

-- 
Eduardo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 01.12.2020 um 22:23 hat Paolo Bonzini geschrieben:
> On 01/12/20 20:35, Kevin Wolf wrote:
> > Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben:
> > I don't think this is actually a new things. We already have types and
> > commands declared with things like 'if': 'defined(TARGET_S390X)'.
> > As far as I understand, QAPI generated files are already built per
> > target, so not compiling things into all binaries should be entirely
> > possible.
> 
> There is some complication due to having different discriminators per
> target.  So yes it should be possible.

We have conditional union variants in existing code, too.

> But probably best left after objects because it's so much bigger a
> task and because objects have a bit more freedom for experimentation
> (less ties to other qdev-specific concepts, e.g.  the magic "bus"
> property).

Yes, I would like to get user creatable objects to a state that we
like and only then start converting other object types. I feel user
creatable objects are a nice set of types that aren't so trivial that
major problems could go unnoticed, but still a managable enough number
that it doesn't matter much if we take one or two extra steps until we
find our preferred shape.

> > So maybe only the abstract base class that actually defines the machine
> > properties (like generic-pc-machine) should be described in QAPI, and
> > then the concrete machine types would inherit from it without being
> > described in QAPI themselves?
> 
> Yes, maybe.
> 
> > > 1) whether to generate _all_ boilerplate or only properties
> > 
> > I would like to generate as much boilerplate as possible. That is, I
> > don't want to constrain us to only properties, but at the same time, I'm
> > not sure if it's possible to get rid of all boilerplate.
> > 
> > Basically, the vision I have in mind is that QAPI would generate code
> > that is the same for most instances, and then provide an option that
> > prevents code generation for a specific part for more complicated cases,
> > so that the respective function can (and must) be provided in the C
> > source.
> 
> Ok, so that's a bit different from what I am thinking of.  I don't
> care very much about the internal boilerplate, only the external
> interface for configuration.  So I don't care about type registration,
> dynamic cast macros etc., only essentially the part that leads to
> ucc->complete.

Once QAPI knows about the class hierarchy you get the internal
boilerplate generation almost for free, so I'm not sure why we would
pass on it. But I agree it's not critical to have it.

> > > 2) whether we want to introduce a separation between configuration
> > > schema and run-time state
> > 
> > You mean the internal run-time state? How is this separation not already
> > present with getter/setter functions for each property? In many cases
> > they just directly access the run-time state, but there are other cases
> > where they actually do things.
> 
> I mean moving more towards the blockdev/chardev way of doing things,
> increasing the separation very much by having separate configuration
> structs that have (potentially) no link to the run-time state struct.

I'm not sure, I'm seeing pros and contras. Also, to be honest, I'm
quite certain that the blockdev/chardev way is accidental rather than
the result of a careful design process.

Having to copy every option from the separate configuration into the
run-time state is somewhat inconvenient. On the other hand, it ensures
that every option is touched in the initialisation function, which
probably increases chances that it's checked for validity.

The separation might have made the kind of bugs more obvious where
property setters just change the configuration without actually applying
the updated value.

I guess we might end up with a mixed model where configuration is
separated into a different struct (the ObjectOptions branches), but
still kept around for the lifetime of the object so that qom-get can
keep working.

> > > 3) in the latter case, whether properties will survive at all---iothread and
> > > throttle-groups don't really need them even if they're writable after
> > > creation.
> > 
> > How do you define properties, i.e. at which point would they stop
> > existing and what would be a non-property alternative?
> 
> Properties are only a useful concept if they have a use.  If
> -object/object_add/object-add can do the same job without properties,
> properties are not needed anymore.

Yes, I think object creation doesn't need properties. But no, that
doesn't automatically make them useless because of property updates
later on. If you want to get fully rid of them, you need a replacement
for the latter.

But you're right that I would like to make property setters only about
runtime changes (i.e. changes after creation) rather than part of the
creation itself. When they have only one job to do, it's more likely
that they actually implement something working for it.

I guess fully getting rid of them and removing qom-set/get in favour of
something new could be a future step, but feature removal requires code
changes in libvirt etc. so let's try to stay compatible for now. It's
hard enough without dealing with changes to external interfaces.

> Right now QOM is all about exposing properties, and having multiple
> interfaces to set them (by picking a different visitor).  But in practice
> most QOM objects have a lifetime that consists of 1) set properties 2) flip
> a switch (realized/complete/open) 3) let the object live on its own.  1+2
> are a single monitor command or CLI option; during 3 you access the object
> through monitor commands, not properties.

True, and you don't need properties for this case.

> > So in summary, it seems to me that the QOM way is more flexible because
> > you can get both models out of it. Whether we actually need this
> > flexibility I can't say.
> 
> I'm thinking there's no need for it, but maybe I'm overly optimistic.

I think you can express everything with the blockdev-reopen style. The
client just happens to repeat itself a lot then, and updating the full
state atomically might be a bit harder than doing just every property
change individually. But there is probably nothing that can be done with
individual properties, but not with an all-or-nothing update mechanism.

> > * Configuration options are described in the QAPI schema. This is mainly
> >    for object creation, but runtime modifiable properties are a subset of
> >    this.
> > 
> > * Properties are generated for each option. By default, the getter
> >    just returns the value from the configuration at creation time, though
> >    generation of it can be disabled so that it can be overridden. Also,
> >    setters just return an error by default.
> > 
> > * Property setters aren't called for object creation. Instead, the
> >    relevant ObjectOptions branch is made available to some init method.
> > 
> > * Runtime modifiable properties (declared as such in the schema) don't
> >    get the default setter, so you have to provide an implementation for
> >    them.
> 
> I wouldn't bother with properties at all in the QAPI schema.  Just do the
> first and third bullet.  Declaring read-only QOM properties is trivial.

Trivial sounds like it's something the computer should be doing.

But there are two more important reasons why I think QAPI should take
care of this:

The first is that I want to get rid of the duplication that properties
have to be defined both in the code and as creation options in QAPI. One
place to describe them is enough. Otherwise you might end up with
options that can't be queried later because you forgot to manually add
the trivial getter property.

The second is that the QAPI schema should actually be a full description
of the external interface of an object, and properties are part of this.
So if we don't describe in it which options are available as (writable)
properties after object creation, the schema will tell only half of the
story.

> > So while this series is doing only one part of the whole solution, that
> > the second part is missing doesn't make the first part wrong.
> 
> Yeah, I think it's clear that for the long term we're not really disagreeing
> (or perhaps I'm even more radical than you :)).  I'm just worried about
> having yet another incomplete transition.

Would you really feel at home in a QEMU without incomplete transitions?
:-)

More seriously, I'm certain that we can get the transition completed for
user creatable options in a reasonable timeframe. And they form a
separate group, as your suggestion to use ucc->complete to implement
things shows, so it wouldn't even be an incomplete transition if we
stopped there instead of extending the concept to other parts.

qdev is a lot larger and it's unrealistic to change all devices at once,
so I can see the risk of an incomplete transition there. But if you
don't want to take this risk, you can't change anything.

> > I was hoping that by converting object-add in this series, and the CLI
> > options soon afterwards, it would be very obvious if you forget to
> > change the schema because your new property just wouldn't work (at least
> > not during creation).
> 
> Converting the CLI options is not entirely trivial due to -readconfig and
> friends, so I was expecting that to last until that part of my 6.0 keyval
> work goes in.  (It's almost ready for posting BTW,
> https://gitlab.com/bonzini/qemu/-/commit/b59288c86c).

Yes, -readconfig is what would be in the way when doing the change on
master. But since you had already posted RFC patches a while ago to
address this, I considered it solved.

> As soon as we have an idea of what we want UserCreatable to look in the end,
> on both the QAPI side and the object implementation side.  That's also the
> part where we have the biggest need to document the schema. With that part
> at least roughly sketched out (no code needed), I'm okay with this series
> going in.

I think so far we agree on these steps:

1. This series (mostly for documentation and introspection)

2. -object and HMP object_add (so that we get QAPI's validation, and to
   make sure that forgetting to update the schema means that the new
   options just doesn't work)

3. Create a separate 'object' entity in QAPI, generate ucc->complete
   implementations that call a create function in the C source code with
   type-specific parameters (like QMP command handlers)

What's still open: Should QAPI cover run-time properties, too? Should
run-time properties even exist at all in the final state? What is the
exact QAPI representation of everything? (The last one includes that I
need to have a closer look at how QAPI can best be taught about
inheritance.)

Kevin

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 02/12/20 11:27, Kevin Wolf wrote:
>> Declaring read-only QOM properties is trivial.
> 
> Trivial sounds like it's something the computer should be doing.

Possibly, but not necessarily.  There's always a cost to automatic code 
generation.  If things are _too_ trivial and easy to get right, the cost 
of automatic code generation exceeds the cost of doing things by hand.

You pretty much proved that ucc->complete is hard enough to get right, 
that it benefits from code generation.  But I am a bit more conservative 
about extending that to the rest of QOM.

>> I'm just worried about having yet another incomplete transition.
> 
> Would you really feel at home in a QEMU without incomplete transitions?

One can always dream. :)

> But since you had already posted RFC patches a while ago to
> address this, I considered it solved.

Yup, I posted the non-RFC today.

> 1. This series (mostly for documentation and introspection)
> 
> 2. -object and HMP object_add (so that we get QAPI's validation, and to
>     make sure that forgetting to update the schema means that the new
>     options just doesn't work)
> 
> 3. Create a separate 'object' entity in QAPI, generate ucc->complete
>     implementations that call a create function in the C source code with
>     type-specific parameters (like QMP command handlers)

If we agree on all of these that's already a pretty good place to be in. 
The decreasing length of the replies to the thread suggests that we are.

I wouldn't mind an example of (3) that is hand-coded and may only work 
for one object type (even a simple one such as iothread), to show what 
the generated QAPI code would look like.  It's not a blocker as far as I 
am concerned, but it would be nice.

More important, Markus and John should agree with the plan before (1) is 
committed.  That may also require the aforementioned example, but it's 
up to them.

> What's still open: Should QAPI cover run-time properties, too? Should
> run-time properties even exist at all in the final state? What is the
> exact QAPI representation of everything? (The last one includes that I
> need to have a closer look at how QAPI can best be taught about
> inheritance.)

Run-time properties will exist but they will probably be cut down 
substantially, and most of them will be read-only (they already are as 
far as external code is concerned, i.e. they have a setter but qom-set 
always fails because it's called too late).

Paolo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Peter Krempa]

On Mon, Nov 30, 2020 at 13:25:20 +0100, Kevin Wolf wrote:
> This series adds a QAPI type for the properties of all user creatable
> QOM types and finally makes QMP object-add use the new ObjectOptions
> union so that QAPI introspection can be used for user creatable objects.

FYI, here's a libvirt patchset which (apart from refactors) adds
compatibility with the deprecated/removed 'props' sub-object and also
adds testing to see whether all objects used by libvirt conform to the
new schema:

https://www.redhat.com/archives/libvir-list/2020-November/msg01625.html

Spoiler alert: surprisingly they do match so no updates are necessary!