Re: [PATCH 03/12] qom: Make object_class_property_add_uint*_ptr() get offset

[Igor Mammedov]

On Thu, 29 Oct 2020 08:56:34 -0400
Eduardo Habkost <ehabkost@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 <ehabkost@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 <ehabkost@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()).
>