Re: [PULL 13/29] qapi: Flatten object-add

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 08/07/20 18:05, Kevin Wolf wrote:
>> Markus was going to introduce new QAPI schema syntax that would allow to
>> specify a few options explicitly and then one option for "the rest" that
>> would just be mapped to a QDict like "any" or "gen": false, and that
>> wouldn't require any nesting.
>
> Oh, I wasn't aware of that.  That would be something like 'properties':
> 'remainder' I guess.  That would be fine too.

Glad I'm spared a design argument ;)

>> I'm not sure if any progress was made there, but making things

Not yet; error handling ate me whole, and spit me out only the day
before yesterday or so.

>> consistent between device_add, netdev_add and object_add was a step in
>> this direction anyway.

Yes.

Permit me to digress.

We ran into the "and also arbitray properties" need repeatedly, and
tried several solutions over time.  QAPI/QMP is big, and the left hand
often wasn't too interested in what the right hand had been doing.

If memory and my git archaeology skills serve, the first instance was
device_add:

    3418bd25e1 "qdev hotplug: infrastructure and monitor commands."
    2009-10-05

Simple adaption of the QemuOpts-based -device for HMP.  Since QemuOpts
is flat, so is device_add.  QAPI didn't exist, yet.  A QMP version
followed:

    8bc27249f0 "monitor: convert do_device_add() to QObject"
    2010-03-16

Just as flat.

Next was netdev_add:

    ae82d3242d "monitor: New commands netdev_add, netdev_del"
    2010-04-18

Flat for the same reason.

When QAPI came along in

    ebffe2afce "Merge remote-tracking branch 'qmp/queue/qmp' into
    staging"
    2011-10-10

these two commands were left unQAPIfied, as the schema language could
not express "and also arbitrary properties".  Soon "solved" with a cop
out:

    5dbee474f3 "qapi: allow a 'gen' key to suppress code generation"
    2011-12-15

A half-hearted QAPIfication of netdev_add followed:

    928059a37b "qapi: convert netdev_add"
    2012-06-04

QAPI schema:

    { 'command': 'netdev_add',
      'data': {'type': 'str', 'id': 'str', '*props': '**'},
      'gen': 'no' }

Note the bogus type '**', which only works because 'gen': 'no' also
bypasses type checking (which you wouldn't guess from the commit
message, documentation, or even the schema).  In fact, the whole 'props'
thing is a lie: there is no such parameter, the command is as flat as it
ever was.  Fixed in

    b8a98326d5 "qapi-schema: Fix up misleading specification of
    netdev_add"
    2015-09-21

    { 'command': 'netdev_add',
      'data': {'type': 'str', 'id': 'str'},
      'gen': false }

but by then it had already spread to object-add, with an equally bogus
type 'dict':

    cff8b2c6fc "monitor: add object-add (QMP) and object_add (HMP)
    command"
    2014-01-06

    { 'command': 'object-add',
      'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'},
      'gen': 'no' }

Only 'props' was real this time: you really had to wrap the properties
in "props": { ... }.  Non-flat.  Meh.

Eventually, even device_add made it into the schema:

    94cfd07f26 "qapi-schema: add 'device_add'"
    2016-09-19

    { 'command': 'device_add',
      'data': {'driver': 'str', 'id': 'str'},
      'gen': false } # so we can get the additional arguments

And netdev_add was finally done right:

    db2a380c84 "net: Complete qapi-fication of netdev_add"
    2020-03-17

    { 'command': 'netdev_add', 'data': 'Netdev', 'boxed': true }

Doing device_add and object-add right is harder (impractical?), because
their schema would be a union with one branch per device / object type.

End of digression.

>>> As an aside, it would have been nice to run this through Markus and me,
>>> though in all fairness I'm not sure I would have been responsive back
>>> in February.
>> It went through my tree because of the other patches in the series, but
>> I wrote this patch specifically at Markus's request.

Yes.  We discussed how to best satisfy Kevin's immediate needs without
making other problems harder.  Perhaps we should have posted a summary
to the list.

>>> I would like to un-deprecate this for 5.1, and revert it in either 5.1
>>> or 5.2.  (Also I will be away next week, so the decision would have to
>>> be taken quickly).
>> Please discuss it with Markus then.