Re: [Qemu-devel] [RFC v0 6/8] xilinx dont cast to interface types with links

[Paolo Bonzini <pbonzini@redhat.com>]

Il 13/06/2012 11:38, Peter A. G. Crosthwaite ha scritto:
> Something is broken with casting to an interface type then setting a link. Cast
> these two to object for linking instead and it all works.

I don't have a board image but I don't see anything visibly bad without this patch.

However, something that _is_ bad indeed happens; we try to ref/unref an interface
object.  This patch fixes it while keeping things efficient (and in fact fixes one
TODO).  Can add a link to an image on http://wiki.qemu.org/Testing and/or test it?

Anthony, is this above your disgust level?

Paolo


------------------------ 8< -----------------------

diff --git a/qom/object.c b/qom/object.c
index c3a7a47..bd60838 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -63,7 +63,7 @@ struct TypeImpl
     InterfaceImpl interfaces[MAX_INTERFACES];
 };
 
-#define INTERFACE(obj) INTERFACE_CHECK(obj, TYPE_INTERFACE)
+#define INTERFACE(obj) ((Interface *)(obj))
 
 static Type type_interface;
 
@@ -239,13 +239,21 @@ static void type_initialize(TypeImpl *ti)
     }
 }
 
+#define INTERFACE_MAGIC    ((GSList *) (intptr_t)0xBAD0BAD)
+
+static inline bool object_is_interface(Object *obj) {
+    return obj->interfaces == INTERFACE_MAGIC;
+}
+
 static void object_interface_init(Object *obj, InterfaceImpl *iface)
 {
     TypeImpl *ti = iface->type;
     Interface *iface_obj;
 
+    assert(!object_is_interface(obj));
     iface_obj = INTERFACE(object_new(ti->name));
     iface_obj->iface_obj = obj;
+    iface_obj->iface_parent.interfaces = INTERFACE_MAGIC;
 
     obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
 }
@@ -332,10 +340,12 @@ 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 (!object_is_interface(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)) {
@@ -410,6 +420,13 @@ Object *object_dynamic_cast(Object *obj, const char *typename)
     TypeImpl *target_type = type_get_by_name(typename);
     GSList *i;
 
+    /* Check if obj is an interface and its containing object is a direct
+     * ancestor of typename.  object_is_interface is a very fast test.
+     */
+    if (object_is_interface(obj)) {
+        obj = INTERFACE(obj)->iface_obj;
+    }
+
     /* Check if typename is a direct ancestor.  Special-case TYPE_OBJECT,
      * we want to go back from interfaces to the parent.
     */
@@ -417,24 +434,6 @@ 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)->iface_obj;
-        if (object_is_type(obj, target_type)) {
-            return obj;
-        }
-    }
-
     if (!target_type) {
         return obj;
     }
@@ -597,11 +596,19 @@ GSList *object_class_get_list(const char *implements_type,
 
 void object_ref(Object *obj)
 {
+    if (object_is_interface(obj)) {
+        obj = INTERFACE(obj)->iface_obj;
+    }
+
     obj->ref++;
 }
 
 void object_unref(Object *obj)
 {
+    if (object_is_interface(obj)) {
+        obj = INTERFACE(obj)->iface_obj;
+    }
+
     g_assert(obj->ref > 0);
     obj->ref--;
 
@@ -979,7 +986,7 @@ gchar *object_get_canonical_path(Object *obj)
     Object *root = object_get_root();
     char *newpath = NULL, *path = NULL;
 
-    if (object_is_type(obj, type_interface)) {
+    if (object_is_interface(obj)) {
         obj = INTERFACE(obj)->iface_obj;
     }
 

Paolo