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.od...@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.od...@daynix.com>
> > Signed-off-by: Peter Krempa <pkre...@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 <jdene...@redhat.com>
>
