Re: [Qemu-devel] [PATCH 08/14] qdev: convert busses to QEMU Object Model

["Andreas Färber" <afaerber@suse.de>]

Am 01.05.2012 20:18, schrieb Anthony Liguori:
> This is far less interesting than it sounds.  We simply add an Object to each
> BusInfo and then register the types appropriately.  Most of the interesting
> refactoring will follow in the next patches.
> 
> Since we're changing fundamental type names (BusInfo -> BusClass), it all needs
> to convert at once.  Fortunately, not a lot of code is affected.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
[...]
> diff --git a/hw/intel-hda.c b/hw/intel-hda.c
> index 9405f54..58497e5 100644
> --- a/hw/intel-hda.c
> +++ b/hw/intel-hda.c
> @@ -29,16 +29,19 @@
>  /* --------------------------------------------------------------------- */
>  /* hda bus                                                               */
>  
> -static struct BusInfo hda_codec_bus_info = {
> -    .name      = "HDA",
> -    .size      = sizeof(HDACodecBus),
> +#define TYPE_HDA_BUS "HDA"

I stumbled over this being a pretty generic term for a type name.
"HDA-codec-bus" maybe?

> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index f5ee166..97673dd 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
[...]
> @@ -432,16 +433,16 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>          if (!bus) {
>              return NULL;
>          }
> -        if (bus->info != k->bus_info) {
> +        if (strcmp(object_get_typename(OBJECT(bus)), k->bus_type) != 0){

Space before {.

> diff --git a/hw/qdev.h b/hw/qdev.h
> index 6d2261f..8ac703e 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
[...]
> @@ -79,28 +79,30 @@ struct DeviceState {
>      int alias_required_for_version;
>  };
>  
> -typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent);
> -typedef char *(*bus_get_dev_path)(DeviceState *dev);
>  /*
>   * This callback is used to create Open Firmware device path in accordance with
>   * OF spec http://forthworks.com/standards/of1275.pdf. Indicidual bus bindings
>   * can be found here http://playground.sun.com/1275/bindings/.
>   */
> -typedef char *(*bus_get_fw_dev_path)(DeviceState *dev);
> -typedef int (qbus_resetfn)(BusState *bus);
>  
> -struct BusInfo {
> -    const char *name;
> -    size_t size;
> -    bus_dev_printfn print_dev;
> -    bus_get_dev_path get_dev_path;
> -    bus_get_fw_dev_path get_fw_dev_path;
> -    qbus_resetfn *reset;
> +#define TYPE_BUS "bus"
> +#define BUS(obj) OBJECT_CHECK(BusState, (obj), TYPE_BUS)
> +#define BUS_CLASS(klass) OBJECT_CLASS_CHECK(BusClass, (klass), TYPE_BUS)
> +#define BUS_GET_CLASS(obj) OBJECT_GET_CLASS(BusClass, (obj), TYPE_BUS)
> +
> +struct BusClass {
> +    ObjectClass parent_class;
> +
> +    /* FIXME first arg should be BusState */
> +    void (*print_dev)(Monitor *mon, DeviceState *dev, int indent);
> +    char *(*get_dev_path)(DeviceState *dev);
> +    char *(*get_fw_dev_path)(DeviceState *dev);
> +    int (*reset)(BusState *bus);
>  };
>  
>  struct BusState {
> +    Object obj;
>      DeviceState *parent;
> -    BusInfo *info;
>      const char *name;
>      int allow_hotplug;
>      int qdev_allocated;

Indicate what is /*< private >*/ and what not?

> @@ -321,7 +323,7 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>  char *qdev_get_fw_dev_path(DeviceState *dev);
>  
>  /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
> -extern struct BusInfo system_bus_info;
> +#define TYPE_SYSTEM_BUS "System"

"System-bus"?

Regarding the bus names, these have been merely moved here but I'm
raising the topic because they are now in the global name space where
they will potentially conflict with devices and other types.

Otherwise looks okay, thanks for your work on this.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg