On Mon, Nov 30, 2020 at 05:13:57PM +0100, Kevin Wolf wrote:
> Am 30.11.2020 um 16:30 hat Daniel P. Berrangé geschrieben:
> > On Mon, Nov 30, 2020 at 03:58:23PM +0100, Paolo Bonzini wrote:
> > > On 30/11/20 13:25, Kevin Wolf wrote:
> > > > This series adds a QAPI type for the properties of all user creatable
> > > > QOM types and finally makes QMP object-add use the new ObjectOptions
> > > > union so that QAPI introspection can be used for user creatable objects.
> > > >
> > > > After this series, there is least one obvious next step that needs to be
> > > > done: Change HMP and all of the command line parser to use
> > > > ObjectOptions, too, so that the QAPI schema is consistently enforced in
> > > > all external interfaces. I am planning to send another series to address
> > > > this.
> > > >
> > > > In a third step, we can try to start deduplicating and integrating
> > > > things
> > > > better between QAPI and the QOM implementation, e.g. by generating parts
> > > > of the QOM boilerplate from the QAPI schema.
> > >
> > > With this series it's basically pointless to have QOM properties at all.
> > > Instead, you are basically having half of QEMU's backend data model into a
> > > single struct.
> > >
> > > So the question is, are we okay with shoveling half of QEMU's backend data
> > > model into a single struct? If so, there are important consequences.
> >
> > In theory they should have the same set of options, but nothing in
> > this series will enforce that. So we're introducing the danger that
> > QMP object-add will miss some property, and thus be less functional
> > than the CLI -object. If we convert CLI -object to use the QAPI
> > parser too, we eliminate that danger, but we still have the struct
> > duplication.
>
> I think converting the CLI is doable in the short term. I already have
> the patch for qemu-storage-daemon, but decided to keep it for a separate
> series.
>
> The most difficult part is probably -readconfig, but with Paolo's RFC
> patches to move it away from QemuOpts, even that shouldn't be very hard.
>
> > > 1) QOM basically does not need properties anymore except for devices and
> > > machines (accelerators could be converted to QAPI as well). All
> > > user-creatable objects can be changed to something like chardev's "get a
> > > struct and use it fill in the fields", and only leave properties to
> > > devices
> > > and machines.
> > >
> > > 2) User-creatable objects can have a much more flexible schema. This
> > > means
> > > there's no reason to have block device creation as its own command and
> > > struct for example.
> > >
> > > The problem with this series is that you are fine with deduplicating
> > > things
> > > as a third step, but you cannot be sure that such deduplication is
> > > possible
> > > at all. So while I don't have any problems in principle with the
> > > ObjectOptions concept, I don't think it should be committed without a
> > > clear
> > > idea of how to do the third step.
> >
> > I feel like we should at least aim to kill the struct duplication, even if
> > we ignore the bigger QOM stuff like setters/getters/constructors/etc. The
> > generated structs are not far off being usable.
> >
> > eg for the secret object we have the QAPI schema
> >
> > { 'struct': 'SecretCommonProperties',
> > 'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
> > '*format': 'QCryptoSecretFormat',
> > '*keyid': 'str',
> > '*iv': 'str' } }
> >
> > { 'struct': 'SecretProperties',
> > 'base': 'SecretCommonProperties',
> > 'data': { '*data': 'str',
> > '*file': 'str' } }
> >
> > IIUC this will resulting in a QAPI generated flattened struct:
> >
> > struct SecretProperties {
> > bool loaded;
> > QCryptoSecretFormat format;
> > char *keyid;
> > char *iv;
> > char *data;
> > char *file;
> > };
> >
> > vs the QOM manually written structs
> >
> > struct QCryptoSecretCommon {
> > Object parent_obj;
> > uint8_t *rawdata;
> > size_t rawlen;
> > QCryptoSecretFormat format;
> > char *keyid;
> > char *iv;
> > };
> >
> > struct QCryptoSecret {
> > QCryptoSecretCommon parent_obj;
> > char *data;
> > char *file;
> > };
> >
> > The key differences
> >
> > - The parent struct is embedded, rather than flattened
> > - The "loaded" property doesn't need to exist
> > - Some extra fields are live state (rawdata, rawlen)
> >
> > Lets pretend we just kill "loaded" entirely, so ignore that.
> >
> > We could simply make QOM "Object" a well known built-in type, so
> > we can reference it as a "parent". Then any struct with "Object"
> > as a parent could use struct embedding rather flattening and thus
> > just work.
> >
> > Can we invent a "state" field for fields that are internal
> > only, separate from the public "data" fields.
> >
> > eg the secret QAPI def would only need a couple of changes:
> >
> > { 'struct': 'QCryptoSecretCommon',
> > 'base': 'Object',
> > 'state': { 'rawdata': '*uint8_t',
> > 'rawlen': 'size_t' },
> > 'data': { '*format': 'QCryptoSecretFormat',
> > '*keyid': 'str',
> > '*iv': 'str' } }
> >
> > { 'struct': 'QCryptoSecret',
> > 'base': 'QCryptoSecretCommon',
> > 'data': { '*data': 'str',
> > '*file': 'str' } }
>
> I haven't given much though to the details yet, but I was thinking of
> introducing a new QAPI entity type for objects. We could include
> additional fields there, where the type would just directly be a C type
> rather than being interpreted by QAPI.
>
> Maybe like this:
>
> { 'object': 'secret-common',
> 'abstract': true,
> 'properties': 'SecretCommonProperties',
> 'state': { 'rawdata': '*uint8_t',
> 'rawlen': 'size_t' } }
>
> { 'object': 'secret',
> 'parent': 'secret-common',
> 'properties': 'SecretProperties' } }
>
> Maybe it would actually be nicer to have 'state' just as a string
> property that contains the C type name of the state struct and then QAPI
> just adds a pointer to it.
Yep, it would be nice to have clear separation of the "state" from
the "config", as that also makes it more obvious what is derived
info.
>
> Either way, there is some duplication there because we have a
> parent-child relationship both between the object types themselves and
> between their property classes. Either we remove the base from
> SecretProperties (which would make object-add and the CLI more
> complicated) or we just let the QAPI generator check that they are
> consistent.
I don't really like the duplicate hierarchies there either. I did
consider, a new 'object' entity instead of 'struct', but if we go
that way we should exclusively use "object" for the QOM types.
eg an "object" entity type would be a specialization of the
"struct" entity type, rather than something bolted onto the
side.
Basically I think the QOM struct and the properties struct should
remain one & the same thing.
If we think of "object" as a specialization of 'struct' then and
have "state" as a separate struct, we avoid the duplicate hierarchies
{ 'object': 'QCryptoSecretCommon',
'state': 'QCryptoSecretCommonState',
'data': { '*format': 'QCryptoSecretFormat',
'*keyid': 'str',
'*iv': 'str' } }
{ 'object': 'QCryptoSecret',
'base': 'QCryptoSecretCommon',
'data': { '*data': 'str',
'*file': 'str' } }
there's not really much difference to this than just carrying
on using "struct" entity type though, and having the special
"Object" parent type as a built-in type.
> > There would need to be a
> >
> > void QCryptoSecretCommonFreeState(QCryptoSecretCommon *obj)
> >
> > method defined manually by the programmer to take care of free'ing any
> > pointers in the "state".
>
> Isn't this the job of the normal .instance_finalize method?
Yes, but I was thinking the fact how to wire into the free methods that
QAPI generates.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
