Re: [PATCH v2 09/13] qemu: Fill in model of 'usb' disks to preserve ABI compatibility

From: Peter Krempa <pkrempa(a)redhat.com&gt; 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(a)redhat.com&gt; --- 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)