Re: [Qemu-devel] [PATCH 1/5] qdev: introduce qdev_create_kid(Object *parent, const char *type)

[liu ping fan]

On Tue, Jul 10, 2012 at 4:45 PM, Paolo Bonzini <address@hidden> wrote:
> Il 10/07/2012 08:16, Liu Ping Fan ha scritto:
>> DeviceState can be created as kid of DeviceState/CPUState, not neccesary
>> attached to bus. This will be helpful to simulate the real hardware
>> submodule which sits inside package.
>>
>> Signed-off-by: Liu Ping Fan <address@hidden>
>> ---
>>  hw/qdev.c |   28 ++++++++++++++++++++++++++++
>>  hw/qdev.h |    1 +
>>  2 files changed, 29 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index af54467..d2100a1 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -26,6 +26,7 @@
>>     this API directly.  */
>>
>>  #include "net.h"
>> +#include "qemu/cpu.h"
>>  #include "qdev.h"
>>  #include "sysemu.h"
>>  #include "error.h"
>> @@ -145,6 +146,33 @@ DeviceState *qdev_try_create(BusState *bus, const char 
>> *type)
>>      return dev;
>>  }
>>
>> +DeviceState *qdev_create_kid(Object *parent, const char *type)
>> +{
>> +    DeviceState *dev;
>> +    assert(parent);
>> +
>> +    if (object_class_by_name(type) == NULL) {
>> +        return NULL;
>> +    }
>> +
>> +    if (object_is_type_str(parent, TYPE_BUS)) {
>> +        return qdev_create(BUS(parent), type);
>> +    }
>> +
>> +    if (!object_is_type_str(parent, TYPE_DEVICE)
>> +        || !object_is_type_str(parent, TYPE_CPU)) {
>> +        return NULL;
>> +    }
>> +
>> +    dev = DEVICE(object_new(type));
>> +    if (!dev) {
>> +        return NULL;
>> +    }
>> +    object_property_add_child(OBJECT(parent), type, OBJECT(dev), NULL);
>
> I don't like this.  The only additional functionality is "magic"
> dispatching between qdev_create for buses and object_property_add_child
> for devices.  This should be done with a method that is implemented in
> both objects (e.g. an interface), not with type checks like this.
>
> However, you're not even using the functionality, and designing APIs
> without an effective need usually makes for bad APIs.
>
> Instead, you can just move APIC creation in the CPU, and use
> object_property_add_child there.
>
OK, I will move the creation in the CPU. But I think as part of qom,
DeviceState can have a DeviceState child, so there is need for wrapper
for the function.  Maybe just make the qdev_create_kid(Object*) ->
qdev_create_kid(DeviceState*) ?

regards,
pingfan

> Paolo
>
>> +    return dev;
>> +}
>> +
>>  /* Initialize a device.  Device properties should be set before calling
>>     this function.  IRQs and MMIO regions should be connected/mapped after
>>     calling this function.
>> diff --git a/hw/qdev.h b/hw/qdev.h
>> index f4683dc..aecc69e 100644
>> --- a/hw/qdev.h
>> +++ b/hw/qdev.h
>> @@ -154,6 +154,7 @@ typedef struct GlobalProperty {
>>
>>  DeviceState *qdev_create(BusState *bus, const char *name);
>>  DeviceState *qdev_try_create(BusState *bus, const char *name);
>> +DeviceState *qdev_create_kid(Object *parent, const char *type);
>>  bool qdev_exists(const char *name);
>>  int qdev_device_help(QemuOpts *opts);
>>  DeviceState *qdev_device_add(QemuOpts *opts);
>>
>
>