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.od...@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; > } >
