| Message ID | 20250303-usb-v1-1-70f700a181fd@daynix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb-storage: Allow manually adding SCSI device | expand |
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.
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,
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
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[] = {
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(-) --- base-commit: b69801dd6b1eb4d107f7c2f643adf0a4e3ec9124 change-id: 20250301-usb-5dde4bcb1467 Best regards,