From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55000) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YqmKo-0003im-St for qemu-devel@nongnu.org; Fri, 08 May 2015 13:45:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YqmKl-0000m6-KE for qemu-devel@nongnu.org; Fri, 08 May 2015 13:45:14 -0400 Received: from cantor2.suse.de ([195.135.220.15]:43274 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YqmKl-0000lv-Aa for qemu-devel@nongnu.org; Fri, 08 May 2015 13:45:11 -0400 Message-ID: <554CF626.5000009@suse.de> Date: Fri, 08 May 2015 19:45:10 +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-7-git-send-email-berrange@redhat.com> In-Reply-To: <1430476206-26034-7-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 6/7] qom: add a object_property_add_enum helper 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: > 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. >=20 > 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. >=20 > typedef enum { > MYDEV_TYPE_FROG, > MYDEV_TYPE_ALLIGATOR, > MYDEV_TYPE_PLATYPUS, >=20 > MYDEV_TYPE_LAST > } MyDevType; >=20 > Then provide a table of enum <-> string mappings >=20 > static const char *const mydevtypemap[MYDEV_TYPE_LAST + 1] =3D { > [MYDEV_TYPE_FROG] =3D "frog", > [MYDEV_TYPE_ALLIGATOR] =3D "alligator", > [MYDEV_TYPE_PLATYPUS] =3D "platypus", > [MYDEV_TYPE_LAST] =3D NULL, > }; >=20 > Assuming an object struct of >=20 > typedef struct { > Object parent; > MyDevType devtype; > ...other fields... > } MyDev; >=20 > The property can then be registered as follows: >=20 > static int mydev_prop_get_devtype(Object *obj, > Error **errp G_GNUC_UNUSED) > { > MyDev *dev =3D MYDEV(obj); >=20 > return dev->devtype; > } >=20 > static void mydev_prop_set_devtype(Object *obj, > int value, > Error **errp G_GNUC_UNUSED) > { > MyDev *dev =3D MYDEV(obj); >=20 > dev->endpoint =3D value; > } >=20 > object_property_add_enum(obj, "devtype", > mydevtypemap, "MyDevType", > mydev_prop_get_devtype, > mydev_prop_set_devtype, > NULL); >=20 > 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. >=20 > Signed-off-by: Daniel P. Berrange > --- > 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); > =20 > /** > + * 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, > } > } > =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) > +{ > + EnumProperty *prop =3D opaque; > + int value; > + > + value =3D 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 =3D 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 =3D 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 =3D NULL; > + EnumProperty *prop =3D g_malloc0(sizeof(*prop)); > + > + prop->strings =3D strings; > + prop->get =3D get; > + prop->set =3D 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) > =20 > +typedef enum DummyAnimal DummyAnimal; > + > +enum DummyAnimal { > + DUMMY_FROG, > + DUMMY_ALLIGATOR, > + DUMMY_PLATYPUS, > + > + DUMMY_LAST, > +}; > + > +static const char *const dummyanimalmap[DUMMY_LAST + 1] =3D { dummy_animal_map would be slightly easier to read. > + [DUMMY_FROG] =3D "frog", > + [DUMMY_ALLIGATOR] =3D "alligator", > + [DUMMY_PLATYPUS] =3D "platypus", > + [DUMMY_LAST] =3D NULL, > +}; > + > struct DummyObject { > Object parent; > =20 > bool bv; > + DummyAnimal av; > char *sv; > }; > =20 > @@ -62,6 +80,24 @@ static bool dummy_get_bv(Object *obj, > } > =20 > =20 > +static void dummy_set_av(Object *obj, > + int value, > + Error **errp) > +{ > + DummyObject *dobj =3D DUMMY_OBJECT(obj); > + > + dobj->av =3D value; > +} > + > +static int dummy_get_av(Object *obj, > + Error **errp) > +{ > + DummyObject *dobj =3D 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); > } > =20 > 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)); > =20 > g_assert(dobj !=3D NULL); > g_assert(err =3D=3D NULL); > g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss")); > g_assert(dobj->bv =3D=3D true); > + g_assert(dobj->av =3D=3D DUMMY_PLATYPUS); > =20 > g_assert(object_resolve_path_component(parent, "dummy0") > =3D=3D OBJECT(dobj)); > @@ -163,12 +207,14 @@ static void test_dummy_createlist(void) > parent, > "bv", "yes", > "sv", "Hiss hiss hiss", > + "av", "platypus", > NULL)); > =20 > g_assert(dobj !=3D NULL); > g_assert(err =3D=3D NULL); > g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss")); > g_assert(dobj->bv =3D=3D true); > + g_assert(dobj->av =3D=3D DUMMY_PLATYPUS); > =20 > g_assert(object_resolve_path_component(parent, "dummy0") > =3D=3D OBJECT(dobj)); > @@ -176,6 +222,33 @@ static void test_dummy_createlist(void) > object_unparent(OBJECT(dobj)); > } > =20 > +static void test_dummy_badenum(void) > +{ > + Error *err =3D NULL; > + Object *parent =3D container_get(object_get_root(), > + "/objects"); > + DummyObject *dobj =3D DUMMY_OBJECT( > + object_new_propv(TYPE_DUMMY, > + parent, > + "dummy0", > + &err, > + "bv", "yes", > + "sv", "Hiss hiss hiss", > + "av", "yeti", > + NULL)); > + > + g_assert(dobj =3D=3D NULL); Superfluous. > + g_assert(err !=3D 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") > + =3D=3D 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) > =20 > 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); > =20 > return g_test_run(); > } 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)