[Qemu-devel] [PATCH 0/3] qom: refactor Interfaces

[Qemu-devel] [PATCH 0/3] qom: refactor Interfaces

[Anthony Liguori]

The interface implementation was pretty busted.  The way it created Objects for
each interface was extremely clumbsy and brittle.

This is a new implementation that does something quite a bit more natural.  It
simply modifies classes such that they can affectively have more than one super
class.

Interfaces never get instantiated.  Instead an object's class just refers to its
parent class and it's implemented interfaces.

This should solve the issues Peter's run into and also eliminate the recursive
call to object_new() (its no longer necessary to allocate anything when creating
an object).

This also comes with a test case for object.

[Qemu-devel] [PATCH 1/3] tests: fix dependency inclusion

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 32550cb..93046bf 100644
--- a/Makefile
+++ b/Makefile
@@ -406,4 +406,4 @@ Makefile: $(GENERATED_HEADERS)
 
 # Include automatically generated dependency files
 -include $(wildcard *.d audio/*.d slirp/*.d block/*.d net/*.d ui/*.d qapi/*.d)
--include $(wildcard qga/*.d hw/*.d hw/usb/*.d)
+-include $(wildcard qga/*.d hw/*.d hw/usb/*.d tests/*.d)
-- 
1.7.5.4

[Qemu-devel] [PATCH 2/3] qom: reimplement Interfaces

[Anthony Liguori]

The current implementation of Interfaces is poorly designed.  Each interface
that an object implements end up being an object that's tracked by the
implementing object.  There's all sorts of gymnastics to deal with casting
between these objects.

By an interface shouldn't be associated with an Object.  Interfaces are global
to a class.  This patch moves all Interface knowledge to ObjectClass eliminating
the relationship between Object and Interfaces.

Interfaces are now abstract (as they should be) but this is okay.  Interfaces
essentially act as additional parents for the classes and are treated as such.

With this new implementation, we should fully support derived interfaces
including reimplementing an inherited interface.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 include/qemu/object.h |   64 +++++++++++---
 qom/object.c          |  231 +++++++++++++++++++++++-------------------------
 2 files changed, 160 insertions(+), 135 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index d93b772..72cb290 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -239,6 +239,7 @@ struct ObjectClass
 {
     /*< private >*/
     Type type;
+    GSList *interfaces;
 };
 
 /**
@@ -260,7 +261,6 @@ struct Object
 {
     /*< private >*/
     ObjectClass *class;
-    GSList *interfaces;
     QTAILQ_HEAD(, ObjectProperty) properties;
     uint32_t ref;
     Object *parent;
@@ -381,6 +381,17 @@ struct TypeInfo
     OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name)
 
 /**
+ * InterfaceInfo:
+ * @type: The name of the interface.
+ *
+ * The information associated with an interface.
+ */
+struct InterfaceInfo
+{
+    const char *type;
+};
+
+/**
  * InterfaceClass:
  * @parent_class: the base class
  *
@@ -390,26 +401,31 @@ struct TypeInfo
 struct InterfaceClass
 {
     ObjectClass parent_class;
+    /*< private >*/
+    ObjectClass *concrete_class;
 };
 
+#define TYPE_INTERFACE "interface"
+
 /**
- * InterfaceInfo:
- * @type: The name of the interface.
- * @interface_initfn: This method is called during class initialization and is
- *   used to initialize an interface associated with a class.  This function
- *   should initialize any default virtual functions for a class and/or override
- *   virtual functions in a parent class.
+ * INTERFACE_CLASS:
+ * @klass: class to cast from
  *
- * The information associated with an interface.
+ * Returns: An #InterfaceClass or raise an error if cast is invalid
  */
-struct InterfaceInfo
-{
-    const char *type;
+#define INTERFACE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(InterfaceClass, klass, TYPE_INTERFACE)
 
-    void (*interface_initfn)(ObjectClass *class, void *data);
-};
-
-#define TYPE_INTERFACE "interface"
+/**
+ * INTERFACE_CHECK:
+ * @interface: the type to return
+ * @obj: the object to convert to an interface
+ * @name: the interface type name
+ *
+ * Returns: @obj casted to @interface if cast is valid, otherwise raise error.
+ */
+#define INTERFACE_CHECK(interface, obj, name) \
+    ((interface *)interface_dynamic_cast_assert(OBJECT((obj)), (name)))
 
 /**
  * object_new:
@@ -548,6 +564,24 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *klass,
                                        const char *typename);
 
 /**
+ * interface_dynamic_cast_assert:
+ * @obj: the object to cast to an interface type
+ * @typename: the type name of the interface
+ *
+ * Returns: @obj if @obj implements @typename, otherwise raise an error
+ */
+Object *interface_dynamic_cast_assert(Object *obj, const char *typename);
+
+/**
+ * interface_dynamic_cast_assert:
+ * @obj: the object to cast to an interface type
+ * @typename: the type name of the interface
+ *
+ * Returns: @obj if @obj implements @typename, otherwise %NULL
+ */
+Object *interface_dynamic_cast(Object *obj, const char *typename);
+
+/**
  * object_class_get_name:
  * @klass: The class to obtain the QOM typename for.
  *
diff --git a/qom/object.c b/qom/object.c
index 6f839ad..aa26693 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -31,9 +31,7 @@ typedef struct TypeImpl TypeImpl;
 
 struct InterfaceImpl
 {
-    const char *parent;
-    void (*interface_initfn)(ObjectClass *class, void *data);
-    TypeImpl *type;
+    const char *typename;
 };
 
 struct TypeImpl
@@ -63,14 +61,6 @@ struct TypeImpl
     InterfaceImpl interfaces[MAX_INTERFACES];
 };
 
-typedef struct Interface
-{
-    Object parent;
-    Object *obj;
-} Interface;
-
-#define INTERFACE(obj) OBJECT_CHECK(Interface, obj, TYPE_INTERFACE)
-
 static Type type_interface;
 
 static GHashTable *type_table_get(void)
@@ -97,6 +87,7 @@ static TypeImpl *type_table_lookup(const char *name)
 TypeImpl *type_register(const TypeInfo *info)
 {
     TypeImpl *ti = g_malloc0(sizeof(*ti));
+    int i;
 
     g_assert(info->name != NULL);
 
@@ -120,15 +111,10 @@ TypeImpl *type_register(const TypeInfo *info)
 
     ti->abstract = info->abstract;
 
-    if (info->interfaces) {
-        int i;
-
-        for (i = 0; info->interfaces[i].type; i++) {
-            ti->interfaces[i].parent = info->interfaces[i].type;
-            ti->interfaces[i].interface_initfn = info->interfaces[i].interface_initfn;
-            ti->num_interfaces++;
-        }
+    for (i = 0; info->interfaces && info->interfaces[i].type; i++) {
+        ti->interfaces[i].typename = g_strdup(info->interfaces[i].type);
     }
+    ti->num_interfaces = i;
 
     type_table_add(ti);
 
@@ -190,26 +176,48 @@ static size_t type_object_get_size(TypeImpl *ti)
     return 0;
 }
 
-static void type_class_interface_init(TypeImpl *ti, InterfaceImpl *iface)
+static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
 {
-    TypeInfo info = {
-        .instance_size = sizeof(Interface),
-        .parent = iface->parent,
-        .class_size = sizeof(InterfaceClass),
-        .class_init = iface->interface_initfn,
-        .abstract = true,
-    };
-    char *name = g_strdup_printf("<%s::%s>", ti->name, iface->parent);
+    assert(target_type);
+
+    /* Check if typename is a direct ancestor of type */
+    while (type) {
+        if (type == target_type) {
+            return true;
+        }
+
+        type = type_get_parent(type);
+    }
+
+    return false;
+}
+
+static void type_initialize(TypeImpl *ti);
+
+static void type_initialize_interface(TypeImpl *ti, const char *parent)
+{
+    InterfaceClass *new_iface;
+    TypeInfo info = { };
+    TypeImpl *iface_impl;
 
-    info.name = name;
-    iface->type = type_register(&info);
-    g_free(name);
+    info.parent = parent;
+    info.name = g_strdup_printf("%s::%s", ti->name, info.parent);
+    info.abstract = true;
+
+    iface_impl = type_register(&info);
+    type_initialize(iface_impl);
+    g_free((char *)info.name);
+
+    new_iface = (InterfaceClass *)iface_impl->class;
+    new_iface->concrete_class = ti->class;
+
+    ti->class->interfaces = g_slist_append(ti->class->interfaces,
+                                           iface_impl->class);
 }
 
 static void type_initialize(TypeImpl *ti)
 {
     size_t class_size = sizeof(ObjectClass);
-    int i;
 
     if (ti->class) {
         return;
@@ -221,8 +229,12 @@ static void type_initialize(TypeImpl *ti)
     ti->class = g_malloc0(ti->class_size);
     ti->class->type = ti;
 
+    ti->class->interfaces = NULL;
+
     if (type_has_parent(ti)) {
         TypeImpl *parent = type_get_parent(ti);
+        GSList *e;
+        int i;
 
         type_initialize(parent);
 
@@ -232,42 +244,46 @@ static void type_initialize(TypeImpl *ti)
         memcpy((void *)ti->class + sizeof(ObjectClass),
                (void *)parent->class + sizeof(ObjectClass),
                parent->class_size - sizeof(ObjectClass));
-    }
 
-    memset((void *)ti->class + class_size, 0, ti->class_size - class_size);
+        for (e = parent->class->interfaces; e; e = e->next) {
+            ObjectClass *iface = e->data;
+            type_initialize_interface(ti, object_class_get_name(iface));
+        }
 
-    for (i = 0; i < ti->num_interfaces; i++) {
-        type_class_interface_init(ti, &ti->interfaces[i]);
-    }
+        for (i = 0; i < ti->num_interfaces; i++) {
+            TypeImpl *t = type_get_by_name(ti->interfaces[i].typename);
 
-    if (ti->class_init) {
-        ti->class_init(ti->class, ti->class_data);
+            for (e = ti->class->interfaces; e; e = e->next) {
+                TypeImpl *target_type = OBJECT_CLASS(e->data)->type;
+
+                if (type_is_ancestor(target_type, t)) {
+                    break;
+                }
+            }
+
+            if (e) {
+                continue;
+            }
+                
+            type_initialize_interface(ti, ti->interfaces[i].typename);
+        }
     }
-}
 
-static void object_interface_init(Object *obj, InterfaceImpl *iface)
-{
-    TypeImpl *ti = iface->type;
-    Interface *iface_obj;
+    /* If this type is an interface and num_interfaces, bugger out */
 
-    iface_obj = INTERFACE(object_new(ti->name));
-    iface_obj->obj = obj;
+    memset((void *)ti->class + class_size, 0, ti->class_size - class_size);
 
-    obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
+    if (ti->class_init) {
+        ti->class_init(ti->class, ti->class_data);
+    }
 }
 
 static void object_init_with_type(Object *obj, TypeImpl *ti)
 {
-    int i;
-
     if (type_has_parent(ti)) {
         object_init_with_type(obj, type_get_parent(ti));
     }
 
-    for (i = 0; i < ti->num_interfaces; i++) {
-        object_interface_init(obj, &ti->interfaces[i]);
-    }
-
     if (ti->instance_init) {
         ti->instance_init(obj);
     }
@@ -338,12 +354,6 @@ static void object_deinit(Object *obj, TypeImpl *type)
         type->instance_finalize(obj);
     }
 
-    while (obj->interfaces) {
-        Interface *iface_obj = obj->interfaces->data;
-        obj->interfaces = g_slist_delete_link(obj->interfaces, obj->interfaces);
-        object_delete(OBJECT(iface_obj));
-    }
-
     if (type_has_parent(type)) {
         object_deinit(obj, type_get_parent(type));
     }
@@ -390,22 +400,6 @@ void object_delete(Object *obj)
     g_free(obj);
 }
 
-static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
-{
-    assert(target_type);
-
-    /* Check if typename is a direct ancestor of type */
-    while (type) {
-        if (type == target_type) {
-            return true;
-        }
-
-        type = type_get_parent(type);
-    }
-
-    return false;
-}
-
 static bool object_is_type(Object *obj, TypeImpl *target_type)
 {
     return !target_type || type_is_ancestor(obj->class->type, target_type);
@@ -414,7 +408,6 @@ static bool object_is_type(Object *obj, TypeImpl *target_type)
 Object *object_dynamic_cast(Object *obj, const char *typename)
 {
     TypeImpl *target_type = type_get_by_name(typename);
-    GSList *i;
 
     /* Check if typename is a direct ancestor.  Special-case TYPE_OBJECT,
      * we want to go back from interfaces to the parent.
@@ -423,46 +416,18 @@ Object *object_dynamic_cast(Object *obj, const char *typename)
         return obj;
     }
 
-    /* Check if obj is an interface and its containing object is a direct
-     * ancestor of typename.  In principle we could do this test at the very
-     * beginning of object_dynamic_cast, avoiding a second call to
-     * object_is_type.  However, casting between interfaces is relatively
-     * rare, and object_is_type(obj, type_interface) would fail almost always.
-     *
-     * Perhaps we could add a magic value to the object header for increased
-     * (run-time) type safety and to speed up tests like this one.  If we ever
-     * do that we can revisit the order here.
-     */
-    if (object_is_type(obj, type_interface)) {
-        assert(!obj->interfaces);
-        obj = INTERFACE(obj)->obj;
-        if (object_is_type(obj, target_type)) {
-            return obj;
-        }
-    }
-
     if (!target_type) {
         return obj;
     }
 
-    /* Check if obj has an interface of typename */
-    for (i = obj->interfaces; i; i = i->next) {
-        Interface *iface = i->data;
-
-        if (object_is_type(OBJECT(iface), target_type)) {
-            return OBJECT(iface);
-        }
-    }
-
     return NULL;
 }
 
-
 static void register_types(void)
 {
     static TypeInfo interface_info = {
         .name = TYPE_INTERFACE,
-        .instance_size = sizeof(Interface),
+        .class_size = sizeof(InterfaceClass),
         .abstract = true,
     };
 
@@ -491,16 +456,30 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
 {
     TypeImpl *target_type = type_get_by_name(typename);
     TypeImpl *type = class->type;
+    ObjectClass *ret = NULL;
 
-    while (type) {
-        if (type == target_type) {
-            return class;
+    if (type->num_interfaces && type_is_ancestor(target_type, type_interface)) {
+        int found = 0;
+        GSList *i;
+
+        for (i = class->interfaces; i; i = i->next) {
+            ObjectClass *target_class = i->data;
+
+            if (type_is_ancestor(target_class->type, target_type)) {
+                ret = target_class;
+                found++;
+            }
         }
 
-        type = type_get_parent(type);
+        /* The match was ambiguous, don't allow a cast */
+        if (found > 1) {
+            ret = NULL;
+        }
+    } else if (type_is_ancestor(type, target_type)) {
+        ret = class;
     }
 
-    return NULL;
+    return ret;
 }
 
 ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
@@ -517,6 +496,28 @@ ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
     return ret;
 }
 
+Object *interface_dynamic_cast(Object *obj, const char *typename)
+{
+    if (object_class_dynamic_cast(object_get_class(obj), typename)) {
+        return obj;
+    }
+
+    return NULL;
+}
+
+Object *interface_dynamic_cast_assert(Object *obj, const char *typename)
+{
+    Object *ret = interface_dynamic_cast(obj, typename);
+
+    if (!ret) {
+        fprintf(stderr, "Object %p is not an instance of type %s\n",
+                obj, typename);
+        abort();
+    }
+
+    return ret;
+}
+
 const char *object_get_typename(Object *obj)
 {
     return obj->class->type->name;
@@ -883,12 +884,6 @@ void object_property_add_child(Object *obj, const char *name,
 {
     gchar *type;
 
-    /* Registering an interface object in the composition tree will mightily
-     * confuse object_get_canonical_path (which, on the other hand, knows how
-     * to get the canonical path of an interface object).
-     */
-    assert(!object_is_type(obj, type_interface));
-
     type = g_strdup_printf("child<%s>", object_get_typename(OBJECT(child)));
 
     object_property_add(obj, name, type, object_get_child_property,
@@ -985,10 +980,6 @@ gchar *object_get_canonical_path(Object *obj)
     Object *root = object_get_root();
     char *newpath = NULL, *path = NULL;
 
-    if (object_is_type(obj, type_interface)) {
-        obj = INTERFACE(obj)->obj;
-    }
-
     while (obj != root) {
         ObjectProperty *prop = NULL;
 
-- 
1.7.5.4

[Qemu-devel] [PATCH 3/3] qom: add unit test for Interfaces

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 tests/Makefile      |    5 +-
 tests/test-object.c |  222 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 226 insertions(+), 1 deletions(-)
 create mode 100644 tests/test-object.c

diff --git a/tests/Makefile b/tests/Makefile
index d66ab19..d1f979d 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -14,6 +14,7 @@ check-unit-y += tests/test-string-input-visitor$(EXESUF)
 check-unit-y += tests/test-string-output-visitor$(EXESUF)
 check-unit-y += tests/test-coroutine$(EXESUF)
 check-unit-y += tests/test-visitor-serialization$(EXESUF)
+check-unit-y += tests/test-object$(EXESUF)
 
 check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
 
@@ -32,7 +33,8 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
 	tests/test-coroutine.o tests/test-string-output-visitor.o \
 	tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
 	tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
-	tests/test-qmp-commands.o tests/test-visitor-serialization.o
+	tests/test-qmp-commands.o tests/test-visitor-serialization.o \
+	tests/test-object.o
 
 test-qapi-obj-y =  $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y)
 test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
@@ -66,6 +68,7 @@ tests/test-qmp-input-visitor$(EXESUF): tests/test-qmp-input-visitor.o $(test-qap
 tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o $(test-qapi-obj-y)
 tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o tests/test-qmp-marshal.o $(test-qapi-obj-y)
 tests/test-visitor-serialization$(EXESUF): tests/test-visitor-serialization.o $(test-qapi-obj-y)
+tests/test-object$(EXESUF): tests/test-object.o $(qom-obj-y) $(test-qapi-obj-y)
 
 tests/rtc-test$(EXESUF): tests/rtc-test.o $(trace-obj-y)
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o $(trace-obj-y)
diff --git a/tests/test-object.c b/tests/test-object.c
new file mode 100644
index 0000000..9f41da0
--- /dev/null
+++ b/tests/test-object.c
@@ -0,0 +1,222 @@
+/*
+ * QEMU Object Model unit test
+ *
+ * Copyright IBM, Corp. 2012
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#include "qemu/object.h"
+#include "module.h"
+
+#define TYPE_HERBIVORE "herbivore"
+
+#define HERBIVORE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(HerbivoreClass, (klass), TYPE_HERBIVORE)
+#define HERBIVORE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(HerbivoreClass, (obj), TYPE_HERBIVORE)
+#define HERBIVORE(obj) \
+    INTERFACE_CHECK(Herbivore, (obj), TYPE_HERBIVORE)
+
+typedef struct Herbivore
+{
+    Object obj;
+} Herbivore;
+
+typedef struct HerbivoreClass
+{
+    InterfaceClass parent;
+
+    void (*feed_greens)(Herbivore *obj);
+} HerbivoreClass;
+
+static void herbivore_feed_greens(Herbivore *herbie)
+{
+    HerbivoreClass *k = HERBIVORE_GET_CLASS(herbie);
+
+    k->feed_greens(herbie);
+}
+
+static TypeInfo herbivore_info = {
+    .name = TYPE_HERBIVORE,
+    .parent = TYPE_INTERFACE,
+    .class_size = sizeof(HerbivoreClass),
+};
+
+#define TYPE_CARNIVORE "carnivore"
+#define CARNIVORE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(CarnivoreClass, (klass), TYPE_CARNIVORE)
+#define CARNIVORE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(CarnivoreClass, (obj), TYPE_CARNIVORE)
+#define CARNIVORE(obj) \
+    INTERFACE_CHECK(Carnivore, (obj), TYPE_CARNIVORE)
+
+typedef struct Carnivore
+{
+    Object parent;
+} Carnivore;
+
+typedef struct CarnivoreClass
+{
+    InterfaceClass parent;
+
+    void (*feed_bugs)(Carnivore *obj);
+} CarnivoreClass;
+
+static void carnivore_feed_bugs(Carnivore *carnie)
+{
+    CarnivoreClass *k = CARNIVORE_GET_CLASS(carnie);
+
+    k->feed_bugs(carnie);
+}
+
+static TypeInfo carnivore_info = {
+    .name = TYPE_CARNIVORE,
+    .parent = TYPE_INTERFACE,
+    .class_size = sizeof(CarnivoreClass),
+};
+
+#define TYPE_REPTILE "reptile"
+#define REPTILE(obj) OBJECT_CHECK(Reptile, (obj), TYPE_REPTILE)
+
+typedef struct Reptile
+{
+    Object parent;
+} Reptile;
+
+static TypeInfo reptile_info = {
+    .name = TYPE_REPTILE,
+    .instance_size = sizeof(Reptile),
+    .abstract = true,
+    .class_size = sizeof(ObjectClass),
+};
+
+#define TYPE_LIZARD "lizard"
+#define LIZARD(obj) OBJECT_CHECK(Lizard, (obj), TYPE_LIZARD)
+
+typedef struct Lizard
+{
+    Reptile parent;
+} Lizard;
+
+static TypeInfo lizard_info = {
+    .name = TYPE_LIZARD,
+    .parent = TYPE_REPTILE,
+    .instance_size = sizeof(Lizard),
+    .abstract = true,
+};
+
+#define TYPE_IGUANA "iguana"
+#define IGUANA(obj) OBJECT_CHECK(Iguana, (obj), TYPE_IGUANA)
+
+typedef struct Iguana
+{
+    Lizard parent;
+    int greens;
+} Iguana;
+
+static void iguana_feed(Herbivore *herbie)
+{
+    Iguana *iggie = IGUANA(herbie);
+
+    iggie->greens++;
+}
+
+static void iguana_class_initfn(ObjectClass *klass, void *data)
+{
+    HerbivoreClass *iface = HERBIVORE_CLASS(klass);
+
+    iface->feed_greens = iguana_feed;
+}
+
+static TypeInfo iguana_info = {
+    .name = TYPE_IGUANA,
+    .parent = TYPE_LIZARD,
+    .instance_size = sizeof(Iguana),
+    .class_init = iguana_class_initfn,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HERBIVORE },
+        { }
+    },
+};
+
+#define TYPE_BEARDED_DRAGON "bearded-dragon"
+#define BEARDED_DRAGON(obj) OBJECT_CHECK(BeardedDragon, (obj), TYPE_BEARDED_DRAGON)
+
+typedef struct BeardedDragon
+{
+    Lizard parent;
+    int bugs;
+} BeardedDragon;
+
+static void bearded_dragon_feed(Carnivore *carnie)
+{
+    BeardedDragon *dragon = BEARDED_DRAGON(carnie);
+
+    dragon->bugs++;
+}
+
+static void bearded_dragon_class_initfn(ObjectClass *klass, void *data)
+{
+    CarnivoreClass *iface = CARNIVORE_CLASS(klass);
+
+    iface->feed_bugs = bearded_dragon_feed;
+}
+
+static TypeInfo bearded_dragon_info = {
+    .name = TYPE_BEARDED_DRAGON,
+    .parent = TYPE_LIZARD,
+    .instance_size = sizeof(BeardedDragon),
+    .class_init = bearded_dragon_class_initfn,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_CARNIVORE },
+        { }
+    },
+};
+
+static void basic_init(void)
+{
+    BeardedDragon dragon;
+    Iguana iguana;
+
+    object_initialize(&dragon, TYPE_BEARDED_DRAGON);
+
+    g_assert_cmpint(dragon.bugs, ==, 0);
+    carnivore_feed_bugs(CARNIVORE(&dragon));
+    g_assert_cmpint(dragon.bugs, ==, 1);
+
+    object_finalize(&dragon);
+
+    object_initialize(&iguana, TYPE_IGUANA);
+
+    g_assert_cmpint(iguana.greens, ==, 0);
+    herbivore_feed_greens(HERBIVORE(&iguana));
+    g_assert_cmpint(iguana.greens, ==, 1);
+
+    object_finalize(&iguana);
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    module_call_init(MODULE_INIT_QOM);
+
+    type_register_static(&carnivore_info);
+    type_register_static(&herbivore_info);
+
+    type_register_static(&reptile_info);
+    type_register_static(&lizard_info);
+    type_register_static(&iguana_info);
+    type_register_static(&bearded_dragon_info);
+
+    g_test_add_func("/basic/init", basic_init);
+
+    g_test_run();
+
+    return 0;
+}
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 0/3] qom: refactor Interfaces

[Paolo Bonzini]

Il 13/06/2012 22:54, Anthony Liguori ha scritto:
> The interface implementation was pretty busted.  The way it created Objects for
> each interface was extremely clumbsy and brittle.
> 
> This is a new implementation that does something quite a bit more natural.  It
> simply modifies classes such that they can affectively have more than one super
> class.
> 
> Interfaces never get instantiated.  Instead an object's class just refers to its
> parent class and it's implemented interfaces.
> 
> This should solve the issues Peter's run into and also eliminate the recursive
> call to object_new() (its no longer necessary to allocate anything when creating
> an object).
> 
> This also comes with a test case for object.
> 

I gave it only a quick look, but the approach is much more sane.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH 1/3] tests: fix dependency inclusion

[Andreas Färber]

Am 13.06.2012 22:55, schrieb Anthony Liguori:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  Makefile |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 32550cb..93046bf 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -406,4 +406,4 @@ Makefile: $(GENERATED_HEADERS)
>  
>  # Include automatically generated dependency files
>  -include $(wildcard *.d audio/*.d slirp/*.d block/*.d net/*.d ui/*.d qapi/*.d)
> --include $(wildcard qga/*.d hw/*.d hw/usb/*.d)
> +-include $(wildcard qga/*.d hw/*.d hw/usb/*.d tests/*.d)

The equivalent fix for qom/*.d was denied me just recently...

I don't see that solved through Paolo's Makefile redesign though, my
local patch still applies.

Andreas
-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/3] tests: fix dependency inclusion

[Paolo Bonzini]

Il 14/06/2012 15:08, Andreas Färber ha scritto:
>> >  # Include automatically generated dependency files
>> >  -include $(wildcard *.d audio/*.d slirp/*.d block/*.d net/*.d ui/*.d qapi/*.d)
>> > --include $(wildcard qga/*.d hw/*.d hw/usb/*.d)
>> > +-include $(wildcard qga/*.d hw/*.d hw/usb/*.d tests/*.d)
> The equivalent fix for qom/*.d was denied me just recently...
> 
> I don't see that solved through Paolo's Makefile redesign though, my
> local patch still applies.

Yeah, let's just do this for now (which applies to your patch as well,
Andreas).  We'll fix it right for 1.2, but that's no reason to inflict
pain upon ourselves until then.

Paolo

Re: [Qemu-devel] [PATCH 0/3] qom: refactor Interfaces

[Peter Crosthwaite]

On Thu, Jun 14, 2012 at 6:54 AM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> The interface implementation was pretty busted.  The way it created Objects for
> each interface was extremely clumbsy and brittle.
>
> This is a new implementation that does something quite a bit more natural.  It
> simply modifies classes such that they can affectively have more than one super
> class.
>
> Interfaces never get instantiated.  Instead an object's class just refers to its
> parent class and it's implemented interfaces.
>
> This should solve the issues Peter's run into

I will rebase my series on this and retest shortly.

Regards,
Peter

 and also eliminate the recursive
> call to object_new() (its no longer necessary to allocate anything when creating
> an object).
>
> This also comes with a test case for object.
>
>

Re: [Qemu-devel] [PATCH 3/3] qom: add unit test for Interfaces

[Peter Crosthwaite]

On Thu, Jun 14, 2012 at 6:55 AM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Reviewed-by: Peter A.G. Crosthwaite <peter.crosthwaite@petalogix.com>

> ---
>  tests/Makefile      |    5 +-
>  tests/test-object.c |  222 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 226 insertions(+), 1 deletions(-)
>  create mode 100644 tests/test-object.c
>
> diff --git a/tests/Makefile b/tests/Makefile
> index d66ab19..d1f979d 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -14,6 +14,7 @@ check-unit-y += tests/test-string-input-visitor$(EXESUF)
>  check-unit-y += tests/test-string-output-visitor$(EXESUF)
>  check-unit-y += tests/test-coroutine$(EXESUF)
>  check-unit-y += tests/test-visitor-serialization$(EXESUF)
> +check-unit-y += tests/test-object$(EXESUF)
>
>  check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
>
> @@ -32,7 +33,8 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
>        tests/test-coroutine.o tests/test-string-output-visitor.o \
>        tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
>        tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
> -       tests/test-qmp-commands.o tests/test-visitor-serialization.o
> +       tests/test-qmp-commands.o tests/test-visitor-serialization.o \
> +       tests/test-object.o
>
>  test-qapi-obj-y =  $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y)
>  test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
> @@ -66,6 +68,7 @@ tests/test-qmp-input-visitor$(EXESUF): tests/test-qmp-input-visitor.o $(test-qap
>  tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o $(test-qapi-obj-y)
>  tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o tests/test-qmp-marshal.o $(test-qapi-obj-y)
>  tests/test-visitor-serialization$(EXESUF): tests/test-visitor-serialization.o $(test-qapi-obj-y)
> +tests/test-object$(EXESUF): tests/test-object.o $(qom-obj-y) $(test-qapi-obj-y)
>
>  tests/rtc-test$(EXESUF): tests/rtc-test.o $(trace-obj-y)
>  tests/m48t59-test$(EXESUF): tests/m48t59-test.o $(trace-obj-y)
> diff --git a/tests/test-object.c b/tests/test-object.c
> new file mode 100644
> index 0000000..9f41da0
> --- /dev/null
> +++ b/tests/test-object.c
> @@ -0,0 +1,222 @@
> +/*
> + * QEMU Object Model unit test
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +#include "qemu/object.h"
> +#include "module.h"
> +
> +#define TYPE_HERBIVORE "herbivore"
> +
> +#define HERBIVORE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(HerbivoreClass, (klass), TYPE_HERBIVORE)
> +#define HERBIVORE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(HerbivoreClass, (obj), TYPE_HERBIVORE)
> +#define HERBIVORE(obj) \
> +    INTERFACE_CHECK(Herbivore, (obj), TYPE_HERBIVORE)
> +
> +typedef struct Herbivore
> +{
> +    Object obj;
> +} Herbivore;

All this is doing is saying Herbivores are Objects right? A user cant
add anything to this struct given that interfaces are stateless so
could this be simplified to

typedef Object Herbivore;

?

> +
> +typedef struct HerbivoreClass
> +{
> +    InterfaceClass parent;
> +
> +    void (*feed_greens)(Herbivore *obj);
> +} HerbivoreClass;
> +
> +static void herbivore_feed_greens(Herbivore *herbie)
> +{
> +    HerbivoreClass *k = HERBIVORE_GET_CLASS(herbie);
> +
> +    k->feed_greens(herbie);
> +}
> +
> +static TypeInfo herbivore_info = {
> +    .name = TYPE_HERBIVORE,
> +    .parent = TYPE_INTERFACE,
> +    .class_size = sizeof(HerbivoreClass),
> +};
> +
> +#define TYPE_CARNIVORE "carnivore"
> +#define CARNIVORE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(CarnivoreClass, (klass), TYPE_CARNIVORE)
> +#define CARNIVORE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(CarnivoreClass, (obj), TYPE_CARNIVORE)
> +#define CARNIVORE(obj) \
> +    INTERFACE_CHECK(Carnivore, (obj), TYPE_CARNIVORE)
> +
> +typedef struct Carnivore
> +{
> +    Object parent;
> +} Carnivore;
> +
> +typedef struct CarnivoreClass
> +{
> +    InterfaceClass parent;
> +
> +    void (*feed_bugs)(Carnivore *obj);
> +} CarnivoreClass;
> +
> +static void carnivore_feed_bugs(Carnivore *carnie)
> +{
> +    CarnivoreClass *k = CARNIVORE_GET_CLASS(carnie);
> +
> +    k->feed_bugs(carnie);
> +}
> +
> +static TypeInfo carnivore_info = {
> +    .name = TYPE_CARNIVORE,
> +    .parent = TYPE_INTERFACE,
> +    .class_size = sizeof(CarnivoreClass),
> +};
> +
> +#define TYPE_REPTILE "reptile"
> +#define REPTILE(obj) OBJECT_CHECK(Reptile, (obj), TYPE_REPTILE)
> +
> +typedef struct Reptile
> +{
> +    Object parent;
> +} Reptile;
> +
> +static TypeInfo reptile_info = {
> +    .name = TYPE_REPTILE,
> +    .instance_size = sizeof(Reptile),
> +    .abstract = true,
> +    .class_size = sizeof(ObjectClass),
> +};
> +
> +#define TYPE_LIZARD "lizard"
> +#define LIZARD(obj) OBJECT_CHECK(Lizard, (obj), TYPE_LIZARD)
> +
> +typedef struct Lizard
> +{
> +    Reptile parent;
> +} Lizard;
> +
> +static TypeInfo lizard_info = {
> +    .name = TYPE_LIZARD,
> +    .parent = TYPE_REPTILE,
> +    .instance_size = sizeof(Lizard),
> +    .abstract = true,
> +};
> +
> +#define TYPE_IGUANA "iguana"
> +#define IGUANA(obj) OBJECT_CHECK(Iguana, (obj), TYPE_IGUANA)
> +
> +typedef struct Iguana
> +{
> +    Lizard parent;
> +    int greens;
> +} Iguana;
> +
> +static void iguana_feed(Herbivore *herbie)
> +{
> +    Iguana *iggie = IGUANA(herbie);
> +
> +    iggie->greens++;
> +}
> +
> +static void iguana_class_initfn(ObjectClass *klass, void *data)
> +{
> +    HerbivoreClass *iface = HERBIVORE_CLASS(klass);
> +
> +    iface->feed_greens = iguana_feed;
> +}
> +
> +static TypeInfo iguana_info = {
> +    .name = TYPE_IGUANA,
> +    .parent = TYPE_LIZARD,
> +    .instance_size = sizeof(Iguana),
> +    .class_init = iguana_class_initfn,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HERBIVORE },
> +        { }
> +    },
> +};
> +
> +#define TYPE_BEARDED_DRAGON "bearded-dragon"
> +#define BEARDED_DRAGON(obj) OBJECT_CHECK(BeardedDragon, (obj), TYPE_BEARDED_DRAGON)
> +
> +typedef struct BeardedDragon
> +{
> +    Lizard parent;
> +    int bugs;
> +} BeardedDragon;
> +
> +static void bearded_dragon_feed(Carnivore *carnie)
> +{
> +    BeardedDragon *dragon = BEARDED_DRAGON(carnie);
> +
> +    dragon->bugs++;
> +}
> +
> +static void bearded_dragon_class_initfn(ObjectClass *klass, void *data)
> +{
> +    CarnivoreClass *iface = CARNIVORE_CLASS(klass);
> +
> +    iface->feed_bugs = bearded_dragon_feed;
> +}
> +
> +static TypeInfo bearded_dragon_info = {
> +    .name = TYPE_BEARDED_DRAGON,
> +    .parent = TYPE_LIZARD,
> +    .instance_size = sizeof(BeardedDragon),
> +    .class_init = bearded_dragon_class_initfn,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_CARNIVORE },
> +        { }
> +    },
> +};
> +
> +static void basic_init(void)
> +{
> +    BeardedDragon dragon;
> +    Iguana iguana;
> +
> +    object_initialize(&dragon, TYPE_BEARDED_DRAGON);
> +
> +    g_assert_cmpint(dragon.bugs, ==, 0);
> +    carnivore_feed_bugs(CARNIVORE(&dragon));
> +    g_assert_cmpint(dragon.bugs, ==, 1);
> +
> +    object_finalize(&dragon);
> +
> +    object_initialize(&iguana, TYPE_IGUANA);
> +
> +    g_assert_cmpint(iguana.greens, ==, 0);
> +    herbivore_feed_greens(HERBIVORE(&iguana));
> +    g_assert_cmpint(iguana.greens, ==, 1);
> +
> +    object_finalize(&iguana);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    module_call_init(MODULE_INIT_QOM);
> +
> +    type_register_static(&carnivore_info);
> +    type_register_static(&herbivore_info);
> +
> +    type_register_static(&reptile_info);
> +    type_register_static(&lizard_info);
> +    type_register_static(&iguana_info);
> +    type_register_static(&bearded_dragon_info);
> +
> +    g_test_add_func("/basic/init", basic_init);
> +
> +    g_test_run();
> +
> +    return 0;
> +}
> --
> 1.7.5.4
>

Re: [Qemu-devel] [PATCH 2/3] qom: reimplement Interfaces

[Peter Crosthwaite]

On Wed, 2012-06-13 at 15:55 -0500, Anthony Liguori wrote:
> The current implementation of Interfaces is poorly designed.  Each interface
> that an object implements end up being an object that's tracked by the

"ends"

> implementing object.  There's all sorts of gymnastics to deal with casting
> between these objects.
> 
> By an interface shouldn't be associated with an Object.  Interfaces are global

"But" ?

> to a class.  This patch moves all Interface knowledge to ObjectClass eliminating
> the relationship between Object and Interfaces.
> 
> Interfaces are now abstract (as they should be) but this is okay.  Interfaces
> essentially act as additional parents for the classes and are treated as such.
> 
> With this new implementation, we should fully support derived interfaces
> including reimplementing an inherited interface.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  include/qemu/object.h |   64 +++++++++++---
>  qom/object.c          |  231 +++++++++++++++++++++++-------------------------
>  2 files changed, 160 insertions(+), 135 deletions(-)
> 
> diff --git a/include/qemu/object.h b/include/qemu/object.h
> index d93b772..72cb290 100644
> --- a/include/qemu/object.h
> +++ b/include/qemu/object.h
> @@ -239,6 +239,7 @@ struct ObjectClass
>  {
>      /*< private >*/
>      Type type;
> +    GSList *interfaces;
>  };
>  
>  /**
> @@ -260,7 +261,6 @@ struct Object
>  {
>      /*< private >*/
>      ObjectClass *class;
> -    GSList *interfaces;
>      QTAILQ_HEAD(, ObjectProperty) properties;
>      uint32_t ref;
>      Object *parent;
> @@ -381,6 +381,17 @@ struct TypeInfo
>      OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name)
>  
>  /**
> + * InterfaceInfo:
> + * @type: The name of the interface.
> + *
> + * The information associated with an interface.
> + */
> +struct InterfaceInfo
> +{
> +    const char *type;
> +};
> +
> +/**
>   * InterfaceClass:
>   * @parent_class: the base class
>   *
> @@ -390,26 +401,31 @@ struct TypeInfo
>  struct InterfaceClass
>  {
>      ObjectClass parent_class;
> +    /*< private >*/
> +    ObjectClass *concrete_class;
>  };
>  
> +#define TYPE_INTERFACE "interface"
> +
>  /**
> - * InterfaceInfo:
> - * @type: The name of the interface.
> - * @interface_initfn: This method is called during class initialization and is
> - *   used to initialize an interface associated with a class.  This function
> - *   should initialize any default virtual functions for a class and/or override
> - *   virtual functions in a parent class.
> + * INTERFACE_CLASS:
> + * @klass: class to cast from
>   *
> - * The information associated with an interface.
> + * Returns: An #InterfaceClass or raise an error if cast is invalid
>   */
> -struct InterfaceInfo
> -{
> -    const char *type;
> +#define INTERFACE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(InterfaceClass, klass, TYPE_INTERFACE)
>  
> -    void (*interface_initfn)(ObjectClass *class, void *data);
> -};
> -
> -#define TYPE_INTERFACE "interface"
> +/**
> + * INTERFACE_CHECK:
> + * @interface: the type to return
> + * @obj: the object to convert to an interface
> + * @name: the interface type name
> + *
> + * Returns: @obj casted to @interface if cast is valid, otherwise raise error.
> + */
> +#define INTERFACE_CHECK(interface, obj, name) \
> +    ((interface *)interface_dynamic_cast_assert(OBJECT((obj)), (name)))
>  
>  /**
>   * object_new:
> @@ -548,6 +564,24 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *klass,
>                                         const char *typename);
>  
>  /**
> + * interface_dynamic_cast_assert:
> + * @obj: the object to cast to an interface type
> + * @typename: the type name of the interface
> + *
> + * Returns: @obj if @obj implements @typename, otherwise raise an error
> + */
> +Object *interface_dynamic_cast_assert(Object *obj, const char *typename);
> +
> +/**
> + * interface_dynamic_cast_assert:
> + * @obj: the object to cast to an interface type
> + * @typename: the type name of the interface
> + *
> + * Returns: @obj if @obj implements @typename, otherwise %NULL
> + */
> +Object *interface_dynamic_cast(Object *obj, const char *typename);
> +

This is where bug was introduced for links. The link setter code uses
object_dynamic_cast() which shortcuts the logic here, that is needed for
casting interfaces. The new result is you can use interface with links
cos the cast pukes.

I have proposed a solution to this in my new revision (P3) of the
axi-stream series.

Regards,
Peter

> +/**
>   * object_class_get_name:
>   * @klass: The class to obtain the QOM typename for.
>   *
> diff --git a/qom/object.c b/qom/object.c
> index 6f839ad..aa26693 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -31,9 +31,7 @@ typedef struct TypeImpl TypeImpl;
>  
>  struct InterfaceImpl
>  {
> -    const char *parent;
> -    void (*interface_initfn)(ObjectClass *class, void *data);
> -    TypeImpl *type;
> +    const char *typename;
>  };
>  
>  struct TypeImpl
> @@ -63,14 +61,6 @@ struct TypeImpl
>      InterfaceImpl interfaces[MAX_INTERFACES];
>  };
>  
> -typedef struct Interface
> -{
> -    Object parent;
> -    Object *obj;
> -} Interface;
> -
> -#define INTERFACE(obj) OBJECT_CHECK(Interface, obj, TYPE_INTERFACE)
> -
>  static Type type_interface;
>  
>  static GHashTable *type_table_get(void)
> @@ -97,6 +87,7 @@ static TypeImpl *type_table_lookup(const char *name)
>  TypeImpl *type_register(const TypeInfo *info)
>  {
>      TypeImpl *ti = g_malloc0(sizeof(*ti));
> +    int i;
>  
>      g_assert(info->name != NULL);
>  
> @@ -120,15 +111,10 @@ TypeImpl *type_register(const TypeInfo *info)
>  
>      ti->abstract = info->abstract;
>  
> -    if (info->interfaces) {
> -        int i;
> -
> -        for (i = 0; info->interfaces[i].type; i++) {
> -            ti->interfaces[i].parent = info->interfaces[i].type;
> -            ti->interfaces[i].interface_initfn = info->interfaces[i].interface_initfn;
> -            ti->num_interfaces++;
> -        }
> +    for (i = 0; info->interfaces && info->interfaces[i].type; i++) {
> +        ti->interfaces[i].typename = g_strdup(info->interfaces[i].type);
>      }
> +    ti->num_interfaces = i;
>  
>      type_table_add(ti);
>  
> @@ -190,26 +176,48 @@ static size_t type_object_get_size(TypeImpl *ti)
>      return 0;
>  }
>  
> -static void type_class_interface_init(TypeImpl *ti, InterfaceImpl *iface)
> +static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
>  {
> -    TypeInfo info = {
> -        .instance_size = sizeof(Interface),
> -        .parent = iface->parent,
> -        .class_size = sizeof(InterfaceClass),
> -        .class_init = iface->interface_initfn,
> -        .abstract = true,
> -    };
> -    char *name = g_strdup_printf("<%s::%s>", ti->name, iface->parent);
> +    assert(target_type);
> +
> +    /* Check if typename is a direct ancestor of type */
> +    while (type) {
> +        if (type == target_type) {
> +            return true;
> +        }
> +
> +        type = type_get_parent(type);
> +    }
> +
> +    return false;
> +}
> +
> +static void type_initialize(TypeImpl *ti);
> +
> +static void type_initialize_interface(TypeImpl *ti, const char *parent)
> +{
> +    InterfaceClass *new_iface;
> +    TypeInfo info = { };
> +    TypeImpl *iface_impl;
>  
> -    info.name = name;
> -    iface->type = type_register(&info);
> -    g_free(name);
> +    info.parent = parent;
> +    info.name = g_strdup_printf("%s::%s", ti->name, info.parent);
> +    info.abstract = true;
> +
> +    iface_impl = type_register(&info);
> +    type_initialize(iface_impl);
> +    g_free((char *)info.name);
> +
> +    new_iface = (InterfaceClass *)iface_impl->class;
> +    new_iface->concrete_class = ti->class;
> +
> +    ti->class->interfaces = g_slist_append(ti->class->interfaces,
> +                                           iface_impl->class);
>  }
>  
>  static void type_initialize(TypeImpl *ti)
>  {
>      size_t class_size = sizeof(ObjectClass);
> -    int i;
>  
>      if (ti->class) {
>          return;
> @@ -221,8 +229,12 @@ static void type_initialize(TypeImpl *ti)
>      ti->class = g_malloc0(ti->class_size);
>      ti->class->type = ti;
>  
> +    ti->class->interfaces = NULL;
> +
>      if (type_has_parent(ti)) {
>          TypeImpl *parent = type_get_parent(ti);
> +        GSList *e;
> +        int i;
>  
>          type_initialize(parent);
>  
> @@ -232,42 +244,46 @@ static void type_initialize(TypeImpl *ti)
>          memcpy((void *)ti->class + sizeof(ObjectClass),
>                 (void *)parent->class + sizeof(ObjectClass),
>                 parent->class_size - sizeof(ObjectClass));
> -    }
>  
> -    memset((void *)ti->class + class_size, 0, ti->class_size - class_size);
> +        for (e = parent->class->interfaces; e; e = e->next) {
> +            ObjectClass *iface = e->data;
> +            type_initialize_interface(ti, object_class_get_name(iface));
> +        }
>  
> -    for (i = 0; i < ti->num_interfaces; i++) {
> -        type_class_interface_init(ti, &ti->interfaces[i]);
> -    }
> +        for (i = 0; i < ti->num_interfaces; i++) {
> +            TypeImpl *t = type_get_by_name(ti->interfaces[i].typename);
>  
> -    if (ti->class_init) {
> -        ti->class_init(ti->class, ti->class_data);
> +            for (e = ti->class->interfaces; e; e = e->next) {
> +                TypeImpl *target_type = OBJECT_CLASS(e->data)->type;
> +
> +                if (type_is_ancestor(target_type, t)) {
> +                    break;
> +                }
> +            }
> +
> +            if (e) {
> +                continue;
> +            }
> +                
> +            type_initialize_interface(ti, ti->interfaces[i].typename);
> +        }
>      }
> -}
>  
> -static void object_interface_init(Object *obj, InterfaceImpl *iface)
> -{
> -    TypeImpl *ti = iface->type;
> -    Interface *iface_obj;
> +    /* If this type is an interface and num_interfaces, bugger out */
>  
> -    iface_obj = INTERFACE(object_new(ti->name));
> -    iface_obj->obj = obj;
> +    memset((void *)ti->class + class_size, 0, ti->class_size - class_size);
>  
> -    obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
> +    if (ti->class_init) {
> +        ti->class_init(ti->class, ti->class_data);
> +    }
>  }
>  
>  static void object_init_with_type(Object *obj, TypeImpl *ti)
>  {
> -    int i;
> -
>      if (type_has_parent(ti)) {
>          object_init_with_type(obj, type_get_parent(ti));
>      }
>  
> -    for (i = 0; i < ti->num_interfaces; i++) {
> -        object_interface_init(obj, &ti->interfaces[i]);
> -    }
> -
>      if (ti->instance_init) {
>          ti->instance_init(obj);
>      }
> @@ -338,12 +354,6 @@ static void object_deinit(Object *obj, TypeImpl *type)
>          type->instance_finalize(obj);
>      }
>  
> -    while (obj->interfaces) {
> -        Interface *iface_obj = obj->interfaces->data;
> -        obj->interfaces = g_slist_delete_link(obj->interfaces, obj->interfaces);
> -        object_delete(OBJECT(iface_obj));
> -    }
> -
>      if (type_has_parent(type)) {
>          object_deinit(obj, type_get_parent(type));
>      }
> @@ -390,22 +400,6 @@ void object_delete(Object *obj)
>      g_free(obj);
>  }
>  
> -static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
> -{
> -    assert(target_type);
> -
> -    /* Check if typename is a direct ancestor of type */
> -    while (type) {
> -        if (type == target_type) {
> -            return true;
> -        }
> -
> -        type = type_get_parent(type);
> -    }
> -
> -    return false;
> -}
> -
>  static bool object_is_type(Object *obj, TypeImpl *target_type)
>  {
>      return !target_type || type_is_ancestor(obj->class->type, target_type);
> @@ -414,7 +408,6 @@ static bool object_is_type(Object *obj, TypeImpl *target_type)
>  Object *object_dynamic_cast(Object *obj, const char *typename)
>  {
>      TypeImpl *target_type = type_get_by_name(typename);
> -    GSList *i;
>  
>      /* Check if typename is a direct ancestor.  Special-case TYPE_OBJECT,
>       * we want to go back from interfaces to the parent.
> @@ -423,46 +416,18 @@ Object *object_dynamic_cast(Object *obj, const char *typename)
>          return obj;
>      }
>  
> -    /* Check if obj is an interface and its containing object is a direct
> -     * ancestor of typename.  In principle we could do this test at the very
> -     * beginning of object_dynamic_cast, avoiding a second call to
> -     * object_is_type.  However, casting between interfaces is relatively
> -     * rare, and object_is_type(obj, type_interface) would fail almost always.
> -     *
> -     * Perhaps we could add a magic value to the object header for increased
> -     * (run-time) type safety and to speed up tests like this one.  If we ever
> -     * do that we can revisit the order here.
> -     */
> -    if (object_is_type(obj, type_interface)) {
> -        assert(!obj->interfaces);
> -        obj = INTERFACE(obj)->obj;
> -        if (object_is_type(obj, target_type)) {
> -            return obj;
> -        }
> -    }
> -
>      if (!target_type) {
>          return obj;
>      }
>  
> -    /* Check if obj has an interface of typename */
> -    for (i = obj->interfaces; i; i = i->next) {
> -        Interface *iface = i->data;
> -
> -        if (object_is_type(OBJECT(iface), target_type)) {
> -            return OBJECT(iface);
> -        }
> -    }
> -
>      return NULL;
>  }
>  
> -
>  static void register_types(void)
>  {
>      static TypeInfo interface_info = {
>          .name = TYPE_INTERFACE,
> -        .instance_size = sizeof(Interface),
> +        .class_size = sizeof(InterfaceClass),
>          .abstract = true,
>      };
>  
> @@ -491,16 +456,30 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>  {
>      TypeImpl *target_type = type_get_by_name(typename);
>      TypeImpl *type = class->type;
> +    ObjectClass *ret = NULL;
>  
> -    while (type) {
> -        if (type == target_type) {
> -            return class;
> +    if (type->num_interfaces && type_is_ancestor(target_type, type_interface)) {
> +        int found = 0;
> +        GSList *i;
> +
> +        for (i = class->interfaces; i; i = i->next) {
> +            ObjectClass *target_class = i->data;
> +
> +            if (type_is_ancestor(target_class->type, target_type)) {
> +                ret = target_class;
> +                found++;
> +            }
>          }
>  
> -        type = type_get_parent(type);
> +        /* The match was ambiguous, don't allow a cast */
> +        if (found > 1) {
> +            ret = NULL;
> +        }
> +    } else if (type_is_ancestor(type, target_type)) {
> +        ret = class;
>      }
>  
> -    return NULL;
> +    return ret;
>  }
>  
>  ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
> @@ -517,6 +496,28 @@ ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
>      return ret;
>  }
>  
> +Object *interface_dynamic_cast(Object *obj, const char *typename)
> +{
> +    if (object_class_dynamic_cast(object_get_class(obj), typename)) {
> +        return obj;
> +    }
> +
> +    return NULL;
> +}
> +
> +Object *interface_dynamic_cast_assert(Object *obj, const char *typename)
> +{
> +    Object *ret = interface_dynamic_cast(obj, typename);
> +
> +    if (!ret) {
> +        fprintf(stderr, "Object %p is not an instance of type %s\n",
> +                obj, typename);
> +        abort();
> +    }
> +
> +    return ret;
> +}
> +
>  const char *object_get_typename(Object *obj)
>  {
>      return obj->class->type->name;
> @@ -883,12 +884,6 @@ void object_property_add_child(Object *obj, const char *name,
>  {
>      gchar *type;
>  
> -    /* Registering an interface object in the composition tree will mightily
> -     * confuse object_get_canonical_path (which, on the other hand, knows how
> -     * to get the canonical path of an interface object).
> -     */
> -    assert(!object_is_type(obj, type_interface));
> -
>      type = g_strdup_printf("child<%s>", object_get_typename(OBJECT(child)));
>  
>      object_property_add(obj, name, type, object_get_child_property,
> @@ -985,10 +980,6 @@ gchar *object_get_canonical_path(Object *obj)
>      Object *root = object_get_root();
>      char *newpath = NULL, *path = NULL;
>  
> -    if (object_is_type(obj, type_interface)) {
> -        obj = INTERFACE(obj)->obj;
> -    }
> -
>      while (obj != root) {
>          ObjectProperty *prop = NULL;
>  

Re: [Qemu-devel] [PATCH 3/3] qom: add unit test for Interfaces

[Anthony Liguori]

On 06/16/2012 05:31 AM, Peter Crosthwaite wrote:
> On Thu, Jun 14, 2012 at 6:55 AM, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>
> Reviewed-by: Peter A.G. Crosthwaite<peter.crosthwaite@petalogix.com>
>
>> ---
>>   tests/Makefile      |    5 +-
>>   tests/test-object.c |  222 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 226 insertions(+), 1 deletions(-)
>>   create mode 100644 tests/test-object.c
>>
>> diff --git a/tests/Makefile b/tests/Makefile
>> index d66ab19..d1f979d 100644
>> --- a/tests/Makefile
>> +++ b/tests/Makefile
>> @@ -14,6 +14,7 @@ check-unit-y += tests/test-string-input-visitor$(EXESUF)
>>   check-unit-y += tests/test-string-output-visitor$(EXESUF)
>>   check-unit-y += tests/test-coroutine$(EXESUF)
>>   check-unit-y += tests/test-visitor-serialization$(EXESUF)
>> +check-unit-y += tests/test-object$(EXESUF)
>>
>>   check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
>>
>> @@ -32,7 +33,8 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
>>         tests/test-coroutine.o tests/test-string-output-visitor.o \
>>         tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
>>         tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
>> -       tests/test-qmp-commands.o tests/test-visitor-serialization.o
>> +       tests/test-qmp-commands.o tests/test-visitor-serialization.o \
>> +       tests/test-object.o
>>
>>   test-qapi-obj-y =  $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y)
>>   test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
>> @@ -66,6 +68,7 @@ tests/test-qmp-input-visitor$(EXESUF): tests/test-qmp-input-visitor.o $(test-qap
>>   tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o $(test-qapi-obj-y)
>>   tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o tests/test-qmp-marshal.o $(test-qapi-obj-y)
>>   tests/test-visitor-serialization$(EXESUF): tests/test-visitor-serialization.o $(test-qapi-obj-y)
>> +tests/test-object$(EXESUF): tests/test-object.o $(qom-obj-y) $(test-qapi-obj-y)
>>
>>   tests/rtc-test$(EXESUF): tests/rtc-test.o $(trace-obj-y)
>>   tests/m48t59-test$(EXESUF): tests/m48t59-test.o $(trace-obj-y)
>> diff --git a/tests/test-object.c b/tests/test-object.c
>> new file mode 100644
>> index 0000000..9f41da0
>> --- /dev/null
>> +++ b/tests/test-object.c
>> @@ -0,0 +1,222 @@
>> +/*
>> + * QEMU Object Model unit test
>> + *
>> + * Copyright IBM, Corp. 2012
>> + *
>> + * Authors:
>> + *  Anthony Liguori<aliguori@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +#include "qemu/object.h"
>> +#include "module.h"
>> +
>> +#define TYPE_HERBIVORE "herbivore"
>> +
>> +#define HERBIVORE_CLASS(klass) \
>> +    OBJECT_CLASS_CHECK(HerbivoreClass, (klass), TYPE_HERBIVORE)
>> +#define HERBIVORE_GET_CLASS(obj) \
>> +    OBJECT_GET_CLASS(HerbivoreClass, (obj), TYPE_HERBIVORE)
>> +#define HERBIVORE(obj) \
>> +    INTERFACE_CHECK(Herbivore, (obj), TYPE_HERBIVORE)
>> +
>> +typedef struct Herbivore
>> +{
>> +    Object obj;
>> +} Herbivore;
>
> All this is doing is saying Herbivores are Objects right? A user cant
> add anything to this struct given that interfaces are stateless so
> could this be simplified to
>
> typedef Object Herbivore;

This is admittedly a little wierd...

Interfaces don't exist as Objects in QOM.  They are just classes.  But it's very 
handy to be able to have a Herbivore type that you can cast objects to.

I probably need to respin this though.  INTERFACE_CHECK() asserts that an object 
implements the interface class and then just returns the obj and casts it to the 
dummy Interface object type.

A better approach would be to just teach object_dynamic_cast to do this for 
anythign that's an interface type.  That would fix the link problem you pointed 
out too.  I'll spin a v2.

Regards,

Anthony Liguori

>
> ?
>
>> +
>> +typedef struct HerbivoreClass
>> +{
>> +    InterfaceClass parent;
>> +
>> +    void (*feed_greens)(Herbivore *obj);
>> +} HerbivoreClass;
>> +
>> +static void herbivore_feed_greens(Herbivore *herbie)
>> +{
>> +    HerbivoreClass *k = HERBIVORE_GET_CLASS(herbie);
>> +
>> +    k->feed_greens(herbie);
>> +}
>> +
>> +static TypeInfo herbivore_info = {
>> +    .name = TYPE_HERBIVORE,
>> +    .parent = TYPE_INTERFACE,
>> +    .class_size = sizeof(HerbivoreClass),
>> +};
>> +
>> +#define TYPE_CARNIVORE "carnivore"
>> +#define CARNIVORE_CLASS(klass) \
>> +    OBJECT_CLASS_CHECK(CarnivoreClass, (klass), TYPE_CARNIVORE)
>> +#define CARNIVORE_GET_CLASS(obj) \
>> +    OBJECT_GET_CLASS(CarnivoreClass, (obj), TYPE_CARNIVORE)
>> +#define CARNIVORE(obj) \
>> +    INTERFACE_CHECK(Carnivore, (obj), TYPE_CARNIVORE)
>> +
>> +typedef struct Carnivore
>> +{
>> +    Object parent;
>> +} Carnivore;
>> +
>> +typedef struct CarnivoreClass
>> +{
>> +    InterfaceClass parent;
>> +
>> +    void (*feed_bugs)(Carnivore *obj);
>> +} CarnivoreClass;
>> +
>> +static void carnivore_feed_bugs(Carnivore *carnie)
>> +{
>> +    CarnivoreClass *k = CARNIVORE_GET_CLASS(carnie);
>> +
>> +    k->feed_bugs(carnie);
>> +}
>> +
>> +static TypeInfo carnivore_info = {
>> +    .name = TYPE_CARNIVORE,
>> +    .parent = TYPE_INTERFACE,
>> +    .class_size = sizeof(CarnivoreClass),
>> +};
>> +
>> +#define TYPE_REPTILE "reptile"
>> +#define REPTILE(obj) OBJECT_CHECK(Reptile, (obj), TYPE_REPTILE)
>> +
>> +typedef struct Reptile
>> +{
>> +    Object parent;
>> +} Reptile;
>> +
>> +static TypeInfo reptile_info = {
>> +    .name = TYPE_REPTILE,
>> +    .instance_size = sizeof(Reptile),
>> +    .abstract = true,
>> +    .class_size = sizeof(ObjectClass),
>> +};
>> +
>> +#define TYPE_LIZARD "lizard"
>> +#define LIZARD(obj) OBJECT_CHECK(Lizard, (obj), TYPE_LIZARD)
>> +
>> +typedef struct Lizard
>> +{
>> +    Reptile parent;
>> +} Lizard;
>> +
>> +static TypeInfo lizard_info = {
>> +    .name = TYPE_LIZARD,
>> +    .parent = TYPE_REPTILE,
>> +    .instance_size = sizeof(Lizard),
>> +    .abstract = true,
>> +};
>> +
>> +#define TYPE_IGUANA "iguana"
>> +#define IGUANA(obj) OBJECT_CHECK(Iguana, (obj), TYPE_IGUANA)
>> +
>> +typedef struct Iguana
>> +{
>> +    Lizard parent;
>> +    int greens;
>> +} Iguana;
>> +
>> +static void iguana_feed(Herbivore *herbie)
>> +{
>> +    Iguana *iggie = IGUANA(herbie);
>> +
>> +    iggie->greens++;
>> +}
>> +
>> +static void iguana_class_initfn(ObjectClass *klass, void *data)
>> +{
>> +    HerbivoreClass *iface = HERBIVORE_CLASS(klass);
>> +
>> +    iface->feed_greens = iguana_feed;
>> +}
>> +
>> +static TypeInfo iguana_info = {
>> +    .name = TYPE_IGUANA,
>> +    .parent = TYPE_LIZARD,
>> +    .instance_size = sizeof(Iguana),
>> +    .class_init = iguana_class_initfn,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { TYPE_HERBIVORE },
>> +        { }
>> +    },
>> +};
>> +
>> +#define TYPE_BEARDED_DRAGON "bearded-dragon"
>> +#define BEARDED_DRAGON(obj) OBJECT_CHECK(BeardedDragon, (obj), TYPE_BEARDED_DRAGON)
>> +
>> +typedef struct BeardedDragon
>> +{
>> +    Lizard parent;
>> +    int bugs;
>> +} BeardedDragon;
>> +
>> +static void bearded_dragon_feed(Carnivore *carnie)
>> +{
>> +    BeardedDragon *dragon = BEARDED_DRAGON(carnie);
>> +
>> +    dragon->bugs++;
>> +}
>> +
>> +static void bearded_dragon_class_initfn(ObjectClass *klass, void *data)
>> +{
>> +    CarnivoreClass *iface = CARNIVORE_CLASS(klass);
>> +
>> +    iface->feed_bugs = bearded_dragon_feed;
>> +}
>> +
>> +static TypeInfo bearded_dragon_info = {
>> +    .name = TYPE_BEARDED_DRAGON,
>> +    .parent = TYPE_LIZARD,
>> +    .instance_size = sizeof(BeardedDragon),
>> +    .class_init = bearded_dragon_class_initfn,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { TYPE_CARNIVORE },
>> +        { }
>> +    },
>> +};
>> +
>> +static void basic_init(void)
>> +{
>> +    BeardedDragon dragon;
>> +    Iguana iguana;
>> +
>> +    object_initialize(&dragon, TYPE_BEARDED_DRAGON);
>> +
>> +    g_assert_cmpint(dragon.bugs, ==, 0);
>> +    carnivore_feed_bugs(CARNIVORE(&dragon));
>> +    g_assert_cmpint(dragon.bugs, ==, 1);
>> +
>> +    object_finalize(&dragon);
>> +
>> +    object_initialize(&iguana, TYPE_IGUANA);
>> +
>> +    g_assert_cmpint(iguana.greens, ==, 0);
>> +    herbivore_feed_greens(HERBIVORE(&iguana));
>> +    g_assert_cmpint(iguana.greens, ==, 1);
>> +
>> +    object_finalize(&iguana);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    g_test_init(&argc,&argv, NULL);
>> +
>> +    module_call_init(MODULE_INIT_QOM);
>> +
>> +    type_register_static(&carnivore_info);
>> +    type_register_static(&herbivore_info);
>> +
>> +    type_register_static(&reptile_info);
>> +    type_register_static(&lizard_info);
>> +    type_register_static(&iguana_info);
>> +    type_register_static(&bearded_dragon_info);
>> +
>> +    g_test_add_func("/basic/init", basic_init);
>> +
>> +    g_test_run();
>> +
>> +    return 0;
>> +}
>> --
>> 1.7.5.4
>>
>

Re: [Qemu-devel] [PATCH 3/3] qom: add unit test for Interfaces

[Peter Crosthwaite]

>>> +#define HERBIVORE(obj) \
>>> +    INTERFACE_CHECK(Herbivore, (obj), TYPE_HERBIVORE)
>>> +
>>> +typedef struct Herbivore
>>> +{
>>> +    Object obj;
>>> +} Herbivore;
>>
>>
>> All this is doing is saying Herbivores are Objects right? A user cant
>> add anything to this struct given that interfaces are stateless so
>> could this be simplified to
>>
>> typedef Object Herbivore;
>
>
> This is admittedly a little wierd...
>
> Interfaces don't exist as Objects in QOM.

Not in the sense that they used too, but all objects that implement an
interface are still OBJECTs just through the inheritance path of their
concrete class, which is the point of this struct yes?

 They are just classes.  But it's
> very handy to be able to have a Herbivore type that you can cast objects to.
>

Yes I agree in full, but the typedef definition is functionally
equivalent to what you have there, and removes the temptation to add
fields to the object type. Having a skeletal struct there gives the
illusion to readers that interface objects are in someway extensible.

> I probably need to respin this though.  INTERFACE_CHECK() asserts that an
> object implements the interface class and then just returns the obj and
> casts it to the dummy Interface object type.
>
> A better approach would be to just teach object_dynamic_cast to do this for
> anythign that's an interface type.  That would fix the link problem you
> pointed out too.  I'll spin a v2.

Please see my patch, I have a little 3 patch series up on the list.
that starts with your v1, does the axi-stream stuff then fixes that
bug.

Regards,
Peter

>
> Regards,
>
> Anthony Liguori
>
>>
>> ?
>>
>>> +
>>> +typedef struct HerbivoreClass
>>> +{
>>> +    InterfaceClass parent;
>>> +
>>> +    void (*feed_greens)(Herbivore *obj);

Re: [Qemu-devel] [PATCH 3/3] qom: add unit test for Interfaces

[Andreas Färber]

Am 18.06.2012 15:46, schrieb Peter Crosthwaite:
>>>> +#define HERBIVORE(obj) \
>>>> +    INTERFACE_CHECK(Herbivore, (obj), TYPE_HERBIVORE)
>>>> +
>>>> +typedef struct Herbivore
>>>> +{
>>>> +    Object obj;
>>>> +} Herbivore;
>>>
>>>
>>> All this is doing is saying Herbivores are Objects right? A user cant
>>> add anything to this struct given that interfaces are stateless so
>>> could this be simplified to
>>>
>>> typedef Object Herbivore;
>>
>>
>> This is admittedly a little wierd...
>>
>> Interfaces don't exist as Objects in QOM.
> 
> Not in the sense that they used too, but all objects that implement an
> interface are still OBJECTs just through the inheritance path of their
> concrete class, which is the point of this struct yes?
> 
>  They are just classes.  But it's
>> very handy to be able to have a Herbivore type that you can cast objects to.
>>
> 
> Yes I agree in full, but the typedef definition is functionally
> equivalent to what you have there, and removes the temptation to add
> fields to the object type. Having a skeletal struct there gives the
> illusion to readers that interface objects are in someway extensible.

Why have a typedef at all then? You can just use Object directly. If
that ever changes it leads to all kinds of problems (well, necessary
adjustments), as seen in the pci_host series.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 3/3] qom: add unit test for Interfaces

[Anthony Liguori]

On 06/18/2012 08:51 AM, Andreas Färber wrote:
> Am 18.06.2012 15:46, schrieb Peter Crosthwaite:
>>>>> +#define HERBIVORE(obj) \
>>>>> +    INTERFACE_CHECK(Herbivore, (obj), TYPE_HERBIVORE)
>>>>> +
>>>>> +typedef struct Herbivore
>>>>> +{
>>>>> +    Object obj;
>>>>> +} Herbivore;
>>>>
>>>>
>>>> All this is doing is saying Herbivores are Objects right? A user cant
>>>> add anything to this struct given that interfaces are stateless so
>>>> could this be simplified to
>>>>
>>>> typedef Object Herbivore;
>>>
>>>
>>> This is admittedly a little wierd...
>>>
>>> Interfaces don't exist as Objects in QOM.
>>
>> Not in the sense that they used too, but all objects that implement an
>> interface are still OBJECTs just through the inheritance path of their
>> concrete class, which is the point of this struct yes?

Yes, they are objects, which is why the struct just contains Object.  It's 
basically just a type-safe wrapper for Object.  Nothing more than that.

>>
>>   They are just classes.  But it's
>>> very handy to be able to have a Herbivore type that you can cast objects to.
>>>
>>
>> Yes I agree in full, but the typedef definition is functionally
>> equivalent to what you have there, and removes the temptation to add
>> fields to the object type. Having a skeletal struct there gives the
>> illusion to readers that interface objects are in someway extensible.

typedef struct Object Herbivore;

is a weak alias.  IOW, it's not an error to pass Object * instead of Herbivore 
*.  Worse yet, passing Carnivore * also is not an error.

The struct definition provides a strict alias.  We could wrap it in a macro to 
avoid confusing the reader I guess.

> Why have a typedef at all then? You can just use Object directly. If
> that ever changes it leads to all kinds of problems (well, necessary
> adjustments), as seen in the pci_host series.

Type safety.

Regards,

Anthony Liguori



>
> Andreas
>

Re: [Qemu-devel] [PATCH 2/3] qom: reimplement Interfaces

[Peter Crosthwaite]

Hi Anthony,

With the latest qom-next merge, this fails to rebase with non-trivial
conflicts. Do you have a rebased version of this floating around in
one of your trees somewhere? We are trying to get our machine models
as QOMified as we can, especially the axi-stream stuff. I will also be
able to put some --tested-by to this stuff with the microblaze machine
model. Let me know if/how I can help.

Regards,
Peter

On Sat, Jun 16, 2012 at 8:47 PM, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> On Wed, 2012-06-13 at 15:55 -0500, Anthony Liguori wrote:
>> The current implementation of Interfaces is poorly designed.  Each interface
>> that an object implements end up being an object that's tracked by the
>
> "ends"
>
>> implementing object.  There's all sorts of gymnastics to deal with casting
>> between these objects.
>>
>> By an interface shouldn't be associated with an Object.  Interfaces are global
>
> "But" ?
>
>> to a class.  This patch moves all Interface knowledge to ObjectClass eliminating
>> the relationship between Object and Interfaces.
>>
>> Interfaces are now abstract (as they should be) but this is okay.  Interfaces
>> essentially act as additional parents for the classes and are treated as such.
>>
>> With this new implementation, we should fully support derived interfaces
>> including reimplementing an inherited interface.
>>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  include/qemu/object.h |   64 +++++++++++---
>>  qom/object.c          |  231 +++++++++++++++++++++++-------------------------
>>  2 files changed, 160 insertions(+), 135 deletions(-)
>>
>> diff --git a/include/qemu/object.h b/include/qemu/object.h
>> index d93b772..72cb290 100644
>> --- a/include/qemu/object.h
>> +++ b/include/qemu/object.h
>> @@ -239,6 +239,7 @@ struct ObjectClass
>>  {
>>      /*< private >*/
>>      Type type;
>> +    GSList *interfaces;
>>  };
>>
>>  /**
>> @@ -260,7 +261,6 @@ struct Object
>>  {
>>      /*< private >*/
>>      ObjectClass *class;
>> -    GSList *interfaces;
>>      QTAILQ_HEAD(, ObjectProperty) properties;
>>      uint32_t ref;
>>      Object *parent;
>> @@ -381,6 +381,17 @@ struct TypeInfo
>>      OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name)
>>
>>  /**
>> + * InterfaceInfo:
>> + * @type: The name of the interface.
>> + *
>> + * The information associated with an interface.
>> + */
>> +struct InterfaceInfo
>> +{
>> +    const char *type;
>> +};
>> +
>> +/**
>>   * InterfaceClass:
>>   * @parent_class: the base class
>>   *
>> @@ -390,26 +401,31 @@ struct TypeInfo
>>  struct InterfaceClass
>>  {
>>      ObjectClass parent_class;
>> +    /*< private >*/
>> +    ObjectClass *concrete_class;
>>  };
>>
>> +#define TYPE_INTERFACE "interface"
>> +
>>  /**
>> - * InterfaceInfo:
>> - * @type: The name of the interface.
>> - * @interface_initfn: This method is called during class initialization and is
>> - *   used to initialize an interface associated with a class.  This function
>> - *   should initialize any default virtual functions for a class and/or override
>> - *   virtual functions in a parent class.
>> + * INTERFACE_CLASS:
>> + * @klass: class to cast from
>>   *
>> - * The information associated with an interface.
>> + * Returns: An #InterfaceClass or raise an error if cast is invalid
>>   */
>> -struct InterfaceInfo
>> -{
>> -    const char *type;
>> +#define INTERFACE_CLASS(klass) \
>> +    OBJECT_CLASS_CHECK(InterfaceClass, klass, TYPE_INTERFACE)
>>
>> -    void (*interface_initfn)(ObjectClass *class, void *data);
>> -};
>> -
>> -#define TYPE_INTERFACE "interface"
>> +/**
>> + * INTERFACE_CHECK:
>> + * @interface: the type to return
>> + * @obj: the object to convert to an interface
>> + * @name: the interface type name
>> + *
>> + * Returns: @obj casted to @interface if cast is valid, otherwise raise error.
>> + */
>> +#define INTERFACE_CHECK(interface, obj, name) \
>> +    ((interface *)interface_dynamic_cast_assert(OBJECT((obj)), (name)))
>>
>>  /**
>>   * object_new:
>> @@ -548,6 +564,24 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *klass,
>>                                         const char *typename);
>>
>>  /**
>> + * interface_dynamic_cast_assert:
>> + * @obj: the object to cast to an interface type
>> + * @typename: the type name of the interface
>> + *
>> + * Returns: @obj if @obj implements @typename, otherwise raise an error
>> + */
>> +Object *interface_dynamic_cast_assert(Object *obj, const char *typename);
>> +
>> +/**
>> + * interface_dynamic_cast_assert:
>> + * @obj: the object to cast to an interface type
>> + * @typename: the type name of the interface
>> + *
>> + * Returns: @obj if @obj implements @typename, otherwise %NULL
>> + */
>> +Object *interface_dynamic_cast(Object *obj, const char *typename);
>> +
>
> This is where bug was introduced for links. The link setter code uses
> object_dynamic_cast() which shortcuts the logic here, that is needed for
> casting interfaces. The new result is you can use interface with links
> cos the cast pukes.
>
> I have proposed a solution to this in my new revision (P3) of the
> axi-stream series.
>
> Regards,
> Peter
>
>> +/**
>>   * object_class_get_name:
>>   * @klass: The class to obtain the QOM typename for.
>>   *
>> diff --git a/qom/object.c b/qom/object.c
>> index 6f839ad..aa26693 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -31,9 +31,7 @@ typedef struct TypeImpl TypeImpl;
>>
>>  struct InterfaceImpl
>>  {
>> -    const char *parent;
>> -    void (*interface_initfn)(ObjectClass *class, void *data);
>> -    TypeImpl *type;
>> +    const char *typename;
>>  };
>>
>>  struct TypeImpl
>> @@ -63,14 +61,6 @@ struct TypeImpl
>>      InterfaceImpl interfaces[MAX_INTERFACES];
>>  };
>>
>> -typedef struct Interface
>> -{
>> -    Object parent;
>> -    Object *obj;
>> -} Interface;
>> -
>> -#define INTERFACE(obj) OBJECT_CHECK(Interface, obj, TYPE_INTERFACE)
>> -
>>  static Type type_interface;
>>
>>  static GHashTable *type_table_get(void)
>> @@ -97,6 +87,7 @@ static TypeImpl *type_table_lookup(const char *name)
>>  TypeImpl *type_register(const TypeInfo *info)
>>  {
>>      TypeImpl *ti = g_malloc0(sizeof(*ti));
>> +    int i;
>>
>>      g_assert(info->name != NULL);
>>
>> @@ -120,15 +111,10 @@ TypeImpl *type_register(const TypeInfo *info)
>>
>>      ti->abstract = info->abstract;
>>
>> -    if (info->interfaces) {
>> -        int i;
>> -
>> -        for (i = 0; info->interfaces[i].type; i++) {
>> -            ti->interfaces[i].parent = info->interfaces[i].type;
>> -            ti->interfaces[i].interface_initfn = info->interfaces[i].interface_initfn;
>> -            ti->num_interfaces++;
>> -        }
>> +    for (i = 0; info->interfaces && info->interfaces[i].type; i++) {
>> +        ti->interfaces[i].typename = g_strdup(info->interfaces[i].type);
>>      }
>> +    ti->num_interfaces = i;
>>
>>      type_table_add(ti);
>>
>> @@ -190,26 +176,48 @@ static size_t type_object_get_size(TypeImpl *ti)
>>      return 0;
>>  }
>>
>> -static void type_class_interface_init(TypeImpl *ti, InterfaceImpl *iface)
>> +static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
>>  {
>> -    TypeInfo info = {
>> -        .instance_size = sizeof(Interface),
>> -        .parent = iface->parent,
>> -        .class_size = sizeof(InterfaceClass),
>> -        .class_init = iface->interface_initfn,
>> -        .abstract = true,
>> -    };
>> -    char *name = g_strdup_printf("<%s::%s>", ti->name, iface->parent);
>> +    assert(target_type);
>> +
>> +    /* Check if typename is a direct ancestor of type */
>> +    while (type) {
>> +        if (type == target_type) {
>> +            return true;
>> +        }
>> +
>> +        type = type_get_parent(type);
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static void type_initialize(TypeImpl *ti);
>> +
>> +static void type_initialize_interface(TypeImpl *ti, const char *parent)
>> +{
>> +    InterfaceClass *new_iface;
>> +    TypeInfo info = { };
>> +    TypeImpl *iface_impl;
>>
>> -    info.name = name;
>> -    iface->type = type_register(&info);
>> -    g_free(name);
>> +    info.parent = parent;
>> +    info.name = g_strdup_printf("%s::%s", ti->name, info.parent);
>> +    info.abstract = true;
>> +
>> +    iface_impl = type_register(&info);
>> +    type_initialize(iface_impl);
>> +    g_free((char *)info.name);
>> +
>> +    new_iface = (InterfaceClass *)iface_impl->class;
>> +    new_iface->concrete_class = ti->class;
>> +
>> +    ti->class->interfaces = g_slist_append(ti->class->interfaces,
>> +                                           iface_impl->class);
>>  }
>>
>>  static void type_initialize(TypeImpl *ti)
>>  {
>>      size_t class_size = sizeof(ObjectClass);
>> -    int i;
>>
>>      if (ti->class) {
>>          return;
>> @@ -221,8 +229,12 @@ static void type_initialize(TypeImpl *ti)
>>      ti->class = g_malloc0(ti->class_size);
>>      ti->class->type = ti;
>>
>> +    ti->class->interfaces = NULL;
>> +
>>      if (type_has_parent(ti)) {
>>          TypeImpl *parent = type_get_parent(ti);
>> +        GSList *e;
>> +        int i;
>>
>>          type_initialize(parent);
>>
>> @@ -232,42 +244,46 @@ static void type_initialize(TypeImpl *ti)
>>          memcpy((void *)ti->class + sizeof(ObjectClass),
>>                 (void *)parent->class + sizeof(ObjectClass),
>>                 parent->class_size - sizeof(ObjectClass));
>> -    }
>>
>> -    memset((void *)ti->class + class_size, 0, ti->class_size - class_size);
>> +        for (e = parent->class->interfaces; e; e = e->next) {
>> +            ObjectClass *iface = e->data;
>> +            type_initialize_interface(ti, object_class_get_name(iface));
>> +        }
>>
>> -    for (i = 0; i < ti->num_interfaces; i++) {
>> -        type_class_interface_init(ti, &ti->interfaces[i]);
>> -    }
>> +        for (i = 0; i < ti->num_interfaces; i++) {
>> +            TypeImpl *t = type_get_by_name(ti->interfaces[i].typename);
>>
>> -    if (ti->class_init) {
>> -        ti->class_init(ti->class, ti->class_data);
>> +            for (e = ti->class->interfaces; e; e = e->next) {
>> +                TypeImpl *target_type = OBJECT_CLASS(e->data)->type;
>> +
>> +                if (type_is_ancestor(target_type, t)) {
>> +                    break;
>> +                }
>> +            }
>> +
>> +            if (e) {
>> +                continue;
>> +            }
>> +
>> +            type_initialize_interface(ti, ti->interfaces[i].typename);
>> +        }
>>      }
>> -}
>>
>> -static void object_interface_init(Object *obj, InterfaceImpl *iface)
>> -{
>> -    TypeImpl *ti = iface->type;
>> -    Interface *iface_obj;
>> +    /* If this type is an interface and num_interfaces, bugger out */
>>
>> -    iface_obj = INTERFACE(object_new(ti->name));
>> -    iface_obj->obj = obj;
>> +    memset((void *)ti->class + class_size, 0, ti->class_size - class_size);
>>
>> -    obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
>> +    if (ti->class_init) {
>> +        ti->class_init(ti->class, ti->class_data);
>> +    }
>>  }
>>
>>  static void object_init_with_type(Object *obj, TypeImpl *ti)
>>  {
>> -    int i;
>> -
>>      if (type_has_parent(ti)) {
>>          object_init_with_type(obj, type_get_parent(ti));
>>      }
>>
>> -    for (i = 0; i < ti->num_interfaces; i++) {
>> -        object_interface_init(obj, &ti->interfaces[i]);
>> -    }
>> -
>>      if (ti->instance_init) {
>>          ti->instance_init(obj);
>>      }
>> @@ -338,12 +354,6 @@ static void object_deinit(Object *obj, TypeImpl *type)
>>          type->instance_finalize(obj);
>>      }
>>
>> -    while (obj->interfaces) {
>> -        Interface *iface_obj = obj->interfaces->data;
>> -        obj->interfaces = g_slist_delete_link(obj->interfaces, obj->interfaces);
>> -        object_delete(OBJECT(iface_obj));
>> -    }
>> -
>>      if (type_has_parent(type)) {
>>          object_deinit(obj, type_get_parent(type));
>>      }
>> @@ -390,22 +400,6 @@ void object_delete(Object *obj)
>>      g_free(obj);
>>  }
>>
>> -static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
>> -{
>> -    assert(target_type);
>> -
>> -    /* Check if typename is a direct ancestor of type */
>> -    while (type) {
>> -        if (type == target_type) {
>> -            return true;
>> -        }
>> -
>> -        type = type_get_parent(type);
>> -    }
>> -
>> -    return false;
>> -}
>> -
>>  static bool object_is_type(Object *obj, TypeImpl *target_type)
>>  {
>>      return !target_type || type_is_ancestor(obj->class->type, target_type);
>> @@ -414,7 +408,6 @@ static bool object_is_type(Object *obj, TypeImpl *target_type)
>>  Object *object_dynamic_cast(Object *obj, const char *typename)
>>  {
>>      TypeImpl *target_type = type_get_by_name(typename);
>> -    GSList *i;
>>
>>      /* Check if typename is a direct ancestor.  Special-case TYPE_OBJECT,
>>       * we want to go back from interfaces to the parent.
>> @@ -423,46 +416,18 @@ Object *object_dynamic_cast(Object *obj, const char *typename)
>>          return obj;
>>      }
>>
>> -    /* Check if obj is an interface and its containing object is a direct
>> -     * ancestor of typename.  In principle we could do this test at the very
>> -     * beginning of object_dynamic_cast, avoiding a second call to
>> -     * object_is_type.  However, casting between interfaces is relatively
>> -     * rare, and object_is_type(obj, type_interface) would fail almost always.
>> -     *
>> -     * Perhaps we could add a magic value to the object header for increased
>> -     * (run-time) type safety and to speed up tests like this one.  If we ever
>> -     * do that we can revisit the order here.
>> -     */
>> -    if (object_is_type(obj, type_interface)) {
>> -        assert(!obj->interfaces);
>> -        obj = INTERFACE(obj)->obj;
>> -        if (object_is_type(obj, target_type)) {
>> -            return obj;
>> -        }
>> -    }
>> -
>>      if (!target_type) {
>>          return obj;
>>      }
>>
>> -    /* Check if obj has an interface of typename */
>> -    for (i = obj->interfaces; i; i = i->next) {
>> -        Interface *iface = i->data;
>> -
>> -        if (object_is_type(OBJECT(iface), target_type)) {
>> -            return OBJECT(iface);
>> -        }
>> -    }
>> -
>>      return NULL;
>>  }
>>
>> -
>>  static void register_types(void)
>>  {
>>      static TypeInfo interface_info = {
>>          .name = TYPE_INTERFACE,
>> -        .instance_size = sizeof(Interface),
>> +        .class_size = sizeof(InterfaceClass),
>>          .abstract = true,
>>      };
>>
>> @@ -491,16 +456,30 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>>  {
>>      TypeImpl *target_type = type_get_by_name(typename);
>>      TypeImpl *type = class->type;
>> +    ObjectClass *ret = NULL;
>>
>> -    while (type) {
>> -        if (type == target_type) {
>> -            return class;
>> +    if (type->num_interfaces && type_is_ancestor(target_type, type_interface)) {
>> +        int found = 0;
>> +        GSList *i;
>> +
>> +        for (i = class->interfaces; i; i = i->next) {
>> +            ObjectClass *target_class = i->data;
>> +
>> +            if (type_is_ancestor(target_class->type, target_type)) {
>> +                ret = target_class;
>> +                found++;
>> +            }
>>          }
>>
>> -        type = type_get_parent(type);
>> +        /* The match was ambiguous, don't allow a cast */
>> +        if (found > 1) {
>> +            ret = NULL;
>> +        }
>> +    } else if (type_is_ancestor(type, target_type)) {
>> +        ret = class;
>>      }
>>
>> -    return NULL;
>> +    return ret;
>>  }
>>
>>  ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
>> @@ -517,6 +496,28 @@ ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
>>      return ret;
>>  }
>>
>> +Object *interface_dynamic_cast(Object *obj, const char *typename)
>> +{
>> +    if (object_class_dynamic_cast(object_get_class(obj), typename)) {
>> +        return obj;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +Object *interface_dynamic_cast_assert(Object *obj, const char *typename)
>> +{
>> +    Object *ret = interface_dynamic_cast(obj, typename);
>> +
>> +    if (!ret) {
>> +        fprintf(stderr, "Object %p is not an instance of type %s\n",
>> +                obj, typename);
>> +        abort();
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>  const char *object_get_typename(Object *obj)
>>  {
>>      return obj->class->type->name;
>> @@ -883,12 +884,6 @@ void object_property_add_child(Object *obj, const char *name,
>>  {
>>      gchar *type;
>>
>> -    /* Registering an interface object in the composition tree will mightily
>> -     * confuse object_get_canonical_path (which, on the other hand, knows how
>> -     * to get the canonical path of an interface object).
>> -     */
>> -    assert(!object_is_type(obj, type_interface));
>> -
>>      type = g_strdup_printf("child<%s>", object_get_typename(OBJECT(child)));
>>
>>      object_property_add(obj, name, type, object_get_child_property,
>> @@ -985,10 +980,6 @@ gchar *object_get_canonical_path(Object *obj)
>>      Object *root = object_get_root();
>>      char *newpath = NULL, *path = NULL;
>>
>> -    if (object_is_type(obj, type_interface)) {
>> -        obj = INTERFACE(obj)->obj;
>> -    }
>> -
>>      while (obj != root) {
>>          ObjectProperty *prop = NULL;
>>
>
>