Patchwork [v2,01/12] qdev: export and use qbus_init

login
register
mail settings
Submitter Paolo Bonzini
Date Jan. 21, 2013, 12:30 p.m.
Message ID <1358771422-14282-2-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/214129/
State New
Headers show

Comments

Paolo Bonzini - Jan. 21, 2013, 12:30 p.m.
BusState subclasses need to do their own allocation because
qbus_create_inplace calls object_initialize (which wipes out the
"free" callback).  This patch separates the initialization of the object
(object_initialize) from its insertion in the qdev tree (qbus_realize); to
do so, it moves the remaining bits of qbus_create_inplace to qbus_realize
and export it as qbus_init.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-core.h |  1 +
 hw/qdev.c      | 18 +++++++-----------
 2 files changed, 8 insertions(+), 11 deletions(-)
Andreas Färber - Jan. 21, 2013, 1:01 p.m.
Am 21.01.2013 13:30, schrieb Paolo Bonzini:
> BusState subclasses need to do their own allocation because
> qbus_create_inplace calls object_initialize (which wipes out the
> "free" callback).  This patch separates the initialization of the object
> (object_initialize) from its insertion in the qdev tree (qbus_realize); to
> do so, it moves the remaining bits of qbus_create_inplace to qbus_realize
> and export it as qbus_init.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I think I left some comments on v1: Would it be possible to leave a
bus_initialize() function (without q ;)) in place that calls
object_initialize() plus the-artist-formerly-known-as-qbus_realize(),
shared between object_new() and object_initialize()? ->free was always
set afterwards. The issue I am trying to contain here is a surge of
*_init functions beyond class_init, instance_init, DeviceClass::init.
Sticking to the QOM naming of having *bus_initialize() and *bus_new()
would address that.

Maybe if we reorder the two patches, dropping the use of g_malloc0() first?

Currently care needs to be taken with the in-place bus initialization
functions to not apply PCI_BUS() etc. on the uninitialized variable.
Having a pci_bus_initialize(void *, ...) -> bus_initialize(void *, ...)
-> object_initialize(void *, ...) call chain would solve that.

Andreas
Paolo Bonzini - Jan. 21, 2013, 1:15 p.m.
Il 21/01/2013 14:01, Andreas Färber ha scritto:
>> > BusState subclasses need to do their own allocation because
>> > qbus_create_inplace calls object_initialize (which wipes out the
>> > "free" callback).  This patch separates the initialization of the object
>> > (object_initialize) from its insertion in the qdev tree (qbus_realize); to
>> > do so, it moves the remaining bits of qbus_create_inplace to qbus_realize
>> > and export it as qbus_init.
>> > 
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> I think I left some comments on v1: Would it be possible to leave a
> bus_initialize() function (without q ;)) in place that calls
> object_initialize() plus the-artist-formerly-known-as-qbus_realize(),
> shared between object_new() and object_initialize()? ->free was always
> set afterwards. The issue I am trying to contain here is a surge of
> *_init functions beyond class_init, instance_init, DeviceClass::init.
> Sticking to the QOM naming of having *bus_initialize() and *bus_new()
> would address that.

Yes, you did and I thought about it but I had no good ideas really.  We
have a lot of differently named functions, but in the end "everything
before realize should be named init" sounds like the only sensible rule...

Paolo

Patch

diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index 731aadd..c9f7fa1 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -229,6 +229,7 @@  DeviceState *qdev_find_recursive(BusState *bus, const char *id);
 typedef int (qbus_walkerfn)(BusState *bus, void *opaque);
 typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque);
 
+void qbus_init(BusState *bus, DeviceState *parent, const char *name);
 void qbus_create_inplace(BusState *bus, const char *typename,
                          DeviceState *parent, const char *name);
 BusState *qbus_create(const char *typename, DeviceState *parent, const char *name);
diff --git a/hw/qdev.c b/hw/qdev.c
index 9761016..b473bd7 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -390,14 +390,16 @@  DeviceState *qdev_find_recursive(BusState *bus, const char *id)
     return NULL;
 }
 
-static void qbus_realize(BusState *bus)
+void qbus_init(BusState *bus, DeviceState *parent, const char *name)
 {
     const char *typename = object_get_typename(OBJECT(bus));
     char *buf;
     int i,len;
 
-    if (bus->name) {
-        /* use supplied name */
+    bus->parent = parent;
+
+    if (name) {
+        bus->name = g_strdup(name);
     } else if (bus->parent && bus->parent->id) {
         /* parent device has id -> use it for bus name */
         len = strlen(bus->parent->id) + 16;
@@ -430,10 +432,7 @@  void qbus_create_inplace(BusState *bus, const char *typename,
                          DeviceState *parent, const char *name)
 {
     object_initialize(bus, typename);
-
-    bus->parent = parent;
-    bus->name = name ? g_strdup(name) : NULL;
-    qbus_realize(bus);
+    qbus_init(bus, parent, name);
 }
 
 BusState *qbus_create(const char *typename, DeviceState *parent, const char *name)
@@ -441,10 +440,7 @@  BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
     BusState *bus;
 
     bus = BUS(object_new(typename));
-
-    bus->parent = parent;
-    bus->name = name ? g_strdup(name) : NULL;
-    qbus_realize(bus);
+    qbus_init(bus, parent, name);
 
     return bus;
 }