From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40982) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c2gKN-0003s5-1j for qemu-devel@nongnu.org; Fri, 04 Nov 2016 11:22:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c2gKJ-0003nH-So for qemu-devel@nongnu.org; Fri, 04 Nov 2016 11:22:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44258) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c2gKJ-0003mc-KV for qemu-devel@nongnu.org; Fri, 04 Nov 2016 11:22:43 -0400 From: Markus Armbruster References: <1477705687-31175-1-git-send-email-ehabkost@redhat.com> <1477705687-31175-6-git-send-email-ehabkost@redhat.com> Date: Fri, 04 Nov 2016 16:22:40 +0100 In-Reply-To: <1477705687-31175-6-git-send-email-ehabkost@redhat.com> (Eduardo Habkost's message of "Fri, 28 Oct 2016 23:48:04 -0200") Message-ID: <87ins35kxr.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v4 5/8] qdev: Register static properties as class properties List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org, Igor Mammedov , Andreas =?utf-8?Q?F=C3=A4rber?= Eduardo Habkost writes: > Instead of registering qdev static properties on instance_init, > register them as class properties, at qdev_class_set_props(). > > qdev_property_add_legacy() was replaced by an equivalent > qdev_class_property_add_legacy() function. > qdev_property_add_static(), on the other hand, can't be > eliminated yet because it is used by arm_cpu_post_init(). > > As class properties don't have a release function called when an > object instance is destroyed, the release functions for > properties are called manually from device_finalize(). Ignorant question: should class properties have such a release function? Hmm, I see object_class_property_add() does take a release(). Is that one called at the wrong time? One typo noted below. > Signed-off-by: Eduardo Habkost > --- > Changes series v1 -> v2: > * (none) > > Changes v2 -> v3: > * Fix code alignemnt > * Reported-by: Igor Mammedov > > Changes series v3 -> v4: > * s/Device/Class/ on qdev_class_property_add_legacy() doc comment > * Reported-by: Igor Mammedov > * Call prop->info->release on instance_finalize (fix > tests/drive_del-test failure) > --- > hw/core/qdev.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 69 insertions(+), 13 deletions(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 85952e8..e695fa8 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -739,12 +739,12 @@ static void qdev_get_legacy_property(Object *obj, Visitor *v, > } > > /** > - * qdev_property_add_legacy: > - * @dev: Device to add the property to. > + * qdev_class_property_add_legacy: > + * @oc: Class to add the property to. > * @prop: The qdev property definition. > * @errp: location to store error information. > * > - * Add a legacy QOM property to @dev for qdev property @prop. > + * Add a legacy QOM property to @oc for qdev property @prop. > * On error, store error in @errp. > * > * Legacy properties are string versions of QOM properties. The format of > @@ -754,8 +754,8 @@ static void qdev_get_legacy_property(Object *obj, Visitor *v, > * Do not use this is new code! QOM Properties added through this interface > * will be given names in the "legacy" namespace. > */ > -static void qdev_property_add_legacy(DeviceState *dev, Property *prop, > - Error **errp) > +static void qdev_class_property_add_legacy(ObjectClass *oc, Property *prop, > + Error **errp) > { > gchar *name; > > @@ -765,11 +765,12 @@ static void qdev_property_add_legacy(DeviceState *dev, Property *prop, > } > > name = g_strdup_printf("legacy-%s", prop->name); > - object_property_add(OBJECT(dev), name, "str", > - prop->info->print ? qdev_get_legacy_property : prop->info->get, > - NULL, > - NULL, > - prop, errp); > + object_class_property_add(oc, name, "str", > + prop->info->print ? > + qdev_get_legacy_property : > + prop->info->get, > + NULL, NULL, > + prop, errp); > > g_free(name); > } > @@ -844,6 +845,48 @@ void qdev_property_add_static(DeviceState *dev, Property *prop, > qdev_property_set_to_default(dev, prop, &error_abort); > } > > +/** > + * qdev_class_property_add_static: > + * @oc: Class to add the property to. > + * @prop: The qdev property definition. > + * @errp: location to store error information. > + * > + * Add a static QOM property to @oc for qdev property @prop. > + * On error, store error in @errp. Static properties access data in a struct. > + * The type of the QOM property is derived from prop->info. > + */ > +static void qdev_class_property_add_static(ObjectClass *oc, Property *prop, > + Error **errp) > +{ > + Error *local_err = NULL; > + > + /* > + * TODO qdev_prop_ptr does not have getters or setters. It must > + * go now that it can be replaced with links. The test should be > + * removed along with it: all static properties are read/write. > + */ > + if (!prop->info->get && !prop->info->set) { > + return; > + } > + > + /* Note: prop->info->release is called on device_finalize(), > + * because it needs to be called on instaqnce destruction, not on instance > + * class property removal. > + */ > + object_class_property_add(oc, prop->name, prop->info->name, > + prop->info->get, prop->info->set, > + NULL, prop, &local_err); > + > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + > + object_class_property_set_description(oc, prop->name, > + prop->info->description, > + &error_abort); > +} > + > /* @qdev_alias_all_properties - Add alias properties to the source object for > * all qdev properties on the target DeviceState. > */ > @@ -867,8 +910,15 @@ void qdev_alias_all_properties(DeviceState *target, Object *source) > > void device_class_set_props(DeviceClass *dc, Property *props) > { > + Property *prop; > + ObjectClass *oc = OBJECT_CLASS(dc); > + > assert(!dc->props); > dc->props = props; > + for (prop = dc->props; prop && prop->name; prop++) { > + qdev_class_property_add_legacy(oc, prop, &error_abort); > + qdev_class_property_add_static(oc, prop, &error_abort); > + } > } > > static int qdev_add_hotpluggable_device(Object *obj, void *opaque) > @@ -1068,8 +1118,7 @@ static void device_initfn(Object *obj) > class = object_get_class(OBJECT(dev)); > do { > for (prop = DEVICE_CLASS(class)->props; prop && prop->name; prop++) { > - qdev_property_add_legacy(dev, prop, &error_abort); > - qdev_property_add_static(dev, prop, &error_abort); > + qdev_property_set_to_default(dev, prop, &error_abort); > } > class = object_class_get_parent(class); > } while (class != object_class_by_name(TYPE_DEVICE)); > @@ -1088,9 +1137,16 @@ static void device_post_init(Object *obj) > /* Unlink device from bus and free the structure. */ > static void device_finalize(Object *obj) > { > + DeviceState *dev = DEVICE(obj); > + DeviceClass *dc = DEVICE_GET_CLASS(dev); > + Property *prop; > NamedGPIOList *ngl, *next; > > - DeviceState *dev = DEVICE(obj); > + for (prop = dc->props; prop && prop->name; prop++) { > + if (prop->info->release) { > + prop->info->release(obj, prop->name, prop); > + } > + } > > QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) { > QLIST_REMOVE(ngl, node);