Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange: > A QOM property can be parsed as enum using the visit_type_enum() > helper method, but this forces callers to use the more complex > generic object_property_add() method when registering it. It > also requires that users of that object have access to the > string map when they want to read the property value. > > This patch introduces a specialized object_property_add_enum() > method which simplifies the use of enum properties, so the > setters/getters directly get passed the int value. > > typedef enum { > MYDEV_TYPE_FROG, > MYDEV_TYPE_ALLIGATOR, > MYDEV_TYPE_PLATYPUS, > > MYDEV_TYPE_LAST > } MyDevType; > > Then provide a table of enum <-> string mappings > > static const char *const mydevtypemap[MYDEV_TYPE_LAST + 1] = { > [MYDEV_TYPE_FROG] = "frog", > [MYDEV_TYPE_ALLIGATOR] = "alligator", > [MYDEV_TYPE_PLATYPUS] = "platypus", > [MYDEV_TYPE_LAST] = NULL, > }; > > Assuming an object struct of > > typedef struct { > Object parent; > MyDevType devtype; > ...other fields... > } MyDev; > > The property can then be registered as follows: > > static int mydev_prop_get_devtype(Object *obj, > Error **errp G_GNUC_UNUSED) > { > MyDev *dev = MYDEV(obj); > > return dev->devtype; > } > > static void mydev_prop_set_devtype(Object *obj, > int value, > Error **errp G_GNUC_UNUSED) > { > MyDev *dev = MYDEV(obj); > > dev->endpoint = value; > } > > object_property_add_enum(obj, "devtype", > mydevtypemap, "MyDevType", > mydev_prop_get_devtype, > mydev_prop_set_devtype, > NULL); > > Note there is no need to check the range of 'value' in > the setter, because the string->enum conversion code will > have already done that and reported an error as required. > > Signed-off-by: Daniel P. Berrange <berra...@redhat.com> > --- > include/qom/object.h | 19 ++++++++++++ > qom/object.c | 58 ++++++++++++++++++++++++++++++++++++ > tests/check-qom-proplist.c | 74 > ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 151 insertions(+)
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. > 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"? > + */ > +void object_property_add_enum(Object *obj, const char *name, > + const char *typename, > + const char * const *strings, > + int (*get)(Object *, Error **), > + void (*set)(Object *, int, Error **), > + Error **errp); > + > +/** > * object_property_add_tm: > * @obj: the object to add a property to > * @name: the name of the property > diff --git a/qom/object.c b/qom/object.c > index 077a5fe..ba0e4b8 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -1609,6 +1609,64 @@ void object_property_add_bool(Object *obj, const char > *name, > } > } > > +typedef struct EnumProperty { > + const char * const *strings; > + int (*get)(Object *, Error **); > + void (*set)(Object *, int, Error **); > +} EnumProperty; > + > +static void property_get_enum(Object *obj, Visitor *v, void *opaque, > + const char *name, Error **errp) > +{ > + EnumProperty *prop = opaque; > + int value; > + > + value = prop->get(obj, errp); > + visit_type_enum(v, &value, prop->strings, NULL, name, errp); > +} > + > +static void property_set_enum(Object *obj, Visitor *v, void *opaque, > + const char *name, Error **errp) > +{ > + EnumProperty *prop = opaque; > + int value; > + > + visit_type_enum(v, &value, prop->strings, NULL, name, errp); > + prop->set(obj, value, errp); > +} > + > +static void property_release_enum(Object *obj, const char *name, > + void *opaque) > +{ > + EnumProperty *prop = opaque; > + g_free(prop); > +} > + > +void object_property_add_enum(Object *obj, const char *name, > + const char *typename, > + const char * const *strings, > + int (*get)(Object *, Error **), > + void (*set)(Object *, int, Error **), > + Error **errp) > +{ > + Error *local_err = NULL; > + EnumProperty *prop = g_malloc0(sizeof(*prop)); > + > + prop->strings = strings; > + prop->get = get; > + prop->set = set; Since all three fields are set, we could also use g_malloc() as micro-optimization. > + > + object_property_add(obj, name, typename, > + get ? property_get_enum : NULL, > + set ? property_set_enum : NULL, > + property_release_enum, > + prop, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + g_free(prop); > + } > +} > + > typedef struct TMProperty { > void (*get)(Object *, struct tm *, Error **); > } TMProperty; > 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. > + [DUMMY_FROG] = "frog", > + [DUMMY_ALLIGATOR] = "alligator", > + [DUMMY_PLATYPUS] = "platypus", > + [DUMMY_LAST] = NULL, > +}; > + > struct DummyObject { > Object parent; > > bool bv; > + DummyAnimal av; > char *sv; > }; > > @@ -62,6 +80,24 @@ static bool dummy_get_bv(Object *obj, > } > > > +static void dummy_set_av(Object *obj, > + int value, > + Error **errp) > +{ > + DummyObject *dobj = DUMMY_OBJECT(obj); > + > + dobj->av = value; > +} > + > +static int dummy_get_av(Object *obj, > + Error **errp) > +{ > + DummyObject *dobj = DUMMY_OBJECT(obj); > + > + return dobj->av; > +} > + > + > static void dummy_set_sv(Object *obj, > const char *value, > Error **errp) > @@ -91,6 +127,12 @@ static void dummy_init(Object *obj) > dummy_get_sv, > dummy_set_sv, > NULL); > + object_property_add_enum(obj, "av", > + "DummyAnimal", > + dummyanimalmap, > + dummy_get_av, > + dummy_set_av, > + NULL); > } > > static void dummy_finalize(Object *obj) > @@ -122,12 +164,14 @@ static void test_dummy_createv(void) > &err, > "bv", "yes", > "sv", "Hiss hiss hiss", > + "av", "platypus", > NULL)); > > g_assert(dobj != NULL); > g_assert(err == NULL); > g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss")); > g_assert(dobj->bv == true); > + g_assert(dobj->av == DUMMY_PLATYPUS); > > g_assert(object_resolve_path_component(parent, "dummy0") > == OBJECT(dobj)); > @@ -163,12 +207,14 @@ static void test_dummy_createlist(void) > parent, > "bv", "yes", > "sv", "Hiss hiss hiss", > + "av", "platypus", > NULL)); > > g_assert(dobj != NULL); > g_assert(err == NULL); > g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss")); > g_assert(dobj->bv == true); > + g_assert(dobj->av == DUMMY_PLATYPUS); > > g_assert(object_resolve_path_component(parent, "dummy0") > == OBJECT(dobj)); > @@ -176,6 +222,33 @@ static void test_dummy_createlist(void) > object_unparent(OBJECT(dobj)); > } > > +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? > + > + g_assert(object_resolve_path_component(parent, "dummy0") > + == NULL); > + > + error_free(err); > +} > + > + > int main(int argc, char **argv) > { > g_test_init(&argc, &argv, NULL); > @@ -185,6 +258,7 @@ int main(int argc, char **argv) > > g_test_add_func("/qom/proplist/createlist", test_dummy_createlist); > g_test_add_func("/qom/proplist/createv", test_dummy_createv); > + g_test_add_func("/qom/proplist/badenum", test_dummy_badenum); > > return g_test_run(); > } Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB 21284 (AG Nürnberg)