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

[Peter Crosthwaite]

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;
>