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

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

On Wed, 28 Oct 2020 16:22:40 +0100
Paolo Bonzini <pbonzini@redhat.com> 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.

object_class_property_add_uint*_ptr() is similar to what we have in QDEV
properties already implemented. But that is all hidden behind macro
magic, so users aren't using it directly.

But what I dislike the most is adding _class_ variants of those with
offsets exposed to users call site without any type checking.
It might be easier and safer to make current QDEV properties to work
with Object in one go, instead of duplication small parts of it in
object_foo() API.
But then I haven't actually tried so ...

> 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).
> 
> Paolo
> 