Patchwork [RFC,04/34] qdev: Prepare "realized" property

login
register
mail settings
Submitter Andreas Färber
Date Nov. 26, 2012, 12:12 a.m.
Message ID <1353888766-6951-5-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/201619/
State New
Headers show

Comments

Andreas Färber - Nov. 26, 2012, 12:12 a.m.
Based on earlier patches by Paolo and me, introduce the QOM realizefn at
device level only, as requested by Anthony.

For now this just wraps the qdev initfn.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
Cc: Anthony Liguori <anthony@codemonkey.ws>
---
 hw/qdev-core.h |    4 +++
 hw/qdev.c      |  100 ++++++++++++++++++++++++++++++++++++++++++--------------
 2 Dateien geändert, 80 Zeilen hinzugefügt(+), 24 Zeilen entfernt(-)
Eduardo Habkost - Dec. 12, 2012, 2:29 p.m.
On Mon, Nov 26, 2012 at 01:12:16AM +0100, Andreas Färber wrote:
> Based on earlier patches by Paolo and me, introduce the QOM realizefn at
> device level only, as requested by Anthony.
> 
> For now this just wraps the qdev initfn.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Cc: Anthony Liguori <anthony@codemonkey.ws>
> ---
>  hw/qdev-core.h |    4 +++
>  hw/qdev.c      |  100 ++++++++++++++++++++++++++++++++++++++++++--------------
>  2 Dateien geändert, 80 Zeilen hinzugefügt(+), 24 Zeilen entfernt(-)
> 
> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
> index f0d3a5e..580a811 100644
> --- a/hw/qdev-core.h
> +++ b/hw/qdev-core.h
> @@ -29,6 +29,8 @@ enum {
>  typedef int (*qdev_initfn)(DeviceState *dev);
>  typedef int (*qdev_event)(DeviceState *dev);
>  typedef void (*qdev_resetfn)(DeviceState *dev);
> +typedef void (*DeviceRealize)(DeviceState *dev, Error **err);
> +typedef void (*DeviceUnrealize)(DeviceState *dev, Error **err);
>  
>  struct VMStateDescription;
>  
> @@ -47,6 +49,8 @@ typedef struct DeviceClass {
>      const struct VMStateDescription *vmsd;
>  
>      /* Private to qdev / bus.  */
> +    DeviceRealize realize;
> +    DeviceUnrealize unrealize;
>      qdev_initfn init;
>      qdev_event unplug;
>      qdev_event exit;
> diff --git a/hw/qdev.c b/hw/qdev.c
> index bce6ad5..d7b6320 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -147,37 +147,30 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
>     Return 0 on success.  */
>  int qdev_init(DeviceState *dev)
>  {
> -    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> -    int rc;
> +    Error *local_err = NULL;
>  
>      assert(!dev->realized);
>  
> -    rc = dc->init(dev);
> -    if (rc < 0) {
> +    object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
> +    if (local_err != NULL) {
> +        error_free(local_err);
>          object_delete(OBJECT(dev));
> -        return rc;
> +        return -1;
>      }
> +    return 0;
> +}
>  
> -    if (!OBJECT(dev)->parent) {
> -        static int unattached_count = 0;
> -        gchar *name = g_strdup_printf("device[%d]", unattached_count++);
> -
> -        object_property_add_child(container_get(qdev_get_machine(),
> -                                                "/unattached"),
> -                                  name, OBJECT(dev), NULL);
> -        g_free(name);
> -    }
> +static void device_realize(DeviceState *dev, Error **err)
> +{
> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>  
> -    if (qdev_get_vmsd(dev)) {
> -        vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
> -                                       dev->instance_id_alias,
> -                                       dev->alias_required_for_version);
> -    }
> -    dev->realized = true;
> -    if (dev->hotplugged) {
> -        device_reset(dev);
> +    if (dc->init) {
> +        int rc = dc->init(dev);
> +        if (rc < 0) {
> +            error_setg(err, "Device initialization failed.");
> +            return;
> +        }
>      }
> -    return 0;
>  }
[...]
> +static void device_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    dc->realize = device_realize;
> +}
> +
[...]

Stupid question: what exactly is the difference in capabilities and
semantics between DeviceClass.init() and DeviceClass.realize()? They
look exactly the same to me, from a first look. On which cases would
somebody use the former instead of the latter, and why?
Andreas Färber - Dec. 12, 2012, 4:51 p.m.
Am 12.12.2012 15:29, schrieb Eduardo Habkost:
> On Mon, Nov 26, 2012 at 01:12:16AM +0100, Andreas Färber wrote:
>> Based on earlier patches by Paolo and me, introduce the QOM realizefn at
>> device level only, as requested by Anthony.
>>
>> For now this just wraps the qdev initfn.

...which it deprecates.

>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> Cc: Anthony Liguori <anthony@codemonkey.ws>
>> ---
>>  hw/qdev-core.h |    4 +++
>>  hw/qdev.c      |  100 ++++++++++++++++++++++++++++++++++++++++++--------------
>>  2 Dateien geändert, 80 Zeilen hinzugefügt(+), 24 Zeilen entfernt(-)
>>
>> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
>> index f0d3a5e..580a811 100644
>> --- a/hw/qdev-core.h
>> +++ b/hw/qdev-core.h
>> @@ -29,6 +29,8 @@ enum {
>>  typedef int (*qdev_initfn)(DeviceState *dev);
>>  typedef int (*qdev_event)(DeviceState *dev);
>>  typedef void (*qdev_resetfn)(DeviceState *dev);
>> +typedef void (*DeviceRealize)(DeviceState *dev, Error **err);
>> +typedef void (*DeviceUnrealize)(DeviceState *dev, Error **err);
>>  
>>  struct VMStateDescription;
>>  
>> @@ -47,6 +49,8 @@ typedef struct DeviceClass {
>>      const struct VMStateDescription *vmsd;
>>  
>>      /* Private to qdev / bus.  */
>> +    DeviceRealize realize;
>> +    DeviceUnrealize unrealize;
>>      qdev_initfn init;
>>      qdev_event unplug;
>>      qdev_event exit;
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index bce6ad5..d7b6320 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -147,37 +147,30 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
>>     Return 0 on success.  */
>>  int qdev_init(DeviceState *dev)
>>  {
>> -    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>> -    int rc;
>> +    Error *local_err = NULL;
>>  
>>      assert(!dev->realized);
>>  
>> -    rc = dc->init(dev);
>> -    if (rc < 0) {
>> +    object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
>> +    if (local_err != NULL) {
>> +        error_free(local_err);
>>          object_delete(OBJECT(dev));
>> -        return rc;
>> +        return -1;
>>      }
>> +    return 0;
>> +}
>>  
>> -    if (!OBJECT(dev)->parent) {
>> -        static int unattached_count = 0;
>> -        gchar *name = g_strdup_printf("device[%d]", unattached_count++);
>> -
>> -        object_property_add_child(container_get(qdev_get_machine(),
>> -                                                "/unattached"),
>> -                                  name, OBJECT(dev), NULL);
>> -        g_free(name);
>> -    }
>> +static void device_realize(DeviceState *dev, Error **err)
>> +{
>> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>>  
>> -    if (qdev_get_vmsd(dev)) {
>> -        vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
>> -                                       dev->instance_id_alias,
>> -                                       dev->alias_required_for_version);
>> -    }
>> -    dev->realized = true;
>> -    if (dev->hotplugged) {
>> -        device_reset(dev);
>> +    if (dc->init) {
>> +        int rc = dc->init(dev);
>> +        if (rc < 0) {
>> +            error_setg(err, "Device initialization failed.");
>> +            return;
>> +        }
>>      }
>> -    return 0;
>>  }
> [...]
>> +static void device_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +    dc->realize = device_realize;
>> +}
>> +
> [...]
> 
> Stupid question: what exactly is the difference in capabilities and
> semantics between DeviceClass.init() and DeviceClass.realize()? They
> look exactly the same to me, from a first look. On which cases would
> somebody use the former instead of the latter, and why?

Wherever possible, users should use the new DeviceClass::realize.

"init" or "initfn" clashes with QOM's instance_init name-wise.
As shown in this series, instance_init can be used in devices today
already, without having realize.

The distinction between instance_init and realize is that the former
initializes simple variables, sets up properties and anything the user
may want to modify, then realize processes these variables and is able
to fail and report the cause of failure (unlike DeviceClass::init).

In the isa patch after all the boring QOM'ish preparations, isa-device's
class_init overwrites DeviceClass:realize, thereby no longer invoking
any DeviceClass::init callback for the ISA devices. I2C, PCI, SysBus,
etc. would need to be done in further steps, but a) this is work, b) the
series shouldn't get too long for review/merge and c) it all depends on
in which class and thereby with which signature we want to have realize
- DeviceClass::realize(DeviceState *, Error **) or
ObjectClass::realize(Object *, Error **).

Andreas
Eduardo Habkost - Dec. 12, 2012, 6:16 p.m.
On Wed, Dec 12, 2012 at 05:51:18PM +0100, Andreas Färber wrote:
> Am 12.12.2012 15:29, schrieb Eduardo Habkost:
> > On Mon, Nov 26, 2012 at 01:12:16AM +0100, Andreas Färber wrote:
> >> Based on earlier patches by Paolo and me, introduce the QOM realizefn at
> >> device level only, as requested by Anthony.
> >>
> >> For now this just wraps the qdev initfn.
> 
> ...which it deprecates.

Good to know. I thought they had different purposes and I was missing
something.


[...]
> >> +static void device_realize(DeviceState *dev, Error **err)
> >> +{
> >> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> >>  
> >> -    if (qdev_get_vmsd(dev)) {
> >> -        vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
> >> -                                       dev->instance_id_alias,
> >> -                                       dev->alias_required_for_version);
> >> -    }
> >> -    dev->realized = true;
> >> -    if (dev->hotplugged) {
> >> -        device_reset(dev);
> >> +    if (dc->init) {
> >> +        int rc = dc->init(dev);
> >> +        if (rc < 0) {
> >> +            error_setg(err, "Device initialization failed.");
> >> +            return;
> >> +        }
> >>      }
> >> -    return 0;
> >>  }
> > [...]
> >> +static void device_class_init(ObjectClass *oc, void *data)
> >> +{
> >> +    DeviceClass *dc = DEVICE_CLASS(oc);
> >> +    dc->realize = device_realize;
> >> +}
> >> +
> > [...]
> > 
> > Stupid question: what exactly is the difference in capabilities and
> > semantics between DeviceClass.init() and DeviceClass.realize()? They
> > look exactly the same to me, from a first look. On which cases would
> > somebody use the former instead of the latter, and why?
> 
> Wherever possible, users should use the new DeviceClass::realize.
> 
> "init" or "initfn" clashes with QOM's instance_init name-wise.
> As shown in this series, instance_init can be used in devices today
> already, without having realize.

True. That's something I like in the new name.  :-)

> 
> The distinction between instance_init and realize is that the former
> initializes simple variables, sets up properties and anything the user
> may want to modify, then realize processes these variables and is able
> to fail and report the cause of failure (unlike DeviceClass::init).

I understood the difference between ObjectClass.instance_init() and
DeviceClass.init() (now called realize()), already. My question was
about the difference between DeviceClass.init() and
DeviceClass.realize().


> 
> In the isa patch after all the boring QOM'ish preparations, isa-device's
> class_init overwrites DeviceClass:realize, thereby no longer invoking
> any DeviceClass::init callback for the ISA devices. I2C, PCI, SysBus,
> etc. would need to be done in further steps, but a) this is work, b) the
> series shouldn't get too long for review/merge and c) it all depends on
> in which class and thereby with which signature we want to have realize
> - DeviceClass::realize(DeviceState *, Error **) or
> ObjectClass::realize(Object *, Error **).

So, just to check if I understood all correctly:

DeviceClass.init() is just the old and deprecated version of
DeviceClass.realize() (just because the former has no Error parameter),
but both have exactly the same semantics/purpose, and devices should now
use DeviceClass.realize() instead. Correct?
Andreas Färber - Dec. 12, 2012, 6:25 p.m.
Am 12.12.2012 19:16, schrieb Eduardo Habkost:
> So, just to check if I understood all correctly:
> 
> DeviceClass.init() is just the old and deprecated version of
> DeviceClass.realize() (just because the former has no Error parameter),
> but both have exactly the same semantics/purpose, and devices should now
> use DeviceClass.realize() instead. Correct?

Almost! Some qdev initfns today do things that instance_init should do
and realizefn shouldn't. Creating child devices in qdev initfn tampers
with our future ability to set realized = true recursively.

The other difference that I see (RFC) is that qdev uses an
only-the-instance's-class-sets-init approach, whereas with QOM any class
in the hierarchy can set/override the "virtual" method but needs to take
care of storing and (if desired) calling the parent's version itself,
i.e. an inversion of control towards child classes.

Andreas
Eduardo Habkost - Dec. 12, 2012, 6:44 p.m.
On Wed, Dec 12, 2012 at 07:25:01PM +0100, Andreas Färber wrote:
> Am 12.12.2012 19:16, schrieb Eduardo Habkost:
> > So, just to check if I understood all correctly:
> > 
> > DeviceClass.init() is just the old and deprecated version of
> > DeviceClass.realize() (just because the former has no Error parameter),
> > but both have exactly the same semantics/purpose, and devices should now
> > use DeviceClass.realize() instead. Correct?
> 
> Almost! Some qdev initfns today do things that instance_init should do
> and realizefn shouldn't. Creating child devices in qdev initfn tampers
> with our future ability to set realized = true recursively.

In those cases, where should the child device cration be moved to?

Whatever the answer is, can we have those subtle differences documented
inside DeviceClass?

I specifically mean documenting the purpose of the init/realize/unrealize
function pointers and their semantics/requirements for subclasses, not
just documenting the semantics of the "realized" property and
device_set_realized(). Knowing what device_set_realize() and qdev_init()
are supposed to do when all classes already do what they are supposed to
is easy. Knowing what exactly subclasses can/can't to do when setting
init/realize/unrealize functions is a bit more complicated.

> 
> The other difference that I see (RFC) is that qdev uses an
> only-the-instance's-class-sets-init approach, whereas with QOM any class
> in the hierarchy can set/override the "virtual" method but needs to take
> care of storing and (if desired) calling the parent's version itself,
> i.e. an inversion of control towards child classes.

I see. In theory we could change DeviceClass.init() to have the same
requirements/semantic, but then we would need to change every class to
save the parent's init() function and call it.

Patch

diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index f0d3a5e..580a811 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -29,6 +29,8 @@  enum {
 typedef int (*qdev_initfn)(DeviceState *dev);
 typedef int (*qdev_event)(DeviceState *dev);
 typedef void (*qdev_resetfn)(DeviceState *dev);
+typedef void (*DeviceRealize)(DeviceState *dev, Error **err);
+typedef void (*DeviceUnrealize)(DeviceState *dev, Error **err);
 
 struct VMStateDescription;
 
@@ -47,6 +49,8 @@  typedef struct DeviceClass {
     const struct VMStateDescription *vmsd;
 
     /* Private to qdev / bus.  */
+    DeviceRealize realize;
+    DeviceUnrealize unrealize;
     qdev_initfn init;
     qdev_event unplug;
     qdev_event exit;
diff --git a/hw/qdev.c b/hw/qdev.c
index bce6ad5..d7b6320 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -147,37 +147,30 @@  DeviceState *qdev_try_create(BusState *bus, const char *type)
    Return 0 on success.  */
 int qdev_init(DeviceState *dev)
 {
-    DeviceClass *dc = DEVICE_GET_CLASS(dev);
-    int rc;
+    Error *local_err = NULL;
 
     assert(!dev->realized);
 
-    rc = dc->init(dev);
-    if (rc < 0) {
+    object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
+    if (local_err != NULL) {
+        error_free(local_err);
         object_delete(OBJECT(dev));
-        return rc;
+        return -1;
     }
+    return 0;
+}
 
-    if (!OBJECT(dev)->parent) {
-        static int unattached_count = 0;
-        gchar *name = g_strdup_printf("device[%d]", unattached_count++);
-
-        object_property_add_child(container_get(qdev_get_machine(),
-                                                "/unattached"),
-                                  name, OBJECT(dev), NULL);
-        g_free(name);
-    }
+static void device_realize(DeviceState *dev, Error **err)
+{
+    DeviceClass *dc = DEVICE_GET_CLASS(dev);
 
-    if (qdev_get_vmsd(dev)) {
-        vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
-                                       dev->instance_id_alias,
-                                       dev->alias_required_for_version);
-    }
-    dev->realized = true;
-    if (dev->hotplugged) {
-        device_reset(dev);
+    if (dc->init) {
+        int rc = dc->init(dev);
+        if (rc < 0) {
+            error_setg(err, "Device initialization failed.");
+            return;
+        }
     }
-    return 0;
 }
 
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
@@ -649,6 +642,55 @@  void qdev_property_add_static(DeviceState *dev, Property *prop,
     assert_no_error(local_err);
 }
 
+static bool device_get_realized(Object *obj, Error **err)
+{
+    DeviceState *dev = DEVICE(obj);
+    return dev->realized;
+}
+
+static void device_set_realized(Object *obj, bool value, Error **err)
+{
+    DeviceState *dev = DEVICE(obj);
+    DeviceClass *dc = DEVICE_GET_CLASS(dev);
+    Error *local_err = NULL;
+
+    if (value && !dev->realized) {
+        if (dc->realize) {
+            dc->realize(dev, &local_err);
+        }
+
+        if (!obj->parent && local_err == NULL) {
+            static int unattached_count;
+            gchar *name = g_strdup_printf("device[%d]", unattached_count++);
+
+            object_property_add_child(container_get(qdev_get_machine(),
+                                                    "/unattached"),
+                                      name, obj, &local_err);
+            g_free(name);
+        }
+
+        if (qdev_get_vmsd(dev) && local_err == NULL) {
+            vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
+                                           dev->instance_id_alias,
+                                           dev->alias_required_for_version);
+        }
+        if (dev->hotplugged && local_err == NULL) {
+            device_reset(dev);
+        }
+    } else if (!value && dev->realized) {
+        if (dc->unrealize) {
+            dc->unrealize(dev, &local_err);
+        }
+    }
+
+    if (local_err != NULL) {
+        error_propagate(err, local_err);
+        return;
+    }
+
+    dev->realized = value;
+}
+
 static void device_initfn(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
@@ -663,6 +705,9 @@  static void device_initfn(Object *obj)
     dev->instance_id_alias = -1;
     dev->realized = false;
 
+    object_property_add_bool(obj, "realized",
+                             device_get_realized, device_set_realized, NULL);
+
     class = object_get_class(OBJECT(dev));
     do {
         for (prop = DEVICE_CLASS(class)->props; prop && prop->name; prop++) {
@@ -714,6 +759,12 @@  static void device_class_base_init(ObjectClass *class, void *data)
     klass->props = NULL;
 }
 
+static void device_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    dc->realize = device_realize;
+}
+
 void device_reset(DeviceState *dev)
 {
     DeviceClass *klass = DEVICE_GET_CLASS(dev);
@@ -734,13 +785,14 @@  Object *qdev_get_machine(void)
     return dev;
 }
 
-static TypeInfo device_type_info = {
+static const TypeInfo device_type_info = {
     .name = TYPE_DEVICE,
     .parent = TYPE_OBJECT,
     .instance_size = sizeof(DeviceState),
     .instance_init = device_initfn,
     .instance_finalize = device_finalize,
     .class_base_init = device_class_base_init,
+    .class_init = device_class_init,
     .abstract = true,
     .class_size = sizeof(DeviceClass),
 };