[1/2] hw/sysbus.h: New sysbus_init_child() helper function

Message ID 20180216134516.6269-2-peter.maydell@linaro.org
State New
Headers show
Series
  • Reduce QOM boilerplate with sysbus_init_child() helper
Related show

Commit Message

Peter Maydell Feb. 16, 2018, 1:45 p.m.
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

Comments

Igor Mammedov Feb. 16, 2018, 4:28 p.m. | #1
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);
Philippe Mathieu-Daudé Feb. 16, 2018, 5:40 p.m. | #2
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.

> 

>> +}

>> +
Paolo Bonzini Feb. 20, 2018, 12:13 p.m. | #3
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
Igor Mammedov Feb. 20, 2018, 1:23 p.m. | #4
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
Philippe Mathieu-Daudé March 24, 2018, 3:35 p.m. | #5
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.
Igor Mammedov March 26, 2018, 12:41 p.m. | #6
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.

Patch

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);