Re: [Qemu-devel] [PATCH for-2.9 2/2] qdev: Change signature of PropertyInfo::release

[Markus Armbruster <armbru@redhat.com>]

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.