From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35210) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c7LmJ-0003ht-BK for qemu-devel@nongnu.org; Thu, 17 Nov 2016 07:26:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c7LmF-0004PA-Ck for qemu-devel@nongnu.org; Thu, 17 Nov 2016 07:26:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45784) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c7LmF-0004Ot-5e for qemu-devel@nongnu.org; Thu, 17 Nov 2016 07:26:51 -0500 From: Markus Armbruster References: <1479322664-2253-1-git-send-email-ehabkost@redhat.com> <1479322664-2253-3-git-send-email-ehabkost@redhat.com> Date: Thu, 17 Nov 2016 13:26:47 +0100 In-Reply-To: <1479322664-2253-3-git-send-email-ehabkost@redhat.com> (Eduardo Habkost's message of "Wed, 16 Nov 2016 16:57:44 -0200") Message-ID: <878tsil2bc.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH for-2.9 2/2] qdev: Change signature of PropertyInfo::release List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org, Andreas =?utf-8?Q?F=C3=A4rber?= Eduardo Habkost writes: > Change the function signature to make implementations simpler and > safer. No void pointers and Object->DeviceState casts inside each > release function. > > Signed-off-by: Eduardo Habkost > --- > hw/core/qdev-properties-system.c | 8 ++------ > hw/core/qdev-properties.c | 10 +++++----- > hw/core/qdev.c | 10 +++++++++- > include/hw/qdev-core.h | 2 +- > 4 files changed, 17 insertions(+), 13 deletions(-) > > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c > index 1b7ea50..4f49109 100644 > --- a/hw/core/qdev-properties-system.c > +++ b/hw/core/qdev-properties-system.c > @@ -112,10 +112,8 @@ fail: > } > } > > -static void release_drive(Object *obj, const char *name, void *opaque) > +static void release_drive(DeviceState *dev, Property *prop) > { > - DeviceState *dev = DEVICE(obj); > - Property *prop = opaque; > BlockBackend **ptr = qdev_get_prop_ptr(dev, prop); > > if (*ptr) { > @@ -210,10 +208,8 @@ static void set_chr(Object *obj, Visitor *v, const char *name, void *opaque, > g_free(str); > } > > -static void release_chr(Object *obj, const char *name, void *opaque) > +static void release_chr(DeviceState *dev, Property *prop) > { > - DeviceState *dev = DEVICE(obj); > - Property *prop = opaque; > CharBackend *be = qdev_get_prop_ptr(dev, prop); > > qemu_chr_fe_deinit(be); > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index 2a82768..3709050 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -383,10 +383,9 @@ PropertyInfo qdev_prop_uint64 = { > > /* --- string --- */ > > -static void release_string(Object *obj, const char *name, void *opaque) > +static void release_string(DeviceState *dev, Property *prop) > { > - Property *prop = opaque; > - g_free(*(char **)qdev_get_prop_ptr(DEVICE(obj), prop)); > + g_free(*(char **)qdev_get_prop_ptr(dev, prop)); > } Type-punning moved from the three release methods to ... > > static void get_string(Object *obj, Visitor *v, const char *name, > @@ -823,7 +822,7 @@ PropertyInfo qdev_prop_pci_host_devaddr = { > typedef struct { > struct Property prop; > char *propname; > - ObjectPropertyRelease *release; > + void (*release)(DeviceState *dev, Property *prop); > } ArrayElementProperty; > > /* object property release callback for array element properties: > @@ -832,9 +831,10 @@ typedef struct { > */ > static void array_element_release(Object *obj, const char *name, void *opaque) > { > + DeviceState *dev = DEVICE(obj); > ArrayElementProperty *p = opaque; > if (p->release) { > - p->release(obj, name, opaque); > + p->release(dev, &p->prop); > } > g_free(p->propname); > g_free(p); ... this old wrapper and ... > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 5783442..b859e15 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -774,6 +774,14 @@ static void qdev_property_add_legacy(DeviceState *dev, Property *prop, > g_free(name); > } > > +static void qdev_release_prop(Object *obj, const char *name, void *opaque) > +{ > + DeviceState *dev = DEVICE(obj); > + Property *prop = opaque; ... a new one. Hmm. > + > + prop->info->release(dev, prop); Roundabout way to say prop->info->release(DEVICE(obj), opaque). Matter of taste. > +} > + > /** > * qdev_property_add_static: > * @dev: Device to add the property to. > @@ -801,7 +809,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop, > > object_property_add(obj, prop->name, prop->info->name, > prop->info->get, prop->info->set, > - prop->info->release, > + prop->info->release ? qdev_release_prop : NULL, > prop, &local_err); > > if (local_err) { > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 2c97347..5ea2095 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -251,7 +251,7 @@ struct PropertyInfo { > int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len); > ObjectPropertyAccessor *get; > ObjectPropertyAccessor *set; > - ObjectPropertyRelease *release; > + void (*release)(DeviceState *dev, Property *prop); > }; > > /** The transformation looks correct to me, but I'm not sure it's worthwhile. Moreover, it creates an inconsistency between set()/get() and release(), both here and in the concrete implementations. For instance, get_string() and set_string() continue to take obj, name, opaque, while release_string() is changed to take dev, prop. I don't like that.