| Message ID | 20201104160021.2342108-1-ehabkost@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Make qdev static property API usable by any QOM type | expand |
Patchew URL: https://patchew.org/QEMU/20201104160021.2342108-1-ehabkost@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20201104160021.2342108-1-ehabkost@redhat.com
Subject: [PATCH v2 00/44] Make qdev static property API usable by any QOM type
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/20201104160021.2342108-1-ehabkost@redhat.com -> patchew/20201104160021.2342108-1-ehabkost@redhat.com
- [tag update] patchew/cover.1604464950.git.alistair.francis@wdc.com -> patchew/cover.1604464950.git.alistair.francis@wdc.com
Switched to a new branch 'test'
74d2a4f machine: Register most properties as field properties
e57a72b tests: Use field properties at check-qom-proplist test case
46f06f6 qom: Include static property API reference in documentation
e2754e0 qdev: Move base property types to qom/property-types.c
c83750c qdev: Move core field property code to QOM
c1f0ca8 qdev: PROP_* macros
ba5c6dc qdev: Rename qdev_prop_* to prop_info_*
151ee6d qdev: Move qdev_prop_tpm declaration to tpm_prop.h
d69260f qdev: Rename qdev_get_prop_ptr() to object_field_prop_ptr()
a403dfd qdev: Rename qdev_propinfo_* to field_prop_*
abf9c0e qdev: Make qdev_propinfo_get_uint16() static
949e6fd qdev: Make qdev_prop_allow_set() a ObjectProperty.allow_set callback
5b51a20 qom: Add allow_set callback to ObjectProperty
a785525 qdev: Reuse object_property_add_field() when adding array elements
9c2a384 qdev: Get rid of ArrayElementProperty struct
ae7a992 qdev: Remove ArrayElementProperty.propname field
f065d16 qdev: Avoid unnecessary DeviceState* variable at set_prop_arraylen()
529f3bf qdev: Don't set qdev_prop_name for array elements
a0b808f qdev: Rename Property.name to Property.qdev_prop_name
531c8f7 qdev: Separate generic and device-specific property registration
d7a02b8 qdev: Make qdev_class_add_property() more flexible
05ee196 qdev: Make PropertyInfo.create return ObjectProperty*
ff520f7 qdev: Move dev->realized check to qdev_property_set()
2822481 qdev: Wrap getters and setters in separate helpers
e25f29d qdev: Add name argument to PropertyInfo.create method
a709b9f qdev: Add name parameter to qdev_class_add_property()
0dac961 qdev: Avoid using prop->name unnecessarily
460626c qdev: Get just property name at error_set_from_qdev_prop_error()
3b17a2f sparc: Use DEFINE_PROP for nwindows property
5d719ec qdev: Reuse DEFINE_PROP in all DEFINE_PROP_* macros
461571e qdev: Move softmmu properties to qdev-properties-system.h
39ddc78 qdev: Move UUID property to qdev-properties-system.c
0f14166 qdev: Make error_set_from_qdev_prop_error() get Object* argument
9785bda qdev: Make check_prop_still_unset() get Object* argument
f0f09a3 qdev: Make qdev_find_global_prop() get Object* argument
755cc12 qdev: Make qdev_get_prop_ptr() get Object* arg
743a7b9 qdev: Make bit_prop_set() get Object* argument
c2dedd0 qdev: Make PropertyInfo.print method get Object* argument
40c9d21 qdev: Don't use dev->id on set_size32() error message
8ca5b2b sparc: Check dev->realized at sparc_set_nwindows()
bc82801 qdev: Check dev->realized at set_size()
f2497db qdev: Move property code to qdev-properties.[ch]
969306e cpu: Move cpu_common_props to hw/core/cpu.c
3d0ba5f cs4231: Get rid of empty property array
=== OUTPUT BEGIN ===
1/44 Checking commit 3d0ba5f057e5 (cs4231: Get rid of empty property array)
2/44 Checking commit 969306e01eb3 (cpu: Move cpu_common_props to hw/core/cpu.c)
WARNING: Block comments use a leading /* on a separate line
#51: FILE: hw/core/cpu.c:398:
+ /* Create a memory property for softmmu CPU object,
total: 0 errors, 1 warnings, 49 lines checked
Patch 2/44 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/44 Checking commit f2497db84ed4 (qdev: Move property code to qdev-properties.[ch])
4/44 Checking commit bc8280160af2 (qdev: Check dev->realized at set_size())
5/44 Checking commit 8ca5b2bcbef0 (sparc: Check dev->realized at sparc_set_nwindows())
6/44 Checking commit 40c9d2144d73 (qdev: Don't use dev->id on set_size32() error message)
7/44 Checking commit c2dedd0f2664 (qdev: Make PropertyInfo.print method get Object* argument)
8/44 Checking commit 743a7b94d217 (qdev: Make bit_prop_set() get Object* argument)
9/44 Checking commit 755cc1262e1b (qdev: Make qdev_get_prop_ptr() get Object* arg)
10/44 Checking commit f0f09a33e829 (qdev: Make qdev_find_global_prop() get Object* argument)
11/44 Checking commit 9785bdade8e1 (qdev: Make check_prop_still_unset() get Object* argument)
12/44 Checking commit 0f14166bb10b (qdev: Make error_set_from_qdev_prop_error() get Object* argument)
13/44 Checking commit 39ddc785b42a (qdev: Move UUID property to qdev-properties-system.c)
14/44 Checking commit 461571e7c248 (qdev: Move softmmu properties to qdev-properties-system.h)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#879:
new file mode 100644
total: 0 errors, 1 warnings, 700 lines checked
Patch 14/44 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
15/44 Checking commit 5d719ec2d9af (qdev: Reuse DEFINE_PROP in all DEFINE_PROP_* macros)
16/44 Checking commit 3b17a2f75037 (sparc: Use DEFINE_PROP for nwindows property)
17/44 Checking commit 460626cfbfdf (qdev: Get just property name at error_set_from_qdev_prop_error())
18/44 Checking commit 0dac96100bac (qdev: Avoid using prop->name unnecessarily)
19/44 Checking commit a709b9f94797 (qdev: Add name parameter to qdev_class_add_property())
20/44 Checking commit e25f29d417a4 (qdev: Add name argument to PropertyInfo.create method)
21/44 Checking commit 282248183d5e (qdev: Wrap getters and setters in separate helpers)
22/44 Checking commit ff520f7eebac (qdev: Move dev->realized check to qdev_property_set())
23/44 Checking commit 05ee196058a0 (qdev: Make PropertyInfo.create return ObjectProperty*)
WARNING: line over 80 characters
#33: FILE: hw/core/qdev-properties.c:829:
+ qdev_prop_allow_set_link_before_realize,
total: 0 errors, 1 warnings, 28 lines checked
Patch 23/44 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
24/44 Checking commit d7a02b80be4a (qdev: Make qdev_class_add_property() more flexible)
25/44 Checking commit 531c8f760558 (qdev: Separate generic and device-specific property registration)
26/44 Checking commit a0b808face3a (qdev: Rename Property.name to Property.qdev_prop_name)
WARNING: line over 80 characters
#129: FILE: softmmu/qdev-monitor.c:707:
+ value = object_property_print(OBJECT(dev), props->qdev_prop_name, true,
total: 0 errors, 1 warnings, 105 lines checked
Patch 26/44 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
27/44 Checking commit 529f3bfca271 (qdev: Don't set qdev_prop_name for array elements)
28/44 Checking commit f065d1696dbe (qdev: Avoid unnecessary DeviceState* variable at set_prop_arraylen())
29/44 Checking commit ae7a99263c15 (qdev: Remove ArrayElementProperty.propname field)
30/44 Checking commit 9c2a3841ee20 (qdev: Get rid of ArrayElementProperty struct)
31/44 Checking commit a7855258978f (qdev: Reuse object_property_add_field() when adding array elements)
32/44 Checking commit 5b51a20c251d (qom: Add allow_set callback to ObjectProperty)
33/44 Checking commit 949e6fd4ca12 (qdev: Make qdev_prop_allow_set() a ObjectProperty.allow_set callback)
34/44 Checking commit abf9c0ec584b (qdev: Make qdev_propinfo_get_uint16() static)
35/44 Checking commit a403dfd606b4 (qdev: Rename qdev_propinfo_* to field_prop_*)
36/44 Checking commit d69260f76ab9 (qdev: Rename qdev_get_prop_ptr() to object_field_prop_ptr())
37/44 Checking commit 151ee6da64f2 (qdev: Move qdev_prop_tpm declaration to tpm_prop.h)
38/44 Checking commit ba5c6dc490b9 (qdev: Rename qdev_prop_* to prop_info_*)
39/44 Checking commit c1f0ca806ddf (qdev: PROP_* macros)
ERROR: storage class should be at the beginning of the declaration
#28: FILE: include/hw/qdev-properties.h:183:
+ ({ static Property _p = def; &p; })
total: 1 errors, 0 warnings, 60 lines checked
Patch 39/44 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
40/44 Checking commit c83750c46889 (qdev: Move core field property code to QOM)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#255:
rename from hw/core/qdev-prop-internal.h
total: 0 errors, 1 warnings, 428 lines checked
Patch 40/44 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
41/44 Checking commit e2754e0204f6 (qdev: Move base property types to qom/property-types.c)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#759:
new file mode 100644
ERROR: storage class should be at the beginning of the declaration
#901: FILE: include/qom/property-types.h:138:
+ ({ static Property _p = def; &p; })
WARNING: Block comments use a leading /* on a separate line
#1408: FILE: qom/property-types.c:442:
+ /* Setter for the property which defines the length of a
WARNING: Block comments use a leading /* on a separate line
#1433: FILE: qom/property-types.c:467:
+ /* DEFINE_PROP_ARRAY guarantees that name should start with this prefix;
WARNING: Block comments use a leading /* on a separate line
#1440: FILE: qom/property-types.c:474:
+ /* Note that it is the responsibility of the individual device's deinit
WARNING: Block comments use a leading /* on a separate line
#1448: FILE: qom/property-types.c:482:
+ /* This ugly piece of pointer arithmetic sets up the offset so
total: 1 errors, 5 warnings, 1471 lines checked
Patch 41/44 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
42/44 Checking commit 46f06f686087 (qom: Include static property API reference in documentation)
WARNING: line over 80 characters
#81: FILE: include/qom/field-property.h:40:
+ * @defval: default value for the property. This is used only if @set_default
total: 0 errors, 1 warnings, 271 lines checked
Patch 42/44 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
43/44 Checking commit e57a72b253fa (tests: Use field properties at check-qom-proplist test case)
ERROR: storage class should be at the beginning of the declaration
#22: FILE: include/qom/property-types.h:245:
+ ({ static Property _p = def; &_p; })
total: 1 errors, 0 warnings, 107 lines checked
Patch 43/44 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
44/44 Checking commit 74d2a4fb19a6 (machine: Register most properties as field properties)
WARNING: line over 80 characters
#294: FILE: hw/core/machine.c:643:
+ PROP_BOOL(MachineState, dump_guest_core, true),
WARNING: line over 80 characters
#314: FILE: hw/core/machine.c:659:
+ PROP_BOOL(MachineState, enable_graphics, true),
WARNING: line over 80 characters
#330: FILE: hw/core/machine.c:670:
+ PROP_BOOL(MachineState, suppress_vmdesc, false),
total: 0 errors, 3 warnings, 346 lines checked
Patch 44/44 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20201104160021.2342108-1-ehabkost@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Am 04.11.2020 um 16:59 hat Eduardo Habkost geschrieben: > This series refactor the qdev property code so the static > property system can be used by any QOM type. As an example, at > the end of the series some properties in TYPE_MACHINE are > converted to static properties to demonstrate the new API. > > Changes v1 -> v2 > ---------------- > > * Rename functions and source files to call the new feature > "field property" instead of "static property" > > * Change the API signature from an array-based interface similar > to device_class_set_props() to a object_property_add()-like > interface. > > This means instead of doing this: > > object_class_property_add_static_props(oc, my_array_of_props); > > properties are registered like this: > > object_class_property_add_field(oc, "property-name" > PROP_XXX(MyState, my_field), > prop_allow_set_always); > > where PROP_XXX is a PROP_* macro like PROP_STRING, PROP_BOOL, > etc. In comparison, I really like the resulting code from the array based interface in v1 better. I think it's mostly for two reasons: First, the array is much more compact and easier to read. And maybe even more importantly, you know it's static data and only static data. The function based interface can be mixed with other code or the parameter list can contain calls to other functions with side effects, so there are a lot more opportunities for surprises. What I didn't like about the v1 interface is that there is still a separate object_class_property_set_description() for each property, but I think that could have been fixed by moving the description to the definitions in the array, too. On Fri, Oct 30, 2020 at 06:10:34PM +0100, Paolo Bonzini wrote: > On 29/10/20 23:02, Eduardo Habkost wrote: > > +static Property machine_props[] = { > > + DEFINE_PROP_STRING("kernel", MachineState, kernel_filename), > > + DEFINE_PROP_STRING("initrd", MachineState, initrd_filename), > > + DEFINE_PROP_STRING("append", MachineState, kernel_cmdline), > > + DEFINE_PROP_STRING("dtb", MachineState, dtb), > > + DEFINE_PROP_STRING("dumpdtb", MachineState, dumpdtb), > > + DEFINE_PROP_STRING("dt-compatible", MachineState, dt_compatible), > > + DEFINE_PROP_STRING("firmware", MachineState, firmware), > > + DEFINE_PROP_STRING("memory-backend", MachineState, ram_memdev_id), > > + DEFINE_PROP_END_OF_LIST(), > > +}; > > + > > While I think generalizing the _code_ for static properties is obviously > a good idea, I am not sure about generalizing the interface for adding them. > > The reason is that we already have a place for adding properties in > class_init, and having a second makes things "less local", so to speak. As long as you have the function call to apply the properites array in .class_init, it should be obvious enough what you're doing. Of course, I think we should refrain from mixing both styles in a single object, but generally speaking the array feels so much better that I don't think we should reject it just because QOM only had a different interface so far (and the property array is preexisting in qdev, too, so we already have differences between objects - in fact, the majority of objects is probably qdev today). Kevin
On Fri, Nov 06, 2020 at 10:45:11AM +0100, Kevin Wolf wrote: > Am 04.11.2020 um 16:59 hat Eduardo Habkost geschrieben: > > This series refactor the qdev property code so the static > > property system can be used by any QOM type. As an example, at > > the end of the series some properties in TYPE_MACHINE are > > converted to static properties to demonstrate the new API. > > > > Changes v1 -> v2 > > ---------------- > > > > * Rename functions and source files to call the new feature > > "field property" instead of "static property" > > > > * Change the API signature from an array-based interface similar > > to device_class_set_props() to a object_property_add()-like > > interface. > > > > This means instead of doing this: > > > > object_class_property_add_static_props(oc, my_array_of_props); > > > > properties are registered like this: > > > > object_class_property_add_field(oc, "property-name" > > PROP_XXX(MyState, my_field), > > prop_allow_set_always); > > > > where PROP_XXX is a PROP_* macro like PROP_STRING, PROP_BOOL, > > etc. > > In comparison, I really like the resulting code from the array based > interface in v1 better. > > I think it's mostly for two reasons: First, the array is much more > compact and easier to read. And maybe even more importantly, you know > it's static data and only static data. The function based interface can > be mixed with other code or the parameter list can contain calls to > other functions with side effects, so there are a lot more opportunities > for surprises. This is a really good point, and I strongly agree with it. Letting code do funny tricks at runtime is one of the reasons QOM properties became hard to introspect. > > What I didn't like about the v1 interface is that there is still a > separate object_class_property_set_description() for each property, but > I think that could have been fixed by moving the description to the > definitions in the array, too. This would be very easy to implement. > > On Fri, Oct 30, 2020 at 06:10:34PM +0100, Paolo Bonzini wrote: > > On 29/10/20 23:02, Eduardo Habkost wrote: > > > +static Property machine_props[] = { > > > + DEFINE_PROP_STRING("kernel", MachineState, kernel_filename), > > > + DEFINE_PROP_STRING("initrd", MachineState, initrd_filename), > > > + DEFINE_PROP_STRING("append", MachineState, kernel_cmdline), > > > + DEFINE_PROP_STRING("dtb", MachineState, dtb), > > > + DEFINE_PROP_STRING("dumpdtb", MachineState, dumpdtb), > > > + DEFINE_PROP_STRING("dt-compatible", MachineState, dt_compatible), > > > + DEFINE_PROP_STRING("firmware", MachineState, firmware), > > > + DEFINE_PROP_STRING("memory-backend", MachineState, ram_memdev_id), > > > + DEFINE_PROP_END_OF_LIST(), > > > +}; > > > + > > > > While I think generalizing the _code_ for static properties is obviously > > a good idea, I am not sure about generalizing the interface for adding them. > > > > The reason is that we already have a place for adding properties in > > class_init, and having a second makes things "less local", so to speak. > > As long as you have the function call to apply the properites array in > .class_init, it should be obvious enough what you're doing. > > Of course, I think we should refrain from mixing both styles in a single > object, but generally speaking the array feels so much better that I > don't think we should reject it just because QOM only had a different > interface so far (and the property array is preexisting in qdev, too, so > we already have differences between objects - in fact, the majority of > objects is probably qdev today). This is also a strong argument. The QEMU code base has ~500 matches for "object*_property_add*" calls, and ~2100 for "DEFINE_PROP*". Converting qdev arrays to object_class_property_add_*() calls would be a huge effort with no gains. The end result would be two different APIs for registering class field properties coexisting for a long time, and people still confused on what's the preferred API.
On Fri, Nov 06, 2020 at 10:50:19AM -0500, Eduardo Habkost wrote: > On Fri, Nov 06, 2020 at 10:45:11AM +0100, Kevin Wolf wrote: > > Am 04.11.2020 um 16:59 hat Eduardo Habkost geschrieben: > > > This series refactor the qdev property code so the static > > > property system can be used by any QOM type. As an example, at > > > the end of the series some properties in TYPE_MACHINE are > > > converted to static properties to demonstrate the new API. > > > > > > Changes v1 -> v2 > > > ---------------- > > > > > > * Rename functions and source files to call the new feature > > > "field property" instead of "static property" > > > > > > * Change the API signature from an array-based interface similar > > > to device_class_set_props() to a object_property_add()-like > > > interface. > > > > > > This means instead of doing this: > > > > > > object_class_property_add_static_props(oc, my_array_of_props); > > > > > > properties are registered like this: > > > > > > object_class_property_add_field(oc, "property-name" > > > PROP_XXX(MyState, my_field), > > > prop_allow_set_always); > > > > > > where PROP_XXX is a PROP_* macro like PROP_STRING, PROP_BOOL, > > > etc. > > > > In comparison, I really like the resulting code from the array based > > interface in v1 better. > > > > I think it's mostly for two reasons: First, the array is much more > > compact and easier to read. And maybe even more importantly, you know > > it's static data and only static data. The function based interface can > > be mixed with other code or the parameter list can contain calls to > > other functions with side effects, so there are a lot more opportunities > > for surprises. > > This is a really good point, and I strongly agree with it. > Letting code do funny tricks at runtime is one of the reasons QOM > properties became hard to introspect. > > > > > What I didn't like about the v1 interface is that there is still a > > separate object_class_property_set_description() for each property, but > > I think that could have been fixed by moving the description to the > > definitions in the array, too. > > This would be very easy to implement. This was implemented at: https://gitlab.com/ehabkost/qemu/-/commits/work/qdev-make-generic This is the interface I'd like to submit as v3: static Property machine_props[] = { DEFINE_PROP_STRING("kernel", MachineState, kernel_filename, .description = "Linux kernel image file"), DEFINE_PROP_STRING("initrd", MachineState, initrd_filename, .description = "Linux initial ramdisk file"), DEFINE_PROP_STRING("append", MachineState, kernel_cmdline, .description = "Linux kernel command line"), DEFINE_PROP_STRING("dtb", MachineState, dtb, .description = "Linux kernel device tree file"), DEFINE_PROP_STRING("dumpdtb", MachineState, dumpdtb, .description = "Dump current dtb to a file and quit"), DEFINE_PROP_STRING("dt-compatible", MachineState, dt_compatible, .description = "Overrides the \"compatible\" " "property of the dt root node"), DEFINE_PROP_STRING("firmware", MachineState, firmware, .description = "Firmware image"), DEFINE_PROP_STRING("memory-backend", MachineState, ram_memdev_id, .description = "ID of memory backend object"), DEFINE_PROP_BOOL("dump-guest-core", MachineState, dump_guest_core, true, .description = "Include guest memory in a core dump"), DEFINE_PROP_BOOL("mem-merge", MachineState, mem_merge, true, .description = "Enable/disable memory merge support"), DEFINE_PROP_BOOL("graphics", MachineState, enable_graphics, true, .description = "Set on/off to enable/disable graphics emulation"), DEFINE_PROP_BOOL("suppress-vmdesc", MachineState, suppress_vmdesc, false, .description = "Set on to disable self-describing migration"), DEFINE_PROP_UINT32("phandle-start", MachineState, phandle_start, 0, .description = "The first phandle ID we may generate dynamically"), DEFINE_PROP_END_OF_LIST(), }; static void machine_class_init(ObjectClass *oc, void *data) { ... object_class_add_field_properties(oc, machine_props, prop_allow_set_always); ... } > > > > > On Fri, Oct 30, 2020 at 06:10:34PM +0100, Paolo Bonzini wrote: > > > On 29/10/20 23:02, Eduardo Habkost wrote: > > > > +static Property machine_props[] = { > > > > + DEFINE_PROP_STRING("kernel", MachineState, kernel_filename), > > > > + DEFINE_PROP_STRING("initrd", MachineState, initrd_filename), > > > > + DEFINE_PROP_STRING("append", MachineState, kernel_cmdline), > > > > + DEFINE_PROP_STRING("dtb", MachineState, dtb), > > > > + DEFINE_PROP_STRING("dumpdtb", MachineState, dumpdtb), > > > > + DEFINE_PROP_STRING("dt-compatible", MachineState, dt_compatible), > > > > + DEFINE_PROP_STRING("firmware", MachineState, firmware), > > > > + DEFINE_PROP_STRING("memory-backend", MachineState, ram_memdev_id), > > > > + DEFINE_PROP_END_OF_LIST(), > > > > +}; > > > > + > > > > > > While I think generalizing the _code_ for static properties is obviously > > > a good idea, I am not sure about generalizing the interface for adding them. > > > > > > The reason is that we already have a place for adding properties in > > > class_init, and having a second makes things "less local", so to speak. > > > > As long as you have the function call to apply the properites array in > > .class_init, it should be obvious enough what you're doing. > > > > Of course, I think we should refrain from mixing both styles in a single > > object, but generally speaking the array feels so much better that I > > don't think we should reject it just because QOM only had a different > > interface so far (and the property array is preexisting in qdev, too, so > > we already have differences between objects - in fact, the majority of > > objects is probably qdev today). > > This is also a strong argument. The QEMU code base has ~500 > matches for "object*_property_add*" calls, and ~2100 for > "DEFINE_PROP*". > > Converting qdev arrays to object_class_property_add_*() calls > would be a huge effort with no gains. The end result would be > two different APIs for registering class field properties > coexisting for a long time, and people still confused on what's > the preferred API. > > -- > Eduardo
On 06/11/20 22:10, Eduardo Habkost wrote: > This was implemented at: > https://gitlab.com/ehabkost/qemu/-/commits/work/qdev-make-generic > > This is the interface I'd like to submit as v3: > > static Property machine_props[] = { > DEFINE_PROP_STRING("kernel", MachineState, kernel_filename, > .description = "Linux kernel image file"), > DEFINE_PROP_STRING("initrd", MachineState, initrd_filename, > .description = "Linux initial ramdisk file"), > DEFINE_PROP_STRING("append", MachineState, kernel_cmdline, > .description = "Linux kernel command line"), > DEFINE_PROP_STRING("dtb", MachineState, dtb, > .description = "Linux kernel device tree file"), > DEFINE_PROP_STRING("dumpdtb", MachineState, dumpdtb, > .description = "Dump current dtb to a file and quit"), > DEFINE_PROP_STRING("dt-compatible", MachineState, dt_compatible, > .description = "Overrides the \"compatible\" " > "property of the dt root node"), > DEFINE_PROP_STRING("firmware", MachineState, firmware, > .description = "Firmware image"), > DEFINE_PROP_STRING("memory-backend", MachineState, ram_memdev_id, > .description = "ID of memory backend object"), > DEFINE_PROP_BOOL("dump-guest-core", MachineState, dump_guest_core, true, > .description = "Include guest memory in a core dump"), > DEFINE_PROP_BOOL("mem-merge", MachineState, mem_merge, true, > .description = "Enable/disable memory merge support"), > DEFINE_PROP_BOOL("graphics", MachineState, enable_graphics, true, > .description = "Set on/off to enable/disable graphics emulation"), > DEFINE_PROP_BOOL("suppress-vmdesc", MachineState, suppress_vmdesc, false, > .description = "Set on to disable self-describing migration"), > DEFINE_PROP_UINT32("phandle-start", MachineState, phandle_start, 0, > .description = "The first phandle ID we may generate dynamically"), > DEFINE_PROP_END_OF_LIST(), > }; > > static void machine_class_init(ObjectClass *oc, void *data) > { > ... > object_class_add_field_properties(oc, machine_props, prop_allow_set_always); > ... > } If all properties were like this, it would be okay. But the API in v2 is the one that is most consistent with QOM in general. Here is how this change would be a loss in term of consistency: - you have the field properties split in two (with the property itself in one place and the allow-set function in a different place), and also you'd have some properties listed as array and some as function calls. - we would have different ways to handle device field properties (with dc->props) compared to object properties. - while it's true that the QEMU code base has ~500 matches for "object*_property_add*" calls, and ~2100 for "DEFINE_PROP*", the new field properties would pretty much be used only in places that use object_class_property_add*. (And converting DEFINE_PROP* to PROP* would be relatively easy to script, unlike having an array-based definition for all uses of object_class_property*). The choice to describe class properties as function calls was made in 2016 (commit 16bf7f522a, "qom: Allow properties to be registered against classes", 2016-01-18); so far I don't see that it has been misused. Also, I don't think it's any easier to write an introspection code generator with DEFINE_PROP_*. You would still have to parse the class init function to find the reference to the array (and likewise the TypeInfo struct to find the class init function). Paolo
Am 08.11.2020 um 15:05 hat Paolo Bonzini geschrieben: > On 06/11/20 22:10, Eduardo Habkost wrote: > > This was implemented at: > > https://gitlab.com/ehabkost/qemu/-/commits/work/qdev-make-generic > > > > This is the interface I'd like to submit as v3: > > > > static Property machine_props[] = { > > DEFINE_PROP_STRING("kernel", MachineState, kernel_filename, > > .description = "Linux kernel image file"), > > DEFINE_PROP_STRING("initrd", MachineState, initrd_filename, > > .description = "Linux initial ramdisk file"), > > DEFINE_PROP_STRING("append", MachineState, kernel_cmdline, > > .description = "Linux kernel command line"), > > DEFINE_PROP_STRING("dtb", MachineState, dtb, > > .description = "Linux kernel device tree file"), > > DEFINE_PROP_STRING("dumpdtb", MachineState, dumpdtb, > > .description = "Dump current dtb to a file and quit"), > > DEFINE_PROP_STRING("dt-compatible", MachineState, dt_compatible, > > .description = "Overrides the \"compatible\" " > > "property of the dt root node"), > > DEFINE_PROP_STRING("firmware", MachineState, firmware, > > .description = "Firmware image"), > > DEFINE_PROP_STRING("memory-backend", MachineState, ram_memdev_id, > > .description = "ID of memory backend object"), > > DEFINE_PROP_BOOL("dump-guest-core", MachineState, dump_guest_core, true, > > .description = "Include guest memory in a core dump"), > > DEFINE_PROP_BOOL("mem-merge", MachineState, mem_merge, true, > > .description = "Enable/disable memory merge support"), > > DEFINE_PROP_BOOL("graphics", MachineState, enable_graphics, true, > > .description = "Set on/off to enable/disable graphics emulation"), > > DEFINE_PROP_BOOL("suppress-vmdesc", MachineState, suppress_vmdesc, false, > > .description = "Set on to disable self-describing migration"), > > DEFINE_PROP_UINT32("phandle-start", MachineState, phandle_start, 0, > > .description = "The first phandle ID we may generate dynamically"), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > static void machine_class_init(ObjectClass *oc, void *data) > > { > > ... > > object_class_add_field_properties(oc, machine_props, prop_allow_set_always); > > ... > > } > > If all properties were like this, it would be okay. But the API in v2 is > the one that is most consistent with QOM in general. Here is how this change > would be a loss in term of consistency: > > - you have the field properties split in two (with the property itself in > one place and the allow-set function in a different place), and also you'd > have some properties listed as array and some as function calls. Why would you have properties defined as function calls for the same object that uses the array? I'm not entirely sure what you mean with allow-set. The only things I can find are related to link properties, specifically the check() callback of object_class_property_add_link(). If it's this, what would be the problem with just adding it to DEFINE_PROP_LINK instead of using a separate function call to define link properties? > - we would have different ways to handle device field properties (with > dc->props) compared to object properties. You mean dynamic per-object properties, i.e. not class properties? I think having different ways for different things (class vs. object) is better than having different ways for the same things (class in qdev vs. class in non-qdev). > - while it's true that the QEMU code base has ~500 matches for > "object*_property_add*" calls, and ~2100 for "DEFINE_PROP*", the new field > properties would pretty much be used only in places that use > object_class_property_add*. (And converting DEFINE_PROP* to PROP* would be > relatively easy to script, unlike having an array-based definition for all > uses of object_class_property*). > > The choice to describe class properties as function calls was made in 2016 > (commit 16bf7f522a, "qom: Allow properties to be registered against > classes", 2016-01-18); so far I don't see that it has been misused. This was the obvious incremental step forward at the time because you just had to replace one function call with another function call. The commit message doesn't explain that not using data was a conscious decision. I think it would probably have been out of scope then. > Also, I don't think it's any easier to write an introspection code generator > with DEFINE_PROP_*. You would still have to parse the class init function > to find the reference to the array (and likewise the TypeInfo struct to find > the class init function). I don't think we should parse any C code. In my opinion, both introspection and the array should eventually be generated by the QAPI generator from the schema. Kevin
On 09/11/20 12:34, Kevin Wolf wrote: >> If all properties were like this, it would be okay. But the API in v2 is >> the one that is most consistent with QOM in general. Here is how this change >> would be a loss in term of consistency: >> >> - you have the field properties split in two (with the property itself in >> one place and the allow-set function in a different place), and also you'd >> have some properties listed as array and some as function calls. > > Why would you have properties defined as function calls for the same > object that uses the array? Because some properties would not be field properties, for example. For example, any non-scalar property would need to invoke visit_SomeQapiStruct manually and would not be a field property. > I'm not entirely sure what you mean with allow-set. The only things I > can find are related to link properties, specifically the check() > callback of object_class_property_add_link(). If it's this, what would > be the problem with just adding it to DEFINE_PROP_LINK instead of > using a separate function call to define link properties? Eduardo's series is adding allow-set functions to field properties as well. If it's be specified in the function call to object_class_add_field_properties, you'd have part of the property described in the array and part in the object_class_add_field_properties. >> - we would have different ways to handle device field properties (with >> dc->props) compared to object properties. > > You mean dynamic per-object properties, i.e. not class properties? No, I mean that device properties would be handled as dc->props = foo; while object properties would be handled as object_class_add_field_properties(oc, foo, prop_allow_set_always); There would be two different preferred ways to do field properties in qdev vs. non-qdev. > I think having different ways for different things (class vs. object) is > better than having different ways for the same things (class in qdev vs. > class in non-qdev). Right, but qdev's DEFINE_PROP_STRING would be easy to change to something like - DEFINE_PROP_STRING("name", ...), + device_class_add_field_property(dc, "name", PROP_STRING(...)); >> The choice to describe class properties as function calls was made in 2016 >> (commit 16bf7f522a, "qom: Allow properties to be registered against >> classes", 2016-01-18); so far I don't see that it has been misused. > > This was the obvious incremental step forward at the time because you > just had to replace one function call with another function call. The > commit message doesn't explain that not using data was a conscious > decision. I think it would probably have been out of scope then. > >> Also, I don't think it's any easier to write an introspection code generator >> with DEFINE_PROP_*. You would still have to parse the class init function >> to find the reference to the array (and likewise the TypeInfo struct to find >> the class init function). > > I don't think we should parse any C code. In my opinion, both > introspection and the array should eventually be generated by the QAPI > generator from the schema. That'd be a good plan, and I'd add generating the property description from the doc comment. (Though there's still the issue of how to add non-field properties to the introspection. Those would be harder to move to the QAPI generator). But at that point the array vs. function call would not change anything (if anything the function call would be a tiny bit better), so that's another reason to stay with the API that Eduardo has in v2. Paolo
On Mon, Nov 09, 2020 at 03:15:26PM +0100, Paolo Bonzini wrote: > On 09/11/20 12:34, Kevin Wolf wrote: > > > If all properties were like this, it would be okay. But the API in v2 is > > > the one that is most consistent with QOM in general. Here is how this change > > > would be a loss in term of consistency: > > > > > > - you have the field properties split in two (with the property itself in > > > one place and the allow-set function in a different place), and also you'd > > > have some properties listed as array and some as function calls. > > > > Why would you have properties defined as function calls for the same > > object that uses the array? > > Because some properties would not be field properties, for example. For > example, any non-scalar property would need to invoke visit_SomeQapiStruct > manually and would not be a field property. Nothing prevents us from describing those properties inside the same property array. > > > I'm not entirely sure what you mean with allow-set. The only things I > > can find are related to link properties, specifically the check() > > callback of object_class_property_add_link(). If it's this, what would > > be the problem with just adding it to DEFINE_PROP_LINK instead of > > using a separate function call to define link properties? > > Eduardo's series is adding allow-set functions to field properties as well. > If it's be specified in the function call to > object_class_add_field_properties, you'd have part of the property described > in the array and part in the object_class_add_field_properties. > > > > - we would have different ways to handle device field properties (with > > > dc->props) compared to object properties. > > > > You mean dynamic per-object properties, i.e. not class properties? > > No, I mean that device properties would be handled as > > dc->props = foo; More precisely, it is device_class_set_props(dc, foo); which is supposed to become a one-line wrapper to object_class_add_field_properties(). > > while object properties would be handled as > > object_class_add_field_properties(oc, foo, prop_allow_set_always); > > There would be two different preferred ways to do field properties in qdev > vs. non-qdev. They should become exactly the same method, just with a different allow_set function. (There's also the possibility we let the class provide a default allow_set function, and both would become 100% the same) > > > I think having different ways for different things (class vs. object) is > > better than having different ways for the same things (class in qdev vs. > > class in non-qdev). > > Right, but qdev's DEFINE_PROP_STRING would be easy to change to something > like > > - DEFINE_PROP_STRING("name", ...), > + device_class_add_field_property(dc, "name", PROP_STRING(...)); I'm not worried about this direction of conversion (which is easy). I'm worried about the function call => QAPI schema conversion. Function calls are too flexible and requires parsing and executing C code. Requiring all property descriptions to be evaluated at compilation time is an intentional feature of the new API. > > > > The choice to describe class properties as function calls was made in 2016 > > > (commit 16bf7f522a, "qom: Allow properties to be registered against > > > classes", 2016-01-18); so far I don't see that it has been misused. > > > > This was the obvious incremental step forward at the time because you > > just had to replace one function call with another function call. The > > commit message doesn't explain that not using data was a conscious > > decision. I think it would probably have been out of scope then. > > > > > Also, I don't think it's any easier to write an introspection code generator > > > with DEFINE_PROP_*. You would still have to parse the class init function > > > to find the reference to the array (and likewise the TypeInfo struct to find > > > the class init function). > > > > I don't think we should parse any C code. In my opinion, both > > introspection and the array should eventually be generated by the QAPI > > generator from the schema. > > That'd be a good plan, and I'd add generating the property description from > the doc comment. (Though there's still the issue of how to add non-field > properties to the introspection. Those would be harder to move to the QAPI > generator). > > But at that point the array vs. function call would not change anything (if > anything the function call would be a tiny bit better), so that's another > reason to stay with the API that Eduardo has in v2. I don't agree the function call is a tiny bit better. In the best case, I find it a tiny bit worse.
On 09/11/20 16:21, Eduardo Habkost wrote: > On Mon, Nov 09, 2020 at 03:15:26PM +0100, Paolo Bonzini wrote: >> On 09/11/20 12:34, Kevin Wolf wrote: >>>> If all properties were like this, it would be okay. But the API in v2 is >>>> the one that is most consistent with QOM in general. Here is how this change >>>> would be a loss in term of consistency: >>>> >>>> - you have the field properties split in two (with the property itself in >>>> one place and the allow-set function in a different place), and also you'd >>>> have some properties listed as array and some as function calls. >>> >>> Why would you have properties defined as function calls for the same >>> object that uses the array? >> >> Because some properties would not be field properties, for example. For >> example, any non-scalar property would need to invoke visit_SomeQapiStruct >> manually and would not be a field property. > > Nothing prevents us from describing those properties inside the > same property array. Do you mean adding PropertyInfos for them? Adding a once-only PropertyInfo is worse than writing a custom getter/setter pair, because: - without (DEFINE_)PROP_* you lose the type safety. - with (DEFINE_)PROP_* you have much more boilerplate to write > More precisely, it is > device_class_set_props(dc, foo); > > which is supposed to become a one-line wrapper to > object_class_add_field_properties(). You're right, I'm a few years late. So that objection is withdrawn. > (There's also the possibility we let the class provide a default > allow_set function, and both would become 100% the same) That's a possibility too. Though if we ever have a need for multiple allow_set functions it would be somewhat complicated to add it back. Instead of class-wide allow_set, we might as well have a "bool constructed" field in Object and remove the function pointer altogether: just replace prop->allow_set(obj) with just "!obj->constructed". >>> I think having different ways for different things (class vs. object) is >>> better than having different ways for the same things (class in qdev vs. >>> class in non-qdev). >> >> Right, but qdev's DEFINE_PROP_STRING would be easy to change to something >> like >> >> - DEFINE_PROP_STRING("name", ...), >> + device_class_add_field_property(dc, "name", PROP_STRING(...)); > > I'm not worried about this direction of conversion (which is > easy). I'm worried about the function call => QAPI schema > conversion. Function calls are too flexible and requires parsing > and executing C code. Converting DEFINE_PROP_STRING to a schema also requires parsing C code, since you can have handwritten Property literals (especially for custom PropertyInfo). Converting DEFINE_PROP_STRING it also requires matching the array against calls to object_class_add_field_properties (which could be hidden behind helpers such as device_class_set_props). (Plus matching class_init functions against TypeInfo). So, you don't save any parsing by using arrays. (In fact I would probably skip the parsing, and use your suggestion of *executing* C code: write the QAPI schema generator in C, link into QEMU and run it just once to generate the QOM schema). QOM has been using function calls for many years, are there any cases of misuse of that flexibility that you have in mind? I can only think of two *uses*, in fact. One is eepro100_register_types is the only case I can remember where types are registered dynamically. The other is S390 CPU features. In fact, $ git grep \ object_class_property_add|grep -v '([a-z0-9_]*, \"' shows some cases where property names are macros (an mst-ism :), but no other case where properties are being defined dynamically. Paolo
On Mon, Nov 09, 2020 at 05:34:01PM +0100, Paolo Bonzini wrote: > On 09/11/20 16:21, Eduardo Habkost wrote: > > On Mon, Nov 09, 2020 at 03:15:26PM +0100, Paolo Bonzini wrote: > > > On 09/11/20 12:34, Kevin Wolf wrote: > > > > > If all properties were like this, it would be okay. But the API in v2 is > > > > > the one that is most consistent with QOM in general. Here is how this change > > > > > would be a loss in term of consistency: > > > > > > > > > > - you have the field properties split in two (with the property itself in > > > > > one place and the allow-set function in a different place), and also you'd > > > > > have some properties listed as array and some as function calls. > > > > > > > > Why would you have properties defined as function calls for the same > > > > object that uses the array? > > > > > > Because some properties would not be field properties, for example. For > > > example, any non-scalar property would need to invoke visit_SomeQapiStruct > > > manually and would not be a field property. > > > > Nothing prevents us from describing those properties inside the > > same property array. > > Do you mean adding PropertyInfos for them? Adding a once-only PropertyInfo > is worse than writing a custom getter/setter pair, because: > > - without (DEFINE_)PROP_* you lose the type safety. > > - with (DEFINE_)PROP_* you have much more boilerplate to write I mean extending the API to let custom setters and getters appear on the Property array, not using the existing API. > > > More precisely, it is > > device_class_set_props(dc, foo); > > > > which is supposed to become a one-line wrapper to > > object_class_add_field_properties(). > > You're right, I'm a few years late. So that objection is withdrawn. > > > (There's also the possibility we let the class provide a default > > allow_set function, and both would become 100% the same) > > That's a possibility too. Though if we ever have a need for multiple > allow_set functions it would be somewhat complicated to add it back. > > Instead of class-wide allow_set, we might as well have a "bool constructed" > field in Object and remove the function pointer altogether: just replace > prop->allow_set(obj) with just "!obj->constructed". That's a possibility, and maybe that could be the default allow_set behavior. It may be a bit tricky to convert the existing backend objects to the new behavior in a gradual way, though. > > > > > I think having different ways for different things (class vs. object) is > > > > better than having different ways for the same things (class in qdev vs. > > > > class in non-qdev). > > > > > > Right, but qdev's DEFINE_PROP_STRING would be easy to change to something > > > like > > > > > > - DEFINE_PROP_STRING("name", ...), > > > + device_class_add_field_property(dc, "name", PROP_STRING(...)); > > > > I'm not worried about this direction of conversion (which is > > easy). I'm worried about the function call => QAPI schema > > conversion. Function calls are too flexible and requires parsing > > and executing C code. > > Converting DEFINE_PROP_STRING to a schema also requires parsing C code, > since you can have handwritten Property literals (especially for custom > PropertyInfo). Converting DEFINE_PROP_STRING it also requires matching the > array against calls to object_class_add_field_properties (which could be > hidden behind helpers such as device_class_set_props). (Plus matching > class_init functions against TypeInfo). Parsing an array containing a handful of macros (a tiny subset of C) isn't even comparable to parsing and executing C code where object*_property_add*() calls can be buried deep in many levels of C function calls (which may or may not be conditional). (Also, I don't think we should allow handwritten Property literals.) > > So, you don't save any parsing by using arrays. (In fact I would probably > skip the parsing, and use your suggestion of *executing* C code: write the > QAPI schema generator in C, link into QEMU and run it just once to generate > the QOM schema). If we do that with the existing code, we can't be sure the generated schema doesn't depend on configure flags or run time checks inside class_init. Even locating the cases where this is happening is being a challenge because the API is too flexible. However, if we require the property list to be always evaluated at compile time, we can be sure that this method will be reliable. > > QOM has been using function calls for many years, are there any cases of > misuse of that flexibility that you have in mind? I can only think of two > *uses*, in fact. One is eepro100_register_types is the only case I can > remember where types are registered dynamically. The other is S390 CPU > features. [...] The list of tricky dynamic properties is large and I don't think we even found all cases yet. John documented many of them here: https://gitlab.com/jsnow/qemu/-/blob/cli_audit/docs/cli_audit.md Look, for example, for the sections named "Features" for CPU options. > [...] In fact, > > $ git grep \ object_class_property_add|grep -v '([a-z0-9_]*, \"' > > shows some cases where property names are macros (an mst-ism :), but no > other case where properties are being defined dynamically. You are excluding instance-level object_property_add*() calls. Most of them are supposed to be class properties but were never converted. You are also ignoring the complexity of the code path that leads to the object*_property_add*() calls, which is the main problem on most cases.
On 09/11/20 18:16, Eduardo Habkost wrote: > On Mon, Nov 09, 2020 at 05:34:01PM +0100, Paolo Bonzini wrote: >> On 09/11/20 16:21, Eduardo Habkost wrote: >>> Nothing prevents us from describing those properties inside the >>> same property array. >> >> Do you mean adding PropertyInfos for them? Adding a once-only PropertyInfo >> is worse than writing a custom getter/setter pair, because: >> >> - without (DEFINE_)PROP_* you lose the type safety. >> >> - with (DEFINE_)PROP_* you have much more boilerplate to write > > I mean extending the API to let custom setters and getters appear > on the Property array, not using the existing API. That seems like conflicting goals. The field property API is based on getters and setters hidden in PropertyInfo. The "other" property API is based on getters and setters in plain sight in the declaration of the property. >>>>> I think having different ways for different things (class vs. object) is >>>>> better than having different ways for the same things (class in qdev vs. >>>>> class in non-qdev). >>>> >>>> Right, but qdev's DEFINE_PROP_STRING would be easy to change to something >>>> like >>>> >>>> - DEFINE_PROP_STRING("name", ...), >>>> + device_class_add_field_property(dc, "name", PROP_STRING(...)); >>> >>> I'm not worried about this direction of conversion (which is >>> easy). I'm worried about the function call => QAPI schema >>> conversion. Function calls are too flexible and requires parsing >>> and executing C code. >> >> Converting DEFINE_PROP_STRING to a schema also requires parsing C code, >> since you can have handwritten Property literals (especially for custom >> PropertyInfo). Converting DEFINE_PROP_STRING it also requires matching the >> array against calls to object_class_add_field_properties (which could be >> hidden behind helpers such as device_class_set_props). (Plus matching >> class_init functions against TypeInfo). > > Parsing an array containing a handful of macros (a tiny subset of > C) isn't even comparable to parsing and executing C code where > object*_property_add*() calls can be buried deep in many levels > of C function calls (which may or may not be conditional). Finding the array would also require finding calls buried deep in C code, wouldn't they? > (Also, I don't think we should allow handwritten Property literals.) How would you do custom setters and getters then---without separate PropertyInfos, without Property literals, and without an exploding number of macros? >> So, you don't save any parsing by using arrays. (In fact I would probably >> skip the parsing, and use your suggestion of *executing* C code: write the >> QAPI schema generator in C, link into QEMU and run it just once to generate >> the QOM schema). > > If we do that with the existing code, we can't be sure the > generated schema doesn't depend on configure flags or run time > checks inside class_init. We can use grep or Coccinelle or manual code review to identify problematic cases. > Even locating the cases where this is > happening is being a challenge because the API is too flexible. > > However, if we require the property list to be always evaluated > at compile time, we can be sure that this method will be > reliable. > >> QOM has been using function calls for many years, are there any cases of >> misuse of that flexibility that you have in mind? I can only think of two >> *uses*, in fact. One is eepro100_register_types is the only case I can >> remember where types are registered dynamically. The other is S390 CPU >> features. [...] > > The list of tricky dynamic properties is large and I don't think > we even found all cases yet. John documented many of them here: > > https://gitlab.com/jsnow/qemu/-/blob/cli_audit/docs/cli_audit.md > > Look, for example, for the sections named "Features" for CPU > options. Yes, I'm only considering object_class_property calls. Those are the ones that I claim aren't being misused enough for this to be a problem. Making instance-level properties appear in the schema is a completely different kind of conversion, because there is plenty of manual work (and unsolved problems for e.g. subobject property aliases). > You are also ignoring the complexity of the code path that leads > to the object*_property_add*() calls, which is the main problem > on most cases. I would like an example of the complexity of those code paths. I don't see much complexity, as long as the object exists at all, and I don't see how it would be simpler to find the code paths that lead to object_class_add_field_properties. Paolo
On Mon, Nov 09, 2020 at 06:33:04PM +0100, Paolo Bonzini wrote: > On 09/11/20 18:16, Eduardo Habkost wrote: > > On Mon, Nov 09, 2020 at 05:34:01PM +0100, Paolo Bonzini wrote: > > > On 09/11/20 16:21, Eduardo Habkost wrote: > > > > Nothing prevents us from describing those properties inside the > > > > same property array. > > > > > > Do you mean adding PropertyInfos for them? Adding a once-only PropertyInfo > > > is worse than writing a custom getter/setter pair, because: > > > > > > - without (DEFINE_)PROP_* you lose the type safety. > > > > > > - with (DEFINE_)PROP_* you have much more boilerplate to write > > > > I mean extending the API to let custom setters and getters appear > > on the Property array, not using the existing API. > > That seems like conflicting goals. The field property API is based on > getters and setters hidden in PropertyInfo. The "other" property API is > based on getters and setters in plain sight in the declaration of the > property. There's nothing that prevents a void object_class_add_properties(oc, Property *props); function from supporting both. > > > > > > > I think having different ways for different things (class vs. object) is > > > > > > better than having different ways for the same things (class in qdev vs. > > > > > > class in non-qdev). > > > > > > > > > > Right, but qdev's DEFINE_PROP_STRING would be easy to change to something > > > > > like > > > > > > > > > > - DEFINE_PROP_STRING("name", ...), > > > > > + device_class_add_field_property(dc, "name", PROP_STRING(...)); > > > > > > > > I'm not worried about this direction of conversion (which is > > > > easy). I'm worried about the function call => QAPI schema > > > > conversion. Function calls are too flexible and requires parsing > > > > and executing C code. > > > > > > Converting DEFINE_PROP_STRING to a schema also requires parsing C code, > > > since you can have handwritten Property literals (especially for custom > > > PropertyInfo). Converting DEFINE_PROP_STRING it also requires matching the > > > array against calls to object_class_add_field_properties (which could be > > > hidden behind helpers such as device_class_set_props). (Plus matching > > > class_init functions against TypeInfo). > > > > Parsing an array containing a handful of macros (a tiny subset of > > C) isn't even comparable to parsing and executing C code where > > object*_property_add*() calls can be buried deep in many levels > > of C function calls (which may or may not be conditional). > > Finding the array would also require finding calls buried deep in C code, > wouldn't they? Yes, but I don't expect this to happen if the API doesn't encourage that. > > > (Also, I don't think we should allow handwritten Property literals.) > > How would you do custom setters and getters then---without separate > PropertyInfos, without Property literals, and without an exploding number of > macros? Property with struct field: /* We call this DEFINE_PROP_UINT32 today. We can keep the * existing name just to reduce churn. */ DEFINE_PROP_UINT32_FIELD("myproperty", MyState, my_field) Prop with struct field but custom setter: DEFINE_PROP_UINT32_FIELD("myproperty", MyState, my_field, .custom_setter = my_custom_setter) Prop with no struct field, and custom setter/getter: DEFINE_PROP("myproperty", prop_type_uint32, .custom_getter = my_getter, .custom_setter = my_setter) Definitions for above: #define DEFINE_PROP(_name, _typeinfo, ...) \ { .name = _name, .info = &_typeinfo, __VA_ARGS__ } #define DEFINE_FIELD_PROP(name, typeinfo, type, state, field, ...) \ DEFINE_PROP(name, typeinfo, .offset = offsetof(state, field) + type_check(typeof_field(state, field), type), __VA_ARGS__) #define DEFINE_PROP_UINT32_FIELD(name, state, field, ...) \ DEFINE_FIELD_PROP(name, prop_type_uint32, uint32_t, state, field, __VA_ARGS__) Alternative DEFINE_FIELD_PROP definition if we implement some macro magic to declare the expected type for each typeinfo variable: /* Will make ACTUAL_C_TYPE(prop_type_uint32) expand to uint32_t */ DECLARE_QOM_TYPE(prop_type_uint32, uint32_t) /* Will make ACTUAL_C_TYPE(prop_type_uint64) expand to uint64_t) DECLARE_QOM_TYPE(prop_type_uint64, uint64_t) #define DEFINE_FIELD_PROP(name, typeinfo, state, field, ...) \ DEFINE_PROP(name, typeinfo, .offset = offsetof(state, field) + type_check(typeof_field(state, field), ACTUAL_C_TYPE(typeinfo)), __VA_ARGS__) > > > > So, you don't save any parsing by using arrays. (In fact I would probably > > > skip the parsing, and use your suggestion of *executing* C code: write the > > > QAPI schema generator in C, link into QEMU and run it just once to generate > > > the QOM schema). > > > > If we do that with the existing code, we can't be sure the > > generated schema doesn't depend on configure flags or run time > > checks inside class_init. > > We can use grep or Coccinelle or manual code review to identify problematic > cases. We can, but I believe it is better and simpler to have an API that enforces (or at least encourages) this. > > > Even locating the cases where this is > > happening is being a challenge because the API is too flexible. > > > > However, if we require the property list to be always evaluated > > at compile time, we can be sure that this method will be > > reliable. > > > > > QOM has been using function calls for many years, are there any cases of > > > misuse of that flexibility that you have in mind? I can only think of two > > > *uses*, in fact. One is eepro100_register_types is the only case I can > > > remember where types are registered dynamically. The other is S390 CPU > > > features. [...] > > > > The list of tricky dynamic properties is large and I don't think > > we even found all cases yet. John documented many of them here: > > > > https://gitlab.com/jsnow/qemu/-/blob/cli_audit/docs/cli_audit.md > > > > Look, for example, for the sections named "Features" for CPU > > options. > > Yes, I'm only considering object_class_property calls. Those are the ones > that I claim aren't being misused enough for this to be a problem. > instance-level properties are where most of the complexity was introduced because the class API didn't exist yet. I don't think we should ignore them, or we risk having the same issues when converting them to class properties. > Making instance-level properties appear in the schema is a completely > different kind of conversion, because there is plenty of manual work (and > unsolved problems for e.g. subobject property aliases). I'd like us to convert instance-level properties to an API that is easy to use and where the same problems won't happen again. > > > You are also ignoring the complexity of the code path that leads > > to the object*_property_add*() calls, which is the main problem > > on most cases. > > I would like an example of the complexity of those code paths. I don't see > much complexity, as long as the object exists at all, and I don't see how it > would be simpler to find the code paths that lead to > object_class_add_field_properties. Possibly the most complex case is x86_cpu_register_bit_prop(). The qdev_property_add_static() calls at arm_cpu_post_init() are tricky too. If object*_property_add*() is hidden behind a function call or a `if` statement, it's already too much complexity to me. I don't want us to need a second audit like the one John made when we decide to represent QOM class properties in a QAPI schema.
On 09/11/20 19:55, Eduardo Habkost wrote: > On Mon, Nov 09, 2020 at 06:33:04PM +0100, Paolo Bonzini wrote: >> On 09/11/20 18:16, Eduardo Habkost wrote: >>> I mean extending the API to let custom setters and getters appear >>> on the Property array, not using the existing API. >> >> That seems like conflicting goals. The field property API is based on >> getters and setters hidden in PropertyInfo. The "other" property API is >> based on getters and setters in plain sight in the declaration of the >> property. > > There's nothing that prevents a > void object_class_add_properties(oc, Property *props); > function from supporting both. Sorry but I don't believe this until I see it. The two APIs are just too different. And at some point the complexity of DEFINE_PROP becomes: 1) harder to document 2) just as hard to parse and build a QAPI schema from And in the final desired result where QAPI generators are what generates the list of properties, it's pointless to shoehorn both kinds of properties in the same array if different things can just generate calls to different functions. >>> Parsing an array containing a handful of macros (a tiny subset of >>> C) isn't even comparable to parsing and executing C code where >>> object*_property_add*() calls can be buried deep in many levels >>> of C function calls (which may or may not be conditional). >> >> Finding the array would also require finding calls buried deep in C code, >> wouldn't they? > > Yes, but I don't expect this to happen if the API doesn't > encourage that. Out of 700 calls to object_class_property_add*, there are maybe 5 that are dynamic. So on one hand I understand why you want an API that makes those things harder, but on the other hand I don't see such a big risk of misuse, and it won't even matter at all if we later end up with properties described in a QAPI schema. >>> (Also, I don't think we should allow handwritten Property literals.) >> >> How would you do custom setters and getters then---without separate >> PropertyInfos, without Property literals, and without an exploding number of >> macros? > > Prop with no struct field, and custom setter/getter: > > DEFINE_PROP("myproperty", prop_type_uint32, > .custom_getter = my_getter, > .custom_setter = my_setter) It would have to use all the Visitor crap and would be even harder to use than object_class_property_add_str. Thanks but no thanks. :) >>> we can't be sure the [set of QOM properties] >>> doesn't depend on configure flags or run time >>> checks inside class_init. >> >> We can use grep or Coccinelle or manual code review to identify problematic >> cases. > > We can, but I believe it is better and simpler to have an API > that enforces (or at least encourages) this. I don't see how if (...) { object_class_add_field_properties(oc, props); } is discouraged any more than if (...) { object_class_add_field_property(oc, "prop1", PROP_STRING(...)); object_class_add_field_property(oc, "prop2", PROP_STRING(...)); object_class_add_field_property(oc, "prop3", PROP_STRING(...)); object_class_add_field_property(oc, "prop4", PROP_STRING(...)); } (If anything, the former is more natural and less ugly than the latter). > I'd like us to convert instance-level properties to an API that > is easy to use and where the same problems won't happen again. I agree. I just don't think that arrays are enough to make sure the same problems won't happen again. >>> You are also ignoring the complexity of the code path that leads >>> to the object*_property_add*() calls, which is the main problem >>> on most cases. >> >> I would like an example of the complexity of those code paths. I don't see >> much complexity, as long as the object exists at all, and I don't see how it >> would be simpler to find the code paths that lead to >> object_class_add_field_properties. > > Possibly the most complex case is x86_cpu_register_bit_prop(). > The qdev_property_add_static() calls at arm_cpu_post_init() are > tricky too. The problem with those code paths is that there's a reason why they look like they do. For x86_cpu_register_feature_bit_props, for example either you introduce duplication between QOM property definitions and feat_names array, or you resort to run-time logic like that. If you want to make those properties introspectable (i.e. known at compilation time) you wouldn't anyway use DEFINE_PROP*, because it would cause duplication. Instead, you could have a plug-in parser for qapi-gen, reading files akin to target/s390x/cpu_features_def.h.inc. The parser would generate both QAPI schema and calls to x86_cpu_register_bit_prop(). To sum up: for users where properties are heavily dependent on run-time logic, the solution doesn't come from providing a more limited API. A crippled API will simply not solve the problem that prompted the usage of run-time logic, and therefore won't be used. (I don't know enough of the ARM case to say something meaningful about it). > If object*_property_add*() is hidden behind a function call or a > `if` statement, it's already too much complexity to me. You want to remove hiding behind a function call, but why is it any better to hide behind layers of macros? Just the example you had in your email included DEFINE_PROP, DEFINE_FIELD_PROP, DEFINE_PROP_UINT32. It's still impossible to figure out without either parsing or executing C code. Paolo
On Mon, Nov 09, 2020 at 08:27:21PM +0100, Paolo Bonzini wrote: > On 09/11/20 19:55, Eduardo Habkost wrote: > > On Mon, Nov 09, 2020 at 06:33:04PM +0100, Paolo Bonzini wrote: > > > On 09/11/20 18:16, Eduardo Habkost wrote: > > > > I mean extending the API to let custom setters and getters appear > > > > on the Property array, not using the existing API. > > > > > > That seems like conflicting goals. The field property API is based on > > > getters and setters hidden in PropertyInfo. The "other" property API is > > > based on getters and setters in plain sight in the declaration of the > > > property. > > > > There's nothing that prevents a > > void object_class_add_properties(oc, Property *props); > > function from supporting both. > > Sorry but I don't believe this until I see it. The two APIs are just too > different. And at some point the complexity of DEFINE_PROP becomes: > > 1) harder to document > > 2) just as hard to parse and build a QAPI schema from > > And in the final desired result where QAPI generators are what generates the > list of properties, it's pointless to shoehorn both kinds of properties in > the same array if different things can just generate calls to different > functions. > > > > > Parsing an array containing a handful of macros (a tiny subset of > > > > C) isn't even comparable to parsing and executing C code where > > > > object*_property_add*() calls can be buried deep in many levels > > > > of C function calls (which may or may not be conditional). > > > > > > Finding the array would also require finding calls buried deep in C code, > > > wouldn't they? > > > > Yes, but I don't expect this to happen if the API doesn't > > encourage that. > > Out of 700 calls to object_class_property_add*, there are maybe 5 that are > dynamic. So on one hand I understand why you want an API that makes those > things harder, but on the other hand I don't see such a big risk of misuse, > and it won't even matter at all if we later end up with properties described > in a QAPI schema. > > > > > (Also, I don't think we should allow handwritten Property literals.) > > > > > > How would you do custom setters and getters then---without separate > > > PropertyInfos, without Property literals, and without an exploding number of > > > macros? > > > > Prop with no struct field, and custom setter/getter: > > > > DEFINE_PROP("myproperty", prop_type_uint32, > > .custom_getter = my_getter, > > .custom_setter = my_setter) > > It would have to use all the Visitor crap and would be even harder to use > than object_class_property_add_str. Thanks but no thanks. :) Point taken, I dislike the visitor API too. > > > > > we can't be sure the [set of QOM properties] > > > > doesn't depend on configure flags or run time > > > > checks inside class_init. > > > > > > We can use grep or Coccinelle or manual code review to identify problematic > > > cases. > > > > We can, but I believe it is better and simpler to have an API > > that enforces (or at least encourages) this. > > I don't see how > > if (...) { > object_class_add_field_properties(oc, props); > } > > is discouraged any more than > > if (...) { > object_class_add_field_property(oc, "prop1", > PROP_STRING(...)); > object_class_add_field_property(oc, "prop2", > PROP_STRING(...)); > object_class_add_field_property(oc, "prop3", > PROP_STRING(...)); > object_class_add_field_property(oc, "prop4", > PROP_STRING(...)); > } > > (If anything, the former is more natural and less ugly than the latter). On the former, "adding a new property" means adding an entry to a const array. On the latter, it means adding a new function call. On the former, a conditional property would require defining a new array. A non-constant property name or type would require making the array non-const and modifying it at runtime. On the latter, adding a if statement on the front of that function call or a non-constant expression as argument to the function is trivial. > > > I'd like us to convert instance-level properties to an API that > > is easy to use and where the same problems won't happen again. > > I agree. I just don't think that arrays are enough to make sure the same > problems won't happen again. > > > > > You are also ignoring the complexity of the code path that leads > > > > to the object*_property_add*() calls, which is the main problem > > > > on most cases. > > > > > > I would like an example of the complexity of those code paths. I don't see > > > much complexity, as long as the object exists at all, and I don't see how it > > > would be simpler to find the code paths that lead to > > > object_class_add_field_properties. > > > > Possibly the most complex case is x86_cpu_register_bit_prop(). > > The qdev_property_add_static() calls at arm_cpu_post_init() are > > tricky too. > > The problem with those code paths is that there's a reason why they look > like they do. For x86_cpu_register_feature_bit_props, for example either > you introduce duplication between QOM property definitions and feat_names > array, or you resort to run-time logic like that. > > If you want to make those properties introspectable (i.e. known at > compilation time) you wouldn't anyway use DEFINE_PROP*, because it would > cause duplication. Instead, you could have a plug-in parser for qapi-gen, > reading files akin to target/s390x/cpu_features_def.h.inc. The parser would > generate both QAPI schema and calls to x86_cpu_register_bit_prop(). > > To sum up: for users where properties are heavily dependent on run-time > logic, the solution doesn't come from providing a more limited API. A > crippled API will simply not solve the problem that prompted the usage of > run-time logic, and therefore won't be used. I don't know yet what's the best solution for the x86 feature case. Maybe duplicating the list of feature names would be a small price to pay to get a static list of properties defined at compilation time? Maybe we can replace FeatureWordInfo.feat_names[] with property introspection code that will find the property name for a given struct field? In either case, we need something that works for x86 and other complex cases, or it won't be used. Point taken. > > (I don't know enough of the ARM case to say something meaningful about it). > > > If object*_property_add*() is hidden behind a function call or a > > `if` statement, it's already too much complexity to me. > > You want to remove hiding behind a function call, but why is it any better > to hide behind layers of macros? Just the example you had in your email > included DEFINE_PROP, DEFINE_FIELD_PROP, DEFINE_PROP_UINT32. It's still > impossible to figure out without either parsing or executing C code. Because we can be absolutely sure the macros (and the property array) will be constant expressions evaluated at compilation time. * * * Anyway, If we are the only ones discussing this, I will just defer to your suggestions as QOM maintainer. I hope we hear from others.
Am 09.11.2020 um 21:28 hat Eduardo Habkost geschrieben: > On Mon, Nov 09, 2020 at 08:27:21PM +0100, Paolo Bonzini wrote: > > On 09/11/20 19:55, Eduardo Habkost wrote: > > > On Mon, Nov 09, 2020 at 06:33:04PM +0100, Paolo Bonzini wrote: > > > > On 09/11/20 18:16, Eduardo Habkost wrote: > > > > > I mean extending the API to let custom setters and getters appear > > > > > on the Property array, not using the existing API. > > > > > > > > That seems like conflicting goals. The field property API is based on > > > > getters and setters hidden in PropertyInfo. The "other" property API is > > > > based on getters and setters in plain sight in the declaration of the > > > > property. > > > > > > There's nothing that prevents a > > > void object_class_add_properties(oc, Property *props); > > > function from supporting both. > > > > Sorry but I don't believe this until I see it. The two APIs are just too > > different. And at some point the complexity of DEFINE_PROP becomes: > > > > 1) harder to document > > > > 2) just as hard to parse and build a QAPI schema from I don't think that automatically generating a reasonable QAPI schema is possible. This is not only because the code is hard to parse, but because it just doesn't contain all the information - most QOM objects are undocumented, but the QAPI schema wants documentation. When I wrote a QAPI schema for a simple user creatable object, iothread, I spent basically no time in copying over the fields and data types and considerable more time finding appropriate documentation for it. This is manual work no matter what we do. > > And in the final desired result where QAPI generators are what generates the > > list of properties, it's pointless to shoehorn both kinds of properties in > > the same array if different things can just generate calls to different > > functions. > > > > > > > Parsing an array containing a handful of macros (a tiny subset of > > > > > C) isn't even comparable to parsing and executing C code where > > > > > object*_property_add*() calls can be buried deep in many levels > > > > > of C function calls (which may or may not be conditional). > > > > > > > > Finding the array would also require finding calls buried deep in C code, > > > > wouldn't they? > > > > > > Yes, but I don't expect this to happen if the API doesn't > > > encourage that. > > > > Out of 700 calls to object_class_property_add*, there are maybe 5 that are > > dynamic. So on one hand I understand why you want an API that makes those > > things harder, but on the other hand I don't see such a big risk of misuse, > > and it won't even matter at all if we later end up with properties described > > in a QAPI schema. > > > > > > > (Also, I don't think we should allow handwritten Property literals.) > > > > > > > > How would you do custom setters and getters then---without separate > > > > PropertyInfos, without Property literals, and without an exploding number of > > > > macros? > > > > > > Prop with no struct field, and custom setter/getter: > > > > > > DEFINE_PROP("myproperty", prop_type_uint32, > > > .custom_getter = my_getter, > > > .custom_setter = my_setter) > > > > It would have to use all the Visitor crap and would be even harder to use > > than object_class_property_add_str. Thanks but no thanks. :) > > Point taken, I dislike the visitor API too. QAPI is the tool that hides the visitor API in generated code and instead passes native C types to manually written code. But we'll have to make one step after another. And anyway, while not having to use the visitor API would be a bit nicer, getting or setting the value is just one line in the function. So it's probably the least of our problems. > > > > > we can't be sure the [set of QOM properties] > > > > > doesn't depend on configure flags or run time > > > > > checks inside class_init. > > > > > > > > We can use grep or Coccinelle or manual code review to identify problematic > > > > cases. > > > > > > We can, but I believe it is better and simpler to have an API > > > that enforces (or at least encourages) this. > > > > I don't see how > > > > if (...) { > > object_class_add_field_properties(oc, props); > > } > > > > is discouraged any more than > > > > if (...) { > > object_class_add_field_property(oc, "prop1", > > PROP_STRING(...)); > > object_class_add_field_property(oc, "prop2", > > PROP_STRING(...)); > > object_class_add_field_property(oc, "prop3", > > PROP_STRING(...)); > > object_class_add_field_property(oc, "prop4", > > PROP_STRING(...)); > > } > > > > (If anything, the former is more natural and less ugly than the latter). > > On the former, "adding a new property" means adding an entry to a > const array. On the latter, it means adding a new function call. > > On the former, a conditional property would require defining a > new array. A non-constant property name or type would require > making the array non-const and modifying it at runtime. > > On the latter, adding a if statement on the front of that > function call or a non-constant expression as argument to the > function is trivial. > > > > > > I'd like us to convert instance-level properties to an API that > > > is easy to use and where the same problems won't happen again. > > > > I agree. I just don't think that arrays are enough to make sure the same > > problems won't happen again. > > > > > > > You are also ignoring the complexity of the code path that leads > > > > > to the object*_property_add*() calls, which is the main problem > > > > > on most cases. > > > > > > > > I would like an example of the complexity of those code paths. I don't see > > > > much complexity, as long as the object exists at all, and I don't see how it > > > > would be simpler to find the code paths that lead to > > > > object_class_add_field_properties. > > > > > > Possibly the most complex case is x86_cpu_register_bit_prop(). > > > The qdev_property_add_static() calls at arm_cpu_post_init() are > > > tricky too. > > > > The problem with those code paths is that there's a reason why they look > > like they do. For x86_cpu_register_feature_bit_props, for example either > > you introduce duplication between QOM property definitions and feat_names > > array, or you resort to run-time logic like that. > > > > If you want to make those properties introspectable (i.e. known at > > compilation time) you wouldn't anyway use DEFINE_PROP*, because it would > > cause duplication. Instead, you could have a plug-in parser for qapi-gen, > > reading files akin to target/s390x/cpu_features_def.h.inc. The parser would > > generate both QAPI schema and calls to x86_cpu_register_bit_prop(). > > > > To sum up: for users where properties are heavily dependent on run-time > > logic, the solution doesn't come from providing a more limited API. A > > crippled API will simply not solve the problem that prompted the usage of > > run-time logic, and therefore won't be used. > > I don't know yet what's the best solution for the x86 feature > case. Maybe duplicating the list of feature names would be a > small price to pay to get a static list of properties defined at > compilation time? Maybe we can replace > FeatureWordInfo.feat_names[] with property introspection code > that will find the property name for a given struct field? > > In either case, we need something that works for x86 and other > complex cases, or it won't be used. Point taken. > > > > > (I don't know enough of the ARM case to say something meaningful about it). > > > > > If object*_property_add*() is hidden behind a function call or a > > > `if` statement, it's already too much complexity to me. > > > > You want to remove hiding behind a function call, but why is it any better > > to hide behind layers of macros? Just the example you had in your email > > included DEFINE_PROP, DEFINE_FIELD_PROP, DEFINE_PROP_UINT32. It's still > > impossible to figure out without either parsing or executing C code. > > Because we can be absolutely sure the macros (and the property > array) will be constant expressions evaluated at compilation > time. > > * * * > > Anyway, If we are the only ones discussing this, I will just > defer to your suggestions as QOM maintainer. I hope we hear from > others. Well, I expressed my preference for what you're arguing for now, but my expertise is more on the QAPI side than on the QOM side, so I can't really contribute more arguments in the details than you are already doing. In the end, as soon as it's generated code, it won't matter much any more what it looks like. But I'm not sure how soon we will be there and for the meantime I'll always prefer static data to runtime code. Kevin
On 09/11/20 21:28, Eduardo Habkost wrote: > I don't know yet what's the best solution for the x86 feature > case. Maybe duplicating the list of feature names would be a > small price to pay to get a static list of properties defined at > compilation time? Maybe we can replace > FeatureWordInfo.feat_names[] with property introspection code > that will find the property name for a given struct field? The problem is associating the names with the metadata (feature word/bit). Right now we do that by placing the names in the feat_names[] arrays, which are indexed by feature word and bit. >>> If object*_property_add*() is hidden behind a function call or a >>> `if` statement, it's already too much complexity to me. >> You want to remove hiding behind a function call, but why is it any better >> to hide behind layers of macros? Just the example you had in your email >> included DEFINE_PROP, DEFINE_FIELD_PROP, DEFINE_PROP_UINT32. It's still >> impossible to figure out without either parsing or executing C code. > > Because we can be absolutely sure the macros (and the property > array) will be constant expressions evaluated at compilation > time. That's not entirely true. You can always build Property objects manually in a for loop. (Though at that point you might as well use the existing API and not the new one). I think we agree on where _to go_ (schema described outside C code, and possibly integrated with the QAPI schema). I think neither of us has a clear idea of how to get there. :) I don't see this series as a step towards that; I see it more as a worthwhile way to remove boilerplate from QOM objects. In my opinion the next steps for QOM (in general, not necessarily related to the goal) should be to: 1) audit the code and ensure that there are no conditional properties 2) figure out if it makes sense to provide run-time (not compile-time) introspection of QOM class properties, as either a stable or an experimental interface, and how it works together with the QAPI introspection. In particular, whether compound QAPI types can be matched across QOM and QAPI introspection. 3) figure out if there are any instance properties that can be easily extended to class properties. In particular, figure out if we can do class-level property aliasing. Paolo
On Tue, Nov 10, 2020 at 11:58:29AM +0100, Paolo Bonzini wrote: > On 09/11/20 21:28, Eduardo Habkost wrote: > > I don't know yet what's the best solution for the x86 feature > > case. Maybe duplicating the list of feature names would be a > > small price to pay to get a static list of properties defined at > > compilation time? Maybe we can replace > > FeatureWordInfo.feat_names[] with property introspection code > > that will find the property name for a given struct field? > > The problem is associating the names with the metadata (feature word/bit). > Right now we do that by placing the names in the feat_names[] arrays, which > are indexed by feature word and bit. Right, that would require an introspection interface letting us get the property name for a given struct field + bit. Anyway, let's get back to this later. > > > > > If object*_property_add*() is hidden behind a function call or a > > > > `if` statement, it's already too much complexity to me. > > > You want to remove hiding behind a function call, but why is it any better > > > to hide behind layers of macros? Just the example you had in your email > > > included DEFINE_PROP, DEFINE_FIELD_PROP, DEFINE_PROP_UINT32. It's still > > > impossible to figure out without either parsing or executing C code. > > > > Because we can be absolutely sure the macros (and the property > > array) will be constant expressions evaluated at compilation > > time. > > That's not entirely true. You can always build Property objects manually in > a for loop. (Though at that point you might as well use the existing API > and not the new one). This is true if the property array is always declared as static const. > > I think we agree on where _to go_ (schema described outside C code, and > possibly integrated with the QAPI schema). I think neither of us has a > clear idea of how to get there. :) I don't see this series as a step > towards that; I see it more as a worthwhile way to remove boilerplate from > QOM objects. My first goal here is to facilitate (3) below, and allow it to be done with less effort and less churn. This series is not essential to do (3), but I'd like to avoid porting the same code to a different API 2 or 3 times because we keep introducing new mechanisms. > > In my opinion the next steps for QOM (in general, not necessarily related to > the goal) should be to: > > 1) audit the code and ensure that there are no conditional properties > > 2) figure out if it makes sense to provide run-time (not compile-time) > introspection of QOM class properties, as either a stable or an experimental > interface, and how it works together with the QAPI introspection. In > particular, whether compound QAPI types can be matched across QOM and QAPI > introspection. Can you clarify this item? Do you mean an external interface, or internal APIs? > > 3) figure out if there are any instance properties that can be easily > extended to class properties. In particular, figure out if we can do > class-level property aliasing. Most of them need to be moved to class properties somehow, because they are externally visible. The only exceptions I see are read-only link properties and child properties. The trickiest ones are object_property_add_alias() (no class-level equivalent) and object_property_add_*_ptr() (no usable class-level equivalent). object_property_add_*_ptr() is what prompted the creation of this series. See https://lore.kernel.org/qemu-devel/20201009160122.1662082-1-ehabkost@redhat.com
On Tue, Nov 10, 2020 at 11:38:04AM +0100, Kevin Wolf wrote: > Am 09.11.2020 um 21:28 hat Eduardo Habkost geschrieben: [...] > > Anyway, If we are the only ones discussing this, I will just > > defer to your suggestions as QOM maintainer. I hope we hear from > > others. > > Well, I expressed my preference for what you're arguing for now, but my > expertise is more on the QAPI side than on the QOM side, so I can't > really contribute more arguments in the details than you are already > doing. Sorry, I didn't mean to ignore the feedback you had already sent. Let me rephrase: I was hoping we hear more from you and others. > > In the end, as soon as it's generated code, it won't matter much any > more what it looks like. But I'm not sure how soon we will be there and > for the meantime I'll always prefer static data to runtime code. Agreed. I really hope we can convince Paolo that properties as static const data are a desirable feature, and not a crippled API. Paolo convinced me that we still need object_class_add_field() for cases like x86, but I still believe static const arrays of properties should be the rule for all the rest. In the meantime, I'll do the following: I will submit v3 of this series with both object_class_property_add_field() and object_class_add_field_properties() as internal QOM APIs. object_class_add_field_properties() will be used to implement device_class_set_props(). After that, solving this controversy would be just a matter of deciding if we want to make object_class_add_field_properties() a public function or not.
On 11/11/20 19:39, Eduardo Habkost wrote: > I will submit v3 of this series with both > object_class_property_add_field() and > object_class_add_field_properties() as internal QOM APIs. > object_class_add_field_properties() will be used to implement > device_class_set_props(). I have no problem making both of them public APIs. If an object can use only a single array of static^Wfield properties that's totally fine; I'm just not sure about splitting properties between class_init and static arrays, which is the less consistent case. Paolo
On Thu, Nov 12, 2020 at 09:11:48AM +0100, Paolo Bonzini wrote: > On 11/11/20 19:39, Eduardo Habkost wrote: > > I will submit v3 of this series with both > > object_class_property_add_field() and > > object_class_add_field_properties() as internal QOM APIs. > > object_class_add_field_properties() will be used to implement > > device_class_set_props(). > > I have no problem making both of them public APIs. If an object can use > only a single array of static^Wfield properties that's totally fine; I'm > just not sure about splitting properties between class_init and static > arrays, which is the less consistent case. I agree that using a static array for a couple of properties and object_class_property_add*() for all the rest isn't desirable. Making both APIs public sounds like a good plan. I'd like us to make almost every object use only an array (just like almost every device already use only an array, today), but maybe we'll hit too many obstacles trying to do that. We'll see.
This series refactor the qdev property code so the static property system can be used by any QOM type. As an example, at the end of the series some properties in TYPE_MACHINE are converted to static properties to demonstrate the new API. Changes v1 -> v2 ---------------- * Rename functions and source files to call the new feature "field property" instead of "static property" * Change the API signature from an array-based interface similar to device_class_set_props() to a object_property_add()-like interface. This means instead of doing this: object_class_property_add_static_props(oc, my_array_of_props); properties are registered like this: object_class_property_add_field(oc, "property-name" PROP_XXX(MyState, my_field), prop_allow_set_always); where PROP_XXX is a PROP_* macro like PROP_STRING, PROP_BOOL, etc. * Make Property.name optional. Rename it to qdev_prop_name, and restrict its usage to qdev property tracking code. * Make allow_set callback mandatory, to avoid ambiguity * Big cleanup of array property code. We don't need a custom release function for array elements anymore, because we don't need to save the property name in the Property struct anymore. * Moved UUID property to qdev-properties-system, because it still has dependencies on qdev code Eduardo Habkost (44): cs4231: Get rid of empty property array cpu: Move cpu_common_props to hw/core/cpu.c qdev: Move property code to qdev-properties.[ch] qdev: Check dev->realized at set_size() sparc: Check dev->realized at sparc_set_nwindows() qdev: Don't use dev->id on set_size32() error message qdev: Make PropertyInfo.print method get Object* argument qdev: Make bit_prop_set() get Object* argument qdev: Make qdev_get_prop_ptr() get Object* arg qdev: Make qdev_find_global_prop() get Object* argument qdev: Make check_prop_still_unset() get Object* argument qdev: Make error_set_from_qdev_prop_error() get Object* argument qdev: Move UUID property to qdev-properties-system.c qdev: Move softmmu properties to qdev-properties-system.h qdev: Reuse DEFINE_PROP in all DEFINE_PROP_* macros sparc: Use DEFINE_PROP for nwindows property qdev: Get just property name at error_set_from_qdev_prop_error() qdev: Avoid using prop->name unnecessarily qdev: Add name parameter to qdev_class_add_property() qdev: Add name argument to PropertyInfo.create method qdev: Wrap getters and setters in separate helpers qdev: Move dev->realized check to qdev_property_set() qdev: Make PropertyInfo.create return ObjectProperty* qdev: Make qdev_class_add_property() more flexible qdev: Separate generic and device-specific property registration qdev: Rename Property.name to Property.qdev_prop_name qdev: Don't set qdev_prop_name for array elements qdev: Avoid unnecessary DeviceState* variable at set_prop_arraylen() qdev: Remove ArrayElementProperty.propname field qdev: Get rid of ArrayElementProperty struct qdev: Reuse object_property_add_field() when adding array elements qom: Add allow_set callback to ObjectProperty qdev: Make qdev_prop_allow_set() a ObjectProperty.allow_set callback qdev: Make qdev_propinfo_get_uint16() static qdev: Rename qdev_propinfo_* to field_prop_* qdev: Rename qdev_get_prop_ptr() to object_field_prop_ptr() qdev: Move qdev_prop_tpm declaration to tpm_prop.h qdev: Rename qdev_prop_* to prop_info_* qdev: PROP_* macros qdev: Move core field property code to QOM qdev: Move base property types to qom/property-types.c qom: Include static property API reference in documentation tests: Use field properties at check-qom-proplist test case machine: Register most properties as field properties docs/devel/qom.rst | 17 +- audio/audio.h | 1 + hw/core/qdev-prop-internal.h | 30 - hw/tpm/tpm_prop.h | 2 + include/hw/block/block.h | 1 + include/hw/core/cpu.h | 1 - include/hw/qdev-core.h | 37 -- include/hw/qdev-properties-system.h | 77 +++ include/hw/qdev-properties.h | 244 +------- include/net/net.h | 1 + include/qom/field-property-internal.h | 43 ++ include/qom/field-property.h | 112 ++++ include/qom/object.h | 38 ++ include/qom/property-types.h | 292 ++++++++++ backends/tpm/tpm_util.c | 16 +- cpu.c | 15 - hw/acpi/vmgenid.c | 1 + hw/arm/pxa2xx.c | 1 + hw/arm/strongarm.c | 1 + hw/audio/cs4231.c | 5 - hw/block/fdc.c | 1 + hw/block/m25p80.c | 1 + hw/block/nand.c | 1 + hw/block/onenand.c | 1 + hw/block/pflash_cfi01.c | 1 + hw/block/pflash_cfi02.c | 1 + hw/block/vhost-user-blk.c | 1 + hw/block/xen-block.c | 11 +- hw/char/avr_usart.c | 1 + hw/char/bcm2835_aux.c | 1 + hw/char/cadence_uart.c | 1 + hw/char/cmsdk-apb-uart.c | 1 + hw/char/debugcon.c | 1 + hw/char/digic-uart.c | 1 + hw/char/escc.c | 1 + hw/char/etraxfs_ser.c | 1 + hw/char/exynos4210_uart.c | 1 + hw/char/grlib_apbuart.c | 1 + hw/char/ibex_uart.c | 1 + hw/char/imx_serial.c | 1 + hw/char/ipoctal232.c | 1 + hw/char/lm32_juart.c | 1 + hw/char/lm32_uart.c | 1 + hw/char/mcf_uart.c | 1 + hw/char/milkymist-uart.c | 1 + hw/char/nrf51_uart.c | 1 + hw/char/parallel.c | 1 + hw/char/pl011.c | 1 + hw/char/renesas_sci.c | 1 + hw/char/sclpconsole-lm.c | 1 + hw/char/sclpconsole.c | 1 + hw/char/serial-pci-multi.c | 1 + hw/char/serial.c | 1 + hw/char/spapr_vty.c | 1 + hw/char/stm32f2xx_usart.c | 1 + hw/char/terminal3270.c | 1 + hw/char/virtio-console.c | 1 + hw/char/xilinx_uartlite.c | 1 + hw/core/cpu.c | 15 + hw/core/machine.c | 256 ++------ hw/core/qdev-properties-system.c | 258 ++++----- hw/core/qdev-properties.c | 806 +++----------------------- hw/core/qdev.c | 120 ---- hw/hyperv/vmbus.c | 1 + hw/i386/kvm/i8254.c | 1 + hw/ide/qdev.c | 1 + hw/intc/arm_gicv3_common.c | 2 +- hw/intc/rx_icu.c | 4 +- hw/ipmi/ipmi_bmc_extern.c | 1 + hw/ipmi/ipmi_bmc_sim.c | 1 + hw/misc/allwinner-sid.c | 1 + hw/misc/arm_sysctl.c | 4 +- hw/misc/ivshmem.c | 1 + hw/misc/mac_via.c | 1 + hw/misc/sifive_u_otp.c | 1 + hw/net/e1000e.c | 6 +- hw/net/rocker/rocker.c | 1 + hw/nvram/eeprom_at24c.c | 1 + hw/nvram/spapr_nvram.c | 1 + hw/pci-bridge/gen_pcie_root_port.c | 1 + hw/pci/pci.c | 1 + hw/ppc/pnv_pnor.c | 1 + hw/rdma/vmw/pvrdma_main.c | 1 + hw/rtc/mc146818rtc.c | 1 + hw/s390x/css.c | 13 +- hw/s390x/s390-pci-bus.c | 10 +- hw/scsi/scsi-disk.c | 1 + hw/scsi/scsi-generic.c | 1 + hw/scsi/vhost-user-scsi.c | 1 + hw/sd/sd.c | 1 + hw/usb/ccid-card-passthru.c | 1 + hw/usb/dev-serial.c | 1 + hw/usb/redirect.c | 1 + hw/vfio/pci-quirks.c | 11 +- hw/vfio/pci.c | 1 + hw/virtio/vhost-user-fs.c | 1 + hw/virtio/vhost-user-vsock.c | 1 + hw/virtio/virtio-iommu-pci.c | 1 + hw/xen/xen_pt.c | 1 + migration/migration.c | 1 + qom/field-property.c | 108 ++++ qom/object.c | 16 + qom/property-types.c | 545 +++++++++++++++++ softmmu/qdev-monitor.c | 9 +- target/arm/cpu.c | 2 +- target/sparc/cpu.c | 5 +- tests/check-qom-proplist.c | 64 +- qom/meson.build | 2 + 108 files changed, 1630 insertions(+), 1639 deletions(-) delete mode 100644 hw/core/qdev-prop-internal.h create mode 100644 include/hw/qdev-properties-system.h create mode 100644 include/qom/field-property-internal.h create mode 100644 include/qom/field-property.h create mode 100644 include/qom/property-types.h create mode 100644 qom/field-property.c create mode 100644 qom/property-types.c