From: Akihiko Odaki <akihiko.odaki@daynix.com>
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@daynix.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
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_alias.c b/src/qemu/qemu_alias.c
index 9d39ebd63d..67cbddd470 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -268,8 +268,24 @@ qemuAssignDeviceDiskAlias(virDomainDef *def,
break;
case VIR_DOMAIN_DISK_BUS_USB:
- diskPriv->qomName = g_strdup_printf("/machine/peripheral/%s/%s.0/legacy[0]",
- disk->info.alias, disk->info.alias);
+ switch (disk->model) {
+ case VIR_DOMAIN_DISK_MODEL_USB_STORAGE:
+ diskPriv->qomName = g_strdup_printf("/machine/peripheral/%s/%s.0/legacy[0]",
+ disk->info.alias, disk->info.alias);
+ break;
+
+ case VIR_DOMAIN_DISK_MODEL_USB_BOT:
+ diskPriv->qomName = g_strdup_printf("/machine/peripheral/%s-disk",
+ disk->info.alias);
+ break;
+
+ case VIR_DOMAIN_DISK_MODEL_DEFAULT:
+ case VIR_DOMAIN_DISK_MODEL_VIRTIO:
+ case VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL:
+ case VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL:
+ case VIR_DOMAIN_DISK_MODEL_LAST:
+ break;
+ }
break;
case VIR_DOMAIN_DISK_BUS_XEN:
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index f480060e50..ef6ec661ff 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -6472,7 +6472,8 @@ virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCaps *qemuCaps,
VIR_DOMAIN_DISK_BUS_VIRTIO,
/* VIR_DOMAIN_DISK_BUS_SD */);
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE))
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE) ||
+ virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_BOT))
VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, VIR_DOMAIN_DISK_BUS_USB);
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_AHCI))
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 910242a389..98fd6fee85 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1626,6 +1626,33 @@ qemuBuildIothreadMappingProps(GSList *iothreads)
return g_steal_pointer(&ret);
}
+int
+qemuBuildDiskBusProps(const virDomainDef *def,
+ const virDomainDiskDef *disk,
+ virJSONValue **propsRet)
+{
+ g_autoptr(virJSONValue) props = NULL;
+
+ *propsRet = NULL;
+
+ if (disk->bus != VIR_DOMAIN_DISK_BUS_USB ||
+ disk->model != VIR_DOMAIN_DISK_MODEL_USB_BOT)
+ return 0;
+
+ if (virJSONValueObjectAdd(&props,
+ "s:driver", "usb-bot",
+ "s:id", disk->info.alias,
+ "S:serial", disk->serial,
+ NULL) < 0)
+ return -1;
+
+ if (qemuBuildDeviceAddressProps(props, def, &disk->info) < 0)
+ return -1;
+
+ *propsRet = g_steal_pointer(&props);
+
+ return 0;
+}
virJSONValue *
qemuBuildDiskDeviceProps(const virDomainDef *def,
@@ -1652,6 +1679,18 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
const char *rpolicy = NULL;
const char *model = NULL;
const char *product = NULL;
+ const char *alias = disk->info.alias;
+ g_autofree char *usbdiskalias = NULL;
+ const virDomainDeviceInfo *deviceinfo = &disk->info;
+ virDomainDeviceInfo usbSCSIinfo = {
+ .type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE,
+ .addr.drive = { .diskbus = VIR_DOMAIN_DISK_BUS_USB },
+ .effectiveBootIndex = deviceinfo->effectiveBootIndex,
+ .alias = deviceinfo->alias,
+ };
+
+ if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)
+ disk->info.addr.drive.diskbus = disk->bus;
switch (disk->bus) {
case VIR_DOMAIN_DISK_BUS_IDE:
@@ -1735,13 +1774,35 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
break;
case VIR_DOMAIN_DISK_BUS_USB:
- driver = "usb-storage";
+ switch (disk->model) {
+ case VIR_DOMAIN_DISK_MODEL_USB_STORAGE:
+ driver = "usb-storage";
- if (disk->removable == VIR_TRISTATE_SWITCH_ABSENT)
- removable = VIR_TRISTATE_SWITCH_OFF;
- else
- removable = disk->removable;
+ if (disk->removable == VIR_TRISTATE_SWITCH_ABSENT)
+ removable = VIR_TRISTATE_SWITCH_OFF;
+ else
+ removable = disk->removable;
+ break;
+
+ case VIR_DOMAIN_DISK_MODEL_USB_BOT:
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
+ driver = "scsi-cd";
+ } else {
+ driver = "scsi-hd";
+ removable = disk->removable;
+ }
+ deviceinfo = &usbSCSIinfo;
+ alias = usbdiskalias = g_strdup_printf("%s-device", disk->info.alias);
+ break;
+
+ case VIR_DOMAIN_DISK_MODEL_DEFAULT:
+ case VIR_DOMAIN_DISK_MODEL_VIRTIO:
+ case VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL:
+ case VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL:
+ case VIR_DOMAIN_DISK_MODEL_LAST:
+ break;
+ }
break;
case VIR_DOMAIN_DISK_BUS_FDC:
@@ -1771,10 +1832,7 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
return NULL;
}
- if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)
- disk->info.addr.drive.diskbus = disk->bus;
-
- if (qemuBuildDeviceAddressProps(props, def, &disk->info) < 0)
+ if (qemuBuildDeviceAddressProps(props, def, deviceinfo) < 0)
return NULL;
if (disk->src->shared)
@@ -1836,7 +1894,7 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
"T:share-rw", shareRW,
"S:drive", drive,
"S:chardev", chardev,
- "s:id", disk->info.alias,
+ "s:id", alias,
"p:bootindex", bootindex,
"S:loadparm", bootLoadparm,
"p:logical_block_size", logical_block_size,
@@ -2201,6 +2259,7 @@ qemuBuildDiskCommandLine(virCommand *cmd,
virQEMUCaps *qemuCaps)
{
g_autoptr(virJSONValue) devprops = NULL;
+ g_autoptr(virJSONValue) busprops = NULL;
if (qemuBuildDiskSourceCommandLine(cmd, disk, qemuCaps) < 0)
return -1;
@@ -2216,6 +2275,13 @@ qemuBuildDiskCommandLine(virCommand *cmd,
if (qemuCommandAddExtDevice(cmd, &disk->info, def, qemuCaps) < 0)
return -1;
+ if (qemuBuildDiskBusProps(def, disk, &busprops) < 0)
+ return -1;
+
+ if (busprops &&
+ qemuBuildDeviceCommandlineFromJSON(cmd, busprops, def, qemuCaps) < 0)
+ return -1;
+
if (!(devprops = qemuBuildDiskDeviceProps(def, disk, qemuCaps)))
return -1;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 2d43cf5506..574dffdc96 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -129,6 +129,11 @@ qemuBuildThrottleFiltersAttachPrepareBlockdev(virDomainDiskDef *disk);
qemuBlockThrottleFiltersData *
qemuBuildThrottleFiltersDetachPrepareBlockdev(virDomainDiskDef *disk);
+int
+qemuBuildDiskBusProps(const virDomainDef *def,
+ const virDomainDiskDef *disk,
+ virJSONValue **propsRet);
+
virJSONValue *
qemuBuildDiskDeviceProps(const virDomainDef *def,
virDomainDiskDef *disk,
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
@@ -703,8 +703,10 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
g_autoptr(qemuBlockStorageSourceChainData) data = NULL;
g_autoptr(qemuBlockThrottleFiltersData) filterData = NULL;
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virJSONValue) busprops = NULL;
g_autoptr(virJSONValue) devprops = NULL;
bool extensionDeviceAttached = false;
+ bool busAdded = false;
int rc;
g_autoptr(qemuSnapshotDiskContext) transientDiskSnapshotCtxt = NULL;
bool origReadonly = disk->src->readonly;
@@ -774,6 +776,9 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
}
}
+ if (qemuBuildDiskBusProps(vm->def, disk, &busprops) < 0)
+ goto rollback;
+
if (!(devprops = qemuBuildDiskDeviceProps(vm->def, disk, priv->qemuCaps)))
goto rollback;
@@ -783,6 +788,10 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
if ((rc = qemuDomainAttachExtensionDevice(priv->mon, &disk->info)) == 0)
extensionDeviceAttached = true;
+ if (rc == 0 && busprops &&
+ (rc = qemuMonitorAddDeviceProps(priv->mon, &busprops)) == 0)
+ busAdded = true;
+
if (rc == 0)
rc = qemuMonitorAddDeviceProps(priv->mon, &devprops);
@@ -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);
+ }
+
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"));
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''"));
return -1;
}
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index fdb5093549..7881ccf327 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -746,7 +746,9 @@ mymain(void)
DO_TEST_ATTACH("x86_64", "base-live", "cdrom-usb", false, true,
"blockdev-add", QMP_OK,
"device_add", QMP_OK,
- "query-block", QMP_EMPTY_ARRAY);
+ "device_add", QMP_OK,
+ "query-block", QMP_EMPTY_ARRAY,
+ "qom-set", QMP_OK);
DO_TEST_DETACH("x86_64", "base-live", "cdrom-usb", true, true,
"device_del", QMP_OK);
DO_TEST_DETACH("x86_64", "base-live", "cdrom-usb", false, false,
diff --git a/tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.abi-update.args b/tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.abi-update.args
index cebc6e66e8..44bf27e9a4 100644
--- a/tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.abi-update.args
+++ b/tests/qemuxmlconfdata/disk-cdrom-usb-empty.x86_64-latest.abi-update.args
@@ -27,7 +27,8 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
-no-shutdown \
-boot strict=on \
-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1","id":"usb-disk1","removable":false}' \
+-device '{"driver":"usb-bot","id":"usb-disk1","bus":"usb.0","port":"1"}' \
+-device '{"driver":"scsi-cd","bus":"usb-disk1.0","scsi-id":0,"lun":0,"id":"usb-disk1-device"}' \
-audiodev '{"id":"audio1","driver":"none"}' \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
-msg timestamp=on
diff --git a/tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.args b/tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.args
index 6d31319a49..f50b0d5849 100644
--- a/tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.args
+++ b/tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.args
@@ -33,9 +33,11 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
-blockdev '{"driver":"file","filename":"/tmp/img2","node-name":"libvirt-5-storage","read-only":true}' \
-device '{"driver":"usb-storage","bus":"usb.0","port":"1.1","drive":"libvirt-5-storage","id":"usb-disk1","removable":false}' \
-blockdev '{"driver":"file","filename":"/tmp/img3","node-name":"libvirt-4-storage","read-only":false}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1.2","drive":"libvirt-4-storage","id":"usb-disk2","removable":false}' \
+-device '{"driver":"usb-bot","id":"usb-disk2","bus":"usb.0","port":"1.2"}' \
+-device '{"driver":"scsi-hd","bus":"usb-disk2.0","scsi-id":0,"lun":0,"drive":"libvirt-4-storage","id":"usb-disk2-device"}' \
-blockdev '{"driver":"file","filename":"/tmp/img4","node-name":"libvirt-3-storage","read-only":true}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1.3","drive":"libvirt-3-storage","id":"usb-disk3","removable":false}' \
+-device '{"driver":"usb-bot","id":"usb-disk3","bus":"usb.0","port":"1.3"}' \
+-device '{"driver":"scsi-cd","bus":"usb-disk3.0","scsi-id":0,"lun":0,"drive":"libvirt-3-storage","id":"usb-disk3-device"}' \
-blockdev '{"driver":"file","filename":"/tmp/img5","node-name":"libvirt-2-storage","read-only":false}' \
-device '{"driver":"usb-storage","bus":"usb.0","port":"1.4","drive":"libvirt-2-storage","id":"usb-disk4","removable":false}' \
-blockdev '{"driver":"file","filename":"/tmp/img6","node-name":"libvirt-1-storage","read-only":true}' \
diff --git a/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.abi-update.args b/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.abi-update.args
index 079dfe5d99..d8208d29b6 100644
--- a/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.abi-update.args
+++ b/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.abi-update.args
@@ -32,19 +32,23 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
-blockdev '{"driver":"file","filename":"/tmp/img1","node-name":"libvirt-12-storage","read-only":false}' \
-device '{"driver":"usb-storage","bus":"usb.0","port":"1.1","drive":"libvirt-12-storage","id":"usb-disk0","bootindex":1,"removable":false}' \
-blockdev '{"driver":"file","filename":"/tmp/img2","node-name":"libvirt-11-storage","read-only":true}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1.2","drive":"libvirt-11-storage","id":"usb-disk1","removable":false}' \
+-device '{"driver":"usb-bot","id":"usb-disk1","bus":"usb.0","port":"1.2"}' \
+-device '{"driver":"scsi-cd","bus":"usb-disk1.0","scsi-id":0,"lun":0,"drive":"libvirt-11-storage","id":"usb-disk1-device"}' \
-blockdev '{"driver":"file","filename":"/tmp/img3","node-name":"libvirt-10-storage","read-only":false}' \
-device '{"driver":"usb-storage","bus":"usb.0","port":"1.3","drive":"libvirt-10-storage","id":"usb-disk2","removable":false,"serial":"testserial1"}' \
-blockdev '{"driver":"file","filename":"/tmp/img4","node-name":"libvirt-9-storage","read-only":true}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1.4","drive":"libvirt-9-storage","id":"usb-disk3","removable":false,"serial":"testserial2"}' \
+-device '{"driver":"usb-bot","id":"usb-disk3","serial":"testserial2","bus":"usb.0","port":"1.4"}' \
+-device '{"driver":"scsi-cd","bus":"usb-disk3.0","scsi-id":0,"lun":0,"drive":"libvirt-9-storage","id":"usb-disk3-device","serial":"testserial2"}' \
-blockdev '{"driver":"file","filename":"/tmp/img5","node-name":"libvirt-8-storage","read-only":false}' \
-device '{"driver":"usb-storage","bus":"usb.0","port":"1.5","drive":"libvirt-8-storage","id":"ua-test1","removable":false}' \
-blockdev '{"driver":"file","filename":"/tmp/img6","node-name":"libvirt-7-storage","read-only":true}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1.6","drive":"libvirt-7-storage","id":"ua-test2","removable":false}' \
+-device '{"driver":"usb-bot","id":"ua-test2","bus":"usb.0","port":"1.6"}' \
+-device '{"driver":"scsi-cd","bus":"ua-test2.0","scsi-id":0,"lun":0,"drive":"libvirt-7-storage","id":"ua-test2-device"}' \
-blockdev '{"driver":"file","filename":"/tmp/img7","node-name":"libvirt-6-storage","read-only":false}' \
-device '{"driver":"usb-storage","bus":"usb.0","port":"1.7","drive":"libvirt-6-storage","id":"ua-test3","removable":false,"serial":"testserial3"}' \
-blockdev '{"driver":"file","filename":"/tmp/img8","node-name":"libvirt-5-storage","read-only":true}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1.8","drive":"libvirt-5-storage","id":"ua-test4","removable":false,"serial":"testserial4"}' \
+-device '{"driver":"usb-bot","id":"ua-test4","serial":"testserial4","bus":"usb.0","port":"1.8"}' \
+-device '{"driver":"scsi-cd","bus":"ua-test4.0","scsi-id":0,"lun":0,"drive":"libvirt-5-storage","id":"ua-test4-device","serial":"testserial4"}' \
-blockdev '{"driver":"file","filename":"/tmp/img9","node-name":"libvirt-4-storage","read-only":false}' \
-device '{"driver":"usb-storage","bus":"usb.0","port":"2.1","drive":"libvirt-4-storage","id":"usb-disk8","removable":true}' \
-blockdev '{"driver":"file","filename":"/tmp/imga","node-name":"libvirt-3-storage","read-only":false}' \
--
2.49.0On Mon, Jun 23, 2025 at 21:59:18 +0200, Peter Krempa via Devel wrote:
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
>
> 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@daynix.com>
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
> 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_alias.c b/src/qemu/qemu_alias.c
> index 9d39ebd63d..67cbddd470 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -268,8 +268,24 @@ qemuAssignDeviceDiskAlias(virDomainDef *def,
> break;
>
> case VIR_DOMAIN_DISK_BUS_USB:
> - diskPriv->qomName = g_strdup_printf("/machine/peripheral/%s/%s.0/legacy[0]",
> - disk->info.alias, disk->info.alias);
> + switch (disk->model) {
> + case VIR_DOMAIN_DISK_MODEL_USB_STORAGE:
> + diskPriv->qomName = g_strdup_printf("/machine/peripheral/%s/%s.0/legacy[0]",
> + disk->info.alias, disk->info.alias);
> + break;
> +
> + case VIR_DOMAIN_DISK_MODEL_USB_BOT:
> + diskPriv->qomName = g_strdup_printf("/machine/peripheral/%s-disk",
I noticed that this is supposed to be "%s-device" to format the proper
qom name:
virsh qemu-monitor-command --pretty fedora41-mig query-block
{
"return": [
[...]
{
"io-status": "ok",
"device": "",
"locked": false,
"removable": false,
"inserted": {
"iops_rd": 0,
"detect_zeroes": "off",
"active": true,
"image": {
"virtual-size": 104857600,
"filename": "/tmp/img1",
"format": "file",
"actual-size": 104857600,
"format-specific": {
"type": "file",
"data": {}
},
"dirty-flag": false
},
"iops_wr": 0,
"ro": false,
"node-name": "libvirt-6-storage",
"backing_file_depth": 0,
"drv": "file",
"iops": 0,
"bps_wr": 0,
"write_threshold": 0,
"encrypted": false,
"bps": 0,
"bps_rd": 0,
"cache": {
"no-flush": false,
"direct": false,
"writeback": true
},
"file": "/tmp/img1"
},
"qdev": "usb-disk0-device",
^^^^^^^^^^^^^^^^
"type": "unknown"
},
[...]
I'll update it before pushing.
On Mon, Jun 23, 2025 at 21:59:18 +0200, Peter Krempa wrote:
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
>
> 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@daynix.com>
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
> 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@redhat.com>
On Tue, Jun 24, 2025 at 14:57:13 +0200, Jiri Denemark wrote:
> On Mon, Jun 23, 2025 at 21:59:18 +0200, Peter Krempa wrote:
> > From: Akihiko Odaki <akihiko.odaki@daynix.com>
> >
> > 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@daynix.com>
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> > 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 == ...)
Yeah; this was inspired by the block above that also starts with:
if (rc == 0)
>
> > +
> > 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?
I used this as the final check that makes sure that a model was picked,
as other startup code relies on the fact that it's already selected.
Now the post-parse code should not do any rejection of configs here in
case when e.g. the installed qemu will not have any of thos features, so
this case should remain here, so that the VMs don't vanish.
Although the error should probably be improved.
> > 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@redhat.com>
>
© 2016 - 2025 Red Hat, Inc.