Patchwork [08/12] block: Catch attempt to attach multiple devices to a blockdev

login
register
mail settings
Submitter Markus Armbruster
Date June 25, 2010, 4:53 p.m.
Message ID <1277484812-22012-9-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/56958/
State New
Headers show

Comments

Markus Armbruster - June 25, 2010, 4:53 p.m.
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 <armbru@redhat.com>
---
 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(-)
Christoph Hellwig - June 26, 2010, 10:11 a.m.
On Fri, Jun 25, 2010 at 06:53:28PM +0200, Markus Armbruster wrote:
> 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.

And from some quick testing a while ago the thing seems to actually
work.  Not that I think that it is a good idea, but do we want to change
behaviour in that respect?

> Device usb-storage deliberately attaches twice to the same blockdev,
> which fails with the fix in place.  Detach before the second attach
> there.

Can anyone explain what the hell usb storage is actually trying to do
with the two drives?
Markus Armbruster - June 26, 2010, 2:44 p.m.
Christoph Hellwig <hch@lst.de> writes:

> On Fri, Jun 25, 2010 at 06:53:28PM +0200, Markus Armbruster wrote:
>> 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.
>
> And from some quick testing a while ago the thing seems to actually
> work.  Not that I think that it is a good idea, but do we want to change
> behaviour in that respect?

Valid question.  I'd answer yes.  It's an easy error to make, and likely
to end in massive file system corruption in the guest.

>> Device usb-storage deliberately attaches twice to the same blockdev,
>> which fails with the fix in place.  Detach before the second attach
>> there.
>
> Can anyone explain what the hell usb storage is actually trying to do
> with the two drives?

It's actually a SCSI controller with a single drive on its single bus.

-device usb-storage,drive=foo creates *two* devices: usb-storage itself,
which serves as SCSI controller, and scsi-disk for the drive.
usb-storage copies its drive property to scsi-disk.

I don't like this.  Each -device should create just one device.
Christoph Hellwig - June 27, 2010, 9:36 a.m.
On Sat, Jun 26, 2010 at 04:44:11PM +0200, Markus Armbruster wrote:
> Valid question.  I'd answer yes.  It's an easy error to make, and likely
> to end in massive file system corruption in the guest.

I suspect a modern distro in the guest will detect it as a multi-path setup.

> > Can anyone explain what the hell usb storage is actually trying to do
> > with the two drives?
> 
> It's actually a SCSI controller with a single drive on its single bus.
> 
> -device usb-storage,drive=foo creates *two* devices: usb-storage itself,
> which serves as SCSI controller, and scsi-disk for the drive.
> usb-storage copies its drive property to scsi-disk.
> 
> I don't like this.  Each -device should create just one device.

Indeed.  I'd also prefer to get rid of this.  Anthony, how hard are the
rules on backwards compatiblity for things like this?

Note that currently the usb storage emulation is extremly broken anyway,
just writing to it produces I/O errors after a short while.  This means
it can't be used very much at all.
Kevin Wolf - June 28, 2010, 8:24 a.m.
Am 27.06.2010 11:36, schrieb Christoph Hellwig:
> On Sat, Jun 26, 2010 at 04:44:11PM +0200, Markus Armbruster wrote:
>> Valid question.  I'd answer yes.  It's an easy error to make, and likely
>> to end in massive file system corruption in the guest.
> 
> I suspect a modern distro in the guest will detect it as a multi-path setup.
> 
>>> Can anyone explain what the hell usb storage is actually trying to do
>>> with the two drives?
>>
>> It's actually a SCSI controller with a single drive on its single bus.
>>
>> -device usb-storage,drive=foo creates *two* devices: usb-storage itself,
>> which serves as SCSI controller, and scsi-disk for the drive.
>> usb-storage copies its drive property to scsi-disk.
>>
>> I don't like this.  Each -device should create just one device.
> 
> Indeed.  I'd also prefer to get rid of this.  Anthony, how hard are the
> rules on backwards compatiblity for things like this?

How would breaking compatibility help us? For the user a USB MSD is only
one device, so requiring two -device parameters sounds wrong.

If anything, scsi-disk must change to be able to share code without
requiring a device - and this is a change that would be kept internal
with no change on the user interface.

Or are you just talking about things like migration between versions
which would very likely break? That would probably be okay for USB MSD.

> Note that currently the usb storage emulation is extremly broken anyway,
> just writing to it produces I/O errors after a short while.  This means
> it can't be used very much at all.

When I tried last time, it did produce lots of kernel error messages in
the guest, but in the end the data was written correctly. So it doesn't
seem to be completely unusable.

Kevin
Christoph Hellwig - June 28, 2010, 10:16 a.m.
On Mon, Jun 28, 2010 at 10:24:49AM +0200, Kevin Wolf wrote:
> How would breaking compatibility help us? For the user a USB MSD is only
> one device, so requiring two -device parameters sounds wrong.

But it is separate devices.  At least the standards compliant usb
storage devices just are a bride of scsi commands over usb and fit into
the SAM device model, which makes a difference between initiator, target
and LUN.  So having a different device for the specific target vs the
initiator port makes a difference. (and yes, we're still totally missing
support for multiple luns, which would require another level of
devices).  Trying to hide this is not all that useful - not anymore
useful than hiding it on a "normal" scsi host controller anyway.
Kevin Wolf - June 28, 2010, 10:26 a.m.
Am 28.06.2010 12:16, schrieb Christoph Hellwig:
> On Mon, Jun 28, 2010 at 10:24:49AM +0200, Kevin Wolf wrote:
>> How would breaking compatibility help us? For the user a USB MSD is only
>> one device, so requiring two -device parameters sounds wrong.
> 
> But it is separate devices.  At least the standards compliant usb
> storage devices just are a bride of scsi commands over usb and fit into
> the SAM device model, which makes a difference between initiator, target
> and LUN.  So having a different device for the specific target vs the
> initiator port makes a difference. (and yes, we're still totally missing
> support for multiple luns, which would require another level of
> devices).  Trying to hide this is not all that useful - not anymore
> useful than hiding it on a "normal" scsi host controller anyway.

Maybe we need something like composed devices? So when the user asks for
a USB stick, he actually gets all devices that this stick internally
uses? Otherwise it becomes really hard to use -device directly.

I guess the same applies for mainboards, CPUs and probably some more
things, though I don't really know how these are (planned to be) done in
qdev.

Kevin
Gerd Hoffmann - June 29, 2010, 8:06 a.m.
Hi,

>> Note that currently the usb storage emulation is extremly broken anyway,
>> just writing to it produces I/O errors after a short while.  This means
>> it can't be used very much at all.
>
> When I tried last time, it did produce lots of kernel error messages in
> the guest, but in the end the data was written correctly. So it doesn't
> seem to be completely unusable.

I think it is actually the linux kernel being at fault here.  I've seen 
I/O errors on real hardware too, on my pretty old laptop which has usb 
1.1 only.  Probably something like timeouts being *way* too low for usb 
1.1 xfer speeds.

cheers,
   Gerd
Kevin Wolf - June 29, 2010, 1:32 p.m.
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 <armbru@redhat.com>
> ---
>  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
Markus Armbruster - June 29, 2010, 2:29 p.m.
Kevin Wolf <kwolf@redhat.com> writes:

> 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.
[...]
>> 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.

Correct.

>>          if (qdev_init(&dev->qdev) < 0)
>>              dev = NULL;
>>          break;
[...]
>> 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.

Yes.

>>      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.

Yes.

I double-checked the entire patch series, and could not find further
leaks.

Thanks a lot!
Markus Armbruster - June 30, 2010, 11:33 a.m.
Christoph Hellwig <hch@lst.de> writes:

>Markus Armbruster <armbru@redhat.com> writes:
>
>> Christoph Hellwig <hch@lst.de> writes:
>>
>>> On Fri, Jun 25, 2010 at 06:53:28PM +0200, Markus Armbruster wrote:
>>>> 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.
>>>
>>> And from some quick testing a while ago the thing seems to actually
>>> work.  Not that I think that it is a good idea, but do we want to change
>>> behaviour in that respect?
>>
>> Valid question.  I'd answer yes.  It's an easy error to make, and likely
>> to end in massive file system corruption in the guest.
>
> I suspect a modern distro in the guest will detect it as a multi-path setup.

Really?  The guest sees two disks, different serial numbers, possibly on
different buses (one could be SCSI, the other iDE).

[...]
Markus Armbruster - June 30, 2010, 11:52 a.m.
Kevin Wolf <kwolf@redhat.com> writes:

> Am 28.06.2010 12:16, schrieb Christoph Hellwig:
>> On Mon, Jun 28, 2010 at 10:24:49AM +0200, Kevin Wolf wrote:
>>
>>> Am 27.06.2010 11:36, schrieb Christoph Hellwig:
>>>> On Sat, Jun 26, 2010 at 04:44:11PM +0200, Markus Armbruster wrote:
[...]
>>>>> -device usb-storage,drive=foo creates *two* devices: usb-storage itself,
>>>>> which serves as SCSI controller, and scsi-disk for the drive.
>>>>> usb-storage copies its drive property to scsi-disk.
>>>>>
>>>>> I don't like this.  Each -device should create just one device.
>>>> 
>>>> Indeed.  I'd also prefer to get rid of this.  Anthony, how hard are the
>>>> rules on backwards compatiblity for things like this?
>>>
>>> How would breaking compatibility help us? For the user a USB MSD is only
>>> one device, so requiring two -device parameters sounds wrong.

-device designed to be simple, stupid and straightforward: you get
exactly what you asked for, no more, no less.  usb-storage breaks this
design maxim.

>> But it is separate devices.  At least the standards compliant usb
>> storage devices just are a bride of scsi commands over usb and fit into
>> the SAM device model, which makes a difference between initiator, target
>> and LUN.  So having a different device for the specific target vs the
>> initiator port makes a difference. (and yes, we're still totally missing
>> support for multiple luns, which would require another level of
>> devices).  Trying to hide this is not all that useful - not anymore
>> useful than hiding it on a "normal" scsi host controller anyway.
>
> Maybe we need something like composed devices? So when the user asks for
> a USB stick, he actually gets all devices that this stick internally
> uses? Otherwise it becomes really hard to use -device directly.

Could be useful.

> I guess the same applies for mainboards, CPUs and probably some more
> things, though I don't really know how these are (planned to be) done in
> qdev.

I'd like to keep -device stupid.  If we need "smarter" controls, let's
layer them on top.
Kevin Wolf - June 30, 2010, 12:13 p.m.
Am 30.06.2010 13:52, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 28.06.2010 12:16, schrieb Christoph Hellwig:
>>> On Mon, Jun 28, 2010 at 10:24:49AM +0200, Kevin Wolf wrote:
>>>
>>>> Am 27.06.2010 11:36, schrieb Christoph Hellwig:
>>>>> On Sat, Jun 26, 2010 at 04:44:11PM +0200, Markus Armbruster wrote:
> [...]
>>>>>> -device usb-storage,drive=foo creates *two* devices: usb-storage itself,
>>>>>> which serves as SCSI controller, and scsi-disk for the drive.
>>>>>> usb-storage copies its drive property to scsi-disk.
>>>>>>
>>>>>> I don't like this.  Each -device should create just one device.
>>>>>
>>>>> Indeed.  I'd also prefer to get rid of this.  Anthony, how hard are the
>>>>> rules on backwards compatiblity for things like this?
>>>>
>>>> How would breaking compatibility help us? For the user a USB MSD is only
>>>> one device, so requiring two -device parameters sounds wrong.
> 
> -device designed to be simple, stupid and straightforward: you get
> exactly what you asked for, no more, no less.  usb-storage breaks this
> design maxim.

I suppose the real question is: What is a device? qemu's internal view
(dozens of devices that communicate with each other) and the user's view
(it's one USB stick/mainboard/...) may differ.

>> Maybe we need something like composed devices? So when the user asks for
>> a USB stick, he actually gets all devices that this stick internally
>> uses? Otherwise it becomes really hard to use -device directly.
> 
> Could be useful.
> 
>> I guess the same applies for mainboards, CPUs and probably some more
>> things, though I don't really know how these are (planned to be) done in
>> qdev.
> 
> I'd like to keep -device stupid.  If we need "smarter" controls, let's
> layer them on top.

Makes sense. I'm not opposed to having something that deals only with
"atomic devices" or whatever you want to call them.

What users will usually want to have is something that treats an USB
stick as one device, though. So maybe the stupid version should become
-qdev or something and the extended version that is meant for users
should get the easy-to-remember name -device.

Kevin

Patch

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;
+        }
         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;
+    }
     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;