Re: [Qemu-devel] [PATCH 10/14] qbus: move get_dev_path to DeviceState

[Paolo Bonzini <pbonzini@redhat.com>]

Il 01/05/2012 20:18, Anthony Liguori ha scritto:
> It should have never been a bus method.

If in the long term you want slots to be child properties in the bus,
the method _will_ actually belong to buses.

It is clear cut for print_dev, but not so much for the others.

Paolo

> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/pci.c      |   75 ++++++++++++++++++++++++++++-----------------------------
>  hw/qdev.c     |   11 ++------
>  hw/qdev.h     |    2 +-
>  hw/scsi-bus.c |   10 ++++----
>  hw/usb/bus.c  |   41 +++++++++++++++----------------
>  5 files changed, 66 insertions(+), 73 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index bff303b..291181e 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -40,7 +40,6 @@
>  #endif
>  
>  static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
> -static char *pcibus_get_dev_path(DeviceState *dev);
>  static char *pcibus_get_fw_dev_path(DeviceState *dev);
>  static int pcibus_reset(BusState *qbus);
>  
> @@ -49,7 +48,6 @@ static void pci_bus_class_init(ObjectClass *klass, void *data)
>      BusClass *k = BUS_CLASS(klass);
>  
>      k->print_dev = pcibus_dev_print;
> -    k->get_dev_path = pcibus_get_dev_path;
>      k->get_fw_dev_path = pcibus_get_fw_dev_path;
>      k->reset = pcibus_reset;
>  }
> @@ -1899,7 +1897,42 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev)
>      return strdup(path);
>  }
>  
> -static char *pcibus_get_dev_path(DeviceState *dev)
> +static int pci_qdev_find_recursive(PCIBus *bus,
> +                                   const char *id, PCIDevice **pdev)
> +{
> +    DeviceState *qdev = qdev_find_recursive(&bus->qbus, id);
> +    if (!qdev) {
> +        return -ENODEV;
> +    }
> +
> +    /* roughly check if given qdev is pci device */
> +    if (object_dynamic_cast(OBJECT(qdev), TYPE_PCI_DEVICE)) {
> +        *pdev = PCI_DEVICE(qdev);
> +        return 0;
> +    }
> +    return -EINVAL;
> +}
> +
> +int pci_qdev_find_device(const char *id, PCIDevice **pdev)
> +{
> +    struct PCIHostBus *host;
> +    int rc = -ENODEV;
> +
> +    QLIST_FOREACH(host, &host_buses, next) {
> +        int tmp = pci_qdev_find_recursive(host->bus, id, pdev);
> +        if (!tmp) {
> +            rc = 0;
> +            break;
> +        }
> +        if (tmp != -ENODEV) {
> +            rc = tmp;
> +        }
> +    }
> +
> +    return rc;
> +}
> +
> +static char *pci_qdev_get_dev_path(DeviceState *dev)
>  {
>      PCIDevice *d = container_of(dev, PCIDevice, qdev);
>      PCIDevice *t;
> @@ -1948,41 +1981,6 @@ static char *pcibus_get_dev_path(DeviceState *dev)
>      return path;
>  }
>  
> -static int pci_qdev_find_recursive(PCIBus *bus,
> -                                   const char *id, PCIDevice **pdev)
> -{
> -    DeviceState *qdev = qdev_find_recursive(&bus->qbus, id);
> -    if (!qdev) {
> -        return -ENODEV;
> -    }
> -
> -    /* roughly check if given qdev is pci device */
> -    if (object_dynamic_cast(OBJECT(qdev), TYPE_PCI_DEVICE)) {
> -        *pdev = PCI_DEVICE(qdev);
> -        return 0;
> -    }
> -    return -EINVAL;
> -}
> -
> -int pci_qdev_find_device(const char *id, PCIDevice **pdev)
> -{
> -    struct PCIHostBus *host;
> -    int rc = -ENODEV;
> -
> -    QLIST_FOREACH(host, &host_buses, next) {
> -        int tmp = pci_qdev_find_recursive(host->bus, id, pdev);
> -        if (!tmp) {
> -            rc = 0;
> -            break;
> -        }
> -        if (tmp != -ENODEV) {
> -            rc = tmp;
> -        }
> -    }
> -
> -    return rc;
> -}
> -
>  MemoryRegion *pci_address_space(PCIDevice *dev)
>  {
>      return dev->bus->address_space_mem;
> @@ -2000,6 +1998,7 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
>      k->unplug = pci_unplug_device;
>      k->exit = pci_unregister_device;
>      k->bus_type = TYPE_PCI_BUS;
> +    k->get_dev_path = pci_qdev_get_dev_path;
>  }
>  
>  static Property pci_bus_properties[] = {
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 87ff1a5..eaa3e12 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -528,15 +528,10 @@ char* qdev_get_fw_dev_path(DeviceState *dev)
>  
>  char *qdev_get_dev_path(DeviceState *dev)
>  {
> -    BusClass *bc;
> -
> -    if (!dev->parent_bus) {
> -        return NULL;
> -    }
> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>  
> -    bc = BUS_GET_CLASS(dev->parent_bus);
> -    if (bc->get_dev_path) {
> -        return bc->get_dev_path(dev);
> +    if (dc->get_dev_path) {
> +        return dc->get_dev_path(dev);
>      }
>  
>      return NULL;
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 8ac703e..30bfbef 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -47,6 +47,7 @@ typedef struct DeviceClass {
>  
>      /* callbacks */
>      void (*reset)(DeviceState *dev);
> +    char *(*get_dev_path)(DeviceState *dev);
>  
>      /* device state */
>      const VMStateDescription *vmsd;
> @@ -95,7 +96,6 @@ struct BusClass {
>  
>      /* 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);
>  };
> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
> index 9949192..38189f3 100644
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -16,7 +16,6 @@ static void scsi_bus_class_init(ObjectClass *klass, void *data)
>  {
>      BusClass *k = BUS_CLASS(klass);
>  
> -    k->get_dev_path = scsibus_get_dev_path;
>      k->get_fw_dev_path = scsibus_get_fw_dev_path;
>  }
>  
> @@ -1428,15 +1427,15 @@ void scsi_device_purge_requests(SCSIDevice *sdev, SCSISense sense)
>      sdev->unit_attention = sense;
>  }
>  
> -static char *scsibus_get_dev_path(DeviceState *dev)
> +static char *scsi_qdev_get_dev_path(DeviceState *dev)
>  {
> -    SCSIDevice *d = DO_UPCAST(SCSIDevice, qdev, dev);
> +    SCSIDevice *d = SCSI_DEVICE(dev);
>      DeviceState *hba = dev->parent_bus->parent;
>      char *id = NULL;
>      char *path;
>  
> -    if (hba && hba->parent_bus && hba->parent_bus->info->get_dev_path) {
> -        id = hba->parent_bus->info->get_dev_path(hba);
> +    if (hba) {
> +        id = qdev_get_dev_path(hba);
>      }
>      if (id) {
>          path = g_strdup_printf("%s/%d:%d:%d", id, d->channel, d->id, d->lun);
> @@ -1579,6 +1578,7 @@ static void scsi_device_class_init(ObjectClass *klass, void *data)
>      k->init     = scsi_qdev_init;
>      k->unplug   = qdev_simple_unplug_cb;
>      k->exit     = scsi_qdev_exit;
> +    k->get_dev_path = scsi_qdev_get_dev_path;
>  }
>  
>  static Property scsi_bus_properties[] = {
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index e2a87ed..9b57d1c 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -7,7 +7,6 @@
>  
>  static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
>  
> -static char *usb_get_dev_path(DeviceState *dev);
>  static char *usb_get_fw_dev_path(DeviceState *qdev);
>  static int usb_qdev_exit(DeviceState *qdev);
>  
> @@ -18,7 +17,6 @@ static void usb_bus_class_init(ObjectClass *klass, void *data)
>      BusClass *k = BUS_CLASS(klass);
>  
>      k->print_dev = usb_bus_dev_print;
> -    k->get_dev_path = usb_get_dev_path;
>      k->get_fw_dev_path = usb_get_fw_dev_path;
>  }
>  
> @@ -464,25 +462,6 @@ static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
>                     dev->attached ? ", attached" : "");
>  }
>  
> -static char *usb_get_dev_path(DeviceState *qdev)
> -{
> -    USBDevice *dev = USB_DEVICE(qdev);
> -    DeviceState *hcd = qdev->parent_bus->parent;
> -    char *id = NULL;
> -
> -    if ((dev->flags & (1 << USB_DEV_FLAG_FULL_PATH)) &&
> -        hcd && hcd->parent_bus && hcd->parent_bus->info->get_dev_path) {
> -        id = hcd->parent_bus->info->get_dev_path(hcd);
> -    }
> -    if (id) {
> -        char *ret = g_strdup_printf("%s/%s", id, dev->port->path);
> -        g_free(id);
> -        return ret;
> -    } else {
> -        return g_strdup(dev->port->path);
> -    }
> -}
> -
>  static char *usb_get_fw_dev_path(DeviceState *qdev)
>  {
>      USBDevice *dev = USB_DEVICE(qdev);
> @@ -578,13 +557,33 @@ USBDevice *usbdevice_create(const char *cmdline)
>      return f->usbdevice_init(bus, params);
>  }
>  
> +static char *usb_qdev_get_dev_path(DeviceState *qdev)
> +{
> +    USBDevice *dev = USB_DEVICE(qdev);
> +    DeviceState *hcd = qdev->parent_bus->parent;
> +    char *id = NULL;
> +
> +    if ((dev->flags & (1 << USB_DEV_FLAG_FULL_PATH))) {
> +        id = qdev_get_dev_path(hcd);
> +    }
> +    if (id) {
> +        char *ret = g_strdup_printf("%s/%s", id, dev->port->path);
> +        g_free(id);
> +        return ret;
> +    } else {
> +        return g_strdup(dev->port->path);
> +    }
> +}
> +
>  static void usb_device_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *k = DEVICE_CLASS(klass);
> +
>      k->bus_type = TYPE_USB_BUS;
>      k->init     = usb_qdev_init;
>      k->unplug   = qdev_simple_unplug_cb;
>      k->exit     = usb_qdev_exit;
> +    k->get_dev_path = usb_qdev_get_dev_path;
>  }
>  
>  static Property usb_bus_properties[] = {