Re: [Qemu-devel] [PATCH v3 6/7] qom: add a object_property_add_enum helper method

[Daniel P. Berrange]

On Fri, May 08, 2015 at 07:45:10PM +0200, Andreas Färber wrote:
> Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange:
> 
> Looks good in general. Some minor nits below, and one limitation
> possibly worth mentioning in the second paragraph of the commit message:
> It assumes a 1:1 mapping. I do guess most ones are, but I remember some
> CPUID bits having different names for the same values, for instance.

Worst case, in that edge case, we can simply not use this stricter
enum property type - just carry on with existing custom property

> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index bf76f7a..f6a2a9d 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -1271,6 +1271,25 @@ void object_property_add_bool(Object *obj, const 
> > char *name,
> >                                Error **errp);
> >  
> >  /**
> > + * object_property_add_enum:
> > + * @obj: the object to add a property to
> > + * @name: the name of the property
> > + * @typename: the name of the enum data type
> > + * @get: the getter or NULL if the property is write-only.
> 
> %NULL
> 
> > + * @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 enum property using getters/setters.  This function will add a
> > + * property of type 'enum'.
> 
> This is slightly ambiguous, as I understand it the type we're actually
> using is the one in @typename, not "enum"?

Yeah, you are right - this doc mistake is left over from a previous
version before Paolo asked me to add the @typename parameter.

> > diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> > index 9f16cdb..de142e3 100644
> > --- a/tests/check-qom-proplist.c
> > +++ b/tests/check-qom-proplist.c
> > @@ -32,10 +32,28 @@ typedef struct DummyObjectClass DummyObjectClass;
> >  #define DUMMY_OBJECT(obj)                               \
> >      OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY)
> >  
> > +typedef enum DummyAnimal DummyAnimal;
> > +
> > +enum DummyAnimal {
> > +    DUMMY_FROG,
> > +    DUMMY_ALLIGATOR,
> > +    DUMMY_PLATYPUS,
> > +
> > +    DUMMY_LAST,
> > +};
> > +
> > +static const char *const dummyanimalmap[DUMMY_LAST + 1] = {
> 
> dummy_animal_map would be slightly easier to read.

Sure

> > +static void test_dummy_badenum(void)
> > +{
> > +    Error *err = NULL;
> > +    Object *parent = container_get(object_get_root(),
> > +                                   "/objects");
> > +    DummyObject *dobj = DUMMY_OBJECT(
> > +        object_new_propv(TYPE_DUMMY,
> > +                         parent,
> > +                         "dummy0",
> > +                         &err,
> > +                         "bv", "yes",
> > +                         "sv", "Hiss hiss hiss",
> > +                         "av", "yeti",
> > +                         NULL));
> > +
> > +    g_assert(dobj == NULL);
> 
> Superfluous.
> 
> > +    g_assert(err != NULL);
> > +    g_assert(g_str_equal(error_get_pretty(err),
> > +                         "Invalid parameter 'yeti'"));
> 
> Same question as in previous test: alternatives?

Yep, will check

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|