[1/2] qom: Reimplement Interfaces

From: Anthony Liguori <aliguori@us.ibm.com>

The current implementation of Interfaces is poorly designed.  Each interface
that an object implements ends up being an object that's tracked by the
implementing object.  There's all sorts of gymnastics to deal with casting
between these objects.

But an interface shouldn't be associated with an Object.  Interfaces are global
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.

PC: Rebased against qom-next merge Jun-2012.

PC: Removed replication of cast logic for interfaces, i.e. there is only
one cast function - object_dynamic_cast() (and object_dynamic_cast_assert())

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 include/qemu/object.h |   46 +++++++----
 qom/object.c          |  220 +++++++++++++++++++------------------------------
 2 files changed, 116 insertions(+), 150 deletions(-)

Hi All,

We seem to be having difficulty getting a review/merge on this patch. I
have sent two series, two pings and a PULL, with only a single reply
from P. Maydell asking for other reviewers to weigh in:

--------- on July 17 P. Maydell wrote -----------

I guess I should mention that I'm assuming somebody else is going to
review
this patchset. It looks generally OK to me but whether the stream API
has the details of QOM interface use right is a bit out of my area
of expertise.

-- PMM

------------------------------------------------

What needs to happen (and what can I do) to make this series go through?
Edgar has reviewed P2 this morning and I have already remade the series,
but my primary concern is this patch, considering it touches the core
QOM infrastructure.

I have tested this patch on target i386 with no obvious breakages along
with testing on the the embedded platforms we work with (Zynq and MB).

Regards,
Peter

On Mon, 2012-08-06 at 13:07 +1000, Peter A. G. Crosthwaite wrote:
> From: Anthony Liguori <aliguori@us.ibm.com>
> 
> The current implementation of Interfaces is poorly designed.  Each interface
> that an object implements ends up being an object that's tracked by the
> implementing object.  There's all sorts of gymnastics to deal with casting
> between these objects.
> 
> But an interface shouldn't be associated with an Object.  Interfaces are global
> 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.
> 
> PC: Rebased against qom-next merge Jun-2012.
> 
> PC: Removed replication of cast logic for interfaces, i.e. there is only
> one cast function - object_dynamic_cast() (and object_dynamic_cast_assert())
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> ---
>  include/qemu/object.h |   46 +++++++----
>  qom/object.c          |  220 +++++++++++++++++++------------------------------
>  2 files changed, 116 insertions(+), 150 deletions(-)
> 
> diff --git a/include/qemu/object.h b/include/qemu/object.h
> index 8b17776..cc75fee 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;
> @@ -387,6 +387,16 @@ 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
>   *
> @@ -396,26 +406,30 @@ 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.
> - *
> - * The information associated with an interface.
> + * INTERFACE_CLASS:
> + * @klass: class to cast from
> + * 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 *)object_dynamic_cast_assert(OBJECT((obj)), (name)))
>  
>  /**
>   * object_new:
> diff --git a/qom/object.c b/qom/object.c
> index 00bb3b0..a552be2 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
> @@ -64,14 +62,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)
> @@ -98,6 +88,7 @@ static TypeImpl *type_table_lookup(const char *name)
>  static TypeImpl *type_register_internal(const TypeInfo *info)
>  {
>      TypeImpl *ti = g_malloc0(sizeof(*ti));
> +    int i;
>  
>      g_assert(info->name != NULL);
>  
> @@ -122,15 +113,10 @@ static TypeImpl *type_register_internal(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);
>  
> @@ -198,26 +184,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;
> +        }
>  
> -    info.name = name;
> -    iface->type = type_register_internal(&info);
> -    g_free(name);
> +        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.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)
>  {
>      TypeImpl *parent;
> -    int i;
>  
>      if (ti->class) {
>          return;
> @@ -231,9 +239,33 @@ static void type_initialize(TypeImpl *ti)
>      parent = type_get_parent(ti);
>      if (parent) {
>          type_initialize(parent);
> +        GSList *e;
> +        int i;
>  
>          g_assert(parent->class_size <= ti->class_size);
>          memcpy(ti->class, parent->class, parent->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++) {
> +            TypeImpl *t = type_get_by_name(ti->interfaces[i].typename);
> +            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);
> +        }
>      }
>  
>      ti->class->type = ti;
> @@ -245,38 +277,19 @@ static void type_initialize(TypeImpl *ti)
>          parent = type_get_parent(parent);
>      }
>  
> -    for (i = 0; i < ti->num_interfaces; i++) {
> -        type_class_interface_init(ti, &ti->interfaces[i]);
> -    }
> -
>      if (ti->class_init) {
>          ti->class_init(ti->class, ti->class_data);
>      }
> -}
>  
> -static void object_interface_init(Object *obj, InterfaceImpl *iface)
> -{
> -    TypeImpl *ti = iface->type;
> -    Interface *iface_obj;
> -
> -    iface_obj = INTERFACE(object_new(ti->name));
> -    iface_obj->obj = obj;
>  
> -    obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
>  }
>  
>  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);
>      }
> @@ -357,12 +370,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));
>      }
> @@ -409,74 +416,15 @@ 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);
> -}
> -
>  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.
> -    */
> -    if (target_type && object_is_type(obj, target_type)) {
> +    if (object_class_dynamic_cast(object_get_class(obj), 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;
>  }
>  
> -
>  Object *object_dynamic_cast_assert(Object *obj, const char *typename)
>  {
>      Object *inst;
> @@ -497,16 +445,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;
>  
> -        type = type_get_parent(type);
> +        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++;
> +            }
> +         }
> +
> +        /* 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,
> @@ -920,12 +882,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,
> @@ -1022,10 +978,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;
>  
> @@ -1246,7 +1198,7 @@ static void register_types(void)
>  {
>      static TypeInfo interface_info = {
>          .name = TYPE_INTERFACE,
> -        .instance_size = sizeof(Interface),
> +        .class_size = sizeof(InterfaceClass),
>          .abstract = true,
>      };
>

Il 08/08/2012 01:46, Peter Crosthwaite ha scritto:
> Hi All,
> 
> We seem to be having difficulty getting a review/merge on this patch. I
> have sent two series, two pings and a PULL, with only a single reply
> from P. Maydell asking for other reviewers to weigh in:
> 
> --------- on July 17 P. Maydell wrote -----------
> 
> I guess I should mention that I'm assuming somebody else is going to
> review
> this patchset. It looks generally OK to me but whether the stream API
> has the details of QOM interface use right is a bit out of my area
> of expertise.
> 
> -- PMM
> 
> ------------------------------------------------
> 
> What needs to happen (and what can I do) to make this series go through?
> Edgar has reviewed P2 this morning and I have already remade the series,
> but my primary concern is this patch, considering it touches the core
> QOM infrastructure.
> 
> I have tested this patch on target i386 with no obvious breakages along
> with testing on the the embedded platforms we work with (Zynq and MB).

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

:)

Paolo