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

[Anthony Liguori]

On 06/25/2012 03:47 PM, Andreas Färber wrote:
> 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.

Okay, this is not the approach for this I had envisioned.

Instead, I see this as a complex property.  That is, one who's visitors function 
uses a nested struct.  IOW:

/machine/cpu[1]/features -> type X86CPUFeatures

which visits as:

{ 'x2apic': True, 'sse4': False, ... }

We haven't really encountered what setting a complex type entails yet.  I think 
with the existing QAPI visitors, you could very well do partial setting and it 
would all Just Work.

So if you model this as a QAPI type, you'd get it all for free.  So just add:

{ 'type': 'X86CPUFeatures', 'data': {'x2apic': 'bool', 'sse4': 'bool', ...} }

to qapi-schema.json

Then make use of the X86CPUFeatures generated type to actually deal with 
features.

Then you can just use visit_type_X86CPUFeatures to implement the 
PropertyAccessor methods.

Then the following QMP command would be valid:

{ 'command': 'qom-set', 'arguments': { 'path': '/machine/cpu[1]', 'property': 
'features', 'value': { 'x2apic': true } } }

And calling it multiple times to set specific properties would be valid.

It's all up to what you do in PropertyAccessor with the resulting Error object.

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

I think you're modeling something in QOM that's quite reasonably handled with 
Visitors.  There's no need to try to introduce a container.

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

That's why properties use visitors and exactly what complex types are for.

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

No, there's a side-affect so it's not a static property.

> Is that the new design paradigm?

I've *never* liked static properties.  There's nothing new about that.  But from 
the earliest proposals around QOM, I've always said I wanted to allow 
setter/getter functions for properties.  In fact, that's why we have one in the 
rtc device because that was the first example.

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

The various object_property_set_* functions are inconsistent because I didn't 
write them.  I'm not sure why they take value/name.  They should take name/value 
IMHO.

The visitor functions take:

Visitor, opaque, name, error

That's because you're visiting a value (in this case, opaque is substituting for 
value) and the name is merely a hint.  It's often ignored.  That's very 
different from property_set/get where the name is the thing you're actually 
working on (it's value is very important).

Regards,

Anthony Liguori

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