Re: [Qemu-devel] [PATCH 1/6] qom: macroify integer property helpers

[Alexander Graf <agraf@suse.de>]

On 02.07.14 05:29, Peter Crosthwaite wrote:
> On Wed, Jul 2, 2014 at 7:49 AM, Alexander Graf <agraf@suse.de> wrote:
>> We have a bunch of nice helpers that allow us to easily register an integer
>> field as QOM property. However, we have those duplicated for every integer
>> size available.
>>
>> This is very cumbersome (and prone to bugs) to work with and extend, so let's
>> strip out the only difference there is (the size) and generate the actual
>> functions via a macro.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>   qom/object.c | 83 ++++++++++++++++++------------------------------------------
>>   1 file changed, 24 insertions(+), 59 deletions(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 0e8267b..73cd717 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -1507,65 +1507,30 @@ static char *qdev_get_type(Object *obj, Error **errp)
>>       return g_strdup(object_get_typename(obj));
>>   }
>>
>> -static void property_get_uint8_ptr(Object *obj, Visitor *v,
>> -                                   void *opaque, const char *name,
>> -                                   Error **errp)
>> -{
>> -    uint8_t value = *(uint8_t *)opaque;
>> -    visit_type_uint8(v, &value, name, errp);
>> -}
>> -
>> -static void property_get_uint16_ptr(Object *obj, Visitor *v,
>> -                                   void *opaque, const char *name,
>> -                                   Error **errp)
>> -{
>> -    uint16_t value = *(uint16_t *)opaque;
>> -    visit_type_uint16(v, &value, name, errp);
>> -}
>> -
>> -static void property_get_uint32_ptr(Object *obj, Visitor *v,
>> -                                   void *opaque, const char *name,
>> -                                   Error **errp)
>> -{
>> -    uint32_t value = *(uint32_t *)opaque;
>> -    visit_type_uint32(v, &value, name, errp);
>> -}
>> -
>> -static void property_get_uint64_ptr(Object *obj, Visitor *v,
>> -                                   void *opaque, const char *name,
>> -                                   Error **errp)
>> -{
>> -    uint64_t value = *(uint64_t *)opaque;
>> -    visit_type_uint64(v, &value, name, errp);
>> -}
>> -
>> -void object_property_add_uint8_ptr(Object *obj, const char *name,
>> -                                   const uint8_t *v, Error **errp)
>> -{
>> -    object_property_add(obj, name, "uint8", property_get_uint8_ptr,
>> -                        NULL, NULL, (void *)v, errp);
>> -}
>> -
>> -void object_property_add_uint16_ptr(Object *obj, const char *name,
>> -                                    const uint16_t *v, Error **errp)
>> -{
>> -    object_property_add(obj, name, "uint16", property_get_uint16_ptr,
>> -                        NULL, NULL, (void *)v, errp);
>> -}
>> -
>> -void object_property_add_uint32_ptr(Object *obj, const char *name,
>> -                                    const uint32_t *v, Error **errp)
>> -{
>> -    object_property_add(obj, name, "uint32", property_get_uint32_ptr,
>> -                        NULL, NULL, (void *)v, errp);
>> -}
>> -
>> -void object_property_add_uint64_ptr(Object *obj, const char *name,
>> -                                    const uint64_t *v, Error **errp)
>> -{
>> -    object_property_add(obj, name, "uint64", property_get_uint64_ptr,
>> -                        NULL, NULL, (void *)v, errp);
>> -}
>> +#define DECLARE_INTEGER_VISITOR(size)                                          \
> macro needs a better name. DECLARE_PROP_SET_GET - something to make it
> clear its about properties.
>
>> +                                                                               \
>> +static void glue(glue(property_get_,size),_ptr)(Object *obj, Visitor *v,       \
>> +                                                void *opaque,                  \
>> +                                                const char *name,              \
>> +                                                Error **errp)                  \
>> +{                                                                              \
>> +    glue(size,_t) value = *(glue(size,_t) *)opaque;                            \
>> +    glue(visit_type_,size)(v, &value, name, errp);                             \
>> +}                                                                              \
>> +                                                                               \
>> +void glue(glue(object_property_add_,size),_ptr)(Object *obj, const char *name, \
>> +                                                const glue(size,_t) *v,        \
>> +                                                Error **errp)                  \
>> +{                                                                              \
>> +    ObjectPropertyAccessor *get = glue(glue(property_get_,size),_ptr);         \
>> +    object_property_add(obj, name, stringify(size), get, NULL, NULL, (void *)v,\
>> +                        errp);                                                 \
>> +}                                                                              \
>> +
>> +DECLARE_INTEGER_VISITOR(uint8)
>> +DECLARE_INTEGER_VISITOR(uint16)
>> +DECLARE_INTEGER_VISITOR(uint32)
>> +DECLARE_INTEGER_VISITOR(uint64)
>>
> Worth getting bool working this way too? Theres been a few times now
> where I have wanted to object_property_add_bool_ptr. Perhaps:
>
> #define DECLARE_PROP_SET_GET(name, type)
>
> DECLARE_PROP_SET_GET(uint8, uint8_t)
> DECLARE_PROP_SET_GET(uint16, uint16_t)
> DECLARE_PROP_SET_GET(uint32, uint32_t)
> DECLARE_PROP_SET_GET(uint64, uint64_t)
> DECLARE_PROP_SET_GET(bool, bool)
>
> Can actually add the bool one later in follow-up patch but just
> thinking ahead to get the macro right for wider usage.

I think that makes sense. Let me change it :).


Alex