On Wed, Oct 28, 2020 at 04:22:40PM +0100, Paolo Bonzini wrote: > On 23/10/20 17:33, Igor Mammedov wrote: > > On Wed, 21 Oct 2020 09:30:41 -0400 > > Eduardo Habkost <ehabk...@redhat.com> wrote: > > > >> On Wed, Oct 21, 2020 at 02:24:08PM +0200, Igor Mammedov wrote: > >>> On Fri, 9 Oct 2020 12:01:13 -0400 > >>> Eduardo Habkost <ehabk...@redhat.com> wrote: > >>> > >>>> The existing object_class_property_add_uint*_ptr() functions are > >>>> not very useful, because they need a pointer to the property > >>>> value, which can't really be provided before the object is > >>>> created. > >>>> > >>>> Replace the pointer parameter in those functions with a > >>>> `ptrdiff_t offset` parameter. > >>>> > >>>> Include a uint8 class property in check-qom-proplist unit tests, > >>>> to ensure the feature is working. > >>> > >>> > >>> Not sure I like approach, it's reinventing qdev pointer properties in QOM > >>> form. > >> > >> Yes, and that's on purpose. If we want to eventually merge the > >> two competing APIs into a single one, we need to make them > >> converge. > >> > >>> I had an impression that Paolo wanted qdev pointer properties be gone > >>> and replaced by something like link properties. > >> > >> This is completely unrelated to qdev pointer properties and link > >> properties. The properties that use object_property_add_uint*_ptr() > >> today are not qdev pointer properties and will never be link > >> properties. They are just integer properties. > > I think this series a step in the right direction, but please take more > "inspiration" from link properties, which are done right. In > particular, properties should have an optional check function and be > read-only unless the check function is there. > > You can make the check function take an uint64_t for simplicity, so that > all the check functions for uint properties have the same prototype. > For example a single "property_check_uint_allow" function can allow > setting the property (which is almost always wrong, but an easy cop out > for this series).
A property check callback that needs the property value is a more complex use case, and would require too much property-type-specific boilerplate today. I plan to address it, but not right now. In my next series that makes static properties usable by any QOM object, I will add a separate "allow_set" callback to the internal QOM property API, which will not take the property value as argument. This would be enough for the dev->realized checks that are currently in qdev. Interestingly, there is only one link property check callback function in the QEMU tree that actually cares about the property value: isa_ipmi_bmc_check(). All other cases either don't care about the property value at all (qdev_prop_allow_set_link_before_realize(), object_property_allow_set_link()), or are being misused for something other than property checking (xlnx_dp_set_dpdma()). -- Eduardo
