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

[Philippe Mathieu-Daudé]

Hi,

On 02/20/2018 10:23 AM, Igor Mammedov wrote:
> 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.

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.