From: Eduardo Habkost <ehabkost@redhat.com>
To: Igor Mammedov <imammedo@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>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"John Snow" <jsnow@redhat.com>
Subject: Re: [PATCH 03/12] qom: Make object_class_property_add_uint*_ptr() get offset
Date: Wed, 21 Oct 2020 09:30:41 -0400 [thread overview]
Message-ID: <20201021133041.GT5733@habkost.net> (raw)
In-Reply-To: <20201021142408.72d2b92f@redhat.com>
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
next prev parent reply other threads:[~2020-10-21 13:32 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-09 16:01 [PATCH 00/12] qom: Make all -object types use only class properties Eduardo Habkost
2020-10-09 16:01 ` [PATCH 01/12] qom: Helpers for pointer properties Eduardo Habkost
2020-10-09 16:01 ` [PATCH 02/12] qom: Introduce PointerProperty struct Eduardo Habkost
2020-10-09 16:01 ` [PATCH 03/12] qom: Make object_class_property_add_uint*_ptr() get offset Eduardo Habkost
2020-10-09 17:24 ` Eric Blake
2020-10-09 17:31 ` Eduardo Habkost
2020-10-21 12:24 ` Igor Mammedov
2020-10-21 13:30 ` Eduardo Habkost [this message]
2020-10-22 5:06 ` Markus Armbruster
2020-10-22 21:34 ` Eduardo Habkost
2020-10-23 15:33 ` Igor Mammedov
2020-10-27 22:18 ` Eduardo Habkost
2020-10-28 15:22 ` Paolo Bonzini
2020-10-28 15:53 ` Igor Mammedov
2020-10-29 12:56 ` Eduardo Habkost
2020-10-29 13:37 ` Igor Mammedov
2020-10-09 16:01 ` [PATCH 04/12] sev: Use class properties Eduardo Habkost
2020-10-09 16:01 ` [PATCH 05/12] rng: " Eduardo Habkost
2020-10-09 16:01 ` [PATCH 06/12] can_host: " Eduardo Habkost
2020-10-12 14:52 ` Pavel Pisa
2020-10-09 16:01 ` [PATCH 07/12] colo: " Eduardo Habkost
2020-10-09 16:01 ` [PATCH 08/12] netfilter: Reorder functions Eduardo Habkost
2020-10-09 16:01 ` [PATCH 09/12] netfilter: Use class properties Eduardo Habkost
2020-10-09 16:01 ` [PATCH 10/12] input: " Eduardo Habkost
2020-10-13 12:54 ` Gerd Hoffmann
2020-10-09 16:01 ` [PATCH 11/12] [RFC] qom: Property lock mechanism Eduardo Habkost
2020-10-09 16:01 ` [PATCH 12/12] [RFC] qom: Lock properties of all TYPE_USER_CREATABLE types Eduardo Habkost
2020-10-09 21:31 ` [PATCH] check-qom-proplist: Don't register instance props for user-creatable type Eduardo Habkost
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201021133041.GT5733@habkost.net \
--to=ehabkost@redhat.com \
--cc=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=imammedo@redhat.com \
--cc=jsnow@redhat.com \
--cc=kwolf@redhat.com \
--cc=marcandre.lureau@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.