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

[Anthony Liguori <anthony@codemonkey.ws>]

On 12/01/2011 09:52 AM, Kevin Wolf wrote:
> 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.

Yeah, instead of taking a void *opaque, it could then just take the 
DeviceProperty and use container_of adding a good bit more type safety.  I like 
it, thanks for the suggestion.

>
>> @@ -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?

You're clearly thwarting my attempts at slowly introducing GSList as a 
replacement for qemu-queue ;-)

I really dislike qemu-queue.  I think it's a whole lot more difficult to use in 
practice.  The glib data structures are much more rich than qemu-queue.

> 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.

Yes, you are correct.

Regards,

Anthony Liguori

>
> Kevin
>