Re: [PATCH] usb-storage: Allow manually adding SCSI device

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 3/3/25 11:28, Akihiko Odaki wrote:
> usb-storage automatically adds a SCSI device, but it limits
> configurability of the added SCSI device and causes usability
> problems as observed in:
> https://gitlab.com/libvirt/libvirt/-/issues/368
> 
> Allow manually adding SCSI device when the drive option is not
> specified.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   hw/usb/dev-storage-classic.c | 33 ++++++++++++++++-----------------
>   1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/usb/dev-storage-classic.c b/hw/usb/dev-storage-classic.c
> index 56ef39da2e634d1639a07ac4636cdaa000989f5f..33e5a7cfc8bdf3f92b18014e885771aee6d32f5e 100644
> --- a/hw/usb/dev-storage-classic.c
> +++ b/hw/usb/dev-storage-classic.c
> @@ -33,10 +33,9 @@ static void usb_msd_storage_realize(USBDevice *dev, Error **errp)
>       BlockBackend *blk = s->conf.blk;
>       SCSIDevice *scsi_dev;
>   
> -    if (!blk) {
> -        error_setg(errp, "drive property not set");
> -        return;
> -    }
> +    usb_desc_create_serial(dev);
> +    scsi_bus_init(&s->bus, sizeof(s->bus), DEVICE(dev),
> +                 &usb_msd_scsi_info_storage);
>   
>       /*
>        * Hack alert: this pretends to be a block device, but it's really
> @@ -48,23 +47,23 @@ static void usb_msd_storage_realize(USBDevice *dev, Error **errp)
>        *
>        * The hack is probably a bad idea.
>        */
> -    blk_ref(blk);
> -    blk_detach_dev(blk, DEVICE(s));
> -    s->conf.blk = NULL;
> +    if (blk) {
> +        blk_ref(blk);
> +        blk_detach_dev(blk, DEVICE(s));
> +        s->conf.blk = NULL;
> +
> +        scsi_dev = scsi_bus_legacy_add_drive(&s->bus, blk, 0, !!s->removable,
> +                                             &s->conf, dev->serial, errp);
> +        blk_unref(blk);
> +        if (!scsi_dev) {
> +            return;
> +        }
> +        s->scsi_dev = scsi_dev;
> +    }
>   
> -    usb_desc_create_serial(dev);
>       usb_desc_init(dev);
>       dev->flags |= (1 << USB_DEV_FLAG_IS_SCSI_STORAGE);
> -    scsi_bus_init(&s->bus, sizeof(s->bus), DEVICE(dev),
> -                 &usb_msd_scsi_info_storage);
> -    scsi_dev = scsi_bus_legacy_add_drive(&s->bus, blk, 0, !!s->removable,
> -                                         &s->conf, dev->serial, errp);
> -    blk_unref(blk);
> -    if (!scsi_dev) {
> -        return;
> -    }
>       usb_msd_handle_reset(dev);
> -    s->scsi_dev = scsi_dev;
>   }

LGTM but I'd rather feedback from block team (Cc'ed).

Regards,

Phil.