From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56288) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Seo2o-0002Bc-8n for qemu-devel@nongnu.org; Wed, 13 Jun 2012 09:55:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Seo2i-0008H0-RM for qemu-devel@nongnu.org; Wed, 13 Jun 2012 09:55:33 -0400 Received: from mail-pb0-f45.google.com ([209.85.160.45]:47568) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Seo2i-0008Gi-Hq for qemu-devel@nongnu.org; Wed, 13 Jun 2012 09:55:28 -0400 Received: by pbbro12 with SMTP id ro12so2429644pbb.4 for ; Wed, 13 Jun 2012 06:55:26 -0700 (PDT) Message-ID: <4FD89BCB.7090407@codemonkey.ws> Date: Wed, 13 Jun 2012 08:55:23 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <4FD86E40.1000100@redhat.com> In-Reply-To: <4FD86E40.1000100@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC v0 6/8] xilinx dont cast to interface types with links List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: peter.maydell@linaro.org, aliguori@us.ibm.com, qemu-devel@nongnu.org, "Peter A. G. Crosthwaite" , paul@codesourcery.com, edgar.iglesias@gmail.com, afaerber@suse.de, john.williams@petalogix.com, avi@redhat.com 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 > >