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

[Kevin Wolf <kwolf@redhat.com>]

Am 02.12.2011 02:08, schrieb Anthony Liguori:
> 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.

qemu-queue.h is type safe, GSList is not. IMO that's a show stopper and
I can't understand why we even need to talk about it.

If you want to convince me of the opposite, it certainly needs more than
vague "easier to use" hand waving.

Kevin