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

[Paolo Bonzini <pbonzini@redhat.com>]

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 <pingfank@linux.vnet.ibm.com>
> ---
>  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.

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