| Message ID | 20180216134516.6269-2-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Reduce QOM boilerplate with sysbus_init_child() helper | expand |
On Fri, 16 Feb 2018 13:45:15 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > If you're using the increasingly-common QOM style of > having container devices create their child objects > in-place, you end up with a lot of boilerplate in the > container's init function: > > object_initialize() to init the child > object_property_add_child() to make the child a > child of the parent > qdev_set_parent_bus() to put the child on the > sysbus default bus > > If you forget the second of these then things sort of > work but trying to add a child to the child will segfault; > if you forget the third then the device won't get reset. > > Provide a helper function sysbus_init_child() which > does all these things for you, reducing the boilerplate > and making it harder to get wrong. > > Code that used to look like this: > object_initialize(&s->ic, sizeof(s->ic), TYPE_BCM2835_IC); > object_property_add_child(obj, "ic", OBJECT(&s->ic), NULL); > qdev_set_parent_bus(DEVICE(&s->ic), sysbus_get_default()); > can now look like this: > sysbus_init_child(obj, "ic", &s->ic, sizeof(s->ic), TYPE_BCM2835_IC); > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > include/hw/sysbus.h | 12 ++++++++++++ > hw/core/sysbus.c | 14 ++++++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h > index e88bb6dae0..6095e24e86 100644 > --- a/include/hw/sysbus.h > +++ b/include/hw/sysbus.h > @@ -118,4 +118,16 @@ static inline DeviceState *sysbus_try_create_simple(const char *name, > return sysbus_try_create_varargs(name, addr, irq, NULL); > } > > +/** > + * sysbus_init_child: in-place initialize and parent a SysBus device > + * @parent: object to parent the device to > + * @childname: name to use as the child property name > + * @child: child object > + * @childsize: size of the storage for the object > + * @childtype: type name of the child > + */ > +void sysbus_init_child(Object *parent, const char *childname, > + void *child, size_t childsize, > + const char *childtype); > + > #endif /* HW_SYSBUS_H */ > diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c > index 5d0887f499..acfa52dc68 100644 > --- a/hw/core/sysbus.c > +++ b/hw/core/sysbus.c > @@ -21,6 +21,7 @@ > #include "hw/sysbus.h" > #include "monitor/monitor.h" > #include "exec/address-spaces.h" > +#include "qapi/error.h" > > static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent); > static char *sysbus_get_fw_dev_path(DeviceState *dev); > @@ -372,6 +373,19 @@ BusState *sysbus_get_default(void) > return main_system_bus; > } > > +void sysbus_init_child(Object *parent, const char *childname, > + void *child, size_t childsize, > + const char *childtype) > +{ > + object_initialize(child, childsize, childtype); > + /* error_abort is fine here because this can only fail for > + * programming-error reasons: child already parented, or > + * parent already has a child with the given name. > + */ > + object_property_add_child(parent, childname, OBJECT(child), &error_abort); It would be useful not only for sysbus devices. maybe we should extend object_initialize instead, something like this: void object_initialize(void *data, size_t size, const char *typename, Object *parent, const char *name) and set parent in it. git counts about 152 uses, so it would be tree wide change but still not too much. > + qdev_set_parent_bus(DEVICE(child), sysbus_get_default()); and then assuming we don't create sysbus devices, nor should be able to, which are not attached to sysbus_get_default() this one can go sysbus' instance_init() then there won't be need for sysbus specific helper, inheritance will do the rest of the job. > +} > + > static void sysbus_register_types(void) > { > type_register_static(&system_bus_info);
On 02/16/2018 01:28 PM, Igor Mammedov wrote: > On Fri, 16 Feb 2018 13:45:15 +0000 > Peter Maydell <peter.maydell@linaro.org> wrote: ... >> static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent); >> static char *sysbus_get_fw_dev_path(DeviceState *dev); >> @@ -372,6 +373,19 @@ BusState *sysbus_get_default(void) >> return main_system_bus; >> } >> >> +void sysbus_init_child(Object *parent, const char *childname, >> + void *child, size_t childsize, >> + const char *childtype) >> +{ > > >> + object_initialize(child, childsize, childtype); >> + /* error_abort is fine here because this can only fail for >> + * programming-error reasons: child already parented, or >> + * parent already has a child with the given name. >> + */ >> + object_property_add_child(parent, childname, OBJECT(child), &error_abort); > It would be useful not only for sysbus devices. > maybe we should extend object_initialize instead, > something like this: > void object_initialize(void *data, size_t size, const char *typename, > Object *parent, const char *name) > and set parent in it. > git counts about 152 uses, so it would be tree wide change > but still not too much. we can keep object_initialize() when no parent, and add object_initialize_child(, const char *childname, Object *parent) 'parent' last because all previous args are child-related. > > >> + qdev_set_parent_bus(DEVICE(child), sysbus_get_default()); > and then assuming we don't create sysbus devices, nor should be able to, > which are not attached to sysbus_get_default() this one can go sysbus' instance_init() good idea. > > then there won't be need for sysbus specific helper, > inheritance will do the rest of the job. > >> +} >> +
On 16/02/2018 18:40, Philippe Mathieu-Daudé wrote: > we can keep object_initialize() when no parent, > and add object_initialize_child(, const char *childname, Object *parent) > 'parent' last because all previous args are child-related. > >> >>> + qdev_set_parent_bus(DEVICE(child), sysbus_get_default()); >> and then assuming we don't create sysbus devices, nor should be able to, >> which are not attached to sysbus_get_default() this one can go sysbus' instance_init() > good idea. > Either last or first, like object_initialize_child(Object *parent, const char *childname, void *data, size_t size, const char *typename); I'm not sure about moving qdev_set_parent_bus to instance_init(). It would be a bit different from other buses and possibly confusing. Potentially there could be a "hierarchy" of *_initialize_child functions, adding or removing arguments as needed for the specific kind of bus: /* adds qdev_set_parent_bus */ device_initialize_child(Object *parent, const char *childname, BusState *bus, void *data, size_t size, const char *typename); /* uses sysbus_get_default() */ sysbus_initialize_child(Object *parent, const char *childname, void *data, size_t size, const char *typename); /* initializes "addr" property too */ pci_initialize_child(Object *parent, const char *childname, BusState *bus, int addr, void *data, size_t size, const char *typename); Thanks, Paolo
On Tue, 20 Feb 2018 13:13:49 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 16/02/2018 18:40, Philippe Mathieu-Daudé wrote: > > we can keep object_initialize() when no parent, > > and add object_initialize_child(, const char *childname, Object *parent) > > 'parent' last because all previous args are child-related. > > > >> > >>> + qdev_set_parent_bus(DEVICE(child), sysbus_get_default()); > >> and then assuming we don't create sysbus devices, nor should be able to, > >> which are not attached to sysbus_get_default() this one can go sysbus' instance_init() > > good idea. > > [...] > I'm not sure about moving qdev_set_parent_bus to instance_init(). It > would be a bit different from other buses and possibly confusing. That's what this series sort of does, i.e. creating a type/class specific helper(s). Which becomes confusing as number of helpers increases (frankly it's just 2 different approaches to code i.e. functional vs OOM). It could be better to keep single QOM API and let SYSBUS base class instance_init() to do all implicit initialization that must be done for inherited classes. Yes, it will be different from other devices with buses but users won't really care (there is no other buss to assign to), for them it will look like a typical bus-less device and it would be less error-prone to code. > Potentially there could be a "hierarchy" of *_initialize_child > functions, adding or removing arguments as needed for the specific kind > of bus: > > /* adds qdev_set_parent_bus */ > device_initialize_child(Object *parent, const char *childname, > BusState *bus, void *data, size_t size, > const char *typename); > /* uses sysbus_get_default() */ > sysbus_initialize_child(Object *parent, const char *childname, > void *data, size_t size, > const char *typename); > /* initializes "addr" property too */ > pci_initialize_child(Object *parent, const char *childname, > BusState *bus, int addr, > void *data, size_t size, > const char *typename); PCI could also incorporate PCI specific bus setting/ verification logic inside of base class. It could allow us to drop bus assigning magic in qdev_device_add(), bringing it closer to simple QOM object handling, with specifics abstracted by respective TYPE implementations. Maybe we would be able to unify -device with -object in the end. > Thanks, > > Paolo > qdev_device_add
Hi, On 02/20/2018 10:23 AM, Igor Mammedov wrote: > On Tue, 20 Feb 2018 13:13:49 +0100 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> On 16/02/2018 18:40, Philippe Mathieu-Daudé wrote: >>> we can keep object_initialize() when no parent, >>> and add object_initialize_child(, const char *childname, Object *parent) >>> 'parent' last because all previous args are child-related. >>> >>>> >>>>> + qdev_set_parent_bus(DEVICE(child), sysbus_get_default()); >>>> and then assuming we don't create sysbus devices, nor should be able to, >>>> which are not attached to sysbus_get_default() this one can go sysbus' instance_init() >>> good idea. >>> > [...] > >> I'm not sure about moving qdev_set_parent_bus to instance_init(). It >> would be a bit different from other buses and possibly confusing. > That's what this series sort of does, i.e. creating a type/class > specific helper(s). Which becomes confusing as number of helpers > increases (frankly it's just 2 different approaches to code i.e. > functional vs OOM). > > It could be better to keep single QOM API and let SYSBUS base class > instance_init() to do all implicit initialization that must be done > for inherited classes. What is the consensus on this series once 2.12 gets released? > Yes, it will be different from other devices with buses but > users won't really care (there is no other buss to assign to), > for them it will look like a typical bus-less device and it > would be less error-prone to code. > > >> Potentially there could be a "hierarchy" of *_initialize_child >> functions, adding or removing arguments as needed for the specific kind >> of bus: >> >> /* adds qdev_set_parent_bus */ >> device_initialize_child(Object *parent, const char *childname, >> BusState *bus, void *data, size_t size, >> const char *typename); >> /* uses sysbus_get_default() */ >> sysbus_initialize_child(Object *parent, const char *childname, >> void *data, size_t size, >> const char *typename); > > >> /* initializes "addr" property too */ >> pci_initialize_child(Object *parent, const char *childname, >> BusState *bus, int addr, >> void *data, size_t size, >> const char *typename); > PCI could also incorporate PCI specific bus setting/ > verification logic inside of base class. > > It could allow us to drop bus assigning magic in qdev_device_add(), > bringing it closer to simple QOM object handling, with specifics > abstracted by respective TYPE implementations. > Maybe we would be able to unify -device with -object in the end. Thanks, Phil.
On Sat, 24 Mar 2018 12:35:22 -0300 Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Hi, > > On 02/20/2018 10:23 AM, Igor Mammedov wrote: > > On Tue, 20 Feb 2018 13:13:49 +0100 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > >> On 16/02/2018 18:40, Philippe Mathieu-Daudé wrote: > >>> we can keep object_initialize() when no parent, > >>> and add object_initialize_child(, const char *childname, Object *parent) > >>> 'parent' last because all previous args are child-related. > >>> > >>>> > >>>>> + qdev_set_parent_bus(DEVICE(child), sysbus_get_default()); > >>>> and then assuming we don't create sysbus devices, nor should be able to, > >>>> which are not attached to sysbus_get_default() this one can go sysbus' instance_init() > >>> good idea. > >>> > > [...] > > > >> I'm not sure about moving qdev_set_parent_bus to instance_init(). It > >> would be a bit different from other buses and possibly confusing. > > That's what this series sort of does, i.e. creating a type/class > > specific helper(s). Which becomes confusing as number of helpers > > increases (frankly it's just 2 different approaches to code i.e. > > functional vs OOM). > > > > It could be better to keep single QOM API and let SYSBUS base class > > instance_init() to do all implicit initialization that must be done > > for inherited classes. > > What is the consensus on this series once 2.12 gets released? At least we should add and make current code use it object_initialize_child() It should save quite a bit of code in ARM target As for qdev_set_parent_bus(DEVICE, sysbus_get_default()) it is a separate issue, but I'd still go with TYPE_SYS_BUS_DEVICE setting bus in its instance_init so that inherited types nor their users would have to deal with it. > > Yes, it will be different from other devices with buses but > > users won't really care (there is no other buss to assign to), > > for them it will look like a typical bus-less device and it > > would be less error-prone to code. > > > > > >> Potentially there could be a "hierarchy" of *_initialize_child > >> functions, adding or removing arguments as needed for the specific kind > >> of bus: > >> > >> /* adds qdev_set_parent_bus */ > >> device_initialize_child(Object *parent, const char *childname, > >> BusState *bus, void *data, size_t size, > >> const char *typename); > >> /* uses sysbus_get_default() */ > >> sysbus_initialize_child(Object *parent, const char *childname, > >> void *data, size_t size, > >> const char *typename); > > > > > >> /* initializes "addr" property too */ > >> pci_initialize_child(Object *parent, const char *childname, > >> BusState *bus, int addr, > >> void *data, size_t size, > >> const char *typename); > > PCI could also incorporate PCI specific bus setting/ > > verification logic inside of base class. > > > > It could allow us to drop bus assigning magic in qdev_device_add(), > > bringing it closer to simple QOM object handling, with specifics > > abstracted by respective TYPE implementations. > > Maybe we would be able to unify -device with -object in the end. > > Thanks, > > Phil.
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h index e88bb6dae0..6095e24e86 100644 --- a/include/hw/sysbus.h +++ b/include/hw/sysbus.h @@ -118,4 +118,16 @@ static inline DeviceState *sysbus_try_create_simple(const char *name, return sysbus_try_create_varargs(name, addr, irq, NULL); } +/** + * sysbus_init_child: in-place initialize and parent a SysBus device + * @parent: object to parent the device to + * @childname: name to use as the child property name + * @child: child object + * @childsize: size of the storage for the object + * @childtype: type name of the child + */ +void sysbus_init_child(Object *parent, const char *childname, + void *child, size_t childsize, + const char *childtype); + #endif /* HW_SYSBUS_H */ diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c index 5d0887f499..acfa52dc68 100644 --- a/hw/core/sysbus.c +++ b/hw/core/sysbus.c @@ -21,6 +21,7 @@ #include "hw/sysbus.h" #include "monitor/monitor.h" #include "exec/address-spaces.h" +#include "qapi/error.h" static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent); static char *sysbus_get_fw_dev_path(DeviceState *dev); @@ -372,6 +373,19 @@ BusState *sysbus_get_default(void) return main_system_bus; } +void sysbus_init_child(Object *parent, const char *childname, + void *child, size_t childsize, + const char *childtype) +{ + object_initialize(child, childsize, childtype); + /* error_abort is fine here because this can only fail for + * programming-error reasons: child already parented, or + * parent already has a child with the given name. + */ + object_property_add_child(parent, childname, OBJECT(child), &error_abort); + qdev_set_parent_bus(DEVICE(child), sysbus_get_default()); +} + static void sysbus_register_types(void) { type_register_static(&system_bus_info);
If you're using the increasingly-common QOM style of having container devices create their child objects in-place, you end up with a lot of boilerplate in the container's init function: object_initialize() to init the child object_property_add_child() to make the child a child of the parent qdev_set_parent_bus() to put the child on the sysbus default bus If you forget the second of these then things sort of work but trying to add a child to the child will segfault; if you forget the third then the device won't get reset. Provide a helper function sysbus_init_child() which does all these things for you, reducing the boilerplate and making it harder to get wrong. Code that used to look like this: object_initialize(&s->ic, sizeof(s->ic), TYPE_BCM2835_IC); object_property_add_child(obj, "ic", OBJECT(&s->ic), NULL); qdev_set_parent_bus(DEVICE(&s->ic), sysbus_get_default()); can now look like this: sysbus_init_child(obj, "ic", &s->ic, sizeof(s->ic), TYPE_BCM2835_IC); Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/hw/sysbus.h | 12 ++++++++++++ hw/core/sysbus.c | 14 ++++++++++++++ 2 files changed, 26 insertions(+) -- 2.16.1