Re: [Qemu-devel] [PATCH 1/4] object: add object_property_add_bool

[Andreas Färber]

Am 25.06.2012 20:35, schrieb Anthony Liguori:
> On 06/25/2012 11:32 AM, Andreas Färber wrote:
>> Am 25.06.2012 18:09, schrieb Anthony Liguori:
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 00bb3b0..1357397 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -1232,6 +1232,62 @@ void object_property_add_str(Object *obj,
>>> const char *name,
>>>                           prop, errp);
>>>   }
>>>
>>> +typedef struct BoolProperty
>>> +{
>>> +    bool (*get)(Object *, Error **);
>>> +    void (*set)(Object *, bool, Error **);
>>> +} BoolProperty;
>>
>> This follows the modelling of the StringProperty and looks okay to me.
>> However, I stumbled over whether we should allow passing an opaque to
>> the typed accessors, too?
> 
> I'd prefer not to.
> 
> You don't have to use these interfaces.  They are just convenient
> wrappers. Adding more parameters just makes them less convenient to use.
> 
>> The use case I have in mind is having a dummy device bundle properties
>> for some parent (wild example: /machine/cpu[1]/features x2apic rather
>> than /machine/cpu[1] x2apic-feature), i.e. where the obj passed to the
>> property and the storage of the value differ somehow.
>>
>> Not a blocker for this series, general design question. CC'ing Paolo.
> 
> I'm not sure I totally understand the use case.  I think you mean
> forwarding properties?  If so, can you give a more detailed concrete
> use-case?

What I mean is that the generic property addition API allows not only to
pass a StringProperty or BoolProperty struct or NULL but also an Object
as opaque value. I think my example was already quite concrete and not
so unrealistic thinking more about it:

We could have an X86CPU object "/machine/cpu[1]". We add a
child<Container> "features", which has no storage of its own (Object).
We add a boolean property "x2apic" to the "features" container and pass
the CPU as opaque. With the generic property interface we can then read
and set a value in the CPU's CPUX86State.
With your newly proposed wrappers this is not as arbitrarily possible
because the BoolProperty struct does not have an opaque member that
could be passed through to the get/set callbacks (in the case of a
direct parent we could access Object::parent of course).
There is no forwarding involved unless you count the actual
implementation accessing opaque rather than obj.

Right now there are hardly any accessible QOM objects for the user and
not that many properties per object. I expect that to change over time
and I expect in particular the CPU properties to explode in numbers,
especially for x86 but also for arm. So I was thinking about ways to
give, say, 200 properties some more structure: adding path components
rather than exploding property name length. Path components allow
semantic grouping such as "features", something that qom-list with
"foobar-feature" properties cannot do. You yourself made rules to freeze
the QOM "ABI", so this is something we cannot decide shortsightedly with
just the handful of today's properties in mind. We don't need to
overengineer but we shouldn't oversimplify either. Thus if we allow it
in one place I think we should be consequent and symmetric and allow the
same freedom in the other place as well when it's as cheap as here. I
don't see you dropping the Error** argument either just because in most
places we're passing NULL for convenience, including in your
rng_backend_get_opened(). The alternative would be to make the opaque
version static so that it can only be used by the convenient string,
bool, etc. versions.

In many places the convenient way to string or bool getters/setters
would be generic static properties btw. And what you're doing in 2/4 is
basically open-coding static properties with s/realized/opened/g. Is
that the new design paradigm? I'm not sure what to make of this series,
it's labeled PATCH so I reviewed this as a serious proposal whereas the
RFC PATCH cover letter that has now arrived makes it sound more like an
FYI for the virtio-rng people, yet this is adding generic QOM
infrastructure which apparently needs more review than just them...

As for convenience, personally I find all these property API signatures
inconvenient enough that I copy&paste when I add a new one or need to
look up object.h. The inconsistent or at least unintuitive order of
value and name is also quite confusing for me. When I want to add a
child<> property I think, to object obj (that's always first for
object_* functions) add a property "foo" with value "bar", but as it is
the value goes before the name. Maybe a matter of taste.

Regards,
Andreas

> That makes me a bit nervous because I think it will be hard to maintain
> compatibility over time if we allow too much of an ad-hoc interface with
> properties.
> 
> Regards,
> 
> Anthony Liguori
> 
>>
>> Andreas
>>
>>> +
>>> +static void property_get_bool(Object *obj, Visitor *v, void *opaque,
>>> +                              const char *name, Error **errp)
>>> +{
>>> +    BoolProperty *prop = opaque;
>>> +    bool value;
>>> +
>>> +    value = prop->get(obj, errp);
>>> +    visit_type_bool(v,&value, name, errp);
>>> +}
>>> +
>>> +static void property_set_bool(Object *obj, Visitor *v, void *opaque,
>>> +                              const char *name, Error **errp)
>>> +{
>>> +    BoolProperty *prop = opaque;
>>> +    bool value;
>>> +    Error *local_err = NULL;
>>> +
>>> +    visit_type_bool(v,&value, name,&local_err);
>>> +    if (local_err) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>>> +
>>> +    prop->set(obj, value, errp);
>>> +}
>>> +
>>> +static void property_release_bool(Object *obj, const char *name,
>>> +                                  void *opaque)
>>> +{
>>> +    BoolProperty *prop = opaque;
>>> +    g_free(prop);
>>> +}
>>> +
>>> +void object_property_add_bool(Object *obj, const char *name,
>>> +                              bool (*get)(Object *, Error **),
>>> +                              void (*set)(Object *, bool, Error **),
>>> +                              Error **errp)
>>> +{
>>> +    BoolProperty *prop = g_malloc0(sizeof(*prop));
>>> +
>>> +    prop->get = get;
>>> +    prop->set = set;
>>> +
>>> +    object_property_add(obj, name, "bool",
>>> +                        get ? property_get_bool : NULL,
>>> +                        set ? property_set_bool : NULL,
>>> +                        property_release_bool,
>>> +                        prop, errp);
>>> +}
>>> +
>>>   static char *qdev_get_type(Object *obj, Error **errp)
>>>   {
>>>       return g_strdup(object_get_typename(obj));

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg