| Message ID | 20201130122538.27674-1-kwolf@redhat.com |
|---|---|
| Headers | show |
| Series | qapi/qom: QAPIfy object-add | expand |
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. 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. In the meanwhile, of course I have no problem with deprecating the opened and loaded properties. Paolo
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. > 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' } } 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". Regards, Daniel
Am 30.11.2020 um 15:58 hat Paolo Bonzini geschrieben: > 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. Not entirely, because there are still some writable properties that can be changed later on. After working through all the user creatable objects, I would say that separating these from the creation-time options is actually a good thing because there are basically two types of property setters in the existing implementations: 1. It starts with something like 'if (completed)' and takes two different paths, so they are already separated. Often one path is just returning an error, but sometimes we actually make an effort to update the internal state according to the new value. 2. No distinction is made. Usually the result is inconsistent state because the property values themselves are updated, but they have been interpreted once in ucc->complete and are ignored afterwards. Or maybe even worse, they are still used, but no care is taken that they are consistent with the rest of the internal state. Unfortunately my impression is that this is the more common type. So with this in mind, I think I'm in favour of completely leaving the initialisation of properties on object creation to QAPI, and only providing individual setters if we actually intend to allow property changes after creation. > 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. Yeah, the single struct bothers me a bit, both in the QAPI schema and in the C source. We probably need to have it present in the schema in some way so we can actually check input against the schema. Maybe we can have it automatically compiled by the QAPI generator so that we don't need to manually update the enum and the union each time. In the C source, I guess the other option would be to have pointers rather than directly embedding all struct types. In the long run this might make more sense. As long as it's only user-creatable objects, it's no worse than BlockdevOptions. > 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. True for those properties that don't support updates after object creation. For these, leaving the work to QAPI simplifies things a lot. > 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. In theory yes. The block layer isn't really QAPIfied, though, it just has a QAPI wrapper (similar to how this series doesn't QAPIfy QOM, but justs wraps it). But for the long term vision, I think it's a reasonable goal to have block nodes represented as QOM-with-QAPI objects. > 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. Do you have any specific concerns why the deduplication might not possible, or just because it's uncharted territory? The only reason why I wouldn't like to wait too long with merging this is because of merge conflicts (the list of properties or their details might change and this could go unnoticed). Maybe if we don't want to commit to keeping the ObjectOptions schema, the part that should wait is object-add and I should do the command line options first? Then the schema remains an implementation detail for now that is invisible in introspection. > In the meanwhile, of course I have no problem with deprecating the opened > and loaded properties. If we decide that we don't want to have the schema at all (which I hope we won't decide), I can split the deprecation into separate patches. Kevin
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. secret doesn't need writable properties, but for those objects that do need it, I would include a list that defines some properties as having setters and then the QAPI generated property code would call a manually written callback. > 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? Kevin
On 30/11/20 16:30, Daniel P. Berrangé wrote: > { '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' } } No, please don't do this. I want to keep writing C code, not a weird JSON syntax for C. I'd much rather have a FooOptions field in my struct so that I can just do visit_FooOptions in the UserCreatable.complete() method, that's feasible. Paolo
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
On 30/11/20 16:46, Kevin Wolf wrote: > Am 30.11.2020 um 15:58 hat Paolo Bonzini geschrieben: >> With this series it's basically pointless to have QOM properties at >> all. > > Not entirely, because there are still some writable properties that can > be changed later on. Are there really any (that are not bugs like opened/loaded)? That's also why Eduardo and I discussed a class-wide allow_set function for his field properties series. > So with this in mind, I think I'm in favour of completely leaving the > initialisation of properties on object creation to QAPI, and only > providing individual setters if we actually intend to allow property > changes after creation. The main problem is that it wouldn't extend well, if at all, to machines and devices. So those would still not be integrated into the QAPI schema. >> 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. > > Yeah, the single struct bothers me a bit, both in the QAPI schema and in > the C source. The single struct doesn't bother me _too much_ actually. What bothers me is that it won't be a single source of all QOM objects, only those that happen to be created by object-add. So I start to wonder if QOM as it exists now is the right solution for all kind of objects: - backends and other object-add types (not per-target, relatively few classes and even fewer hierarchies) - machine (per-target, many classes, no hierarchy) - device (can be treated as not per-target, many many classes, very few hierarchies) - accelerator (per-target, few classes, no hierarchy) - chardev (ok those are the same as the first category) If QOM is the right solution, this patch goes in the wrong direction. If QOM is the wrong solution, this patch is okay but then we have another problem to solve. :) >> 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. > > Do you have any specific concerns why the deduplication might not > possible, or just because it's uncharted territory? Mostly because it's more or less the same issue that you have with BlockdevOptions, with the extra complication that this series only deals with the easy one of the four above categories. > Maybe if we don't want to commit to keeping the ObjectOptions schema, > the part that should wait is object-add and I should do the command line > options first? Then the schema remains an implementation detail for now > that is invisible in introspection. I don't see much benefit in converting _any_ of the three actually. The only good reason I see for QAPIfying this is the documentation, and the promise of deduplicating QOM boilerplate. The latter is only a promise, but documentation alone is a damn good reason and it's enough to get this work into a mergeable shape as soon as possible! But maybe, we could start in the opposite direction: start with the use QAPI to eliminate QOM boilerplate. Basing your work on Eduardo's field properties series, you could add a new 'object' "kind" to QAPI that would create an array of field properties (e.g. a macro expanding to a compound literal?) . Something like +{ 'object': 'InputBarrier', + 'data': { 'name': 'str', + 'x-origin': 'int16', + 'y-origin': 'int16', + 'width': 'int16', + 'height': 'int16' }, + 'properties': { 'server': 'str', + 'port': 'str' } } would create a macro QOM_InputBarrier_FIELDS defining properties for the following fields of the InputBarrier struct: gchar *name; int16_t x_origin, y_origin; int16_t width, height; while server and port would only appear in the documentation (or alternatively you could leave them out completely, as you wish). The advantages would be noticeable: 1) the information would be moved in the QAPI schema JSON from the beginning, decoupling the conflict-heavy part from the complex question of how to expose the QOM schema in the introspection data 2) there would not be any more duplication than before (there would be duplication between structs and QAPI schema, but not between structs and C code that defines properties). 3) it would be opt-in, so it doesn't put on you the burden of keeping the series in sync with new objects that are added (I have one for the qtest server for example). At the same time it would be quite appealing for owners of QOM code to convert their objects to field properties and get documentation for free. 4) we could special-case 'object' definitions and generate them in the system manual as well, since they are also useful to document -object. Yes it's a huge change but you have the boring part already done. What do you think? Paolo >> In the meanwhile, of course I have no problem with deprecating the opened >> and loaded properties. > > If we decide that we don't want to have the schema at all (which I hope > we won't decide), I can split the deprecation into separate patches. > > Kevin >
Am 30.11.2020 um 17:57 hat Paolo Bonzini geschrieben: > On 30/11/20 16:46, Kevin Wolf wrote: > > Am 30.11.2020 um 15:58 hat Paolo Bonzini geschrieben: > > > With this series it's basically pointless to have QOM properties at > > > all. > > > > Not entirely, because there are still some writable properties that can > > be changed later on. > > Are there really any (that are not bugs like opened/loaded)? That's also why > Eduardo and I discussed a class-wide allow_set function for his field > properties series. Yes. I don't really know most objects apart from what I had to learn for this series, but I know at least two that actually allow this intentionally: iothread and throttle-group. > > So with this in mind, I think I'm in favour of completely leaving the > > initialisation of properties on object creation to QAPI, and only > > providing individual setters if we actually intend to allow property > > changes after creation. > > The main problem is that it wouldn't extend well, if at all, to > machines and devices. So those would still not be integrated into the > QAPI schema. What do you think is the biggest difference there? Don't devices work the same as user creatable objects in that you first set a bunch of properties (which would now be done through QAPI instead) and then call the completion/realize method that actually makes use of them? I must admit that I don't know how machine types work. > > > 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. > > > > Yeah, the single struct bothers me a bit, both in the QAPI schema and in > > the C source. > > The single struct doesn't bother me _too much_ actually. What bothers me is > that it won't be a single source of all QOM objects, only those that happen > to be created by object-add. But isn't it only natural that a list of these objects will exist in some way, implicitly or explicitly? object-add must somehow decide which object types it allows the user to create. Once we describe the object types in the schema (rather than only their properties), which is required if we want to generate the QOM boilerplate, this can possibly become implicit because then QAPI can know which objects implement USER_CREATABLE. > So I start to wonder if QOM as it exists now is the right solution for > all kind of objects: > > - backends and other object-add types (not per-target, relatively few > classes and even fewer hierarchies) > > - machine (per-target, many classes, no hierarchy) > > - device (can be treated as not per-target, many many classes, very few > hierarchies) > > - accelerator (per-target, few classes, no hierarchy) > > - chardev (ok those are the same as the first category) > > If QOM is the right solution, this patch goes in the wrong direction. > > If QOM is the wrong solution, this patch is okay but then we have another > problem to solve. :) I think the requirements for all of them are probably similar enough that they can potentially be covered by a single thing. I'm also pretty sure that QOM as it exists now is not the right solution for any of them because it has some major shortcomings. It's too easy to get things wrong (like the writable properties after creation), its introspection is rather weak and separated from the QAPI introspection, it doesn't encourage documentation, and it involves quite a bit of boilerplate and duplicated code between class implementations. A modified QOM might be the right solution, though. I would like to bring QAPI and QOM together because most of these weaknesses are strengths of QAPI. I guess the real question is what aspects of QOM need to be changed to make it the right solution. > > > 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. > > > > Do you have any specific concerns why the deduplication might not > > possible, or just because it's uncharted territory? > > Mostly because it's more or less the same issue that you have with > BlockdevOptions, with the extra complication that this series only deals > with the easy one of the four above categories. I'm not sure which exact problem with BlockdevOptions you mean. The reason why the block layer doesn't use BlockdevOptions everywhere is -drive support. > > Maybe if we don't want to commit to keeping the ObjectOptions schema, > > the part that should wait is object-add and I should do the command line > > options first? Then the schema remains an implementation detail for now > > that is invisible in introspection. > > I don't see much benefit in converting _any_ of the three actually. The > only good reason I see for QAPIfying this is the documentation, and the > promise of deduplicating QOM boilerplate. The latter is only a promise, but > documentation alone is a damn good reason and it's enough to get this work > into a mergeable shape as soon as possible! I think having some input validation done by QAPI instead of in each QOM object individually is nice, too. You get it after CLI, QMP and HMP all go through QAPI. > But maybe, we could start in the opposite direction: start with the use QAPI > to eliminate QOM boilerplate. Basing your work on Eduardo's field > properties series, you could add a new 'object' "kind" to QAPI that would > create an array of field properties (e.g. a macro expanding to a compound > literal?) There is a very simple reason why I don't want to start with the QAPI generator: John has multiple series pending that touch basically every part of the QAPI generator. This means not only that I need to be careful about merge conflict (or base my work on top of five other series, which feels adventurous), but also that I would be competing with John for Markus' reviewer capacity, further slowing things down. Well, two reasons: Also because this series for the external interface of the objects already exists and it's an incremental step towards your proposal: The types for 'properties' will already exist then and I won't have to convert both internal state and external interfaces at the same time. > . Something like > > > +{ 'object': 'InputBarrier', > + 'data': { 'name': 'str', > + 'x-origin': 'int16', > + 'y-origin': 'int16', > + 'width': 'int16', > + 'height': 'int16' }, > + 'properties': { 'server': 'str', > + 'port': 'str' } } I think we have similar ideas there (see my reply to Dan), just that I see it as an incremental step on top of this one. > would create a macro QOM_InputBarrier_FIELDS defining properties for the > following fields of the InputBarrier struct: > > gchar *name; > int16_t x_origin, y_origin; > int16_t width, height; > > while server and port would only appear in the documentation (or > alternatively you could leave them out completely, as you wish). > The advantages would be noticeable: > > 1) the information would be moved in the QAPI schema JSON from the > beginning, decoupling the conflict-heavy part from the complex question of > how to expose the QOM schema in the introspection data > > 2) there would not be any more duplication than before (there would be > duplication between structs and QAPI schema, but not between structs and C > code that defines properties). > > 3) it would be opt-in, so it doesn't put on you the burden of keeping the > series in sync with new objects that are added (I have one for the qtest > server for example). At the same time it would be quite appealing for > owners of QOM code to convert their objects to field properties and get > documentation for free. > > 4) we could special-case 'object' definitions and generate them in the > system manual as well, since they are also useful to document -object. > > Yes it's a huge change but you have the boring part already done. What do > you think? Yes, I would be happy to work on something like that. Just not as the first step, because I think it's nothing that would help us get this series in a mergable state as soon as possible. Kevi
On Mon, Nov 30, 2020 at 13:25:20 +0100, 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. FYI, here's a libvirt patchset which (apart from refactors) adds compatibility with the deprecated/removed 'props' sub-object and also adds testing to see whether all objects used by libvirt conform to the new schema: https://www.redhat.com/archives/libvir-list/2020-November/msg01625.html Spoiler alert: surprisingly they do match so no updates are necessary!
On 30/11/20 19:10, Kevin Wolf wrote: > Am 30.11.2020 um 17:57 hat Paolo Bonzini geschrieben: >> The main problem is that it wouldn't extend well, if at all, to >> machines and devices. So those would still not be integrated into the >> QAPI schema. > > What do you think is the biggest difference there? Don't devices work > the same as user creatable objects in that you first set a bunch of > properties (which would now be done through QAPI instead) and then call > the completion/realize method that actually makes use of them? For devices it's just the practical issue that there are too many to have something like this series. For machine types the main issue is that the version-specific machine types would have to be defined in one more place. >> The single struct doesn't bother me _too much_ actually. What bothers me is >> that it won't be a single source of all QOM objects, only those that happen >> to be created by object-add. > > But isn't it only natural that a list of these objects will exist in > some way, implicitly or explicitly? object-add must somehow decide which > object types it allows the user to create. There's already one, it's objects that implement user creatable. I don't think having it as a QAPI enum (or discriminator) is worthwhile, and it's one more thing that can go out of sync between the QAPI schema and the C code. > I'm also pretty sure that QOM as it exists now is not the right solution > for any of them because it has some major shortcomings. It's too easy to > get things wrong (like the writable properties after creation), its > introspection is rather weak and separated from the QAPI introspection, > it doesn't encourage documentation, and it involves quite a bit of > boilerplate and duplicated code between class implementations. > > A modified QOM might be the right solution, though. I would like to > bring QAPI and QOM together because most of these weaknesses are > strengths of QAPI. I agree wholeheartedly. But your series goes to the opposite excess. Eduardo is doing work in QOM to mitigate the issues you point out, and you have to meet in the middle with him. Starting with the user-visible parts focuses on the irrelevant parts. >> Mostly because it's more or less the same issue that you have with >> BlockdevOptions, with the extra complication that this series only deals >> with the easy one of the four above categories. > > I'm not sure which exact problem with BlockdevOptions you mean. The > reason why the block layer doesn't use BlockdevOptions everywhere is > -drive support. You don't have a single BlockdevOptions* field in the structs of block/. My interpretation of this is that BlockdevOptions* is a good schema for configuring things but not for run-time state. >>> Maybe if we don't want to commit to keeping the ObjectOptions schema, >>> the part that should wait is object-add and I should do the command line >>> options first? Then the schema remains an implementation detail for now >>> that is invisible in introspection. >> >> I don't see much benefit in converting _any_ of the three actually. The >> only good reason I see for QAPIfying this is the documentation, and the >> promise of deduplicating QOM boilerplate. The latter is only a promise, but >> documentation alone is a damn good reason and it's enough to get this work >> into a mergeable shape as soon as possible! > > I think having some input validation done by QAPI instead of in each QOM > object individually is nice, too. You get it after CLI, QMP and HMP all > go through QAPI. But the right way to do that is to use Eduardo's field properties: they make QOM do the right thing, adding another layer of parsing is just adding more complexity. Are there any validation bugs that you're fixing? Is that something that cannot be fixed elsewhere, or are you papering over bad QOM coding? (Again, I'm not debating that QOM properties are hard to write correctly). >> But maybe, we could start in the opposite direction: start with the use QAPI >> to eliminate QOM boilerplate. Basing your work on Eduardo's field >> properties series, you could add a new 'object' "kind" to QAPI that would >> create an array of field properties (e.g. a macro expanding to a compound >> literal?) > > There is a very simple reason why I don't want to start with the QAPI > generator: John has multiple series pending that touch basically every > part of the QAPI generator. This means not only that I need to be > careful about merge conflict (or base my work on top of five other > series, which feels adventurous), but also that I would be competing > with John for Markus' reviewer capacity, further slowing things down. That's something that you have to discuss with John and Markus. It may be that John has to shelve part of his work or rebase on top of yours instead. By adding an extra parsing layer you're adding complexity that may not be needed in the end, and we're very bad of removing it later. So I'm worried that this series will become technical debt in just a few months. > Well, two reasons: Also because this series for the external interface > of the objects already exists and it's an incremental step towards your > proposal: The types for 'properties' will already exist then and I won't > have to convert both internal state and external interfaces at the same > time. I don't think converting internal state to QAPI is useful. QAPI and QOM are external interfaces and that's what they should remain; anything that is not an external interface should (must) remain plain C code. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 30/11/20 16:30, Daniel P. Berrangé wrote: >> { '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' } } > > No, please don't do this. I want to keep writing C code, not a weird > JSON syntax for C. > > I'd much rather have a FooOptions field in my struct so that I can > just do visit_FooOptions in the UserCreatable.complete() method, > that's feasible. It should be, because it's what we've done elsewhere, isn't it? Yes, we can extend QAPI to let us embed opaques outside the QAPI schema's scope ("state"), along with means to create, destroy, and clone. This is new. But we can also invert: put the QAPI-generated C type ("config") in a handwritten C type that marries it to "state". Code to create and destroy is handwritten, and uses QAPI-generated code for the "config" part. A generic interface to handwritten creation is possible: Take the QAPI-generated "config" type as argument, extract enough information to call the appropriate constructor, return its value. Generic destruction is also possible: all it needs is a map from instance to destructor function. None of this is new in QEMU.
Am 30.11.2020 um 20:35 hat Paolo Bonzini geschrieben: > On 30/11/20 19:10, Kevin Wolf wrote: > > Am 30.11.2020 um 17:57 hat Paolo Bonzini geschrieben: > > > The main problem is that it wouldn't extend well, if at all, to > > > machines and devices. So those would still not be integrated into the > > > QAPI schema. > > > > What do you think is the biggest difference there? Don't devices work > > the same as user creatable objects in that you first set a bunch of > > properties (which would now be done through QAPI instead) and then call > > the completion/realize method that actually makes use of them? > > For devices it's just the practical issue that there are too many to have > something like this series. For machine types the main issue is that the > version-specific machine types would have to be defined in one more place. Sure, the number of devices means that we can't convert all of them at once. And we don't need to, we can make the change incrementally. The only thing we can't do during this incremental phase is switching device_add from 'gen': false to actually using QAPI types. Doing that would be the very last step in the conversion. > > > The single struct doesn't bother me _too much_ actually. What bothers me is > > > that it won't be a single source of all QOM objects, only those that happen > > > to be created by object-add. > > > > But isn't it only natural that a list of these objects will exist in > > some way, implicitly or explicitly? object-add must somehow decide which > > object types it allows the user to create. > > There's already one, it's objects that implement user creatable. I don't > think having it as a QAPI enum (or discriminator) is worthwhile, and it's > one more thing that can go out of sync between the QAPI schema and the C > code. Well, we all know that this series duplicates things. But at the same time, we also know that this isn't going to be the final state. Once QAPI learns about QOM inheritance (which it has to if it should generate the boilerplate), it will know which objects are user creatable without a an explicitly defined separate enum. I think it will still need to have the enum internally, but as long as it's automatically generated, that shouldn't be a big deal. > > I'm also pretty sure that QOM as it exists now is not the right solution > > for any of them because it has some major shortcomings. It's too easy to > > get things wrong (like the writable properties after creation), its > > introspection is rather weak and separated from the QAPI introspection, > > it doesn't encourage documentation, and it involves quite a bit of > > boilerplate and duplicated code between class implementations. > > > > A modified QOM might be the right solution, though. I would like to > > bring QAPI and QOM together because most of these weaknesses are > > strengths of QAPI. > > I agree wholeheartedly. But your series goes to the opposite excess. > Eduardo is doing work in QOM to mitigate the issues you point out, and you > have to meet in the middle with him. Starting with the user-visible parts > focuses on the irrelevant parts. QAPI is first and foremost about user-visible parts, and in fact most of the problems I listed are about external interfaces. It seems to make a lot of sense to me to start there. > > > Mostly because it's more or less the same issue that you have with > > > BlockdevOptions, with the extra complication that this series only deals > > > with the easy one of the four above categories. > > > > I'm not sure which exact problem with BlockdevOptions you mean. The > > reason why the block layer doesn't use BlockdevOptions everywhere is > > -drive support. > > You don't have a single BlockdevOptions* field in the structs of block/. My > interpretation of this is that BlockdevOptions* is a good schema for > configuring things but not for run-time state. I'm not sure what you mean with run-time state. Yes, BlockdevOptions is about external interfaces, not about implementation details. Same thing as QOM properties are external interfaces, not implementation details. There may be fields in the internal state that correspond 1:1 to the externally visible QAPI type/QOM property, but it's not necessarily the case. > > > > Maybe if we don't want to commit to keeping the ObjectOptions schema, > > > > the part that should wait is object-add and I should do the command line > > > > options first? Then the schema remains an implementation detail for now > > > > that is invisible in introspection. > > > > > > I don't see much benefit in converting _any_ of the three actually. The > > > only good reason I see for QAPIfying this is the documentation, and the > > > promise of deduplicating QOM boilerplate. The latter is only a promise, but > > > documentation alone is a damn good reason and it's enough to get this work > > > into a mergeable shape as soon as possible! > > > > I think having some input validation done by QAPI instead of in each QOM > > object individually is nice, too. You get it after CLI, QMP and HMP all > > go through QAPI. > > But the right way to do that is to use Eduardo's field properties: they make > QOM do the right thing, adding another layer of parsing is just adding more > complexity. QAPI is already here and it's going to stay. QOM doesn't have to duplicate input validation that existing code can already perform. I'm not sure which complexity you think I'm introducing: QAPI is already there. I'm adding the schema, which you agree is valuable documentation, so we want to have it either case. The actual change to QOM that we have in this series is this: include/qom/object_interfaces.h | 7 ------- qom/qom-qmp-cmds.c | 44 ++++++++++++++++++++++---------------------- 2 files changed, 22 insertions(+), 29 deletions(-) Arguably some more runtime complexity on object-add because we do the QAPI validation and build an ObjectOptions object just to feed it back to a visitor. Do we really care? > Are there any validation bugs that you're fixing? Is that > something that cannot be fixed elsewhere, or are you papering over bad QOM > coding? (Again, I'm not debating that QOM properties are hard to write > correctly). Yes, I found bugs that the QAPI schema would have prevented. They were generally about not checking whether mandatory propertes are actually set. Of course you can fix broken QOM property implementations manually. And I sent separate patches to fix them manually because we don't go through QAPI for user creatable types yet (even after this series, -object would still be missing). But if a property is declared mandatory in the schema, I shouldn't have to bother with manual checks in ucc->complete. > > > But maybe, we could start in the opposite direction: start with the use QAPI > > > to eliminate QOM boilerplate. Basing your work on Eduardo's field > > > properties series, you could add a new 'object' "kind" to QAPI that would > > > create an array of field properties (e.g. a macro expanding to a compound > > > literal?) > > > > There is a very simple reason why I don't want to start with the QAPI > > generator: John has multiple series pending that touch basically every > > part of the QAPI generator. This means not only that I need to be > > careful about merge conflict (or base my work on top of five other > > series, which feels adventurous), but also that I would be competing > > with John for Markus' reviewer capacity, further slowing things down. > > That's something that you have to discuss with John and Markus. It may be > that John has to shelve part of his work or rebase on top of yours instead. > > By adding an extra parsing layer you're adding complexity that may not be > needed in the end, and we're very bad of removing it later. So I'm worried > that this series will become technical debt in just a few months. Again: Which complexity? > > Well, two reasons: Also because this series for the external interface > > of the objects already exists and it's an incremental step towards your > > proposal: The types for 'properties' will already exist then and I won't > > have to convert both internal state and external interfaces at the same > > time. > > I don't think converting internal state to QAPI is useful. QAPI and QOM are > external interfaces and that's what they should remain; anything that is not > an external interface should (must) remain plain C code. Yes, I agree. What I mean is changing property declarations and probably replacing most existing property getters or setters, which in turn access the internal state. That's just different work from describing the external interface. Kevin
On 01/12/20 17:20, Kevin Wolf wrote: > Am 30.11.2020 um 20:35 hat Paolo Bonzini geschrieben: >> For devices it's just the practical issue that there are too many to have >> something like this series. For machine types the main issue is that the >> version-specific machine types would have to be defined in one more place. > > Sure, the number of devices means that we can't convert all of them at > once. And we don't need to, we can make the change incrementally. There's also the question that most devices are not present in all binaries. So QAPI introspection would tell you what properties are supported but not what devices are. Also the marshaling/unmarshaling code for hundreds of devices would bloat the QEMU executables unnecessarily (it'd all be reachable from visit_DeviceOptions so it'd not be possible to compile it out, even with LTO). Plus the above issue with machine types. >>>> The single struct doesn't bother me _too much_ actually. What bothers me is >>>> that it won't be a single source of all QOM objects, only those that happen >>>> to be created by object-add. >>> >>> But isn't it only natural that a list of these objects will exist in >>> some way, implicitly or explicitly? object-add must somehow decide which >>> object types it allows the user to create. >> >> There's already one, it's objects that implement user creatable. I don't >> think having it as a QAPI enum (or discriminator) is worthwhile, and it's >> one more thing that can go out of sync between the QAPI schema and the C >> code. > > Well, we all know that this series duplicates things. But at the same > time, we also know that this isn't going to be the final state. > > Once QAPI learns about QOM inheritance (which it has to if it should > generate the boilerplate), it will know which objects are user creatable > without a an explicitly defined separate enum. > > I think it will still need to have the enum internally, but as long as > it's automatically generated, that shouldn't be a big deal. Right, I don't want to have the final state now but I'd like to have a rough idea of the plan: 1) whether to generate _all_ boilerplate or only properties 2) whether we want to introduce a separation between configuration schema and run-time state 3) in the latter case, whether properties will survive at all---iothread and throttle-groups don't really need them even if they're writable after creation. These questions have a substantial effect on how to handle this series. Without answers to this questions I cannot understand the long term and its interaction with other long term plans for QOM. >>> A modified QOM might be the right solution, though. I would like to >>> bring QAPI and QOM together because most of these weaknesses are >>> strengths of QAPI. >> >> I agree wholeheartedly. But your series goes to the opposite excess. >> Eduardo is doing work in QOM to mitigate the issues you point out, and you >> have to meet in the middle with him. Starting with the user-visible parts >> focuses on the irrelevant parts. > > QAPI is first and foremost about user-visible parts, and in fact most of > the problems I listed are about external interfaces. Yes, but QAPI is also about interfacing with existing code. Also, QAPI does not generate only structs, it also generate C function prototypes. I'm not sure whether a QOM object's more similar to the struct case (what you do here) or to the QMP command case: - there's a 1:1 correspondence between ObjectOptions cases and ucc->complete implementations - there's a registry of object types just like there's one for QMP commands. So another possibility could be that the QAPI generator essentially generated the user_creatable_add_type function that called out to user-provided functions qom_scsi_pr_helper_complete, qom_throttle_group_complete, etc. The arguments to those functions would be the configuration. That is a very interesting prospect (though one that would require changes to the QAPI code generator). > BlockdevOptions is about external interfaces, not about > implementation details. Same thing as QOM properties are external > interfaces, not implementation details. There may be fields in the > internal state that correspond 1:1 to the externally visible QAPI > type/QOM property, but it's not necessarily the case. Right. It may well be that we decide that, as a result of this series, QOM's property interface is declared essentially a failed experiment. I wouldn't be surprised, and that's why I want to understand better where we want to go. For example, Eduardo is focusing specifically on external interfaces that correspond 1:1 to the internal implementation. If we decide that non-1:1-mappings and checks on mandatory properties are an important part of QOM, that may make large parts of his work moot. If we decide that most QOM objects need no properties at all, then we don't want to move more qdev-specific stuff from to QOM. > QAPI is already here and it's going to stay. QOM doesn't have to > duplicate input validation that existing code can already perform. > > I'm not sure which complexity you think I'm introducing: QAPI is already > there. I'm adding the schema, which you agree is valuable documentation, > so we want to have it either case. The actual change to QOM that we have > in this series is this: The complexity is that properties used to be split in two places, and now they're split in three places. It may very well be that this is a good first step to at least have classes described in the QAPI schema. But since _in the short term_ there are things that the series makes worse (and has a risk of bringing things out of sync), I'd like to understand the long term plan and ensure that the QAPI maintainers are on board with it. Can you at least add a comment to all UserCreatable classes that says "if you add a property, remember to modify ... as well in the QAPI schema"? >> Are there any validation bugs that you're fixing? Is that >> something that cannot be fixed elsewhere, or are you papering over bad QOM >> coding? (Again, I'm not debating that QOM properties are hard to write >> correctly). > > Yes, I found bugs that the QAPI schema would have prevented. They were > generally about not checking whether mandatory propertes are actually > set. Okay, I found your series at https://patchew.org/QEMU/20201130105615.21799-1-kwolf@redhat.com/ too, good to know. So that's another useful thing that can be chalked to this series at least if -object and object_add are converted (and also, another thing against QOM properties and 1:1 mappings between configuration schema and run-time state). Paolo
On Tue, Dec 01, 2020 at 06:16:14PM +0100, Paolo Bonzini wrote: > On 01/12/20 17:20, Kevin Wolf wrote: [...] > > BlockdevOptions is about external interfaces, not about > > implementation details. Same thing as QOM properties are external > > interfaces, not implementation details. There may be fields in the > > internal state that correspond 1:1 to the externally visible QAPI > > type/QOM property, but it's not necessarily the case. > > Right. It may well be that we decide that, as a result of this series, > QOM's property interface is declared essentially a failed experiment. I > wouldn't be surprised, and that's why I want to understand better where we > want to go. > > For example, Eduardo is focusing specifically on external interfaces that > correspond 1:1 to the internal implementation. If we decide that > non-1:1-mappings and checks on mandatory properties are an important part of > QOM, that may make large parts of his work moot. [...] Whatever we decide, my first and biggest worry is to have a reasonable migration path for any new API. We could describe in detail what's the API we _want_, but we also need a reasonable migration path for the code we already _have_. If a new API that requires manual conversion of existing devices, it will probably coexist with the existing qdev property API for years. (Note that I haven't read this whole thread yet.) > [...] If we decide that most QOM > objects need no properties at all, then we don't want to move more > qdev-specific stuff from to QOM. I don't understand what "move more qdev-specific stuff to QOM" means. I consider the QOM field property API valuable even if we decide the only user of the API will be legacy qdev code, because it separates the core mechanism (that's code that already existed) from qdev-specific policies. > > > QAPI is already here and it's going to stay. QOM doesn't have to > > duplicate input validation that existing code can already perform. > > > > I'm not sure which complexity you think I'm introducing: QAPI is already > > there. I'm adding the schema, which you agree is valuable documentation, > > so we want to have it either case. The actual change to QOM that we have > > in this series is this: > > The complexity is that properties used to be split in two places, and now > they're split in three places. > > It may very well be that this is a good first step to at least have classes > described in the QAPI schema. But since _in the short term_ there are > things that the series makes worse (and has a risk of bringing things out of > sync), I'd like to understand the long term plan and ensure that the QAPI > maintainers are on board with it. > > Can you at least add a comment to all UserCreatable classes that says "if > you add a property, remember to modify ... as well in the QAPI schema"? > > > > Are there any validation bugs that you're fixing? Is that > > > something that cannot be fixed elsewhere, or are you papering over bad QOM > > > coding? (Again, I'm not debating that QOM properties are hard to write > > > correctly). > > > > Yes, I found bugs that the QAPI schema would have prevented. They were > > generally about not checking whether mandatory propertes are actually > > set. > > Okay, I found your series at > https://patchew.org/QEMU/20201130105615.21799-1-kwolf@redhat.com/ too, good > to know. > > So that's another useful thing that can be chalked to this series at least > if -object and object_add are converted (and also, another thing against QOM > properties and 1:1 mappings between configuration schema and run-time > state). > > Paolo >
Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben: > On 01/12/20 17:20, Kevin Wolf wrote: > > Am 30.11.2020 um 20:35 hat Paolo Bonzini geschrieben: > > > For devices it's just the practical issue that there are too many to have > > > something like this series. For machine types the main issue is that the > > > version-specific machine types would have to be defined in one more place. > > > > Sure, the number of devices means that we can't convert all of them at > > once. And we don't need to, we can make the change incrementally. > > There's also the question that most devices are not present in all binaries. > So QAPI introspection would tell you what properties are supported but not > what devices are. Also the marshaling/unmarshaling code for hundreds of > devices would bloat the QEMU executables unnecessarily (it'd all be > reachable from visit_DeviceOptions so it'd not be possible to compile it > out, even with LTO). I don't think this is actually a new things. We already have types and commands declared with things like 'if': 'defined(TARGET_S390X)'. As far as I understand, QAPI generated files are already built per target, so not compiling things into all binaries should be entirely possible. What probably needs a solution is that an explicit 'if' in the schema would duplicate information that is actually defined in the build system configuration. So we would somehow need a common source for both. > Plus the above issue with machine types. As I said, I don't know the machine type code well, so I'm not really sure about the best way there. But maybe QAPI isn't for version-specific machine types. I think they never have additional properties, but only different init functions. So their description in QAPI would be essentially empty anyway. So maybe only the abstract base class that actually defines the machine properties (like generic-pc-machine) should be described in QAPI, and then the concrete machine types would inherit from it without being described in QAPI themselves? > > > > > The single struct doesn't bother me _too much_ actually. What bothers me is > > > > > that it won't be a single source of all QOM objects, only those that happen > > > > > to be created by object-add. > > > > > > > > But isn't it only natural that a list of these objects will exist in > > > > some way, implicitly or explicitly? object-add must somehow decide which > > > > object types it allows the user to create. > > > > > > There's already one, it's objects that implement user creatable. I don't > > > think having it as a QAPI enum (or discriminator) is worthwhile, and it's > > > one more thing that can go out of sync between the QAPI schema and the C > > > code. > > > > Well, we all know that this series duplicates things. But at the same > > time, we also know that this isn't going to be the final state. > > > > Once QAPI learns about QOM inheritance (which it has to if it should > > generate the boilerplate), it will know which objects are user creatable > > without a an explicitly defined separate enum. > > > > I think it will still need to have the enum internally, but as long as > > it's automatically generated, that shouldn't be a big deal. > > Right, I don't want to have the final state now but I'd like to have a > rough idea of the plan: > > 1) whether to generate _all_ boilerplate or only properties I would like to generate as much boilerplate as possible. That is, I don't want to constrain us to only properties, but at the same time, I'm not sure if it's possible to get rid of all boilerplate. Basically, the vision I have in mind is that QAPI would generate code that is the same for most instances, and then provide an option that prevents code generation for a specific part for more complicated cases, so that the respective function can (and must) be provided in the C source. > 2) whether we want to introduce a separation between configuration > schema and run-time state You mean the internal run-time state? How is this separation not already present with getter/setter functions for each property? In many cases they just directly access the run-time state, but there are other cases where they actually do things. Or do you mean between create-time options and properties that can be written at runtime? In that case, yes, I think that would be very desirable because mashing them together has resulted in lots of bugs. > 3) in the latter case, whether properties will survive at all---iothread and > throttle-groups don't really need them even if they're writable after > creation. How do you define properties, i.e. at which point would they stop existing and what would be a non-property alternative? Outside of QOM, usually have a query-* command that returns the whole state rather than a single property, and possibly individual QMP commands to update the state. blockdev-reopen takes a new configuration (the same structure as blockdev-add) and then applies that whole new configuration to the existing block node. I guess this would be an alternative, though it is somewhat inconvenient because you have to repeat all the options that you don't want to change. The blockdev-reopen way has one important advantage: You can change multiple options at the same time when there are dependencies between the options so that individually changing each option would be invalid, but together it's possible. throttle-group needs this in QOM, and it's easily implemented: It just has a single 'limits' property, which is a struct and is always updated as a single unit. So in summary, it seems to me that the QOM way is more flexible because you can get both models out of it. Whether we actually need this flexibility I can't say. But there is another reason why I didn't intend to remove properties entirely, which is that things are a lot easier as long as you don't have to break external interfaces, which qom-set/get/list are. The way I imagined it so far is roughly like this: * Configuration options are described in the QAPI schema. This is mainly for object creation, but runtime modifiable properties are a subset of this. * Properties are generated for each option. By default, the getter just returns the value from the configuration at creation time, though generation of it can be disabled so that it can be overridden. Also, setters just return an error by default. * Property setters aren't called for object creation. Instead, the relevant ObjectOptions branch is made available to some init method. * Runtime modifiable properties (declared as such in the schema) don't get the default setter, so you have to provide an implementation for them. > These questions have a substantial effect on how to handle this series. > Without answers to this questions I cannot understand the long term and its > interaction with other long term plans for QOM. > > > > > A modified QOM might be the right solution, though. I would like to > > > > bring QAPI and QOM together because most of these weaknesses are > > > > strengths of QAPI. > > > > > > I agree wholeheartedly. But your series goes to the opposite excess. > > > Eduardo is doing work in QOM to mitigate the issues you point out, and you > > > have to meet in the middle with him. Starting with the user-visible parts > > > focuses on the irrelevant parts. > > > > QAPI is first and foremost about user-visible parts, and in fact most of > > the problems I listed are about external interfaces. > > Yes, but QAPI is also about interfacing with existing code. Also, QAPI does > not generate only structs, it also generate C function prototypes. I'm not > sure whether a QOM object's more similar to the struct case (what you do > here) or to the QMP command case: > > - there's a 1:1 correspondence between ObjectOptions cases and ucc->complete > implementations > > - there's a registry of object types just like there's one for QMP commands. This is an interesting comparison. I never thought of likening objects to commands, but there is some truth to it. I certainly did envision objects as separate type of QAPI entities, which I guess is why I didn't really like Dan's suggestion to make objects an extension of structs, but I couldn't really express why. I think your comparison gives a good answer. However, this series is still not doing the wrong thing: If you think about commands, they don't exist without an arguments struct. Similarly, objects don't exist without a properties struct. So while this series is doing only one part of the whole solution, that the second part is missing doesn't make the first part wrong. > So another possibility could be that the QAPI generator essentially > generated the user_creatable_add_type function that called out to > user-provided functions qom_scsi_pr_helper_complete, > qom_throttle_group_complete, etc. The arguments to those functions would be > the configuration. That is a very interesting prospect (though one that > would require changes to the QAPI code generator). I was thinking of using .instance_init because it's more generic, but using ucc->complete (and in the long run realize for qdev) might require less modifications of non-user-creatable objects. One possibly nasty detail to consider there is that we sometimes declare the USER_CREATABLE interface in the base class, so ucc->complete is for the base class rather than the actually instantiated class. If we only instantiate leaf classes (I think this is true), we can move USER_CREATABLE there. I also had in mind just passing the whole configuration struct (essentially always 'boxed': true), but you're right that individual parameters like for commands would be possible. I'm not entirely convinced that they would be better (there was a reason why we introduced 'boxed': true), but it's an option. > > BlockdevOptions is about external interfaces, not about > > implementation details. Same thing as QOM properties are external > > interfaces, not implementation details. There may be fields in the > > internal state that correspond 1:1 to the externally visible QAPI > > type/QOM property, but it's not necessarily the case. > > Right. It may well be that we decide that, as a result of this series, > QOM's property interface is declared essentially a failed experiment. I > wouldn't be surprised, and that's why I want to understand better where we > want to go. > > For example, Eduardo is focusing specifically on external interfaces that > correspond 1:1 to the internal implementation. If we decide that > non-1:1-mappings and checks on mandatory properties are an important part of > QOM, that may make large parts of his work moot. If we decide that most QOM > objects need no properties at all, then we don't want to move more > qdev-specific stuff from to QOM. Hm, it's a good point. 1:1 properties would actually end up in generated code anyway, so simplifying them is maybe not that useful in the end. It would probably result in nicer generated code, but that's it. > > QAPI is already here and it's going to stay. QOM doesn't have to > > duplicate input validation that existing code can already perform. > > > > I'm not sure which complexity you think I'm introducing: QAPI is already > > there. I'm adding the schema, which you agree is valuable documentation, > > so we want to have it either case. The actual change to QOM that we have > > in this series is this: > > The complexity is that properties used to be split in two places, and now > they're split in three places. > > It may very well be that this is a good first step to at least have classes > described in the QAPI schema. But since _in the short term_ there are > things that the series makes worse (and has a risk of bringing things out of > sync), I'd like to understand the long term plan and ensure that the QAPI > maintainers are on board with it. > > Can you at least add a comment to all UserCreatable classes that says "if > you add a property, remember to modify ... as well in the QAPI schema"? I was hoping that by converting object-add in this series, and the CLI options soon afterwards, it would be very obvious if you forget to change the schema because your new property just wouldn't work (at least not during creation). But I can add comments if you think this is still worthwhile. > > > Are there any validation bugs that you're fixing? Is that > > > something that cannot be fixed elsewhere, or are you papering over bad QOM > > > coding? (Again, I'm not debating that QOM properties are hard to write > > > correctly). > > > > Yes, I found bugs that the QAPI schema would have prevented. They were > > generally about not checking whether mandatory propertes are actually > > set. > > Okay, I found your series at > https://patchew.org/QEMU/20201130105615.21799-1-kwolf@redhat.com/ too, good > to know. And two more patches of the same type here: https://patchew.org/QEMU/20201117163045.307451-1-kwolf@redhat.com/ > So that's another useful thing that can be chalked to this series at least > if -object and object_add are converted (and also, another thing against QOM > properties and 1:1 mappings between configuration schema and run-time > state). Yes. object-add is already covered by this series and -object should be easy, so we can have this more or less now. Getting the design right for QOM code generation will certainly take more time, discussion and also experimentation. Kevin
On 01/12/20 20:35, Kevin Wolf wrote: > Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben: > I don't think this is actually a new things. We already have types and > commands declared with things like 'if': 'defined(TARGET_S390X)'. > As far as I understand, QAPI generated files are already built per > target, so not compiling things into all binaries should be entirely > possible. There is some complication due to having different discriminators per target. So yes it should be possible. But probably best left after objects because it's so much bigger a task and because objects have a bit more freedom for experimentation (less ties to other qdev-specific concepts, e.g. the magic "bus" property). > So maybe only the abstract base class that actually defines the machine > properties (like generic-pc-machine) should be described in QAPI, and > then the concrete machine types would inherit from it without being > described in QAPI themselves? Yes, maybe. >> 1) whether to generate _all_ boilerplate or only properties > > I would like to generate as much boilerplate as possible. That is, I > don't want to constrain us to only properties, but at the same time, I'm > not sure if it's possible to get rid of all boilerplate. > > Basically, the vision I have in mind is that QAPI would generate code > that is the same for most instances, and then provide an option that > prevents code generation for a specific part for more complicated cases, > so that the respective function can (and must) be provided in the C > source. Ok, so that's a bit different from what I am thinking of. I don't care very much about the internal boilerplate, only the external interface for configuration. So I don't care about type registration, dynamic cast macros etc., only essentially the part that leads to ucc->complete. >> 2) whether we want to introduce a separation between configuration >> schema and run-time state > > You mean the internal run-time state? How is this separation not already > present with getter/setter functions for each property? In many cases > they just directly access the run-time state, but there are other cases > where they actually do things. I mean moving more towards the blockdev/chardev way of doing things, increasing the separation very much by having separate configuration structs that have (potentially) no link to the run-time state struct. >> 3) in the latter case, whether properties will survive at all---iothread and >> throttle-groups don't really need them even if they're writable after >> creation. > > How do you define properties, i.e. at which point would they stop > existing and what would be a non-property alternative? Properties are only a useful concept if they have a use. If -object/object_add/object-add can do the same job without properties, properties are not needed anymore. Right now QOM is all about exposing properties, and having multiple interfaces to set them (by picking a different visitor). But in practice most QOM objects have a lifetime that consists of 1) set properties 2) flip a switch (realized/complete/open) 3) let the object live on its own. 1+2 are a single monitor command or CLI option; during 3 you access the object through monitor commands, not properties. > So in summary, it seems to me that the QOM way is more flexible because > you can get both models out of it. Whether we actually need this > flexibility I can't say. I'm thinking there's no need for it, but maybe I'm overly optimistic. > * Configuration options are described in the QAPI schema. This is mainly > for object creation, but runtime modifiable properties are a subset of > this. > > * Properties are generated for each option. By default, the getter > just returns the value from the configuration at creation time, though > generation of it can be disabled so that it can be overridden. Also, > setters just return an error by default. > > * Property setters aren't called for object creation. Instead, the > relevant ObjectOptions branch is made available to some init method. > > * Runtime modifiable properties (declared as such in the schema) don't > get the default setter, so you have to provide an implementation for > them. I wouldn't bother with properties at all in the QAPI schema. Just do the first and third bullet. Declaring read-only QOM properties is trivial. > So while this series is doing only one part of the whole solution, that > the second part is missing doesn't make the first part wrong. Yeah, I think it's clear that for the long term we're not really disagreeing (or perhaps I'm even more radical than you :)). I'm just worried about having yet another incomplete transition. > One possibly nasty detail to consider there is that we sometimes declare > the USER_CREATABLE interface in the base class, so ucc->complete is for > the base class rather than the actually instantiated class. If we only > instantiate leaf classes (I think this is true), we can move > USER_CREATABLE there. You can also use a while loop covering each superclass to decide how to dispatch ucc->complete. I don't care much about these details, they're Simple Matter Of Programming. :) > I also had in mind just passing the whole configuration struct > (essentially always 'boxed': true), but you're right that individual > parameters like for commands would be possible. I'm not entirely > convinced that they would be better (there was a reason why we > introduced 'boxed': true), but it's an option. Having 'boxed': true by default would be just an implementation choice, nothing to worry about. (When I said the arguments would be the configuration, having a boxed struct as the argument would fit the description just as well). > I was hoping that by converting object-add in this series, and the CLI > options soon afterwards, it would be very obvious if you forget to > change the schema because your new property just wouldn't work (at least > not during creation). Converting the CLI options is not entirely trivial due to -readconfig and friends, so I was expecting that to last until that part of my 6.0 keyval work goes in. (It's almost ready for posting BTW, https://gitlab.com/bonzini/qemu/-/commit/b59288c86c). As soon as we have an idea of what we want UserCreatable to look in the end, on both the QAPI side and the object implementation side. That's also the part where we have the biggest need to document the schema. With that part at least roughly sketched out (no code needed), I'm okay with this series going in. I still don't like the triplication, but as George Michael puts it I just gotta have faith---because I must admit, I'm positively surprised at the ideas that came out of the discussion. Paolo
On Tue, Dec 01, 2020 at 10:23:57PM +0100, Paolo Bonzini wrote: > On 01/12/20 20:35, Kevin Wolf wrote: > > Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben: > > I don't think this is actually a new things. We already have types and > > commands declared with things like 'if': 'defined(TARGET_S390X)'. > > As far as I understand, QAPI generated files are already built per > > target, so not compiling things into all binaries should be entirely > > possible. > > There is some complication due to having different discriminators per > target. So yes it should be possible. But probably best left after objects > because it's so much bigger a task and because objects have a bit more > freedom for experimentation (less ties to other qdev-specific concepts, e.g. > the magic "bus" property). > > > So maybe only the abstract base class that actually defines the machine > > properties (like generic-pc-machine) should be described in QAPI, and > > then the concrete machine types would inherit from it without being > > described in QAPI themselves? > > Yes, maybe. > > > > 1) whether to generate _all_ boilerplate or only properties > > > > I would like to generate as much boilerplate as possible. That is, I > > don't want to constrain us to only properties, but at the same time, I'm > > not sure if it's possible to get rid of all boilerplate. > > > > Basically, the vision I have in mind is that QAPI would generate code > > that is the same for most instances, and then provide an option that > > prevents code generation for a specific part for more complicated cases, > > so that the respective function can (and must) be provided in the C > > source. > > Ok, so that's a bit different from what I am thinking of. I don't care very > much about the internal boilerplate, only the external interface for > configuration. So I don't care about type registration, dynamic cast macros > etc., only essentially the part that leads to ucc->complete. > > > > 2) whether we want to introduce a separation between configuration > > > schema and run-time state > > > > You mean the internal run-time state? How is this separation not already > > present with getter/setter functions for each property? In many cases > > they just directly access the run-time state, but there are other cases > > where they actually do things. > > I mean moving more towards the blockdev/chardev way of doing things, > increasing the separation very much by having separate configuration structs > that have (potentially) no link to the run-time state struct. > > > > 3) in the latter case, whether properties will survive at all---iothread and > > > throttle-groups don't really need them even if they're writable after > > > creation. > > > > How do you define properties, i.e. at which point would they stop > > existing and what would be a non-property alternative? > > Properties are only a useful concept if they have a use. If > -object/object_add/object-add can do the same job without properties, > properties are not needed anymore. Do you mean "not needed for -object anymore"? Properties are still used by internal C code (esp. board code), -device/device_add, -machine, -cpu, and debugging commands (like "info qtree" and qom-list/qom-get/qom-set). > > Right now QOM is all about exposing properties, and having multiple > interfaces to set them (by picking a different visitor). But in practice > most QOM objects have a lifetime that consists of 1) set properties 2) flip > a switch (realized/complete/open) 3) let the object live on its own. 1+2 > are a single monitor command or CLI option; during 3 you access the object > through monitor commands, not properties. I agree with this, except for the word "all" in "QOM is all about". QOM is also an extensively used internal QEMU API, including internal usage of the QOM property system. > > > So in summary, it seems to me that the QOM way is more flexible because > > you can get both models out of it. Whether we actually need this > > flexibility I can't say. > > I'm thinking there's no need for it, but maybe I'm overly optimistic. > > > * Configuration options are described in the QAPI schema. This is mainly > > for object creation, but runtime modifiable properties are a subset of > > this. > > > > * Properties are generated for each option. By default, the getter > > just returns the value from the configuration at creation time, though > > generation of it can be disabled so that it can be overridden. Also, > > setters just return an error by default. > > > > * Property setters aren't called for object creation. Instead, the > > relevant ObjectOptions branch is made available to some init method. > > > > * Runtime modifiable properties (declared as such in the schema) don't > > get the default setter, so you have to provide an implementation for > > them. > > I wouldn't bother with properties at all in the QAPI schema. Just do the > first and third bullet. Declaring read-only QOM properties is trivial. I'm liking the direction this is taking. However, I would still like to have a clearer and feasible plan that would work for -device, -machine, and -cpu. > > > So while this series is doing only one part of the whole solution, that > > the second part is missing doesn't make the first part wrong. > > Yeah, I think it's clear that for the long term we're not really disagreeing > (or perhaps I'm even more radical than you :)). I'm just worried about > having yet another incomplete transition. > > > One possibly nasty detail to consider there is that we sometimes declare > > the USER_CREATABLE interface in the base class, so ucc->complete is for > > the base class rather than the actually instantiated class. If we only > > instantiate leaf classes (I think this is true), we can move > > USER_CREATABLE there. > > You can also use a while loop covering each superclass to decide how to > dispatch ucc->complete. I don't care much about these details, they're > Simple Matter Of Programming. :) > > > I also had in mind just passing the whole configuration struct > > (essentially always 'boxed': true), but you're right that individual > > parameters like for commands would be possible. I'm not entirely > > convinced that they would be better (there was a reason why we > > introduced 'boxed': true), but it's an option. > > Having 'boxed': true by default would be just an implementation choice, > nothing to worry about. (When I said the arguments would be the > configuration, having a boxed struct as the argument would fit the > description just as well). > > > I was hoping that by converting object-add in this series, and the CLI > > options soon afterwards, it would be very obvious if you forget to > > change the schema because your new property just wouldn't work (at least > > not during creation). > > Converting the CLI options is not entirely trivial due to -readconfig and > friends, so I was expecting that to last until that part of my 6.0 keyval > work goes in. (It's almost ready for posting BTW, > https://gitlab.com/bonzini/qemu/-/commit/b59288c86c). > > As soon as we have an idea of what we want UserCreatable to look in the end, > on both the QAPI side and the object implementation side. That's also the > part where we have the biggest need to document the schema. With that part > at least roughly sketched out (no code needed), I'm okay with this series > going in. > > I still don't like the triplication, but as George Michael puts it I just > gotta have faith---because I must admit, I'm positively surprised at the > ideas that came out of the discussion. > > Paolo >
On 01/12/20 23:08, Eduardo Habkost wrote: >> Properties are only a useful concept if they have a use. If >> -object/object_add/object-add can do the same job without properties, >> properties are not needed anymore. > > Do you mean "not needed for -object anymore"? Properties are > still used by internal C code (esp. board code), > -device/device_add, -machine, -cpu, and debugging commands (like > "info qtree" and qom-list/qom-get/qom-set). Yes. >> Right now QOM is all about exposing properties, and having multiple >> interfaces to set them (by picking a different visitor). But in practice >> most QOM objects have a lifetime that consists of 1) set properties 2) flip >> a switch (realized/complete/open) 3) let the object live on its own. 1+2 >> are a single monitor command or CLI option; during 3 you access the object >> through monitor commands, not properties. > > I agree with this, except for the word "all" in "QOM is all > about". QOM is also an extensively used internal QEMU API, > including internal usage of the QOM property system. Yeah, "all about exposing properties" includes internal usage. And you're right that some "phase 3" monitor commands do work at the property level (mostly "info qtree", but also "qom-get" because there are some cases of public run-time properties). > I'm liking the direction this is taking. However, I would still > like to have a clearer and feasible plan that would work for > -device, -machine, and -cpu. -cpu is not a problem since it's generally created with a static configuration (now done with global properties, in the future it could be a struct). -machine and -device in principle could be done the same way as -object, just through a different registry (_not_ a huge struct; that's an acceptable stopgap for -object but that's it). The static aka field properties would remain as read-only, with defaults moved to instance_init or realize. But there would be again "triplication" with a trivial conversion: 1) in the QAPI schema, e.g. 'num_queues': 'int16' 2) in the struct, "int16_t num_queues;" 3) in the realize function, s->num_queues = cfg->has_num_queues ? cfg->num_queues : 8; So having a mechanism for defaults in the QAPI schema would be good. Maybe 'num_queues': { 'type': 'int16', 'default': '8' }? I also need to review more the part of this code with respect to the application of global properties. I wonder if there are visitor tricks that we can do, so that global properties keep working but correspond to QAPI fields instead of QOM properties. Paolo
Am 01.12.2020 um 22:23 hat Paolo Bonzini geschrieben: > On 01/12/20 20:35, Kevin Wolf wrote: > > Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben: > > I don't think this is actually a new things. We already have types and > > commands declared with things like 'if': 'defined(TARGET_S390X)'. > > As far as I understand, QAPI generated files are already built per > > target, so not compiling things into all binaries should be entirely > > possible. > > There is some complication due to having different discriminators per > target. So yes it should be possible. We have conditional union variants in existing code, too. > But probably best left after objects because it's so much bigger a > task and because objects have a bit more freedom for experimentation > (less ties to other qdev-specific concepts, e.g. the magic "bus" > property). Yes, I would like to get user creatable objects to a state that we like and only then start converting other object types. I feel user creatable objects are a nice set of types that aren't so trivial that major problems could go unnoticed, but still a managable enough number that it doesn't matter much if we take one or two extra steps until we find our preferred shape. > > So maybe only the abstract base class that actually defines the machine > > properties (like generic-pc-machine) should be described in QAPI, and > > then the concrete machine types would inherit from it without being > > described in QAPI themselves? > > Yes, maybe. > > > > 1) whether to generate _all_ boilerplate or only properties > > > > I would like to generate as much boilerplate as possible. That is, I > > don't want to constrain us to only properties, but at the same time, I'm > > not sure if it's possible to get rid of all boilerplate. > > > > Basically, the vision I have in mind is that QAPI would generate code > > that is the same for most instances, and then provide an option that > > prevents code generation for a specific part for more complicated cases, > > so that the respective function can (and must) be provided in the C > > source. > > Ok, so that's a bit different from what I am thinking of. I don't > care very much about the internal boilerplate, only the external > interface for configuration. So I don't care about type registration, > dynamic cast macros etc., only essentially the part that leads to > ucc->complete. Once QAPI knows about the class hierarchy you get the internal boilerplate generation almost for free, so I'm not sure why we would pass on it. But I agree it's not critical to have it. > > > 2) whether we want to introduce a separation between configuration > > > schema and run-time state > > > > You mean the internal run-time state? How is this separation not already > > present with getter/setter functions for each property? In many cases > > they just directly access the run-time state, but there are other cases > > where they actually do things. > > I mean moving more towards the blockdev/chardev way of doing things, > increasing the separation very much by having separate configuration > structs that have (potentially) no link to the run-time state struct. I'm not sure, I'm seeing pros and contras. Also, to be honest, I'm quite certain that the blockdev/chardev way is accidental rather than the result of a careful design process. Having to copy every option from the separate configuration into the run-time state is somewhat inconvenient. On the other hand, it ensures that every option is touched in the initialisation function, which probably increases chances that it's checked for validity. The separation might have made the kind of bugs more obvious where property setters just change the configuration without actually applying the updated value. I guess we might end up with a mixed model where configuration is separated into a different struct (the ObjectOptions branches), but still kept around for the lifetime of the object so that qom-get can keep working. > > > 3) in the latter case, whether properties will survive at all---iothread and > > > throttle-groups don't really need them even if they're writable after > > > creation. > > > > How do you define properties, i.e. at which point would they stop > > existing and what would be a non-property alternative? > > Properties are only a useful concept if they have a use. If > -object/object_add/object-add can do the same job without properties, > properties are not needed anymore. Yes, I think object creation doesn't need properties. But no, that doesn't automatically make them useless because of property updates later on. If you want to get fully rid of them, you need a replacement for the latter. But you're right that I would like to make property setters only about runtime changes (i.e. changes after creation) rather than part of the creation itself. When they have only one job to do, it's more likely that they actually implement something working for it. I guess fully getting rid of them and removing qom-set/get in favour of something new could be a future step, but feature removal requires code changes in libvirt etc. so let's try to stay compatible for now. It's hard enough without dealing with changes to external interfaces. > Right now QOM is all about exposing properties, and having multiple > interfaces to set them (by picking a different visitor). But in practice > most QOM objects have a lifetime that consists of 1) set properties 2) flip > a switch (realized/complete/open) 3) let the object live on its own. 1+2 > are a single monitor command or CLI option; during 3 you access the object > through monitor commands, not properties. True, and you don't need properties for this case. > > So in summary, it seems to me that the QOM way is more flexible because > > you can get both models out of it. Whether we actually need this > > flexibility I can't say. > > I'm thinking there's no need for it, but maybe I'm overly optimistic. I think you can express everything with the blockdev-reopen style. The client just happens to repeat itself a lot then, and updating the full state atomically might be a bit harder than doing just every property change individually. But there is probably nothing that can be done with individual properties, but not with an all-or-nothing update mechanism. > > * Configuration options are described in the QAPI schema. This is mainly > > for object creation, but runtime modifiable properties are a subset of > > this. > > > > * Properties are generated for each option. By default, the getter > > just returns the value from the configuration at creation time, though > > generation of it can be disabled so that it can be overridden. Also, > > setters just return an error by default. > > > > * Property setters aren't called for object creation. Instead, the > > relevant ObjectOptions branch is made available to some init method. > > > > * Runtime modifiable properties (declared as such in the schema) don't > > get the default setter, so you have to provide an implementation for > > them. > > I wouldn't bother with properties at all in the QAPI schema. Just do the > first and third bullet. Declaring read-only QOM properties is trivial. Trivial sounds like it's something the computer should be doing. But there are two more important reasons why I think QAPI should take care of this: The first is that I want to get rid of the duplication that properties have to be defined both in the code and as creation options in QAPI. One place to describe them is enough. Otherwise you might end up with options that can't be queried later because you forgot to manually add the trivial getter property. The second is that the QAPI schema should actually be a full description of the external interface of an object, and properties are part of this. So if we don't describe in it which options are available as (writable) properties after object creation, the schema will tell only half of the story. > > So while this series is doing only one part of the whole solution, that > > the second part is missing doesn't make the first part wrong. > > Yeah, I think it's clear that for the long term we're not really disagreeing > (or perhaps I'm even more radical than you :)). I'm just worried about > having yet another incomplete transition. Would you really feel at home in a QEMU without incomplete transitions? :-) More seriously, I'm certain that we can get the transition completed for user creatable options in a reasonable timeframe. And they form a separate group, as your suggestion to use ucc->complete to implement things shows, so it wouldn't even be an incomplete transition if we stopped there instead of extending the concept to other parts. qdev is a lot larger and it's unrealistic to change all devices at once, so I can see the risk of an incomplete transition there. But if you don't want to take this risk, you can't change anything. > > I was hoping that by converting object-add in this series, and the CLI > > options soon afterwards, it would be very obvious if you forget to > > change the schema because your new property just wouldn't work (at least > > not during creation). > > Converting the CLI options is not entirely trivial due to -readconfig and > friends, so I was expecting that to last until that part of my 6.0 keyval > work goes in. (It's almost ready for posting BTW, > https://gitlab.com/bonzini/qemu/-/commit/b59288c86c). Yes, -readconfig is what would be in the way when doing the change on master. But since you had already posted RFC patches a while ago to address this, I considered it solved. > As soon as we have an idea of what we want UserCreatable to look in the end, > on both the QAPI side and the object implementation side. That's also the > part where we have the biggest need to document the schema. With that part > at least roughly sketched out (no code needed), I'm okay with this series > going in. I think so far we agree on these steps: 1. This series (mostly for documentation and introspection) 2. -object and HMP object_add (so that we get QAPI's validation, and to make sure that forgetting to update the schema means that the new options just doesn't work) 3. Create a separate 'object' entity in QAPI, generate ucc->complete implementations that call a create function in the C source code with type-specific parameters (like QMP command handlers) What's still open: Should QAPI cover run-time properties, too? Should run-time properties even exist at all in the final state? What is the exact QAPI representation of everything? (The last one includes that I need to have a closer look at how QAPI can best be taught about inheritance.) Kevin
Am 02.12.2020 um 10:30 hat Paolo Bonzini geschrieben: > On 01/12/20 23:08, Eduardo Habkost wrote: > > > Properties are only a useful concept if they have a use. If > > > -object/object_add/object-add can do the same job without properties, > > > properties are not needed anymore. > > > > Do you mean "not needed for -object anymore"? Properties are > > still used by internal C code (esp. board code), > > -device/device_add, -machine, -cpu, and debugging commands (like > > "info qtree" and qom-list/qom-get/qom-set). > > Yes. Are internal uses mostly just right after object creation, or do we make a lot of use of them during runtime? > > > Right now QOM is all about exposing properties, and having multiple > > > interfaces to set them (by picking a different visitor). But in practice > > > most QOM objects have a lifetime that consists of 1) set properties 2) flip > > > a switch (realized/complete/open) 3) let the object live on its own. 1+2 > > > are a single monitor command or CLI option; during 3 you access the object > > > through monitor commands, not properties. > > > > I agree with this, except for the word "all" in "QOM is all > > about". QOM is also an extensively used internal QEMU API, > > including internal usage of the QOM property system. > > Yeah, "all about exposing properties" includes internal usage. And you're > right that some "phase 3" monitor commands do work at the property level > (mostly "info qtree", but also "qom-get" because there are some cases of > public run-time properties). > > > I'm liking the direction this is taking. However, I would still > > like to have a clearer and feasible plan that would work for > > -device, -machine, and -cpu. > > -cpu is not a problem since it's generally created with a static > configuration (now done with global properties, in the future it could be a > struct). > > -machine and -device in principle could be done the same way as -object, > just through a different registry (_not_ a huge struct; that's an acceptable > stopgap for -object but that's it). The static aka field properties would > remain as read-only, with defaults moved to instance_init or realize. But > there would be again "triplication" with a trivial conversion: > > 1) in the QAPI schema, e.g. 'num_queues': 'int16' > > 2) in the struct, "int16_t num_queues;" This one is optional, you can use the QAPI type even in the run-time state. I guess this goes back to how much separation you want between the configuration and the internal state. > 3) in the realize function, > > s->num_queues = cfg->has_num_queues ? cfg->num_queues : 8; > > So having a mechanism for defaults in the QAPI schema would be good. Maybe > 'num_queues': { 'type': 'int16', 'default': '8' }? Defaults have been on the QAPI wishlist for a long time, and everyone agrees that it would be nice to have them. Maybe it's time to finally implement them. > I also need to review more the part of this code with respect to the > application of global properties. I wonder if there are visitor tricks that > we can do, so that global properties keep working but correspond to QAPI > fields instead of QOM properties. Kevin
On 02/12/20 11:38, Kevin Wolf wrote: > Am 02.12.2020 um 10:30 hat Paolo Bonzini geschrieben: >> On 01/12/20 23:08, Eduardo Habkost wrote: >>>> Properties are only a useful concept if they have a use. If >>>> -object/object_add/object-add can do the same job without properties, >>>> properties are not needed anymore. >>> >>> Do you mean "not needed for -object anymore"? Properties are >>> still used by internal C code (esp. board code), >>> -device/device_add, -machine, -cpu, and debugging commands (like >>> "info qtree" and qom-list/qom-get/qom-set). >> >> Yes. > > Are internal uses mostly just right after object creation, or do we make > a lot of use of them during runtime? Not "a lot" but enough to be nasty. There are a couple uses of "well-known" QOM properties (e.g. to access the RTC time or the balloon stats), plus "info qtree". Paolo
On 02/12/20 11:27, Kevin Wolf wrote: >> Declaring read-only QOM properties is trivial. > > Trivial sounds like it's something the computer should be doing. Possibly, but not necessarily. There's always a cost to automatic code generation. If things are _too_ trivial and easy to get right, the cost of automatic code generation exceeds the cost of doing things by hand. You pretty much proved that ucc->complete is hard enough to get right, that it benefits from code generation. But I am a bit more conservative about extending that to the rest of QOM. >> I'm just worried about having yet another incomplete transition. > > Would you really feel at home in a QEMU without incomplete transitions? One can always dream. :) > But since you had already posted RFC patches a while ago to > address this, I considered it solved. Yup, I posted the non-RFC today. > 1. This series (mostly for documentation and introspection) > > 2. -object and HMP object_add (so that we get QAPI's validation, and to > make sure that forgetting to update the schema means that the new > options just doesn't work) > > 3. Create a separate 'object' entity in QAPI, generate ucc->complete > implementations that call a create function in the C source code with > type-specific parameters (like QMP command handlers) If we agree on all of these that's already a pretty good place to be in. The decreasing length of the replies to the thread suggests that we are. I wouldn't mind an example of (3) that is hand-coded and may only work for one object type (even a simple one such as iothread), to show what the generated QAPI code would look like. It's not a blocker as far as I am concerned, but it would be nice. More important, Markus and John should agree with the plan before (1) is committed. That may also require the aforementioned example, but it's up to them. > What's still open: Should QAPI cover run-time properties, too? Should > run-time properties even exist at all in the final state? What is the > exact QAPI representation of everything? (The last one includes that I > need to have a closer look at how QAPI can best be taught about > inheritance.) Run-time properties will exist but they will probably be cut down substantially, and most of them will be read-only (they already are as far as external code is concerned, i.e. they have a setter but qom-set always fails because it's called too late). Paolo
On Wed, Dec 02, 2020 at 10:30:11AM +0100, Paolo Bonzini wrote: > On 01/12/20 23:08, Eduardo Habkost wrote: > > > Properties are only a useful concept if they have a use. If > > > -object/object_add/object-add can do the same job without properties, > > > properties are not needed anymore. > > > > Do you mean "not needed for -object anymore"? Properties are > > still used by internal C code (esp. board code), > > -device/device_add, -machine, -cpu, and debugging commands (like > > "info qtree" and qom-list/qom-get/qom-set). > > Yes. > > > > Right now QOM is all about exposing properties, and having multiple > > > interfaces to set them (by picking a different visitor). But in practice > > > most QOM objects have a lifetime that consists of 1) set properties 2) flip > > > a switch (realized/complete/open) 3) let the object live on its own. 1+2 > > > are a single monitor command or CLI option; during 3 you access the object > > > through monitor commands, not properties. > > > > I agree with this, except for the word "all" in "QOM is all > > about". QOM is also an extensively used internal QEMU API, > > including internal usage of the QOM property system. > > Yeah, "all about exposing properties" includes internal usage. And you're > right that some "phase 3" monitor commands do work at the property level > (mostly "info qtree", but also "qom-get" because there are some cases of > public run-time properties). I still disagree on the "all about" part even for internal usage. But this shouldn't really matter for this discussion, I guess. > > > I'm liking the direction this is taking. However, I would still > > like to have a clearer and feasible plan that would work for > > -device, -machine, and -cpu. > > -cpu is not a problem since it's generally created with a static > configuration (now done with global properties, in the future it could be a > struct). It is a problem if it requires manually converting existing code defining CPU properties and we don't have a transition plan. > > -machine and -device in principle could be done the same way as -object, > just through a different registry (_not_ a huge struct; that's an acceptable > stopgap for -object but that's it). The static aka field properties would > remain as read-only, with defaults moved to instance_init or realize. But > there would be again "triplication" with a trivial conversion: > > 1) in the QAPI schema, e.g. 'num_queues': 'int16' > > 2) in the struct, "int16_t num_queues;" > > 3) in the realize function, > > s->num_queues = cfg->has_num_queues ? cfg->num_queues : 8; > > So having a mechanism for defaults in the QAPI schema would be good. Maybe > 'num_queues': { 'type': 'int16', 'default': '8' }? > Would a -device conversion also involve non-user-creatable devices, or would we keep existing internal usage of QOM properties? Even if it's just for user-creatable devices, getting rid of QOM property usage in devices sounds like a very ambitious goal. I'd like us to have a good transition plan, in addition to declaring what's our ideal end goal. > I also need to review more the part of this code with respect to the > application of global properties. I wonder if there are visitor tricks that > we can do, so that global properties keep working but correspond to QAPI > fields instead of QOM properties. > > Paolo >
On 02/12/20 13:51, Eduardo Habkost wrote: >>> I'm liking the direction this is taking. However, I would still >>> like to have a clearer and feasible plan that would work for >>> -device, -machine, and -cpu. >> >> -cpu is not a problem since it's generally created with a static >> configuration (now done with global properties, in the future it could be a >> struct). > > It is a problem if it requires manually converting existing code > defining CPU properties and we don't have a transition plan. We do not have to convert everything _if_ for some objects there are good reasons to do programmatically-generated properties. CPUs might be one of those cases (or if we decide to convert them, they might endure some more code duplication than other devices because they have so many properties). > Would a -device conversion also involve non-user-creatable > devices, or would we keep existing internal usage of QOM > properties? > > Even if it's just for user-creatable devices, getting rid of QOM > property usage in devices sounds like a very ambitious goal. I'd > like us to have a good transition plan, in addition to declaring > what's our ideal end goal. For user-creatable objects Kevin is doing work in lockstep on all classes; but once we have the infrastructure for QAPI object configuration schemas we can proceed in the other direction and operate on one device at a time. With some handwaving, something like (see create_unimplemented_device) DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE); qdev_prop_set_string(dev, "name", name); qdev_prop_set_uint64(dev, "size", size); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); might become something like { 'object': 'unimplemented-device', 'config': { 'name': 'str', 'size': 'size' }, } DeviceState *dev = qapi_Unimplemented_new(&( (UnimplementedDeviceOptions) { .name = name, .size = size }, &error_fatal); object_unref(dev); (i.e. all typesafe!) and the underlying code would be something like void (*init_properties)(Object *obj, Visitor *v, Error **errp); ... // QAPI generated constructor UnimplementedDevice *qapi_Unimplemented_new( UnimplementedDeviceOptions *opt, Error **errp) { Object *obj = object_new(TYPE_UNIMPLEMENTED_DEVICE); if (!qapi_Unimplemented_init_properties(obj, opt, errp)) { object_unref(obj); return NULL; } return obj; } // QAPI generated Visitor->struct adapter bool qapi_Unimplemented_init_properties( Object *obj, Visitor *v, Error **errp) { UnimplementedDeviceOptions opt; Error *local_err = NULL; visit_type_UnimplementedDeviceOptions(v, NULL, &opt, &local_err); if (local_err) { error_propagate(errp, local_err); return false; } unimplemented_init_properties(UNIMPLEMENTED_DEVICE(obj), &opt, &local_err); if (local_err) { error_propagate(errp, local_err); return false; } return true; } // Hand-written (?) initializer: void unimplemented_init_properties(Object *obj, UnimplementedDeviceOptions *opt, Error **errp) { obj->name = name; obj->size = size; device_realize(obj, errp); } // class_init code: oc->init_properties = qapi_Unimplemented_init_properties; and all read-only-after-realize QOM properties could be made *really* read-only. Paolo
On Wed, Dec 02, 2020 at 02:26:44PM +0100, Paolo Bonzini wrote: > On 02/12/20 13:51, Eduardo Habkost wrote: > > > > I'm liking the direction this is taking. However, I would still > > > > like to have a clearer and feasible plan that would work for > > > > -device, -machine, and -cpu. > > > > > > -cpu is not a problem since it's generally created with a static > > > configuration (now done with global properties, in the future it could be a > > > struct). > > > > It is a problem if it requires manually converting existing code > > defining CPU properties and we don't have a transition plan. > > We do not have to convert everything _if_ for some objects there are good > reasons to do programmatically-generated properties. CPUs might be one of > those cases (or if we decide to convert them, they might endure some more > code duplication than other devices because they have so many properties). OK, we just need to agree on what the transition will look like when we do it. I think we should put as much care into transition/glue infrastructure as we put into the new infrastructure. > > > Would a -device conversion also involve non-user-creatable > > devices, or would we keep existing internal usage of QOM > > properties? > > > > Even if it's just for user-creatable devices, getting rid of QOM > > property usage in devices sounds like a very ambitious goal. I'd > > like us to have a good transition plan, in addition to declaring > > what's our ideal end goal. > > For user-creatable objects Kevin is doing work in lockstep on all classes; > but once we have the infrastructure for QAPI object configuration schemas we > can proceed in the other direction and operate on one device at a time. > > With some handwaving, something like (see create_unimplemented_device) > > DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE); > > qdev_prop_set_string(dev, "name", name); > qdev_prop_set_uint64(dev, "size", size); > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); > > might become something like > > { 'object': 'unimplemented-device', > 'config': { > 'name': 'str', > 'size': 'size' > }, > } > > DeviceState *dev = qapi_Unimplemented_new(&( > (UnimplementedDeviceOptions) { > .name = name, > .size = size > }, &error_fatal); > object_unref(dev); > > (i.e. all typesafe!) and the underlying code would be something like [...] > Looks nice as end goal. Then, these are a few questions I would have about the transition plan: Would it require changing both device implementation and device users in lockstep? Should we have a compatibility layer to allow existing qdev_new()+qdev_prop_set_*() code to keep working after the devices are converted to the new system? If not, why not? If we add a compatibility layer, is the end goal to convert all existing qdev_new() users to the new system? If yes, why? If not, why not? What about subclasses? Would base classes need to be converted in lockstep with all subclasses? How would the transition process of (e.g.) PCI devices look like?
Am 02.12.2020 um 14:54 hat Eduardo Habkost geschrieben: > On Wed, Dec 02, 2020 at 02:26:44PM +0100, Paolo Bonzini wrote: > > On 02/12/20 13:51, Eduardo Habkost wrote: > > > > > I'm liking the direction this is taking. However, I would still > > > > > like to have a clearer and feasible plan that would work for > > > > > -device, -machine, and -cpu. > > > > > > > > -cpu is not a problem since it's generally created with a static > > > > configuration (now done with global properties, in the future it could be a > > > > struct). > > > > > > It is a problem if it requires manually converting existing code > > > defining CPU properties and we don't have a transition plan. > > > > We do not have to convert everything _if_ for some objects there are good > > reasons to do programmatically-generated properties. CPUs might be one of > > those cases (or if we decide to convert them, they might endure some more > > code duplication than other devices because they have so many properties). > > OK, we just need to agree on what the transition will look like > when we do it. I think we should put as much care into > transition/glue infrastructure as we put into the new > infrastructure. > > > > > > > Would a -device conversion also involve non-user-creatable > > > devices, or would we keep existing internal usage of QOM > > > properties? > > > > > > Even if it's just for user-creatable devices, getting rid of QOM > > > property usage in devices sounds like a very ambitious goal. I'd > > > like us to have a good transition plan, in addition to declaring > > > what's our ideal end goal. > > > > For user-creatable objects Kevin is doing work in lockstep on all classes; > > but once we have the infrastructure for QAPI object configuration schemas we > > can proceed in the other direction and operate on one device at a time. > > > > With some handwaving, something like (see create_unimplemented_device) > > > > DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE); > > > > qdev_prop_set_string(dev, "name", name); > > qdev_prop_set_uint64(dev, "size", size); > > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); > > > > might become something like > > > > { 'object': 'unimplemented-device', > > 'config': { > > 'name': 'str', > > 'size': 'size' > > }, > > } > > > > DeviceState *dev = qapi_Unimplemented_new(&( > > (UnimplementedDeviceOptions) { > > .name = name, > > .size = size > > }, &error_fatal); > > object_unref(dev); > > > > (i.e. all typesafe!) and the underlying code would be something like > [...] > > > > Looks nice as end goal. Then, these are a few questions I would > have about the transition plan: > > Would it require changing both device implementation and device > users in lockstep? Should we have a compatibility layer to allow > existing qdev_new()+qdev_prop_set_*() code to keep working after > the devices are converted to the new system? If not, why not? Technically, it doesn't strictly require a lockstep update. You can have two code paths leading to a fully initialised device, one being the traditional way of setting properties and finally calling dc->realize, the other being a new method that takes the configuration in its parameters and also sets dev->realized = true at the end. If at all possible, I would however prefer a lockstep update because having two paths is a weird intermediate state and the code paths could easily diverge. Keeping the old way around for a device also means that property setters are still doing two different jobs (initial configuration and updates at runtime). > If we add a compatibility layer, is the end goal to convert all > existing qdev_new() users to the new system? If yes, why? If > not, why not? My personal goal is covering -object and -device, i.e. the external interfaces. Converting purely internally created devices is not as interesting (especially as long as we focus only on object creation), but might be desirable for consistency. > What about subclasses? Would base classes need to be converted > in lockstep with all subclasses? How would the transition > process of (e.g.) PCI devices look like? I don't think so. If you want to convert base classes first, you may need to take the path shown above where both initialisation paths coexist while the children are converted because instantiation of a child class involves setting properties of the base class. So you can only restrict these properties to runtime-only after all children have been converted. The other way around might be easier: You will need to describe the properties of base classes in the QAPI schema from the beginning, but that doesn't mean that their initialisation code has to change just yet. The child classes will need to forward the part of their configuration that belongs to the base class. The downside is that this code will have to be changed again when the base class is finally converted. So we have options there, and we can decide case by case which one is most appropriate for the specific class to be converted (depending on how many child classes exist, how many properties are inherited, etc.) Kevin
On Wed, Dec 02, 2020 at 04:17:13PM +0100, Kevin Wolf wrote: > Am 02.12.2020 um 14:54 hat Eduardo Habkost geschrieben: > > On Wed, Dec 02, 2020 at 02:26:44PM +0100, Paolo Bonzini wrote: > > > On 02/12/20 13:51, Eduardo Habkost wrote: > > > > > > I'm liking the direction this is taking. However, I would still > > > > > > like to have a clearer and feasible plan that would work for > > > > > > -device, -machine, and -cpu. > > > > > > > > > > -cpu is not a problem since it's generally created with a static > > > > > configuration (now done with global properties, in the future it could be a > > > > > struct). > > > > > > > > It is a problem if it requires manually converting existing code > > > > defining CPU properties and we don't have a transition plan. > > > > > > We do not have to convert everything _if_ for some objects there are good > > > reasons to do programmatically-generated properties. CPUs might be one of > > > those cases (or if we decide to convert them, they might endure some more > > > code duplication than other devices because they have so many properties). > > > > OK, we just need to agree on what the transition will look like > > when we do it. I think we should put as much care into > > transition/glue infrastructure as we put into the new > > infrastructure. > > > > > > > > > > > Would a -device conversion also involve non-user-creatable > > > > devices, or would we keep existing internal usage of QOM > > > > properties? > > > > > > > > Even if it's just for user-creatable devices, getting rid of QOM > > > > property usage in devices sounds like a very ambitious goal. I'd > > > > like us to have a good transition plan, in addition to declaring > > > > what's our ideal end goal. > > > > > > For user-creatable objects Kevin is doing work in lockstep on all classes; > > > but once we have the infrastructure for QAPI object configuration schemas we > > > can proceed in the other direction and operate on one device at a time. > > > > > > With some handwaving, something like (see create_unimplemented_device) > > > > > > DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE); > > > > > > qdev_prop_set_string(dev, "name", name); > > > qdev_prop_set_uint64(dev, "size", size); > > > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); > > > > > > might become something like > > > > > > { 'object': 'unimplemented-device', > > > 'config': { > > > 'name': 'str', > > > 'size': 'size' > > > }, > > > } > > > > > > DeviceState *dev = qapi_Unimplemented_new(&( > > > (UnimplementedDeviceOptions) { > > > .name = name, > > > .size = size > > > }, &error_fatal); > > > object_unref(dev); > > > > > > (i.e. all typesafe!) and the underlying code would be something like > > [...] > > > > > > > Looks nice as end goal. Then, these are a few questions I would > > have about the transition plan: > > > > Would it require changing both device implementation and device > > users in lockstep? Should we have a compatibility layer to allow > > existing qdev_new()+qdev_prop_set_*() code to keep working after > > the devices are converted to the new system? If not, why not? > > Technically, it doesn't strictly require a lockstep update. You can have > two code paths leading to a fully initialised device, one being the > traditional way of setting properties and finally calling dc->realize, > the other being a new method that takes the configuration in its > parameters and also sets dev->realized = true at the end. > > If at all possible, I would however prefer a lockstep update because > having two paths is a weird intermediate state and the code paths could > easily diverge. Keeping the old way around for a device also means that > property setters are still doing two different jobs (initial > configuration and updates at runtime). I'd like to understand better how that intermediate state would look like and why there's risk of separate code paths diverging. Could we have an intermediate state that doesn't require any duplication and thus have no separate code paths that could diverge? > > > If we add a compatibility layer, is the end goal to convert all > > existing qdev_new() users to the new system? If yes, why? If > > not, why not? > > My personal goal is covering -object and -device, i.e. the external > interfaces. Converting purely internally created devices is not as > interesting (especially as long as we focus only on object creation), > but might be desirable for consistency. I wonder how much consistency we will lose and how much confusion we'll cause if we end up with two completely separate methods for creating devices. > > > What about subclasses? Would base classes need to be converted > > in lockstep with all subclasses? How would the transition > > process of (e.g.) PCI devices look like? > > I don't think so. > > If you want to convert base classes first, you may need to take the > path shown above where both initialisation paths coexist while the > children are converted because instantiation of a child class involves > setting properties of the base class. So you can only restrict these > properties to runtime-only after all children have been converted. > > The other way around might be easier: You will need to describe the > properties of base classes in the QAPI schema from the beginning, but > that doesn't mean that their initialisation code has to change just yet. > The child classes will need to forward the part of their configuration > that belongs to the base class. The downside is that this code will have > to be changed again when the base class is finally converted. > > So we have options there, and we can decide case by case which one is > most appropriate for the specific class to be converted (depending on > how many child classes exist, how many properties are inherited, etc.) Right now it's hard for me to understand what those intermediate states would look like. It sounds like it requires too many complicated manual changes to be done by humans, and lots of room for mistakes when maintaining two parallel code paths. I'd prefer to delegate the translation job to a machine. In other words, I'd prefer to have compatibility layer(s) that would make the same implementation work with the both old-style and new-style APIs for creating devices. Maybe this would involve generating QAPI code from QOM/qdev property lists, and/or generating qdev property registration code from the QAPI schema, I don't know. Providing a compatibility layer would also help us be more confident that there are no gaps in the abstractions provided by the two systems (QOM properties and QAPI schema) that we still need to address.
Am 02.12.2020 um 17:05 hat Eduardo Habkost geschrieben: > > > Looks nice as end goal. Then, these are a few questions I would > > > have about the transition plan: > > > > > > Would it require changing both device implementation and device > > > users in lockstep? Should we have a compatibility layer to allow > > > existing qdev_new()+qdev_prop_set_*() code to keep working after > > > the devices are converted to the new system? If not, why not? > > > > Technically, it doesn't strictly require a lockstep update. You can have > > two code paths leading to a fully initialised device, one being the > > traditional way of setting properties and finally calling dc->realize, > > the other being a new method that takes the configuration in its > > parameters and also sets dev->realized = true at the end. > > > > If at all possible, I would however prefer a lockstep update because > > having two paths is a weird intermediate state and the code paths could > > easily diverge. Keeping the old way around for a device also means that > > property setters are still doing two different jobs (initial > > configuration and updates at runtime). > > I'd like to understand better how that intermediate state would > look like and why there's risk of separate code paths diverging. > > Could we have an intermediate state that doesn't require any > duplication and thus have no separate code paths that could > diverge? The one requirement we have for an intermediate state is that it supports both interfaces: The well-know create/set properties/realize dance, and a new DeviceClass method, say .create(), that takes the configuration in parameters instead of relying on previously set properties. I assumed two separate implementations of transferring the configuration into the internal state. On second thought, this assumption is maybe wrong. You can implement the new method as wrapper around the old way: It could just set all the properties and call realize. Of course, you don't win much in terms of improving the class implementation this way, but just support the new interface, but I guess it can be a reasonable intermediate step to resolve complicated dependencies etc. It would be much nicer to do the wrapper the other way round, i.e. setting properties before the device is realized would update a configuration struct and realize would then call .create() with that struct. To me, this sounds much harder, though also a more useful state. As you have worked a lot with properties recently, maybe you have a good idea how we could get an intermediate state closer to this? > > > If we add a compatibility layer, is the end goal to convert all > > > existing qdev_new() users to the new system? If yes, why? If > > > not, why not? > > > > My personal goal is covering -object and -device, i.e. the external > > interfaces. Converting purely internally created devices is not as > > interesting (especially as long as we focus only on object creation), > > but might be desirable for consistency. > > I wonder how much consistency we will lose and how much confusion > we'll cause if we end up with two completely separate methods for > creating devices. I do think we should follow through and convert everything. It's just not my main motivation, and if the people who work more with qdev think it's better to leave that part unchanged (or that it won't make much of a difference), I won't insist. > > > What about subclasses? Would base classes need to be converted > > > in lockstep with all subclasses? How would the transition > > > process of (e.g.) PCI devices look like? > > > > I don't think so. > > > > If you want to convert base classes first, you may need to take the > > path shown above where both initialisation paths coexist while the > > children are converted because instantiation of a child class involves > > setting properties of the base class. So you can only restrict these > > properties to runtime-only after all children have been converted. > > > > The other way around might be easier: You will need to describe the > > properties of base classes in the QAPI schema from the beginning, but > > that doesn't mean that their initialisation code has to change just yet. > > The child classes will need to forward the part of their configuration > > that belongs to the base class. The downside is that this code will have > > to be changed again when the base class is finally converted. > > > > So we have options there, and we can decide case by case which one is > > most appropriate for the specific class to be converted (depending on > > how many child classes exist, how many properties are inherited, etc.) > > Right now it's hard for me to understand what those intermediate > states would look like. It sounds like it requires too many > complicated manual changes to be done by humans, and lots of room > for mistakes when maintaining two parallel code paths. I'd > prefer to delegate the translation job to a machine. Maybe devices are in a better shape, but my conclusion from user creatable objects is that it needs to be done by a human. Even just writing a schema for an existing object without actually changing its code (i.e. what this series does for user creatable objects) involves: * Figuring out which properties even exist. I guess if you know your way around QOM, this could be automatically be generated for the common case. If property definitions are conditional or dynamic, you'll probably miss them. * Finding the right data type for each property. The const char *type passed to object_(class_)property_add() is often enough wrong that it becomes useless. If object_property_add_<type> is used, chances are that you know the right type, but strings might actually be hidden enums. If not, figuring out the type involves analysing the setter and getter callbacks. * Finding out which properties are mandatory and which are optional. This is usually some handwritten check in complete/realize that returns an error. Sometimes it's just a segfault that happens sooner or later if the property hasn't been set. * Finding the default for documentation. There are multiple ways to do this. It's code, not data. * Writing (preferably good) documentation for the property. I see very little opportunity for automating a significant part of this. Once you have this information, going to the intermediate state where .create() is just a wrapper that sets properties and calls realize is fairly easy. Maybe we can have QAPI support for this so that you can request generation of the wrapper function in the schema. Then you would just have to set the pointer to it in .class_init. > In other words, I'd prefer to have compatibility layer(s) that > would make the same implementation work with the both old-style > and new-style APIs for creating devices. Maybe this would > involve generating QAPI code from QOM/qdev property lists, and/or > generating qdev property registration code from the QAPI schema, > I don't know. > > Providing a compatibility layer would also help us be more > confident that there are no gaps in the abstractions provided by > the two systems (QOM properties and QAPI schema) that we still > need to address. qdev properties could be more useful to generate at least a skeleton schema from than generic QOM properties. But there will still be large parts that a human needs to do. My concern with the compatibility layer is just that it could happen more easily that we're stuck with it forever. Kevin
On Wed, Dec 02, 2020 at 06:35:06PM +0100, Kevin Wolf wrote: > Am 02.12.2020 um 17:05 hat Eduardo Habkost geschrieben: > > > > Looks nice as end goal. Then, these are a few questions I would > > > > have about the transition plan: > > > > > > > > Would it require changing both device implementation and device > > > > users in lockstep? Should we have a compatibility layer to allow > > > > existing qdev_new()+qdev_prop_set_*() code to keep working after > > > > the devices are converted to the new system? If not, why not? > > > > > > Technically, it doesn't strictly require a lockstep update. You can have > > > two code paths leading to a fully initialised device, one being the > > > traditional way of setting properties and finally calling dc->realize, > > > the other being a new method that takes the configuration in its > > > parameters and also sets dev->realized = true at the end. > > > > > > If at all possible, I would however prefer a lockstep update because > > > having two paths is a weird intermediate state and the code paths could > > > easily diverge. Keeping the old way around for a device also means that > > > property setters are still doing two different jobs (initial > > > configuration and updates at runtime). > > > > I'd like to understand better how that intermediate state would > > look like and why there's risk of separate code paths diverging. > > > > Could we have an intermediate state that doesn't require any > > duplication and thus have no separate code paths that could > > diverge? > > The one requirement we have for an intermediate state is that it > supports both interfaces: The well-know create/set properties/realize > dance, and a new DeviceClass method, say .create(), that takes the > configuration in parameters instead of relying on previously set > properties. I agree completely. > > I assumed two separate implementations of transferring the configuration > into the internal state. On second thought, this assumption is maybe > wrong. > > You can implement the new method as wrapper around the old way: It could > just set all the properties and call realize. Of course, you don't win > much in terms of improving the class implementation this way, but just > support the new interface, but I guess it can be a reasonable > intermediate step to resolve complicated dependencies etc. > > It would be much nicer to do the wrapper the other way round, i.e. > setting properties before the device is realized would update a > configuration struct and realize would then call .create() with that > struct. To me, this sounds much harder, though also a more useful state. Comment about this below (look for [1]). > > As you have worked a lot with properties recently, maybe you have a good > idea how we could get an intermediate state closer to this? I'd have to re-read this whole thread and think about it. > > > > > If we add a compatibility layer, is the end goal to convert all > > > > existing qdev_new() users to the new system? If yes, why? If > > > > not, why not? > > > > > > My personal goal is covering -object and -device, i.e. the external > > > interfaces. Converting purely internally created devices is not as > > > interesting (especially as long as we focus only on object creation), > > > but might be desirable for consistency. > > > > I wonder how much consistency we will lose and how much confusion > > we'll cause if we end up with two completely separate methods for > > creating devices. > > I do think we should follow through and convert everything. It's just > not my main motivation, and if the people who work more with qdev think > it's better to leave that part unchanged (or that it won't make much of > a difference), I won't insist. This worries me. Converting thousands of lines of code that don't involve user-visible interfaces seems complicated and maybe pointless. On the other hand, having two separate APIs for creating objects internally would cause confusion. Maybe we should accept the fact that the 2 APIs will exist, and address the confusion part: we should guarantee the two APIs to be 100% equivalent, except for the fact that the newer one gives us type safety in the C code. I'd like to avoid a case like qdev vs QOM APIs, where they have similar but slightly different features, and nobody knows which one to use. > > > > > What about subclasses? Would base classes need to be converted > > > > in lockstep with all subclasses? How would the transition > > > > process of (e.g.) PCI devices look like? > > > > > > I don't think so. > > > > > > If you want to convert base classes first, you may need to take the > > > path shown above where both initialisation paths coexist while the > > > children are converted because instantiation of a child class involves > > > setting properties of the base class. So you can only restrict these > > > properties to runtime-only after all children have been converted. > > > > > > The other way around might be easier: You will need to describe the > > > properties of base classes in the QAPI schema from the beginning, but > > > that doesn't mean that their initialisation code has to change just yet. > > > The child classes will need to forward the part of their configuration > > > that belongs to the base class. The downside is that this code will have > > > to be changed again when the base class is finally converted. > > > > > > So we have options there, and we can decide case by case which one is > > > most appropriate for the specific class to be converted (depending on > > > how many child classes exist, how many properties are inherited, etc.) > > > > Right now it's hard for me to understand what those intermediate > > states would look like. It sounds like it requires too many > > complicated manual changes to be done by humans, and lots of room > > for mistakes when maintaining two parallel code paths. I'd > > prefer to delegate the translation job to a machine. > > Maybe devices are in a better shape, but my conclusion from user > creatable objects is that it needs to be done by a human. I agree with the "done by a human part", but I prefer a different approach: making the APIs converge (using human work), and then making an automated transition. More on this below: > > Even just writing a schema for an existing object without actually > changing its code (i.e. what this series does for user creatable > objects) involves: > > * Figuring out which properties even exist. > > I guess if you know your way around QOM, this could be automatically > be generated for the common case. If property definitions are > conditional or dynamic, you'll probably miss them. With qdev static properties and class-level properties, this stage is easy. > > * Finding the right data type for each property. > > The const char *type passed to object_(class_)property_add() is often > enough wrong that it becomes useless. If object_property_add_<type> is > used, chances are that you know the right type, but strings might > actually be hidden enums. If not, figuring out the type involves > analysing the setter and getter callbacks. With object_property_add_*() and object_class_property_add_*(), this part is very difficult. With qdev static properties (now known as "field properties" in QOM), this stage is easy. Luckily, most devices use qdev static properties. > > * Finding out which properties are mandatory and which are optional. > > This is usually some handwritten check in complete/realize that > returns an error. Sometimes it's just a segfault that happens sooner > or later if the property hasn't been set. This stage is tricky. But we can start by: - Making all of them optional by default (which is true); - Adding a flag to allow us to declare properties as mandatory. > > * Finding the default for documentation. > > There are multiple ways to do this. It's code, not data. In this case, qdev static properties / field properties make that job possible. > > * Writing (preferably good) documentation for the property. This part will require manual work. > > I see very little opportunity for automating a significant part of this. I see a big opportunity to automate this if the device is already using the field property system (which is true for most of the devices). The remaining work that needs to be done by humans is converting existing dynamic properties to field properties. About doing the compatibility layer the other way around[1]: if conversion from qdev properties to QAPI schema is automated, we can simply choose a point in time where the results of the conversion would be saved in the git tree, and the original property list would be deleted from the C source. Then, the compatibility layer would work the other way around. > > Once you have this information, going to the intermediate state where > .create() is just a wrapper that sets properties and calls realize is > fairly easy. Maybe we can have QAPI support for this so that you can > request generation of the wrapper function in the schema. Then you would > just have to set the pointer to it in .class_init. Agreed. > > > In other words, I'd prefer to have compatibility layer(s) that > > would make the same implementation work with the both old-style > > and new-style APIs for creating devices. Maybe this would > > involve generating QAPI code from QOM/qdev property lists, and/or > > generating qdev property registration code from the QAPI schema, > > I don't know. > > > > Providing a compatibility layer would also help us be more > > confident that there are no gaps in the abstractions provided by > > the two systems (QOM properties and QAPI schema) that we still > > need to address. > > qdev properties could be more useful to generate at least a skeleton > schema from than generic QOM properties. But there will still be large > parts that a human needs to do. I agree there's lot of manual work to do. My proposal is doing the manual work during in the conversion of dynamic properties to field properties. Conversion from field properties to the new API then would be automated. > > My concern with the compatibility layer is just that it could happen > more easily that we're stuck with it forever. I hear you. I'm a pessimistic regarding this. I believe we _will_ be stuck in the transition forever (or for so long that it will feel like forever). But with a good transition/compatibility layer, that transition stage will be less painful. I also believe that writing compatibility layer(s) would be less work than writing and reviewing patches doing manual conversion of device code directly to the new API.
Hi, > It would be much nicer to do the wrapper the other way round, i.e. > setting properties before the device is realized would update a > configuration struct and realize would then call .create() with that > struct. To me, this sounds much harder, though also a more useful state. Well, in some places we already have separate config structs. We have NICConf for example, which is typically used like this: struct USBNetState { USBDevice dev; [ ... ] NICConf conf; [ ... ] }; and static Property net_properties[] = { DEFINE_NIC_PROPERTIES(USBNetState, conf), DEFINE_PROP_END_OF_LIST(), }; So I think we could: (1) move *all* properties into structs. (2) generate those structs from qapi schemas. (3) generate Property lists (or functions with object_class_property_add_*() calls) from qapi schema. We could then convert devices one-by-one without breaking anything or needing two code paths essentially doing the same thing in two different ways. take care, Gerd
On 02/12/20 18:35, Kevin Wolf wrote: >> Could we have an intermediate state that doesn't require any >> duplication and thus have no separate code paths that could >> diverge? > > The one requirement we have for an intermediate state is that it > supports both interfaces: The well-know create/set properties/realize > dance, and a new DeviceClass method, say .create(), that takes the > configuration in parameters instead of relying on previously set > properties. > > I assumed two separate implementations of transferring the configuration > into the internal state. On second thought, this assumption is maybe > wrong. > > You can implement the new method as wrapper around the old way: It could > just set all the properties and call realize. Of course, you don't win > much in terms of improving the class implementation this way, but just > support the new interface, but I guess it can be a reasonable > intermediate step to resolve complicated dependencies etc. I sketched something at https://wiki.qemu.org/Features/QOM-QAPI_integration. The main difference with what we discussed so far is that it doesn't subsume the complete/realize step, only the setting of properties. The idea is that user_creatable_add_type does the following: - calls an oc->configure method on every superclass of the object - takes what's left of the input visitor and uses it to set properties - then calls ucc->complete So in the end the only new step is the first. If the first two steps are bundled in a new function object_configure, they can be reused for devices, machines and accelerators. The QAPI code generator can also easily wrap them into a C API for QOM object creation. I glossed completely over the generation of properties within the QAPI code generator. Making properties read-only (or, in the field-properties world, having a default allow_set of "return false") is already a nice improvement over > It would be much nicer to do the wrapper the other way round, i.e. > setting properties before the device is realized would update a > configuration struct and realize would then call .create() with that > struct. To me, this sounds much harder, though also a more useful state. This sounds much harder. However, based on the RngEgd sample, going from a basic QAPI struct to a full conversion is not too hard and makes code shorter. So it's win-win even though it's human work. Paolo
On Thu, Dec 03, 2020 at 07:46:29AM +0100, Gerd Hoffmann wrote: > Hi, > > > It would be much nicer to do the wrapper the other way round, i.e. > > setting properties before the device is realized would update a > > configuration struct and realize would then call .create() with that > > struct. To me, this sounds much harder, though also a more useful state. > > Well, in some places we already have separate config structs. We have > NICConf for example, which is typically used like this: > > struct USBNetState { > USBDevice dev; > [ ... ] > NICConf conf; > [ ... ] > }; > > and > > static Property net_properties[] = { > DEFINE_NIC_PROPERTIES(USBNetState, conf), > DEFINE_PROP_END_OF_LIST(), > }; > > So I think we could: > > (1) move *all* properties into structs. > (2) generate those structs from qapi schemas. > (3) generate Property lists (or functions with > object_class_property_add_*() calls) from qapi > schema. > > We could then convert devices one-by-one without breaking anything > or needing two code paths essentially doing the same thing in two > different ways. Sounds great to me. This can also work the other way around for devices that weren't converted yet: we should be able to generate a QAPI schema from the property lists.
Am 03.12.2020 um 12:11 hat Paolo Bonzini geschrieben: > On 02/12/20 18:35, Kevin Wolf wrote: > > > Could we have an intermediate state that doesn't require any > > > duplication and thus have no separate code paths that could > > > diverge? > > > > The one requirement we have for an intermediate state is that it > > supports both interfaces: The well-know create/set properties/realize > > dance, and a new DeviceClass method, say .create(), that takes the > > configuration in parameters instead of relying on previously set > > properties. > > > > I assumed two separate implementations of transferring the configuration > > into the internal state. On second thought, this assumption is maybe > > wrong. > > > > You can implement the new method as wrapper around the old way: It could > > just set all the properties and call realize. Of course, you don't win > > much in terms of improving the class implementation this way, but just > > support the new interface, but I guess it can be a reasonable > > intermediate step to resolve complicated dependencies etc. > > I sketched something at https://wiki.qemu.org/Features/QOM-QAPI_integration. > > The main difference with what we discussed so far is that it doesn't subsume > the complete/realize step, only the setting of properties. The idea is that > user_creatable_add_type does the following: > > - calls an oc->configure method on every superclass of the object > > - takes what's left of the input visitor and uses it to set properties > > - then calls ucc->complete > > So in the end the only new step is the first. If the first two steps are > bundled in a new function object_configure, they can be reused for devices, > machines and accelerators. > > The QAPI code generator can also easily wrap them into a C API for QOM > object creation. > > I glossed completely over the generation of properties within the QAPI code > generator. Making properties read-only (or, in the field-properties world, > having a default allow_set of "return false") is already a nice improvement > over I don't think this is an intermediate state like Eduardo wants to have. Creating the object, then setting properties, then realize [1] will fail after your change. But keeping it working was the whole point of the exercise. I'm also not really sure why you go from RngEgdOptions to QObject to a visitor, only to reconstruct RngEgdOptions at the end. I think the class implementations should have a normal C interface without visitors and we should be able to just pass the existing RngEgdOptions object (or the individual values for its fields for 'boxed': false). Kevin [1] Or complete for ucc, but the number of these is small enough that we don't really need any intermediate state, except maybe as a proof of concept for the later qdev conversion.
On 03/12/20 16:15, Kevin Wolf wrote: > I don't think this is an intermediate state like Eduardo wants to have. > Creating the object, then setting properties, then realize [1] will fail > after your change. But keeping it working was the whole point of the > exercise. With the sample code, you must remove object_class_property_set calls at the same time as you remove the setters. Usually that'd be when you convert to QAPI and oc->configure, but it doesn't have to be that way if there are good reasons not to do so. Also, it still allows you to do so one class at a time, and I *think* the presence of subclasses or superclasses doesn't matter (only whether properties are still writable). We can use chardevs (see ChardevCommon in qapi/char.json) to validate that before tackling devices. (In fact, this means that your series---plus -object and object_add conversion---would be good, pretty much unchanged, as a first step. The second would be adding oc->configure and object_configure, and converting all user-creatable objects to oc->configure. The third would involve QAPI code generation). > I'm also not really sure why you go from RngEgdOptions to QObject to a > visitor, only to reconstruct RngEgdOptions at the end. The two visits are just because you cannot create an input visitor directly on C data. I stole that from your patch 18/18 actually, just with object_new+object_configure instead of user_creatable_add_type. But I wouldn't read too much in the automatically-generated *_new functions since they are already in QAPI code generator territory. Instead the basic object_configure idea can be applied even without having automatic code generation. > I think the class > implementations should have a normal C interface without visitors and we > should be able to just pass the existing RngEgdOptions object (or the > individual values for its fields for 'boxed': false). Sure, however that requires changes to the QAPI code generator which was only item (3) in your list list. Until then you can already work with a visitor interface: void rng_egd_configure(Object *obj, Visitor *v, Error **errp) { RngEgd *s = RNG_EGD(obj); s->config = g_new0(MemoryBackendOptions, 1); visit_type_MemoryBackendOptions(v, NULL, &s->config, errp); s->config->share = (s->config->has_share ? s->config->share : false); ... } but if you had a QAPI description { 'object': 'RngEgd', 'qom-type': 'rng-egd', 'configuration': 'RngEgdOptions', 'boxed': true } the QAPI generator could produce the oc->configure implementation. Similar to commands, that implementation would be an unmarshaling wrapper that calls out to the natural C interface: void qapi_RngEgd_configure(Object *obj, Visitor *v, Error **errp); { Error *local_err = NULL; g_autoptr(MemoryBackendOptions) *config = g_new0(MemoryBackendOptions, 1); visit_type_MemoryBackendOptions(v, NULL, &s->config, &local_err); if (local_err) { error_propagate(errp, local_err); return; } qom_rng_egd_configure(RNG_EGD(obj), config, errp); } void qom_rng_egd_configure(RngEng *s, RngEgdOptions *config, Error **errp) { config->share = (config->has_share ? config->share : false); ... s->config = QAPI_CLONE(RngEgdOptions, config); } Paolo
Am 03.12.2020 um 17:50 hat Paolo Bonzini geschrieben: > On 03/12/20 16:15, Kevin Wolf wrote: > > I don't think this is an intermediate state like Eduardo wants to have. > > Creating the object, then setting properties, then realize [1] will fail > > after your change. But keeping it working was the whole point of the > > exercise. > > With the sample code, you must remove object_class_property_set calls at the > same time as you remove the setters. Usually that'd be when you convert to > QAPI and oc->configure, but it doesn't have to be that way if there are good > reasons not to do so. Okay, thanks, I think I understand now. So I assume that in the common case, we'll never have the state that you describe, but we'll want to directly skip to QAPI generated code. But it's good to know that we can make smaller steps if we need to in more complicated cases. > Also, it still allows you to do so one class at a time, and I *think* the > presence of subclasses or superclasses doesn't matter (only whether > properties are still writable). We can use chardevs (see ChardevCommon in > qapi/char.json) to validate that before tackling devices. Yes, it looks like it should be working. > (In fact, this means that your series---plus -object and object_add > conversion---would be good, pretty much unchanged, as a first step. The > second would be adding oc->configure and object_configure, and converting > all user-creatable objects to oc->configure. The third would involve QAPI > code generation). I think I'd want to do step 2 and 3 combined, because converting user-creatable objects to oc->configure means manually writing the configure function that will be generated from QAPI in step 3. Writing code just to throw it away isn't my favourite pastime. > > I'm also not really sure why you go from RngEgdOptions to QObject to a > > visitor, only to reconstruct RngEgdOptions at the end. > > The two visits are just because you cannot create an input visitor directly > on C data. I stole that from your patch 18/18 actually, just with > object_new+object_configure instead of user_creatable_add_type. > > But I wouldn't read too much in the automatically-generated *_new functions > since they are already in QAPI code generator territory. Instead the basic > object_configure idea can be applied even without having automatic code > generation. Yes, I was just wondering why we're going through visitors at all. But this is what provides the compatibility with the old property system, so it makes sense if you need an intermediate step. > > I think the class > > implementations should have a normal C interface without visitors and we > > should be able to just pass the existing RngEgdOptions object (or the > > individual values for its fields for 'boxed': false). > > Sure, however that requires changes to the QAPI code generator which was > only item (3) in your list list. Until then you can already work with a > visitor interface: > > void rng_egd_configure(Object *obj, Visitor *v, Error **errp) > { > RngEgd *s = RNG_EGD(obj); > s->config = g_new0(MemoryBackendOptions, 1); > visit_type_MemoryBackendOptions(v, NULL, &s->config, errp); > > s->config->share = (s->config->has_share > ? s->config->share : false); > ... > } > > but if you had a QAPI description > > { 'object': 'RngEgd', > 'qom-type': 'rng-egd', > 'configuration': 'RngEgdOptions', > 'boxed': true > } > > the QAPI generator could produce the oc->configure implementation. Similar > to commands, that implementation would be an unmarshaling wrapper that calls > out to the natural C interface: > > void qapi_RngEgd_configure(Object *obj, Visitor *v, Error **errp); > { > Error *local_err = NULL; > g_autoptr(MemoryBackendOptions) *config = > g_new0(MemoryBackendOptions, 1); > visit_type_MemoryBackendOptions(v, NULL, &s->config, &local_err); > if (local_err) { > error_propagate(errp, local_err); > return; > } > qom_rng_egd_configure(RNG_EGD(obj), config, errp); > } > > void qom_rng_egd_configure(RngEng *s, > RngEgdOptions *config, > Error **errp) > { > config->share = (config->has_share > ? config->share : false); > ... > s->config = QAPI_CLONE(RngEgdOptions, config); > } Yes, exactly. Kevin
On Thu, Dec 03, 2020 at 05:50:46PM +0100, Paolo Bonzini wrote: > On 03/12/20 16:15, Kevin Wolf wrote: > > I don't think this is an intermediate state like Eduardo wants to have. > > Creating the object, then setting properties, then realize [1] will fail > > after your change. But keeping it working was the whole point of the > > exercise. > > With the sample code, you must remove object_class_property_set calls at the Do you mean object_property_set()? > same time as you remove the setters. Usually that'd be when you convert to > QAPI and oc->configure, but it doesn't have to be that way if there are good > reasons not to do so. Having two (or more) similar but incompatible APIs to do exactly the same thing is a mistake we did before, and I wouldn't like us to repeat it. If we can keep qdev_new() + object_property_set() + realize working after the device is converted, we should. I believe we can. If we can make object_new_configure() work with all (or most) device types before we manually convert them to the new system, we should. I believe we can. We may be able avoid these questions with -object because converting all backends at the same time is doable. With devices, API usability and maintainability during the transition period (which could be very long) needs to be taken into account.
On 03/12/20 18:43, Kevin Wolf wrote: > I think I'd want to do step 2 and 3 combined, because converting > user-creatable objects to oc->configure means manually writing the > configure function that will be generated from QAPI in step 3. Writing > code just to throw it away isn't my favourite pastime. It would only be a couple lines of extra code, but yeah you don't have to do it. It still is clearer to show the steps one by one as they are logically separate and it shows what (modulo inlining) the generated code ends up doing. That said having no setter might simplify the introduction of field properties too (no allow_set to worry about); perhaps that's a good reason to do the oc->configure conversion earlier rather than later, especially if QAPI code generation ends up taking a bit longer. Another good reason is to make sure the API is stable before moving to generated code, especially with respect to inheritance. Paolo
On 03/12/20 18:52, Eduardo Habkost wrote: > On Thu, Dec 03, 2020 at 05:50:46PM +0100, Paolo Bonzini wrote: >> On 03/12/20 16:15, Kevin Wolf wrote: >>> I don't think this is an intermediate state like Eduardo wants to have. >>> Creating the object, then setting properties, then realize [1] will fail >>> after your change. But keeping it working was the whole point of the >>> exercise. >> >> With the sample code, you must remove object_class_property_set calls at the > > Do you mean object_property_set()? Yes. >> same time as you remove the setters. Usually that'd be when you convert to >> QAPI and oc->configure, but it doesn't have to be that way if there are good >> reasons not to do so. > > Having two (or more) similar but incompatible APIs to do exactly > the same thing is a mistake we did before, and I wouldn't like us > to repeat it. > > If we can keep qdev_new() + object_property_set() + realize > working after the device is converted, we should. I believe we > can. You can. If you want to do that, you have to give up on removing the setters; but that's not so beneficial for devices because they already use static properties anyway. They have much less boilerplate than -object objects. > If we can make object_new_configure() work with all (or most) > device types before we manually convert them to the new system, > we should. I believe we can. Yup, object_new_configure() is the low-level visitor-based API and therefore it supports both properties and oc->configure. > We may be able avoid these questions with -object because > converting all backends at the same time is doable. With > devices, API usability and maintainability during the transition > period (which could be very long) needs to be taken into account. I think we're in violent agreement. :) Paolo
On Thu, Dec 03, 2020 at 07:10:37PM +0100, Paolo Bonzini wrote: > On 03/12/20 18:52, Eduardo Habkost wrote: > > On Thu, Dec 03, 2020 at 05:50:46PM +0100, Paolo Bonzini wrote: > > > On 03/12/20 16:15, Kevin Wolf wrote: > > > > I don't think this is an intermediate state like Eduardo wants to have. > > > > Creating the object, then setting properties, then realize [1] will fail > > > > after your change. But keeping it working was the whole point of the > > > > exercise. > > > > > > With the sample code, you must remove object_class_property_set calls at the > > > > Do you mean object_property_set()? > > Yes. > > > > same time as you remove the setters. Usually that'd be when you convert to > > > QAPI and oc->configure, but it doesn't have to be that way if there are good > > > reasons not to do so. > > > > Having two (or more) similar but incompatible APIs to do exactly > > the same thing is a mistake we did before, and I wouldn't like us > > to repeat it. > > > > If we can keep qdev_new() + object_property_set() + realize > > working after the device is converted, we should. I believe we > > can. > > You can. If you want to do that, you have to give up on removing the > setters; but that's not so beneficial for devices because they already use > static properties anyway. They have much less boilerplate than -object > objects. Understood. We can also get rid of most setters in -object backends using field properties. Maybe not a necessary step, but a useful intermediate step in case the new API takes time to be ready. > > > If we can make object_new_configure() work with all (or most) > > device types before we manually convert them to the new system, > > we should. I believe we can. > > Yup, object_new_configure() is the low-level visitor-based API and therefore > it supports both properties and oc->configure. Perfect. That part was not clear yet to me (I just skimmed to the example code you posted on the wiki). > > > We may be able avoid these questions with -object because > > converting all backends at the same time is doable. With > > devices, API usability and maintainability during the transition > > period (which could be very long) needs to be taken into account. > > I think we're in violent agreement. :) > > Paolo >