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>
---
src/qemu/qemu_alias.c | 3 +-
src/qemu/qemu_capabilities.c | 2 +-
src/qemu/qemu_command.c | 55 ++++++++++++++++++++--
src/qemu/qemu_command.h | 5 ++
src/qemu/qemu_hotplug.c | 34 ++++++++++---
src/qemu/qemu_validate.c | 4 +-
tests/qemuhotplugtest.c | 8 +++-
.../qemuxmlconfdata/disk-cache.x86_64-latest.args | 3 +-
.../disk-cdrom-bus-other.x86_64-latest.args | 3 +-
.../disk-device-removable.x86_64-latest.args | 3 +-
.../disk-usb-device.x86_64-latest.args | 3 +-
11 files changed, 102 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 3e6bced4a88538a040dd8a65f40dc9f7f56b8ec9..64986368d0588f7778c8e3a09ae3169c1961b456 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -267,8 +267,7 @@ 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);
+ diskPriv->qomName = g_strconcat("scsi", disk->info.alias, NULL);
break;
case VIR_DOMAIN_DISK_BUS_XEN:
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2c7f9cfe994c45dd3bbecc9cd9fff51889adfb0f..b253cb407e8334320cc8ecd49d28d2481d82e9b4 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -6477,7 +6477,7 @@ 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_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 0ad73af335974a236341fa85d02c83f14af08934..29db916e70804ec482392c078a280904a992eaf3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1611,6 +1611,31 @@ 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)
+ return 0;
+
+ if (virJSONValueObjectAdd(&props,
+ "s:driver", "usb-bot",
+ "s:id", disk->info.alias,
+ 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,
@@ -1619,6 +1644,7 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
{
g_autoptr(virJSONValue) props = NULL;
const char *driver = NULL;
+ g_autofree char *usbBusId = NULL;
g_autofree char *scsiVPDDeviceId = NULL;
virTristateSwitch shareRW = VIR_TRISTATE_SWITCH_ABSENT;
g_autofree char *chardev = NULL;
@@ -1637,6 +1663,7 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
const char *rpolicy = NULL;
const char *model = NULL;
const char *product = NULL;
+ const char *id = disk->info.alias;
switch (disk->bus) {
case VIR_DOMAIN_DISK_BUS_IDE:
@@ -1650,6 +1677,17 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
break;
+ case VIR_DOMAIN_DISK_BUS_USB:
+ usbBusId = g_strconcat(disk->info.alias, ".0", NULL);
+ if (virJSONValueObjectAdd(&props,
+ "s:bus", usbBusId,
+ "u:scsi-id", 0,
+ "u:lun", 0,
+ NULL) < 0)
+ return NULL;
+
+ id = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName;
+ G_GNUC_FALLTHROUGH;
case VIR_DOMAIN_DISK_BUS_SCSI:
if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
driver = "scsi-block";
@@ -1719,8 +1757,6 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
}
break;
- case VIR_DOMAIN_DISK_BUS_USB:
- driver = "usb-storage";
if (disk->removable == VIR_TRISTATE_SWITCH_ABSENT)
removable = VIR_TRISTATE_SWITCH_OFF;
@@ -1755,7 +1791,8 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
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 (disk->bus != VIR_DOMAIN_DISK_BUS_USB &&
+ qemuBuildDeviceAddressProps(props, def, &disk->info) < 0)
return NULL;
if (disk->src->shared)
@@ -1816,7 +1853,7 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
"T:share-rw", shareRW,
"S:drive", drive,
"S:chardev", chardev,
- "s:id", disk->info.alias,
+ "s:id", id,
"p:bootindex", bootindex,
"S:loadparm", bootLoadparm,
"p:logical_block_size", logical_block_size,
@@ -2110,6 +2147,16 @@ qemuBuildDiskCommandLine(virCommand *cmd,
if (qemuCommandAddExtDevice(cmd, &disk->info, def, qemuCaps) < 0)
return -1;
+ if (qemuBuildDiskBusProps(def, disk, &devprops) < 0)
+ return -1;
+
+ if (devprops) {
+ if (qemuBuildDeviceCommandlineFromJSON(cmd, devprops, def, qemuCaps) < 0)
+ return -1;
+
+ g_clear_pointer(&devprops, virJSONValueFree);
+ }
+
if (!(devprops = qemuBuildDiskDeviceProps(def, disk, qemuCaps)))
return -1;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 76c514b5f744c60e433f0d8d8760a8730c886eed..48b8ed8ac58d005503c8bdb7e255af097a548fd9 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -116,6 +116,11 @@ qemuBlockStorageSourceChainData *
qemuBuildStorageSourceChainAttachPrepareBlockdevTop(virStorageSource *top,
virStorageSource *backingStore);
+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 28ca321c5c194678d25d67aa02ec82c13175f4f5..cfbaf195bd0a8991c8902795560bda3ee85e42bc 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -31,6 +31,7 @@
#include "qemu_command.h"
#include "qemu_hostdev.h"
#include "qemu_interface.h"
+#include "qemu_monitor_json.h"
#include "qemu_passt.h"
#include "qemu_process.h"
#include "qemu_security.h"
@@ -709,8 +710,10 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
{
g_autoptr(qemuBlockStorageSourceChainData) data = 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;
@@ -766,6 +769,9 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
}
}
+ if (qemuBuildDiskBusProps(vm->def, disk, &busprops) < 0)
+ goto rollback;
+
if (!(devprops = qemuBuildDiskDeviceProps(vm->def, disk, priv->qemuCaps)))
goto rollback;
@@ -775,16 +781,20 @@ 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);
- /* Setup throttling of disk via block_set_io_throttle QMP command. This
- * is a hack until the 'throttle' blockdev driver will support modification
- * of the trhottle group. See also qemuProcessSetupDiskThrottlingBlockdev.
- * As there isn't anything sane to do if this fails, let's just return
- * success.
- */
if (rc == 0) {
+ /* Setup throttling of disk via block_set_io_throttle QMP command. This
+ * is a hack until the 'throttle' blockdev driver will support modification
+ * of the trhottle group. See also qemuProcessSetupDiskThrottlingBlockdev.
+ * As there isn't anything sane to do if this fails, let's just return
+ * success.
+ */
qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
g_autoptr(GHashTable) blockinfo = NULL;
@@ -801,6 +811,15 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
qemuProcessRefreshDiskProps(disk, diskinfo);
}
}
+
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
+ qemuMonitorJSONObjectProperty prop = {
+ .type = QEMU_MONITOR_OBJECT_PROPERTY_BOOLEAN,
+ .val.b = true,
+ };
+
+ rc = qemuMonitorJSONSetObjectProperty(priv->mon, disk->info.alias, "attached", &prop);
+ }
}
qemuDomainObjExitMonitor(vm);
@@ -814,6 +833,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 f3ef1be660b1cbbfe8ed5643baf2e9f4a63cf60d..f9ec3a7c076c3755e1c3ddf3c63aeeb7ed576f2b 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3146,9 +3146,9 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk,
break;
case VIR_DOMAIN_DISK_BUS_USB:
- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) {
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_BOT)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("This QEMU doesn't support '-device usb-storage'"));
+ _("This QEMU doesn't support '-device usb-bot'"));
return -1;
}
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index d2a1f5acf1c17236f0cc038fda43e4f3052cc28b..96ab222836f2594ce7dbd88236ebfe704c18838b 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -590,7 +590,9 @@ mymain(void)
DO_TEST_ATTACH("x86_64", "base-live", "disk-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", "disk-usb", true, true,
"device_del", QMP_OK);
DO_TEST_DETACH("x86_64", "base-live", "disk-usb", false, false,
@@ -746,7 +748,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-cache.x86_64-latest.args b/tests/qemuxmlconfdata/disk-cache.x86_64-latest.args
index 27be6441774b13dc0c491328431aed6ccd3eddaa..206f8ab9ca46008ff1895730159838aa7dd1e018 100644
--- a/tests/qemuxmlconfdata/disk-cache.x86_64-latest.args
+++ b/tests/qemuxmlconfdata/disk-cache.x86_64-latest.args
@@ -42,7 +42,8 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
-device '{"driver":"virtio-blk-pci","bus":"pci.0","addr":"0x3","drive":"libvirt-2-format","id":"virtio-disk0","write-cache":"off"}' \
-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap","cache":{"direct":true,"no-flush":false}}' \
-blockdev '{"node-name":"libvirt-1-format","read-only":false,"cache":{"direct":true,"no-flush":false},"driver":"qcow2","file":"libvirt-1-storage"}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1","drive":"libvirt-1-format","id":"usb-disk1","removable":false,"write-cache":"off"}' \
+-device '{"driver":"usb-bot","id":"usb-disk1","bus":"usb.0","port":"1"}' \
+-device '{"bus":"usb-disk1.0","scsi-id":0,"lun":0,"driver":"scsi-hd","device_id":"drive-usb-disk1","drive":"libvirt-1-format","id":"scsiusb-disk1","write-cache":"off"}' \
-audiodev '{"id":"audio1","driver":"none"}' \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
-msg timestamp=on
diff --git a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args
index e1406af663625bbb1c27203450bc0135af3861e7..8e5c60ac1ec185d1b717b3b22ceb07b721b54846 100644
--- a/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args
+++ b/tests/qemuxmlconfdata/disk-cdrom-bus-other.x86_64-latest.args
@@ -28,7 +28,8 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
-boot strict=on \
-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
-blockdev '{"driver":"file","filename":"/root/boot.iso","node-name":"libvirt-1-storage","read-only":true}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1","drive":"libvirt-1-storage","id":"usb-disk0","removable":false}' \
+-device '{"driver":"usb-bot","id":"usb-disk0","bus":"usb.0","port":"1"}' \
+-device '{"bus":"usb-disk0.0","scsi-id":0,"lun":0,"driver":"scsi-cd","device_id":"drive-usb-disk0","drive":"libvirt-1-storage","id":"scsiusb-disk0"}' \
-audiodev '{"id":"audio1","driver":"none"}' \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
-msg timestamp=on
diff --git a/tests/qemuxmlconfdata/disk-device-removable.x86_64-latest.args b/tests/qemuxmlconfdata/disk-device-removable.x86_64-latest.args
index e0701f4bd25d79b60dc4f9294a603d0a7cd9f0b1..d9b259bc59cc54b10ede57e696e553b3d5f2ef06 100644
--- a/tests/qemuxmlconfdata/disk-device-removable.x86_64-latest.args
+++ b/tests/qemuxmlconfdata/disk-device-removable.x86_64-latest.args
@@ -31,7 +31,8 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-3-storage","read-only":false}' \
-device '{"driver":"ide-hd","bus":"ide.0","unit":0,"drive":"libvirt-3-storage","id":"ide0-0-0","bootindex":1}' \
-blockdev '{"driver":"file","filename":"/tmp/usbdisk.img","node-name":"libvirt-2-storage","read-only":false}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1","drive":"libvirt-2-storage","id":"usb-disk0","removable":true}' \
+-device '{"driver":"usb-bot","id":"usb-disk0","bus":"usb.0","port":"1"}' \
+-device '{"bus":"usb-disk0.0","scsi-id":0,"lun":0,"driver":"scsi-hd","device_id":"drive-usb-disk0","drive":"libvirt-2-storage","id":"scsiusb-disk0","removable":true}' \
-blockdev '{"driver":"file","filename":"/tmp/scsidisk.img","node-name":"libvirt-1-storage","read-only":false}' \
-device '{"driver":"scsi-hd","bus":"scsi0.0","channel":0,"scsi-id":0,"lun":1,"device_id":"drive-scsi0-0-0-1","drive":"libvirt-1-storage","id":"scsi0-0-0-1","removable":true}' \
-audiodev '{"id":"audio1","driver":"none"}' \
diff --git a/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.args b/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.args
index 0fd7e755b1384227b63d6a080bd6af947596ae06..534780faf1a34af1a60dcca4dc71f91943267ea9 100644
--- a/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.args
+++ b/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.args
@@ -30,7 +30,8 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-2-storage","read-only":false}' \
-device '{"driver":"ide-hd","bus":"ide.0","unit":0,"drive":"libvirt-2-storage","id":"ide0-0-0","bootindex":1}' \
-blockdev '{"driver":"file","filename":"/tmp/usbdisk.img","node-name":"libvirt-1-storage","read-only":false}' \
--device '{"driver":"usb-storage","bus":"usb.0","port":"1","drive":"libvirt-1-storage","id":"usb-disk0","removable":false}' \
+-device '{"driver":"usb-bot","id":"usb-disk0","bus":"usb.0","port":"1"}' \
+-device '{"bus":"usb-disk0.0","scsi-id":0,"lun":0,"driver":"scsi-hd","device_id":"drive-usb-disk0","drive":"libvirt-1-storage","id":"scsiusb-disk0"}' \
-audiodev '{"id":"audio1","driver":"none"}' \
-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
--
2.48.1On Sat, Mar 08, 2025 at 14:57:41 +0900, Akihiko Odaki wrote:
> 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.
Mention that the devices are ABI compatible so the patch is replacing
the old implementation without keeping the legacy code.
>
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/368
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> src/qemu/qemu_alias.c | 3 +-
> src/qemu/qemu_capabilities.c | 2 +-
> src/qemu/qemu_command.c | 55 ++++++++++++++++++++--
> src/qemu/qemu_command.h | 5 ++
> src/qemu/qemu_hotplug.c | 34 ++++++++++---
> src/qemu/qemu_validate.c | 4 +-
> tests/qemuhotplugtest.c | 8 +++-
> .../qemuxmlconfdata/disk-cache.x86_64-latest.args | 3 +-
> .../disk-cdrom-bus-other.x86_64-latest.args | 3 +-
> .../disk-device-removable.x86_64-latest.args | 3 +-
> .../disk-usb-device.x86_64-latest.args | 3 +-
> 11 files changed, 102 insertions(+), 21 deletions(-)
Sorry for not forgetting this series for long time ...
>
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index 3e6bced4a88538a040dd8a65f40dc9f7f56b8ec9..64986368d0588f7778c8e3a09ae3169c1961b456 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -267,8 +267,7 @@ 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);
> + diskPriv->qomName = g_strconcat("scsi", disk->info.alias, NULL);a
What you create here is not the qom path. when used it's reported as
/machine/peripheral/scsiusb-disk0
This'll most likely cause failures if throttling were applied but I
didn't test this.
When you change this make sure to re-test it as qemu sends 2
DEVICE_DELETED events in this case. Fixing the above may make libvirt to
trigger on both.
> break;
>
> case VIR_DOMAIN_DISK_BUS_XEN:
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 0ad73af335974a236341fa85d02c83f14af08934..29db916e70804ec482392c078a280904a992eaf3 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1611,6 +1611,31 @@ qemuBuildIothreadMappingProps(GSList *iothreads)
> return g_steal_pointer(&ret);
> }
>
> +int
> +qemuBuildDiskBusProps(const virDomainDef *def,
> + const virDomainDiskDef *disk,
> + virJSONValue **propsRet)
Normally this would be a "controller" the disk would just connect to it.
In case when we'd want to use it as a direct replacement libvirt doesn't
yet have any mechanism to allow adding controller and disk separately.
Also it wouldn't make sense beceause we want it to be a direct
replacement.
So doing this is okay but the function name ought to be limited to just
USB controllers, because this results in another -device (the
controller) and not just "bus properties".
how about qemuBuildDiskUSBBotProps
> +{
> + g_autoptr(virJSONValue) props = NULL;
> +
> + *propsRet = NULL;
> +
> + if (disk->bus != VIR_DOMAIN_DISK_BUS_USB)
> + return 0;
> +
> + if (virJSONValueObjectAdd(&props,
> + "s:driver", "usb-bot",
> + "s:id", disk->info.alias,
> + 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,
> @@ -1619,6 +1644,7 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
> {
> g_autoptr(virJSONValue) props = NULL;
> const char *driver = NULL;
> + g_autofree char *usbBusId = NULL;
> g_autofree char *scsiVPDDeviceId = NULL;
> virTristateSwitch shareRW = VIR_TRISTATE_SWITCH_ABSENT;
> g_autofree char *chardev = NULL;
> @@ -1637,6 +1663,7 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
> const char *rpolicy = NULL;
> const char *model = NULL;
> const char *product = NULL;
> + const char *id = disk->info.alias;
>
> switch (disk->bus) {
> case VIR_DOMAIN_DISK_BUS_IDE:
> @@ -1650,6 +1677,17 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
>
> break;
>
> + case VIR_DOMAIN_DISK_BUS_USB:
> + usbBusId = g_strconcat(disk->info.alias, ".0", NULL);
> + if (virJSONValueObjectAdd(&props,
> + "s:bus", usbBusId,
> + "u:scsi-id", 0,
> + "u:lun", 0,
> + NULL) < 0)
Formatting this here makes it appear before 'driver'. While not a
problem for qemu it makes it harder when looking at commandlines to see
what's happening.
Please move the USB address formatting after the pre-existing props are
formatted even if it involves another block/check.
> + return NULL;
> +
> + id = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName;
> + G_GNUC_FALLTHROUGH;
Also add comment why we are falling trhough.
> case VIR_DOMAIN_DISK_BUS_SCSI:
> if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
> driver = "scsi-block";
> @@ -1719,8 +1757,6 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
> }
> break;
>
> - case VIR_DOMAIN_DISK_BUS_USB:
> - driver = "usb-storage";
>
> if (disk->removable == VIR_TRISTATE_SWITCH_ABSENT)
> removable = VIR_TRISTATE_SWITCH_OFF;
> @@ -1755,7 +1791,8 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
> if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)
> disk->info.addr.drive.diskbus = disk->bus;
>
> - if (qemuBuildDeviceAddressProps(props, def, &disk->info) < 0)
Please add a comment why the address is not formatted here.
> + if (disk->bus != VIR_DOMAIN_DISK_BUS_USB &&
> + qemuBuildDeviceAddressProps(props, def, &disk->info) < 0)
> return NULL;
>
> if (disk->src->shared)
> @@ -1816,7 +1853,7 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
> "T:share-rw", shareRW,
> "S:drive", drive,
> "S:chardev", chardev,
> - "s:id", disk->info.alias,
> + "s:id", id,
> "p:bootindex", bootindex,
> "S:loadparm", bootLoadparm,
> "p:logical_block_size", logical_block_size,
> @@ -2110,6 +2147,16 @@ qemuBuildDiskCommandLine(virCommand *cmd,
> if (qemuCommandAddExtDevice(cmd, &disk->info, def, qemuCaps) < 0)
> return -1;
>
> + if (qemuBuildDiskBusProps(def, disk, &devprops) < 0)
> + return -1;
> +
> + if (devprops) {
Change the name to 'usbprops ...
> + if (qemuBuildDeviceCommandlineFromJSON(cmd, devprops, def, qemuCaps) < 0)
> + return -1;
> +
> + g_clear_pointer(&devprops, virJSONValueFree);
and don't reuse the variable.
> + }
> +
> if (!(devprops = qemuBuildDiskDeviceProps(def, disk, qemuCaps)))
> return -1;
>
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 76c514b5f744c60e433f0d8d8760a8730c886eed..48b8ed8ac58d005503c8bdb7e255af097a548fd9 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -116,6 +116,11 @@ qemuBlockStorageSourceChainData *
> qemuBuildStorageSourceChainAttachPrepareBlockdevTop(virStorageSource *top,
> virStorageSource *backingStore);
>
> +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 28ca321c5c194678d25d67aa02ec82c13175f4f5..cfbaf195bd0a8991c8902795560bda3ee85e42bc 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -31,6 +31,7 @@
> #include "qemu_command.h"
> #include "qemu_hostdev.h"
> #include "qemu_interface.h"
> +#include "qemu_monitor_json.h"
> #include "qemu_passt.h"
> #include "qemu_process.h"
> #include "qemu_security.h"
> @@ -709,8 +710,10 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
> {
> g_autoptr(qemuBlockStorageSourceChainData) data = 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;
> @@ -766,6 +769,9 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
> }
> }
>
> + if (qemuBuildDiskBusProps(vm->def, disk, &busprops) < 0)
> + goto rollback;
Move the preparation of the property object before code that needs to be
rolled back.
I need to also do the same for other bits of code that were added
recently and I missed that during review.
> +
> if (!(devprops = qemuBuildDiskDeviceProps(vm->def, disk, priv->qemuCaps)))
> goto rollback;
>
> @@ -775,16 +781,20 @@ 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);
>
> - /* Setup throttling of disk via block_set_io_throttle QMP command. This
> - * is a hack until the 'throttle' blockdev driver will support modification
> - * of the trhottle group. See also qemuProcessSetupDiskThrottlingBlockdev.
> - * As there isn't anything sane to do if this fails, let's just return
> - * success.
> - */
> if (rc == 0) {
> + /* Setup throttling of disk via block_set_io_throttle QMP command. This
> + * is a hack until the 'throttle' blockdev driver will support modification
> + * of the trhottle group. See also qemuProcessSetupDiskThrottlingBlockdev.
> + * As there isn't anything sane to do if this fails, let's just return
> + * success.
> + */
> qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> g_autoptr(GHashTable) blockinfo = NULL;
>
> @@ -801,6 +811,15 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
> qemuProcessRefreshDiskProps(disk, diskinfo);
> }
> }
> +
> + if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
> + qemuMonitorJSONObjectProperty prop = {
> + .type = QEMU_MONITOR_OBJECT_PROPERTY_BOOLEAN,
> + .val.b = true,
> + };
> +
> + rc = qemuMonitorJSONSetObjectProperty(priv->mon, disk->info.alias, "attached", &prop);
> + }
Preferrably use another block starting with if (rc == 0). The
optimization to reuse the block which doesn't actually modify 'rc' may
obscure the fact that this does.
> }
>
> qemuDomainObjExitMonitor(vm);
> @@ -814,6 +833,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 f3ef1be660b1cbbfe8ed5643baf2e9f4a63cf60d..f9ec3a7c076c3755e1c3ddf3c63aeeb7ed576f2b 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -3146,9 +3146,9 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk,
> break;
>
> case VIR_DOMAIN_DISK_BUS_USB:
> - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) {
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_BOT)) {
This will break hotplug for existing running VMs if libvirt is upgraded.
Libvirt will not reprobe the capabilities of the active VM and this
implementation is replacing the detection here with a new flag. A flag
which didn't exist at the time the VM was started and so it will not
appear.
Since it's being replaced by a supposedly identical configuration (From
guest OS point of view) we don't actually need to keep the legacy code
path around. What we need though is to reuse existing capability.
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("This QEMU doesn't support '-device usb-storage'"));
> + _("This QEMU doesn't support '-device usb-bot'"));
> return -1;
> }
>
On Sat, Mar 08, 2025 at 02:57:41PM +0900, Akihiko Odaki wrote: > 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. Is this change compatible for QEMU's VMState aka migration ABI compatible ? > > Resolves: https://gitlab.com/libvirt/libvirt/-/issues/368 > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > src/qemu/qemu_alias.c | 3 +- > src/qemu/qemu_capabilities.c | 2 +- > src/qemu/qemu_command.c | 55 ++++++++++++++++++++-- > src/qemu/qemu_command.h | 5 ++ > src/qemu/qemu_hotplug.c | 34 ++++++++++--- > src/qemu/qemu_validate.c | 4 +- > tests/qemuhotplugtest.c | 8 +++- > .../qemuxmlconfdata/disk-cache.x86_64-latest.args | 3 +- > .../disk-cdrom-bus-other.x86_64-latest.args | 3 +- > .../disk-device-removable.x86_64-latest.args | 3 +- > .../disk-usb-device.x86_64-latest.args | 3 +- > 11 files changed, 102 insertions(+), 21 deletions(-) With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Mar 10, 2025 at 09:05:28 +0000, Daniel P. Berrangé wrote: > On Sat, Mar 08, 2025 at 02:57:41PM +0900, Akihiko Odaki wrote: > > 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. > > Is this change compatible for QEMU's VMState aka migration ABI compatible ? Based on my tests so far it seems compatible in terms of VMstate and also guest ABI at least to the first glance (lsusb, lsscsi -v etc) seems identical at least for the default 'read-write'/non-cdrom disk. I didn't yet test migrating with a cdrom to an older daemon though, which based on the above paragraph should have guest-visible difference.
On 2025/03/10 18:10, Peter Krempa wrote: > On Mon, Mar 10, 2025 at 09:05:28 +0000, Daniel P. Berrangé wrote: >> On Sat, Mar 08, 2025 at 02:57:41PM +0900, Akihiko Odaki wrote: >>> 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. >> >> Is this change compatible for QEMU's VMState aka migration ABI compatible ? > > Based on my tests so far it seems compatible in terms of VMstate and > also guest ABI at least to the first glance (lsusb, lsscsi -v etc) seems > identical at least for the default 'read-write'/non-cdrom disk. > > I didn't yet test migrating with a cdrom to an older daemon though, > which based on the above paragraph should have guest-visible difference. > It may work but QEMU doesn't guarantee that. Do you have a suggestion to deal with migration? We can keep using usb-storage for hard disk to avoid breaking, but for CD-ROM, we will need to revive the -drive option support (it was used instead of -blockdev and able to create a CD-ROM drive with usb-storage). Honestly I don't think anybody migrates a VM with a USB storage and I guess it's not worth keeping usb-storage and the -drive. Regards, Akihiko Odaki
On Mon, Mar 10, 2025 at 10:10:53AM +0100, Peter Krempa wrote: > On Mon, Mar 10, 2025 at 09:05:28 +0000, Daniel P. Berrangé wrote: > > On Sat, Mar 08, 2025 at 02:57:41PM +0900, Akihiko Odaki wrote: > > > 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. > > > > Is this change compatible for QEMU's VMState aka migration ABI compatible ? > > Based on my tests so far it seems compatible in terms of VMstate and > also guest ABI at least to the first glance (lsusb, lsscsi -v etc) seems > identical at least for the default 'read-write'/non-cdrom disk. > > I didn't yet test migrating with a cdrom to an older daemon though, > which based on the above paragraph should have guest-visible difference. Any such configuratin of the latter is arguably a bug though ? It was never presenting a cdrom, and that we allowed it was just a sign of missing XML validation surely ? With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Mar 10, 2025 at 09:13:11 +0000, Daniel P. Berrangé wrote: > On Mon, Mar 10, 2025 at 10:10:53AM +0100, Peter Krempa wrote: > > On Mon, Mar 10, 2025 at 09:05:28 +0000, Daniel P. Berrangé wrote: > > > On Sat, Mar 08, 2025 at 02:57:41PM +0900, Akihiko Odaki wrote: > > > > 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. > > > > > > Is this change compatible for QEMU's VMState aka migration ABI compatible ? > > > > Based on my tests so far it seems compatible in terms of VMstate and > > also guest ABI at least to the first glance (lsusb, lsscsi -v etc) seems > > identical at least for the default 'read-write'/non-cdrom disk. > > > > I didn't yet test migrating with a cdrom to an older daemon though, > > which based on the above paragraph should have guest-visible difference. > > Any such configuratin of the latter is arguably a bug though ? It was > never presenting a cdrom, and that we allowed it was just a sign of > missing XML validation surely ? Yes that could be the case. Although it is possible that it's even a regression from switching to blockdev as I've seen claims that usb cdrom did behave correctly at that point; but I didn't care to validate that for myself given how long ago it was that we switched to blockdev.
© 2016 - 2026 Red Hat, Inc.