| Message ID | 1277484812-22012-9-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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?
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.
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.
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
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.
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
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
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
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!
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). [...]
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.
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
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;
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(-)