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

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

[Akihiko Odaki]

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;
 }
 
 static const Property msd_properties[] = {

---
base-commit: b69801dd6b1eb4d107f7c2f643adf0a4e3ec9124
change-id: 20250301-usb-5dde4bcb1467

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

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

[Philippe Mathieu-Daudé]

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.

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

[Paolo Bonzini]

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.

I might be misunderstanding what you're doing, but can't you do that 
already with usb-bot?

Paolo

> 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;
>   }
>   
>   static const Property msd_properties[] = {
> 
> ---
> base-commit: b69801dd6b1eb4d107f7c2f643adf0a4e3ec9124
> change-id: 20250301-usb-5dde4bcb1467
> 
> Best regards,

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

[BALATON Zoltan]

On Mon, 3 Mar 2025, Paolo Bonzini wrote:
> 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.
>
> I might be misunderstanding what you're doing, but can't you do that already 
> with usb-bot?

That's quite an obscure device I haven't heard of yet. Could it be 
possible to make -drive media=cdrom,if=usb,file=some.iso do the right 
thing whatever is that for users' convenience?

Regards,
BALATON Zoltan

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

[Akihiko Odaki]

On 2025/03/03 21:17, Paolo Bonzini wrote:
> 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.
> 
> I might be misunderstanding what you're doing, but can't you do that 
> already with usb-bot?

I wtithdraw this patch.

I wrote patches for libvirt to fix its issue mentioned in the patch 
message according to your suggestion to use usb-bot and it works nicely:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/SVXUWW2426H73XBXJXDUMTDJM3YL37QU/

Regards,
Akihiko Odaki

> 
> Paolo
> 
>> 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;
>>   }
>>   static const Property msd_properties[] = {
>>
>> ---
>> base-commit: b69801dd6b1eb4d107f7c2f643adf0a4e3ec9124
>> change-id: 20250301-usb-5dde4bcb1467
>>
>> Best regards,
>