On Fri, 16 Feb 2018 13:45:15 +0000 Peter Maydell <peter.mayd...@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.mayd...@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);