[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.9 2/2] qdev: Change signature of PropertyI
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH for-2.9 2/2] qdev: Change signature of PropertyInfo::release |
Date: |
Thu, 17 Nov 2016 13:26:47 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eduardo Habkost <address@hidden> 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 <address@hidden>
> ---
> 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.