[libvirt] [PATCH v2 00/12] qemu: Fix media changing problems
Fixes regression in media changing/disk hotplug as the ordering of the alias allocation and disk preparation was bad. v2: - be more explicit about old and new definitions used in certain steps - clean up legacy hotplug to not access old disk source definition - also treat network disks as 'raw' if the format was not provided Peter Krempa (12): Revert "qemu: hotplug: Prepare disk source in qemuDomainAttachDeviceDiskLive" Revert "qemu: hotplug: consolidate media change code paths" qemu: hotplug: Don't pretend that we support secrets for media change qemu: domain: Assume 'raw' default storage format also for network storage qemu: hotplug: Remove code handling possible missing disk source format qemu: hotplug: Allow specifying explicit source for disk backend hotplug code qemu: hotplug: Be explicit about old/new sources when changing media qemu: hotplug: Prepare disk source for media changing qemu: hotplug: Add wrapper for disk hotplug code qemu: conf: Export qemuAddSharedDisk qemu: hotplug: Split out media change code from disk hotplug qemu: hotplug: Refactor qemuDomainAttachDeviceDiskLiveInternal src/qemu/qemu_conf.c | 2 +- src/qemu/qemu_conf.h | 5 + src/qemu/qemu_domain.c | 4 +- src/qemu/qemu_driver.c | 7 +- src/qemu/qemu_hotplug.c | 276 ++++++++++-------- src/qemu/qemu_hotplug.h | 9 +- tests/qemuhotplugtest.c | 2 +- .../disk-source-pool-mode.args | 6 +- tests/qemuxml2argvdata/disk-source-pool.args | 4 +- .../disk-source-pool-mode.xml | 6 +- tests/qemuxml2xmloutdata/disk-source-pool.xml | 2 +- 11 files changed, 185 insertions(+), 138 deletions(-) -- 2.17.1
Preparing the storage source prior to assigning the alias will not work as the names of the certain objects depend on the alias for the legacy hotplug case as we generate the object names for the secrets based on the alias. This reverts commit 192fdaa614e3800255048a8a70c1292ccf18397a. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4558a3c02d..ccf3336633 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -781,6 +781,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; qemuHotplugDiskSourceDataPtr diskdata = NULL; char *devstr = NULL; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); if (qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, false) < 0) goto cleanup; @@ -788,6 +789,9 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto error; + if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) + goto error; + if (!(diskdata = qemuHotplugDiskSourceAttachPrepare(disk, priv->qemuCaps))) goto error; @@ -822,6 +826,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, qemuHotplugDiskSourceDataFree(diskdata); qemuDomainSecretDiskDestroy(disk); VIR_FREE(devstr); + virObjectUnref(cfg); return ret; exit_monitor: @@ -1062,8 +1067,6 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, bool forceMediaChange) { size_t i; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDiskDefPtr disk = dev->data.disk; virDomainDiskDefPtr orig_disk = NULL; int ret = -1; @@ -1080,9 +1083,6 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, if (qemuDomainDetermineDiskChain(driver, vm, disk, true) < 0) goto cleanup; - if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) - goto cleanup; - switch ((virDomainDiskDevice) disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: case VIR_DOMAIN_DISK_DEVICE_FLOPPY: @@ -1153,7 +1153,6 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, cleanup: if (ret != 0) ignore_value(qemuRemoveSharedDevice(driver, dev, vm->def->name)); - virObjectUnref(cfg); return ret; } -- 2.17.1
While the idea was good the implementation not so much as we need to take into account the old disk data and the new source. The code will be consolidated later in a different way. This reverts commit 663b1d55de652201b19d875f0eff730dc28e689e. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 20 ++++++++++++++++++-- src/qemu/qemu_hotplug.c | 18 +++--------------- src/qemu/qemu_hotplug.h | 9 +++++++-- tests/qemuhotplugtest.c | 2 +- 4 files changed, 29 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ef87a6ef05..91ac3640d3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7600,7 +7600,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, switch ((virDomainDeviceType)dev->type) { case VIR_DOMAIN_DEVICE_DISK: qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk, NULL); - ret = qemuDomainAttachDeviceDiskLive(driver, vm, dev, false); + ret = qemuDomainAttachDeviceDiskLive(driver, vm, dev); if (!ret) { alias = dev->data.disk->info.alias; dev->data.disk = NULL; @@ -7851,6 +7851,12 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm, virDomainDeviceDef oldDev = { .type = dev->type }; int ret = -1; + if (virDomainDiskTranslateSourcePool(disk) < 0) + goto cleanup; + + if (qemuDomainDetermineDiskChain(driver, vm, disk, true) < 0) + goto cleanup; + if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def, disk->bus, disk->dst))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -7879,8 +7885,18 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm, goto cleanup; } - if (qemuDomainAttachDeviceDiskLive(driver, vm, dev, force) < 0) + /* Add the new disk src into shared disk hash table */ + if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0) goto cleanup; + + if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk, + dev->data.disk->src, force) < 0) { + ignore_value(qemuRemoveSharedDisk(driver, dev->data.disk, + vm->def->name)); + goto cleanup; + } + + dev->data.disk->src = NULL; } orig_disk->startupPolicy = dev->data.disk->startupPolicy; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ccf3336633..86afda636e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -722,7 +722,7 @@ qemuDomainChangeMediaBlockdev(virQEMUDriverPtr driver, * * Returns 0 on success, -1 on error and reports libvirt error */ -static int +int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, @@ -1049,22 +1049,10 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, } -/** - * qemuDomainAttachDeviceDiskLive: - * @driver: qemu driver struct - * @vm: domain object - * @dev: device to attach (expected type is DISK) - * @forceMediaChange: Forcibly open the drive if changing media - * - * Attach a new disk or in case of cdroms/floppies change the media in the drive. - * This function handles all the necessary steps to attach a new storage source - * to the VM. If @forceMediaChange is true the drive is opened forcibly. - */ int qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool forceMediaChange) + virDomainDeviceDefPtr dev) { size_t i; virDomainDiskDefPtr disk = dev->data.disk; @@ -1098,7 +1086,7 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, } if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk, - disk->src, forceMediaChange) < 0) + disk->src, false) < 0) goto cleanup; disk->src = NULL; diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index c085c45082..0297e42a98 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -28,6 +28,12 @@ # include "qemu_domain.h" # include "domain_conf.h" +int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk, + virStorageSourcePtr newsrc, + bool force); + void qemuDomainDelTLSObjects(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob, @@ -54,8 +60,7 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, virDomainControllerDefPtr controller); int qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool forceMediaChange); + virDomainDeviceDefPtr dev); int qemuDomainAttachNetDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainNetDefPtr net); diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index e0e248556f..5b1e0db104 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -120,7 +120,7 @@ testQemuHotplugAttach(virDomainObjPtr vm, /* conn in only used for storage pool and secrets lookup so as long * as we don't use any of them, passing NULL should be safe */ - ret = qemuDomainAttachDeviceDiskLive(&driver, vm, dev, false); + ret = qemuDomainAttachDeviceDiskLive(&driver, vm, dev); break; case VIR_DOMAIN_DEVICE_CHR: ret = qemuDomainAttachChrDevice(&driver, vm, dev->data.chr); -- 2.17.1
Old media changing code does not bother setting up the secrets for new media or actually removing/adding of the corresponding objects. Additionally it uses secrets setup for the old image to be removed as the secret for the new image which is wrong. Remove the support for secrets while changing media for the legacy approach. The only reasonable way to fix it is when using blockdev. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 86afda636e..ad7023c085 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -210,8 +210,6 @@ qemuDomainChangeMediaLegacy(virQEMUDriverPtr driver, char *driveAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); - qemuDomainSecretInfoPtr secinfo = NULL; const char *format = NULL; char *sourcestr = NULL; @@ -221,9 +219,6 @@ qemuDomainChangeMediaLegacy(virQEMUDriverPtr driver, goto cleanup; } - if (srcPriv) - secinfo = srcPriv->secinfo; - if (!(driveAlias = qemuAliasDiskDriveFromDisk(disk))) goto cleanup; @@ -252,7 +247,7 @@ qemuDomainChangeMediaLegacy(virQEMUDriverPtr driver, } if (!virStorageSourceIsEmpty(newsrc)) { - if (qemuGetDriveSourceString(newsrc, secinfo, &sourcestr) < 0) + if (qemuGetDriveSourceString(newsrc, NULL, &sourcestr) < 0) goto cleanup; if (virStorageSourceGetActualType(newsrc) != VIR_STORAGE_TYPE_DIR) { -- 2.17.1
Post parse callback adds the 'raw' type only for local files. Remote files can also have backing store (even local) so we should do this also for network backed storage. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 4 +--- tests/qemuxml2argvdata/disk-source-pool-mode.args | 6 +++--- tests/qemuxml2argvdata/disk-source-pool.args | 4 ++-- tests/qemuxml2xmloutdata/disk-source-pool-mode.xml | 6 +++--- tests/qemuxml2xmloutdata/disk-source-pool.xml | 2 +- 5 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f00f1b3fdb..82737bcdee 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6364,9 +6364,7 @@ qemuDomainDeviceDiskDefPostParse(virDomainDiskDefPtr disk, return -1; /* default disk format for drives */ - if (virDomainDiskGetFormat(disk) == VIR_STORAGE_FILE_NONE && - (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_FILE || - virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_BLOCK)) + if (virDomainDiskGetFormat(disk) == VIR_STORAGE_FILE_NONE) virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW); /* default disk format for mirrored drive */ diff --git a/tests/qemuxml2argvdata/disk-source-pool-mode.args b/tests/qemuxml2argvdata/disk-source-pool-mode.args index e8d9aacd77..930d360d16 100644 --- a/tests/qemuxml2argvdata/disk-source-pool-mode.args +++ b/tests/qemuxml2argvdata/disk-source-pool-mode.args @@ -21,16 +21,16 @@ server,nowait \ -no-shutdown \ -no-acpi \ -usb \ --drive file=/some/block/device/unit:0:0:1,if=none,id=drive-ide0-0-1,\ +-drive file=/some/block/device/unit:0:0:1,format=raw,if=none,id=drive-ide0-0-1,\ media=cdrom,readonly=on \ -device ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \ --drive file=iscsi://iscsi.example.com:3260/demo-target/2,if=none,\ +-drive file=iscsi://iscsi.example.com:3260/demo-target/2,format=raw,if=none,\ id=drive-ide0-0-2,media=cdrom,readonly=on \ -device ide-drive,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2 \ -drive file=/tmp/idedisk.img,format=raw,if=none,id=drive-ide0-0-3 \ -device ide-drive,bus=ide.0,unit=3,drive=drive-ide0-0-3,id=ide0-0-3,\ bootindex=1 \ --drive file=iscsi://iscsi.example.com:3260/demo-target/3,if=none,\ +-drive file=iscsi://iscsi.example.com:3260/demo-target/3,format=raw,if=none,\ id=drive-ide0-0-4,media=cdrom,readonly=on \ -device ide-drive,bus=ide.0,unit=4,drive=drive-ide0-0-4,id=ide0-0-4 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/disk-source-pool.args b/tests/qemuxml2argvdata/disk-source-pool.args index 49dc853bcd..fe95aa2250 100644 --- a/tests/qemuxml2argvdata/disk-source-pool.args +++ b/tests/qemuxml2argvdata/disk-source-pool.args @@ -21,8 +21,8 @@ server,nowait \ -no-shutdown \ -no-acpi \ -usb \ --drive file=/some/block/device/cdrom,if=none,id=drive-ide0-0-1,media=cdrom,\ -readonly=on \ +-drive file=/some/block/device/cdrom,format=raw,if=none,id=drive-ide0-0-1,\ +media=cdrom,readonly=on \ -device ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \ -drive if=none,id=drive-ide0-1-0,media=cdrom,readonly=on \ -device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \ diff --git a/tests/qemuxml2xmloutdata/disk-source-pool-mode.xml b/tests/qemuxml2xmloutdata/disk-source-pool-mode.xml index 29706200db..4d039102fd 100644 --- a/tests/qemuxml2xmloutdata/disk-source-pool-mode.xml +++ b/tests/qemuxml2xmloutdata/disk-source-pool-mode.xml @@ -15,7 +15,7 @@ <devices> <emulator>/usr/bin/qemu-system-i686</emulator> <disk type='volume' device='cdrom'> - <driver name='qemu'/> + <driver name='qemu' type='raw'/> <source pool='pool-iscsi-auth' volume='unit:0:0:1' mode='host'> <seclabel model='selinux' relabel='yes'> <label>system_u:system_r:public_content_t:s0</label> @@ -26,7 +26,7 @@ <address type='drive' controller='0' bus='0' target='0' unit='1'/> </disk> <disk type='volume' device='cdrom'> - <driver name='qemu'/> + <driver name='qemu' type='raw'/> <source pool='pool-iscsi' volume='unit:0:0:2' mode='direct'> <seclabel model='selinux' relabel='yes'> <label>system_u:system_r:public_content_t:s0</label> @@ -43,7 +43,7 @@ <address type='drive' controller='0' bus='0' target='0' unit='3'/> </disk> <disk type='volume' device='cdrom'> - <driver name='qemu'/> + <driver name='qemu' type='raw'/> <auth username='myname'> <secret type='iscsi' usage='mycluster_myname'/> </auth> diff --git a/tests/qemuxml2xmloutdata/disk-source-pool.xml b/tests/qemuxml2xmloutdata/disk-source-pool.xml index 567b22db84..51851b220a 100644 --- a/tests/qemuxml2xmloutdata/disk-source-pool.xml +++ b/tests/qemuxml2xmloutdata/disk-source-pool.xml @@ -15,7 +15,7 @@ <devices> <emulator>/usr/bin/qemu-system-i686</emulator> <disk type='volume' device='cdrom'> - <driver name='qemu'/> + <driver name='qemu' type='raw'/> <source pool='pool-disk' volume='block+cdrom'> <seclabel model='selinux' relabel='yes'> <label>system_u:system_r:public_content_t:s0</label> -- 2.17.1
On 10/5/18 8:14 AM, Peter Krempa wrote:
Post parse callback adds the 'raw' type only for local files. Remote files can also have backing store (even local) so we should do this also for network backed storage.
NIT: The .xml/.args in this patch are not all remote storage - they are volume storage... The disk-source-pool-mode.args gets the look/feel of remote from virDomainDiskTranslateISCSIDirect John
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 4 +--- tests/qemuxml2argvdata/disk-source-pool-mode.args | 6 +++--- tests/qemuxml2argvdata/disk-source-pool.args | 4 ++-- tests/qemuxml2xmloutdata/disk-source-pool-mode.xml | 6 +++--- tests/qemuxml2xmloutdata/disk-source-pool.xml | 2 +- 5 files changed, 10 insertions(+), 12 deletions(-)
[...]
On Mon, Oct 08, 2018 at 07:10:28 -0400, John Ferlan wrote:
On 10/5/18 8:14 AM, Peter Krempa wrote:
Post parse callback adds the 'raw' type only for local files. Remote files can also have backing store (even local) so we should do this also for network backed storage.
NIT: The .xml/.args in this patch are not all remote storage - they are volume storage...
The disk-source-pool-mode.args gets the look/feel of remote from virDomainDiskTranslateISCSIDirect
Oh! actually you are right. For disk type "VOLUME" we should not add any default format in the parser since the function that fetches the volume from the storage driver should do that. I'll investigate slightly further and change the condition to only exclude "VOLUME" disks rather than try to include a variety of the types which require this.
qemu media changing code tried to assume old media's format for the new one if that was not specified. Since the format will always be present it does not make sense to keep the code around. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ad7023c085..8123c12a26 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -250,14 +250,9 @@ qemuDomainChangeMediaLegacy(virQEMUDriverPtr driver, if (qemuGetDriveSourceString(newsrc, NULL, &sourcestr) < 0) goto cleanup; - if (virStorageSourceGetActualType(newsrc) != VIR_STORAGE_TYPE_DIR) { - if (newsrc->format > 0) { - format = virStorageFileFormatTypeToString(newsrc->format); - } else { - if (disk->src->format > 0) - format = virStorageFileFormatTypeToString(disk->src->format); - } - } + if (virStorageSourceGetActualType(newsrc) != VIR_STORAGE_TYPE_DIR) + format = virStorageFileFormatTypeToString(newsrc->format); + qemuDomainObjEnterMonitor(driver, vm); rc = qemuMonitorChangeMedia(priv->mon, driveAlias, -- 2.17.1
Since the code is also used when changing media we need to allow specifying explicit source for which we are going to prepare. With this change callers don't have to replace disk->src with the new source definition for generating these. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8123c12a26..f9a9778fb2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -458,6 +458,7 @@ qemuHotplugRemoveStorageSourcePrepareData(virStorageSourcePtr src, static qemuHotplugDiskSourceDataPtr qemuHotplugDiskSourceRemovePrepare(virDomainDiskDefPtr disk, + virStorageSourcePtr src, virQEMUCapsPtr qemuCaps) { qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); @@ -474,7 +475,7 @@ qemuHotplugDiskSourceRemovePrepare(virDomainDiskDefPtr disk, if (VIR_STRDUP(data->corAlias, diskPriv->nodeCopyOnRead) < 0) goto cleanup; - for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { + for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { if (!(backend = qemuHotplugRemoveStorageSourcePrepareData(n, NULL))) goto cleanup; @@ -485,7 +486,7 @@ qemuHotplugDiskSourceRemovePrepare(virDomainDiskDefPtr disk, if (!(drivealias = qemuAliasDiskDriveFromDisk(disk))) goto cleanup; - if (!(backend = qemuHotplugRemoveStorageSourcePrepareData(disk->src, + if (!(backend = qemuHotplugRemoveStorageSourcePrepareData(src, drivealias))) goto cleanup; @@ -505,6 +506,7 @@ qemuHotplugDiskSourceRemovePrepare(virDomainDiskDefPtr disk, /** * qemuHotplugDiskSourceAttachPrepare: * @disk: disk to generate attachment data for + * @src: disk source to prepare attachment * @qemuCaps: capabilities of the qemu process * * Prepares and returns qemuHotplugDiskSourceData structure filled with all data @@ -512,11 +514,13 @@ qemuHotplugDiskSourceRemovePrepare(virDomainDiskDefPtr disk, */ static qemuHotplugDiskSourceDataPtr qemuHotplugDiskSourceAttachPrepare(virDomainDiskDefPtr disk, + virStorageSourcePtr src, virQEMUCapsPtr qemuCaps) { qemuBlockStorageSourceAttachDataPtr backend = NULL; qemuHotplugDiskSourceDataPtr data; qemuHotplugDiskSourceDataPtr ret = NULL; + virStorageSourcePtr savesrc = NULL; virStorageSourcePtr n; if (VIR_ALLOC(data) < 0) @@ -527,7 +531,7 @@ qemuHotplugDiskSourceAttachPrepare(virDomainDiskDefPtr disk, !(data->corProps = qemuBlockStorageGetCopyOnReadProps(disk))) goto cleanup; - for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { + for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { if (!(backend = qemuBlockStorageSourceAttachPrepareBlockdev(n))) goto cleanup; @@ -538,10 +542,15 @@ qemuHotplugDiskSourceAttachPrepare(virDomainDiskDefPtr disk, goto cleanup; } } else { + VIR_STEAL_PTR(savesrc, disk->src); + disk->src = src; + if (!(backend = qemuBuildStorageSourceAttachPrepareDrive(disk, qemuCaps))) goto cleanup; - if (qemuBuildStorageSourceAttachPrepareCommon(disk->src, backend, qemuCaps) < 0) + VIR_STEAL_PTR(disk->src, savesrc); + + if (qemuBuildStorageSourceAttachPrepareCommon(src, backend, qemuCaps) < 0) goto cleanup; if (VIR_APPEND_ELEMENT(data->backends, data->nbackends, backend) < 0) @@ -551,6 +560,9 @@ qemuHotplugDiskSourceAttachPrepare(virDomainDiskDefPtr disk, VIR_STEAL_PTR(ret, data); cleanup: + if (savesrc) + VIR_STEAL_PTR(disk->src, savesrc); + qemuBlockStorageSourceAttachDataFree(backend); qemuHotplugDiskSourceDataFree(data); return ret; @@ -640,12 +652,13 @@ qemuDomainChangeMediaBlockdev(virQEMUDriverPtr driver, int ret = -1; if (!virStorageSourceIsEmpty(disk->src) && - !(oldbackend = qemuHotplugDiskSourceRemovePrepare(disk, priv->qemuCaps))) + !(oldbackend = qemuHotplugDiskSourceRemovePrepare(disk, disk->src, + priv->qemuCaps))) goto cleanup; disk->src = newsrc; if (!virStorageSourceIsEmpty(disk->src)) { - if (!(newbackend = qemuHotplugDiskSourceAttachPrepare(disk, + if (!(newbackend = qemuHotplugDiskSourceAttachPrepare(disk, disk->src, priv->qemuCaps))) goto cleanup; @@ -782,7 +795,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) goto error; - if (!(diskdata = qemuHotplugDiskSourceAttachPrepare(disk, priv->qemuCaps))) + if (!(diskdata = qemuHotplugDiskSourceAttachPrepare(disk, disk->src, + priv->qemuCaps))) goto error; if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, 0, priv->qemuCaps))) @@ -4159,7 +4173,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, VIR_DEBUG("Removing disk %s from domain %p %s", disk->info.alias, vm, vm->def->name); - if (!(diskbackend = qemuHotplugDiskSourceRemovePrepare(disk, priv->qemuCaps))) + if (!(diskbackend = qemuHotplugDiskSourceRemovePrepare(disk, disk->src, + priv->qemuCaps))) return -1; for (i = 0; i < vm->def->ndisks; i++) { -- 2.17.1
Some functions require us to replace disk->src with the new source for them to work properly. To avoid confusion all places which allow explicit virStorageSource should get the appropriate definition. The legacy code fortunately does not need anything from the old source so that does not require modifications. Blockdev does require the old definition so we'll pass it explicitly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f9a9778fb2..7ee8201ce0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -625,6 +625,7 @@ qemuHotplugDiskSourceRemove(qemuMonitorPtr mon, * @driver: qemu driver structure * @vm: domain definition * @disk: disk definition to change the source of + * @oldsrc: old source definition * @newsrc: new disk source to change to * @force: force the change of media * @@ -639,6 +640,7 @@ static int qemuDomainChangeMediaBlockdev(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, + virStorageSourcePtr oldsrc, virStorageSourcePtr newsrc, bool force) { @@ -646,19 +648,17 @@ qemuDomainChangeMediaBlockdev(virQEMUDriverPtr driver, qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuHotplugDiskSourceDataPtr newbackend = NULL; qemuHotplugDiskSourceDataPtr oldbackend = NULL; - virStorageSourcePtr oldsrc = disk->src; char *nodename = NULL; int rc; int ret = -1; - if (!virStorageSourceIsEmpty(disk->src) && - !(oldbackend = qemuHotplugDiskSourceRemovePrepare(disk, disk->src, + if (!virStorageSourceIsEmpty(oldsrc) && + !(oldbackend = qemuHotplugDiskSourceRemovePrepare(disk, oldsrc, priv->qemuCaps))) goto cleanup; - disk->src = newsrc; - if (!virStorageSourceIsEmpty(disk->src)) { - if (!(newbackend = qemuHotplugDiskSourceAttachPrepare(disk, disk->src, + if (!virStorageSourceIsEmpty(newsrc)) { + if (!(newbackend = qemuHotplugDiskSourceAttachPrepare(disk, newsrc, priv->qemuCaps))) goto cleanup; @@ -704,8 +704,6 @@ qemuDomainChangeMediaBlockdev(virQEMUDriverPtr driver, qemuHotplugDiskSourceDataFree(newbackend); qemuHotplugDiskSourceDataFree(oldbackend); VIR_FREE(nodename); - /* caller handles correct exchange of sources */ - disk->src = oldsrc; return ret; } @@ -743,7 +741,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, goto cleanup; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) - rc = qemuDomainChangeMediaBlockdev(driver, vm, disk, newsrc, force); + rc = qemuDomainChangeMediaBlockdev(driver, vm, disk, disk->src, newsrc, force); else rc = qemuDomainChangeMediaLegacy(driver, vm, disk, newsrc, force); -- 2.17.1
The disk storage source needs to be prepared if we want to use -blockdev or secrets for the new media image. It does not hurt to do the same for the legacy hotplug code as well. Unfortunately helpers like qemuDomainPrepareDiskSource take virDomainDiskDef as an argument and it would be hard to fix them to take an explicit source, so the function also temporarily replaces disk->src for the new source in this function. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7ee8201ce0..d201266805 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -730,10 +730,17 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, virStorageSourcePtr newsrc, bool force) { + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; + virStorageSourcePtr oldsrc = disk->src; int ret = -1; int rc; + disk->src = newsrc; + + if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) + goto cleanup; + if (qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, false) < 0) goto cleanup; @@ -741,11 +748,11 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, goto cleanup; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) - rc = qemuDomainChangeMediaBlockdev(driver, vm, disk, disk->src, newsrc, force); + rc = qemuDomainChangeMediaBlockdev(driver, vm, disk, oldsrc, newsrc, force); else rc = qemuDomainChangeMediaLegacy(driver, vm, disk, newsrc, force); - virDomainAuditDisk(vm, disk->src, newsrc, "update", rc >= 0); + virDomainAuditDisk(vm, oldsrc, newsrc, "update", rc >= 0); if (rc < 0) { ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, true)); @@ -753,17 +760,24 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, } /* remove the old source from shared device list */ + disk->src = oldsrc; ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); - ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true)); + ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, oldsrc, true)); - virStorageSourceFree(disk->src); - VIR_STEAL_PTR(disk->src, newsrc); + /* media was changed, so we can remove the old media definition now */ + virStorageSourceFree(oldsrc); + oldsrc = NULL; + disk->src = newsrc; ignore_value(qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE)); ret = 0; cleanup: + if (oldsrc) + disk->src = oldsrc; + + virObjectUnref(cfg); return ret; } -- 2.17.1
The disk hotplug code also overloads media change which is not ideal. This will allow splitting out of the media change code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index d201266805..ed7076ea01 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1065,10 +1065,10 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, } -int -qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev) +static int +qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) { size_t i; virDomainDiskDefPtr disk = dev->data.disk; @@ -1161,6 +1161,25 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, } +/** + * qemuDomainAttachDeviceDiskLive: + * @driver: qemu driver struct + * @vm: domain object + * @dev: device to attach (expected type is DISK) + * + * Attach a new disk or in case of cdroms/floppies change the media in the drive. + * This function handles all the necessary steps to attach a new storage source + * to the VM. + */ +int +qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + return qemuDomainAttachDeviceDiskLiveInternal(driver, vm, dev); +} + + static void qemuDomainNetDeviceVportRemove(virDomainNetDefPtr net) { -- 2.17.1
In cases where we know the device is a disk we can avoid using the full device definition. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_conf.c | 2 +- src/qemu/qemu_conf.h | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index fc84186a7e..17b7e11e02 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1400,7 +1400,7 @@ qemuSharedDeviceEntryInsert(virQEMUDriverPtr driver, * records all the domains that use the shared device if the entry * already exists, otherwise add a new entry. */ -static int +int qemuAddSharedDisk(virQEMUDriverPtr driver, virDomainDiskDefPtr disk, const char *name) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index c227ac72cc..f876f9117c 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -339,6 +339,11 @@ char *qemuGetSharedDeviceKey(const char *disk_path) void qemuSharedDeviceEntryFree(void *payload, const void *name); +int qemuAddSharedDisk(virQEMUDriverPtr driver, + virDomainDiskDefPtr disk, + const char *name) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + int qemuAddSharedDevice(virQEMUDriverPtr driver, virDomainDeviceDefPtr dev, const char *name) -- 2.17.1
Disk hotplug has slightly different semantics from media changing. Move the media change code out and add proper initialization of the new source object and proper cleanups if something fails. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 15 +------- src/qemu/qemu_hotplug.c | 77 ++++++++++++++++++++++++++--------------- 2 files changed, 51 insertions(+), 41 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 91ac3640d3..a52e2495d5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7851,12 +7851,6 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm, virDomainDeviceDef oldDev = { .type = dev->type }; int ret = -1; - if (virDomainDiskTranslateSourcePool(disk) < 0) - goto cleanup; - - if (qemuDomainDetermineDiskChain(driver, vm, disk, true) < 0) - goto cleanup; - if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def, disk->bus, disk->dst))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -7885,16 +7879,9 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm, goto cleanup; } - /* Add the new disk src into shared disk hash table */ - if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0) - goto cleanup; - if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk, - dev->data.disk->src, force) < 0) { - ignore_value(qemuRemoveSharedDisk(driver, dev->data.disk, - vm->def->name)); + dev->data.disk->src, force) < 0) goto cleanup; - } dev->data.disk->src = NULL; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ed7076ea01..62470b1a2f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -733,11 +733,23 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; virStorageSourcePtr oldsrc = disk->src; + bool sharedAdded = false; int ret = -1; int rc; disk->src = newsrc; + if (virDomainDiskTranslateSourcePool(disk) < 0) + goto cleanup; + + if (qemuAddSharedDisk(driver, disk, vm->def->name) < 0) + goto cleanup; + + sharedAdded = true; + + if (qemuDomainDetermineDiskChain(driver, vm, disk, true) < 0) + goto cleanup; + if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) goto cleanup; @@ -754,10 +766,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, virDomainAuditDisk(vm, oldsrc, newsrc, "update", rc >= 0); - if (rc < 0) { - ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, true)); + if (rc < 0) goto cleanup; - } /* remove the old source from shared device list */ disk->src = oldsrc; @@ -769,11 +779,21 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, oldsrc = NULL; disk->src = newsrc; - ignore_value(qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE)); - ret = 0; cleanup: + /* undo changes to the new disk */ + if (ret < 0) { + if (sharedAdded) + ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); + + ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, true)); + } + + /* remove PR manager object if unneeded */ + ignore_value(qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE)); + + /* revert old image do the disk definition */ if (oldsrc) disk->src = oldsrc; @@ -1072,9 +1092,15 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriverPtr driver, { size_t i; virDomainDiskDefPtr disk = dev->data.disk; - virDomainDiskDefPtr orig_disk = NULL; int ret = -1; + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM || + disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cdrom/floppy device hotplug isn't supported")); + return -1; + } + if (virDomainDiskTranslateSourcePool(disk) < 0) goto cleanup; @@ -1088,27 +1114,6 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriverPtr driver, goto cleanup; switch ((virDomainDiskDevice) disk->device) { - case VIR_DOMAIN_DISK_DEVICE_CDROM: - case VIR_DOMAIN_DISK_DEVICE_FLOPPY: - if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def, - disk->bus, disk->dst))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("No device with bus '%s' and target '%s'. " - "cdrom and floppy device hotplug isn't supported " - "by libvirt"), - virDomainDiskBusTypeToString(disk->bus), - disk->dst); - goto cleanup; - } - - if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk, - disk->src, false) < 0) - goto cleanup; - - disk->src = NULL; - ret = 0; - break; - case VIR_DOMAIN_DISK_DEVICE_DISK: case VIR_DOMAIN_DISK_DEVICE_LUN: for (i = 0; i < vm->def->ndisks; i++) { @@ -1150,6 +1155,8 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriverPtr driver, } break; + case VIR_DOMAIN_DISK_DEVICE_CDROM: + case VIR_DOMAIN_DISK_DEVICE_FLOPPY: case VIR_DOMAIN_DISK_DEVICE_LAST: break; } @@ -1176,6 +1183,22 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { + virDomainDiskDefPtr disk = dev->data.disk; + virDomainDiskDefPtr orig_disk = NULL; + + /* this API overloads media change semantics on disk hotplug + * for devices supporting media changes */ + if ((disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM || + disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) && + (orig_disk = virDomainDiskFindByBusAndDst(vm->def, disk->bus, disk->dst))) { + if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk, + disk->src, false) < 0) + return -1; + + disk->src = NULL; + return 0; + } + return qemuDomainAttachDeviceDiskLiveInternal(driver, vm, dev); } -- 2.17.1
We now explicitly handle media change elsewhere so we can drop the switch statement. This will also make it more intuitive once CDROM device hotplug might be supported. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 70 ++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 62470b1a2f..0a63741b9e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1113,52 +1113,42 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriverPtr driver, if (qemuDomainDetermineDiskChain(driver, vm, disk, true) < 0) goto cleanup; - switch ((virDomainDiskDevice) disk->device) { - case VIR_DOMAIN_DISK_DEVICE_DISK: - case VIR_DOMAIN_DISK_DEVICE_LUN: - for (i = 0; i < vm->def->ndisks; i++) { - if (virDomainDiskDefCheckDuplicateInfo(vm->def->disks[i], disk) < 0) - goto cleanup; - } - - switch ((virDomainDiskBus) disk->bus) { - case VIR_DOMAIN_DISK_BUS_USB: - if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk device='lun' is not supported for usb bus")); - break; - } - ret = qemuDomainAttachUSBMassStorageDevice(driver, vm, disk); - break; - - case VIR_DOMAIN_DISK_BUS_VIRTIO: - ret = qemuDomainAttachVirtioDiskDevice(driver, vm, disk); - break; + for (i = 0; i < vm->def->ndisks; i++) { + if (virDomainDiskDefCheckDuplicateInfo(vm->def->disks[i], disk) < 0) + goto cleanup; + } - case VIR_DOMAIN_DISK_BUS_SCSI: - ret = qemuDomainAttachSCSIDisk(driver, vm, disk); + switch ((virDomainDiskBus) disk->bus) { + case VIR_DOMAIN_DISK_BUS_USB: + if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk device='lun' is not supported for usb bus")); break; - - case VIR_DOMAIN_DISK_BUS_IDE: - case VIR_DOMAIN_DISK_BUS_FDC: - case VIR_DOMAIN_DISK_BUS_XEN: - case VIR_DOMAIN_DISK_BUS_UML: - case VIR_DOMAIN_DISK_BUS_SATA: - case VIR_DOMAIN_DISK_BUS_SD: - /* Note that SD card hotplug support should be added only once - * they support '-device' (don't require -drive only). - * See also: qemuDiskBusNeedsDriveArg */ - case VIR_DOMAIN_DISK_BUS_LAST: - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("disk bus '%s' cannot be hotplugged."), - virDomainDiskBusTypeToString(disk->bus)); } + ret = qemuDomainAttachUSBMassStorageDevice(driver, vm, disk); + break; + + case VIR_DOMAIN_DISK_BUS_VIRTIO: + ret = qemuDomainAttachVirtioDiskDevice(driver, vm, disk); break; - case VIR_DOMAIN_DISK_DEVICE_CDROM: - case VIR_DOMAIN_DISK_DEVICE_FLOPPY: - case VIR_DOMAIN_DISK_DEVICE_LAST: + case VIR_DOMAIN_DISK_BUS_SCSI: + ret = qemuDomainAttachSCSIDisk(driver, vm, disk); break; + + case VIR_DOMAIN_DISK_BUS_IDE: + case VIR_DOMAIN_DISK_BUS_FDC: + case VIR_DOMAIN_DISK_BUS_XEN: + case VIR_DOMAIN_DISK_BUS_UML: + case VIR_DOMAIN_DISK_BUS_SATA: + case VIR_DOMAIN_DISK_BUS_SD: + /* Note that SD card hotplug support should be added only once + * they support '-device' (don't require -drive only). + * See also: qemuDiskBusNeedsDriveArg */ + case VIR_DOMAIN_DISK_BUS_LAST: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("disk bus '%s' cannot be hotplugged."), + virDomainDiskBusTypeToString(disk->bus)); } cleanup: -- 2.17.1
On 10/05/2018 02:13 PM, Peter Krempa wrote:
Fixes regression in media changing/disk hotplug as the ordering of the alias allocation and disk preparation was bad.
v2: - be more explicit about old and new definitions used in certain steps - clean up legacy hotplug to not access old disk source definition - also treat network disks as 'raw' if the format was not provided
Peter Krempa (12): Revert "qemu: hotplug: Prepare disk source in qemuDomainAttachDeviceDiskLive" Revert "qemu: hotplug: consolidate media change code paths" qemu: hotplug: Don't pretend that we support secrets for media change qemu: domain: Assume 'raw' default storage format also for network storage qemu: hotplug: Remove code handling possible missing disk source format qemu: hotplug: Allow specifying explicit source for disk backend hotplug code qemu: hotplug: Be explicit about old/new sources when changing media qemu: hotplug: Prepare disk source for media changing qemu: hotplug: Add wrapper for disk hotplug code qemu: conf: Export qemuAddSharedDisk qemu: hotplug: Split out media change code from disk hotplug qemu: hotplug: Refactor qemuDomainAttachDeviceDiskLiveInternal
src/qemu/qemu_conf.c | 2 +- src/qemu/qemu_conf.h | 5 + src/qemu/qemu_domain.c | 4 +- src/qemu/qemu_driver.c | 7 +- src/qemu/qemu_hotplug.c | 276 ++++++++++-------- src/qemu/qemu_hotplug.h | 9 +- tests/qemuhotplugtest.c | 2 +- .../disk-source-pool-mode.args | 6 +- tests/qemuxml2argvdata/disk-source-pool.args | 4 +- .../disk-source-pool-mode.xml | 6 +- tests/qemuxml2xmloutdata/disk-source-pool.xml | 2 +- 11 files changed, 185 insertions(+), 138 deletions(-)
ACK series. I can also confirm that this passes my testing which caused this in the first place. Michal
participants (3)
-
John Ferlan -
Michal Privoznik -
Peter Krempa