On Mon, Jun 23, 2025 at 21:59:18 +0200, Peter Krempa wrote:
> From: Akihiko Odaki <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Peter Krempa <[email protected]>
> ---
> 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 <[email protected]>
