Re: [Qemu-devel] [PATCH 01/27] qom: add the base Object class

[Anthony Liguori]

On 12/21/2011 07:35 AM, Paolo Bonzini wrote:
> On 12/20/2011 05:51 PM, Anthony Liguori wrote:
> > This class provides the main building block for QEMU Object Model and is
> > extensively documented in the header file. It is largely inspired by GObject.
> >
> > Signed-off-by: Anthony Liguori<address@hidden>
> > ---
> > Makefile.objs | 2 +
> > hw/object.c | 469 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > hw/object.h | 427 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 898 insertions(+), 0 deletions(-)
> > create mode 100644 hw/object.c
> > create mode 100644 hw/object.h
> >
> > diff --git a/Makefile.objs b/Makefile.objs
> > index f753d83..b86e8a1 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -122,6 +122,8 @@ common-obj-$(CONFIG_WIN32) += version.o
> >
> > common-obj-$(CONFIG_SPICE) += ui/spice-core.o ui/spice-input.o
> > ui/spice-display.o spice-qemu-char.o
> >
> > +common-obj-y += object.o
> > +
> > audio-obj-y = audio.o noaudio.o wavaudio.o mixeng.o
> > audio-obj-$(CONFIG_SDL) += sdlaudio.o
> > audio-obj-$(CONFIG_OSS) += ossaudio.o
> > diff --git a/hw/object.c b/hw/object.c
> > new file mode 100644
> > index 0000000..620e63f
> > --- /dev/null
> > +++ b/hw/object.c
> > @@ -0,0 +1,469 @@
> > +/*
> > + * QEMU Object Model
> > + *
> > + * Copyright IBM, Corp. 2011
> > + *
> > + * Authors:
> > + * Anthony Liguori<address@hidden>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "object.h"
> > +
> > +#define MAX_INTERFACES 32
> > +
> > +typedef struct InterfaceImpl
> > +{
> > + const char *parent;
> > + void (*interface_initfn)(ObjectClass *class, void *data);
> > + Type type;
> > +} InterfaceImpl;
> > +
> > +typedef struct TypeImpl
> > +{
> > + const char *name;
> > + Type type;
>
> What's the need for "Type"? You can use simply the TypeImpl * and drop
> type_get_instance. Outside object.h it can be an opaque pointer.

It's a bit nicer for type_register to return a handle that can later be 
unregistered (although that's not currently implemented).

You could have it return TypeImpl * of course.  GObject uses a simpler GType but 
they don't have the notion of a symbolic type name.

I used a symbolic type name to avoid the problem of dependencies.  In order to 
create a type in gobject, you have to reference the parent's GType which usually 
means you have to call the _get_type() function which acts as a singleton which 
registers the type.

Since you have to specify the parent via a function call, you can't define the 
type in a unit-level static structure which I viewed as a critical requirement.

So it might be worth refactoring it away but I'd prefer to do that in the future 
once we're sure it's not useful.

> > +
> > + size_t class_size;
> > +
> > + size_t instance_size;
> > +
> > + void (*base_init)(ObjectClass *klass);
> > + void (*base_finalize)(ObjectClass *klass);
> > +
> > + void (*class_init)(ObjectClass *klass, void *data);
> > + void (*class_finalize)(ObjectClass *klass, void *data);
> > +
> > + void *class_data;
> > +
> > + void (*instance_init)(Object *obj);
> > + void (*instance_finalize)(Object *obj);
> > +
> > + bool abstract;
> > +
> > + const char *parent;
> > +
> > + ObjectClass *class;
> > +
> > + int num_interfaces;
> > + InterfaceImpl interfaces[MAX_INTERFACES];
>
> ... this way you can also allocate dynamically and use a variable-sized array
> for interfaces.

This could be made dynamic fairly easily.


> > +} TypeImpl;
> > +
> > +static int num_types = 1;
> > +static TypeImpl type_table[1024];
>
> ... and you can also drop this.

Yes, this could be gotten rid of.

> > +Type type_register_static(const TypeInfo *info)
> > +{
> > + Type type = num_types++;
> > + TypeImpl *ti;
> > +
> > + ti =&type_table[type];
> > +
> > + assert(info->name != NULL);
> > +
> > + printf("Added type %s -> %s\n", info->name, info->parent);
> > +
> > + ti->name = info->name;
>
> Why no strdup here?

'static' means that the strings are const char *.

> > + ti->parent = info->parent;
>
> Please store a Type or a pointer to TypeImpl.

It needs to be a symbolic name because the TypeImpl may not exist yet since we 
can't guarantee registration order.

This ends up being a tremendous simplification because we don't have to care 
about type registration order.

> Otherwise, I don't see the need to
> distinguish TypeInfo/InterfaceInfo and TypeImpl/InterfaceImpl at all. The only
> difference is num_interfaces, and
>
> for (i = 0; i < ti->num_interfaces; i++) {
>
> can be replaced just as well with
>
> for (i = 0; i < ti->interfaces[i].type; i++) {

InterfaceInfo is specifically limited in what it contains to prevent someone 
from shooting themselves in the foot.  It may be possible to just use TypeImpl 
in the backend but we should keep InterfaceInfo limited.

> > + ti->type = type;
> > +
> > + ti->class_size = info->class_size;
> > + ti->instance_size = info->instance_size;
> > +
> > + ti->base_init = info->base_init;
> > + ti->base_finalize = info->base_finalize;
> > +
> > + ti->class_init = info->class_init;
> > + ti->class_finalize = info->class_finalize;
> > + ti->class_data = info->class_data;
> > +
> > + ti->instance_init = info->instance_init;
> > + ti->instance_finalize = info->instance_finalize;
> > +
> > + 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++;
> > + }
> > + }
> > +
> > + return type;
>
> The return value is unused. Looks like a premature optimization or a leftover
> from GObject.

It will be used to allow type unregistration.  Think about dynamic 
loadable/unloadable modules.

> > +}
> > +
> > +static Type type_register_anonymous(const TypeInfo *info)
> > +{
> > + Type type = num_types++;
> > + TypeImpl *ti;
> > + char buffer[32];
> > + static int count;
> > +
> > + ti =&type_table[type];
> > +
> > + snprintf(buffer, sizeof(buffer), "<anonymous-%d>", count++);
> > + ti->name = g_strdup(buffer);
>
> g_strdup_printf, please. However, this has exactly one use in
> type_class_interface_init. Can you make the name something like
> <Class::Interface>, so that the meaning is more clear?

Ack.

> > +static Type type_get_by_name(const char *name)
> > +{
> > + int i;
> > +
> > + if (name == NULL) {
> > + return 0;
> > + }
> > +
> > + for (i = 1; i< num_types; i++) {
> > + if (strcmp(name, type_table[i].name) == 0) {
> > + return i;
> > + }
> > + }
>
> Please use a hash table here. Ultimately object creation might be in hot paths.
> For example I would like to turn SCSIRequests into QOM objects. It would let me
> reuse the reference counting as well as help with migration.

Ack.

> But in any case, this function should be called as little as possible, and
> should not be static in case other places want to cache the outcome.

Hrm, I don't think it should ever be used outside of object.c.  What are you 
thinking re: caching.

> > +static void object_interface_init(Object *obj, InterfaceImpl *iface)
> > +{
> > + TypeImpl *ti = type_get_instance(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);
>
> Please use a QSIMPLEQ.

I saw your github comment and looked at it.  Turns out, Interface shouldn't be 
public at all (because you only ever subclass InterfaceClass, never Interface). 
 You'll see in the header that I say something like, "don't ever subclass this" 
but why even expose something that a user isn't supposed to use?

So locally, I've moved Interface to be declared in object.c which means that in 
order to use QSIMPLEQ, I would have to make it public again since Object needs 
to hold a list of Interfaces.

Using GSList has the nice property of keeping the contents of the list totally 
private.

> > +}
> > +
> > +static void object_init(Object *obj, const char *typename)
> > +{
> > + TypeImpl *ti = type_get_instance(type_get_by_name(typename));
> > + int i;
> > +
> > + if (ti->parent) {
> > + object_init(obj, ti->parent);
> > + }
> > +
> > + for (i = 0; i< ti->num_interfaces; i++) {
> > + object_interface_init(obj,&ti->interfaces[i]);
> > + }
> > +
> > + if (ti->instance_init) {
> > + ti->instance_init(obj);
> > + }
> > +}
> > +
> > +void object_initialize(void *data, const char *typename)
> > +{
> > + TypeImpl *ti = type_get_instance(type_get_by_name(typename));
>
> I should be able to pass directly a Type (or a TypeImpl, whatever happens of my
> suggestion above) here.

The Type is not public.  It's a dynamic value and unless we introduce a 
_get_type() function (which would create a dependency problem), there's no easy 
way to do it.

Symbolic names really help simplify things by allowing dynamic dependency 
resolution.

> At the very least provide a function that takes a string and one that takes a
> Type/TypeImpl, and always use the latter when you tail call.

You're concerned about performance?

> > + Object *obj = data;
> > +
> > + g_assert(ti->instance_size>= sizeof(ObjectClass));
>
> Instead of this, please add to type_class_init an assertion that the instance
> size is bigger than the parent's.

Will take a look.

> > +
> > + type_class_init(ti);
> > +
> > + g_assert(ti->abstract == false);
> > +
> > + memset(obj, 0, ti->instance_size);
> > +
> > + obj->class = ti->class;
> > +
> > + object_init(obj, typename);
>
> What with the double line spacing? :)

I have no clue :-)  Will fix.

> > +}
> > +
> > +static void object_deinit(Object *obj, const char *typename)
> > +{
> > + TypeImpl *ti = type_get_instance(type_get_by_name(typename));
> > +
> > + if (ti->instance_finalize) {
> > + ti->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 (ti->parent) {
> > + object_init(obj, ti->parent);
> ^^^^
>
> Typo, you want deinit. And again, let's avoid type names. This is C, not BASIC.

Ack on the deinit.  But see above re: type names.

> > +const char *object_get_type(Object *obj)
> > +{
> > + return type_get_name(obj->class->type);
> > +}
>
> Let's not confuse types and type names.

Ack.

> > +ObjectClass *object_get_class(Object *obj)
> > +{
> > + return obj->class;
> > +}
> > +
> > +const char *object_class_get_name(ObjectClass *klass)
> > +{
> > + return type_get_name(klass->type);
> > +}
> > diff --git a/hw/object.h b/hw/object.h
> > new file mode 100644
> > index 0000000..80b7099
> > --- /dev/null
> > +++ b/hw/object.h
> > @@ -0,0 +1,427 @@
> > +/*
> > + * QEMU Object Model
> > + *
> > + * Copyright IBM, Corp. 2011
> > + *
> > + * Authors:
> > + * Anthony Liguori<address@hidden>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#ifndef QEMU_OBJECT_H
> > +#define QEMU_OBJECT_H
> > +
> > +#include "qemu-common.h"
> > +
> > +typedef uint64_t Type;
>
> struct TypeImpl;
> typedef struct TypeImpl *Type;

Not so sure on this.

> > +typedef struct ObjectClass ObjectClass;
> > +typedef struct Object Object;
> > +
> > +typedef struct TypeInfo TypeInfo;
> > +
> > +typedef struct InterfaceClass InterfaceClass;
> > +typedef struct Interface Interface;
> > +typedef struct InterfaceInfo InterfaceInfo;
> > +
> > +#define TYPE_OBJECT NULL
> > +
> > +/**
> > + * SECTION:object.h
> > + * @title:Base Object Type System
> > + * @short_description: interfaces for creating new types and objects
> > + *
> > + * The QEMU Object Model provides a framework for registering user creatable
> > + * types and instantiating objects from those types. QOM provides the following
> > + * features:
> > + *
> > + * - System for dynamically registering types
> > + * - Support for single-inheritance of types
> > + * - Multiple inheritance of stateless interfaces
> > + *
> > + *<example>
> > + *<title>Creating a minimal type</title>
> > + *<programlisting>
> > + * #include "qdev.h"
> > + *
> > + * #define TYPE_MY_DEVICE "my-device"
>
> #define TYPE_MY_DEVICE my_device_get_type()

This makes things pretty ugly.

> ...
>
> static Type my_device_type;
>
> extern void my_device_get_type(void)
> {
> return my_device_type();
> }
>
> static void my_device_type(void)
> {
> my_device_type = type_register_static(&my_device_info);

It cannot be reference an external structure.  You would have to do:

Type my_device_get_type(void)
{
    static Type my_device_type;

    if (my_device_type == 0) {
        TypeInfo my_device_info = {
            .name = "my-device",
            .parent = parent_device_get_type(),
        };

        my_device_type = type_register_static(&my_device_info);
    }

    return my_device_type;
}

static void my_device_module_init(void)
{
    my_device_get_type(); // dummy call to register type
}

device_init(my_device_module_init);

Besides looking uglier, it means that types don't exist until you fetch all of 
the types.  That's why you still need a module init.  The nasty bit is that if 
you do the module init, then you need to make sure that you load dynamic modules 
in dependency order first.

One really nice thing about having a string-lookup method to resolve type names 
is that it makes it very easy to support on-demand module loading.  All we need 
to do is look for a module if a type lookup fails and load it.

> }
>
> > + * After this initial copy, #TypeInfo::base_init is invoked. This is meant to
> > + * handle the case where a class may have a dynamic field that was copied via
> > + * a shallow copy but needs to be deep copied. #TypeInfo::base_init is called
> > + * for* each parent class but not for the class being instantiated.
> > + *
> > + * Once all of the parent classes have been initialized and their
> > + * #TypeInfo::base_init functions have been called, #TypeInfo::class_init is
> > + * called to let the class being instantiated provide default initialize for
> > + * it's virtual functions.
>
> base_init and base_finalize are unused. Unless you have a plan for it, let's
> avoid unused features, for now.

Tentative Ack unless I can come up with a compelling counter argument ;-)

> > + * # Interfaces #
> > + *
> > + * Interfaces allow a limited form of multiple inheritance. Instances are
> > + * similar to normal types except for the fact that are only defined by
> > + * their classes and never carry any state. You can cast an object to one
> > + * of its #Interface types and vice versa.
>
> You can _dynamically_ cast an object to one of its Interface types and vice 
> versa.

Ack.

> > + */
> > +
> > +/**
> > + * ObjectClass:
> > + *
> > + * The base for all classes. The only thing that #ObjectClass contains is an
> > + * integer type handle.
> > + */
> > +struct ObjectClass
> > +{
> > + /*< private>*/
> > + Type type;
> > +};
> > +
> > +/**
> > + * Object:
> > + *
> > + * The base for all objects. The first member of this object is a pointer to
> > + * a #ObjectClass. Since C guarantees that the first member of a structure
> > + * always begins at byte 0 of that structure, as long as any sub-object places
> > + * its parent as the first member, we can cast directly to a #Object.
> > + *
> > + * As a result, #Object contains a reference to the objects type as its
> > + * first member. This allows identification of the real type of the object at
> > + * run time.
> > + *
> > + * #Object also contains a list of #Interfaces that this object
> > + * implements.
> > + */
> > +struct Object
> > +{
> > + /*< private>*/
> > + ObjectClass *class;
> > +
> > + GSList *interfaces;
> > +};
> > +
> > +/**
> > + * TypeInfo:
> > + * @name: The name of the type.
> > + * @parent: The name of the parent type.
> > + * @instance_size: The size of the object (derivative of #Object). If
> > + * @instance_size is 0, then the size of the object will be the size of the
> > + * parent object.
> > + * @instance_init: This function is called to initialize an object. The parent
> > + * class will have already been initialized so the type is only responsible
> > + * for initializing its own members.
> > + * @instance_finalize: This function is called during object destruction. This
> > + * is called before the parent @instance_finalize function has been called.
> > + * An object should only free the members that are unique to its type in this
> > + * function.
> > + * @abstract: If this field is true, then the class is considered abstract and
> > + * cannot be directly instantiated.
> > + * @class_size: The size of the class object (derivative of #ObjectClass)
> > + * for this object. If @class_size is 0, then the size of the class will be
> > + * assumed to be the size of the parent class. This allows a type to avoid
> > + * implementing an explicit class type if they are not adding additional
> > + * virtual functions.
> > + * @base_init: This function is called after memcpy()'ing the base class into
> > + * the new class to reinitialize any members that require deep copy.
> > + * @base_finalize: This function is called during a class's destruction and is
> > + * meant to allow any dynamic parameters allocated by @base_init to be
> > + * released.
> > + * @class_init: This function is called after all parent class initialization
> > + * has occured to allow a class to set its default virtual method pointers.
> > + * This is also the function to use to override virtual methods from a parent
> > + * class.
> > + * @class_finalize: This function is called during class destruction and is
> > + * meant to release and dynamic parameters allocated by @class_init.
> > + * @class_data: Data to pass to the @class_init and @class_finalize functions.
> > + * This can be useful when building dynamic classes.
> > + * @interfaces: The list of interfaces associated with this type. This
> > + * should point to a static array that's terminated with a zero filled
> > + * element.
> > + */
> > +struct TypeInfo
> > +{
> > + const char *name;
> > + const char *parent;
> > +
> > + size_t instance_size;
> > + void (*instance_init)(Object *obj);
> > + void (*instance_finalize)(Object *obj);
> > +
> > + bool abstract;
> > + size_t class_size;
> > +
> > + void (*base_init)(ObjectClass *klass);
> > + void (*base_finalize)(ObjectClass *klass);
> > +
> > + void (*class_init)(ObjectClass *klass, void *data);
> > + void (*class_finalize)(ObjectClass *klass, void *data);
> > + void *class_data;
> > +
> > + InterfaceInfo *interfaces;
> > +};
> > +
> > +/**
> > + * OBJECT:
> > + * @obj: A derivative of #Object
> > + *
> > + * Converts an object to a #Object. Since all objects are #Objects,
> > + * this function will always succeed.
> > + */
> > +#define OBJECT(obj) \
> > + ((Object *)(obj))
> > +
> > +/**
> > + * OBJECT_CHECK:
> > + * @type: The C type to use for the return value.
> > + * @obj: A derivative of @type to cast.
> > + * @name: The QOM typename of @type
> > + *
> > + * A type safe version of @object_dynamic_cast_assert. Typically each class
> > + * will define a macro based on this type to perform type safe dynamic_casts to
> > + * this object type.
> > + *
> > + * If an invalid object is passed to this function, a run time assert will be
> > + * generated.
> > + */
> > +#define OBJECT_CHECK(type, obj, name) \
> > + ((type *)object_dynamic_cast_assert((Object *)(obj), (name)))
> > +
> > +/**
> > + * OBJECT_CLASS_CHECK:
> > + * @class: The C type to use for the return value.
> > + * @obj: A derivative of @type to cast.
> > + * @name: the QOM typename of @class.
> > + *
> > + * A type safe version of @object_check_class. This macro is typically wrapped
> > + * by each type to perform type safe casts of a class to a specific class type.
> > + */
> > +#define OBJECT_CLASS_CHECK(class, obj, name) \
> > + ((class *)object_class_dynamic_cast_assert((ObjectClass *)(obj), (name)))
> > +
> > +/**
> > + * OBJECT_GET_CLASS:
> > + * @class: The C type to use for the return value.
> > + * @obj: The object to obtain the class for.
> > + * @name: The QOM typename of @obj.
> > + *
> > + * This function will return a specific class for a given object. Its generally
> > + * used by each type to provide a type safe macro to get a specific class type
> > + * from an object.
> > + */
> > +#define OBJECT_GET_CLASS(class, obj, name) \
> > + OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name)
> > +
> > +/**
> > + * Interface:
> > + * @parent: The base class.
> > + *
> > + * The base for all Interfaces. This is a subclass of Object. Subclasses
> > + * of #Interface should never have an instance that contains anything other
> > + * than a single #Interface member. Do not attempt to create a type directly
> > + * by deriving from #Interface. Use #TypeInfo::interfaces instead.
> > + */
> > +struct Interface
> > +{
> > + Object parent;
> > +
> > + /*< private>*/
> > +
> > + Object *obj;
> > +};
> > +
> > +/**
> > + * InterfaceClass:
> > + * @parent_class: the base class
> > + *
> > + * The class for all interfaces. Subclasses of this class should only add
> > + * virtual methods.
> > + */
> > +struct InterfaceClass
> > +{
> > + ObjectClass parent_class;
> > +};
> > +
> > +/**
> > + * 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.
> > + */
> > +struct InterfaceInfo
> > +{
> > + const char *type;
> > +
> > + void (*interface_initfn)(ObjectClass *class, void *data);
> > +};
> > +
> > +#define TYPE_INTERFACE "interface"
>
> Let's make TYPE_* constants pointers, not strings.

See above.

Thanks for the thorough review!

Regards,

Anthony Liguori