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

[Paolo Bonzini]

On 12/21/2011 03:35 PM, Anthony Liguori wrote:
> > > +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 *.

Sure, but if you later want to unregister, the ownership of ->name and 
->parent need to be the same for all paths.  In this case 
type_register_anonymous makes it "owned by object.c" and 
type_register_static makes it "owned by caller".

> > > + 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.

Got it now.  However, let's cache the Type as soon as we resolve it the 
first time.

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

Let's keep Impl/Info separate, but use the difference so that we can 
cache name->type mappings as much as possible.

> > > + 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.

Ack.

> > > + 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.

I'm thinking of using

> > > +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).

I agree that Interface and INTERFACE() should be private to object.c, 
but it's not a big deal to leave them as opaque in object.h.  Leaving 
Interface as an opaque type is enough in order to use QSIMPLEQ.

> > > +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.

True, we need to keep types as strings, but we can do some kind of lazy 
initialization.  See IRC conversation + later.

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

Yes, I understand this now.  But once we're out of the initialization 
path we should be able to avoid them in hot paths.
> > 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?

Yes.  At least for a simple subclass of Object, object_new should not be 
_that_ much slower than a g_malloc.

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

Yeah, I see now the problem with dependencies, but I'm pretty sure we'll 
reach a compromise. :)

> > > +typedef uint64_t Type;
> >
> > struct TypeImpl;
> > typedef struct TypeImpl *Type;
>
> Not so sure on this.

It doesn't really matter, it can be refactored away later.  But on IRC 
you said it worked out nicely.

The point is when to use strings and when to use type handles.  It 
doesn't matter whether the handles are direct pointers or indices into a 
registry.

> > > +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.

Yes, indeed.  Let's add a fast path for object_new and object_initialize 
that takes a Type value.  To use these you use a get_type() function or 
fetch the Type from a static variable.

Another reason why I'd like you to provide this fast path from the 
beginning, is because you're doing a lot of type_get_by_name() even 
though the type has been found already.  Having the fast paths would 
make you more honest about what needs to do the lookup and what doesn't. 
 Basically only the toplevel calls need to.

Of course this makes this comment moot too:

> > > +#define TYPE_INTERFACE "interface"
> >
> > Let's make TYPE_* constants pointers, not strings.

Thanks,

Paolo