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

[Eduardo Habkost]

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.

> 
> object_property_add_uintXX_ptr() were introduced as a quick hack,
> when ACPI code generation was moved from Seabios, to avoid more
> code shuffling in device models and adding more boiler plate in
> form of custom setters/getters (the later didn't seem to bother
> us everywhere else where we use object_[class_]property_add() ).
> Then it spread little bit to another places.
> 
> I'd rather get rid of object_property_add_uintXX_ptr() API altogether
> in favor of object_[class_]property_add() like it is used in other places
> to handle intXX properties.
> Adding helpers similar to object_property_add_bool() for intXX
> could reduce boiler plate need for converting current instances of
> _ptr(), and such helpers would also help with reducing boilerplate
> for the rest of instances where object_[class_]property_add()
> currently is used for dealing with integers.

I find object_property_add_bool() terrible.  It requires too much
boilerplate.  I actually have plans to introduce
object*_property_add_bool_ptr() to simplify existing
object_property_add_bool() callers.

I don't love object*_property_add_*_ptr() either.  I consider the
qdev property API better.  But we need a reasonable alternative,
because the qdev API can't be used by non-device objects yet.
I don't think object*_property_add() and
object*_property_add_bool() are reasonable alternatives.

> 
> 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > Cc: qemu-devel@nongnu.org
> > ---
> >  include/qom/object.h       |  8 ++++----
> >  qom/object.c               | 36 +++++++++++++++++++++++-------------
> >  tests/check-qom-proplist.c | 10 ++++++++--
> >  3 files changed, 35 insertions(+), 19 deletions(-)
> > 
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index d378f13a11..1634294e4f 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -1747,7 +1747,7 @@ ObjectProperty *object_property_add_uint8_ptr(Object 
> > *obj, const char *name,
> >  
> >  ObjectProperty *object_class_property_add_uint8_ptr(ObjectClass *klass,
> >                                           const char *name,
> > -                                         const uint8_t *v,
> > +                                         ptrdiff_t offset,
> >                                           ObjectPropertyFlags flags);
> >  
> >  /**
> > @@ -1768,7 +1768,7 @@ ObjectProperty *object_property_add_uint16_ptr(Object 
> > *obj, const char *name,
> >  
> >  ObjectProperty *object_class_property_add_uint16_ptr(ObjectClass *klass,
> >                                            const char *name,
> > -                                          const uint16_t *v,
> > +                                          ptrdiff_t offset,
> >                                            ObjectPropertyFlags flags);
> >  
> >  /**
> > @@ -1789,7 +1789,7 @@ ObjectProperty *object_property_add_uint32_ptr(Object 
> > *obj, const char *name,
> >  
> >  ObjectProperty *object_class_property_add_uint32_ptr(ObjectClass *klass,
> >                                            const char *name,
> > -                                          const uint32_t *v,
> > +                                          ptrdiff_t offset,
> >                                            ObjectPropertyFlags flags);
> >  
> >  /**
> > @@ -1810,7 +1810,7 @@ ObjectProperty *object_property_add_uint64_ptr(Object 
> > *obj, const char *name,
> >  
> >  ObjectProperty *object_class_property_add_uint64_ptr(ObjectClass *klass,
> >                                            const char *name,
> > -                                          const uint64_t *v,
> > +                                          ptrdiff_t offset,
> >                                            ObjectPropertyFlags flags);
> >  
> >  /**
> > diff --git a/qom/object.c b/qom/object.c
> > index 17692ed5c3..bb32f5d3ad 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -2450,13 +2450,22 @@ static char *object_get_type(Object *obj, Error 
> > **errp)
> >  }
> >  
> >  typedef struct {
> > -    /* Pointer to property value */
> > -    void *ptr;
> > +    bool is_offset;
> > +    union {
> > +        /* Pointer to property value.  Valid if is_offset=false */
> > +        void *ptr;
> > +        /* Offset in Object struct.  Valid if is_offset=true */
> > +        ptrdiff_t offset;
> > +    };
> >  } PointerProperty;
> >  
> >  static void *pointer_property_get_ptr(Object *obj, PointerProperty *prop)
> >  {
> > -    return prop->ptr;
> > +    if (prop->is_offset) {
> > +        return (void *)obj + prop->offset;
> > +    } else {
> > +        return prop->ptr;
> > +    }
> >  }
> >  
> >  static void property_get_uint8_ptr(Object *obj, Visitor *v, const char 
> > *name,
> > @@ -2573,10 +2582,11 @@ object_class_property_add_uint_ptr(ObjectClass *oc, 
> > const char *name,
> >                                     ObjectPropertyAccessor getter,
> >                                     ObjectPropertyAccessor setter,
> >                                     ObjectPropertyFlags flags,
> > -                                   void *ptr)
> > +                                   ptrdiff_t offset)
> >  {
> >      PointerProperty *prop = g_new0(PointerProperty, 1);
> > -    prop->ptr = ptr;
> > +    prop->is_offset = true;
> > +    prop->offset = offset;
> >      return object_class_property_add(oc, name, type,
> >                                       (flags & OBJ_PROP_FLAG_READ) ? getter 
> > : NULL,
> >                                       (flags & OBJ_PROP_FLAG_WRITE) ? 
> > setter : NULL,
> > @@ -2597,13 +2607,13 @@ object_property_add_uint8_ptr(Object *obj, const 
> > char *name,
> >  
> >  ObjectProperty *
> >  object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name,
> > -                                    const uint8_t *v,
> > +                                    ptrdiff_t offset,
> >                                      ObjectPropertyFlags flags)
> >  {
> >      return object_class_property_add_uint_ptr(klass, name, "uint8",
> >                                                property_get_uint8_ptr,
> >                                                property_set_uint8_ptr,
> > -                                              flags, (void *)v);
> > +                                              flags, offset);
> >  }
> >  
> >  ObjectProperty *
> > @@ -2620,13 +2630,13 @@ object_property_add_uint16_ptr(Object *obj, const 
> > char *name,
> >  
> >  ObjectProperty *
> >  object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
> > -                                     const uint16_t *v,
> > +                                     ptrdiff_t offset,
> >                                       ObjectPropertyFlags flags)
> >  {
> >      return object_class_property_add_uint_ptr(klass, name, "uint16",
> >                                                property_get_uint16_ptr,
> >                                                property_set_uint16_ptr,
> > -                                              flags, (void *)v);
> > +                                              flags, offset);
> >  }
> >  
> >  ObjectProperty *
> > @@ -2643,13 +2653,13 @@ object_property_add_uint32_ptr(Object *obj, const 
> > char *name,
> >  
> >  ObjectProperty *
> >  object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
> > -                                     const uint32_t *v,
> > +                                     ptrdiff_t offset,
> >                                       ObjectPropertyFlags flags)
> >  {
> >      return object_class_property_add_uint_ptr(klass, name, "uint32",
> >                                                property_get_uint32_ptr,
> >                                                property_set_uint32_ptr,
> > -                                              flags, (void *)v);
> > +                                              flags, offset);
> >  }
> >  
> >  ObjectProperty *
> > @@ -2666,13 +2676,13 @@ object_property_add_uint64_ptr(Object *obj, const 
> > char *name,
> >  
> >  ObjectProperty *
> >  object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
> > -                                     const uint64_t *v,
> > +                                     ptrdiff_t offset,
> >                                       ObjectPropertyFlags flags)
> >  {
> >      return object_class_property_add_uint_ptr(klass, name, "uint64",
> >                                                property_get_uint64_ptr,
> >                                                property_set_uint64_ptr,
> > -                                              flags, (void *)v);
> > +                                              flags, offset);
> >  }
> >  
> >  typedef struct {
> > diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> > index 1b76581980..fba30c20b2 100644
> > --- a/tests/check-qom-proplist.c
> > +++ b/tests/check-qom-proplist.c
> > @@ -61,6 +61,7 @@ struct DummyObject {
> >      bool bv;
> >      DummyAnimal av;
> >      char *sv;
> > +    uint8_t u8v;
> >  };
> >  
> >  struct DummyObjectClass {
> > @@ -141,6 +142,9 @@ static void dummy_class_init(ObjectClass *cls, void 
> > *data)
> >                                     &dummy_animal_map,
> >                                     dummy_get_av,
> >                                     dummy_set_av);
> > +    object_class_property_add_uint8_ptr(cls, "u8v",
> > +                                        offsetof(DummyObject, u8v),
> > +                                        OBJ_PROP_FLAG_READWRITE);
> >  }
> >  
> >  
> > @@ -385,12 +389,14 @@ static void test_dummy_createlist(void)
> >                     "bv", "yes",
> >                     "sv", "Hiss hiss hiss",
> >                     "av", "platypus",
> > +                   "u8v", "42",
> >                     NULL));
> >  
> >      g_assert(err == NULL);
> >      g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
> >      g_assert(dobj->bv == true);
> >      g_assert(dobj->av == DUMMY_PLATYPUS);
> > +    g_assert_cmpint(dobj->u8v, ==, 42);
> >  
> >      g_assert(object_resolve_path_component(parent, "dummy0")
> >               == OBJECT(dobj));
> > @@ -531,7 +537,7 @@ static void test_dummy_iterator(void)
> >  {
> >      const char *expected[] = {
> >          "type",                 /* inherited from TYPE_OBJECT */
> > -        "sv", "av",             /* class properties */
> > +        "sv", "av", "u8v",      /* class properties */
> >          "bv"};                  /* instance property */
> >      Object *parent = object_get_objects_root();
> >      DummyObject *dobj = DUMMY_OBJECT(
> > @@ -552,7 +558,7 @@ static void test_dummy_iterator(void)
> >  
> >  static void test_dummy_class_iterator(void)
> >  {
> > -    const char *expected[] = { "type", "av", "sv" };
> > +    const char *expected[] = { "type", "av", "sv", "u8v" };
> >      ObjectPropertyIterator iter;
> >      ObjectClass *klass = object_class_by_name(TYPE_DUMMY);
> >  
> 

-- 
Eduardo