[PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Kevin Wolf]

This series adds a QAPI type for the properties of all user creatable
QOM types and finally makes the --object command line option (in all
binaries) and the object-add monitor commands (in QMP and HMP) use the
new ObjectOptions union.

This change improves things in more than just one way:

1. Documentation for QOM object types has always been lacking. Adding
   the schema, we get documentation for every property.

2. It prevents bugs by performing parts of the input validation (e.g.
   checking presence of mandatory properties) already in QAPI instead of
   relying on separate manual implementations in each class.

3. It provides QAPI introspection for user creatable objects.

4. Non-scalar properties are now supported everywhere because the
   command line parsers (including HMP) use the keyval parser now.


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.


In a next step after this series, we can add make use of the QAPI
structs in the implementation of the object and separate their
configuration from the runtime state. Specifically, the plan is to
add a .configure() callback to ObjectClass that allows configuring the
object in one place at creation time and keeping QOM property setters
only for properties that can actually be changed at runtime. Paolo made
an example of what the state could look like after this:

    https://wiki.qemu.org/Features/QOM-QAPI_integration

Finally, the intention is to extend the QAPI schema to have separate
'object' entities and generate some of the code that was written
manually in the intermediate state before.


This series is available as a git tag at:

    https://repo.or.cz/qemu/kevin.git qapi-object-v3


v3:
- Removed now useless QAuthZListRuleListHack
- Made some more ObjectOptions branches conditional
- Improved documentation for some properties
- Fixed 'qemu-img compare' exit code for option parsing failure

v2:
- Convert not only object-add, but all external interfaces so that the
  schema will always be enforced and mismatch between implementation and
  schema can't go unnoticed.
- Rebased, covering properties and object types added since v1 (yes,
  things do become outdated rather quickly when you touch all user
  creatable objects)
- Changed the "Since:" version number in the schema documentation to
  refer to the version when the object was introduced rather than 6.0
  where the schema will (hopefully) be added
- Probably some other minor changes

Kevin Wolf (30):
  qapi/qom: Drop deprecated 'props' from object-add
  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 confidential-guest-support
  qapi/qom: Add ObjectOptions for input-*
  qapi/qom: Add ObjectOptions for x-remote-object
  qapi/qom: QAPIfy object-add
  qom: Make "object" QemuOptsList optional
  qemu-storage-daemon: Implement --object with qmp_object_add()
  qom: Remove user_creatable_add_dict()
  qom: Factor out user_creatable_process_cmdline()
  qemu-io: Use user_creatable_process_cmdline() for --object
  qemu-nbd: Use user_creatable_process_cmdline() for --object
  qom: Add user_creatable_add_from_str()
  qemu-img: Use user_creatable_process_cmdline() for --object
  hmp: QAPIfy object_add
  qom: Add user_creatable_parse_str()
  vl: QAPIfy -object
  qom: Drop QemuOpts based interfaces

 qapi/authz.json                      |  61 ++-
 qapi/block-core.json                 |  27 ++
 qapi/common.json                     |  52 +++
 qapi/crypto.json                     | 159 +++++++
 qapi/machine.json                    |  22 +-
 qapi/net.json                        |  20 -
 qapi/qom.json                        | 644 ++++++++++++++++++++++++++-
 qapi/ui.json                         |  13 +-
 docs/system/deprecated.rst           |  25 +-
 docs/system/removed-features.rst     |   5 +
 include/qom/object_interfaces.h      | 106 ++---
 hw/block/xen-block.c                 |  16 +-
 monitor/hmp-cmds.c                   |  17 +-
 monitor/misc.c                       |   2 -
 qemu-img.c                           | 251 ++---------
 qemu-io.c                            |  33 +-
 qemu-nbd.c                           |  34 +-
 qom/object_interfaces.c              | 168 +++----
 qom/qom-qmp-cmds.c                   |  28 +-
 softmmu/vl.c                         | 109 +++--
 storage-daemon/qemu-storage-daemon.c |  27 +-
 tests/check-qom-proplist.c           |  42 +-
 hmp-commands.hx                      |   2 +-
 storage-daemon/qapi/qapi-schema.json |   1 +
 24 files changed, 1231 insertions(+), 633 deletions(-)

-- 
2.29.2

[PATCH v3 01/30] 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>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/qom.json                    |  6 +-----
 docs/system/deprecated.rst       |  5 -----
 docs/system/removed-features.rst |  5 +++++
 qom/qom-qmp-cmds.c               | 21 ---------------------
 4 files changed, 6 insertions(+), 31 deletions(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index 0b0b92944b..96c91c1faf 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -211,10 +211,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.
 #
@@ -232,7 +228,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 561c916da2..893f3e8579 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -206,11 +206,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)
 ''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
 
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index c8481cafbd..95f3fb2912 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -58,6 +58,11 @@ documentation of ``query-hotpluggable-cpus`` for additional details.
 
 Use ``blockdev-change-medium`` or ``change-vnc-password`` instead.
 
+``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 b40ac39f30..19fd5e117f 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -225,27 +225,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.29.2

[PATCH v3 02/30] 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>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/qom.json | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index 96c91c1faf..bf2ecb34be 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: 2.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.29.2

[PATCH v3 03/30] 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>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/authz.json                      | 61 +++++++++++++++++++++++++---
 qapi/qom.json                        | 10 +++++
 storage-daemon/qapi/qapi-schema.json |  1 +
 3 files changed, 67 insertions(+), 5 deletions(-)

diff --git a/qapi/authz.json b/qapi/authz.json
index 42afe752d1..51845e37cc 100644
--- a/qapi/authz.json
+++ b/qapi/authz.json
@@ -50,12 +50,63 @@
            '*format': 'QAuthZListFormat'}}
 
 ##
-# @QAuthZListRuleListHack:
+# @AuthZListProperties:
 #
-# Not exposed via QMP; hack to generate QAuthZListRuleList
-# for use internally by the code.
+# Properties for authz-list objects.
+#
+# @policy: Default policy to apply when no rule matches (default: deny)
+#
+# @rules: Authorization rules based on matching user
+#
+# Since: 4.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: 4.0
+##
+{ 'struct': 'AuthZListFileProperties',
+  'data': { 'filename': 'str',
+            '*refresh': 'bool' } }
+
+##
+# @AuthZPAMProperties:
+#
+# Properties for authz-pam objects.
+#
+# @service: PAM service name to use for authorization
+#
+# Since: 4.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: 4.0
 ##
-{ 'struct': 'QAuthZListRuleListHack',
-  'data': { 'unused': ['QAuthZListRule'] } }
+{ 'struct': 'AuthZSimpleProperties',
+  'data': { 'identity': 'str' } }
diff --git a/qapi/qom.json b/qapi/qom.json
index bf2ecb34be..30ed179bc1 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 28117c3aac..67749d1101 100644
--- a/storage-daemon/qapi/qapi-schema.json
+++ b/storage-daemon/qapi/qapi-schema.json
@@ -26,6 +26,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.29.2

Re: [PATCH v3 03/30] qapi/qom: Add ObjectOptions for authz-*

[Daniel P. Berrangé]

On Mon, Mar 08, 2021 at 05:54:13PM +0100, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the authz-* objects.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Acked-by: Peter Krempa <pkrempa@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  qapi/authz.json                      | 61 +++++++++++++++++++++++++---
>  qapi/qom.json                        | 10 +++++
>  storage-daemon/qapi/qapi-schema.json |  1 +
>  3 files changed, 67 insertions(+), 5 deletions(-)

Acked-by: Daniel P. Berrangé <berrange@redhat.com>


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

[PATCH v3 04/30] 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>
Acked-by: Peter Krempa <pkrempa@redhat.com>
---
 qapi/qom.json | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index 30ed179bc1..46c2cdc6cf 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: 2.8
+##
+{ '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: 2.12
+##
+{ '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,10 @@
       'authz-listfile':             'AuthZListFileProperties',
       'authz-pam':                  'AuthZPAMProperties',
       'authz-simple':               'AuthZSimpleProperties',
+      'cryptodev-backend':          'CryptodevBackendProperties',
+      'cryptodev-backend-builtin':  'CryptodevBackendProperties',
+      'cryptodev-vhost-user':       { 'type': 'CryptodevVhostUserProperties',
+                                      'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' },
       'iothread':                   'IothreadProperties'
   } }
 
-- 
2.29.2

Re: [PATCH v3 04/30] qapi/qom: Add ObjectOptions for cryptodev-*

[Eric Blake]

On 3/8/21 10:54 AM, Kevin Wolf wrote:
> 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>
> Acked-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  qapi/qom.json | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 

> @@ -239,6 +267,9 @@
>      'authz-listfile',
>      'authz-pam',
>      'authz-simple',
> +    'cryptodev-backend',
> +    'cryptodev-backend-builtin',
> +    'cryptodev-vhost-user',

Shouldn't the enum value be conditional...

>      'iothread'
>    ] }
>  
> @@ -262,6 +293,10 @@
>        'authz-listfile':             'AuthZListFileProperties',
>        'authz-pam':                  'AuthZPAMProperties',
>        'authz-simple':               'AuthZSimpleProperties',
> +      'cryptodev-backend':          'CryptodevBackendProperties',
> +      'cryptodev-backend-builtin':  'CryptodevBackendProperties',
> +      'cryptodev-vhost-user':       { 'type': 'CryptodevVhostUserProperties',
> +                                      'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' },

...if the union branch is likewise?

>        'iothread':                   'IothreadProperties'
>    } }
>  
> 

With that fixed,

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v3 05/30] 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>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/qom.json | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index 46c2cdc6cf..942654e05c 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: 5.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'
   ] }
 
@@ -297,6 +314,7 @@
       'cryptodev-backend-builtin':  'CryptodevBackendProperties',
       'cryptodev-vhost-user':       { 'type': 'CryptodevVhostUserProperties',
                                       'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' },
+      'dbus-vmstate':               'DBusVMStateProperties',
       'iothread':                   'IothreadProperties'
   } }
 
-- 
2.29.2

[PATCH v3 06/30] 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>
Acked-by: Peter Krempa <pkrempa@redhat.com>
---
 qapi/common.json  |  20 ++++++++
 qapi/machine.json |  22 +--------
 qapi/qom.json     | 121 +++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 141 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 330189efe3..4322aee782 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 942654e05c..8c0e06c198 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,115 @@
             '*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
+#
+# @x-use-canonical-path-for-ramblock-id: if true, the canoncial path is used
+#                                        for ramblock-id. Disable this for 4.0
+#                                        machine types or older to allow
+#                                        migration with newer QEMU versions.
+#                                        This option is considered stable
+#                                        despite the x- prefix. (default:
+#                                        false generally, but true for machine
+#                                        types <= 4.0)
+#
+# Since: 2.1
+##
+{ 'struct': 'MemoryBackendProperties',
+  'data': { '*dump': 'bool',
+            '*host-nodes': ['uint16'],
+            '*merge': 'bool',
+            '*policy': 'HostMemPolicy',
+            '*prealloc': 'bool',
+            '*prealloc-threads': 'uint32',
+            '*share': 'bool',
+            'size': 'size',
+            '*x-use-canonical-path-for-ramblock-id': 'bool' } }
+
+##
+# @MemoryBackendFileProperties:
+#
+# Properties for memory-backend-file objects.
+#
+# @align: the base address alignment when QEMU mmap(2)s @mem-path. Some
+#         backend stores specified by @mem-path require 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).
+#
+# @readonly: if true, the backing file is opened read-only; if false, it is
+#            opened read-write. (default: false)
+#
+# Since: 2.1
+##
+{ 'struct': 'MemoryBackendFileProperties',
+  'base': 'MemoryBackendProperties',
+  'data': { '*align': 'size',
+            '*discard-data': 'bool',
+            'mem-path': 'str',
+            '*pmem': { 'type': 'bool', 'if': 'defined(CONFIG_LIBPMEM)' },
+            '*readonly': '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: 2.12
+##
+{ 'struct': 'MemoryBackendMemfdProperties',
+  'base': 'MemoryBackendProperties',
+  'data': { '*hugetlb': 'bool',
+            '*hugetlbsize': 'size',
+            '*seal': 'bool' } }
+
 ##
 # @ObjectType:
 #
@@ -287,7 +397,10 @@
     'cryptodev-backend-builtin',
     'cryptodev-vhost-user',
     'dbus-vmstate',
-    'iothread'
+    'iothread',
+    'memory-backend-file',
+    'memory-backend-memfd',
+    'memory-backend-ram'
   ] }
 
 ##
@@ -315,7 +428,11 @@
       'cryptodev-vhost-user':       { 'type': 'CryptodevVhostUserProperties',
                                       'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' },
       'dbus-vmstate':               'DBusVMStateProperties',
-      'iothread':                   'IothreadProperties'
+      'iothread':                   'IothreadProperties',
+      'memory-backend-file':        'MemoryBackendFileProperties',
+      'memory-backend-memfd':       { 'type': 'MemoryBackendMemfdProperties',
+                                      'if': 'defined(CONFIG_LINUX)' },
+      'memory-backend-ram':         'MemoryBackendProperties'
   } }
 
 ##
-- 
2.29.2

Re: [PATCH v3 06/30] qapi/qom: Add ObjectOptions for memory-backend-*

[Eric Blake]

On 3/8/21 10:54 AM, Kevin Wolf wrote:
> 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>
> Acked-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  qapi/common.json  |  20 ++++++++
>  qapi/machine.json |  22 +--------
>  qapi/qom.json     | 121 +++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 141 insertions(+), 22 deletions(-)
> 

> @@ -287,7 +397,10 @@
>      'cryptodev-backend-builtin',
>      'cryptodev-vhost-user',
>      'dbus-vmstate',
> -    'iothread'
> +    'iothread',
> +    'memory-backend-file',
> +    'memory-backend-memfd',
> +    'memory-backend-ram'

Another leaked enum value...

>    ] }
>  
>  ##
> @@ -315,7 +428,11 @@
>        'cryptodev-vhost-user':       { 'type': 'CryptodevVhostUserProperties',
>                                        'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' },
>        'dbus-vmstate':               'DBusVMStateProperties',
> -      'iothread':                   'IothreadProperties'
> +      'iothread':                   'IothreadProperties',
> +      'memory-backend-file':        'MemoryBackendFileProperties',
> +      'memory-backend-memfd':       { 'type': 'MemoryBackendMemfdProperties',
> +                                      'if': 'defined(CONFIG_LINUX)' },
> +      'memory-backend-ram':         'MemoryBackendProperties'
>    } }

...when compared to the union branches.

Once fixed,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v3 07/30] 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>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@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 8c0e06c198..6d3b8c4fe0 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -382,6 +382,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: 1.3
+##
+{ '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: 1.3
+##
+{ '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: 1.3
+##
+{ 'struct': 'RngRandomProperties',
+  'base': 'RngProperties',
+  'data': { '*filename': 'str' } }
+
 ##
 # @ObjectType:
 #
@@ -400,7 +446,10 @@
     'iothread',
     'memory-backend-file',
     'memory-backend-memfd',
-    'memory-backend-ram'
+    'memory-backend-ram',
+    'rng-builtin',
+    'rng-egd',
+    'rng-random'
   ] }
 
 ##
@@ -432,7 +481,10 @@
       'memory-backend-file':        'MemoryBackendFileProperties',
       'memory-backend-memfd':       { 'type': 'MemoryBackendMemfdProperties',
                                       'if': 'defined(CONFIG_LINUX)' },
-      '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 893f3e8579..3dac79f600 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -153,6 +153,15 @@ The ``-writeconfig`` option is not able to serialize the entire contents
 of the QEMU command line.  It is thus considered a failed experiment
 and deprecated, with no current replacement.
 
+``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.29.2

[PATCH v3 08/30] qapi/qom: Add ObjectOptions for throttle-group

[Kevin Wolf]

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

The only purpose of the x-* properties 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 in the future.

To keep the conversion straightforward, add them to the schema anyway.
We can then remove the options and adjust documentation, test cases etc.
in a separate patch.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/block-core.json | 27 +++++++++++++++++++++++++++
 qapi/qom.json        |  7 +++++--
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9f555d5c1d..a67fa0cc59 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2504,6 +2504,33 @@
             '*bps-write-max' : 'int', '*bps-write-max-length' : 'int',
             '*iops-size' : 'int' } }
 
+##
+# @ThrottleGroupProperties:
+#
+# Properties for throttle-group objects.
+#
+# The options starting with x- are aliases for the same key without x- in
+# the @limits object. As indicated by the x- prefix, this is not a stable
+# interface and may be removed or changed incompatibly in the future. Use
+# @limits for a supported stable interface.
+#
+# @limits: limits to apply for this throttle group
+#
+# Since: 2.11
+##
+{ 'struct': 'ThrottleGroupProperties',
+  'data': { '*limits': 'ThrottleLimits',
+            '*x-iops-total' : 'int', '*x-iops-total-max' : 'int',
+            '*x-iops-total-max-length' : 'int', '*x-iops-read' : 'int',
+            '*x-iops-read-max' : 'int', '*x-iops-read-max-length' : 'int',
+            '*x-iops-write' : 'int', '*x-iops-write-max' : 'int',
+            '*x-iops-write-max-length' : 'int', '*x-bps-total' : 'int',
+            '*x-bps-total-max' : 'int', '*x-bps-total-max-length' : 'int',
+            '*x-bps-read' : 'int', '*x-bps-read-max' : 'int',
+            '*x-bps-read-max-length' : 'int', '*x-bps-write' : 'int',
+            '*x-bps-write-max' : 'int', '*x-bps-write-max-length' : 'int',
+            '*x-iops-size' : 'int' } }
+
 ##
 # @block-stream:
 #
diff --git a/qapi/qom.json b/qapi/qom.json
index 6d3b8c4fe0..0721a636f9 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' }
 
 ##
@@ -449,7 +450,8 @@
     'memory-backend-ram',
     'rng-builtin',
     'rng-egd',
-    'rng-random'
+    'rng-random',
+    'throttle-group'
   ] }
 
 ##
@@ -484,7 +486,8 @@
       'memory-backend-ram':         'MemoryBackendProperties',
       'rng-builtin':                'RngProperties',
       'rng-egd':                    'RngEgdProperties',
-      'rng-random':                 'RngRandomProperties'
+      'rng-random':                 'RngRandomProperties',
+      'throttle-group':             'ThrottleGroupProperties'
   } }
 
 ##
-- 
2.29.2

[PATCH v3 09/30] 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>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/crypto.json           | 61 ++++++++++++++++++++++++++++++++++++++
 qapi/qom.json              |  5 ++++
 docs/system/deprecated.rst | 11 +++++++
 3 files changed, 77 insertions(+)

diff --git a/qapi/crypto.json b/qapi/crypto.json
index 2aebe6fa20..0fef3de66d 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: 2.6
+##
+{ '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: 2.6
+##
+{ '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: 5.1
+##
+{ 'struct': 'SecretKeyringProperties',
+  'base': 'SecretCommonProperties',
+  'data': { 'serial': 'int32' } }
diff --git a/qapi/qom.json b/qapi/qom.json
index 0721a636f9..e4bbddd986 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)
@@ -451,6 +452,8 @@
     'rng-builtin',
     'rng-egd',
     'rng-random',
+    'secret',
+    'secret_keyring',
     'throttle-group'
   ] }
 
@@ -487,6 +490,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 3dac79f600..f4e8226963 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -162,6 +162,17 @@ 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.29.2

Re: [PATCH v3 09/30] qapi/qom: Add ObjectOptions for secret*, deprecate 'loaded'

[Daniel P. Berrangé]

On Mon, Mar 08, 2021 at 05:54:19PM +0100, Kevin Wolf wrote:
> 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>
> Acked-by: Peter Krempa <pkrempa@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  qapi/crypto.json           | 61 ++++++++++++++++++++++++++++++++++++++
>  qapi/qom.json              |  5 ++++
>  docs/system/deprecated.rst | 11 +++++++
>  3 files changed, 77 insertions(+)

Acked-by: Daniel P. Berrangé <berrange@redhat.com>


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

[PATCH v3 10/30] 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>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@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 0fef3de66d..7116ae9a46 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: 2.5
+##
+{ '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: 2.5
+##
+{ '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: 3.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: 2.5
+##
+{ '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 e4bbddd986..512d8fce12 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -454,7 +454,11 @@
     'rng-random',
     'secret',
     'secret_keyring',
-    'throttle-group'
+    'throttle-group',
+    'tls-creds-anon',
+    'tls-creds-psk',
+    'tls-creds-x509',
+    'tls-cipher-suites'
   ] }
 
 ##
@@ -492,7 +496,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.29.2

Re: [PATCH v3 10/30] qapi/qom: Add ObjectOptions for tls-*, deprecate 'loaded'

[Daniel P. Berrangé]

On Mon, Mar 08, 2021 at 05:54:20PM +0100, Kevin Wolf wrote:
> 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>
> Acked-by: Peter Krempa <pkrempa@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  qapi/crypto.json | 98 ++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/qom.json    | 12 +++++-
>  2 files changed, 108 insertions(+), 2 deletions(-)

Acked-by: Daniel P. Berrangé <berrange@redhat.com>


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

[PATCH v3 11/30] 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>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/qom.json | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index 512d8fce12..fd87896bca 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: 2.12
+##
+{ 'struct': 'CanHostSocketcanProperties',
+  'data': { 'if': 'str',
+            'canbus': 'str' } }
+
 ##
 # @CryptodevBackendProperties:
 #
@@ -441,6 +456,8 @@
     'authz-listfile',
     'authz-pam',
     'authz-simple',
+    'can-bus',
+    'can-host-socketcan',
     'cryptodev-backend',
     'cryptodev-backend-builtin',
     'cryptodev-vhost-user',
@@ -481,6 +498,7 @@
       'authz-listfile':             'AuthZListFileProperties',
       'authz-pam':                  'AuthZPAMProperties',
       'authz-simple':               'AuthZSimpleProperties',
+      'can-host-socketcan':         'CanHostSocketcanProperties',
       'cryptodev-backend':          'CryptodevBackendProperties',
       'cryptodev-backend-builtin':  'CryptodevBackendProperties',
       'cryptodev-vhost-user':       { 'type': 'CryptodevVhostUserProperties',
-- 
2.29.2

[PATCH v3 12/30] 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>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/qom.json | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index fd87896bca..a34ae43cb9 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: 2.8
+##
+{ '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:
 #
@@ -458,6 +505,7 @@
     'authz-simple',
     'can-bus',
     'can-host-socketcan',
+    'colo-compare',
     'cryptodev-backend',
     'cryptodev-backend-builtin',
     'cryptodev-vhost-user',
@@ -499,6 +547,7 @@
       'authz-pam':                  'AuthZPAMProperties',
       'authz-simple':               'AuthZSimpleProperties',
       'can-host-socketcan':         'CanHostSocketcanProperties',
+      'colo-compare':               'ColoCompareProperties',
       'cryptodev-backend':          'CryptodevBackendProperties',
       'cryptodev-backend-builtin':  'CryptodevBackendProperties',
       'cryptodev-vhost-user':       { 'type': 'CryptodevVhostUserProperties',
-- 
2.29.2

[PATCH v3 13/30] 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>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@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 c31748c87f..af3f5b0fda 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 a34ae43cb9..6fe775bd83 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: 5.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: 2.5
+##
+{ '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: 2.5
+##
+{ '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: 2.5
+##
+{ '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: 2.6
+##
+{ '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: 2.6
+##
+{ '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: 2.8
+##
+{ 'struct': 'FilterRewriterProperties',
+  'base': 'NetfilterProperties',
+  'data': { '*vnet_hdr_support': 'bool' } }
+
 ##
 # @IothreadProperties:
 #
@@ -510,6 +641,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',
@@ -553,6 +690,12 @@
       'cryptodev-vhost-user':       { 'type': 'CryptodevVhostUserProperties',
                                       'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' },
       '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':       { 'type': 'MemoryBackendMemfdProperties',
-- 
2.29.2

[PATCH v3 14/30] 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>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/qom.json | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index 6fe775bd83..6afac9169f 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -577,6 +577,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: 2.11
+##
+{ 'struct': 'PrManagerHelperProperties',
+  'data': { 'path': 'str' } }
+
 ##
 # @RngProperties:
 #
@@ -651,6 +663,7 @@
     'memory-backend-file',
     'memory-backend-memfd',
     'memory-backend-ram',
+    'pr-manager-helper',
     'rng-builtin',
     'rng-egd',
     'rng-random',
@@ -701,6 +714,7 @@
       'memory-backend-memfd':       { 'type': 'MemoryBackendMemfdProperties',
                                       'if': 'defined(CONFIG_LINUX)' },
       'memory-backend-ram':         'MemoryBackendProperties',
+      'pr-manager-helper':          'PrManagerHelperProperties',
       'rng-builtin':                'RngProperties',
       'rng-egd':                    'RngEgdProperties',
       'rng-random':                 'RngRandomProperties',
-- 
2.29.2

[PATCH v3 15/30] qapi/qom: Add ObjectOptions for confidential-guest-support

[Kevin Wolf]

This adds a QAPI schema for the properties of the objects implementing
the confidential-guest-support interface.

pef-guest and s390x-pv-guest don't have any properties, so they only
need to be added to the ObjectType enum without adding a new branch to
ObjectOptions.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/qom.json | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/qapi/qom.json b/qapi/qom.json
index 6afac9169f..ad72dbdec2 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -635,6 +635,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: 2.12
+##
+{ '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:
 #
@@ -663,12 +695,15 @@
     'memory-backend-file',
     'memory-backend-memfd',
     'memory-backend-ram',
+    {'name': 'pef-guest', 'if': 'defined(CONFIG_PSERIES)' },
     'pr-manager-helper',
     'rng-builtin',
     'rng-egd',
     'rng-random',
     'secret',
     'secret_keyring',
+    {'name': 'sev-guest', 'if': 'defined(CONFIG_SEV)' },
+    's390-pv-guest',
     'throttle-group',
     'tls-creds-anon',
     'tls-creds-psk',
@@ -720,6 +755,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.29.2

[PATCH v3 16/30] 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>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/common.json | 12 ++++++++++
 qapi/qom.json    | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/ui.json     | 13 +----------
 3 files changed, 72 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 ad72dbdec2..6b96e9b0b3 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -444,6 +444,61 @@
   '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 the 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: 4.2
+##
+{ '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: 2.6
+##
+{ 'struct': 'InputLinuxProperties',
+  'data': { 'evdev': 'str',
+            '*grab_all': 'bool',
+            '*repeat': 'bool',
+            '*grab-toggle': 'GrabToggleKeys' } }
+
 ##
 # @IothreadProperties:
 #
@@ -691,6 +746,8 @@
     'filter-redirector',
     'filter-replay',
     'filter-rewriter',
+    'input-barrier',
+    'input-linux',
     'iothread',
     'memory-backend-file',
     'memory-backend-memfd',
@@ -744,6 +801,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':       { 'type': 'MemoryBackendMemfdProperties',
diff --git a/qapi/ui.json b/qapi/ui.json
index d08d72b439..cc1882108b 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.29.2

[PATCH v3 17/30] qapi/qom: Add ObjectOptions for x-remote-object

[Kevin Wolf]

This adds a QAPI schema for the properties of the x-remote-object
object.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/qom.json | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index 6b96e9b0b3..0fd8563693 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -644,6 +644,20 @@
 { 'struct': 'PrManagerHelperProperties',
   'data': { 'path': 'str' } }
 
+##
+# @RemoteObjectProperties:
+#
+# Properties for x-remote-object objects.
+#
+# @fd: file descriptor name previously passed via 'getfd' command
+#
+# @devid: the id of the device to be associated with the file descriptor
+#
+# Since: 6.0
+##
+{ 'struct': 'RemoteObjectProperties',
+  'data': { 'fd': 'str', 'devid': 'str' } }
+
 ##
 # @RngProperties:
 #
@@ -765,7 +779,8 @@
     'tls-creds-anon',
     'tls-creds-psk',
     'tls-creds-x509',
-    'tls-cipher-suites'
+    'tls-cipher-suites',
+    'x-remote-object'
   ] }
 
 ##
@@ -820,7 +835,8 @@
       'tls-creds-anon':             'TlsCredsAnonProperties',
       'tls-creds-psk':              'TlsCredsPskProperties',
       'tls-creds-x509':             'TlsCredsX509Properties',
-      'tls-cipher-suites':          'TlsCredsProperties'
+      'tls-cipher-suites':          'TlsCredsProperties',
+      'x-remote-object':            'RemoteObjectProperties'
   } }
 
 ##
-- 
2.29.2

[PATCH v3 18/30] 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>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@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 +++++++++++++++++++++++--
 storage-daemon/qemu-storage-daemon.c |  2 --
 6 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index 0fd8563693..5b8a5da16f 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -844,13 +844,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
 #
@@ -864,9 +857,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 a3b69e2709..ac82d54063 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -836,17 +836,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 a7650ed747..42efd9e2ab 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -235,8 +235,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 19fd5e117f..e577a96adf 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"
@@ -223,9 +226,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)
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index 23756fc8e5..a1bcbacf05 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -148,8 +148,6 @@ static void init_qmp_commands(void)
     qmp_init_marshal(&qmp_commands);
     qmp_register_command(&qmp_commands, "query-qmp-schema",
                          qmp_query_qmp_schema, QCO_ALLOW_PRECONFIG);
-    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",
-- 
2.29.2

[PATCH v3 19/30] qom: Make "object" QemuOptsList optional

[Kevin Wolf]

This code is going away anyway, but for a few more commits, we'll be in
a state where some binaries still use QemuOpts and others don't. If the
"object" QemuOptsList doesn't even exist, we don't have to remove (or
fail to remove, and therefore abort) a user creatable object from it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qom/object_interfaces.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 7661270b98..d4df2334b7 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -299,6 +299,7 @@ void user_creatable_print_help_from_qdict(QDict *args)
 
 bool user_creatable_del(const char *id, Error **errp)
 {
+    QemuOptsList *opts_list;
     Object *container;
     Object *obj;
 
@@ -318,8 +319,10 @@ bool user_creatable_del(const char *id, Error **errp)
      * if object was defined on the command-line, remove its corresponding
      * option group entry
      */
-    qemu_opts_del(qemu_opts_find(qemu_find_opts_err("object", &error_abort),
-                                 id));
+    opts_list = qemu_find_opts_err("object", NULL);
+    if (opts_list) {
+        qemu_opts_del(qemu_opts_find(opts_list, id));
+    }
 
     object_unparent(obj);
     return true;
-- 
2.29.2

[PATCH v3 20/30] qemu-storage-daemon: Implement --object with qmp_object_add()

[Kevin Wolf]

This QAPIfies --object and ensures that QMP and the command line option
behave the same.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 storage-daemon/qemu-storage-daemon.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index a1bcbacf05..4ab7e73053 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -38,6 +38,7 @@
 #include "qapi/qapi-visit-block-core.h"
 #include "qapi/qapi-visit-block-export.h"
 #include "qapi/qapi-visit-control.h"
+#include "qapi/qapi-visit-qom.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qstring.h"
 #include "qapi/qobject-input-visitor.h"
@@ -134,15 +135,6 @@ enum {
 
 extern QemuOptsList qemu_chardev_opts;
 
-static QemuOptsList qemu_object_opts = {
-    .name = "object",
-    .implied_opt_name = "qom-type",
-    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
-    .desc = {
-        { }
-    },
-};
-
 static void init_qmp_commands(void)
 {
     qmp_init_marshal(&qmp_commands);
@@ -282,14 +274,22 @@ static void process_options(int argc, char *argv[])
             {
                 QDict *args;
                 bool help;
+                Visitor *v;
+                ObjectOptions *options;
 
                 args = keyval_parse(optarg, "qom-type", &help, &error_fatal);
                 if (help) {
                     user_creatable_print_help_from_qdict(args);
                     exit(EXIT_SUCCESS);
                 }
-                user_creatable_add_dict(args, true, &error_fatal);
+
+                v = qobject_input_visitor_new_keyval(QOBJECT(args));
+                visit_type_ObjectOptions(v, NULL, &options, &error_fatal);
+                visit_free(v);
                 qobject_unref(args);
+
+                qmp_object_add(options, &error_fatal);
+                qapi_free_ObjectOptions(options);
                 break;
             }
         case OPTION_PIDFILE:
@@ -338,7 +338,6 @@ int main(int argc, char *argv[])
 
     module_call_init(MODULE_INIT_QOM);
     module_call_init(MODULE_INIT_TRACE);
-    qemu_add_opts(&qemu_object_opts);
     qemu_add_opts(&qemu_trace_opts);
     qcrypto_init(&error_fatal);
     bdrv_init();
-- 
2.29.2

[PATCH v3 21/30] qom: Remove user_creatable_add_dict()

[Kevin Wolf]

This function is now unused and can be removed.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qom/object_interfaces.h | 18 ------------------
 qom/object_interfaces.c         | 32 --------------------------------
 2 files changed, 50 deletions(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 9b9938b8c0..5299603f50 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -86,24 +86,6 @@ Object *user_creatable_add_type(const char *type, const char *id,
                                 const QDict *qdict,
                                 Visitor *v, Error **errp);
 
-/**
- * user_creatable_add_dict:
- * @qdict: the object definition
- * @keyval: if true, use a keyval visitor for processing @qdict (i.e.
- *          assume that all @qdict values are strings); otherwise, use
- *          the normal QObject visitor (i.e. assume all @qdict values
- *          have the QType expected by the QOM object type)
- * @errp: if an error occurs, a pointer to an area to store the error
- *
- * Create an instance of the user creatable object that is defined by
- * @qdict.  The object type is taken from the QDict key 'qom-type', its
- * ID from the key 'id'. The remaining entries in @qdict are used to
- * initialize the object properties.
- *
- * Returns: %true on success, %false on failure.
- */
-bool user_creatable_add_dict(QDict *qdict, bool keyval, Error **errp);
-
 /**
  * user_creatable_add_opts:
  * @opts: the object definition
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index d4df2334b7..02c3934329 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -113,38 +113,6 @@ out:
     return obj;
 }
 
-bool user_creatable_add_dict(QDict *qdict, bool keyval, Error **errp)
-{
-    Visitor *v;
-    Object *obj;
-    g_autofree char *type = NULL;
-    g_autofree char *id = NULL;
-
-    type = g_strdup(qdict_get_try_str(qdict, "qom-type"));
-    if (!type) {
-        error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
-        return false;
-    }
-    qdict_del(qdict, "qom-type");
-
-    id = g_strdup(qdict_get_try_str(qdict, "id"));
-    if (!id) {
-        error_setg(errp, QERR_MISSING_PARAMETER, "id");
-        return false;
-    }
-    qdict_del(qdict, "id");
-
-    if (keyval) {
-        v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
-    } else {
-        v = qobject_input_visitor_new(QOBJECT(qdict));
-    }
-    obj = user_creatable_add_type(type, id, qdict, v, errp);
-    visit_free(v);
-    object_unref(obj);
-    return !!obj;
-}
-
 Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
 {
     Visitor *v;
-- 
2.29.2

[PATCH v3 22/30] qom: Factor out user_creatable_process_cmdline()

[Kevin Wolf]

The implementation for --object can be shared between
qemu-storage-daemon and other binaries, so move it into a function in
qom/object_interfaces.c that is accessible from everywhere.

This also requires moving the implementation of qmp_object_add() into a
new user_creatable_add_qapi(), because qom/qom-qmp-cmds.c is not linked
for tools.

user_creatable_print_help_from_qdict() can become static now.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qom/object_interfaces.h      | 41 +++++++++++++++--------
 qom/object_interfaces.c              | 50 +++++++++++++++++++++++++++-
 qom/qom-qmp-cmds.c                   | 20 +----------
 storage-daemon/qemu-storage-daemon.c | 24 ++-----------
 4 files changed, 80 insertions(+), 55 deletions(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 5299603f50..1e6c51b541 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -2,6 +2,7 @@
 #define OBJECT_INTERFACES_H
 
 #include "qom/object.h"
+#include "qapi/qapi-types-qom.h"
 #include "qapi/visitor.h"
 
 #define TYPE_USER_CREATABLE "user-creatable"
@@ -86,6 +87,18 @@ Object *user_creatable_add_type(const char *type, const char *id,
                                 const QDict *qdict,
                                 Visitor *v, Error **errp);
 
+/**
+ * user_creatable_add_qapi:
+ * @options: the object definition
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Create an instance of the user creatable object according to the
+ * options passed in @opts as described in the QAPI schema documentation.
+ *
+ * Returns: the newly created object or NULL on error
+ */
+void user_creatable_add_qapi(ObjectOptions *options, Error **errp);
+
 /**
  * user_creatable_add_opts:
  * @opts: the object definition
@@ -131,6 +144,21 @@ typedef bool (*user_creatable_add_opts_predicate)(const char *type);
 int user_creatable_add_opts_foreach(void *opaque,
                                     QemuOpts *opts, Error **errp);
 
+/**
+ * user_creatable_process_cmdline:
+ * @optarg: the object definition string as passed on the command line
+ *
+ * Create an instance of the user creatable object by parsing optarg
+ * with a keyval parser and implicit key 'qom-type', converting the
+ * result to ObjectOptions and calling into qmp_object_add().
+ *
+ * If a help option is given, print help instead and exit.
+ *
+ * This function is only meant to be called during command line parsing.
+ * It exits the process on failure or after printing help.
+ */
+void user_creatable_process_cmdline(const char *optarg);
+
 /**
  * user_creatable_print_help:
  * @type: the QOM type to be added
@@ -145,19 +173,6 @@ int user_creatable_add_opts_foreach(void *opaque,
  */
 bool user_creatable_print_help(const char *type, QemuOpts *opts);
 
-/**
- * user_creatable_print_help_from_qdict:
- * @args: options to create
- *
- * Prints help considering the other options given in @args (if "qom-type" is
- * given and valid, print properties for the type, otherwise print valid types)
- *
- * In contrast to user_creatable_print_help(), this function can't return that
- * no help was requested. It should only be called if we know that help is
- * requested and it will always print some help.
- */
-void user_creatable_print_help_from_qdict(QDict *args);
-
 /**
  * user_creatable_del:
  * @id: the unique ID for the object
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 02c3934329..2eaf9971f5 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -2,10 +2,13 @@
 
 #include "qemu/cutils.h"
 #include "qapi/error.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/qmp/qjson.h"
 #include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
 #include "qom/object_interfaces.h"
 #include "qemu/help_option.h"
 #include "qemu/id.h"
@@ -113,6 +116,29 @@ out:
     return obj;
 }
 
+void user_creatable_add_qapi(ObjectOptions *options, Error **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);
+}
+
 Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
 {
     Visitor *v;
@@ -256,7 +282,7 @@ bool user_creatable_print_help(const char *type, QemuOpts *opts)
     return false;
 }
 
-void user_creatable_print_help_from_qdict(QDict *args)
+static void user_creatable_print_help_from_qdict(QDict *args)
 {
     const char *type = qdict_get_try_str(args, "qom-type");
 
@@ -265,6 +291,28 @@ void user_creatable_print_help_from_qdict(QDict *args)
     }
 }
 
+void user_creatable_process_cmdline(const char *optarg)
+{
+    QDict *args;
+    bool help;
+    Visitor *v;
+    ObjectOptions *options;
+
+    args = keyval_parse(optarg, "qom-type", &help, &error_fatal);
+    if (help) {
+        user_creatable_print_help_from_qdict(args);
+        exit(EXIT_SUCCESS);
+    }
+
+    v = qobject_input_visitor_new_keyval(QOBJECT(args));
+    visit_type_ObjectOptions(v, NULL, &options, &error_fatal);
+    visit_free(v);
+    qobject_unref(args);
+
+    user_creatable_add_qapi(options, &error_fatal);
+    qapi_free_ObjectOptions(options);
+}
+
 bool user_creatable_del(const char *id, Error **errp)
 {
     QemuOptsList *opts_list;
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index e577a96adf..2d6f41ecc7 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -228,25 +228,7 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename,
 
 void qmp_object_add(ObjectOptions *options, Error **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);
+    user_creatable_add_qapi(options, errp);
 }
 
 void qmp_object_del(const char *id, Error **errp)
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index 4ab7e73053..5341a494a6 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -38,7 +38,6 @@
 #include "qapi/qapi-visit-block-core.h"
 #include "qapi/qapi-visit-block-export.h"
 #include "qapi/qapi-visit-control.h"
-#include "qapi/qapi-visit-qom.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qstring.h"
 #include "qapi/qobject-input-visitor.h"
@@ -271,27 +270,8 @@ static void process_options(int argc, char *argv[])
                 break;
             }
         case OPTION_OBJECT:
-            {
-                QDict *args;
-                bool help;
-                Visitor *v;
-                ObjectOptions *options;
-
-                args = keyval_parse(optarg, "qom-type", &help, &error_fatal);
-                if (help) {
-                    user_creatable_print_help_from_qdict(args);
-                    exit(EXIT_SUCCESS);
-                }
-
-                v = qobject_input_visitor_new_keyval(QOBJECT(args));
-                visit_type_ObjectOptions(v, NULL, &options, &error_fatal);
-                visit_free(v);
-                qobject_unref(args);
-
-                qmp_object_add(options, &error_fatal);
-                qapi_free_ObjectOptions(options);
-                break;
-            }
+            user_creatable_process_cmdline(optarg);
+            break;
         case OPTION_PIDFILE:
             pid_file = optarg;
             break;
-- 
2.29.2

Re: [PATCH v3 22/30] qom: Factor out user_creatable_process_cmdline()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> The implementation for --object can be shared between
> qemu-storage-daemon and other binaries, so move it into a function in
> qom/object_interfaces.c that is accessible from everywhere.
>
> This also requires moving the implementation of qmp_object_add() into a
> new user_creatable_add_qapi(), because qom/qom-qmp-cmds.c is not linked
> for tools.
>
> user_creatable_print_help_from_qdict() can become static now.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Acked-by: Peter Krempa <pkrempa@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  include/qom/object_interfaces.h      | 41 +++++++++++++++--------
>  qom/object_interfaces.c              | 50 +++++++++++++++++++++++++++-
>  qom/qom-qmp-cmds.c                   | 20 +----------
>  storage-daemon/qemu-storage-daemon.c | 24 ++-----------
>  4 files changed, 80 insertions(+), 55 deletions(-)
>
> diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> index 5299603f50..1e6c51b541 100644
> --- a/include/qom/object_interfaces.h
> +++ b/include/qom/object_interfaces.h
> @@ -2,6 +2,7 @@
>  #define OBJECT_INTERFACES_H
>  
>  #include "qom/object.h"
> +#include "qapi/qapi-types-qom.h"
>  #include "qapi/visitor.h"
>  
>  #define TYPE_USER_CREATABLE "user-creatable"
> @@ -86,6 +87,18 @@ Object *user_creatable_add_type(const char *type, const char *id,
>                                  const QDict *qdict,
>                                  Visitor *v, Error **errp);
>  
> +/**
> + * user_creatable_add_qapi:
> + * @options: the object definition
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Create an instance of the user creatable object according to the
> + * options passed in @opts as described in the QAPI schema documentation.
> + *
> + * Returns: the newly created object or NULL on error
> + */
> +void user_creatable_add_qapi(ObjectOptions *options, Error **errp);
> +
>  /**
>   * user_creatable_add_opts:
>   * @opts: the object definition
> @@ -131,6 +144,21 @@ typedef bool (*user_creatable_add_opts_predicate)(const char *type);
>  int user_creatable_add_opts_foreach(void *opaque,
>                                      QemuOpts *opts, Error **errp);
>  
> +/**
> + * user_creatable_process_cmdline:
> + * @optarg: the object definition string as passed on the command line
> + *
> + * Create an instance of the user creatable object by parsing optarg
> + * with a keyval parser and implicit key 'qom-type', converting the
> + * result to ObjectOptions and calling into qmp_object_add().
> + *
> + * If a help option is given, print help instead and exit.
> + *
> + * This function is only meant to be called during command line parsing.
> + * It exits the process on failure or after printing help.
> + */
> +void user_creatable_process_cmdline(const char *optarg);
> +
>  /**
>   * user_creatable_print_help:
>   * @type: the QOM type to be added
> @@ -145,19 +173,6 @@ int user_creatable_add_opts_foreach(void *opaque,
>   */
>  bool user_creatable_print_help(const char *type, QemuOpts *opts);
>  
> -/**
> - * user_creatable_print_help_from_qdict:
> - * @args: options to create
> - *
> - * Prints help considering the other options given in @args (if "qom-type" is
> - * given and valid, print properties for the type, otherwise print valid types)
> - *
> - * In contrast to user_creatable_print_help(), this function can't return that
> - * no help was requested. It should only be called if we know that help is
> - * requested and it will always print some help.
> - */
> -void user_creatable_print_help_from_qdict(QDict *args);
> -
>  /**
>   * user_creatable_del:
>   * @id: the unique ID for the object
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 02c3934329..2eaf9971f5 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -2,10 +2,13 @@
>  
>  #include "qemu/cutils.h"
>  #include "qapi/error.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/qmp/qjson.h"
>  #include "qapi/qobject-input-visitor.h"
> +#include "qapi/qobject-output-visitor.h"
>  #include "qom/object_interfaces.h"
>  #include "qemu/help_option.h"
>  #include "qemu/id.h"
> @@ -113,6 +116,29 @@ out:
>      return obj;
>  }
>  
> +void user_creatable_add_qapi(ObjectOptions *options, Error **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);
> +}
> +

Observation, not objection:

1. QMP core parses JSON text into QObject, passes to generated
   marshaller.

2. Marshaller converts QObject to ObjectOptions with the QObject input
   visitor, passes to qmp_object_add().

3. qmp_object_add() wraps around user_creatable_add_qapi().

4. user_creatable_add_qapi() converts right back to QObject with the
   QObject output visitor.  It splits the result into qom_type, id and
   the rest, and passes all three to user_creatable_add_type().

5. user_creatable_add_type() performs a virtual visit with the QObject
   input visitor.  The outermost object it visits itself, its children
   it visits by calling object_property_set().

I sure hope we wouldn't write it this way from scratch :)

I think your patch is a reasonable step towards a QOM that is at peace
with QAPI.  But there's plenty of work left.

[...]

Re: [PATCH v3 22/30] qom: Factor out user_creatable_process_cmdline()

[Paolo Bonzini]

On 13/03/21 09:41, Markus Armbruster wrote:
> Observation, not objection:
> 
> 1. QMP core parses JSON text into QObject, passes to generated
>     marshaller.
> 
> 2. Marshaller converts QObject to ObjectOptions with the QObject input
>     visitor, passes to qmp_object_add().
> 
> 3. qmp_object_add() wraps around user_creatable_add_qapi().
> 
> 4. user_creatable_add_qapi() converts right back to QObject with the
>     QObject output visitor.  It splits the result into qom_type, id and
>     the rest, and passes all three to user_creatable_add_type().
> 
> 5. user_creatable_add_type() performs a virtual visit with the QObject
>     input visitor.  The outermost object it visits itself, its children
>     it visits by calling object_property_set().
> 
> I sure hope we wouldn't write it this way from scratch:)

All problems in computer science ca be solved by adding another level of 
indirection, except those that can be solved by adding two more levels 
of indirection.

> I think your patch is a reasonable step towards a QOM that is at peace
> with QAPI.  But there's plenty of work left.

Absolutely, see https://wiki.qemu.org/Features/QOM-QAPI_integration for 
some brainstorming about it.

Paolo

Re: [PATCH v3 22/30] qom: Factor out user_creatable_process_cmdline()

[Kevin Wolf]

Am 13.03.2021 um 09:41 hat Markus Armbruster geschrieben:
> Observation, not objection:
> 
> 1. QMP core parses JSON text into QObject, passes to generated
>    marshaller.
> 
> 2. Marshaller converts QObject to ObjectOptions with the QObject input
>    visitor, passes to qmp_object_add().
> 
> 3. qmp_object_add() wraps around user_creatable_add_qapi().
> 
> 4. user_creatable_add_qapi() converts right back to QObject with the
>    QObject output visitor.  It splits the result into qom_type, id and
>    the rest, and passes all three to user_creatable_add_type().
> 
> 5. user_creatable_add_type() performs a virtual visit with the QObject
>    input visitor.  The outermost object it visits itself, its children
>    it visits by calling object_property_set().
> 
> I sure hope we wouldn't write it this way from scratch :)
> 
> I think your patch is a reasonable step towards a QOM that is at peace
> with QAPI.  But there's plenty of work left.

Yes, obviously the conversion back to QObject is not great. There are
two reasons why we currently need it:

1. user_creatable_add_type() wants to iterate over all properties
   without knowing which properties exist. This should be fixed by not
   visiting the top level in user_creatable_add_type(), but moving this
   part to object-specific code (in the final state probably code
   generated from the QAPI schema).

2. We have ObjectOptions, but need to pass a visitor. We don't have a
   general purpose visitor that visits both sides. The clone visitor
   seems somewhat similar to what we need, but I seem to remember it was
   more restricted to its particular use case.

Kevin

[PATCH v3 23/30] qemu-io: Use user_creatable_process_cmdline() for --object

[Kevin Wolf]

This switches qemu-io from a QemuOpts-based parser for --object to
user_creatable_process_cmdline() which uses a keyval parser and enforces
the QAPI schema.

Apart from being a cleanup, this makes non-scalar properties accessible.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-io.c | 33 +++------------------------------
 1 file changed, 3 insertions(+), 30 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index ac88d8bd40..bf902302e9 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -477,23 +477,6 @@ enum {
     OPTION_IMAGE_OPTS = 257,
 };
 
-static QemuOptsList qemu_object_opts = {
-    .name = "object",
-    .implied_opt_name = "qom-type",
-    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
-    .desc = {
-        { }
-    },
-};
-
-static bool qemu_io_object_print_help(const char *type, QemuOpts *opts)
-{
-    if (user_creatable_print_help(type, opts)) {
-        exit(0);
-    }
-    return true;
-}
-
 static QemuOptsList file_opts = {
     .name = "file",
     .implied_opt_name = "file",
@@ -550,7 +533,6 @@ int main(int argc, char **argv)
     qcrypto_init(&error_fatal);
 
     module_call_init(MODULE_INIT_QOM);
-    qemu_add_opts(&qemu_object_opts);
     qemu_add_opts(&qemu_trace_opts);
     bdrv_init();
 
@@ -612,14 +594,9 @@ int main(int argc, char **argv)
         case 'U':
             force_share = true;
             break;
-        case OPTION_OBJECT: {
-            QemuOpts *qopts;
-            qopts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                            optarg, true);
-            if (!qopts) {
-                exit(1);
-            }
-        }   break;
+        case OPTION_OBJECT:
+            user_creatable_process_cmdline(optarg);
+            break;
         case OPTION_IMAGE_OPTS:
             imageOpts = true;
             break;
@@ -644,10 +621,6 @@ int main(int argc, char **argv)
         exit(1);
     }
 
-    qemu_opts_foreach(&qemu_object_opts,
-                      user_creatable_add_opts_foreach,
-                      qemu_io_object_print_help, &error_fatal);
-
     if (!trace_init_backends()) {
         exit(1);
     }
-- 
2.29.2

[PATCH v3 24/30] qemu-nbd: Use user_creatable_process_cmdline() for --object

[Kevin Wolf]

This switches qemu-nbd from a QemuOpts-based parser for --object to
user_creatable_process_cmdline() which uses a keyval parser and enforces
the QAPI schema.

Apart from being a cleanup, this makes non-scalar properties accessible.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-nbd.c | 34 +++-------------------------------
 1 file changed, 3 insertions(+), 31 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index b1b9430a8f..93ef4e288f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -401,24 +401,6 @@ static QemuOptsList file_opts = {
     },
 };
 
-static QemuOptsList qemu_object_opts = {
-    .name = "object",
-    .implied_opt_name = "qom-type",
-    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
-    .desc = {
-        { }
-    },
-};
-
-static bool qemu_nbd_object_print_help(const char *type, QemuOpts *opts)
-{
-    if (user_creatable_print_help(type, opts)) {
-        exit(0);
-    }
-    return true;
-}
-
-
 static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, bool list,
                                           Error **errp)
 {
@@ -594,7 +576,6 @@ int main(int argc, char **argv)
     qcrypto_init(&error_fatal);
 
     module_call_init(MODULE_INIT_QOM);
-    qemu_add_opts(&qemu_object_opts);
     qemu_add_opts(&qemu_trace_opts);
     qemu_init_exec_dir(argv[0]);
 
@@ -747,14 +728,9 @@ int main(int argc, char **argv)
         case '?':
             error_report("Try `%s --help' for more information.", argv[0]);
             exit(EXIT_FAILURE);
-        case QEMU_NBD_OPT_OBJECT: {
-            QemuOpts *opts;
-            opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                           optarg, true);
-            if (!opts) {
-                exit(EXIT_FAILURE);
-            }
-        }   break;
+        case QEMU_NBD_OPT_OBJECT:
+            user_creatable_process_cmdline(optarg);
+            break;
         case QEMU_NBD_OPT_TLSCREDS:
             tlscredsid = optarg;
             break;
@@ -802,10 +778,6 @@ int main(int argc, char **argv)
         export_name = "";
     }
 
-    qemu_opts_foreach(&qemu_object_opts,
-                      user_creatable_add_opts_foreach,
-                      qemu_nbd_object_print_help, &error_fatal);
-
     if (!trace_init_backends()) {
         exit(1);
     }
-- 
2.29.2

[PATCH v3 25/30] qom: Add user_creatable_add_from_str()

[Kevin Wolf]

This is a version of user_creatable_process_cmdline() with an Error
parameter that never calls exit() and is therefore usable in HMP.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qom/object_interfaces.h | 16 ++++++++++++++++
 qom/object_interfaces.c         | 29 ++++++++++++++++++++++++-----
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 1e6c51b541..07511e6cff 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -144,6 +144,22 @@ typedef bool (*user_creatable_add_opts_predicate)(const char *type);
 int user_creatable_add_opts_foreach(void *opaque,
                                     QemuOpts *opts, Error **errp);
 
+/**
+ * user_creatable_add_from_str:
+ * @optarg: the object definition string as passed on the command line
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Create an instance of the user creatable object by parsing optarg
+ * with a keyval parser and implicit key 'qom-type', converting the
+ * result to ObjectOptions and calling into qmp_object_add().
+ *
+ * If a help option is given, print help instead.
+ *
+ * Returns: true when an object was successfully created, false when an error
+ * occurred (*errp is set then) or help was printed (*errp is not set).
+ */
+bool user_creatable_add_from_str(const char *optarg, Error **errp);
+
 /**
  * user_creatable_process_cmdline:
  * @optarg: the object definition string as passed on the command line
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 2eaf9971f5..bf9f8cd2c6 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -291,26 +291,45 @@ static void user_creatable_print_help_from_qdict(QDict *args)
     }
 }
 
-void user_creatable_process_cmdline(const char *optarg)
+bool user_creatable_add_from_str(const char *optarg, Error **errp)
 {
+    ERRP_GUARD();
     QDict *args;
     bool help;
     Visitor *v;
     ObjectOptions *options;
 
-    args = keyval_parse(optarg, "qom-type", &help, &error_fatal);
+    args = keyval_parse(optarg, "qom-type", &help, errp);
+    if (*errp) {
+        return false;
+    }
     if (help) {
         user_creatable_print_help_from_qdict(args);
-        exit(EXIT_SUCCESS);
+        qobject_unref(args);
+        return false;
     }
 
     v = qobject_input_visitor_new_keyval(QOBJECT(args));
-    visit_type_ObjectOptions(v, NULL, &options, &error_fatal);
+    visit_type_ObjectOptions(v, NULL, &options, errp);
     visit_free(v);
     qobject_unref(args);
 
-    user_creatable_add_qapi(options, &error_fatal);
+    if (*errp) {
+        goto out;
+    }
+
+    user_creatable_add_qapi(options, errp);
+out:
     qapi_free_ObjectOptions(options);
+    return !*errp;
+}
+
+void user_creatable_process_cmdline(const char *optarg)
+{
+    if (!user_creatable_add_from_str(optarg, &error_fatal)) {
+        /* Help was printed */
+        exit(EXIT_SUCCESS);
+    }
 }
 
 bool user_creatable_del(const char *id, Error **errp)
-- 
2.29.2

[PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object

[Kevin Wolf]

This switches qemu-img from a QemuOpts-based parser for --object to
user_creatable_process_cmdline() which uses a keyval parser and enforces
the QAPI schema.

Apart from being a cleanup, this makes non-scalar properties accessible.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
---
 qemu-img.c | 251 ++++++++++-------------------------------------------
 1 file changed, 45 insertions(+), 206 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index e2952fe955..babb5573ab 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -226,23 +226,6 @@ static void QEMU_NORETURN help(void)
     exit(EXIT_SUCCESS);
 }
 
-static QemuOptsList qemu_object_opts = {
-    .name = "object",
-    .implied_opt_name = "qom-type",
-    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
-    .desc = {
-        { }
-    },
-};
-
-static bool qemu_img_object_print_help(const char *type, QemuOpts *opts)
-{
-    if (user_creatable_print_help(type, opts)) {
-        exit(0);
-    }
-    return true;
-}
-
 /*
  * Is @optarg safe for accumulate_options()?
  * It is when multiple of them can be joined together separated by ','.
@@ -566,14 +549,9 @@ static int img_create(int argc, char **argv)
         case 'u':
             flags |= BDRV_O_NO_BACKING;
             break;
-        case OPTION_OBJECT: {
-            QemuOpts *opts;
-            opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                           optarg, true);
-            if (!opts) {
-                goto fail;
-            }
-        }   break;
+        case OPTION_OBJECT:
+            user_creatable_process_cmdline(optarg);
+            break;
         }
     }
 
@@ -589,12 +567,6 @@ static int img_create(int argc, char **argv)
     }
     optind++;
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        goto fail;
-    }
-
     /* Get image size, if specified */
     if (optind < argc) {
         int64_t sval;
@@ -804,14 +776,9 @@ static int img_check(int argc, char **argv)
         case 'U':
             force_share = true;
             break;
-        case OPTION_OBJECT: {
-            QemuOpts *opts;
-            opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                           optarg, true);
-            if (!opts) {
-                return 1;
-            }
-        }   break;
+        case OPTION_OBJECT:
+            user_creatable_process_cmdline(optarg);
+            break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
@@ -831,12 +798,6 @@ static int img_check(int argc, char **argv)
         return 1;
     }
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        return 1;
-    }
-
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", cache);
@@ -1034,14 +995,9 @@ static int img_commit(int argc, char **argv)
                 return 1;
             }
             break;
-        case OPTION_OBJECT: {
-            QemuOpts *opts;
-            opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                           optarg, true);
-            if (!opts) {
-                return 1;
-            }
-        }   break;
+        case OPTION_OBJECT:
+            user_creatable_process_cmdline(optarg);
+            break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
@@ -1058,12 +1014,6 @@ static int img_commit(int argc, char **argv)
     }
     filename = argv[optind++];
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        return 1;
-    }
-
     flags = BDRV_O_RDWR | BDRV_O_UNMAP;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
@@ -1353,7 +1303,7 @@ static int check_empty_sectors(BlockBackend *blk, int64_t offset,
 /*
  * Compares two images. Exit codes:
  *
- * 0 - Images are identical
+ * 0 - Images are identical or the requested help was printed
  * 1 - Images differ
  * >1 - Error occurred
  */
@@ -1423,15 +1373,21 @@ static int img_compare(int argc, char **argv)
         case 'U':
             force_share = true;
             break;
-        case OPTION_OBJECT: {
-            QemuOpts *opts;
-            opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                           optarg, true);
-            if (!opts) {
-                ret = 2;
-                goto out4;
+        case OPTION_OBJECT:
+            {
+                Error *local_err = NULL;
+
+                if (!user_creatable_add_from_str(optarg, &local_err)) {
+                    if (local_err) {
+                        error_report_err(local_err);
+                        exit(2);
+                    } else {
+                        /* Help was printed */
+                        exit(EXIT_SUCCESS);
+                    }
+                }
+                break;
             }
-        }   break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
@@ -1450,13 +1406,6 @@ static int img_compare(int argc, char **argv)
     filename1 = argv[optind++];
     filename2 = argv[optind++];
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        ret = 2;
-        goto out4;
-    }
-
     /* Initialize before goto out */
     qemu_progress_init(progress, 2.0);
 
@@ -1641,7 +1590,6 @@ out2:
     blk_unref(blk1);
 out3:
     qemu_progress_end();
-out4:
     return ret;
 }
 
@@ -2342,15 +2290,9 @@ static int img_convert(int argc, char **argv)
                 goto fail_getopt;
             }
             break;
-        case OPTION_OBJECT: {
-            QemuOpts *object_opts;
-            object_opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                                  optarg, true);
-            if (!object_opts) {
-                goto fail_getopt;
-            }
+        case OPTION_OBJECT:
+            user_creatable_process_cmdline(optarg);
             break;
-        }
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
@@ -2378,12 +2320,6 @@ static int img_convert(int argc, char **argv)
         out_fmt = "raw";
     }
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        goto fail_getopt;
-    }
-
     if (s.compressed && s.copy_range) {
         error_report("Cannot enable copy offloading when -c is used");
         goto fail_getopt;
@@ -2971,14 +2907,9 @@ static int img_info(int argc, char **argv)
         case OPTION_BACKING_CHAIN:
             chain = true;
             break;
-        case OPTION_OBJECT: {
-            QemuOpts *opts;
-            opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                           optarg, true);
-            if (!opts) {
-                return 1;
-            }
-        }   break;
+        case OPTION_OBJECT:
+            user_creatable_process_cmdline(optarg);
+            break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
@@ -2998,12 +2929,6 @@ static int img_info(int argc, char **argv)
         return 1;
     }
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        return 1;
-    }
-
     list = collect_image_info_list(image_opts, filename, fmt, chain,
                                    force_share);
     if (!list) {
@@ -3213,14 +3138,9 @@ static int img_map(int argc, char **argv)
                 return 1;
             }
             break;
-        case OPTION_OBJECT: {
-            QemuOpts *opts;
-            opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                           optarg, true);
-            if (!opts) {
-                return 1;
-            }
-        }   break;
+        case OPTION_OBJECT:
+            user_creatable_process_cmdline(optarg);
+            break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
@@ -3240,12 +3160,6 @@ static int img_map(int argc, char **argv)
         return 1;
     }
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        return 1;
-    }
-
     blk = img_open(image_opts, filename, fmt, 0, false, false, force_share);
     if (!blk) {
         return 1;
@@ -3384,14 +3298,9 @@ static int img_snapshot(int argc, char **argv)
         case 'U':
             force_share = true;
             break;
-        case OPTION_OBJECT: {
-            QemuOpts *opts;
-            opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                           optarg, true);
-            if (!opts) {
-                return 1;
-            }
-        }   break;
+        case OPTION_OBJECT:
+            user_creatable_process_cmdline(optarg);
+            break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
@@ -3403,12 +3312,6 @@ static int img_snapshot(int argc, char **argv)
     }
     filename = argv[optind++];
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        return 1;
-    }
-
     /* Open the image */
     blk = img_open(image_opts, filename, NULL, bdrv_oflags, false, quiet,
                    force_share);
@@ -3542,14 +3445,9 @@ static int img_rebase(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
-        case OPTION_OBJECT: {
-            QemuOpts *opts;
-            opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                           optarg, true);
-            if (!opts) {
-                return 1;
-            }
-        }   break;
+        case OPTION_OBJECT:
+            user_creatable_process_cmdline(optarg);
+            break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
@@ -3571,12 +3469,6 @@ static int img_rebase(int argc, char **argv)
     }
     filename = argv[optind++];
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        return 1;
-    }
-
     qemu_progress_init(progress, 2.0);
     qemu_progress_print(0, 100);
 
@@ -3967,14 +3859,9 @@ static int img_resize(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
-        case OPTION_OBJECT: {
-            QemuOpts *opts;
-            opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                           optarg, true);
-            if (!opts) {
-                return 1;
-            }
-        }   break;
+        case OPTION_OBJECT:
+            user_creatable_process_cmdline(optarg);
+            break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
@@ -3996,12 +3883,6 @@ static int img_resize(int argc, char **argv)
     }
     filename = argv[optind++];
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        return 1;
-    }
-
     /* Choose grow, shrink, or absolute resize mode */
     switch (size[0]) {
     case '+':
@@ -4181,12 +4062,7 @@ static int img_amend(int argc, char **argv)
             quiet = true;
             break;
         case OPTION_OBJECT:
-            opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                           optarg, true);
-            if (!opts) {
-                ret = -1;
-                goto out_no_progress;
-            }
+            user_creatable_process_cmdline(optarg);
             break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
@@ -4201,13 +4077,6 @@ static int img_amend(int argc, char **argv)
         error_exit("Must specify options (-o)");
     }
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        ret = -1;
-        goto out_no_progress;
-    }
-
     if (quiet) {
         progress = false;
     }
@@ -4760,10 +4629,7 @@ static int img_bitmap(int argc, char **argv)
             merge = true;
             break;
         case OPTION_OBJECT:
-            opts = qemu_opts_parse_noisily(&qemu_object_opts, optarg, true);
-            if (!opts) {
-                goto out;
-            }
+            user_creatable_process_cmdline(optarg);
             break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
@@ -4771,12 +4637,6 @@ static int img_bitmap(int argc, char **argv)
         }
     }
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        goto out;
-    }
-
     if (QSIMPLEQ_EMPTY(&actions)) {
         error_report("Need at least one of --add, --remove, --clear, "
                      "--enable, --disable, or --merge");
@@ -5034,10 +4894,7 @@ static int img_dd(int argc, char **argv)
             force_share = true;
             break;
         case OPTION_OBJECT:
-            if (!qemu_opts_parse_noisily(&qemu_object_opts, optarg, true)) {
-                ret = -1;
-                goto out;
-            }
+            user_creatable_process_cmdline(optarg);
             break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
@@ -5084,13 +4941,6 @@ static int img_dd(int argc, char **argv)
         goto out;
     }
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        ret = -1;
-        goto out;
-    }
-
     blk1 = img_open(image_opts, in.filename, fmt, 0, false, false,
                     force_share);
 
@@ -5311,11 +5161,7 @@ static int img_measure(int argc, char **argv)
             force_share = true;
             break;
         case OPTION_OBJECT:
-            object_opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                                  optarg, true);
-            if (!object_opts) {
-                goto out;
-            }
+            user_creatable_process_cmdline(optarg);
             break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
@@ -5345,12 +5191,6 @@ static int img_measure(int argc, char **argv)
         }
     }
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        goto out;
-    }
-
     if (argc - optind > 1) {
         error_report("At most one filename argument is allowed.");
         goto out;
@@ -5490,7 +5330,6 @@ int main(int argc, char **argv)
         error_exit("Not enough arguments");
     }
 
-    qemu_add_opts(&qemu_object_opts);
     qemu_add_opts(&qemu_source_opts);
     qemu_add_opts(&qemu_trace_opts);
 
-- 
2.29.2

Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object

[Eric Blake]

On 3/8/21 10:54 AM, Kevin Wolf wrote:
> This switches qemu-img from a QemuOpts-based parser for --object to
> user_creatable_process_cmdline() which uses a keyval parser and enforces
> the QAPI schema.
> 
> Apart from being a cleanup, this makes non-scalar properties accessible.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Acked-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  qemu-img.c | 251 ++++++++++-------------------------------------------
>  1 file changed, 45 insertions(+), 206 deletions(-)
> 

> @@ -1353,7 +1303,7 @@ static int check_empty_sectors(BlockBackend *blk, int64_t offset,
>  /*
>   * Compares two images. Exit codes:
>   *
> - * 0 - Images are identical
> + * 0 - Images are identical or the requested help was printed

Nice, but does the user-facing documentation need updating to match?

>   * 1 - Images differ
>   * >1 - Error occurred
>   */
> @@ -1423,15 +1373,21 @@ static int img_compare(int argc, char **argv)
>          case 'U':
>              force_share = true;
>              break;
> -        case OPTION_OBJECT: {
> -            QemuOpts *opts;
> -            opts = qemu_opts_parse_noisily(&qemu_object_opts,
> -                                           optarg, true);
> -            if (!opts) {
> -                ret = 2;
> -                goto out4;
> +        case OPTION_OBJECT:
> +            {
> +                Error *local_err = NULL;
> +
> +                if (!user_creatable_add_from_str(optarg, &local_err)) {
> +                    if (local_err) {
> +                        error_report_err(local_err);
> +                        exit(2);
> +                    } else {
> +                        /* Help was printed */
> +                        exit(EXIT_SUCCESS);
> +                    }

The commit message needs to be updated to call out that this bug fix was
intentional, preferably mentioning the commit where we broke it
(334c43e2c3).

The code is fine, though, so with an improved commit message (and maybe
some matching doc tweaks),

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> This switches qemu-img from a QemuOpts-based parser for --object to
> user_creatable_process_cmdline() which uses a keyval parser and enforces
> the QAPI schema.
>
> Apart from being a cleanup, this makes non-scalar properties accessible.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Acked-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  qemu-img.c | 251 ++++++++++-------------------------------------------
>  1 file changed, 45 insertions(+), 206 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index e2952fe955..babb5573ab 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -226,23 +226,6 @@ static void QEMU_NORETURN help(void)
>      exit(EXIT_SUCCESS);
>  }
>  
> -static QemuOptsList qemu_object_opts = {
> -    .name = "object",
> -    .implied_opt_name = "qom-type",
> -    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
> -    .desc = {
> -        { }
> -    },
> -};
> -
> -static bool qemu_img_object_print_help(const char *type, QemuOpts *opts)
> -{
> -    if (user_creatable_print_help(type, opts)) {
> -        exit(0);
> -    }
> -    return true;
> -}
> -
>  /*
>   * Is @optarg safe for accumulate_options()?
>   * It is when multiple of them can be joined together separated by ','.
> @@ -566,14 +549,9 @@ static int img_create(int argc, char **argv)
>          case 'u':
>              flags |= BDRV_O_NO_BACKING;
>              break;
> -        case OPTION_OBJECT: {
> -            QemuOpts *opts;
> -            opts = qemu_opts_parse_noisily(&qemu_object_opts,
> -                                           optarg, true);
> -            if (!opts) {
> -                goto fail;
> -            }
> -        }   break;
> +        case OPTION_OBJECT:
> +            user_creatable_process_cmdline(optarg);
> +            break;
>          }
>      }
>  
> @@ -589,12 +567,6 @@ static int img_create(int argc, char **argv)
>      }
>      optind++;
>  
> -    if (qemu_opts_foreach(&qemu_object_opts,
> -                          user_creatable_add_opts_foreach,
> -                          qemu_img_object_print_help, &error_fatal)) {
> -        goto fail;
> -    }
> -
>      /* Get image size, if specified */
>      if (optind < argc) {
>          int64_t sval;
> @@ -804,14 +776,9 @@ static int img_check(int argc, char **argv)
>          case 'U':
>              force_share = true;
>              break;
> -        case OPTION_OBJECT: {
> -            QemuOpts *opts;
> -            opts = qemu_opts_parse_noisily(&qemu_object_opts,
> -                                           optarg, true);
> -            if (!opts) {
> -                return 1;
> -            }
> -        }   break;
> +        case OPTION_OBJECT:
> +            user_creatable_process_cmdline(optarg);
> +            break;
>          case OPTION_IMAGE_OPTS:
>              image_opts = true;
>              break;
> @@ -831,12 +798,6 @@ static int img_check(int argc, char **argv)
>          return 1;
>      }
>  
> -    if (qemu_opts_foreach(&qemu_object_opts,
> -                          user_creatable_add_opts_foreach,
> -                          qemu_img_object_print_help, &error_fatal)) {
> -        return 1;
> -    }
> -
>      ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
>      if (ret < 0) {
>          error_report("Invalid source cache option: %s", cache);
> @@ -1034,14 +995,9 @@ static int img_commit(int argc, char **argv)
>                  return 1;
>              }
>              break;
> -        case OPTION_OBJECT: {
> -            QemuOpts *opts;
> -            opts = qemu_opts_parse_noisily(&qemu_object_opts,
> -                                           optarg, true);
> -            if (!opts) {
> -                return 1;
> -            }
> -        }   break;
> +        case OPTION_OBJECT:
> +            user_creatable_process_cmdline(optarg);
> +            break;
>          case OPTION_IMAGE_OPTS:
>              image_opts = true;
>              break;
> @@ -1058,12 +1014,6 @@ static int img_commit(int argc, char **argv)
>      }
>      filename = argv[optind++];
>  
> -    if (qemu_opts_foreach(&qemu_object_opts,
> -                          user_creatable_add_opts_foreach,
> -                          qemu_img_object_print_help, &error_fatal)) {
> -        return 1;
> -    }
> -
>      flags = BDRV_O_RDWR | BDRV_O_UNMAP;
>      ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
>      if (ret < 0) {
> @@ -1353,7 +1303,7 @@ static int check_empty_sectors(BlockBackend *blk, int64_t offset,
>  /*
>   * Compares two images. Exit codes:
>   *
> - * 0 - Images are identical
> + * 0 - Images are identical or the requested help was printed
>   * 1 - Images differ
>   * >1 - Error occurred
>   */
> @@ -1423,15 +1373,21 @@ static int img_compare(int argc, char **argv)
>          case 'U':
>              force_share = true;
>              break;
> -        case OPTION_OBJECT: {
> -            QemuOpts *opts;
> -            opts = qemu_opts_parse_noisily(&qemu_object_opts,
> -                                           optarg, true);
> -            if (!opts) {
> -                ret = 2;
> -                goto out4;
> +        case OPTION_OBJECT:
> +            {
> +                Error *local_err = NULL;
> +
> +                if (!user_creatable_add_from_str(optarg, &local_err)) {
> +                    if (local_err) {
> +                        error_report_err(local_err);
> +                        exit(2);
> +                    } else {
> +                        /* Help was printed */
> +                        exit(EXIT_SUCCESS);
> +                    }
> +                }
> +                break;
>              }
> -        }   break;
>          case OPTION_IMAGE_OPTS:
>              image_opts = true;
>              break;

Why is this one different?  The others all call
user_creatable_process_cmdline().


> @@ -1450,13 +1406,6 @@ static int img_compare(int argc, char **argv)
>      filename1 = argv[optind++];
>      filename2 = argv[optind++];
>  
> -    if (qemu_opts_foreach(&qemu_object_opts,
> -                          user_creatable_add_opts_foreach,
> -                          qemu_img_object_print_help, &error_fatal)) {
> -        ret = 2;
> -        goto out4;
> -    }
> -
>      /* Initialize before goto out */
>      qemu_progress_init(progress, 2.0);
>  
> @@ -1641,7 +1590,6 @@ out2:
>      blk_unref(blk1);
>  out3:
>      qemu_progress_end();
> -out4:
>      return ret;
>  }
>  
[...]

Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object

[Paolo Bonzini]

On 13/03/21 08:40, Markus Armbruster wrote:
>> +                if (!user_creatable_add_from_str(optarg, &local_err)) {
>> +                    if (local_err) {
>> +                        error_report_err(local_err);
>> +                        exit(2);
>> +                    } else {
>> +                        /* Help was printed */
>> +                        exit(EXIT_SUCCESS);
>> +                    }
>> +                }
>> +                break;
>>               }
>> -        }   break;
>>           case OPTION_IMAGE_OPTS:
>>               image_opts = true;
>>               break;
> Why is this one different?  The others all call
> user_creatable_process_cmdline().
> 
> 

It's to exit with status code 2 instead of 1.

Paolo

Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 13/03/21 08:40, Markus Armbruster wrote:
>>> +                if (!user_creatable_add_from_str(optarg, &local_err)) {
>>> +                    if (local_err) {
>>> +                        error_report_err(local_err);
>>> +                        exit(2);
>>> +                    } else {
>>> +                        /* Help was printed */
>>> +                        exit(EXIT_SUCCESS);
>>> +                    }
>>> +                }
>>> +                break;
>>>               }
>>> -        }   break;
>>>           case OPTION_IMAGE_OPTS:
>>>               image_opts = true;
>>>               break;
>> Why is this one different?  The others all call
>> user_creatable_process_cmdline().
>> 
>> 
>
> It's to exit with status code 2 instead of 1.

I see.  Worth a comment?

Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object

[Kevin Wolf]

Am 13.03.2021 um 13:30 hat Markus Armbruster geschrieben:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 13/03/21 08:40, Markus Armbruster wrote:
> >>> +                if (!user_creatable_add_from_str(optarg, &local_err)) {
> >>> +                    if (local_err) {
> >>> +                        error_report_err(local_err);
> >>> +                        exit(2);
> >>> +                    } else {
> >>> +                        /* Help was printed */
> >>> +                        exit(EXIT_SUCCESS);
> >>> +                    }
> >>> +                }
> >>> +                break;
> >>>               }
> >>> -        }   break;
> >>>           case OPTION_IMAGE_OPTS:
> >>>               image_opts = true;
> >>>               break;
> >> Why is this one different?  The others all call
> >> user_creatable_process_cmdline().
> >> 
> >> 
> >
> > It's to exit with status code 2 instead of 1.
> 
> I see.  Worth a comment?

There is a comment at the start of the function (which is just a few
lines above) that explains the exit codes:

 * Compares two images. Exit codes:
 *
 * 0 - Images are identical or the requested help was printed
 * 1 - Images differ
 * >1 - Error occurred

Kevin

Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 13.03.2021 um 13:30 hat Markus Armbruster geschrieben:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>> > On 13/03/21 08:40, Markus Armbruster wrote:
>> >>> +                if (!user_creatable_add_from_str(optarg, &local_err)) {
>> >>> +                    if (local_err) {
>> >>> +                        error_report_err(local_err);
>> >>> +                        exit(2);
>> >>> +                    } else {
>> >>> +                        /* Help was printed */
>> >>> +                        exit(EXIT_SUCCESS);
>> >>> +                    }
>> >>> +                }
>> >>> +                break;
>> >>>               }
>> >>> -        }   break;
>> >>>           case OPTION_IMAGE_OPTS:
>> >>>               image_opts = true;
>> >>>               break;
>> >> Why is this one different?  The others all call
>> >> user_creatable_process_cmdline().
>> >> 
>> >> 
>> >
>> > It's to exit with status code 2 instead of 1.
>> 
>> I see.  Worth a comment?
>
> There is a comment at the start of the function (which is just a few
> lines above) that explains the exit codes:
>
>  * Compares two images. Exit codes:
>  *
>  * 0 - Images are identical or the requested help was printed
>  * 1 - Images differ
>  * >1 - Error occurred

I had in mind a comment that helps me over the "why is this not using
user_creatable_process_cmdline()" hump.  Like so:

        case OPTION_OBJECT:
            {
                /*
                 * Can't use user_creatable_process_cmdline(), because
                 * we need to exit(2) on error.
                 */
                ... open-coded variation of
                user_creatable_process_cmdline() ...
            }

Entirely up to you.

Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object

[Kevin Wolf]

Am 15.03.2021 um 15:15 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 13.03.2021 um 13:30 hat Markus Armbruster geschrieben:
> >> Paolo Bonzini <pbonzini@redhat.com> writes:
> >> 
> >> > On 13/03/21 08:40, Markus Armbruster wrote:
> >> >>> +                if (!user_creatable_add_from_str(optarg, &local_err)) {
> >> >>> +                    if (local_err) {
> >> >>> +                        error_report_err(local_err);
> >> >>> +                        exit(2);
> >> >>> +                    } else {
> >> >>> +                        /* Help was printed */
> >> >>> +                        exit(EXIT_SUCCESS);
> >> >>> +                    }
> >> >>> +                }
> >> >>> +                break;
> >> >>>               }
> >> >>> -        }   break;
> >> >>>           case OPTION_IMAGE_OPTS:
> >> >>>               image_opts = true;
> >> >>>               break;
> >> >> Why is this one different?  The others all call
> >> >> user_creatable_process_cmdline().
> >> >> 
> >> >> 
> >> >
> >> > It's to exit with status code 2 instead of 1.
> >> 
> >> I see.  Worth a comment?
> >
> > There is a comment at the start of the function (which is just a few
> > lines above) that explains the exit codes:
> >
> >  * Compares two images. Exit codes:
> >  *
> >  * 0 - Images are identical or the requested help was printed
> >  * 1 - Images differ
> >  * >1 - Error occurred
> 
> I had in mind a comment that helps me over the "why is this not using
> user_creatable_process_cmdline()" hump.  Like so:
> 
>         case OPTION_OBJECT:
>             {
>                 /*
>                  * Can't use user_creatable_process_cmdline(), because
>                  * we need to exit(2) on error.
>                  */
>                 ... open-coded variation of
>                 user_creatable_process_cmdline() ...
>             }
> 
> Entirely up to you.

I see. This patch is already part of a pull request, but I wouldn't mind
a follow-up patch to add this comment if you want to send one.

Kevin

[PATCH v3 27/30] hmp: QAPIfy object_add

[Kevin Wolf]

This switches the HMP command object_add from a QemuOpts-based parser to
user_creatable_add_from_str() which uses a keyval parser and enforces
the QAPI schema.

Apart from being a cleanup, this makes non-scalar properties and help
accessible. In order for help to be printed to the monitor instead of
stdout, the printf() calls in the help functions are changed to
qemu_printf().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 monitor/hmp-cmds.c      | 17 ++---------------
 qom/object_interfaces.c | 11 ++++++-----
 hmp-commands.hx         |  2 +-
 3 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 3c88a4faef..652cf9ff21 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
 
 void hmp_object_add(Monitor *mon, const QDict *qdict)
 {
+    const char *options = qdict_get_str(qdict, "object");
     Error *err = NULL;
-    QemuOpts *opts;
-    Object *obj = NULL;
-
-    opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
-    if (err) {
-        goto end;
-    }
 
-    obj = user_creatable_add_opts(opts, &err);
-    qemu_opts_del(opts);
-
-end:
+    user_creatable_add_from_str(options, &err);
     hmp_handle_error(mon, err);
-
-    if (obj) {
-        object_unref(obj);
-    }
 }
 
 void hmp_getfd(Monitor *mon, const QDict *qdict)
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index bf9f8cd2c6..6dcab60f09 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -14,6 +14,7 @@
 #include "qemu/id.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qemu/qemu-print.h"
 #include "qapi/opts-visitor.h"
 #include "qemu/config-file.h"
 
@@ -221,11 +222,11 @@ static void user_creatable_print_types(void)
 {
     GSList *l, *list;
 
-    printf("List of user creatable objects:\n");
+    qemu_printf("List of user creatable objects:\n");
     list = object_class_get_list_sorted(TYPE_USER_CREATABLE, false);
     for (l = list; l != NULL; l = l->next) {
         ObjectClass *oc = OBJECT_CLASS(l->data);
-        printf("  %s\n", object_class_get_name(oc));
+        qemu_printf("  %s\n", object_class_get_name(oc));
     }
     g_slist_free(list);
 }
@@ -256,12 +257,12 @@ static bool user_creatable_print_type_properites(const char *type)
     }
     g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
     if (array->len > 0) {
-        printf("%s options:\n", type);
+        qemu_printf("%s options:\n", type);
     } else {
-        printf("There are no options for %s.\n", type);
+        qemu_printf("There are no options for %s.\n", type);
     }
     for (i = 0; i < array->len; i++) {
-        printf("%s\n", (char *)array->pdata[i]);
+        qemu_printf("%s\n", (char *)array->pdata[i]);
     }
     g_ptr_array_set_free_func(array, g_free);
     g_ptr_array_free(array, true);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index d4001f9c5d..6f5d9ce2fb 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1337,7 +1337,7 @@ ERST
 
     {
         .name       = "object_add",
-        .args_type  = "object:O",
+        .args_type  = "object:S",
         .params     = "[qom-type=]type,id=str[,prop=value][,...]",
         .help       = "create QOM object",
         .cmd        = hmp_object_add,
-- 
2.29.2

Re: [PATCH v3 27/30] hmp: QAPIfy object_add

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> This switches the HMP command object_add from a QemuOpts-based parser to
> user_creatable_add_from_str() which uses a keyval parser and enforces
> the QAPI schema.
>
> Apart from being a cleanup, this makes non-scalar properties and help
> accessible. In order for help to be printed to the monitor instead of
> stdout, the printf() calls in the help functions are changed to
> qemu_printf().
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Acked-by: Peter Krempa <pkrempa@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  monitor/hmp-cmds.c      | 17 ++---------------
>  qom/object_interfaces.c | 11 ++++++-----
>  hmp-commands.hx         |  2 +-
>  3 files changed, 9 insertions(+), 21 deletions(-)
>
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 3c88a4faef..652cf9ff21 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
>  
>  void hmp_object_add(Monitor *mon, const QDict *qdict)
>  {
> +    const char *options = qdict_get_str(qdict, "object");
>      Error *err = NULL;
> -    QemuOpts *opts;
> -    Object *obj = NULL;
> -
> -    opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
> -    if (err) {
> -        goto end;
> -    }
>  
> -    obj = user_creatable_add_opts(opts, &err);
> -    qemu_opts_del(opts);
> -
> -end:
> +    user_creatable_add_from_str(options, &err);
>      hmp_handle_error(mon, err);
> -
> -    if (obj) {
> -        object_unref(obj);
> -    }
>  }

Doesn't this break the list-valued properties (Memdev member host-nodes,
NumaNodeOptions member cpus) exactly the same way that made us keep
QemuOpts for qemu-system-FOO -object?

[...]

Re: [PATCH v3 27/30] hmp: QAPIfy object_add

[Paolo Bonzini]

On 13/03/21 14:28, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> This switches the HMP command object_add from a QemuOpts-based parser to
>> user_creatable_add_from_str() which uses a keyval parser and enforces
>> the QAPI schema.
>>
>> Apart from being a cleanup, this makes non-scalar properties and help
>> accessible. In order for help to be printed to the monitor instead of
>> stdout, the printf() calls in the help functions are changed to
>> qemu_printf().
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> Acked-by: Peter Krempa <pkrempa@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> ---
>>   monitor/hmp-cmds.c      | 17 ++---------------
>>   qom/object_interfaces.c | 11 ++++++-----
>>   hmp-commands.hx         |  2 +-
>>   3 files changed, 9 insertions(+), 21 deletions(-)
>>
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> index 3c88a4faef..652cf9ff21 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
>>   
>>   void hmp_object_add(Monitor *mon, const QDict *qdict)
>>   {
>> +    const char *options = qdict_get_str(qdict, "object");
>>       Error *err = NULL;
>> -    QemuOpts *opts;
>> -    Object *obj = NULL;
>> -
>> -    opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
>> -    if (err) {
>> -        goto end;
>> -    }
>>   
>> -    obj = user_creatable_add_opts(opts, &err);
>> -    qemu_opts_del(opts);
>> -
>> -end:
>> +    user_creatable_add_from_str(options, &err);
>>       hmp_handle_error(mon, err);
>> -
>> -    if (obj) {
>> -        object_unref(obj);
>> -    }
>>   }
> 
> Doesn't this break the list-valued properties (Memdev member host-nodes,
> NumaNodeOptions member cpus) exactly the same way that made us keep
> QemuOpts for qemu-system-FOO -object?

Yes, it does.  I guess it can just be documented, unlike for the command 
line?

Paolo

Re: [PATCH v3 27/30] hmp: QAPIfy object_add

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 13/03/21 14:28, Markus Armbruster wrote:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>>> This switches the HMP command object_add from a QemuOpts-based parser to
>>> user_creatable_add_from_str() which uses a keyval parser and enforces
>>> the QAPI schema.
>>>
>>> Apart from being a cleanup, this makes non-scalar properties and help
>>> accessible. In order for help to be printed to the monitor instead of
>>> stdout, the printf() calls in the help functions are changed to
>>> qemu_printf().
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> Acked-by: Peter Krempa <pkrempa@redhat.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> ---
>>>   monitor/hmp-cmds.c      | 17 ++---------------
>>>   qom/object_interfaces.c | 11 ++++++-----
>>>   hmp-commands.hx         |  2 +-
>>>   3 files changed, 9 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>>> index 3c88a4faef..652cf9ff21 100644
>>> --- a/monitor/hmp-cmds.c
>>> +++ b/monitor/hmp-cmds.c
>>> @@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
>>>   
>>>   void hmp_object_add(Monitor *mon, const QDict *qdict)
>>>   {
>>> +    const char *options = qdict_get_str(qdict, "object");
>>>       Error *err = NULL;
>>> -    QemuOpts *opts;
>>> -    Object *obj = NULL;
>>> -
>>> -    opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
>>> -    if (err) {
>>> -        goto end;
>>> -    }
>>>   
>>> -    obj = user_creatable_add_opts(opts, &err);
>>> -    qemu_opts_del(opts);
>>> -
>>> -end:
>>> +    user_creatable_add_from_str(options, &err);
>>>       hmp_handle_error(mon, err);
>>> -
>>> -    if (obj) {
>>> -        object_unref(obj);
>>> -    }
>>>   }
>> 
>> Doesn't this break the list-valued properties (Memdev member host-nodes,
>> NumaNodeOptions member cpus) exactly the same way that made us keep
>> QemuOpts for qemu-system-FOO -object?
>
> Yes, it does.  I guess it can just be documented, unlike for the command 
> line?

Maybe.  Judgement call, not mine to make.

Do people create such objects in HMP?  I figure we don't really know.
Educated guess?

If you try, how does it break?  Is it confusing?  Can you show an
example?

Re: [PATCH v3 27/30] hmp: QAPIfy object_add

[Kevin Wolf]

Am 15.03.2021 um 10:39 hat Markus Armbruster geschrieben:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 13/03/21 14:28, Markus Armbruster wrote:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >>> This switches the HMP command object_add from a QemuOpts-based parser to
> >>> user_creatable_add_from_str() which uses a keyval parser and enforces
> >>> the QAPI schema.
> >>>
> >>> Apart from being a cleanup, this makes non-scalar properties and help
> >>> accessible. In order for help to be printed to the monitor instead of
> >>> stdout, the printf() calls in the help functions are changed to
> >>> qemu_printf().
> >>>
> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >>> Acked-by: Peter Krempa <pkrempa@redhat.com>
> >>> Reviewed-by: Eric Blake <eblake@redhat.com>
> >>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>> ---
> >>>   monitor/hmp-cmds.c      | 17 ++---------------
> >>>   qom/object_interfaces.c | 11 ++++++-----
> >>>   hmp-commands.hx         |  2 +-
> >>>   3 files changed, 9 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> >>> index 3c88a4faef..652cf9ff21 100644
> >>> --- a/monitor/hmp-cmds.c
> >>> +++ b/monitor/hmp-cmds.c
> >>> @@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
> >>>   
> >>>   void hmp_object_add(Monitor *mon, const QDict *qdict)
> >>>   {
> >>> +    const char *options = qdict_get_str(qdict, "object");
> >>>       Error *err = NULL;
> >>> -    QemuOpts *opts;
> >>> -    Object *obj = NULL;
> >>> -
> >>> -    opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
> >>> -    if (err) {
> >>> -        goto end;
> >>> -    }
> >>>   
> >>> -    obj = user_creatable_add_opts(opts, &err);
> >>> -    qemu_opts_del(opts);
> >>> -
> >>> -end:
> >>> +    user_creatable_add_from_str(options, &err);
> >>>       hmp_handle_error(mon, err);
> >>> -
> >>> -    if (obj) {
> >>> -        object_unref(obj);
> >>> -    }
> >>>   }
> >> 
> >> Doesn't this break the list-valued properties (Memdev member host-nodes,
> >> NumaNodeOptions member cpus) exactly the same way that made us keep
> >> QemuOpts for qemu-system-FOO -object?
> >
> > Yes, it does.  I guess it can just be documented, unlike for the command 
> > line?
> 
> Maybe.  Judgement call, not mine to make.
> 
> Do people create such objects in HMP?  I figure we don't really know.
> Educated guess?
> 
> If you try, how does it break?  Is it confusing?  Can you show an
> example?

(qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes=0
Error: Invalid parameter type for 'host-nodes', expected: array
(qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes.0=0
(qemu)

HMP is not a stable interface, so changing the syntax didn't feel like a
problem to me. I doubt many people do HMP memory hotplug while setting a
specific NUMA policy, but it wouldn't change my assessment anyway. I
should have made this explicit in the commit message, though.

Kevin

Re: [PATCH v3 27/30] hmp: QAPIfy object_add

[Dr. David Alan Gilbert]

* Kevin Wolf (kwolf@redhat.com) wrote:
> Am 15.03.2021 um 10:39 hat Markus Armbruster geschrieben:
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> > 
> > > On 13/03/21 14:28, Markus Armbruster wrote:
> > >> Kevin Wolf <kwolf@redhat.com> writes:
> > >> 
> > >>> This switches the HMP command object_add from a QemuOpts-based parser to
> > >>> user_creatable_add_from_str() which uses a keyval parser and enforces
> > >>> the QAPI schema.
> > >>>
> > >>> Apart from being a cleanup, this makes non-scalar properties and help
> > >>> accessible. In order for help to be printed to the monitor instead of
> > >>> stdout, the printf() calls in the help functions are changed to
> > >>> qemu_printf().
> > >>>
> > >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > >>> Acked-by: Peter Krempa <pkrempa@redhat.com>
> > >>> Reviewed-by: Eric Blake <eblake@redhat.com>
> > >>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > >>> ---
> > >>>   monitor/hmp-cmds.c      | 17 ++---------------
> > >>>   qom/object_interfaces.c | 11 ++++++-----
> > >>>   hmp-commands.hx         |  2 +-
> > >>>   3 files changed, 9 insertions(+), 21 deletions(-)
> > >>>
> > >>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> > >>> index 3c88a4faef..652cf9ff21 100644
> > >>> --- a/monitor/hmp-cmds.c
> > >>> +++ b/monitor/hmp-cmds.c
> > >>> @@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
> > >>>   
> > >>>   void hmp_object_add(Monitor *mon, const QDict *qdict)
> > >>>   {
> > >>> +    const char *options = qdict_get_str(qdict, "object");
> > >>>       Error *err = NULL;
> > >>> -    QemuOpts *opts;
> > >>> -    Object *obj = NULL;
> > >>> -
> > >>> -    opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
> > >>> -    if (err) {
> > >>> -        goto end;
> > >>> -    }
> > >>>   
> > >>> -    obj = user_creatable_add_opts(opts, &err);
> > >>> -    qemu_opts_del(opts);
> > >>> -
> > >>> -end:
> > >>> +    user_creatable_add_from_str(options, &err);
> > >>>       hmp_handle_error(mon, err);
> > >>> -
> > >>> -    if (obj) {
> > >>> -        object_unref(obj);
> > >>> -    }
> > >>>   }
> > >> 
> > >> Doesn't this break the list-valued properties (Memdev member host-nodes,
> > >> NumaNodeOptions member cpus) exactly the same way that made us keep
> > >> QemuOpts for qemu-system-FOO -object?
> > >
> > > Yes, it does.  I guess it can just be documented, unlike for the command 
> > > line?
> > 
> > Maybe.  Judgement call, not mine to make.
> > 
> > Do people create such objects in HMP?  I figure we don't really know.
> > Educated guess?
> > 
> > If you try, how does it break?  Is it confusing?  Can you show an
> > example?
> 
> (qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes=0
> Error: Invalid parameter type for 'host-nodes', expected: array
> (qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes.0=0
> (qemu)
> 
> HMP is not a stable interface, so changing the syntax didn't feel like a
> problem to me. I doubt many people do HMP memory hotplug while setting a
> specific NUMA policy, but it wouldn't change my assessment anyway. I
> should have made this explicit in the commit message, though.

I'm OK for it to change, but yes I'd like to have the before/after
syntax listed somewhere as easy references for people confused.

Dave

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

Re: [PATCH v3 27/30] hmp: QAPIfy object_add

[Paolo Bonzini]

On 15/03/21 12:38, Dr. David Alan Gilbert wrote:
> * Kevin Wolf (kwolf@redhat.com) wrote:
>> Am 15.03.2021 um 10:39 hat Markus Armbruster geschrieben:
>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>
>>>> On 13/03/21 14:28, Markus Armbruster wrote:
>>>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>>>
>>>>>> This switches the HMP command object_add from a QemuOpts-based parser to
>>>>>> user_creatable_add_from_str() which uses a keyval parser and enforces
>>>>>> the QAPI schema.
>>>>>>
>>>>>> Apart from being a cleanup, this makes non-scalar properties and help
>>>>>> accessible. In order for help to be printed to the monitor instead of
>>>>>> stdout, the printf() calls in the help functions are changed to
>>>>>> qemu_printf().
>>>>>>
>>>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>>>> Acked-by: Peter Krempa <pkrempa@redhat.com>
>>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>>>> ---
>>>>>>    monitor/hmp-cmds.c      | 17 ++---------------
>>>>>>    qom/object_interfaces.c | 11 ++++++-----
>>>>>>    hmp-commands.hx         |  2 +-
>>>>>>    3 files changed, 9 insertions(+), 21 deletions(-)
>>>>>>
>>>>>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>>>>>> index 3c88a4faef..652cf9ff21 100644
>>>>>> --- a/monitor/hmp-cmds.c
>>>>>> +++ b/monitor/hmp-cmds.c
>>>>>> @@ -1670,24 +1670,11 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
>>>>>>    
>>>>>>    void hmp_object_add(Monitor *mon, const QDict *qdict)
>>>>>>    {
>>>>>> +    const char *options = qdict_get_str(qdict, "object");
>>>>>>        Error *err = NULL;
>>>>>> -    QemuOpts *opts;
>>>>>> -    Object *obj = NULL;
>>>>>> -
>>>>>> -    opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
>>>>>> -    if (err) {
>>>>>> -        goto end;
>>>>>> -    }
>>>>>>    
>>>>>> -    obj = user_creatable_add_opts(opts, &err);
>>>>>> -    qemu_opts_del(opts);
>>>>>> -
>>>>>> -end:
>>>>>> +    user_creatable_add_from_str(options, &err);
>>>>>>        hmp_handle_error(mon, err);
>>>>>> -
>>>>>> -    if (obj) {
>>>>>> -        object_unref(obj);
>>>>>> -    }
>>>>>>    }
>>>>>
>>>>> Doesn't this break the list-valued properties (Memdev member host-nodes,
>>>>> NumaNodeOptions member cpus) exactly the same way that made us keep
>>>>> QemuOpts for qemu-system-FOO -object?
>>>>
>>>> Yes, it does.  I guess it can just be documented, unlike for the command
>>>> line?
>>>
>>> Maybe.  Judgement call, not mine to make.
>>>
>>> Do people create such objects in HMP?  I figure we don't really know.
>>> Educated guess?
>>>
>>> If you try, how does it break?  Is it confusing?  Can you show an
>>> example?
>>
>> (qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes=0
>> Error: Invalid parameter type for 'host-nodes', expected: array
>> (qemu) object_add memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes.0=0
>> (qemu)
>>
>> HMP is not a stable interface, so changing the syntax didn't feel like a
>> problem to me. I doubt many people do HMP memory hotplug while setting a
>> specific NUMA policy, but it wouldn't change my assessment anyway. I
>> should have made this explicit in the commit message, though.
> 
> I'm OK for it to change, but yes I'd like to have the before/after
> syntax listed somewhere as easy references for people confused.

I think we should try to improve the string-value QObject visitor to 
also allow JSON values in some places, for example to allow

object_add 
memory-backend-ram,id=mem,size=4G,policy=bind,host-nodes=[0,1,2,3]

Paolo

[PATCH v3 28/30] qom: Add user_creatable_parse_str()

[Kevin Wolf]

The system emulator has a more complicated way of handling command line
options in that it reorders options before it processes them. This means
that parsing object options and creating the object happen at two
different points. Split the parsing part into a separate function that
can be reused by the system emulator command line.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qom/object_interfaces.h | 15 +++++++++++++++
 qom/object_interfaces.c         | 20 ++++++++++++++------
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 07511e6cff..fb32330901 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -144,6 +144,21 @@ typedef bool (*user_creatable_add_opts_predicate)(const char *type);
 int user_creatable_add_opts_foreach(void *opaque,
                                     QemuOpts *opts, Error **errp);
 
+/**
+ * user_creatable_parse_str:
+ * @optarg: the object definition string as passed on the command line
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Parses the option for the user creatable object with a keyval parser and
+ * implicit key 'qom-type', converting the result to ObjectOptions.
+ *
+ * If a help option is given, print help instead.
+ *
+ * Returns: ObjectOptions on success, NULL when an error occurred (*errp is set
+ * then) or help was printed (*errp is not set).
+ */
+ObjectOptions *user_creatable_parse_str(const char *optarg, Error **errp);
+
 /**
  * user_creatable_add_from_str:
  * @optarg: the object definition string as passed on the command line
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 6dcab60f09..62d7db7629 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -292,7 +292,7 @@ static void user_creatable_print_help_from_qdict(QDict *args)
     }
 }
 
-bool user_creatable_add_from_str(const char *optarg, Error **errp)
+ObjectOptions *user_creatable_parse_str(const char *optarg, Error **errp)
 {
     ERRP_GUARD();
     QDict *args;
@@ -302,12 +302,12 @@ bool user_creatable_add_from_str(const char *optarg, Error **errp)
 
     args = keyval_parse(optarg, "qom-type", &help, errp);
     if (*errp) {
-        return false;
+        return NULL;
     }
     if (help) {
         user_creatable_print_help_from_qdict(args);
         qobject_unref(args);
-        return false;
+        return NULL;
     }
 
     v = qobject_input_visitor_new_keyval(QOBJECT(args));
@@ -315,12 +315,20 @@ bool user_creatable_add_from_str(const char *optarg, Error **errp)
     visit_free(v);
     qobject_unref(args);
 
-    if (*errp) {
-        goto out;
+    return options;
+}
+
+bool user_creatable_add_from_str(const char *optarg, Error **errp)
+{
+    ERRP_GUARD();
+    ObjectOptions *options;
+
+    options = user_creatable_parse_str(optarg, errp);
+    if (!options) {
+        return false;
     }
 
     user_creatable_add_qapi(options, errp);
-out:
     qapi_free_ObjectOptions(options);
     return !*errp;
 }
-- 
2.29.2

[PATCH v3 29/30] vl: QAPIfy -object

[Kevin Wolf]

This switches the system emulator from a QemuOpts-based parser for
-object to user_creatable_parse_str() which uses a keyval parser and
enforces the QAPI schema.

Apart from being a cleanup, this makes non-scalar properties accessible.

This adopts a similar model as -blockdev uses: When parsing the option,
create the ObjectOptions and queue them. At the later point where we
used to create objects for the collected QemuOpts, the ObjectOptions
queue is processed instead.

A complication compared to -blockdev is that object definitions are
supported in -readconfig and -writeconfig.

After this patch, -readconfig still works, though it still goes through
the QemuOpts parser, which means that improvements like non-scalar
properties are still not available in config files.

-writeconfig stops working for -object. Tough luck. It has never
supported all options (not even the common ones), so supporting one less
isn't the end of the world. As object definitions from -readconfig still
go through QemuOpts, they are still included in -writeconfig output,
which at least prevents destroying your existing configuration when you
just wanted to add another option.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 softmmu/vl.c | 109 +++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 84 insertions(+), 25 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index 10bd8a10a3..deb061cc78 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -113,6 +113,7 @@
 #include "sysemu/replay.h"
 #include "qapi/qapi-events-run-state.h"
 #include "qapi/qapi-visit-block-core.h"
+#include "qapi/qapi-visit-qom.h"
 #include "qapi/qapi-visit-ui.h"
 #include "qapi/qapi-commands-block-core.h"
 #include "qapi/qapi-commands-migration.h"
@@ -132,6 +133,14 @@ typedef struct BlockdevOptionsQueueEntry {
 
 typedef QSIMPLEQ_HEAD(, BlockdevOptionsQueueEntry) BlockdevOptionsQueue;
 
+typedef struct ObjectOptionsQueueEntry {
+    ObjectOptions *options;
+    Location loc;
+    QTAILQ_ENTRY(ObjectOptionsQueueEntry) next;
+} ObjectOptionsQueueEntry;
+
+typedef QTAILQ_HEAD(, ObjectOptionsQueueEntry) ObjectOptionsQueue;
+
 static const char *cpu_option;
 static const char *mem_path;
 static const char *incoming;
@@ -143,6 +152,7 @@ static int snapshot;
 static bool preconfig_requested;
 static QemuPluginList plugin_list = QTAILQ_HEAD_INITIALIZER(plugin_list);
 static BlockdevOptionsQueue bdo_queue = QSIMPLEQ_HEAD_INITIALIZER(bdo_queue);
+static ObjectOptionsQueue obj_queue = QTAILQ_HEAD_INITIALIZER(obj_queue);
 static bool nographic = false;
 static int mem_prealloc; /* force preallocation of physical target memory */
 static ram_addr_t ram_size;
@@ -1691,12 +1701,9 @@ static int machine_set_property(void *opaque,
  * cannot be created here, as it depends on the chardev
  * already existing.
  */
-static bool object_create_early(const char *type, QemuOpts *opts)
+static bool object_create_early(ObjectOptions *options)
 {
-    if (user_creatable_print_help(type, opts)) {
-        exit(0);
-    }
-
+    const char *type = ObjectType_str(options->qom_type);
     /*
      * Objects should not be made "delayed" without a reason.  If you
      * add one, state the reason in a comment!
@@ -1744,6 +1751,56 @@ static bool object_create_early(const char *type, QemuOpts *opts)
     return true;
 }
 
+static void object_queue_create(bool early)
+{
+    ObjectOptionsQueueEntry *entry, *next;
+
+    QTAILQ_FOREACH_SAFE(entry, &obj_queue, next, next) {
+        if (early != object_create_early(entry->options)) {
+            continue;
+        }
+        QTAILQ_REMOVE(&obj_queue, entry, next);
+        loc_push_restore(&entry->loc);
+        user_creatable_add_qapi(entry->options, &error_fatal);
+        loc_pop(&entry->loc);
+        qapi_free_ObjectOptions(entry->options);
+        g_free(entry);
+    }
+}
+
+/*
+ * -readconfig still parses things into QemuOpts. Convert any such
+ *  configurations to an ObjectOptionsQueueEntry.
+ *
+ *  This is more restricted than the normal -object parser because QemuOpts
+ *  parsed things, so no support for non-scalar properties. Help is also not
+ *  supported (but this shouldn't be requested in a config file anyway).
+ */
+static int object_readconfig_to_qapi(void *opaque, QemuOpts *opts, Error **errp)
+{
+    ERRP_GUARD();
+    ObjectOptionsQueueEntry *entry;
+    ObjectOptions *options;
+    QDict *args = qemu_opts_to_qdict(opts, NULL);
+    Visitor *v;
+
+    v = qobject_input_visitor_new_keyval(QOBJECT(args));
+    visit_type_ObjectOptions(v, NULL, &options, errp);
+    visit_free(v);
+    qobject_unref(args);
+
+    if (*errp) {
+        return -1;
+    }
+
+    entry = g_new0(ObjectOptionsQueueEntry, 1);
+    entry->options = options;
+    loc_save(&entry->loc);
+    QTAILQ_INSERT_TAIL(&obj_queue, entry, next);
+
+    return 0;
+}
+
 static void qemu_apply_machine_options(void)
 {
     MachineClass *machine_class = MACHINE_GET_CLASS(current_machine);
@@ -1816,8 +1873,8 @@ static void qemu_create_early_backends(void)
     }
 
     qemu_opts_foreach(qemu_find_opts("object"),
-                      user_creatable_add_opts_foreach,
-                      object_create_early, &error_fatal);
+                      object_readconfig_to_qapi, NULL, &error_fatal);
+    object_queue_create(true);
 
     /* spice needs the timers to be initialized by this point */
     /* spice must initialize before audio as it changes the default auiodev */
@@ -1841,16 +1898,6 @@ static void qemu_create_early_backends(void)
     audio_init_audiodevs();
 }
 
-
-/*
- * The remainder of object creation happens after the
- * creation of chardev, fsdev, net clients and device data types.
- */
-static bool object_create_late(const char *type, QemuOpts *opts)
-{
-    return !object_create_early(type, opts);
-}
-
 static void qemu_create_late_backends(void)
 {
     if (qtest_chrdev) {
@@ -1859,9 +1906,11 @@ static void qemu_create_late_backends(void)
 
     net_init_clients(&error_fatal);
 
-    qemu_opts_foreach(qemu_find_opts("object"),
-                      user_creatable_add_opts_foreach,
-                      object_create_late, &error_fatal);
+    /*
+     * The remainder of object creation happens after the
+     * creation of chardev, fsdev, net clients and device data types.
+     */
+    object_queue_create(false);
 
     if (tpm_init() < 0) {
         exit(1);
@@ -3408,12 +3457,22 @@ void qemu_init(int argc, char **argv, char **envp)
 #endif
                 break;
             case QEMU_OPTION_object:
-                opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
-                                               optarg, true);
-                if (!opts) {
-                    exit(1);
+                {
+                    ObjectOptionsQueueEntry *entry;
+                    ObjectOptions *options;
+
+                    options = user_creatable_parse_str(optarg, &error_fatal);
+                    if (!options)  {
+                        /* Help was printed */
+                        exit(EXIT_SUCCESS);
+                    }
+
+                    entry = g_new0(ObjectOptionsQueueEntry, 1);
+                    entry->options = options;
+                    loc_save(&entry->loc);
+                    QTAILQ_INSERT_TAIL(&obj_queue, entry, next);
+                    break;
                 }
-                break;
             case QEMU_OPTION_overcommit:
                 opts = qemu_opts_parse_noisily(qemu_find_opts("overcommit"),
                                                optarg, false);
-- 
2.29.2

Re: [PATCH v3 29/30] vl: QAPIfy -object

[Eric Blake]

On 3/8/21 10:54 AM, Kevin Wolf wrote:
> This switches the system emulator from a QemuOpts-based parser for
> -object to user_creatable_parse_str() which uses a keyval parser and
> enforces the QAPI schema.
> 
> Apart from being a cleanup, this makes non-scalar properties accessible.
> 
> This adopts a similar model as -blockdev uses: When parsing the option,
> create the ObjectOptions and queue them. At the later point where we
> used to create objects for the collected QemuOpts, the ObjectOptions
> queue is processed instead.
> 
> A complication compared to -blockdev is that object definitions are
> supported in -readconfig and -writeconfig.
> 
> After this patch, -readconfig still works, though it still goes through
> the QemuOpts parser, which means that improvements like non-scalar
> properties are still not available in config files.
> 
> -writeconfig stops working for -object. Tough luck. It has never
> supported all options (not even the common ones), so supporting one less
> isn't the end of the world. As object definitions from -readconfig still
> go through QemuOpts, they are still included in -writeconfig output,
> which at least prevents destroying your existing configuration when you
> just wanted to add another option.

Maybe worth a tweak to this paragraph now that b979c931 has landed
formally deprecating -writeconfig (all the more reason we don't care
about it).

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Acked-by: Peter Krempa <pkrempa@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

R-b stands either way.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v3 30/30] qom: Drop QemuOpts based interfaces

[Kevin Wolf]

user_creatable_add_opts() has only a single user left, which is a test
case. Rewrite the test to use user_creatable_add_type() instead (which
is the remaining function that doesn't require a QAPI schema) and drop
the QemuOpts related functions.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qom/object_interfaces.h | 59 ------------------------
 qom/object_interfaces.c         | 81 ---------------------------------
 tests/check-qom-proplist.c      | 42 ++++++++---------
 3 files changed, 20 insertions(+), 162 deletions(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index fb32330901..ac6c33ceac 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -99,51 +99,6 @@ Object *user_creatable_add_type(const char *type, const char *id,
  */
 void user_creatable_add_qapi(ObjectOptions *options, Error **errp);
 
-/**
- * user_creatable_add_opts:
- * @opts: the object definition
- * @errp: if an error occurs, a pointer to an area to store the error
- *
- * Create an instance of the user creatable object whose type
- * is defined in @opts by the 'qom-type' option, placing it
- * in the object composition tree with name provided by the
- * 'id' field. The remaining options in @opts are used to
- * initialize the object properties.
- *
- * Returns: the newly created object or NULL on error
- */
-Object *user_creatable_add_opts(QemuOpts *opts, Error **errp);
-
-
-/**
- * user_creatable_add_opts_predicate:
- * @type: the QOM type to be added
- *
- * A callback function to determine whether an object
- * of type @type should be created. Instances of this
- * callback should be passed to user_creatable_add_opts_foreach
- */
-typedef bool (*user_creatable_add_opts_predicate)(const char *type);
-
-/**
- * user_creatable_add_opts_foreach:
- * @opaque: a user_creatable_add_opts_predicate callback or NULL
- * @opts: options to create
- * @errp: unused
- *
- * An iterator callback to be used in conjunction with
- * the qemu_opts_foreach() method for creating a list of
- * objects from a set of QemuOpts
- *
- * The @opaque parameter can be passed a user_creatable_add_opts_predicate
- * callback to filter which types of object are created during iteration.
- * When it fails, report the error.
- *
- * Returns: 0 on success, -1 when an error was reported.
- */
-int user_creatable_add_opts_foreach(void *opaque,
-                                    QemuOpts *opts, Error **errp);
-
 /**
  * user_creatable_parse_str:
  * @optarg: the object definition string as passed on the command line
@@ -190,20 +145,6 @@ bool user_creatable_add_from_str(const char *optarg, Error **errp);
  */
 void user_creatable_process_cmdline(const char *optarg);
 
-/**
- * user_creatable_print_help:
- * @type: the QOM type to be added
- * @opts: options to create
- *
- * Prints help if requested in @type or @opts. Note that if @type is neither
- * "help"/"?" nor a valid user creatable type, no help will be printed
- * regardless of @opts.
- *
- * Returns: true if a help option was found and help was printed, false
- * otherwise.
- */
-bool user_creatable_print_help(const char *type, QemuOpts *opts);
-
 /**
  * user_creatable_del:
  * @id: the unique ID for the object
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 62d7db7629..61d6d74a26 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -10,13 +10,10 @@
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qobject-output-visitor.h"
 #include "qom/object_interfaces.h"
-#include "qemu/help_option.h"
 #include "qemu/id.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/qemu-print.h"
-#include "qapi/opts-visitor.h"
-#include "qemu/config-file.h"
 
 bool user_creatable_complete(UserCreatable *uc, Error **errp)
 {
@@ -140,60 +137,6 @@ void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
     visit_free(v);
 }
 
-Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
-{
-    Visitor *v;
-    QDict *pdict;
-    Object *obj;
-    const char *id = qemu_opts_id(opts);
-    char *type = qemu_opt_get_del(opts, "qom-type");
-
-    if (!type) {
-        error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
-        return NULL;
-    }
-    if (!id) {
-        error_setg(errp, QERR_MISSING_PARAMETER, "id");
-        qemu_opt_set(opts, "qom-type", type, &error_abort);
-        g_free(type);
-        return NULL;
-    }
-
-    qemu_opts_set_id(opts, NULL);
-    pdict = qemu_opts_to_qdict(opts, NULL);
-
-    v = opts_visitor_new(opts);
-    obj = user_creatable_add_type(type, id, pdict, v, errp);
-    visit_free(v);
-
-    qemu_opts_set_id(opts, (char *) id);
-    qemu_opt_set(opts, "qom-type", type, &error_abort);
-    g_free(type);
-    qobject_unref(pdict);
-    return obj;
-}
-
-
-int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error **errp)
-{
-    bool (*type_opt_predicate)(const char *, QemuOpts *) = opaque;
-    Object *obj = NULL;
-    const char *type;
-
-    type = qemu_opt_get(opts, "qom-type");
-    if (type && type_opt_predicate &&
-        !type_opt_predicate(type, opts)) {
-        return 0;
-    }
-
-    obj = user_creatable_add_opts(opts, errp);
-    if (!obj) {
-        return -1;
-    }
-    object_unref(obj);
-    return 0;
-}
-
 char *object_property_help(const char *name, const char *type,
                            QObject *defval, const char *description)
 {
@@ -269,20 +212,6 @@ static bool user_creatable_print_type_properites(const char *type)
     return true;
 }
 
-bool user_creatable_print_help(const char *type, QemuOpts *opts)
-{
-    if (is_help_option(type)) {
-        user_creatable_print_types();
-        return true;
-    }
-
-    if (qemu_opt_has_help_opt(opts)) {
-        return user_creatable_print_type_properites(type);
-    }
-
-    return false;
-}
-
 static void user_creatable_print_help_from_qdict(QDict *args)
 {
     const char *type = qdict_get_try_str(args, "qom-type");
@@ -343,7 +272,6 @@ void user_creatable_process_cmdline(const char *optarg)
 
 bool user_creatable_del(const char *id, Error **errp)
 {
-    QemuOptsList *opts_list;
     Object *container;
     Object *obj;
 
@@ -359,15 +287,6 @@ bool user_creatable_del(const char *id, Error **errp)
         return false;
     }
 
-    /*
-     * if object was defined on the command-line, remove its corresponding
-     * option group entry
-     */
-    opts_list = qemu_find_opts_err("object", NULL);
-    if (opts_list) {
-        qemu_opts_del(qemu_opts_find(opts_list, id));
-    }
-
     object_unparent(obj);
     return true;
 }
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 1b76581980..3bf208ba8b 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -21,6 +21,8 @@
 #include "qemu/osdep.h"
 
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qobject-input-visitor.h"
 #include "qom/object.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
@@ -400,20 +402,30 @@ static void test_dummy_createlist(void)
 
 static void test_dummy_createcmdl(void)
 {
-    QemuOpts *opts;
     DummyObject *dobj;
-    Error *err = NULL;
+    QDict *args;
+    Visitor *v;
+    g_autofree const char *qom_type = NULL;
+    g_autofree const char *id = NULL;
     const char *params = TYPE_DUMMY \
                          ",id=dev0," \
                          "bv=yes,sv=Hiss hiss hiss,av=platypus";
 
-    qemu_add_opts(&qemu_object_opts);
-    opts = qemu_opts_parse(&qemu_object_opts, params, true, &err);
-    g_assert(err == NULL);
-    g_assert(opts);
+    args = keyval_parse(params, "qom-type", NULL, &error_abort);
+
+    qom_type = g_strdup(qdict_get_str(args, "qom-type"));
+    qdict_del(args, "qom-type");
+    g_assert(!strcmp(qom_type, TYPE_DUMMY));
+
+    id = g_strdup(qdict_get_str(args, "id"));
+    qdict_del(args, "id");
+    g_assert(!strcmp(id, "dev0"));
+
+    v = qobject_input_visitor_new_keyval(QOBJECT(args));
+    dobj = DUMMY_OBJECT(user_creatable_add_type(TYPE_DUMMY, id, args, v,
+                                                &error_abort));
+    visit_free(v);
 
-    dobj = DUMMY_OBJECT(user_creatable_add_opts(opts, &err));
-    g_assert(err == NULL);
     g_assert(dobj);
     g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
     g_assert(dobj->bv == true);
@@ -422,20 +434,6 @@ static void test_dummy_createcmdl(void)
     user_creatable_del("dev0", &error_abort);
 
     object_unref(OBJECT(dobj));
-
-    /*
-     * cmdline-parsing via qemu_opts_parse() results in a QemuOpts entry
-     * corresponding to the Object's ID to be added to the QemuOptsList
-     * for objects. To avoid having this entry conflict with future
-     * Objects using the same ID (which can happen in cases where
-     * qemu_opts_parse() is used to parse the object params, such as
-     * with hmp_object_add() at the time of this comment), we need to
-     * check for this in user_creatable_del() and remove the QemuOpts if
-     * it is present.
-     *
-     * The below check ensures this works as expected.
-     */
-    g_assert_null(qemu_opts_find(&qemu_object_opts, "dev0"));
 }
 
 static void test_dummy_badenum(void)
-- 
2.29.2

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Peter Krempa]

On Mon, Mar 08, 2021 at 17:54:10 +0100, Kevin Wolf wrote:
> This series adds a QAPI type for the properties of all user creatable
> QOM types and finally makes the --object command line option (in all
> binaries) and the object-add monitor commands (in QMP and HMP) use the
> new ObjectOptions union.
> 
> This change improves things in more than just one way:
> 
> 1. Documentation for QOM object types has always been lacking. Adding
>    the schema, we get documentation for every property.
> 
> 2. It prevents bugs by performing parts of the input validation (e.g.
>    checking presence of mandatory properties) already in QAPI instead of
>    relying on separate manual implementations in each class.
> 
> 3. It provides QAPI introspection for user creatable objects.
> 
> 4. Non-scalar properties are now supported everywhere because the
>    command line parsers (including HMP) use the keyval parser now.
> 
> 
> 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.
> 
> 
> In a next step after this series, we can add make use of the QAPI
> structs in the implementation of the object and separate their
> configuration from the runtime state. Specifically, the plan is to
> add a .configure() callback to ObjectClass that allows configuring the
> object in one place at creation time and keeping QOM property setters
> only for properties that can actually be changed at runtime. Paolo made
> an example of what the state could look like after this:
> 
>     https://wiki.qemu.org/Features/QOM-QAPI_integration
> 
> Finally, the intention is to extend the QAPI schema to have separate
> 'object' entities and generate some of the code that was written
> manually in the intermediate state before.
> 
> 
> This series is available as a git tag at:
> 
>     https://repo.or.cz/qemu/kevin.git qapi-object-v3
> 
> 
> v3:
> - Removed now useless QAuthZListRuleListHack
> - Made some more ObjectOptions branches conditional
> - Improved documentation for some properties
> - Fixed 'qemu-img compare' exit code for option parsing failure
> 
> v2:
> - Convert not only object-add, but all external interfaces so that the
>   schema will always be enforced and mismatch between implementation and
>   schema can't go unnoticed.
> - Rebased, covering properties and object types added since v1 (yes,
>   things do become outdated rather quickly when you touch all user
>   creatable objects)
> - Changed the "Since:" version number in the schema documentation to
>   refer to the version when the object was introduced rather than 6.0
>   where the schema will (hopefully) be added
> - Probably some other minor changes

I've stumbled upon a regression with this patchset applied:

error: internal error: process exited while connecting to monitor: qemu-system-x86_64: -object memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind: Invalid parameter type for 'host-nodes', expected: array

Full commandline is:

LC_ALL=C \
PATH=/root/.local/bin:/root/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin \
HOME=/var/lib/libvirt/qemu/domain-15-cd \
USER=root \
LOGNAME=root \
XDG_DATA_HOME=/var/lib/libvirt/qemu/domain-15-cd/.local/share \
XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain-15-cd/.cache \
XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain-15-cd/.config \
/home/pipo/git/qemu.git/build/x86_64-softmmu/qemu-system-x86_64 \
-name guest=cd,debug-threads=on \
-S \
-object secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-15-cd/master-key.aes \
-machine pc-i440fx-2.9,accel=kvm,usb=off,vmport=off,dump-guest-core=off,memory-backend=pc.ram \
-cpu EPYC-Rome,x2apic=on,tsc-deadline=on,hypervisor=on,tsc-adjust=on,stibp=on,arch-capabilities=on,ssbd=on,xsaves=on,cmp-legacy=on,amd-ssbd=on,virt-ssbd=on,rdctl-no=on,skip-l1dfl-vmentry=on,mds-no=on,pschange-mc-no=on \
-m 1000 \
-object memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind \
-overcommit mem-lock=off \
-smp 2,maxcpus=8,sockets=8,cores=1,threads=1 \
-uuid 8e70100a-64b4-4186-aff9-e055c3075cb0 \
-no-user-config \
-nodefaults \
-device sga \
-chardev socket,id=charmonitor,fd=42,server=on,wait=off \
-mon chardev=charmonitor,id=monitor,mode=control \
-rtc base=utc,driftfix=slew \
-global kvm-pit.lost_tick_policy=delay \
-no-hpet \
-no-shutdown \
-global PIIX4_PM.disable_s3=1 \
-global PIIX4_PM.disable_s4=1 \
-boot menu=on,strict=on \
-device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x6.0x7 \
-device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x6 \
-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x6.0x1 \
-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x6.0x2 \
-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x9 \
-device ahci,id=sata0,bus=pci.0,addr=0xb \
-device ahci,id=sata1,bus=pci.0,addr=0x3 \
-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 \
-blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/systemrescuecd-amd64-6.1.2.iso","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \
-blockdev '{"node-name":"libvirt-1-format","read-only":true,"driver":"raw","file":"libvirt-1-storage"}' \
-device ide-cd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1 \
-chardev pty,id=charserial0 \
-device isa-serial,chardev=charserial0,id=serial0 \
-chardev socket,id=charchannel0,fd=44,server=on,wait=off \
-device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0 \
-chardev spicevmc,id=charchannel1,name=vdagent \
-device virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,id=channel1,name=com.redhat.spice.0 \
-device usb-tablet,id=input0,bus=usb.0,port=1 \
-audiodev id=audio1,driver=spice \
-spice port=5901,addr=127.0.0.1,disable-ticketing=on,image-compression=off,seamless-migration=on \
-device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pci.0,addr=0x2 \
-device intel-hda,id=sound0,bus=pci.0,addr=0x4 \
-device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0,audiodev=audio1 \
-chardev spicevmc,id=charredir0,name=usbredir \
-device usb-redir,chardev=charredir0,id=redir0,bus=usb.0,port=2 \
-chardev spicevmc,id=charredir1,name=usbredir \
-device usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=3 \
-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0xa \
-object rng-random,id=objrng0,filename=/dev/random \
-device virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.0,addr=0x7 \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
-msg timestamp=on

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Paolo Bonzini]

On 10/03/21 15:22, Peter Krempa wrote:
> I've stumbled upon a regression with this patchset applied:
> 
> error: internal error: process exited while connecting to monitor: qemu-system-x86_64: -object memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind: Invalid parameter type for 'host-nodes', expected: array

This is the magic conversion of "string containing a list of integers" 
to "list of integers".

The relevant code is in qapi/string-input-visitor.c, but I'm not sure 
where to stick it in the keyval-based parsing flow (i.e. 
qobject_input_visitor_new_keyval).  Markus, any ideas?

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Peter Krempa]

On Wed, Mar 10, 2021 at 15:31:57 +0100, Paolo Bonzini wrote:
> On 10/03/21 15:22, Peter Krempa wrote:
> > I've stumbled upon a regression with this patchset applied:
> > 
> > error: internal error: process exited while connecting to monitor: qemu-system-x86_64: -object memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind: Invalid parameter type for 'host-nodes', expected: array
> 
> This is the magic conversion of "string containing a list of integers" to
> "list of integers".
> 
> The relevant code is in qapi/string-input-visitor.c, but I'm not sure where
> to stick it in the keyval-based parsing flow (i.e.
> qobject_input_visitor_new_keyval).  Markus, any ideas?

I've got a slightly off-topic question/idea.

Would it be reasonably easy/straightforward to run qemu's init code
which parses arguments and possibly validates them but quit before
actually starting to initiate resources?

The use case would be to plug it (optionally?) into libvirt's
qemuxml2argvtest to prevent such a thing from happening.

It's not feasible to run all the configurations we have with a real VM
but a partial validation would possibly make sense if it's easy enough.

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Kevin Wolf]

Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben:
> On 10/03/21 15:22, Peter Krempa wrote:
> > I've stumbled upon a regression with this patchset applied:
> > 
> > error: internal error: process exited while connecting to monitor: qemu-system-x86_64: -object memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind: Invalid parameter type for 'host-nodes', expected: array
> 
> This is the magic conversion of "string containing a list of integers"
> to "list of integers".

Ah, crap. This one wouldn't have been a problem when converting only
object-add, and I trusted your audit that user creatable objects don't
depend on any QemuOpts magic. I should have noticed it, too, of course,
but during the convertion I didn't have QemuOpts in mind, only QOM and
QAPI.

I checked all object types, and it seems this is the only one that is
affected. We have a second list in AuthZListProperties, but it contains
structs, so it was never supported on the command line anyway.

> The relevant code is in qapi/string-input-visitor.c, but I'm not sure where
> to stick it in the keyval-based parsing flow (i.e.
> qobject_input_visitor_new_keyval).  Markus, any ideas?

The best I can think of at the moment would be adding a flag to the
keyval parser that would enable the feature only for -object (and only
in the system emulator, because memory-backend-ram doesn't exist in the
tools):

The keyval parser would create a list if multiple values are given for
the same key. Some care needs to be taken to avoid mixing the magic
list feature with the normal indexed list options.

The QAPI schema would then use an alternate between 'int' and ['int'],
with the the memory-backend-ram implementation changed accordingly.

We could consider immediately deprecating the syntax and printing a
warning in the keyval parser when it automatically creates a list from
two values for a key, so that users don't start using this syntax
instead of the normal list syntax in other places. We'd probably still
leave the implementation around for -device and other users of the same
magic.

Kevin

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Peter Krempa]

On Wed, Mar 10, 2021 at 18:30:44 +0100, Kevin Wolf wrote:
> Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben:
> > On 10/03/21 15:22, Peter Krempa wrote:

[...]

> The keyval parser would create a list if multiple values are given for
> the same key. Some care needs to be taken to avoid mixing the magic
> list feature with the normal indexed list options.
> 
> The QAPI schema would then use an alternate between 'int' and ['int'],
> with the the memory-backend-ram implementation changed accordingly.
> 
> We could consider immediately deprecating the syntax and printing a
> warning in the keyval parser when it automatically creates a list from
> two values for a key, so that users don't start using this syntax

By 'creating a list from two values for a key' you mean:

host-nodes=0,host-nodes=1

to be converted into [0, 1] ?

> instead of the normal list syntax in other places. We'd probably still
> leave the implementation around for -device and other users of the same
> magic.

There's three options actually that libvirt uses, visible in one our
test files [1]

For a single value we format:

-object memory-backend-ram,id=ram-node0,size=20971520,host-nodes=3,policy=preferred

For a contiguous list:

-object memory-backend-ram,id=ram-node1,size=676331520,host-nodes=0-7,policy=bind

And for an interleaved list:

-object memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind

If any of the above is to be deprecated we'll need to adjust our
JSON->commandline generator accordignly.

Luckily the 'host-nodes' is storeable as a bitmap and the generator is
actually modular to allow plugging an array interpretor which actually
does the above conversion from a JSON array.

So, what is the preferred syntax here? Additionally we might need a
witness property to detect when to use the new syntax if basing it on
the object-add qapification will not be enough.

[1] https://gitlab.com/libvirt/libvirt/-/blob/master/tests/qemuxml2argvdata/numatune-memnode.args

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Paolo Bonzini]

On 11/03/21 08:47, Peter Krempa wrote:
> And for an interleaved list:
> 
> -object memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind

Uhm, I doubt this works?  I would have expected "host-nodes=1-2,,5,,7" 
instead.

Paolo

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Kevin Wolf]

Am 11.03.2021 um 08:47 hat Peter Krempa geschrieben:
> On Wed, Mar 10, 2021 at 18:30:44 +0100, Kevin Wolf wrote:
> > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben:
> > > On 10/03/21 15:22, Peter Krempa wrote:
> 
> [...]
> 
> > The keyval parser would create a list if multiple values are given for
> > the same key. Some care needs to be taken to avoid mixing the magic
> > list feature with the normal indexed list options.
> > 
> > The QAPI schema would then use an alternate between 'int' and ['int'],
> > with the the memory-backend-ram implementation changed accordingly.
> > 
> > We could consider immediately deprecating the syntax and printing a
> > warning in the keyval parser when it automatically creates a list from
> > two values for a key, so that users don't start using this syntax
> 
> By 'creating a list from two values for a key' you mean:
> 
> host-nodes=0,host-nodes=1
> 
> to be converted into [0, 1] ?
> 
> > instead of the normal list syntax in other places. We'd probably still
> > leave the implementation around for -device and other users of the same
> > magic.
> 
> There's three options actually that libvirt uses, visible in one our
> test files [1]
> 
> For a single value we format:
> 
> -object memory-backend-ram,id=ram-node0,size=20971520,host-nodes=3,policy=preferred
> 
> For a contiguous list:
> 
> -object memory-backend-ram,id=ram-node1,size=676331520,host-nodes=0-7,policy=bind
> 
> And for an interleaved list:
> 
> -object memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind

Oh, we have ranges, too... That makes the compatibility code even
nastier to write. I doubt that we can implement this in the keyval
parser alone without touching the visitor. :-/

> If any of the above is to be deprecated we'll need to adjust our
> JSON->commandline generator accordignly.
> 
> Luckily the 'host-nodes' is storeable as a bitmap and the generator is
> actually modular to allow plugging an array interpretor which actually
> does the above conversion from a JSON array.
> 
> So, what is the preferred syntax here? Additionally we might need a
> witness property to detect when to use the new syntax if basing it on
> the object-add qapification will not be enough.

The list syntax supported by the keyval parser is the one you know from
-blockdev: host-nodes.0=3,host-nodes.1=7, ...

Kevin

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Peter Krempa]

On Thu, Mar 11, 2021 at 09:37:11 +0100, Kevin Wolf wrote:
> Am 11.03.2021 um 08:47 hat Peter Krempa geschrieben:
> > On Wed, Mar 10, 2021 at 18:30:44 +0100, Kevin Wolf wrote:
> > > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben:
> > > > On 10/03/21 15:22, Peter Krempa wrote:

[...]

> > -object memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind
> 
> Oh, we have ranges, too... That makes the compatibility code even
> nastier to write. I doubt that we can implement this in the keyval
> parser alone without touching the visitor. :-/
> 
> > If any of the above is to be deprecated we'll need to adjust our
> > JSON->commandline generator accordignly.
> > 
> > Luckily the 'host-nodes' is storeable as a bitmap and the generator is
> > actually modular to allow plugging an array interpretor which actually
> > does the above conversion from a JSON array.
> > 
> > So, what is the preferred syntax here? Additionally we might need a
> > witness property to detect when to use the new syntax if basing it on
> > the object-add qapification will not be enough.
> 
> The list syntax supported by the keyval parser is the one you know from
> -blockdev: host-nodes.0=3,host-nodes.1=7, ...

I think that should be easy enough to convert to. Is it safe to do right
away (with the old parser?). Otherwise we need to agree on something
which will let us detect when it's safe to change.

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Kevin Wolf]

Am 11.03.2021 um 12:24 hat Peter Krempa geschrieben:
> On Thu, Mar 11, 2021 at 09:37:11 +0100, Kevin Wolf wrote:
> > Am 11.03.2021 um 08:47 hat Peter Krempa geschrieben:
> > > On Wed, Mar 10, 2021 at 18:30:44 +0100, Kevin Wolf wrote:
> > > > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben:
> > > > > On 10/03/21 15:22, Peter Krempa wrote:
> 
> [...]
> 
> > > -object memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind
> > 
> > Oh, we have ranges, too... That makes the compatibility code even
> > nastier to write. I doubt that we can implement this in the keyval
> > parser alone without touching the visitor. :-/
> > 
> > > If any of the above is to be deprecated we'll need to adjust our
> > > JSON->commandline generator accordignly.
> > > 
> > > Luckily the 'host-nodes' is storeable as a bitmap and the generator is
> > > actually modular to allow plugging an array interpretor which actually
> > > does the above conversion from a JSON array.
> > > 
> > > So, what is the preferred syntax here? Additionally we might need a
> > > witness property to detect when to use the new syntax if basing it on
> > > the object-add qapification will not be enough.
> > 
> > The list syntax supported by the keyval parser is the one you know from
> > -blockdev: host-nodes.0=3,host-nodes.1=7, ...
> 
> I think that should be easy enough to convert to.

We could also support JSON syntax in QEMU. That would probably be even
more convenient for libvirt?

> Is it safe to do right away (with the old parser?). Otherwise we need
> to agree on something which will let us detect when it's safe to
> change.

Neither keyval nor JSON syntax work with the old QemuOpts parser.

What is the usual way to do this for command line options? If we don't
have a good way there, we can always tie it to something in the QAPI
schema. If we still get this solved in time for 6.0, we could use the
existence of ObjectOptions. Or all else failing, we can use a feature
flag.

Kevin

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Peter Krempa]

On Thu, Mar 11, 2021 at 12:41:42 +0100, Kevin Wolf wrote:
> Am 11.03.2021 um 12:24 hat Peter Krempa geschrieben:
> > On Thu, Mar 11, 2021 at 09:37:11 +0100, Kevin Wolf wrote:
> > > Am 11.03.2021 um 08:47 hat Peter Krempa geschrieben:
> > > > On Wed, Mar 10, 2021 at 18:30:44 +0100, Kevin Wolf wrote:
> > > > > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben:
> > > > > > On 10/03/21 15:22, Peter Krempa wrote:
> > 
> > [...]
> > 
> > > > -object memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind
> > > 
> > > Oh, we have ranges, too... That makes the compatibility code even
> > > nastier to write. I doubt that we can implement this in the keyval
> > > parser alone without touching the visitor. :-/
> > > 
> > > > If any of the above is to be deprecated we'll need to adjust our
> > > > JSON->commandline generator accordignly.
> > > > 
> > > > Luckily the 'host-nodes' is storeable as a bitmap and the generator is
> > > > actually modular to allow plugging an array interpretor which actually
> > > > does the above conversion from a JSON array.
> > > > 
> > > > So, what is the preferred syntax here? Additionally we might need a
> > > > witness property to detect when to use the new syntax if basing it on
> > > > the object-add qapification will not be enough.
> > > 
> > > The list syntax supported by the keyval parser is the one you know from
> > > -blockdev: host-nodes.0=3,host-nodes.1=7, ...
> > 
> > I think that should be easy enough to convert to.
> 
> We could also support JSON syntax in QEMU. That would probably be even
> more convenient for libvirt?

Definitely yes. We already do have the JSON internal representation, so
outputing JSON directly just skips the commandlinificator.


> > Is it safe to do right away (with the old parser?). Otherwise we need
> > to agree on something which will let us detect when it's safe to
> > change.
> 
> Neither keyval nor JSON syntax work with the old QemuOpts parser.
> 
> What is the usual way to do this for command line options? If we don't
> have a good way there, we can always tie it to something in the QAPI
> schema. If we still get this solved in time for 6.0, we could use the
> existence of ObjectOptions. Or all else failing, we can use a feature
> flag.

Yup, in this release I'd use what I have for detecting qapification of
-object. If we can do JSON with this, it would be ideal.

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 11.03.2021 um 12:24 hat Peter Krempa geschrieben:
>> On Thu, Mar 11, 2021 at 09:37:11 +0100, Kevin Wolf wrote:
>> > Am 11.03.2021 um 08:47 hat Peter Krempa geschrieben:
>> > > On Wed, Mar 10, 2021 at 18:30:44 +0100, Kevin Wolf wrote:
>> > > > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben:
>> > > > > On 10/03/21 15:22, Peter Krempa wrote:
>> 
>> [...]
>> 
>> > > -object memory-backend-ram,id=ram-node2,size=24578621440,host-nodes=1-2,host-nodes=5,host-nodes=7,policy=bind
>> > 
>> > Oh, we have ranges, too... That makes the compatibility code even
>> > nastier to write. I doubt that we can implement this in the keyval
>> > parser alone without touching the visitor. :-/
>> > 
>> > > If any of the above is to be deprecated we'll need to adjust our
>> > > JSON->commandline generator accordignly.
>> > > 
>> > > Luckily the 'host-nodes' is storeable as a bitmap and the generator is
>> > > actually modular to allow plugging an array interpretor which actually
>> > > does the above conversion from a JSON array.
>> > > 
>> > > So, what is the preferred syntax here? Additionally we might need a
>> > > witness property to detect when to use the new syntax if basing it on
>> > > the object-add qapification will not be enough.
>> > 
>> > The list syntax supported by the keyval parser is the one you know from
>> > -blockdev: host-nodes.0=3,host-nodes.1=7, ...
>> 
>> I think that should be easy enough to convert to.
>
> We could also support JSON syntax in QEMU. That would probably be even
> more convenient for libvirt?

Cleanly QAPIfied options like -blockdev do

    if (optarg[0] == '{') {
        parse @optarg as JSON with qobject_from_json()
        convert to C type with qobject_input_visitor_new()
    } else {
        parse @optarg with keyval_parse()
        convert to C type with qobject_input_visitor_new_keyval()
    }

Options where compatibility problems defeat keyval_parse() can do

    if (optarg[0] == '{') {
        parse @optarg as JSON with qobject_from_json()
        convert to C type with qobject_input_visitor_new()
    } else {
        parse the old way
        convert to C type somehow
    }

Precedence: -display.  There, the old way is an ad hoc parser, and the
conversion to C type DisplayOptions is manual.  For -object, the old way
would be QemuOpts, and the conversion would use the opts visitor.

>> Is it safe to do right away (with the old parser?). Otherwise we need
>> to agree on something which will let us detect when it's safe to
>> change.
>
> Neither keyval nor JSON syntax work with the old QemuOpts parser.
>
> What is the usual way to do this for command line options? If we don't
> have a good way there, we can always tie it to something in the QAPI
> schema. If we still get this solved in time for 6.0, we could use the
> existence of ObjectOptions. Or all else failing, we can use a feature
> flag.

You should not look for types in output of query-qmp-schema, only for
commands and events.  To discourage looking for types, query-qmp-schema
masks the names of user-defined types.

A feature flag is fine as last resort.  That's what they were designed
for.

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Paolo Bonzini]

On 10/03/21 18:30, Kevin Wolf wrote:
> Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben:
>> On 10/03/21 15:22, Peter Krempa wrote:
>>> I've stumbled upon a regression with this patchset applied:
>>>
>>> error: internal error: process exited while connecting to monitor: qemu-system-x86_64: -object memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind: Invalid parameter type for 'host-nodes', expected: array
>>
>> This is the magic conversion of "string containing a list of integers"
>> to "list of integers".
> 
> Ah, crap. This one wouldn't have been a problem when converting only
> object-add, and I trusted your audit that user creatable objects don't
> depend on any QemuOpts magic. I should have noticed it, too, of course,
> but during the convertion I didn't have QemuOpts in mind, only QOM and
> QAPI.

Yeah, let's just drop the -object conversion for now. It will just 
remove a few patches.

Who is going to include this series in the next pull request, Markus or 
myself?  The time is ticking for soft freeze.

Paolo

> I checked all object types, and it seems this is the only one that is
> affected. We have a second list in AuthZListProperties, but it contains
> structs, so it was never supported on the command line anyway.
> 
>> The relevant code is in qapi/string-input-visitor.c, but I'm not sure where
>> to stick it in the keyval-based parsing flow (i.e.
>> qobject_input_visitor_new_keyval).  Markus, any ideas?
> 
> The best I can think of at the moment would be adding a flag to the
> keyval parser that would enable the feature only for -object (and only
> in the system emulator, because memory-backend-ram doesn't exist in the
> tools):
> 
> The keyval parser would create a list if multiple values are given for
> the same key. Some care needs to be taken to avoid mixing the magic
> list feature with the normal indexed list options.
> 
> The QAPI schema would then use an alternate between 'int' and ['int'],
> with the the memory-backend-ram implementation changed accordingly.
> 
> We could consider immediately deprecating the syntax and printing a
> warning in the keyval parser when it automatically creates a list from
> two values for a key, so that users don't start using this syntax
> instead of the normal list syntax in other places. We'd probably still
> leave the implementation around for -device and other users of the same
> magic.
> 
> Kevin
> 

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Kevin Wolf]

Am 11.03.2021 um 09:14 hat Paolo Bonzini geschrieben:
> On 10/03/21 18:30, Kevin Wolf wrote:
> > Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben:
> > > On 10/03/21 15:22, Peter Krempa wrote:
> > > > I've stumbled upon a regression with this patchset applied:
> > > > 
> > > > error: internal error: process exited while connecting to monitor: qemu-system-x86_64: -object memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind: Invalid parameter type for 'host-nodes', expected: array
> > > 
> > > This is the magic conversion of "string containing a list of integers"
> > > to "list of integers".
> > 
> > Ah, crap. This one wouldn't have been a problem when converting only
> > object-add, and I trusted your audit that user creatable objects don't
> > depend on any QemuOpts magic. I should have noticed it, too, of course,
> > but during the convertion I didn't have QemuOpts in mind, only QOM and
> > QAPI.
> 
> Yeah, let's just drop the -object conversion for now. It will just remove a
> few patches.

I think it's only patch 29 and 30 that we would have to drop, actually.

Unfortunately, that still removes one of the most immediately useful
features, which is non-scalar properties for -object in the system
emulator. But of course, a lot better than not merging it at all.

> Who is going to include this series in the next pull request, Markus or
> myself?  The time is ticking for soft freeze.

QOM is probably the right subsystem, so that would be you. Or I can just
merge it myself as long as everyone is fine with it.

Eric has some minor comments that I think could be addressed while
applying the series. Or should I send a v4 for that (and for dropping
patches 29 and 30)?

Kevin

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Paolo Bonzini]

On 11/03/21 09:45, Kevin Wolf wrote:
> I think it's only patch 29 and 30 that we would have to drop, actually.
> 
> Unfortunately, that still removes one of the most immediately useful
> features, which is non-scalar properties for -object in the system
> emulator. But of course, a lot better than not merging it at all.
> 
>> Who is going to include this series in the next pull request, Markus or
>> myself?  The time is ticking for soft freeze.
> QOM is probably the right subsystem, so that would be you. Or I can just
> merge it myself as long as everyone is fine with it.
> 
> Eric has some minor comments that I think could be addressed while
> applying the series. Or should I send a v4 for that (and for dropping
> patches 29 and 30)?

I'd say just send a pull request. :)

Paolo

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 10.03.2021 um 15:31 hat Paolo Bonzini geschrieben:
>> On 10/03/21 15:22, Peter Krempa wrote:
>> > I've stumbled upon a regression with this patchset applied:
>> > 
>> > error: internal error: process exited while connecting to monitor: qemu-system-x86_64: -object memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind: Invalid parameter type for 'host-nodes', expected: array
>> 
>> This is the magic conversion of "string containing a list of integers"
>> to "list of integers".
>
> Ah, crap. This one wouldn't have been a problem when converting only
> object-add, and I trusted your audit that user creatable objects don't
> depend on any QemuOpts magic. I should have noticed it, too, of course,
> but during the convertion I didn't have QemuOpts in mind, only QOM and
> QAPI.
>
> I checked all object types, and it seems this is the only one that is
> affected. We have a second list in AuthZListProperties, but it contains
> structs, so it was never supported on the command line anyway.
>
>> The relevant code is in qapi/string-input-visitor.c, but I'm not sure where
>> to stick it in the keyval-based parsing flow (i.e.
>> qobject_input_visitor_new_keyval).  Markus, any ideas?
>
> The best I can think of at the moment would be adding a flag to the
> keyval parser that would enable the feature only for -object (and only
> in the system emulator, because memory-backend-ram doesn't exist in the
> tools):
>
> The keyval parser would create a list if multiple values are given for
> the same key. Some care needs to be taken to avoid mixing the magic
> list feature with the normal indexed list options.

You're cursing^Wtalking about the wrong list hack, I'm afraid.

QemuOpts can indeed be used in a way so that key=val1,key=val2,... get
collected into a list val1, val2, ...  For an example, see how
qemu_semihosting_config_opts() processes the option argument of
-semihosting: repeated arg=... get collected in array
semihosting.argv[].

QOM property "host-nodes" employs a different hack.  The QAPI schema
defines it as

    { 'struct': 'Memdev',
      'data': {
        ...
        'host-nodes': ['uint16'],
        ... }}

The QObject input visitor treats it like any other list.  To get node 0
and 4, you say

    "host-nodes": [0,4]

with its JSON flavor, and

    host-nodes.0=0,host-nodes.1=4

with its dotted keys flavor.

The string input visitor and the opts visitor only support *scalar*
values, *except* they both have a hack to support lists of small
integers.

With the opts visitor, saying

    host-nodes=0,host-nodes=4

gets you node 0 and 4, and both

    host-nodes=0,host-nodes=1
    host-nodes=0-1

gets you nodes 0 and 1.  This is what parses -object.

Setting NumaNode member @cpus via -numa cpus=... is another user of this
hack.

With the string input visitor, repeating the key doesn't work (there is
no syntax for keys, in fact), but comma does.  This is what parses
-global and HMP qom-set.

The problem Peter reported is that switching -object from QemuOpts +
opts visitor to keyval_parse() + QObject input visitor loses the opts
visitor's list-of-integers hack.

The obvious solution is to transplant the hack to the QObject input
visitor.  "Ich kann nicht soviel fressen wie ich kotzen möchte" (okay, I
better don't translate this; all you need to know is that I find the
idea utterly disgusting).

There is the more general problem of human-friendly syntax support.
QAPI/QMP eschew encoding complex data in strings.  They want you to use
complex data types.

Fine for QMP, machines are generally better off with formatting /
parsing verbose JSON than with formatting / parsing lots of ad hoc
little languages.

Not always fine for humans.  Case in point:
host-nodes.0=0,host-nodes.1=4 is decidedly inferior to something like
host-nodes=0+4[*].

Perhaps we need to provide means to define ad hoc little languages for
human use.  Specify the parsing function to use for human input in the
QAPI schema.

Doesn't really help us now, because we can't pull it off in time for the
soft freeze.

Here's a differently terrible hack.  We have

         keyval_parse()       visitor
    optarg --------> QObject --------> QAPI type

Idea: hack the QObject.  If we're working for -object, and QObject maps
key "qom-type" to value "memory-backend-ram", get the value of
host-nodes, and if it's a string, parse it into a list like the opts
visitor does, and put that back, replacing the string value.

Same for other uses of Memdev and NumaNodeOptions with -object, if they
exist.

I told you it's terrible :)

[...]


[*] 0+4 and not 0,4 because ',' would have to be doubled in dotted key
syntax.

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Paolo Bonzini]

On 11/03/21 11:38, Markus Armbruster wrote:
> Here's a differently terrible hack.  We have
> 
>           keyval_parse()       visitor
>      optarg --------> QObject --------> QAPI type
> 
> Idea: hack the QObject.  If we're working for -object, and QObject maps
> key "qom-type" to value "memory-backend-ram", get the value of
> host-nodes, and if it's a string, parse it into a list like the opts
> visitor does, and put that back, replacing the string value.
> 
> Same for other uses of Memdev and NumaNodeOptions with -object, if they
> exist.

This doesn't help with backwards compatibility, since keyval loses the 
first of host-nodes=0,host-nodes=4.

I would rather keep the OptsVisitor here.  Do the same check for JSON 
syntax that you have in qobject_input_visitor_new_str, and whenever you 
need to walk all -object arguments, use something like this:

     typedef struct ObjectArgument {
         const char *id;
         QDict *json;    /* or NULL for QemuOpts */
         QSIMPLEQ_ENTRY(ObjectArgument) next;
     }

I already had patches in my queue to store -object in a GSList of 
dictionaries, changing it to use the above is easy enough.

Paolo

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 11/03/21 11:38, Markus Armbruster wrote:
>> Here's a differently terrible hack.  We have
>>           keyval_parse()       visitor
>>      optarg --------> QObject --------> QAPI type
>> Idea: hack the QObject.  If we're working for -object, and QObject
>> maps
>> key "qom-type" to value "memory-backend-ram", get the value of
>> host-nodes, and if it's a string, parse it into a list like the opts
>> visitor does, and put that back, replacing the string value.
>> Same for other uses of Memdev and NumaNodeOptions with -object, if
>> they
>> exist.
>
> This doesn't help with backwards compatibility, since keyval loses the
> first of host-nodes=0,host-nodes=4.

You're right, I missed the fact that we rely on both hacks working
together for the full syntax.

> I would rather keep the OptsVisitor here.  Do the same check for JSON
> syntax that you have in qobject_input_visitor_new_str, and whenever
> you need to walk all -object arguments, use something like this:
>
>     typedef struct ObjectArgument {
>         const char *id;
>         QDict *json;    /* or NULL for QemuOpts */
>         QSIMPLEQ_ENTRY(ObjectArgument) next;
>     }
>
> I already had patches in my queue to store -object in a GSList of
> dictionaries, changing it to use the above is easy enough.

I think I'd prefer following -display's precedence.  See my reply to
Kevin for details.

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Paolo Bonzini]

On 11/03/21 15:08, Markus Armbruster wrote:
>> I would rather keep the OptsVisitor here.  Do the same check for JSON
>> syntax that you have in qobject_input_visitor_new_str, and whenever
>> you need to walk all -object arguments, use something like this:
>>
>>      typedef struct ObjectArgument {
>>          const char *id;
>>          QDict *json;    /* or NULL for QemuOpts */
>>          QSIMPLEQ_ENTRY(ObjectArgument) next;
>>      }
>>
>> I already had patches in my queue to store -object in a GSList of
>> dictionaries, changing it to use the above is easy enough.
> I think I'd prefer following -display's precedence.  See my reply to
> Kevin for details.
> 

Yeah, I got independently to the same conclusion and posted patches for 
that.  I was scared that visit_type_ObjectOptions was too much for 
OptsVisitor but it seems to work...

Paolo

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 11/03/21 15:08, Markus Armbruster wrote:
>>> I would rather keep the OptsVisitor here.  Do the same check for JSON
>>> syntax that you have in qobject_input_visitor_new_str, and whenever
>>> you need to walk all -object arguments, use something like this:
>>>
>>>      typedef struct ObjectArgument {
>>>          const char *id;
>>>          QDict *json;    /* or NULL for QemuOpts */
>>>          QSIMPLEQ_ENTRY(ObjectArgument) next;
>>>      }
>>>
>>> I already had patches in my queue to store -object in a GSList of
>>> dictionaries, changing it to use the above is easy enough.
>> 
>> I think I'd prefer following -display's precedence.  See my reply to
>> Kevin for details.
>
> Yeah, I got independently to the same conclusion and posted patches
> for that.  I was scared that visit_type_ObjectOptions was too much for 
> OptsVisitor but it seems to work...

We have reason to be scared.  I'll try to cover this in my review.

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Paolo Bonzini]

On 12/03/21 09:14, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 11/03/21 15:08, Markus Armbruster wrote:
>>>> I would rather keep the OptsVisitor here.  Do the same check for JSON
>>>> syntax that you have in qobject_input_visitor_new_str, and whenever
>>>> you need to walk all -object arguments, use something like this:
>>>>
>>>>       typedef struct ObjectArgument {
>>>>           const char *id;
>>>>           QDict *json;    /* or NULL for QemuOpts */
>>>>           QSIMPLEQ_ENTRY(ObjectArgument) next;
>>>>       }
>>>>
>>>> I already had patches in my queue to store -object in a GSList of
>>>> dictionaries, changing it to use the above is easy enough.
>>>
>>> I think I'd prefer following -display's precedence.  See my reply to
>>> Kevin for details.
>>
>> Yeah, I got independently to the same conclusion and posted patches
>> for that.  I was scared that visit_type_ObjectOptions was too much for
>> OptsVisitor but it seems to work...
> 
> We have reason to be scared.  I'll try to cover this in my review.

Yes, it's a good reason to possibly even delay those 3 patches to 6.1.

Paolo

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Peter Krempa]

On Fri, Mar 12, 2021 at 09:46:54 +0100, Paolo Bonzini wrote:
> On 12/03/21 09:14, Markus Armbruster wrote:
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> > 
> > > On 11/03/21 15:08, Markus Armbruster wrote:
> > > > > I would rather keep the OptsVisitor here.  Do the same check for JSON
> > > > > syntax that you have in qobject_input_visitor_new_str, and whenever
> > > > > you need to walk all -object arguments, use something like this:
> > > > > 
> > > > >       typedef struct ObjectArgument {
> > > > >           const char *id;
> > > > >           QDict *json;    /* or NULL for QemuOpts */
> > > > >           QSIMPLEQ_ENTRY(ObjectArgument) next;
> > > > >       }
> > > > > 
> > > > > I already had patches in my queue to store -object in a GSList of
> > > > > dictionaries, changing it to use the above is easy enough.
> > > > 
> > > > I think I'd prefer following -display's precedence.  See my reply to
> > > > Kevin for details.
> > > 
> > > Yeah, I got independently to the same conclusion and posted patches
> > > for that.  I was scared that visit_type_ObjectOptions was too much for
> > > OptsVisitor but it seems to work...
> > 
> > We have reason to be scared.  I'll try to cover this in my review.
> 
> Yes, it's a good reason to possibly even delay those 3 patches to 6.1.

Is there a chance we could get the json syntax for -object for now? I
think it would simplify libvirt's code a bit and sidestep the issue of
converting the already existing parameters from JSON form we have into
the commandline form.

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 11/03/21 15:08, Markus Armbruster wrote:
>>>> I would rather keep the OptsVisitor here.  Do the same check for JSON
>>>> syntax that you have in qobject_input_visitor_new_str, and whenever
>>>> you need to walk all -object arguments, use something like this:
>>>>
>>>>      typedef struct ObjectArgument {
>>>>          const char *id;
>>>>          QDict *json;    /* or NULL for QemuOpts */
>>>>          QSIMPLEQ_ENTRY(ObjectArgument) next;
>>>>      }
>>>>
>>>> I already had patches in my queue to store -object in a GSList of
>>>> dictionaries, changing it to use the above is easy enough.
>>> 
>>> I think I'd prefer following -display's precedence.  See my reply to
>>> Kevin for details.
>>
>> Yeah, I got independently to the same conclusion and posted patches
>> for that.  I was scared that visit_type_ObjectOptions was too much for 
>> OptsVisitor but it seems to work...
>
> We have reason to be scared.  I'll try to cover this in my review.

The opts visitor has serious limitations.  From its header:

 * The Opts input visitor does not implement support for visiting QAPI
 * alternates, numbers (other than integers), null, or arbitrary
 * QTypes.  It also requires a non-null list argument to
 * visit_start_list().

This is retro-documentation for hairy code.  I don't trust it.  Commit
eb7ee2cbeb "qapi: introduce OptsVisitor" hints at additional
restrictions:

    The type tree in the schema, corresponding to an option with a
    discriminator, must have the following structure:
    
      struct
        scalar member for non-discriminated optarg 1 [*]
        list for repeating non-discriminated optarg 2 [*]
          wrapper struct
            single scalar member
        union
          struct for discriminator case 1
            scalar member for optarg 3 [*]
            list for repeating optarg 4 [*]
              wrapper struct
                single scalar member
            scalar member for optarg 5 [*]
          struct for discriminator case 2
            ...
    
    The "type" optarg name is fixed for the discriminator role. Its schema
    representation is "union of structures", and each discriminator value must
    correspond to a member name in the union.
    
    If the option takes no "type" descriminator, then the type subtree rooted
    at the union must be absent from the schema (including the union itself).
    
    Optarg values can be of scalar types str / bool / integers / size.

Unsupported visits are treated as programming error.  Which is a nice
way to say "they crash".

Before this series, we use it for -object as follows.

user_creatable_add_opts() massages the QemuOpts into a QDict containing
just the properties, then calls user_creatable_add_type() with the opts
visitor wrapped around the QemuOpts, and the QDict.

user_creatable_add_type() performs a virtual visit.  The outermost
object it visits itself.  Then it visits members one by one by calling
object_property_set().  It uses the QDict as a list of members to visit.

As long as the object_property_set() only visit scalars other than
floating-point numbers, we safely stay with the opts visitors'
limitations.

After this series, we use the opts visitor to convert the option
argument to a ObjectOption.  This is a non-virtual visit.  We then
convert the ObjectOption to a QDict, and call user_creatable_add_type()
with the QObject input visitor wrapped around the QDict, and the QDict.

Here's the difference in opts visitor use: before the patch, we visit
exactly the members in the optarg that actually name QOM properties (for
the ones that don't, object_property_set() fails without visiting
anything).  Afterwards, we visit the members of ObjectOption, i.e.
all QOM properties, by construction of ObjectOption.

As long as ObjectOption's construction is correct, the series does not
add new visits, i.e. we're no worse off than before.

However, there is now a new way to mess things up: you can change (a
branch of union) ObjectOption in a way that pushes it beyond the opts
visitors limitations.  QMP and tools --object will continue to work, but
qemu-system-FOO -object will crash.

As is, HMP object_add doesn't crash, because it doesn't use the opts
visitor anymore, which breaks backward compatibility.  If we rever to
the opts visitor there, it'll crash as well.

New ways to mess things up are always kind of unwelcome.  This one
doesn't sound *too* dangerous; we "only" have to ensure -object is
tested thoroughly.  Still, comments next to the QAPI definitions that
must not be messed up would be nice.

Paolo, Kevin, any comments?

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Kevin Wolf]

Am 13.03.2021 um 14:40 hat Markus Armbruster geschrieben:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> >
> >> On 11/03/21 15:08, Markus Armbruster wrote:
> >>>> I would rather keep the OptsVisitor here.  Do the same check for JSON
> >>>> syntax that you have in qobject_input_visitor_new_str, and whenever
> >>>> you need to walk all -object arguments, use something like this:
> >>>>
> >>>>      typedef struct ObjectArgument {
> >>>>          const char *id;
> >>>>          QDict *json;    /* or NULL for QemuOpts */
> >>>>          QSIMPLEQ_ENTRY(ObjectArgument) next;
> >>>>      }
> >>>>
> >>>> I already had patches in my queue to store -object in a GSList of
> >>>> dictionaries, changing it to use the above is easy enough.
> >>> 
> >>> I think I'd prefer following -display's precedence.  See my reply to
> >>> Kevin for details.
> >>
> >> Yeah, I got independently to the same conclusion and posted patches
> >> for that.  I was scared that visit_type_ObjectOptions was too much for 
> >> OptsVisitor but it seems to work...
> >
> > We have reason to be scared.  I'll try to cover this in my review.
> 
> The opts visitor has serious limitations.  From its header:
> 
>  * The Opts input visitor does not implement support for visiting QAPI
>  * alternates, numbers (other than integers), null, or arbitrary
>  * QTypes.  It also requires a non-null list argument to
>  * visit_start_list().
> 
> This is retro-documentation for hairy code.  I don't trust it.  Commit
> eb7ee2cbeb "qapi: introduce OptsVisitor" hints at additional
> restrictions:
> 
>     The type tree in the schema, corresponding to an option with a
>     discriminator, must have the following structure:
>     
>       struct
>         scalar member for non-discriminated optarg 1 [*]
>         list for repeating non-discriminated optarg 2 [*]
>           wrapper struct
>             single scalar member
>         union
>           struct for discriminator case 1
>             scalar member for optarg 3 [*]
>             list for repeating optarg 4 [*]
>               wrapper struct
>                 single scalar member
>             scalar member for optarg 5 [*]
>           struct for discriminator case 2
>             ...

Is this a long-winded way of saying that it has to be flat, except that
it allows lists, i.e. there must be no nested objects on the "wire"?

The difference between structs and unions, and different branches inside
the union isn't visible for the visitor anyway.

>     The "type" optarg name is fixed for the discriminator role. Its schema
>     representation is "union of structures", and each discriminator value must
>     correspond to a member name in the union.
>     
>     If the option takes no "type" descriminator, then the type subtree rooted
>     at the union must be absent from the schema (including the union itself).
>     
>     Optarg values can be of scalar types str / bool / integers / size.
> 
> Unsupported visits are treated as programming error.  Which is a nice
> way to say "they crash".

The OptsVisitor never seems to crash explicitly by calling something
like abort().

It may crash because of missing callbacks that are called without a NULL
check, like v->type_null. This case should probably be fixed in
qapi/qapi-visit-core.c to do the check and simply return an error.

Any other cases?

> Before this series, we use it for -object as follows.
> 
> user_creatable_add_opts() massages the QemuOpts into a QDict containing
> just the properties, then calls user_creatable_add_type() with the opts
> visitor wrapped around the QemuOpts, and the QDict.
> 
> user_creatable_add_type() performs a virtual visit.  The outermost
> object it visits itself.  Then it visits members one by one by calling
> object_property_set().  It uses the QDict as a list of members to visit.
> 
> As long as the object_property_set() only visit scalars other than
> floating-point numbers, we safely stay with the opts visitors'
> limitations.

Minor addition: This visits inside object_property_set() are
non-virtual, of course.

> After this series, we use the opts visitor to convert the option
> argument to a ObjectOption.  This is a non-virtual visit.  We then
> convert the ObjectOption to a QDict, and call user_creatable_add_type()
> with the QObject input visitor wrapped around the QDict, and the QDict.
> 
> Here's the difference in opts visitor use: before the patch, we visit
> exactly the members in the optarg that actually name QOM properties (for
> the ones that don't, object_property_set() fails without visiting
> anything).  Afterwards, we visit the members of ObjectOption, i.e.
> all QOM properties, by construction of ObjectOption.
> 
> As long as ObjectOption's construction is correct, the series does not
> add new visits, i.e. we're no worse off than before.
> 
> However, there is now a new way to mess things up: you can change (a
> branch of union) ObjectOption in a way that pushes it beyond the opts
> visitors limitations.  QMP and tools --object will continue to work, but
> qemu-system-FOO -object will crash.

I don't think this is very concerning because the primary way to test
changes to objects is probably -object in the system emulator. So I
think we're lucky enough to have the problem in the most obvious place.

> As is, HMP object_add doesn't crash, because it doesn't use the opts
> visitor anymore, which breaks backward compatibility.  If we rever to
> the opts visitor there, it'll crash as well.
> 
> New ways to mess things up are always kind of unwelcome.  This one
> doesn't sound *too* dangerous; we "only" have to ensure -object is
> tested thoroughly.  Still, comments next to the QAPI definitions that
> must not be messed up would be nice.
> 
> Paolo, Kevin, any comments?

We probably agree that using QemuOpts and the OptsVisitor is only a
stopgap solution for 6.0 anyway. Instead of investing a lot of thought
into how we can make this maintainable for the long term (which isn't
something we want to do anyway), let's put that work into making the
keyval visitor work for the system emulator.

Kevin

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 13.03.2021 um 14:40 hat Markus Armbruster geschrieben:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>> > Paolo Bonzini <pbonzini@redhat.com> writes:
>> >
>> >> On 11/03/21 15:08, Markus Armbruster wrote:
>> >>>> I would rather keep the OptsVisitor here.  Do the same check for JSON
>> >>>> syntax that you have in qobject_input_visitor_new_str, and whenever
>> >>>> you need to walk all -object arguments, use something like this:
>> >>>>
>> >>>>      typedef struct ObjectArgument {
>> >>>>          const char *id;
>> >>>>          QDict *json;    /* or NULL for QemuOpts */
>> >>>>          QSIMPLEQ_ENTRY(ObjectArgument) next;
>> >>>>      }
>> >>>>
>> >>>> I already had patches in my queue to store -object in a GSList of
>> >>>> dictionaries, changing it to use the above is easy enough.
>> >>> 
>> >>> I think I'd prefer following -display's precedence.  See my reply to
>> >>> Kevin for details.
>> >>
>> >> Yeah, I got independently to the same conclusion and posted patches
>> >> for that.  I was scared that visit_type_ObjectOptions was too much for 
>> >> OptsVisitor but it seems to work...
>> >
>> > We have reason to be scared.  I'll try to cover this in my review.
>> 
>> The opts visitor has serious limitations.  From its header:
>> 
>>  * The Opts input visitor does not implement support for visiting QAPI
>>  * alternates, numbers (other than integers), null, or arbitrary
>>  * QTypes.  It also requires a non-null list argument to
>>  * visit_start_list().
>> 
>> This is retro-documentation for hairy code.  I don't trust it.  Commit
>> eb7ee2cbeb "qapi: introduce OptsVisitor" hints at additional
>> restrictions:
>> 
>>     The type tree in the schema, corresponding to an option with a
>>     discriminator, must have the following structure:
>>     
>>       struct
>>         scalar member for non-discriminated optarg 1 [*]
>>         list for repeating non-discriminated optarg 2 [*]
>>           wrapper struct
>>             single scalar member
>>         union
>>           struct for discriminator case 1
>>             scalar member for optarg 3 [*]
>>             list for repeating optarg 4 [*]
>>               wrapper struct
>>                 single scalar member
>>             scalar member for optarg 5 [*]
>>           struct for discriminator case 2
>>             ...
>
> Is this a long-winded way of saying that it has to be flat, except that
> it allows lists, i.e. there must be no nested objects on the "wire"?

I think so.

> The difference between structs and unions, and different branches inside
> the union isn't visible for the visitor anyway.

Yes, only the code using the visitor deals with that.

>>     The "type" optarg name is fixed for the discriminator role. Its schema
>>     representation is "union of structures", and each discriminator value must
>>     correspond to a member name in the union.
>>     
>>     If the option takes no "type" descriminator, then the type subtree rooted
>>     at the union must be absent from the schema (including the union itself).
>>     
>>     Optarg values can be of scalar types str / bool / integers / size.
>> 
>> Unsupported visits are treated as programming error.  Which is a nice
>> way to say "they crash".
>
> The OptsVisitor never seems to crash explicitly by calling something
> like abort().
>
> It may crash because of missing callbacks that are called without a NULL
> check, like v->type_null.

Correct.

>                           This case should probably be fixed in
> qapi/qapi-visit-core.c to do the check and simply return an error.

I retro-documented what I inherited: qapi-visit-core.c code expects the
visitors to implement the full visitor-impl.h interface, but some
visitors don't.  So I documented "method must be set to visit FOOs" in
visitor-impl.h, and for the visitors that don't, I documented "can't
visit FOOs".

If the crashing behavior we've always had gets in the way, there are two
ways to change it:

1. Complicate qapi-visit-core.c slightly to cope with incomplete visitor
   implementations.

2. Complete the visitor implementations: add dummy callbacks that fail.

I prefer 2., because I feel it keeps the visitor-impl.h interface
simpler, and puts the extra complications where they belong.

> Any other cases?

I don't think so.

>> Before this series, we use it for -object as follows.
>> 
>> user_creatable_add_opts() massages the QemuOpts into a QDict containing
>> just the properties, then calls user_creatable_add_type() with the opts
>> visitor wrapped around the QemuOpts, and the QDict.
>> 
>> user_creatable_add_type() performs a virtual visit.  The outermost
>> object it visits itself.  Then it visits members one by one by calling
>> object_property_set().  It uses the QDict as a list of members to visit.
>> 
>> As long as the object_property_set() only visit scalars other than
>> floating-point numbers, we safely stay with the opts visitors'
>> limitations.
>
> Minor addition: This visits inside object_property_set() are
> non-virtual, of course.

Yes.

>> After this series, we use the opts visitor to convert the option
>> argument to a ObjectOption.  This is a non-virtual visit.  We then
>> convert the ObjectOption to a QDict, and call user_creatable_add_type()
>> with the QObject input visitor wrapped around the QDict, and the QDict.
>> 
>> Here's the difference in opts visitor use: before the patch, we visit
>> exactly the members in the optarg that actually name QOM properties (for
>> the ones that don't, object_property_set() fails without visiting
>> anything).  Afterwards, we visit the members of ObjectOption, i.e.
>> all QOM properties, by construction of ObjectOption.
>> 
>> As long as ObjectOption's construction is correct, the series does not
>> add new visits, i.e. we're no worse off than before.
>> 
>> However, there is now a new way to mess things up: you can change (a
>> branch of union) ObjectOption in a way that pushes it beyond the opts
>> visitors limitations.  QMP and tools --object will continue to work, but
>> qemu-system-FOO -object will crash.
>
> I don't think this is very concerning because the primary way to test
> changes to objects is probably -object in the system emulator. So I
> think we're lucky enough to have the problem in the most obvious place.
>
>> As is, HMP object_add doesn't crash, because it doesn't use the opts
>> visitor anymore, which breaks backward compatibility.  If we rever to
>> the opts visitor there, it'll crash as well.
>> 
>> New ways to mess things up are always kind of unwelcome.  This one
>> doesn't sound *too* dangerous; we "only" have to ensure -object is
>> tested thoroughly.  Still, comments next to the QAPI definitions that
>> must not be messed up would be nice.
>> 
>> Paolo, Kevin, any comments?
>
> We probably agree that using QemuOpts and the OptsVisitor is only a
> stopgap solution for 6.0 anyway. Instead of investing a lot of thought
> into how we can make this maintainable for the long term (which isn't
> something we want to do anyway), let's put that work into making the
> keyval visitor work for the system emulator.

Yes, we want to retire the opts visitor.

Aside: and I dislike the string visitors, too.

Re: [PATCH v3 00/30] qapi/qom: QAPIfy --object and object-add

[Kevin Wolf]

Am 15.03.2021 um 16:26 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 13.03.2021 um 14:40 hat Markus Armbruster geschrieben:
> >> Markus Armbruster <armbru@redhat.com> writes:
> >> 
> >> > Paolo Bonzini <pbonzini@redhat.com> writes:
> >> >
> >> >> On 11/03/21 15:08, Markus Armbruster wrote:
> >> >>>> I would rather keep the OptsVisitor here.  Do the same check for JSON
> >> >>>> syntax that you have in qobject_input_visitor_new_str, and whenever
> >> >>>> you need to walk all -object arguments, use something like this:
> >> >>>>
> >> >>>>      typedef struct ObjectArgument {
> >> >>>>          const char *id;
> >> >>>>          QDict *json;    /* or NULL for QemuOpts */
> >> >>>>          QSIMPLEQ_ENTRY(ObjectArgument) next;
> >> >>>>      }
> >> >>>>
> >> >>>> I already had patches in my queue to store -object in a GSList of
> >> >>>> dictionaries, changing it to use the above is easy enough.
> >> >>> 
> >> >>> I think I'd prefer following -display's precedence.  See my reply to
> >> >>> Kevin for details.
> >> >>
> >> >> Yeah, I got independently to the same conclusion and posted patches
> >> >> for that.  I was scared that visit_type_ObjectOptions was too much for 
> >> >> OptsVisitor but it seems to work...
> >> >
> >> > We have reason to be scared.  I'll try to cover this in my review.
> >> 
> >> The opts visitor has serious limitations.  From its header:
> >> 
> >>  * The Opts input visitor does not implement support for visiting QAPI
> >>  * alternates, numbers (other than integers), null, or arbitrary
> >>  * QTypes.  It also requires a non-null list argument to
> >>  * visit_start_list().
> >> 
> >> This is retro-documentation for hairy code.  I don't trust it.  Commit
> >> eb7ee2cbeb "qapi: introduce OptsVisitor" hints at additional
> >> restrictions:
> >> 
> >>     The type tree in the schema, corresponding to an option with a
> >>     discriminator, must have the following structure:
> >>     
> >>       struct
> >>         scalar member for non-discriminated optarg 1 [*]
> >>         list for repeating non-discriminated optarg 2 [*]
> >>           wrapper struct
> >>             single scalar member
> >>         union
> >>           struct for discriminator case 1
> >>             scalar member for optarg 3 [*]
> >>             list for repeating optarg 4 [*]
> >>               wrapper struct
> >>                 single scalar member
> >>             scalar member for optarg 5 [*]
> >>           struct for discriminator case 2
> >>             ...
> >
> > Is this a long-winded way of saying that it has to be flat, except that
> > it allows lists, i.e. there must be no nested objects on the "wire"?
> 
> I think so.
> 
> > The difference between structs and unions, and different branches inside
> > the union isn't visible for the visitor anyway.
> 
> Yes, only the code using the visitor deals with that.
> 
> >>     The "type" optarg name is fixed for the discriminator role. Its schema
> >>     representation is "union of structures", and each discriminator value must
> >>     correspond to a member name in the union.
> >>     
> >>     If the option takes no "type" descriminator, then the type subtree rooted
> >>     at the union must be absent from the schema (including the union itself).
> >>     
> >>     Optarg values can be of scalar types str / bool / integers / size.
> >> 
> >> Unsupported visits are treated as programming error.  Which is a nice
> >> way to say "they crash".
> >
> > The OptsVisitor never seems to crash explicitly by calling something
> > like abort().
> >
> > It may crash because of missing callbacks that are called without a NULL
> > check, like v->type_null.
> 
> Correct.
> 
> >                           This case should probably be fixed in
> > qapi/qapi-visit-core.c to do the check and simply return an error.
> 
> I retro-documented what I inherited: qapi-visit-core.c code expects the
> visitors to implement the full visitor-impl.h interface, but some
> visitors don't.  So I documented "method must be set to visit FOOs" in
> visitor-impl.h, and for the visitors that don't, I documented "can't
> visit FOOs".
> 
> If the crashing behavior we've always had gets in the way, there are two
> ways to change it:
> 
> 1. Complicate qapi-visit-core.c slightly to cope with incomplete visitor
>    implementations.
> 
> 2. Complete the visitor implementations: add dummy callbacks that fail.
> 
> I prefer 2., because I feel it keeps the visitor-impl.h interface
> simpler, and puts the extra complications where they belong.

I suggested making the callbacks optional because I expected that there
might be more than one visitor that doesn't support a callback and I
wouldn't like duplicating dummy callbacks in multiple places. But if
it's only the OptsVisitor, then we wouldn't get any duplication either
way and it becomes a matter of taste.

Kevin