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

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

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?

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.

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

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

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

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

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.

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

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.

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.

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
> 

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

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

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.

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

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.

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

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.

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

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?

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

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.

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