Message ID | 1430476206-26034-7-git-send-email-berrange@redhat.com |
---|---|
State | New |
Headers | show |
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 <berrange@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
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
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. + * @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'. + */ +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; + + 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_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); + g_assert(err != NULL); + g_assert(g_str_equal(error_get_pretty(err), + "Invalid parameter 'yeti'")); + + 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(); }
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 <berrange@redhat.com> --- include/qom/object.h | 19 ++++++++++++ qom/object.c | 58 ++++++++++++++++++++++++++++++++++++ tests/check-qom-proplist.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 151 insertions(+)