Re: [Qemu-devel] [PATCH 01/18] qom: add new dynamic property infrastructure based on Visitors

[Kevin Wolf <kwolf@redhat.com>]

Am 30.11.2011 22:03, schrieb Anthony Liguori:
> qdev properties are settable only during construction and static to classes.
> This isn't flexible enough for QOM.
> 
> This patch introduces a property interface for qdev that provides dynamic
> properties that are tied to objects, instead of classes.  These properties are
> Visitor based instead of string based too.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/qdev.c |   99 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev.h |  118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qerror.c  |    4 ++
>  qerror.h  |    3 ++
>  4 files changed, 224 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 106407f..ad2d44f 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -390,12 +390,33 @@ void qdev_init_nofail(DeviceState *dev)
>      }
>  }
>  
> +static void qdev_property_del_all(DeviceState *dev)
> +{
> +    while (dev->properties) {
> +        GSList *i = dev->properties;
> +        DeviceProperty *prop = i->data;
> +
> +        dev->properties = i->next;
> +
> +        if (prop->release) {
> +            prop->release(dev, prop->name, prop->opaque);
> +        }
> +
> +        g_free(prop->name);
> +        g_free(prop->type);
> +        g_free(prop);
> +        g_free(i);
> +    }
> +}
> +
>  /* Unlink device from bus and free the structure.  */
>  void qdev_free(DeviceState *dev)
>  {
>      BusState *bus;
>      Property *prop;
>  
> +    qdev_property_del_all(dev);
> +
>      if (dev->state == DEV_STATE_INITIALIZED) {
>          while (dev->num_child_bus) {
>              bus = QLIST_FIRST(&dev->child_bus);
> @@ -962,3 +983,81 @@ char* qdev_get_fw_dev_path(DeviceState *dev)
>  
>      return strdup(path);
>  }
> +
> +void qdev_property_add(DeviceState *dev, const char *name, const char *type,
> +                       DevicePropertyEtter *get, DevicePropertyEtter *set,
> +                       DevicePropertyRelease *release, void *opaque,
> +                       Error **errp)

How about letting the caller pass in a DeviceProperty for improved
readability and usability? Instead of memorizing the order of currently
eight parameters (could probably become more in the future) you can use
proper C99 initializers then.

> @@ -45,6 +82,7 @@ struct DeviceState {
>      QTAILQ_ENTRY(DeviceState) sibling;
>      int instance_id_alias;
>      int alias_required_for_version;
> +    GSList *properties;
>  };

Why GSList instead of qemu-queue.h macros that would provide type safety?

I don't think a property can belong to multiple devices, can it?
qdev_property_add only refers to a single device, and nothing else adds
elements to the list.

Kevin