[libvirt] [PATCH 0/8] qemu: Fix disk hotplug/media change regression
Few of my recent patches (see the first two reverts) introduced a regression when adding disks. The disk alias is needed when creating names of certain backend objects (secrets/TLS). The code preparing those was moved prior to alias formatting. Revert those patches and fix it in a different way. Peter Krempa (8): Revert "qemu: hotplug: Prepare disk source in qemuDomainAttachDeviceDiskLive" Revert "qemu: hotplug: consolidate media change code paths" qemu: hotplug: Use new source when preparing/translating for media change 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_driver.c | 7 +- src/qemu/qemu_hotplug.c | 188 ++++++++++++++++++++++------------------ src/qemu/qemu_hotplug.h | 9 +- tests/qemuhotplugtest.c | 2 +- 6 files changed, 123 insertions(+), 90 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
On 9/27/18 11:09 AM, Peter Krempa wrote:
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(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John Should the two reverts go into 4.7 and 4.8 maint streams too?
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 b238309852..64f0ad33e8 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
On 9/27/18 11:09 AM, Peter Krempa wrote:
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(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Some internal helpers use only a virDomainDiskDef to do their job. If we want to change source for the disk we need to update it to the new source in the disk definition for those to work correctly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 86afda636e..3043b3257b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -730,9 +730,12 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, bool force) { qemuDomainObjPrivatePtr priv = vm->privateData; + virStorageSourcePtr oldsrc = disk->src; int ret = -1; int rc; + disk->src = newsrc; + if (qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, false) < 0) goto cleanup; @@ -744,7 +747,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, 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)); @@ -755,7 +758,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true)); - virStorageSourceFree(disk->src); + virStorageSourceFree(oldsrc); + oldsrc = NULL; VIR_STEAL_PTR(disk->src, newsrc); ignore_value(qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE)); @@ -763,6 +767,9 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, ret = 0; cleanup: + if (oldsrc) + disk->src = oldsrc; + return ret; } -- 2.17.1
On 9/27/18 11:09 AM, Peter Krempa wrote:
Some internal helpers use only a virDomainDiskDef to do their job. If we want to change source for the disk we need to update it to the new source in the disk definition for those to work correctly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 86afda636e..3043b3257b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -730,9 +730,12 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, bool force) { qemuDomainObjPrivatePtr priv = vm->privateData; + virStorageSourcePtr oldsrc = disk->src; int ret = -1; int rc;
+ disk->src = newsrc; +
Doesn't seem like this would be right especially considering what each of the following calls passing both @disk and @newsrc do.
if (qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, false) < 0) goto cleanup;
Passing both @disk and @newsrc here after assigning newsrc into disk->src would seem to do nothing since it temporarily swaps them for processing, then returns with them swapped back. Shortly after this passing both @disk and @newsrc into either qemuDomainChangeMediaBlockdev or qemuDomainChangeMediaLegacy would seem to be incorrect especially since each deals with what's in the drive first which would now be incorrect, wouldn't it?
@@ -744,7 +747,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, 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));
This portion would seemingly be problematic too. Hard to review the rest... John
@@ -755,7 +758,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true));
- virStorageSourceFree(disk->src); + virStorageSourceFree(oldsrc); + oldsrc = NULL; VIR_STEAL_PTR(disk->src, newsrc);
ignore_value(qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE)); @@ -763,6 +767,9 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, ret = 0;
cleanup: + if (oldsrc) + disk->src = oldsrc; + return ret; }
On Tue, Oct 02, 2018 at 08:15:07 -0400, John Ferlan wrote:
On 9/27/18 11:09 AM, Peter Krempa wrote:
Some internal helpers use only a virDomainDiskDef to do their job. If we want to change source for the disk we need to update it to the new source in the disk definition for those to work correctly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 86afda636e..3043b3257b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -730,9 +730,12 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, bool force) { qemuDomainObjPrivatePtr priv = vm->privateData; + virStorageSourcePtr oldsrc = disk->src; int ret = -1; int rc;
+ disk->src = newsrc; +
Doesn't seem like this would be right especially considering what each of the following calls passing both @disk and @newsrc do.
if (qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, false) < 0) goto cleanup;
Passing both @disk and @newsrc here after assigning newsrc into disk->src would seem to do nothing since it temporarily swaps them for processing, then returns with them swapped back.
Umm, well it's kind of pointless to pass in newsrc, but it's the exactly expected behaviour. We want to prepare 'newsrc' but half of the functions take disk->src directly.
Shortly after this passing both @disk and @newsrc into either qemuDomainChangeMediaBlockdev or qemuDomainChangeMediaLegacy would seem to be incorrect especially since each deals with what's in the drive first which would now be incorrect, wouldn't it?
Hmm, yeah, the blockdev version actually does care what the old source was, so that would not work. The non-blockdev version does not care so it does'nt matter much. I'll have to change qemuDomainChangeMediaBlockdev to take 'oldsrc' instead of 'newsrc'.
@@ -744,7 +747,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, 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));
This portion would seemingly be problematic too.
As explained above, the only thing that happens is that disk->src is replaced and reverted again, thus 3 extra assignments.
On 10/2/18 8:25 AM, Peter Krempa wrote:
On Tue, Oct 02, 2018 at 08:15:07 -0400, John Ferlan wrote:
On 9/27/18 11:09 AM, Peter Krempa wrote:
Some internal helpers use only a virDomainDiskDef to do their job. If we want to change source for the disk we need to update it to the new source in the disk definition for those to work correctly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 86afda636e..3043b3257b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -730,9 +730,12 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, bool force) { qemuDomainObjPrivatePtr priv = vm->privateData; + virStorageSourcePtr oldsrc = disk->src; int ret = -1; int rc;
+ disk->src = newsrc; +
Doesn't seem like this would be right especially considering what each of the following calls passing both @disk and @newsrc do.
if (qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, false) < 0) goto cleanup;
Passing both @disk and @newsrc here after assigning newsrc into disk->src would seem to do nothing since it temporarily swaps them for processing, then returns with them swapped back.
Umm, well it's kind of pointless to pass in newsrc, but it's the exactly expected behaviour. We want to prepare 'newsrc' but half of the functions take disk->src directly.
Yeah, true - if both this and the one below pass NULL, then nothing in Prepare needs @newsrc... Then it just becomes a process of passing the right one at the right time.
Shortly after this passing both @disk and @newsrc into either qemuDomainChangeMediaBlockdev or qemuDomainChangeMediaLegacy would seem to be incorrect especially since each deals with what's in the drive first which would now be incorrect, wouldn't it?
Hmm, yeah, the blockdev version actually does care what the old source was, so that would not work.
The non-blockdev version does not care so it does'nt matter much.
Seems to me qemuDomainChangeMediaLegacy manipulates @disk (oldsrc) and needs diskPriv just like qemuDomainChangeMediaBlockdev. John
I'll have to change qemuDomainChangeMediaBlockdev to take 'oldsrc' instead of 'newsrc'.
@@ -744,7 +747,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, 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));
This portion would seemingly be problematic too.
As explained above, the only thing that happens is that disk->src is replaced and reverted again, thus 3 extra assignments.
The disk storage source needs to be prepared if we want to use -blockdev or secrets for the new media image. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 3043b3257b..1ec696e2aa 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -729,6 +729,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, virStorageSourcePtr newsrc, bool force) { + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; virStorageSourcePtr oldsrc = disk->src; int ret = -1; @@ -736,6 +737,9 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, disk->src = newsrc; + if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) + goto cleanup; + if (qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, false) < 0) goto cleanup; @@ -770,6 +774,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (oldsrc) disk->src = oldsrc; + virObjectUnref(cfg); return ret; } -- 2.17.1
On 9/27/18 11:09 AM, Peter Krempa wrote:
The disk storage source needs to be prepared if we want to use -blockdev or secrets for the new media image.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 3043b3257b..1ec696e2aa 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -729,6 +729,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, virStorageSourcePtr newsrc, bool force) { + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; virStorageSourcePtr oldsrc = disk->src; int ret = -1; @@ -736,6 +737,9 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
disk->src = newsrc;
+ if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) + goto cleanup; +
"Assuming" this becomes passing "@newdisk" along to qemuDomainPrepareDiskSource, then this would seem to be fine. Compared to the order in AttachDiskGeneric, should this go under PrepareDiskAccess? And since qemuAssignDeviceDiskAlias doesn't use the disk->src, then it doesn't need to be called - whether you want to note that or not, IDC. I didn't go chasing into qemuHotplugAttachManagedPR to determine whether perhaps that failure should have "undone" qemuHotplugPrepareDiskAccess, but looking at qemuDomainAttachDiskGeneric, I think the answer may be yes. IOW: Perhaps an error label would be needed. John
if (qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, false) < 0) goto cleanup;
@@ -770,6 +774,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (oldsrc) disk->src = oldsrc;
+ virObjectUnref(cfg); return ret; }
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 1ec696e2aa..6227a130da 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1061,10 +1061,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; @@ -1157,6 +1157,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
On 9/27/18 11:09 AM, Peter Krempa wrote:
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(-)
Making future review/life easier ;-) Reviewed-by: John Ferlan <jferlan@redhat.com> John
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 33508174cb..6cdc0f407a 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1411,7 +1411,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
On 9/27/18 11:09 AM, Peter Krempa wrote:
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(-)
<sigh> similar comment I make frequently now - stuff I know you know... The ATTRIBUTE_NONNULL only matters if NULL is passed. There's no check for whether the passed parameter is NULL. In the long run, e.g. patch 7, it's one less indirection from qemuAddSharedDevice to qemuAddSharedDisk for *ChangeEjectableMedia. Of course there's an incredible irony that code uses *RemoveSharedDisk, but yet no one up to this point saw fit to export *AddSharedDisk too. Reviewed-by: John Ferlan <jferlan@redhat.com> John
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 33508174cb..6cdc0f407a 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1411,7 +1411,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)
Disk hotplug has slightly different semantics from media changing. Move the media change code out and add proper initialization of the new source object. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 15 +---------- src/qemu/qemu_hotplug.c | 56 +++++++++++++++++++++++++---------------- 2 files changed, 35 insertions(+), 36 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 64f0ad33e8..f609151152 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 6227a130da..ef3f0cd8b3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -737,6 +737,15 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, disk->src = newsrc; + if (virDomainDiskTranslateSourcePool(disk) < 0) + goto cleanup; + + if (qemuAddSharedDisk(driver, disk, vm->def->name) < 0) + goto cleanup; + + if (qemuDomainDetermineDiskChain(driver, vm, disk, true) < 0) + goto cleanup; + if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) goto cleanup; @@ -1068,9 +1077,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; @@ -1084,27 +1099,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++) { @@ -1146,6 +1140,8 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriverPtr driver, } break; + case VIR_DOMAIN_DISK_DEVICE_CDROM: + case VIR_DOMAIN_DISK_DEVICE_FLOPPY: case VIR_DOMAIN_DISK_DEVICE_LAST: break; } @@ -1172,6 +1168,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
On 9/27/18 11:09 AM, Peter Krempa wrote:
Disk hotplug has slightly different semantics from media changing. Move the media change code out and add proper initialization of the new source object.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 15 +---------- src/qemu/qemu_hotplug.c | 56 +++++++++++++++++++++++++---------------- 2 files changed, 35 insertions(+), 36 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 64f0ad33e8..f609151152 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; -
Would removing this affect the subsequent virStorageSourceIsSameLocation call? Since def->disk->src is one of the "changed" values?
- 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 6227a130da..ef3f0cd8b3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -737,6 +737,15 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
disk->src = newsrc;
+ if (virDomainDiskTranslateSourcePool(disk) < 0) + goto cleanup; + + if (qemuAddSharedDisk(driver, disk, vm->def->name) < 0) + goto cleanup;
Failures after this would need to call qemuRemoveSharedDisk John [...]
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 ef3f0cd8b3..d5e1e2a039 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1098,52 +1098,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 9/27/18 11:09 AM, Peter Krempa wrote:
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(-)
Assuming the preceding patches get their issues resolved, this is fine. John
participants (2)
-
John Ferlan -
Peter Krempa