| Message ID | 1479322664-2253-3-git-send-email-ehabkost@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com> > --- > 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.
On Thu, Nov 17, 2016 at 01:26:47PM +0100, Markus Armbruster wrote: [...] > 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. My main goal was just to make it more difficult to mistakenly use the qdev propery release functions (that should be called at object destruction time) as the release functions for class properties (that should be called at class destruction time). But this is not as important if patch 1/2 removes the class property release functions. We can drop this patch.
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)); } 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); 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; + + prop->info->release(dev, prop); +} + /** * 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); }; /**
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 <ehabkost@redhat.com> --- 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(-)