Re: [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command

[Markus Armbruster]

Michael Roth <address@hidden> writes:

> Quoting Markus Armbruster (2013-12-17 01:20:16)
>> [Cc: Anthony, Mike for QAPI schema expertise]
>> 
>> Luiz Capitulino <address@hidden> writes:
>> 
>> > On Tue, 10 Dec 2013 19:15:05 +0100
>> > Paolo Bonzini <address@hidden> wrote:
>> >
>> >> -----BEGIN PGP SIGNED MESSAGE-----
>> >> Hash: SHA1
>> >> 
>> >> Il 10/12/2013 19:00, Eric Blake ha scritto:
>> >> >>> +  'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'}, 
>> >> >>> +  'gen': 'no' }
>> >> > 
>> >> > This feels VERY open-coded.  No where else in qapi-schema do we 
>> >> > have 'dict' as a type
>> >> 
>> >> Yes, in fact the "data" field is entirely skipped by the code
>> >> generator (that's 'gen':'no').
>> >> 
>> >> > ; using it violates all sorts of type-safety (which, I guess, is 
>> >> > the point), making it impossible to introspect what keys are valid
>> >> >  for use in the "props":{...} dictionary.  Do we really want to 
>> >> > play this fast and loose with the type system, or should we try 
>> >> > harder to make this a robust self-describing union of types?
>> >> > 
>> >> > That is, why can't we have object-add use a discriminated union, 
>> >> > where qom-type is the discriminator, and where props is an 
>> >> > appropriate JSON struct type that corresponds to the branch of the
>> >> >  union, so that we get full introspection on the set of valid keys
>> >> >  to put in props for any given qom-type?
>> >> 
>> >> The point of "props" is passing arbitrary data to a QOM object.  We
>> >> should indeed have introspection for QOM objects, where each QOM class
>> >> name can be introspected separately.  However, the union of all
>> >> possible QOM objects need not have a "C struct" representation.
>> >
>> > The "props" key was added to represent the "O" argument type of
>> > early QMP (which is used by commands like device_add), so that
>> > we could convert them to the QAPI. IIRC, we didn't plan for it
>> > to be used by new commands... But I don't have anything better
>> > to suggest, so I won't object to its usage here.
>> 
>> We created monitor argument type "O" to have name=val,... arguments in
>> the human monitor exactly like command line option arguments.  Currently
>> used by device_add and netdev_add.
>> 
>> We shoehorned type "O" into QMP in a bout of QMP feature-completeness
>> desperation.  This was before QAPI.
>> 
>> device_add still isn't in qapi-schema.json, but netdev_add is:
>> 
>> { 'command': 'netdev_add',
>>   'data': {'type': 'str', 'id': 'str', '*props': '**'},
>>   'gen': 'no' }
>> 
>> Note the magic "'*props': '**'" (I'll be hanged if I know what that
>> means[*]), and "'gen': 'no'".
>> 
>> Yes, a proper schema for netdev_add and device_add is desirable.  In
>> both cases (but especially for device_add), the arguments are the
>> obligatory id plus a union discriminated by the device type, contraining
>> that device's properties.
>> 
>> Unless we move device properties definition to qapi-schema.json (bad),
>> or duplicate them there (worse), we need to derive that part of the
>> schema dynamically from device information available in QOM.
>
> Is dumping static properties based on class name sufficient, or do we
> need introspection for dynamic properties as well? (or are those not
> exposed outside of qom-set?)

Can't say whether introspection limited to static properties would be
useful, because I don't actually know how static and dynamic properties
are used.

I challenged the need for dynamic properties in the early QOM days,
because they lead to dynamic schemata and complicate introspection, but
Anthony assured us we cannot do without them.

>                              We could maybe introduce a QAPI 'built-in'
> such as 'ObjectProperties' that automatically does the query based on the
> now-special 'type' param and handles all the type-checking up-front. This
> would avoid an open-ended 'dict' type proliferating too much and provide
> infrastructure for introspection.

I'm afraid I'm too ignorant to understand what you're proposing.

>> [*] Can we have a definition of QAPI schema semantics other than code?
>> Pretty-please?