[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH 01/27] qom: add the base Object class
Date: Wed, 21 Dec 2011 16:28:41 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:7.0.1) Gecko/20110930 Thunderbird/7.0.1

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

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


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


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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]