| Message ID | 1295607609-21091-3-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> writes: > Some SCSI devices may wish to override the removable bit. Add support > for a qdev property on the SCSI device. I find this description a bit misleading. The qdev property is added in 1/4. Here, you merely extend scsi_bus_legacy_add_drive() to provide access to it. Its primary users (-drive if=scsi & friends) don't use that access. But there's another user: usb_msd_initfn()[*], and 3/4 will make that one use the access. I guess I'd squash 2+3 together, but that's strictly a matter of taste. > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > --- > hw/pci-hotplug.c | 2 +- > hw/scsi-bus.c | 8 ++++++-- > hw/scsi.h | 3 ++- > hw/usb-msd.c | 2 +- > 4 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c > index 716133c..270a982 100644 > --- a/hw/pci-hotplug.c > +++ b/hw/pci-hotplug.c > @@ -90,7 +90,7 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter, > * specified). > */ > dinfo->unit = qemu_opt_get_number(dinfo->opts, "unit", -1); > - scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit); > + scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit, false); > if (!scsidev) { > return -1; > } > diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c > index 7febb86..ceeb4ec 100644 > --- a/hw/scsi-bus.c > +++ b/hw/scsi-bus.c > @@ -87,7 +87,8 @@ void scsi_qdev_register(SCSIDeviceInfo *info) > } > > /* handle legacy '-drive if=scsi,...' cmd line args */ > -SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int unit) > +SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, > + int unit, bool removable) > { > const char *driver; > DeviceState *dev; > @@ -95,6 +96,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); > + if (qdev_prop_exists(dev, "removable")) { Isn't this just a funky way to check for "scsi-disk"? Removable gets silently ignored for -device usb-storage with a scsi generic drive. Worth nothing in 4/4. > + qdev_prop_set_bit(dev, "removable", removable); > + } > if (qdev_prop_set_drive(dev, "drive", bdrv) < 0) { > qdev_free(dev); > return NULL; > @@ -117,7 +121,7 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus) > continue; > } > qemu_opts_loc_restore(dinfo->opts); > - if (!scsi_bus_legacy_add_drive(bus, dinfo->bdrv, unit)) { > + if (!scsi_bus_legacy_add_drive(bus, dinfo->bdrv, unit, false)) { > res = -1; > break; > } [...] [*] I hate that usb_msd_initfn() uses scsi_bus_legacy_add_drive(). Actually, I hate pretty much everything about usb-msd.c. But it's not your fault.
On Fri, Jan 21, 2011 at 06:26:14PM +0100, Markus Armbruster wrote: > Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> writes: > > > Some SCSI devices may wish to override the removable bit. Add support > > for a qdev property on the SCSI device. > > I find this description a bit misleading. The qdev property is added in > 1/4. Here, you merely extend scsi_bus_legacy_add_drive() to provide > access to it. Its primary users (-drive if=scsi & friends) don't use > that access. But there's another user: usb_msd_initfn()[*], and 3/4 > will make that one use the access. > > I guess I'd squash 2+3 together, but that's strictly a matter of taste. You're right, the description is poor. Sorry about that. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > > --- > > hw/pci-hotplug.c | 2 +- > > hw/scsi-bus.c | 8 ++++++-- > > hw/scsi.h | 3 ++- > > hw/usb-msd.c | 2 +- > > 4 files changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c > > index 716133c..270a982 100644 > > --- a/hw/pci-hotplug.c > > +++ b/hw/pci-hotplug.c > > @@ -90,7 +90,7 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter, > > * specified). > > */ > > dinfo->unit = qemu_opt_get_number(dinfo->opts, "unit", -1); > > - scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit); > > + scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit, false); > > if (!scsidev) { > > return -1; > > } > > diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c > > index 7febb86..ceeb4ec 100644 > > --- a/hw/scsi-bus.c > > +++ b/hw/scsi-bus.c > > @@ -87,7 +87,8 @@ void scsi_qdev_register(SCSIDeviceInfo *info) > > } > > > > /* handle legacy '-drive if=scsi,...' cmd line args */ > > -SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int unit) > > +SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, > > + int unit, bool removable) > > { > > const char *driver; > > DeviceState *dev; > > @@ -95,6 +96,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); > > + if (qdev_prop_exists(dev, "removable")) { > > Isn't this just a funky way to check for "scsi-disk"? > > Removable gets silently ignored for -device usb-storage with a scsi > generic drive. Worth nothing in 4/4. Yes, it only applies for scsi-disk and I'll note that in 4/4. Stefan
diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c index 716133c..270a982 100644 --- a/hw/pci-hotplug.c +++ b/hw/pci-hotplug.c @@ -90,7 +90,7 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter, * specified). */ dinfo->unit = qemu_opt_get_number(dinfo->opts, "unit", -1); - scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit); + scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit, false); if (!scsidev) { return -1; } diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index 7febb86..ceeb4ec 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -87,7 +87,8 @@ void scsi_qdev_register(SCSIDeviceInfo *info) } /* handle legacy '-drive if=scsi,...' cmd line args */ -SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int unit) +SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, + int unit, bool removable) { const char *driver; DeviceState *dev; @@ -95,6 +96,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); + if (qdev_prop_exists(dev, "removable")) { + qdev_prop_set_bit(dev, "removable", removable); + } if (qdev_prop_set_drive(dev, "drive", bdrv) < 0) { qdev_free(dev); return NULL; @@ -117,7 +121,7 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus) continue; } qemu_opts_loc_restore(dinfo->opts); - if (!scsi_bus_legacy_add_drive(bus, dinfo->bdrv, unit)) { + if (!scsi_bus_legacy_add_drive(bus, dinfo->bdrv, unit, false)) { res = -1; break; } diff --git a/hw/scsi.h b/hw/scsi.h index bf02adf..846fbba 100644 --- a/hw/scsi.h +++ b/hw/scsi.h @@ -94,7 +94,8 @@ static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d) return DO_UPCAST(SCSIBus, qbus, d->qdev.parent_bus); } -SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int unit); +SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, + int unit, bool removable); int scsi_bus_legacy_handle_cmdline(SCSIBus *bus); SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t lun); diff --git a/hw/usb-msd.c b/hw/usb-msd.c index 0a95d8d..ee897b6 100644 --- a/hw/usb-msd.c +++ b/hw/usb-msd.c @@ -544,7 +544,7 @@ static int usb_msd_initfn(USBDevice *dev) s->dev.speed = USB_SPEED_FULL; scsi_bus_new(&s->bus, &s->dev.qdev, 0, 1, usb_msd_command_complete); - s->scsi_dev = scsi_bus_legacy_add_drive(&s->bus, bs, 0); + s->scsi_dev = scsi_bus_legacy_add_drive(&s->bus, bs, 0, false); if (!s->scsi_dev) { return -1; }
Some SCSI devices may wish to override the removable bit. Add support for a qdev property on the SCSI device. Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> --- hw/pci-hotplug.c | 2 +- hw/scsi-bus.c | 8 ++++++-- hw/scsi.h | 3 ++- hw/usb-msd.c | 2 +- 4 files changed, 10 insertions(+), 5 deletions(-)