From: Peter Krempa <pkrempa@redhat.com>
While 'usb-bot' and 'usb-storage' are ABI and migration compatible for
disks it's not the case for cdroms. When migrating from a new config
using 'usb-bot' to an older daemon which would use 'usb-storage' the
guest os will get I/O errors.
Thus we must properly fill in models for 'usb' disks so that cdroms can
be handled properly.
When parsing XML fill in the models and drop the appropriate models when
formatting migratable XML.
The logic is explained in comments in the code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
src/qemu/qemu_domain.c | 21 ++++++++
src/qemu/qemu_postparse.c | 49 ++++++++++++++++++-
src/qemu/qemu_postparse.h | 4 +-
tests/qemublocktest.c | 13 +++--
.../qemuhotplug-base-live+cdrom-usb.xml | 2 +-
.../qemuhotplug-base-live+disk-usb.xml | 2 +-
.../disk-cache.x86_64-latest.xml | 2 +-
.../disk-usb-device-model.x86_64-latest.xml | 4 +-
...test.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml | 24 ++++-----
...date.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml | 24 ++++-----
...sk-usb-device.x86_64-latest.abi-update.xml | 24 ++++-----
.../disk-usb-device.x86_64-latest.xml | 24 ++++-----
12 files changed, 132 insertions(+), 61 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ace42b516a..6e147563f3 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5342,6 +5342,27 @@ qemuDomainDefFormatBufInternal(virQEMUDriver *driver,
}
}
+ for (i = 0; i < def->ndisks; i++) {
+ virDomainDiskDef *disk = def->disks[i];
+
+ /* The 'mode' property for USB disks was introduced long after USB
+ * disks to allow switching between 'usb-storage' and 'usb-bot'
+ * device. Despite sharing identical implementation 'usb-bot' allows
+ * proper configuration of USB cdroms. Unfortunately it is not ABI
+ * compatible.
+ *
+ * To preserve migration to older daemons we can strip the model to
+ * the default if:
+ * - it's a normal disk (not cdrom) as both are identical
+ * - for a usb-cdrom strip the model if it's not 'usb-bot' as that
+ * was the old configuration
+ */
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_USB &&
+ (disk->model == VIR_DOMAIN_DISK_MODEL_USB_STORAGE ||
+ disk->device == VIR_DOMAIN_DISK_DEVICE_DISK))
+ disk->model = VIR_DOMAIN_DISK_MODEL_DEFAULT;
+ }
+
/* Replace the CPU definition updated according to QEMU with the one
* used for starting the domain. The updated def will be sent
* separately for backward compatibility.
diff --git a/src/qemu/qemu_postparse.c b/src/qemu/qemu_postparse.c
index 8150dffac6..7db378c5ce 100644
--- a/src/qemu/qemu_postparse.c
+++ b/src/qemu/qemu_postparse.c
@@ -202,7 +202,8 @@ qemuDomainDeviceDiskDefPostParseRestoreSecAlias(virDomainDiskDef *disk,
int
qemuDomainDeviceDiskDefPostParse(virDomainDiskDef *disk,
- unsigned int parseFlags)
+ unsigned int parseFlags,
+ virQEMUCaps *qemuCaps)
{
virStorageSource *n;
@@ -220,6 +221,50 @@ qemuDomainDeviceDiskDefPostParse(virDomainDiskDef *disk,
disk->mirror->format == VIR_STORAGE_FILE_NONE)
disk->mirror->format = VIR_STORAGE_FILE_RAW;
+ /* default USB disk model:
+ *
+ * Historically we didn't use model for USB disks. It became necessary once
+ * it turned out that 'usb-storage' doesn't properly expose CDROM devices
+ * with -blockdev. Additionally 'usb-bot' which does properly handle them,
+ * while having identical implementation in qemu and driver in guest, are
+ * not in fact ABI compatible. Thus the logic is as follows:
+ *
+ * If ABI update is not allowed:
+ * - use 'usb-storage' for either (unless only 'usb-bot' is supported)
+ *
+ * If ABI update is possible
+ * - for VIR_DOMAIN_DISK_DEVICE_DISK use 'usb-storage' as it doesn't matter
+ * (it is identical with 'usb-bot' ABI wise)
+ * - for VIR_DOMAIN_DISK_DEVICE_CDROM use 'usb-bot' if available
+ * (as it properly exposes cdrom)
+ *
+ * When formatting migratable XML the code strips 'usb-storage' to preserve
+ * migration to older daemons. If a new definition with 'usb-bot' cdrom is
+ * created via new start or hotplug it will fail migrating. Users must
+ * explicitly set the broken config in XML or unplug the device.
+ */
+ if (qemuCaps &&
+ disk->bus == VIR_DOMAIN_DISK_BUS_USB &&
+ disk->model == VIR_DOMAIN_DISK_MODEL_DEFAULT) {
+
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM &&
+ parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) {
+
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_BOT)) {
+ disk->model = VIR_DOMAIN_DISK_MODEL_USB_BOT;
+ } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) {
+ disk->model = VIR_DOMAIN_DISK_MODEL_USB_STORAGE;
+ }
+
+ } else {
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) {
+ disk->model = VIR_DOMAIN_DISK_MODEL_USB_STORAGE;
+ } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_BOT)) {
+ disk->model = VIR_DOMAIN_DISK_MODEL_USB_BOT;
+ }
+ }
+ }
+
/* default disk encryption engine */
for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
if (n->encryption && n->encryption->engine == VIR_STORAGE_ENCRYPTION_ENGINE_DEFAULT)
@@ -843,7 +888,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDef *dev,
break;
case VIR_DOMAIN_DEVICE_DISK:
- ret = qemuDomainDeviceDiskDefPostParse(dev->data.disk, parseFlags);
+ ret = qemuDomainDeviceDiskDefPostParse(dev->data.disk, parseFlags, qemuCaps);
break;
case VIR_DOMAIN_DEVICE_VIDEO:
diff --git a/src/qemu/qemu_postparse.h b/src/qemu/qemu_postparse.h
index ac69c14604..46945adbd6 100644
--- a/src/qemu/qemu_postparse.h
+++ b/src/qemu/qemu_postparse.h
@@ -22,10 +22,12 @@
#pragma once
#include "virconftypes.h"
+#include "qemu_capabilities.h"
int
qemuDomainDeviceDiskDefPostParse(virDomainDiskDef *disk,
- unsigned int parseFlags);
+ unsigned int parseFlags,
+ virQEMUCaps *qemuCaps);
int
qemuDomainDeviceDefPostParse(virDomainDeviceDef *dev,
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index be3e421ac0..095a54e22c 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -271,7 +271,7 @@ testQemuDiskXMLToProps(const void *opaque)
VIR_DOMAIN_DEF_PARSE_STATUS)))
return -1;
- if (qemuDomainDeviceDiskDefPostParse(disk, 0) < 0)
+ if (qemuDomainDeviceDiskDefPostParse(disk, 0, data->qemuCaps) < 0)
return -1;
if (!(vmdef = virDomainDefNew(data->driver->xmlopt)))
@@ -464,7 +464,8 @@ static const char *testQemuImageCreatePath = abs_srcdir "/qemublocktestdata/imag
static virStorageSource *
testQemuImageCreateLoadDiskXML(const char *name,
- virDomainXMLOption *xmlopt)
+ virDomainXMLOption *xmlopt,
+ virQEMUCaps *qemuCaps)
{
g_autoptr(virDomainDiskDef) disk = NULL;
@@ -481,7 +482,7 @@ testQemuImageCreateLoadDiskXML(const char *name,
VIR_DOMAIN_DEF_PARSE_STATUS)))
return NULL;
- if (qemuDomainDeviceDiskDefPostParse(disk, 0) < 0)
+ if (qemuDomainDeviceDiskDefPostParse(disk, 0, qemuCaps) < 0)
return NULL;
return g_steal_pointer(&disk->src);
@@ -502,12 +503,14 @@ testQemuImageCreate(const void *opaque)
g_autofree char *actual = NULL;
g_autofree char *jsonpath = NULL;
- if (!(src = testQemuImageCreateLoadDiskXML(data->name, data->driver->xmlopt)))
+ if (!(src = testQemuImageCreateLoadDiskXML(data->name, data->driver->xmlopt,
+ data->qemuCaps)))
return -1;
if (data->backingname &&
!(src->backingStore = testQemuImageCreateLoadDiskXML(data->backingname,
- data->driver->xmlopt)))
+ data->driver->xmlopt,
+ data->qemuCaps)))
return -1;
if (testQemuDiskXMLToJSONFakeSecrets(src) < 0)
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+cdrom-usb.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+cdrom-usb.xml
index d31136dbc8..bed4dcec14 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+cdrom-usb.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+cdrom-usb.xml
@@ -22,7 +22,7 @@
<on_crash>restart</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-x86_64</emulator>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-bot'>
<driver name='qemu' type='raw' cache='none'/>
<source file='/dev/null' index='1'/>
<backingStore/>
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-usb.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-usb.xml
index 5964c23ba0..6f974892be 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-usb.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-usb.xml
@@ -22,7 +22,7 @@
<on_crash>restart</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-x86_64</emulator>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw' cache='none'/>
<source file='/dev/null' index='1'/>
<backingStore/>
diff --git a/tests/qemuxmlconfdata/disk-cache.x86_64-latest.xml b/tests/qemuxmlconfdata/disk-cache.x86_64-latest.xml
index c770deaaab..a5d9cd388a 100644
--- a/tests/qemuxmlconfdata/disk-cache.x86_64-latest.xml
+++ b/tests/qemuxmlconfdata/disk-cache.x86_64-latest.xml
@@ -41,7 +41,7 @@
<target dev='vda' bus='virtio'/>
<address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
</disk>
- <disk type='block' device='disk'>
+ <disk type='block' device='disk' model='usb-storage'>
<driver name='qemu' type='qcow2' cache='directsync'/>
<source dev='/dev/HostVG/QEMUGuest1'/>
<target dev='sdb' bus='usb'/>
diff --git a/tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.xml b/tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.xml
index 351257bc4a..9f2f383f30 100644
--- a/tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.xml
+++ b/tests/qemuxmlconfdata/disk-usb-device-model.x86_64-latest.xml
@@ -39,12 +39,12 @@
<target dev='sdd' bus='usb'/>
<readonly/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img5'/>
<target dev='sde' bus='usb'/>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img6'/>
<target dev='sdf' bus='usb'/>
diff --git a/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml b/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml
index 8b78f77e63..75e489ede3 100644
--- a/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml
+++ b/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml
@@ -17,51 +17,51 @@
<on_crash>destroy</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-x86_64</emulator>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img1'/>
<target dev='sda' bus='usb'/>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img2'/>
<target dev='sdb' bus='usb'/>
<readonly/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img3'/>
<target dev='sdc' bus='usb'/>
<serial>testserial1</serial>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img4'/>
<target dev='sdd' bus='usb'/>
<readonly/>
<serial>testserial2</serial>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img5'/>
<target dev='sde' bus='usb'/>
<alias name='ua-test1'/>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img6'/>
<target dev='sdf' bus='usb'/>
<readonly/>
<alias name='ua-test2'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img7'/>
<target dev='sdg' bus='usb'/>
<serial>testserial3</serial>
<alias name='ua-test3'/>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img8'/>
<target dev='sdh' bus='usb'/>
@@ -69,24 +69,24 @@
<serial>testserial4</serial>
<alias name='ua-test4'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img9'/>
<target dev='sdi' bus='usb' removable='on'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/imga'/>
<target dev='sdj' bus='usb' removable='on'/>
<serial>testserial5</serial>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/imgb'/>
<target dev='sdk' bus='usb' removable='on'/>
<alias name='ua-test5'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/imgc'/>
<target dev='sdl' bus='usb' removable='on'/>
diff --git a/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.abi-update.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml b/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.abi-update.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml
index 732230e722..b5954973c6 100644
--- a/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.abi-update.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml
+++ b/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.abi-update.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml
@@ -17,27 +17,27 @@
<on_crash>destroy</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-x86_64</emulator>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img1'/>
<target dev='sda' bus='usb'/>
<address type='usb' bus='0' port='1.1'/>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img2'/>
<target dev='sdb' bus='usb'/>
<readonly/>
<address type='usb' bus='0' port='1.2'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img3'/>
<target dev='sdc' bus='usb'/>
<serial>testserial1</serial>
<address type='usb' bus='0' port='1.3'/>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img4'/>
<target dev='sdd' bus='usb'/>
@@ -45,14 +45,14 @@
<serial>testserial2</serial>
<address type='usb' bus='0' port='1.4'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img5'/>
<target dev='sde' bus='usb'/>
<alias name='ua-test1'/>
<address type='usb' bus='0' port='1.5'/>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img6'/>
<target dev='sdf' bus='usb'/>
@@ -60,7 +60,7 @@
<alias name='ua-test2'/>
<address type='usb' bus='0' port='1.6'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img7'/>
<target dev='sdg' bus='usb'/>
@@ -68,7 +68,7 @@
<alias name='ua-test3'/>
<address type='usb' bus='0' port='1.7'/>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img8'/>
<target dev='sdh' bus='usb'/>
@@ -77,27 +77,27 @@
<alias name='ua-test4'/>
<address type='usb' bus='0' port='1.8'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img9'/>
<target dev='sdi' bus='usb' removable='on'/>
<address type='usb' bus='0' port='2.1'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/imga'/>
<target dev='sdj' bus='usb' removable='on'/>
<serial>testserial5</serial>
<address type='usb' bus='0' port='2.2'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/imgb'/>
<target dev='sdk' bus='usb' removable='on'/>
<alias name='ua-test5'/>
<address type='usb' bus='0' port='2.3'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/imgc'/>
<target dev='sdl' bus='usb' removable='on'/>
diff --git a/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.abi-update.xml b/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.abi-update.xml
index 732230e722..b77bf4717c 100644
--- a/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.abi-update.xml
+++ b/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.abi-update.xml
@@ -17,27 +17,27 @@
<on_crash>destroy</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-x86_64</emulator>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img1'/>
<target dev='sda' bus='usb'/>
<address type='usb' bus='0' port='1.1'/>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-bot'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img2'/>
<target dev='sdb' bus='usb'/>
<readonly/>
<address type='usb' bus='0' port='1.2'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img3'/>
<target dev='sdc' bus='usb'/>
<serial>testserial1</serial>
<address type='usb' bus='0' port='1.3'/>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-bot'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img4'/>
<target dev='sdd' bus='usb'/>
@@ -45,14 +45,14 @@
<serial>testserial2</serial>
<address type='usb' bus='0' port='1.4'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img5'/>
<target dev='sde' bus='usb'/>
<alias name='ua-test1'/>
<address type='usb' bus='0' port='1.5'/>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-bot'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img6'/>
<target dev='sdf' bus='usb'/>
@@ -60,7 +60,7 @@
<alias name='ua-test2'/>
<address type='usb' bus='0' port='1.6'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img7'/>
<target dev='sdg' bus='usb'/>
@@ -68,7 +68,7 @@
<alias name='ua-test3'/>
<address type='usb' bus='0' port='1.7'/>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-bot'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img8'/>
<target dev='sdh' bus='usb'/>
@@ -77,27 +77,27 @@
<alias name='ua-test4'/>
<address type='usb' bus='0' port='1.8'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img9'/>
<target dev='sdi' bus='usb' removable='on'/>
<address type='usb' bus='0' port='2.1'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/imga'/>
<target dev='sdj' bus='usb' removable='on'/>
<serial>testserial5</serial>
<address type='usb' bus='0' port='2.2'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/imgb'/>
<target dev='sdk' bus='usb' removable='on'/>
<alias name='ua-test5'/>
<address type='usb' bus='0' port='2.3'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/imgc'/>
<target dev='sdl' bus='usb' removable='on'/>
diff --git a/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.xml b/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.xml
index 8b78f77e63..75e489ede3 100644
--- a/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.xml
+++ b/tests/qemuxmlconfdata/disk-usb-device.x86_64-latest.xml
@@ -17,51 +17,51 @@
<on_crash>destroy</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-x86_64</emulator>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img1'/>
<target dev='sda' bus='usb'/>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img2'/>
<target dev='sdb' bus='usb'/>
<readonly/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img3'/>
<target dev='sdc' bus='usb'/>
<serial>testserial1</serial>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img4'/>
<target dev='sdd' bus='usb'/>
<readonly/>
<serial>testserial2</serial>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img5'/>
<target dev='sde' bus='usb'/>
<alias name='ua-test1'/>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img6'/>
<target dev='sdf' bus='usb'/>
<readonly/>
<alias name='ua-test2'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img7'/>
<target dev='sdg' bus='usb'/>
<serial>testserial3</serial>
<alias name='ua-test3'/>
</disk>
- <disk type='file' device='cdrom'>
+ <disk type='file' device='cdrom' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img8'/>
<target dev='sdh' bus='usb'/>
@@ -69,24 +69,24 @@
<serial>testserial4</serial>
<alias name='ua-test4'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/img9'/>
<target dev='sdi' bus='usb' removable='on'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/imga'/>
<target dev='sdj' bus='usb' removable='on'/>
<serial>testserial5</serial>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/imgb'/>
<target dev='sdk' bus='usb' removable='on'/>
<alias name='ua-test5'/>
</disk>
- <disk type='file' device='disk'>
+ <disk type='file' device='disk' model='usb-storage'>
<driver name='qemu' type='raw'/>
<source file='/tmp/imgc'/>
<target dev='sdl' bus='usb' removable='on'/>
--
2.49.0On Mon, Jun 23, 2025 at 21:59:14 +0200, Peter Krempa wrote:
> From: Peter Krempa <pkrempa@redhat.com>
>
> While 'usb-bot' and 'usb-storage' are ABI and migration compatible for
> disks it's not the case for cdroms. When migrating from a new config
> using 'usb-bot' to an older daemon which would use 'usb-storage' the
> guest os will get I/O errors.
>
> Thus we must properly fill in models for 'usb' disks so that cdroms can
> be handled properly.
>
> When parsing XML fill in the models and drop the appropriate models when
> formatting migratable XML.
>
> The logic is explained in comments in the code.
>
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
> src/qemu/qemu_domain.c | 21 ++++++++
> src/qemu/qemu_postparse.c | 49 ++++++++++++++++++-
> src/qemu/qemu_postparse.h | 4 +-
> tests/qemublocktest.c | 13 +++--
> .../qemuhotplug-base-live+cdrom-usb.xml | 2 +-
> .../qemuhotplug-base-live+disk-usb.xml | 2 +-
> .../disk-cache.x86_64-latest.xml | 2 +-
> .../disk-usb-device-model.x86_64-latest.xml | 4 +-
> ...test.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml | 24 ++++-----
> ...date.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml | 24 ++++-----
> ...sk-usb-device.x86_64-latest.abi-update.xml | 24 ++++-----
> .../disk-usb-device.x86_64-latest.xml | 24 ++++-----
> 12 files changed, 132 insertions(+), 61 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ace42b516a..6e147563f3 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5342,6 +5342,27 @@ qemuDomainDefFormatBufInternal(virQEMUDriver *driver,
> }
> }
>
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainDiskDef *disk = def->disks[i];
> +
> + /* The 'mode' property for USB disks was introduced long after USB
s/mode/model/
> + * disks to allow switching between 'usb-storage' and 'usb-bot'
> + * device. Despite sharing identical implementation 'usb-bot' allows
> + * proper configuration of USB cdroms. Unfortunately it is not ABI
> + * compatible.
> + *
> + * To preserve migration to older daemons we can strip the model to
> + * the default if:
> + * - it's a normal disk (not cdrom) as both are identical
> + * - for a usb-cdrom strip the model if it's not 'usb-bot' as that
> + * was the old configuration
> + */
> + if (disk->bus == VIR_DOMAIN_DISK_BUS_USB &&
> + (disk->model == VIR_DOMAIN_DISK_MODEL_USB_STORAGE ||
> + disk->device == VIR_DOMAIN_DISK_DEVICE_DISK))
> + disk->model = VIR_DOMAIN_DISK_MODEL_DEFAULT;
> + }
> +
> /* Replace the CPU definition updated according to QEMU with the one
> * used for starting the domain. The updated def will be sent
> * separately for backward compatibility.
> diff --git a/src/qemu/qemu_postparse.c b/src/qemu/qemu_postparse.c
> index 8150dffac6..7db378c5ce 100644
> --- a/src/qemu/qemu_postparse.c
> +++ b/src/qemu/qemu_postparse.c
> @@ -202,7 +202,8 @@ qemuDomainDeviceDiskDefPostParseRestoreSecAlias(virDomainDiskDef *disk,
>
> int
> qemuDomainDeviceDiskDefPostParse(virDomainDiskDef *disk,
> - unsigned int parseFlags)
> + unsigned int parseFlags,
> + virQEMUCaps *qemuCaps)
> {
> virStorageSource *n;
>
> @@ -220,6 +221,50 @@ qemuDomainDeviceDiskDefPostParse(virDomainDiskDef *disk,
> disk->mirror->format == VIR_STORAGE_FILE_NONE)
> disk->mirror->format = VIR_STORAGE_FILE_RAW;
>
> + /* default USB disk model:
> + *
> + * Historically we didn't use model for USB disks. It became necessary once
> + * it turned out that 'usb-storage' doesn't properly expose CDROM devices
> + * with -blockdev. Additionally 'usb-bot' which does properly handle them,
> + * while having identical implementation in qemu and driver in guest, are
> + * not in fact ABI compatible. Thus the logic is as follows:
> + *
> + * If ABI update is not allowed:
> + * - use 'usb-storage' for either (unless only 'usb-bot' is supported)
> + *
> + * If ABI update is possible
> + * - for VIR_DOMAIN_DISK_DEVICE_DISK use 'usb-storage' as it doesn't matter
> + * (it is identical with 'usb-bot' ABI wise)
> + * - for VIR_DOMAIN_DISK_DEVICE_CDROM use 'usb-bot' if available
> + * (as it properly exposes cdrom)
> + *
> + * When formatting migratable XML the code strips 'usb-storage' to preserve
> + * migration to older daemons. If a new definition with 'usb-bot' cdrom is
> + * created via new start or hotplug it will fail migrating. Users must
> + * explicitly set the broken config in XML or unplug the device.
> + */
> + if (qemuCaps &&
> + disk->bus == VIR_DOMAIN_DISK_BUS_USB &&
> + disk->model == VIR_DOMAIN_DISK_MODEL_DEFAULT) {
> +
> + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM &&
> + parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) {
> +
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_BOT)) {
> + disk->model = VIR_DOMAIN_DISK_MODEL_USB_BOT;
> + } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) {
> + disk->model = VIR_DOMAIN_DISK_MODEL_USB_STORAGE;
> + }
> +
> + } else {
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) {
I would merge this with the else above to reduce nesting:
if (DEVICE_CDROM && ABI_UPDATE) {
} else if (MODEL_USB_STORAGE) {
...
} else if (MODEL_USB_BOT) {
...
}
> + disk->model = VIR_DOMAIN_DISK_MODEL_USB_STORAGE;
> + } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_BOT)) {
> + disk->model = VIR_DOMAIN_DISK_MODEL_USB_BOT;
> + }
> + }
> + }
> +
The logic is OK. I guess my comment in the previous patch was actually
about using VIR_DOMAIN_DEF_PARSE_ABI_UPDATE flag. But you're not
touching this part here so I guess everything should be fine. Although
I'm still surprised we'd allow ABI update when starting a domain, I
thought this was only allowed when defining it...
> /* default disk encryption engine */
> for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
> if (n->encryption && n->encryption->engine == VIR_STORAGE_ENCRYPTION_ENGINE_DEFAULT)
Jirka
On Tue, Jun 24, 2025 at 11:47:57 +0200, Jiri Denemark wrote:
> On Mon, Jun 23, 2025 at 21:59:14 +0200, Peter Krempa wrote:
> > From: Peter Krempa <pkrempa@redhat.com>
> >
> > While 'usb-bot' and 'usb-storage' are ABI and migration compatible for
> > disks it's not the case for cdroms. When migrating from a new config
> > using 'usb-bot' to an older daemon which would use 'usb-storage' the
> > guest os will get I/O errors.
> >
> > Thus we must properly fill in models for 'usb' disks so that cdroms can
> > be handled properly.
> >
> > When parsing XML fill in the models and drop the appropriate models when
> > formatting migratable XML.
> >
> > The logic is explained in comments in the code.
> >
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> > src/qemu/qemu_domain.c | 21 ++++++++
> > src/qemu/qemu_postparse.c | 49 ++++++++++++++++++-
> > src/qemu/qemu_postparse.h | 4 +-
> > tests/qemublocktest.c | 13 +++--
> > .../qemuhotplug-base-live+cdrom-usb.xml | 2 +-
> > .../qemuhotplug-base-live+disk-usb.xml | 2 +-
> > .../disk-cache.x86_64-latest.xml | 2 +-
> > .../disk-usb-device-model.x86_64-latest.xml | 4 +-
> > ...test.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml | 24 ++++-----
> > ...date.QEMU_CAPS_DEVICE_USB_BOT-disabled.xml | 24 ++++-----
> > ...sk-usb-device.x86_64-latest.abi-update.xml | 24 ++++-----
> > .../disk-usb-device.x86_64-latest.xml | 24 ++++-----
> > 12 files changed, 132 insertions(+), 61 deletions(-)
...
> The logic is OK. I guess my comment in the previous patch was actually
> about using VIR_DOMAIN_DEF_PARSE_ABI_UPDATE flag. But you're not
> touching this part here so I guess everything should be fine. Although
> I'm still surprised we'd allow ABI update when starting a domain, I
> thought this was only allowed when defining it...
>
> > /* default disk encryption engine */
> > for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
> > if (n->encryption && n->encryption->engine == VIR_STORAGE_ENCRYPTION_ENGINE_DEFAULT)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
© 2016 - 2025 Red Hat, Inc.