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 <aligu...@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;
>
