Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Nov 18, 2020 at 12:28:45PM +0100, Kevin Wolf wrote:
>> Am 18.11.2020 um 09:36 hat Markus Armbruster geschrieben:
>> > >> >>                                       [...]  Even better would be
>> > >> >> returning an array of KvmInfo with information on all supported
>> > >> >> accelerators at once, rather than making the user call this command 
>> > >> >> once
>> > >> >> per name.
>> > >> >
>> > >> > Maybe.  It would save us the work of answering the question
>> > >> > above, but is this (querying information for all accelerators at
>> > >> > once) going to be a common use case?
>> > >> 
>> > >> I recommend to scratch the query-accel parameter, and return information
>> > >> on all accelerators in this build of QEMU.  Simple, and consistent with
>> > >> similar ad hoc queries.  If a client is interested in just one, fishing
>> > >> it out of the list is easy enough.
>> > >> 
>> > >
>> > > Works for me. I'll then leave KvmInfo as is and will introduce AccelInfo
>> > > with two fields: name and enabled. enabled will be true only for
>> > > currently active accelerator.
>> > 
>> > Please document that at most on result can have 'enabled': true.
>> 
>> This doesn't feel right.
>> 
>> If I understand right, the proposal is to get a result like this:
>> 
>>     [ { 'name': 'kvm', 'enabled': true },
>>       { 'name': 'tcg', 'enabled': false },
>>       { 'name': 'xen', 'enabled': false } ]
>> 
>> The condition that only one field can be enabled would only be in the
>> documentation instead of being enforced.
>> 
>> Instead, consider a response like this:
>> 
>>     { 'available': [ 'kvm', 'tcg', 'xen' ],
>>       'active': 'kvm' }
>> 
>> This is not only shorter, but also makes sure that only one accelerator
>> can be reported as active.

Better.

Documentation only, not enforced: no duplicate array elements.  We got
that elsewhere already (arrays used as sets, basically).

If we want to provide for reporting additional information on available
accelerators, tweak it to

      {"available": [{"name": "kvm"}, {"name": "tcg"}, {"name": "xen"}],
       "active": "kvm"}

If accelerator-specific extra information is needed, the array elements
can compatibly evolve into flat unions.

Another way to skin this cat:

      {"available": {"kvm": { extra properties... },
                     "tcg": ...,
                     "xen": ...},
       "active": "kvm"}

No need for unions then.  "No dupes" is enforced.

We could inline "available":

      {"kvm": { extra properties... },
       "tcg": ...,
       "xen": ...,
       "active": "kvm"}

Future accelerators can't be named "active" then.

> I guess this can be extended with a union to report extra props for the
> accelerator, discriminated on the 'active' field eg
>
>      { 'available': [ 'kvm', 'tcg', 'xen' ],
>        'active': 'kvm',
>        'data': {
>            "allow-nested": true,
>        }
>     }

Correct.