3:26 p.m.
On 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(a)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;
}