Re: [Qemu-devel] [PATCH v3 2/4] scsi: Allow SCSI devices to override the removable bit

[Markus Armbruster]

Stefan Hajnoczi <address@hidden> 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 <address@hidden>
> ---
>  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.