| Message ID | 1353888766-6951-5-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
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?
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
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?
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
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.
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), };