Re: [Qemu-devel] [PATCH 10/13] Implement scsi device destruction

[Markus Armbruster <armbru@redhat.com>]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/scsi-bus.c     |   24 +++++++++++++++++++++---
>  hw/scsi-disk.c    |    6 ------
>  hw/scsi-generic.c |    2 --
>  3 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
> index 881e363..27defc4 100644
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -30,6 +30,7 @@ static int scsi_qdev_init(DeviceState *qdev, DeviceInfo *base)
>      SCSIDevice *dev = DO_UPCAST(SCSIDevice, qdev, qdev);
>      SCSIDeviceInfo *info = DO_UPCAST(SCSIDeviceInfo, qdev, base);
>      SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
> +    int rc = -1;
>  
>      if (dev->id == -1) {
>          for (dev->id = 0; dev->id < bus->ndev; dev->id++) {
> @@ -43,21 +44,38 @@ static int scsi_qdev_init(DeviceState *qdev, DeviceInfo *base)
>      }
>  
>      if (bus->devs[dev->id]) {
> -        bus->devs[dev->id]->info->destroy(bus->devs[dev->id]);
> +        qdev_free(&bus->devs[dev->id]->qdev);
>      }

If I understand this correctly, SCSI devices "overwrite": if you add a
new one with an existing SCSI ID, the old one gets disconnected
automatically.  Isn't that inconsistent with other buses?  PCI,
specifically.  Question applies before your patch already.

>      bus->devs[dev->id] = dev;
>  
>      dev->info = info;
> -    return dev->info->init(dev);
> +    rc = dev->info->init(dev);
> +    if (rc != 0) {
> +        bus->devs[dev->id] = NULL;
> +    }
>  
>  err:
> -    return -1;
> +    return rc;
> +}
> +
> +static int scsi_qdev_exit(DeviceState *qdev)
> +{
> +    SCSIDevice *dev = DO_UPCAST(SCSIDevice, qdev, qdev);
> +    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
> +
> +    assert(bus->devs[dev->id] != NULL);
> +    if (bus->devs[dev->id]->info->destroy) {
> +        bus->devs[dev->id]->info->destroy(bus->devs[dev->id]);
> +    }
> +    bus->devs[dev->id] = NULL;
> +    return 0;
>  }

You have scsi_qdev_exit() as qdev callback exit().  It does the generic
stuff, then runs the SCSIDevice callback destroy() for the specific
stuff.  Shouldn't the two callbacks be named the same, to make their
relation more obvious?

>  
>  void scsi_qdev_register(SCSIDeviceInfo *info)
>  {
>      info->qdev.bus_info = &scsi_bus_info;
>      info->qdev.init     = scsi_qdev_init;
> +    info->qdev.exit     = scsi_qdev_exit;
>      qdev_register(&info->qdev);
>  }
>  
[...]