Re: [Qemu-devel] [PATCH 1/2] hw/sysbus.h: New sysbus_init_child() helper function

[Igor Mammedov]

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