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

[Igor Mammedov]

On Tue, 20 Feb 2018 13:13:49 +0100
Paolo Bonzini <address@hidden> 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