Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange: > Now that properties can be explicitly registered as an enum > type, there is no need to pass the string table to the > object_get_enum method. The object property registration > already has a pointer to the string table. > > In changing this method signature, the hostmem backend object > has to be converted to use the new enum property registration > code, which simplifies it somewhat. > > Signed-off-by: Daniel P. Berrange <berra...@redhat.com> > --- > backends/hostmem.c | 22 ++++++++-------------- > include/qom/object.h | 4 ++-- > numa.c | 2 +- > qom/object.c | 32 ++++++++++++++++++++++++-------- > tests/check-qom-proplist.c | 46 > ++++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 81 insertions(+), 25 deletions(-) > > diff --git a/backends/hostmem.c b/backends/hostmem.c > index f6db33c..7d74be0 100644 > --- a/backends/hostmem.c > +++ b/backends/hostmem.c > @@ -113,24 +113,17 @@ host_memory_backend_set_host_nodes(Object *obj, Visitor > *v, void *opaque, > #endif > } > > -static void > -host_memory_backend_get_policy(Object *obj, Visitor *v, void *opaque, > - const char *name, Error **errp) > +static int > +host_memory_backend_get_policy(Object *obj, Error **errp G_GNUC_UNUSED) > { > HostMemoryBackend *backend = MEMORY_BACKEND(obj); > - int policy = backend->policy; > - > - visit_type_enum(v, &policy, HostMemPolicy_lookup, NULL, name, errp); > + return backend->policy; > } > > static void > -host_memory_backend_set_policy(Object *obj, Visitor *v, void *opaque, > - const char *name, Error **errp) > +host_memory_backend_set_policy(Object *obj, int policy, Error **errp) > { > HostMemoryBackend *backend = MEMORY_BACKEND(obj); > - int policy; > - > - visit_type_enum(v, &policy, HostMemPolicy_lookup, NULL, name, errp); > backend->policy = policy; > > #ifndef CONFIG_NUMA > @@ -252,9 +245,10 @@ static void host_memory_backend_init(Object *obj) > object_property_add(obj, "host-nodes", "int", > host_memory_backend_get_host_nodes, > host_memory_backend_set_host_nodes, NULL, NULL, > NULL); > - object_property_add(obj, "policy", "HostMemPolicy", > - host_memory_backend_get_policy, > - host_memory_backend_set_policy, NULL, NULL, NULL); > + object_property_add_enum(obj, "policy", "HostMemPolicy", > + HostMemPolicy_lookup, > + host_memory_backend_get_policy, > + host_memory_backend_set_policy, NULL); > } > > MemoryRegion * > diff --git a/include/qom/object.h b/include/qom/object.h > index f6a2a9d..fc347b9 100644 > --- a/include/qom/object.h > +++ b/include/qom/object.h > @@ -1012,7 +1012,7 @@ int64_t object_property_get_int(Object *obj, const char > *name, > * object_property_get_enum: > * @obj: the object > * @name: the name of the property > - * @strings: strings corresponding to enums > + * @typename: the name of the enum data type > * @errp: returns an error if this function fails > * > * Returns: the value of the property, converted to an integer, or > @@ -1020,7 +1020,7 @@ int64_t object_property_get_int(Object *obj, const char > *name, > * an enum). > */ > int object_property_get_enum(Object *obj, const char *name, > - const char * const strings[], Error **errp); > + const char *typename, Error **errp); > > /** > * object_property_get_uint16List: > diff --git a/numa.c b/numa.c > index c975fb2..a64279a 100644 > --- a/numa.c > +++ b/numa.c > @@ -457,7 +457,7 @@ static int query_memdev(Object *obj, void *opaque) > > m->value->policy = object_property_get_enum(obj, > "policy", > - HostMemPolicy_lookup, > + "HostMemPolicy", > &err); > if (err) { > goto error; > diff --git a/qom/object.c b/qom/object.c > index ba0e4b8..6d2a2a9 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -1026,13 +1026,35 @@ int64_t object_property_get_int(Object *obj, const > char *name, > return retval; > } > > +typedef struct EnumProperty { > + const char * const *strings; > + int (*get)(Object *, Error **); > + void (*set)(Object *, int, Error **);
Since get and set and moved unchanged, I would prefer placing it in the final destination in the original patch to avoid churn. > +} EnumProperty; > + > + > int object_property_get_enum(Object *obj, const char *name, > - const char * const strings[], Error **errp) > + const char *typename, Error **errp) > { > StringOutputVisitor *sov; > StringInputVisitor *siv; > char *str; > int ret; > + ObjectProperty *prop = object_property_find(obj, name, errp); > + EnumProperty *enumprop; > + > + if (prop == NULL) { > + return 0; > + } > + > + if (!g_str_equal(prop->type, typename)) { > + error_setg(errp, "Property %s on %s is not '%s' enum type", > + name, object_class_get_name( > + object_get_class(obj)), typename); > + return 0; > + } > + > + enumprop = prop->opaque; > > sov = string_output_visitor_new(false); > object_property_get(obj, string_output_get_visitor(sov), name, errp); > @@ -1040,7 +1062,7 @@ int object_property_get_enum(Object *obj, const char > *name, > siv = string_input_visitor_new(str); > string_output_visitor_cleanup(sov); > visit_type_enum(string_input_get_visitor(siv), > - &ret, strings, NULL, name, errp); > + &ret, enumprop->strings, NULL, name, errp); > > g_free(str); > string_input_visitor_cleanup(siv); > @@ -1609,12 +1631,6 @@ 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) > { > diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c > index de142e3..d5cd38b 100644 > --- a/tests/check-qom-proplist.c > +++ b/tests/check-qom-proplist.c > @@ -249,6 +249,51 @@ static void test_dummy_badenum(void) > } > > > + > +static void test_dummy_getenum(void) > +{ > + Error *err = NULL; > + int val; > + Object *parent = container_get(object_get_root(), > + "/objects"); > + DummyObject *dobj = DUMMY_OBJECT( > + object_new_propv(TYPE_DUMMY, > + parent, > + "dummy0", > + &err, > + "av", "platypus", > + NULL)); > + > + g_assert(dobj != NULL); > + g_assert(err == NULL); > + g_assert(dobj->av == DUMMY_PLATYPUS); > + > + val = object_property_get_enum(OBJECT(dobj), > + "av", > + "DummyAnimal", > + &err); > + g_assert(err == NULL); Is there any significant difference between g_assert()'ing on error and passing &error_abort? > + g_assert(val == DUMMY_PLATYPUS); > + > + /* A bad enum type name */ > + val = object_property_get_enum(OBJECT(dobj), > + "av", > + "BadAnimal", > + &err); > + g_assert(err != NULL); > + error_free(err); > + err = NULL; > + > + /* A non-enum property name */ > + val = object_property_get_enum(OBJECT(dobj), > + "iv", > + "DummyAnimal", > + &err); > + g_assert(err != NULL); > + error_free(err); > +} > + > + > int main(int argc, char **argv) > { > g_test_init(&argc, &argv, NULL); > @@ -259,6 +304,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); > + g_test_add_func("/qom/proplist/getenum", test_dummy_getenum); > > return g_test_run(); > } Otherwise looks good (apart from the dependencies). 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)