diff mbox

[v5,7/8] qom: add a object_property_add_enum helper method

Message ID 1432739276-10452-8-git-send-email-berrange@redhat.com
State New
Headers show

Commit Message

Daniel P. Berrangé May 27, 2015, 3:07 p.m. UTC
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 | 73 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 150 insertions(+)

Comments

Eric Blake June 19, 2015, 4:12 p.m. UTC | #1
On 05/27/2015 09:07 AM, Daniel P. Berrange wrote:
> 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 {

>  /**
> + * 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.
> + * @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

s/a enum/an enum/

> +static void property_release_enum(Object *obj, const char *name,
> +                                  void *opaque)
> +{
> +    EnumProperty *prop = opaque;
> +    g_free(prop);

Could also be written as g_free(opaque), but that's a little less
readable, so I don't see a point in changing it.
Andreas Färber June 19, 2015, 4:26 p.m. UTC | #2
Am 19.06.2015 um 18:12 schrieb Eric Blake:
> On 05/27/2015 09:07 AM, Daniel P. Berrange wrote:
>> 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 {
> 
>>  /**
>> + * 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.
>> + * @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
> 
> s/a enum/an enum/

Thanks, fixed on https://github.com/afaerber/qemu-cpu/commits/qom-next

>> +static void property_release_enum(Object *obj, const char *name,
>> +                                  void *opaque)
>> +{
>> +    EnumProperty *prop = opaque;
>> +    g_free(prop);
> 
> Could also be written as g_free(opaque), but that's a little less
> readable, so I don't see a point in changing it.

I prefer it this way. Consuming opaques directly broke a few call sites
when I refactored the CPU structs, so the savings of one line is not
really worth it IMO.

Regards,
Andreas
Eric Blake June 19, 2015, 4:38 p.m. UTC | #3
On 05/27/2015 09:07 AM, Daniel P. Berrange wrote:
> 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.
> 

> 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;

s/endpoint/devtype/

> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
Andreas Färber June 19, 2015, 4:44 p.m. UTC | #4
Am 19.06.2015 um 18:38 schrieb Eric Blake:
> On 05/27/2015 09:07 AM, Daniel P. Berrange wrote:
>> 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.
>>
> 
>> 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;
> 
> s/endpoint/devtype/

Fixed, and s/parent/parent_obj/ while at it.

Andreas

> 
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> ---
>
diff mbox

Patch

diff --git a/include/qom/object.h b/include/qom/object.h
index d07a506..270a3ef 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1344,6 +1344,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.
+ * @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 '@typename'.
+ */
+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 1421e5a..f0923c5 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1069,6 +1069,12 @@  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)
 {
@@ -1657,6 +1663,58 @@  void object_property_add_bool(Object *obj, const char *name,
     }
 }
 
+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_malloc(sizeof(*prop));
+
+    prop->strings = strings;
+    prop->get = get;
+    prop->set = set;
+
+    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 e82532e..ae4cd4c 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 dummy_animal_map[DUMMY_LAST + 1] = {
+    [DUMMY_FROG] = "frog",
+    [DUMMY_ALLIGATOR] = "alligator",
+    [DUMMY_PLATYPUS] = "platypus",
+    [DUMMY_LAST] = NULL,
+};
+
 struct DummyObject {
     Object parent_obj;
 
     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",
+                             dummy_animal_map,
+                             dummy_get_av,
+                             dummy_set_av,
+                             NULL);
 }
 
 static void dummy_finalize(Object *obj)
@@ -121,11 +163,13 @@  static void test_dummy_createv(void)
                               &err,
                               "bv", "yes",
                               "sv", "Hiss hiss hiss",
+                              "av", "platypus",
                               NULL));
 
     g_assert(err == NULL);
     g_assert_cmpstr(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));
@@ -160,11 +204,13 @@  static void test_dummy_createlist(void)
                    parent,
                    "bv", "yes",
                    "sv", "Hiss hiss hiss",
+                   "av", "platypus",
                    NULL));
 
     g_assert(err == NULL);
     g_assert_cmpstr(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));
@@ -172,6 +218,32 @@  static void test_dummy_createlist(void)
     object_unparent(OBJECT(dobj));
 }
 
+static void test_dummy_badenum(void)
+{
+    Error *err = NULL;
+    Object *parent = object_get_objects_root();
+    Object *dobj =
+        object_new_with_props(TYPE_DUMMY,
+                              parent,
+                              "dummy0",
+                              &err,
+                              "bv", "yes",
+                              "sv", "Hiss hiss hiss",
+                              "av", "yeti",
+                              NULL);
+
+    g_assert(dobj == NULL);
+    g_assert(err != NULL);
+    g_assert_cmpstr(error_get_pretty(err), ==,
+                    "Invalid parameter 'yeti'");
+
+    g_assert(object_resolve_path_component(parent, "dummy0")
+             == NULL);
+
+    error_free(err);
+}
+
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -181,6 +253,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();
 }