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

[Anthony Liguori]

On 06/13/2012 05:41 AM, Paolo Bonzini wrote:
> 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?

Isn't there a patch floating around to make Object a proper TypeInfo?  In which 
case, couldn't we make Interface not inherit from Object and change the OBJECT() 
cast macro to tell the difference?

Regards,

Anthony Liguori

> 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