On Tue, Jun 24, 2025 at 12:51:57 +0200, Peter Krempa wrote: > On Tue, Jun 24, 2025 at 11:25:03 +0200, Jiri Denemark wrote: > > On Mon, Jun 23, 2025 at 21:59:13 +0200, Peter Krempa wrote: > > > From: Peter Krempa <pkre...@redhat.com> > > > > > > Historically libvirt specified 'usb-storage' as driver for USB disks. > > > This though combined with '-blockdev' doesn't properly configure the > > > device to look like CDROM for <disk type='cdrom'>. > > > > > > 'usb-bot' acts like a controler and allows explicitly connecting a > > > -device to it. > > > > > > In qemu the devices share implementation so they are effectively > > > identical and can be used interchangably. There is though a slight > > > difference in how the storage device itself (the SCSI bit) looks when > > > CDROM as they were not declared as cdrom before. > > > > Looks like some words were dropped from the last sentence above. > > > > > As this is effectively a bugfix we'll be fixing the behaviour for the > > > default configuration. The possibility to explicitly set the model is > > > added as a possibility for working around possible problems if they'd > > > appear. > > > > > > Signed-off-by: Peter Krempa <pkre...@redhat.com> > > > --- > > [...] > > > > + ``<disk type='cdrom'>`` is properly exposed as a cdrom device > > > inside the > > > + guest OS. Unfortunately this configuration is not ABI compatible > > > and thus > > > + it can't be interchanged. > > > > This is not ABI compatible with usb-storage model, right? I suggest > > being explicit about it. > > > > > + > > > + The QEMU hypervisor driver will pick ``usb-bot`` for cold starts or > > > + hotplug for cdrom devices to properly configure the devices. This > > > is > > > + not compatible for migration to older versions of libvirt and > > > explicit > > > + configuration needs to be used. > > > > So are you saying starting an existing domain with usb cdrom after > > upgrading libvirt will cause issues during migration to an older > > libvirt? I don't think we can do this. We should rather pick usb-storage > > (which is the case now as I understood from the description) unless > > usb-bot is set explicitly in the XML. > > Yes it will not work if you start a VM and attempt to migrate it to an > older version. The reason why I did this is that this very technically > is a ABI bug fix. > > Prior to use of '-blockdev', when the disk backend was instantiated via > -drive, we'd format 'media=cdrom' with the backend which would be picked > up with the 'usb-storage' frontend to make it into a cdrom: > > Now this went unnoticed at that time. > > While I debated just changing it, the fact that it silently corrupts > inside the guest was a dealbreaker for me. > > But at the same time it's IMO unacceptable to default to the broken > configuration which actually did work before. > > Thus I decided to do this where users can switch to the broken > configuration (or just detach the cdrom) but the config with no extra > bits will result to the proper configuration as it should have before. > > Resulting to the broken config is obviously much easier on the logic > here but I really don't think we should do it.7
As I said in my comments to the following patch, you change the model only when VIR_DOMAIN_DEF_PARSE_ABI_UPDATE and don't change where the flag is actually used (except for tests). So this series is OK. > > > diff --git > > > a/tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.args > > > b/tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.args > > > new file mode 100644 > > > index 0000000000..6d31319a49 > > > --- /dev/null > > > +++ b/tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.args > > > > Well, this is strange. The XML uses both usb-storage and usb-bot, but > > only usb-storage is present in *.args. This kind of makes sense since > > the code for handling model is not wired up yet, but having the tests in > > a commit that describes the new models and how libvirt handles them is > > confusing. > > I thought about activating the capability later, but decided against as > it was showing all updates together. > > So while this ordering creates a history point where this doesn't work , > it's much easier to see what the patches are doing actually. Yeah, I was just thinking the test addition would deserve a dedicated commit to avoid the discrepancy between the test and the documentation in a single patch :-) Reviewed-by: Jiri Denemark <jdene...@redhat.com>
