From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58487) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YqmUA-0002Lw-Na for qemu-devel@nongnu.org; Fri, 08 May 2015 13:54:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YqmU5-0005Uz-Ln for qemu-devel@nongnu.org; Fri, 08 May 2015 13:54:54 -0400 Received: from cantor2.suse.de ([195.135.220.15]:43703 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YqmU5-0005Ud-CM for qemu-devel@nongnu.org; Fri, 08 May 2015 13:54:49 -0400 Message-ID: <554CF868.1080906@suse.de> Date: Fri, 08 May 2015 19:54:48 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1430476206-26034-1-git-send-email-berrange@redhat.com> <1430476206-26034-8-git-send-email-berrange@redhat.com> In-Reply-To: <1430476206-26034-8-git-send-email-berrange@redhat.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 7/7] qom: don't pass string table to object_get_enum method List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Paolo Bonzini 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. >=20 > 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. >=20 > Signed-off-by: Daniel P. Berrange > --- > 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(-) >=20 > 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, V= isitor *v, void *opaque, > #endif > } > =20 > -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 =3D MEMORY_BACKEND(obj); > - int policy =3D backend->policy; > - > - visit_type_enum(v, &policy, HostMemPolicy_lookup, NULL, name, errp= ); > + return backend->policy; > } > =20 > 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 =3D MEMORY_BACKEND(obj); > - int policy; > - > - visit_type_enum(v, &policy, HostMemPolicy_lookup, NULL, name, errp= ); > backend->policy =3D policy; > =20 > #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, NU= LL); > + object_property_add_enum(obj, "policy", "HostMemPolicy", > + HostMemPolicy_lookup, > + host_memory_backend_get_policy, > + host_memory_backend_set_policy, NULL); > } > =20 > 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, cons= t 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, cons= t char *name, > * an enum). > */ > int object_property_get_enum(Object *obj, const char *name, > - const char * const strings[], Error **err= p); > + const char *typename, Error **errp); > =20 > /** > * 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) > =20 > m->value->policy =3D object_property_get_enum(obj, > "policy", > - HostMemPolicy_look= up, > + "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, co= nst char *name, > return retval; > } > =20 > +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 **err= p) > + const char *typename, Error **errp) > { > StringOutputVisitor *sov; > StringInputVisitor *siv; > char *str; > int ret; > + ObjectProperty *prop =3D object_property_find(obj, name, errp); > + EnumProperty *enumprop; > + > + if (prop =3D=3D 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 =3D prop->opaque; > =20 > sov =3D string_output_visitor_new(false); > object_property_get(obj, string_output_get_visitor(sov), name, err= p); > @@ -1040,7 +1062,7 @@ int object_property_get_enum(Object *obj, const c= har *name, > siv =3D 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); > =20 > g_free(str); > string_input_visitor_cleanup(siv); > @@ -1609,12 +1631,6 @@ void object_property_add_bool(Object *obj, const= char *name, > } > } > =20 > -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) > } > =20 > =20 > + > +static void test_dummy_getenum(void) > +{ > + Error *err =3D NULL; > + int val; > + Object *parent =3D container_get(object_get_root(), > + "/objects"); > + DummyObject *dobj =3D DUMMY_OBJECT( > + object_new_propv(TYPE_DUMMY, > + parent, > + "dummy0", > + &err, > + "av", "platypus", > + NULL)); > + > + g_assert(dobj !=3D NULL); > + g_assert(err =3D=3D NULL); > + g_assert(dobj->av =3D=3D DUMMY_PLATYPUS); > + > + val =3D object_property_get_enum(OBJECT(dobj), > + "av", > + "DummyAnimal", > + &err); > + g_assert(err =3D=3D NULL); Is there any significant difference between g_assert()'ing on error and passing &error_abort? > + g_assert(val =3D=3D DUMMY_PLATYPUS); > + > + /* A bad enum type name */ > + val =3D object_property_get_enum(OBJECT(dobj), > + "av", > + "BadAnimal", > + &err); > + g_assert(err !=3D NULL); > + error_free(err); > + err =3D NULL; > + > + /* A non-enum property name */ > + val =3D object_property_get_enum(OBJECT(dobj), > + "iv", > + "DummyAnimal", > + &err); > + g_assert(err !=3D 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); > =20 > return g_test_run(); > } Otherwise looks good (apart from the dependencies). Regards, Andreas --=20 SUSE Linux GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Felix Imend=F6rffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB 21284 (AG N=FCrnberg)