Patchwork [v3,2/4] scsi: Allow SCSI devices to override the removable bit

login
register
mail settings
Submitter Stefan Hajnoczi
Date Jan. 21, 2011, 11 a.m.
Message ID <1295607609-21091-3-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/79831/
State New
Headers show

Comments

Stefan Hajnoczi - Jan. 21, 2011, 11 a.m.
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(-)
Markus Armbruster - Jan. 21, 2011, 5:26 p.m.
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.
Stefan Hajnoczi - Jan. 24, 2011, 2:47 p.m.
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

Patch

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