Re: [Qemu-devel] [PATCH 2/3] qom: reimplement Interfaces

[Peter Crosthwaite]

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
<address@hidden> 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 <address@hidden>
>> ---
>>  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;
>>
>
>