| Message ID | 20201009160122.1662082-4-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qom: Make all -object types use only class properties | expand |
On 10/9/20 11:01 AM, Eduardo Habkost 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. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > static void *pointer_property_get_ptr(Object *obj, PointerProperty *prop) > { > - return prop->ptr; > + if (prop->is_offset) { > + return (void *)obj + prop->offset; Addition on void* is a gcc extension. Does clang support it as well, or should you be casting to char* instead? > + } else { > + return prop->ptr; > + } > } > > +++ 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); I like how it turns out.
On Fri, Oct 09, 2020 at 12:24:19PM -0500, Eric Blake wrote: > On 10/9/20 11:01 AM, Eduardo Habkost 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. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > > static void *pointer_property_get_ptr(Object *obj, PointerProperty *prop) > > { > > - return prop->ptr; > > + if (prop->is_offset) { > > + return (void *)obj + prop->offset; > > Addition on void* is a gcc extension. Does clang support it as well, or > should you be casting to char* instead? Both object_link_get_targetp() and object_link_get_targetp() already use it, so it should be OK.
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. I had an impression that Paolo wanted qdev pointer properties be gone and replaced by something like link 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. > 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); >
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 Habkost <ehabkost@redhat.com> writes: > 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. Emphasis on *yet*: we should be able to lift it up into QOM, shouldn't we? > I don't think object*_property_add() and > object*_property_add_bool() are reasonable alternatives.
On Thu, Oct 22, 2020 at 07:06:58AM +0200, Markus Armbruster wrote: [...] > > 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. > > Emphasis on *yet*: we should be able to lift it up into QOM, shouldn't > we? We should, yes. My plan is to make object_property_*_ptr() and PropertyInfo code converge until they look exactly the same and become a single API.
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. right, _prt confused me for a while. > > > > > 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. But boiler-plate related to QOM properties set/get methods was considered tolerable back then. It was a long time ago, so I don't recall why we decided to abandon qdev properties API. > 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. I also like old qdev API as it can be introspected (it's just data at class level), very concise when used and has default values. Instead of duplicating all that pointer arithmetic from qdev properties in QOM API, it could be better to fix qdev properties so that they would work for Object as well. At least all that thrown away type safety would stay constrained/hidden inside of qdev property macros, instead of being opencoded (offsets) all over the place. How hard it would be make qdev properties to work with Object and what makes you duplicate ugly part of it in QOM instead of making them to handle Object strait away? That would also result in huge removal of boiler plate of current QOM properties. That should suit your goal to make (most) properties introspectable and statically described. [...]
On Fri, Oct 23, 2020 at 05:33:14PM +0200, Igor Mammedov wrote: > On Wed, 21 Oct 2020 09:30:41 -0400 > Eduardo Habkost <ehabkost@redhat.com> wrote: [...] > > 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. > > I also like old qdev API as it can be introspected (it's just data at > class level), very concise when used and has default values. > > Instead of duplicating all that pointer arithmetic from qdev properties > in QOM API, it could be better to fix qdev properties so that they > would work for Object as well. > At least all that thrown away type safety would stay constrained/hidden > inside of qdev property macros, instead of being opencoded (offsets) all > over the place. > > How hard it would be make qdev properties to work with Object and what > makes you duplicate ugly part of it in QOM instead of making them to > handle Object strait away? It is doable, but lots of work. I'm working on this right now. > That would also result in huge removal of boiler plate of current QOM > properties. Yep. > > That should suit your goal to make (most) properties introspectable > and statically described. That's correct. I just don't want a huge qdev refactor to be a reason to delay important work in other areas.
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). Paolo
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 >
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()).
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()). >
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);
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. 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(-)