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

From: Eduardo Habkost <ehabkost@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	qemu-devel@nongnu.org, "Markus Armbruster" <armbru@redhat.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Igor Mammedov" <imammedo@redhat.com>,
	"John Snow" <jsnow@redhat.com>
Subject: Re: [PATCH 03/12] qom: Make object_class_property_add_uint*_ptr() get offset
Date: Thu, 29 Oct 2020 08:56:34 -0400	[thread overview]
Message-ID: <20201029125634.GJ5733@habkost.net> (raw)
In-Reply-To: <6f5c3a13-6798-9d7d-e686-69d549366095@redhat.com>

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.

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