On Thu, 29 Oct 2020 08:56:34 -0400 Eduardo Habkost <ehabk...@redhat.com> wrote:
> 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. sounds good to me, as long as user don't have deal with offsets directly and macro does its type check thing. > 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()). >
