| Message ID | 1430476206-26034-8-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <berrange@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
On Fri, May 08, 2015 at 07:54:48PM +0200, Andreas Färber wrote: > 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 <berrange@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/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. Yep, easy to do. > > 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? I didn't know about &error_abort until now :-) I will use that. Regards, Daniel
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 **); +} 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); + 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(); }
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 <berrange@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(-)