Re: [Qemu-devel] [PATCH v3 7/7] qom: don't pass string table to object_get_enum method

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

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.
> 
> 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.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  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(-)
> 
> 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, Visitor *v, void *opaque,
>  #endif
>  }
>  
> -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 = MEMORY_BACKEND(obj);
> -    int policy = backend->policy;
> -
> -    visit_type_enum(v, &policy, HostMemPolicy_lookup, NULL, name, errp);
> +    return backend->policy;
>  }
>  
>  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 = MEMORY_BACKEND(obj);
> -    int policy;
> -
> -    visit_type_enum(v, &policy, HostMemPolicy_lookup, NULL, name, errp);
>      backend->policy = policy;
>  
>  #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, NULL);
> +    object_property_add_enum(obj, "policy", "HostMemPolicy",
> +                             HostMemPolicy_lookup,
> +                             host_memory_backend_get_policy,
> +                             host_memory_backend_set_policy, NULL);
>  }
>  
>  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, const 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, const char *name,
>   * an enum).
>   */
>  int object_property_get_enum(Object *obj, const char *name,
> -                             const char * const strings[], Error **errp);
> +                             const char *typename, Error **errp);
>  
>  /**
>   * 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)
>  
>          m->value->policy = object_property_get_enum(obj,
>                                                      "policy",
> -                                                    HostMemPolicy_lookup,
> +                                                    "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, const char *name,
>      return retval;
>  }
>  
> +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 **errp)
> +                             const char *typename, Error **errp)
>  {
>      StringOutputVisitor *sov;
>      StringInputVisitor *siv;
>      char *str;
>      int ret;
> +    ObjectProperty *prop = object_property_find(obj, name, errp);
> +    EnumProperty *enumprop;
> +
> +    if (prop == 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 = prop->opaque;
>  
>      sov = string_output_visitor_new(false);
>      object_property_get(obj, string_output_get_visitor(sov), name, errp);
> @@ -1040,7 +1062,7 @@ int object_property_get_enum(Object *obj, const char *name,
>      siv = 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);
>  
>      g_free(str);
>      string_input_visitor_cleanup(siv);
> @@ -1609,12 +1631,6 @@ 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)
>  {
> 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)
>  }
>  
>  
> +
> +static void test_dummy_getenum(void)
> +{
> +    Error *err = NULL;
> +    int val;
> +    Object *parent = container_get(object_get_root(),
> +                                   "/objects");
> +    DummyObject *dobj = DUMMY_OBJECT(
> +        object_new_propv(TYPE_DUMMY,
> +                         parent,
> +                         "dummy0",
> +                         &err,
> +                         "av", "platypus",
> +                         NULL));
> +
> +    g_assert(dobj != NULL);
> +    g_assert(err == NULL);
> +    g_assert(dobj->av == DUMMY_PLATYPUS);
> +
> +    val = object_property_get_enum(OBJECT(dobj),
> +                                   "av",
> +                                   "DummyAnimal",
> +                                   &err);
> +    g_assert(err == NULL);

Is there any significant difference between g_assert()'ing on error and
passing &error_abort?

> +    g_assert(val == DUMMY_PLATYPUS);
> +
> +    /* A bad enum type name */
> +    val = object_property_get_enum(OBJECT(dobj),
> +                                   "av",
> +                                   "BadAnimal",
> +                                   &err);
> +    g_assert(err != NULL);
> +    error_free(err);
> +    err = NULL;
> +
> +    /* A non-enum property name */
> +    val = object_property_get_enum(OBJECT(dobj),
> +                                   "iv",
> +                                   "DummyAnimal",
> +                                   &err);
> +    g_assert(err != 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);
>  
>      return g_test_run();
>  }

Otherwise looks good (apart from the dependencies).

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)