[Qemu-devel] Re: [PATCH 08/12] block: Catch attempt to attach multiple devices to a blockdev

[Kevin Wolf]

Am 25.06.2010 18:53, schrieb Markus Armbruster:
> For instance, -device scsi-disk,drive=foo -device scsi-disk,drive=foo
> happily creates two SCSI disks connected to the same block device.
> It's all downhill from there.
> 
> Device usb-storage deliberately attaches twice to the same blockdev,
> which fails with the fix in place.  Detach before the second attach
> there.
> 
> Also catch attempt to delete while a guest device model is attached.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  block.c              |   22 ++++++++++++++++++++++
>  block.h              |    3 +++
>  block_int.h          |    2 ++
>  hw/fdc.c             |   10 +++++-----
>  hw/ide/qdev.c        |    2 +-
>  hw/pci-hotplug.c     |    5 ++++-
>  hw/qdev-properties.c |   21 ++++++++++++++++++++-
>  hw/qdev.h            |    3 ++-
>  hw/s390-virtio.c     |    2 +-
>  hw/scsi-bus.c        |    4 +++-
>  hw/usb-msd.c         |   11 +++++++----
>  11 files changed, 70 insertions(+), 15 deletions(-)
> 
> diff --git a/block.c b/block.c
> index e71a771..5e0ffa0 100644
> --- a/block.c
> +++ b/block.c
> @@ -659,6 +659,8 @@ void bdrv_close_all(void)
>  
>  void bdrv_delete(BlockDriverState *bs)
>  {
> +    assert(!bs->peer);
> +
>      /* remove from list, if necessary */
>      if (bs->device_name[0] != '\0') {
>          QTAILQ_REMOVE(&bdrv_states, bs, list);
> @@ -672,6 +674,26 @@ void bdrv_delete(BlockDriverState *bs)
>      qemu_free(bs);
>  }
>  
> +int bdrv_attach(BlockDriverState *bs, DeviceState *qdev)
> +{
> +    if (bs->peer) {
> +        return -EBUSY;
> +    }
> +    bs->peer = qdev;
> +    return 0;
> +}
> +
> +void bdrv_detach(BlockDriverState *bs, DeviceState *qdev)
> +{
> +    assert(bs->peer == qdev);
> +    bs->peer = NULL;
> +}
> +
> +DeviceState *bdrv_get_attached(BlockDriverState *bs)
> +{
> +    return bs->peer;
> +}
> +
>  /*
>   * Run consistency checks on an image
>   *
> diff --git a/block.h b/block.h
> index 6a157f4..88ac06e 100644
> --- a/block.h
> +++ b/block.h
> @@ -71,6 +71,9 @@ int bdrv_file_open(BlockDriverState **pbs, const char 
> *filename, int flags);
>  int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
>                BlockDriver *drv);
>  void bdrv_close(BlockDriverState *bs);
> +int bdrv_attach(BlockDriverState *bs, DeviceState *qdev);
> +void bdrv_detach(BlockDriverState *bs, DeviceState *qdev);
> +DeviceState *bdrv_get_attached(BlockDriverState *bs);
>  int bdrv_check(BlockDriverState *bs);
>  int bdrv_read(BlockDriverState *bs, int64_t sector_num,
>                uint8_t *buf, int nb_sectors);
> diff --git a/block_int.h b/block_int.h
> index e60aed4..a94b801 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -148,6 +148,8 @@ struct BlockDriverState {
>      BlockDriver *drv; /* NULL means no media */
>      void *opaque;
>  
> +    DeviceState *peer;
> +
>      char filename[1024];
>      char backing_file[1024]; /* if non zero, the image is a diff of
>                                  this file image */
> diff --git a/hw/fdc.c b/hw/fdc.c
> index 08712bc..1496cfa 100644
> --- a/hw/fdc.c
> +++ b/hw/fdc.c
> @@ -1860,10 +1860,10 @@ FDCtrl *fdctrl_init_isa(DriveInfo **fds)
>  
>      dev = isa_create("isa-fdc");
>      if (fds[0]) {
> -        qdev_prop_set_drive(&dev->qdev, "driveA", fds[0]->bdrv);
> +        qdev_prop_set_drive_nofail(&dev->qdev, "driveA", fds[0]->bdrv);
>      }
>      if (fds[1]) {
> -        qdev_prop_set_drive(&dev->qdev, "driveB", fds[1]->bdrv);
> +        qdev_prop_set_drive_nofail(&dev->qdev, "driveB", fds[1]->bdrv);
>      }
>      if (qdev_init(&dev->qdev) < 0)
>          return NULL;
> @@ -1882,10 +1882,10 @@ FDCtrl *fdctrl_init_sysbus(qemu_irq irq, int 
> dma_chann,
>      fdctrl = &sys->state;
>      fdctrl->dma_chann = dma_chann; /* FIXME */
>      if (fds[0]) {
> -        qdev_prop_set_drive(dev, "driveA", fds[0]->bdrv);
> +        qdev_prop_set_drive_nofail(dev, "driveA", fds[0]->bdrv);
>      }
>      if (fds[1]) {
> -        qdev_prop_set_drive(dev, "driveB", fds[1]->bdrv);
> +        qdev_prop_set_drive_nofail(dev, "driveB", fds[1]->bdrv);
>      }
>      qdev_init_nofail(dev);
>      sysbus_connect_irq(&sys->busdev, 0, irq);
> @@ -1903,7 +1903,7 @@ FDCtrl *sun4m_fdctrl_init(qemu_irq irq, 
> target_phys_addr_t io_base,
>  
>      dev = qdev_create(NULL, "SUNW,fdtwo");
>      if (fds[0]) {
> -        qdev_prop_set_drive(dev, "drive", fds[0]->bdrv);
> +        qdev_prop_set_drive_nofail(dev, "drive", fds[0]->bdrv);
>      }
>      qdev_init_nofail(dev);
>      sys = DO_UPCAST(FDCtrlSysBus, busdev.qdev, dev);
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 3bb94c6..b4bc5ac 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -83,7 +83,7 @@ IDEDevice *ide_create_drive(IDEBus *bus, int unit, 
> DriveInfo *drive)
>  
>      dev = qdev_create(&bus->qbus, "ide-drive");
>      qdev_prop_set_uint32(dev, "unit", unit);
> -    qdev_prop_set_drive(dev, "drive", drive->bdrv);
> +    qdev_prop_set_drive_nofail(dev, "drive", drive->bdrv);
>      qdev_init_nofail(dev);
>      return DO_UPCAST(IDEDevice, qdev, dev);
>  }
> diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
> index d743192..b47e01e 100644
> --- a/hw/pci-hotplug.c
> +++ b/hw/pci-hotplug.c
> @@ -214,7 +214,10 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
>              return NULL;
>          }
>          dev = pci_create(bus, devfn, "virtio-blk-pci");
> -        qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv);
> +        if (qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv) < 0) {
> +            dev = NULL;
> +            break;
> +        }

I think in error cases we'll leak dev.

>          if (qdev_init(&dev->qdev) < 0)
>              dev = NULL;
>          break;
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 257233e..caf6798 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -291,6 +291,8 @@ static int parse_drive(DeviceState *dev, Property *prop, 
> const char *str)
>      bs = bdrv_find(str);
>      if (bs == NULL)
>          return -ENOENT;
> +    if (bdrv_attach(bs, dev) < 0)
> +        return -EEXIST;
>      *ptr = bs;
>      return 0;
>  }
> @@ -300,6 +302,7 @@ static void free_drive(DeviceState *dev, Property *prop)
>      BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop);
>  
>      if (*ptr) {
> +        bdrv_detach(*ptr, dev);
>          blockdev_auto_del(*ptr);
>      }
>  }
> @@ -640,11 +643,27 @@ void qdev_prop_set_string(DeviceState *dev, const char 
> *name, char *value)
>      qdev_prop_set(dev, name, &value, PROP_TYPE_STRING);
>  }
>  
> -void qdev_prop_set_drive(DeviceState *dev, const char *name, 
> BlockDriverState *value)
> +int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState 
> *value)
>  {
> +    int res;
> +
>      qdev_prop_set(dev, name, &value, PROP_TYPE_DRIVE);
> +    res = bdrv_attach(value, dev);
> +    if (res < 0) {
> +        error_report("Can't attach drive %s to %s.%s: %s",
> +                     bdrv_get_device_name(value),
> +                     dev->id ? dev->id : dev->info->name,
> +                     name, strerror(-res));
> +    }
> +    return res;
>  }
>  
> +void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, 
> BlockDriverState *value)
> +{
> +    if (qdev_prop_set_drive(dev, name, value) < 0) {
> +        exit(1);
> +    }
> +}
>  void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState 
> *value)
>  {
>      qdev_prop_set(dev, name, &value, PROP_TYPE_CHR);
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 7a01a81..cbc89f2 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -275,7 +275,8 @@ void qdev_prop_set_string(DeviceState *dev, const char 
> *name, char *value);
>  void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState 
> *value);
>  void qdev_prop_set_netdev(DeviceState *dev, const char *name, 
> VLANClientState *value);
>  void qdev_prop_set_vlan(DeviceState *dev, const char *name, VLANState 
> *value);
> -void qdev_prop_set_drive(DeviceState *dev, const char *name, 
> BlockDriverState *value);
> +int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState 
> *value) QEMU_WARN_UNUSED_RESULT;
> +void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, 
> BlockDriverState *value);
>  void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t 
> *value);
>  /* FIXME: Remove opaque pointer properties.  */
>  void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index c7c3fc9..6af58e2 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -262,7 +262,7 @@ static void s390_init(ram_addr_t ram_size,
>          }
>  
>          dev = qdev_create((BusState *)s390_bus, "virtio-blk-s390");
> -        qdev_prop_set_drive(dev, "drive", dinfo->bdrv);
> +        qdev_prop_set_drive_nofail(dev, "drive", dinfo->bdrv);
>          qdev_init_nofail(dev);
>      }
>  }
> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
> index 2c58aca..9678328 100644
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -91,7 +91,9 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, 
> BlockDriverState *bdrv, int
>      driver = bdrv_is_sg(bdrv) ? "scsi-generic" : "scsi-disk";
>      dev = qdev_create(&bus->qbus, driver);
>      qdev_prop_set_uint32(dev, "scsi-id", unit);
> -    qdev_prop_set_drive(dev, "drive", bdrv);
> +    if (qdev_prop_set_drive(dev, "drive", bdrv) < 0) {
> +        return NULL;
> +    }

As we do here.

>      if (qdev_init(dev) < 0)
>          return NULL;
>      return DO_UPCAST(SCSIDevice, qdev, dev);
> diff --git a/hw/usb-msd.c b/hw/usb-msd.c
> index 19a14b4..008cc0f 100644
> --- a/hw/usb-msd.c
> +++ b/hw/usb-msd.c
> @@ -532,12 +532,13 @@ static int usb_msd_initfn(USBDevice *dev)
>      /*
>       * Hack alert: this pretends to be a block device, but it's really
>       * a SCSI bus that can serve only a single device, which it
> -     * creates automatically.  Two drive properties pointing to the
> -     * same drive is not good: free_drive() dies for the second one.
> -     * Zap the one we're not going to use.
> +     * creates automatically.  But first it needs to detach from its
> +     * blockdev, or else scsi_bus_legacy_add_drive() dies when it
> +     * attaches again.
>       *
>       * The hack is probably a bad idea.
>       */
> +    bdrv_detach(bs, &s->dev.qdev);
>      s->conf.bs = NULL;
>  
>      s->dev.speed = USB_SPEED_FULL;
> @@ -609,7 +610,9 @@ static USBDevice *usb_msd_init(const char *filename)
>      if (!dev) {
>          return NULL;
>      }
> -    qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv);
> +    if (qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv) < 0) {
> +        return NULL;
> +    }
>      if (qdev_init(&dev->qdev) < 0)
>          return NULL;

And here.

Kevin