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

[Anthony Liguori]

On 06/25/2012 11:32 AM, Andreas Färber wrote:
> Am 25.06.2012 18:09, schrieb Anthony Liguori:
> > Signed-off-by: Anthony Liguori<address@hidden>
> > ---
> >   include/qemu/object.h |   16 ++++++++++++++
> >   qom/object.c          |   56 +++++++++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 72 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/qemu/object.h b/include/qemu/object.h
> > index 8b17776..3711054 100644
> > --- a/include/qemu/object.h
> > +++ b/include/qemu/object.h
> > @@ -933,6 +933,22 @@ void object_property_add_str(Object *obj, const char *name,
> >                                struct Error **errp);
> >
> >   /**
> > + * object_property_add_bool:
> > + * @obj: the object to add a property to
> > + * @name: the name of the property
> > + * @get: the getter or NULL if the property is write-only.
> > + * @set: the setter or NULL if the property is read-only
> > + * @errp: if an error occurs, a pointer to an area to store the error
> > + *
> > + * Add a bool property using getters/setters.  This function will add a
> > + * property of type 'bool'.
> > + */
> > +void object_property_add_bool(Object *obj, const char *name,
> > +                             bool (*get)(Object *, struct Error **),
> > +                             void (*set)(Object *, bool, struct Error **),
> > +                             struct Error **errp);
>
> Indentation is off by one.

doh, c&p error.

> > +
> > +/**
> >    * object_child_foreach:
> >    * @obj: the object whose children will be navigated
> >    * @fn: the iterator function to be called
> > 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?

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));