Re: [PATCH v2 13/13] qemu: Replace usb-storage with usb-bot

On Mon, Jun 23, 2025 at 21:59:18 +0200, Peter Krempa wrote: > From: Akihiko Odaki <akihiko.odaki(a)daynix.com&gt; > > usb-storage is a compound device that automatically creates a USB mass > storage device and a SCSI device as its backend. Unfortunately it lacks > some configuration options that are usually present with a SCSI device, > and cannot represent CD-ROM in particular. > > Replace usb-storage with usb-bot, which can be combined with a manually > created SCSI device. libvirt will configure the SCSI device in a way > identical with how QEMU does for usb-storage except that now it respects > a configuration option to represent CD-ROM. > > Resolves: https://gitlab.com/libvirt/libvirt/-/issues/368 > Signed-off-by: Akihiko Odaki <akihiko.odaki(a)daynix.com&gt; > Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; > --- > src/qemu/qemu_alias.c | 20 ++++- > src/qemu/qemu_capabilities.c | 3 +- > src/qemu/qemu_command.c | 86 ++++++++++++++++--- > src/qemu/qemu_command.h | 5 ++ > src/qemu/qemu_hotplug.c | 18 ++++ > src/qemu/qemu_validate.c | 38 ++++++-- > tests/qemuhotplugtest.c | 4 +- > ...om-usb-empty.x86_64-latest.abi-update.args | 3 +- > .../disk-usb-device-model.x86_64-latest.args | 6 +- > ...k-usb-device.x86_64-latest.abi-update.args | 12 ++- > 10 files changed, 167 insertions(+), 28 deletions(-) ... > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index e1ed8181e3..4f6a3d3414 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c ... > @@ -811,6 +820,12 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm, > } > } > > + if (rc == 0) { > + if (disk->bus == VIR_DOMAIN_DISK_BUS_USB && > + disk->model == VIR_DOMAIN_DISK_MODEL_USB_BOT) > + rc = qemuMonitorSetUSBDiskAttached(priv->mon, disk->info.alias); > + } Why not just if (rc == 0 && disk->bus == ... && disk->model == ...)

> + > qemuDomainObjExitMonitor(vm); > > if (rc < 0) > @@ -822,6 +837,9 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm, > if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) > return -1; > > + if (busAdded) > + ignore_value(qemuMonitorDelDevice(priv->mon, disk->info.alias)); > + > if (extensionDeviceAttached) > ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &disk->info)); > > diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c > index 7e0e4db516..4b8d4fc692 100644 > --- a/src/qemu/qemu_validate.c > +++ b/src/qemu/qemu_validate.c > @@ -3183,16 +3183,40 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk, > break; > > case VIR_DOMAIN_DISK_BUS_USB: > - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > - _("This QEMU doesn't support '-device usb-storage'")); > + switch (disk->model) { > + case VIR_DOMAIN_DISK_MODEL_DEFAULT: > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("USB disk model was not selected by selection code")); This should only be possible if neither usb-storage nor usb-bot is supported by QEMU, shouldn't it? In that case, shouldn't the selection code itself report the error and actually be explicit about the failure?

> return -1; > - } > > - if (disk->model == VIR_DOMAIN_DISK_MODEL_USB_STORAGE && > - virStorageSourceIsEmpty(disk->src)) { > + case VIR_DOMAIN_DISK_MODEL_USB_STORAGE: > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("This QEMU doesn't support '-device usb-storage'")); > + return -1; > + } > + > + if (virStorageSourceIsEmpty(disk->src)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("'usb' disk must not be empty")); > + return -1; > + } > + break; > + > + case VIR_DOMAIN_DISK_MODEL_USB_BOT: > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_BOT)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("This QEMU doesn't support '-device usb-bot'")); > + return -1; > + } > + break; > + > + case VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL: > + case VIR_DOMAIN_DISK_MODEL_VIRTIO: > + case VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL: > + case VIR_DOMAIN_DISK_MODEL_LAST: > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > - _("'usb' disk must not be empty")); > + _("USB disk supports only models: 'usb-storage', 'usb-bot''")); s/''/'/ And the error message looks weird to me, how about s/models/the following models/ ? > return -1; > } > ... Reviewed-by: Jiri Denemark <jdenemar(a)redhat.com&gt;