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