Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

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.

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.

> 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?