Re: [Qemu-devel] [PATCH v3 6/7] qom: add a object_property_add_enum helper method

["Andreas Färber" <afaerber@suse.de>]

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 <berrange@redhat.com>
> ---
>  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)