Re: [Qemu-devel] Issues around TYPE_INTERFACE

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> We have two separate type trees, object types rooted at TYPE_OBJECT, and
> interface types at TYPE_INTERFACE.
>
> Object types are fore defining ultimately concrete types, i.e. any
> concrete type is a descendant of TYPE_OBJECT.  Interior nodes of the
> TYPE_OBJECT tree are commonly abstract, with a few exceptions.  Leaves
> need to be concrete to be of any use.
>
> Interface types are for defining interfaces object types provide (by
> listing them in a type's .interfaces).  TYPE_INTERFACE and all its
> descendants are abstract.
[...]
> Issue #2: INTERFACE_CHECK()
>
>     #define OBJECT_CHECK(type, obj, name) \
>         ((type *)object_dynamic_cast_assert(OBJECT(obj), (name), \
>                                             __FILE__, __LINE__, __func__))
>
>     #define INTERFACE_CHECK(interface, obj, name) \
>         ((interface *)object_dynamic_cast_assert(OBJECT((obj)), (name), \
>                                                  __FILE__, __LINE__, 
> __func__))
>
> The two are identical (except the former lacks parenthesis around obj,
> which is a minor bug).
>
> Obvious question: shouldn't we de-duplicate the code?

On closer look, I wonder how the interface types' checked conversion to
the interface type can possibly work.

Recall the general QOM boilerplate for a type:

    #define TYPE_MY_FROB "my-frob"

    #define MY_FROB(obj) \
        OBJECT_CHECK(MyFrob, (obj), TYPE_MY_FROB)
    #define MY_FROB_CLASS(klass) \
        OBJECT_CLASS_CHECK(MyFrobClass, (klass), TYPE_MY_FROB)
    #define MY_FROB_GET_CLASS(obj) \
        OBJECT_GET_CLASS(MyFrobClass, (obj), TYPE_MY_FROB)

    typedef struct MyFrob MyFrob;
    typedef struct MyFrobClass MyFrobClass;

A QOM object struct such as MyFrob has its (single) parent object struct
embedded as first member.

The *unchecked* conversion to a (direct or indirect) parent type is a
simple type cast.

The *checked* conversion additionally checks the type we convert to is
actually a (direct or indirect) parent type.

Any interface structs can only be stored elsewhere.  Where is the code
to map from @obj to "elsewhere", unchecked or checked?  Let's go hunting
for it.

The obvious place to start is the checked conversion
object_dynamic_cast():

    /**
     * object_dynamic_cast:
     * @obj: The object to cast.
     * @typename: The @typename to cast to.
     *
     * This function will determine if @obj is-a @typename.  @obj can refer to 
an
     * object or an interface associated with an object.
     *
     * Returns: This function returns @obj on success or #NULL on failure.
     */
    Object *object_dynamic_cast(Object *obj, const char *typename)
    {
        if (obj && object_class_dynamic_cast(object_get_class(obj), typename)) {
            return obj;
        }

        return NULL;
    }

This cannot possibly return a pointer to the interface struct!

Example 1: MEMORY_DEVICE(obj) where @obj is an instance of TYPE_PC_DIMM.

    The macro expands via

        INTERFACE_CHECK(MemoryDeviceState, (obj), TYPE_MEMORY_DEVICE)

    into

        ((MemoryDeviceState *)object_dynamic_cast_assert(OBJECT((obj)),
                                            (TYPE_MEMORY_DEVICE),
                                            __FILE__, __LINE__, __func__))

    object_dynamic_cast_assert() is basically just object_dynamic_cast()
    wrapped in tracing, caching, and "assert success", plus a way to
    suppress the actual check at compile time for speed.

    To make the cast to MemoryDeviceState * safe, we need
    object_dynamic_cast() return a MemoryDeviceState (or a subtype, but
    none exist) on success.

    But it doesn't!  It returns @obj, i.e. a PCDIMMDevice.

    Why doesn't this explode?

    Turns out MemoryDeviceState is an *incomplete* type.  As far as I
    can tell, all we do with it is converting it back to PCDIMMDevice,
    e.g. in pc_dimm_md_get_memory_region(), or accessing properties
    PCDIMMDevice has, e.g. in pc_dimm_md_get_addr().

In other words, the conversions from object to interface don't actually
return what they pretend to return (a pointer to the interface struct),
but that's alright, because their callers don't use the value to access
what they pretend to return (the interface struct), but instead use it
to access what they *actually* return, namely the *object*, casting away
the bogus interface type.

What a mess.


But wait, I got a few bonus messes for you!

object_dynamic_cast() delegates the actual checking to
object_class_dynamic_cast():

    /**
     * object_class_dynamic_cast:
     * @klass: The #ObjectClass to attempt to cast.
     * @typename: The QOM typename of the class to cast to.
     *
     * Returns: If @typename is a class, this function returns @klass if
     * @typename is a subtype of @klass, else returns #NULL.

"a subtype of @klass"?  Really?  To make object_dynamic_cast() work as
advertized ("determine if @obj is-a @typename"), we need supertype here,
don't we?

     *
     * If @typename is an interface, this function returns the interface
     * definition for @klass if @klass implements it unambiguously; #NULL
     * is returned if @klass does not implement the interface or if multiple
     * classes or interfaces on the hierarchy leading to @klass implement
     * it.  (FIXME: perhaps this can be detected at type definition time?)
     */

If we ignore interfaces, then this mirrors object_dynamic_cast(): it
returns either its first argument or NULL,

With interfaces, the contract become rather confusing.  What is "the
interface definition for @klass"?

Let's see what the code actually does.

    ObjectClass *object_class_dynamic_cast(ObjectClass *class,
                                           const char *typename)
    {
        ObjectClass *ret = NULL;
        TypeImpl *target_type;
        TypeImpl *type;

        if (!class) {
            return NULL;
        }

Fail on null @class.  Okay.  Can't happen when called from
object_dynamic_cast().

        /* A simple fast path that can trigger a lot for leaf classes.  */
        type = class->type;
        if (type->name == typename) {
            return class;
        }

Fast path for conversion to itself.  Okay.

        target_type = type_get_by_name(typename);
        if (!target_type) {
            /* target class type unknown, so fail the cast */
            return NULL;
        }

Fail if @typename doesn't exist.  Perhaps this should be a programming
error instead, but we might also use it to test whether a type is
compiled in or not.  Let's move on.

        if (type->class->interfaces &&
                type_is_ancestor(target_type, type_interface)) {

Note: type->class must be the same as @class.  Never pass a chance to
confuse.

type_is_ancestor(t1, t2) searches t1's parent chain for t2.  Simple
enough.

If @class has interfaces and @typename names an interface...

            int found = 0;
            GSList *i;

            for (i = class->interfaces; i; i = i->next) {

... search interfaces for a match.

Note that class->interfaces holds the type's and all its parent types'
interfaces.  See type_initialize() below.

The condition does *not* match the function contract's "if @typename is
an interface"!

                ObjectClass *target_class = i->data;

                if (type_is_ancestor(target_class->type, target_type)) {
                    ret = target_class;
                    found++;
                }

Any interface that is a descendant of interface @typename is a match.

             }

            /* The match was ambiguous, don't allow a cast */
            if (found > 1) {
                ret = NULL;
            }

Fail unless @class and all its parents have exactly one interface that
is a descendant of interface @typename.

On success, return that interface's class.

        } else if (type_is_ancestor(type, target_type)) {

Fail unless @class is a descendant of @typename.

            ret = class;
        }

        return ret;
    }

Example 2: MEMORY_DEVICE_CLASS(klass) where @klass is of TYPE_PC_DIMM.

    The macro expands via

        OBJECT_CLASS_CHECK(MemoryDeviceClass, (klass), TYPE_MEMORY_DEVICE)

    into

        ((MemoryDeviceClass 
*)object_class_dynamic_cast_assert(OBJECT_CLASS(class),
                                            (TYPE_MEMORY_DEVICE),
                                            __FILE__, __LINE__, __func__))

    Since @klass has interfaces and TYPE_MEMORY_DEVICE is an interface,
    we take the first arm of object_class_dynamic_cast()'s conditional.
    We find the class of interface TYPE_MEMORY_DEVICE, and its the only
    match, so we return it.  Good.

Example 2: object_class_dynamic_cast(klass, TYPE_INTERFACE) where @klass
    is something like INTERFACE_PCIE_DEVICE.

    First, consider the function contract.

    On the one hand, TYPE_INTERFACE is certainly an interface, so it should
    return "the interface definition for @klass", whatever that may be.
    Since INTERFACE_PCIE_DEVICE has no interfaces, I suppose it means we
    should return NULL.

    On the other hand, INTERFACE_PCIE_DEVICE is a child of TYPE_INTERFACE,
    so it should obviously use the other clause and return @klass.

    Now consider the actual code.  Since @klass has no interfaces, we
    take the second argum of object_class_dynamic_cast()'s conditional.
    Since TYPE_INTERFACE is INTERFACE_PCIE_DEVICE's parent, we return
    @klass.

I believe what the code does is okay, but the function contract is crap.


Finally, have a look at type_initialize(), specifically how it
initializes ti->class->interfaces:

    static void type_initialize(TypeImpl *ti)
    {
        TypeImpl *parent;

        if (ti->class) {
            return;
        }

        ti->class_size = type_class_get_size(ti);
        ti->instance_size = type_object_get_size(ti);
        /* Any type with zero instance_size is implicitly abstract.
         * This means interface types are all abstract.
         */
        if (ti->instance_size == 0) {
            ti->abstract = true;
        }
        if (type_is_ancestor(ti, type_interface)) {
            assert(ti->instance_size == 0);
            assert(ti->abstract);
            assert(!ti->instance_init);
            assert(!ti->instance_post_init);
            assert(!ti->instance_finalize);
            assert(!ti->num_interfaces);
        }
        ti->class = g_malloc0(ti->class_size);

        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);
            ti->class->interfaces = NULL;

Start with the empty list.

            ti->class->properties = g_hash_table_new_full(
                g_str_hash, g_str_equal, g_free, object_property_free);

            for (e = parent->class->interfaces; e; e = e->next) {
                InterfaceClass *iface = e->data;
                ObjectClass *klass = OBJECT_CLASS(iface);

                type_initialize_interface(ti, iface->interface_type, 
klass->type);
            }

Copy the parent->class->interfaces to it.

            for (i = 0; i < ti->num_interfaces; i++) {

For all own interfaces...

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

If we already have one that is an ancestor, skip.

                type_initialize_interface(ti, t, t);

Else add.

            }

WTF?

Consider TYPE_INTERFACE parent of TYPE_CHILD_INTERFACE parent of
TYPE_GRANDCHILD_INTERFACE.

Say we add TYPE_CHILD_INTERFACE in either loop before considering
TYPE_GRANDCHILD_INTERFACE in the second loop, i.e.

    @t is TYPE_GRANDCHILD_INTERFACE
    @target_type is TYPE_CHILD_INTERFACE

type_is_ancestor(target_type, t) is false.  We add both interfaces.

But if it's the other way round, we add only TYPE_GRANDCHILD_INTERFACE.

Either I'm totally confused, or there's something wrong here.

        } else {
            ti->class->properties = g_hash_table_new_full(
                g_str_hash, g_str_equal, g_free, object_property_free);

We silently ignore interfaces in types without parents.

        }

        ti->class->type = ti;

        while (parent) {
            if (parent->class_base_init) {
                parent->class_base_init(ti->class, ti->class_data);
            }
            parent = type_get_parent(parent);
        }

        if (ti->class_init) {
            ti->class_init(ti->class, ti->class_data);
        }
    }


Can't pass up the opportunity to quote Hoare:

    There are two ways of constructing a software design: One way is to
    make it so simple that there are obviously no deficiencies, and the
    other way is to make it so complicated that there are no obvious
    deficiencies.


[...]