Re: [Qemu-devel] [PATCH v4 04/24] scsi: introduce SCSIBusOps

[Blue Swirl]

On Mon, May 23, 2011 at 7:08 PM, Paolo Bonzini <address@hidden> wrote:
> There are more operations than a SCSI bus can handle, besides completing
> commands.  The current callback in fact is overloaded and can be called
> with two different meanings already.  Another example, which this series
> will introduce, is cleaning up after a request is cancelled.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> Cc: Christoph Hellwig <address@hidden>
> ---
>  hw/esp.c          |    6 +++++-
>  hw/lsi53c895a.c   |    6 +++++-
>  hw/scsi-bus.c     |   12 ++++++------
>  hw/scsi-generic.c |    2 +-
>  hw/scsi.h         |   13 +++++++------
>  hw/spapr_vscsi.c  |    6 +++++-
>  hw/usb-msd.c      |    6 +++++-
>  7 files changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/hw/esp.c b/hw/esp.c
> index fa9d2a2..d8bba7a 100644
> --- a/hw/esp.c
> +++ b/hw/esp.c
> @@ -714,6 +714,10 @@ void esp_init(target_phys_addr_t espaddr, int it_shift,
>     *dma_enable = qdev_get_gpio_in(dev, 1);
>  }
>
> +static struct SCSIBusOps esp_scsi_ops = {

It looks like structs like this could be made 'const'.

> +    .complete = esp_command_complete
> +};
> +
>  static int esp_init1(SysBusDevice *dev)
>  {
>     ESPState *s = FROM_SYSBUS(ESPState, dev);
> @@ -728,7 +732,7 @@ static int esp_init1(SysBusDevice *dev)
>
>     qdev_init_gpio_in(&dev->qdev, esp_gpio_demux, 2);
>
> -    scsi_bus_new(&s->bus, &dev->qdev, 0, ESP_MAX_DEVS, esp_command_complete);
> +    scsi_bus_new(&s->bus, &dev->qdev, 0, ESP_MAX_DEVS, &esp_scsi_ops);
>     return scsi_bus_legacy_handle_cmdline(&s->bus);
>  }
>
> diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
> index 2ce38a9..ccea6ad 100644
> --- a/hw/lsi53c895a.c
> +++ b/hw/lsi53c895a.c
> @@ -2205,6 +2205,10 @@ static int lsi_scsi_uninit(PCIDevice *d)
>     return 0;
>  }
>
> +static struct SCSIBusOps lsi_scsi_ops = {
> +    .complete = lsi_command_complete
> +};
> +
>  static int lsi_scsi_init(PCIDevice *dev)
>  {
>     LSIState *s = DO_UPCAST(LSIState, dev, dev);
> @@ -2241,7 +2245,7 @@ static int lsi_scsi_init(PCIDevice *dev)
>                            PCI_BASE_ADDRESS_SPACE_MEMORY, lsi_ram_mapfunc);
>     QTAILQ_INIT(&s->queue);
>
> -    scsi_bus_new(&s->bus, &dev->qdev, 1, LSI_MAX_DEVS, lsi_command_complete);
> +    scsi_bus_new(&s->bus, &dev->qdev, 1, LSI_MAX_DEVS, &lsi_scsi_ops);
>     if (!dev->qdev.hotplugged) {
>         return scsi_bus_legacy_handle_cmdline(&s->bus);
>     }
> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
> index 191cbab..f21704f 100644
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -21,13 +21,13 @@ static int next_scsi_bus;
>
>  /* Create a scsi bus, and attach devices to it.  */
>  void scsi_bus_new(SCSIBus *bus, DeviceState *host, int tcq, int ndev,
> -                  scsi_completionfn complete)
> +                  SCSIBusOps *ops)
>  {
>     qbus_create_inplace(&bus->qbus, &scsi_bus_info, host, NULL);
>     bus->busnr = next_scsi_bus++;
>     bus->tcq = tcq;
>     bus->ndev = ndev;
> -    bus->complete = complete;
> +    bus->ops = ops;
>     bus->qbus.allow_hotplug = 1;
>  }
>
> @@ -503,7 +503,7 @@ static const char *scsi_command_name(uint8_t cmd)
>  void scsi_req_data(SCSIRequest *req, int len)
>  {
>     trace_scsi_req_data(req->dev->id, req->lun, req->tag, len);
> -    req->bus->complete(req->bus, SCSI_REASON_DATA, req->tag, len);
> +    req->bus->ops->complete(req->bus, SCSI_REASON_DATA, req->tag, len);
>  }
>
>  void scsi_req_print(SCSIRequest *req)
> @@ -538,9 +538,9 @@ void scsi_req_complete(SCSIRequest *req)
>  {
>     assert(req->status != -1);
>     scsi_req_dequeue(req);
> -    req->bus->complete(req->bus, SCSI_REASON_DONE,
> -                       req->tag,
> -                       req->status);
> +    req->bus->ops->complete(req->bus, SCSI_REASON_DONE,
> +                            req->tag,
> +                            req->status);
>  }
>
>  static char *scsibus_get_fw_dev_path(DeviceState *dev)
> diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
> index e4f1f30..f09458b 100644
> --- a/hw/scsi-generic.c
> +++ b/hw/scsi-generic.c
> @@ -335,7 +335,7 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t 
> tag,
>         s->senselen = 7;
>         s->driver_status = SG_ERR_DRIVER_SENSE;
>         bus = scsi_bus_from_device(d);
> -        bus->complete(bus, SCSI_REASON_DONE, tag, CHECK_CONDITION);
> +        bus->ops->complete(bus, SCSI_REASON_DONE, tag, CHECK_CONDITION);
>         return 0;
>     }
>
> diff --git a/hw/scsi.h b/hw/scsi.h
> index 7c09f32..20cf397 100644
> --- a/hw/scsi.h
> +++ b/hw/scsi.h
> @@ -16,10 +16,9 @@ enum scsi_reason {
>  };
>
>  typedef struct SCSIBus SCSIBus;
> +typedef struct SCSIBusOps SCSIBusOps;
>  typedef struct SCSIDevice SCSIDevice;
>  typedef struct SCSIDeviceInfo SCSIDeviceInfo;
> -typedef void (*scsi_completionfn)(SCSIBus *bus, int reason, uint32_t tag,
> -                                  uint32_t arg);
>
>  enum SCSIXferMode {
>     SCSI_XFER_NONE,      /*  TEST_UNIT_READY, ...            */
> @@ -74,20 +73,22 @@ struct SCSIDeviceInfo {
>     uint8_t *(*get_buf)(SCSIDevice *s, uint32_t tag);
>  };
>
> -typedef void (*SCSIAttachFn)(DeviceState *host, BlockDriverState *bdrv,
> -              int unit);
> +struct SCSIBusOps {
> +    void (*complete)(SCSIBus *bus, int reason, uint32_t tag, uint32_t arg);
> +};
> +
>  struct SCSIBus {
>     BusState qbus;
>     int busnr;
>
>     int tcq, ndev;
> -    scsi_completionfn complete;
> +    SCSIBusOps *ops;
>
>     SCSIDevice *devs[MAX_SCSI_DEVS];
>  };
>
>  void scsi_bus_new(SCSIBus *bus, DeviceState *host, int tcq, int ndev,
> -                  scsi_completionfn complete);
> +                  SCSIBusOps *ops);
>  void scsi_qdev_register(SCSIDeviceInfo *info);
>
>  static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d)
> diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
> index 9928334..9f5c154 100644
> --- a/hw/spapr_vscsi.c
> +++ b/hw/spapr_vscsi.c
> @@ -907,6 +907,10 @@ static int vscsi_do_crq(struct VIOsPAPRDevice *dev, 
> uint8_t *crq_data)
>     return 0;
>  }
>
> +static struct SCSIBusOps vscsi_scsi_ops = {
> +    .complete = vscsi_command_complete
> +};
> +
>  static int spapr_vscsi_init(VIOsPAPRDevice *dev)
>  {
>     VSCSIState *s = DO_UPCAST(VSCSIState, vdev, dev);
> @@ -923,7 +927,7 @@ static int spapr_vscsi_init(VIOsPAPRDevice *dev)
>     dev->crq.SendFunc = vscsi_do_crq;
>
>     scsi_bus_new(&s->bus, &dev->qdev, 1, VSCSI_REQ_LIMIT,
> -                 vscsi_command_complete);
> +                 &vscsi_scsi_ops);
>     if (!dev->qdev.hotplugged) {
>         scsi_bus_legacy_handle_cmdline(&s->bus);
>     }
> diff --git a/hw/usb-msd.c b/hw/usb-msd.c
> index bd1c3a4..7a07a12 100644
> --- a/hw/usb-msd.c
> +++ b/hw/usb-msd.c
> @@ -487,6 +487,10 @@ static void usb_msd_password_cb(void *opaque, int err)
>         qdev_unplug(&s->dev.qdev);
>  }
>
> +static struct SCSIBusOps usb_msd_scsi_ops = {
> +    .complete = usb_msd_command_complete
> +};
> +
>  static int usb_msd_initfn(USBDevice *dev)
>  {
>     MSDState *s = DO_UPCAST(MSDState, dev, dev);
> @@ -516,7 +520,7 @@ static int usb_msd_initfn(USBDevice *dev)
>     }
>
>     usb_desc_init(dev);
> -    scsi_bus_new(&s->bus, &s->dev.qdev, 0, 1, usb_msd_command_complete);
> +    scsi_bus_new(&s->bus, &s->dev.qdev, 0, 1, &usb_msd_scsi_ops);
>     s->scsi_dev = scsi_bus_legacy_add_drive(&s->bus, bs, 0, !!s->removable);
>     if (!s->scsi_dev) {
>         return -1;
> --
> 1.7.4.4
>
>
>
>