[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 6/7] qom: add a object_property_add_enum help
From: |
Andreas Färber |
Subject: |
Re: [Qemu-devel] [PATCH v3 6/7] qom: add a object_property_add_enum helper method |
Date: |
Fri, 08 May 2015 19:45:10 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 |
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 <address@hidden>
> ---
> 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
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
- Re: [Qemu-devel] [PATCH v3 3/7] qom: create objects in two phases, (continued)
- [Qemu-devel] [PATCH v3 5/7] qom: make enum string tables const-correct, Daniel P. Berrange, 2015/05/01
- [Qemu-devel] [PATCH v3 6/7] qom: add a object_property_add_enum helper method, Daniel P. Berrange, 2015/05/01
- Re: [Qemu-devel] [PATCH v3 6/7] qom: add a object_property_add_enum helper method,
Andreas Färber <=
- [Qemu-devel] [PATCH v3 7/7] qom: don't pass string table to object_get_enum method, Daniel P. Berrange, 2015/05/01
- Re: [Qemu-devel] [PATCH v3 0/7] qom: misc fixes & enhancements to support TLS work, Paolo Bonzini, 2015/05/05