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

[Markus Armbruster <armbru@redhat.com>]

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.