[libvirt] [PATCH 00/10] qemu: hotplug: Refactor for -blockdev support (blockdev-add saga)
A bunch of refactors which could be separated from my blockdev add branch. The patches modify the disk hotplug/unplug and media change code so that we can easily add support for -blockdev later on. Peter Krempa (10): qemu: hotplug: Reuse qemuHotplugPrepareDiskAccess in qemuDomainRemoveDiskDevice qemu: hotplug: Remove pointless variable qemu: hotplug: Don't format NULL in %s in qemuHotplugPrepareDiskAccess qemu: hotplug: Prepare for multiple backing chain member hotplug qemu: hotplug: Don't leak 'disk' if VM crashes during unplug finishing qemu: hotplug: Reuse qemuHotplugDiskSourceRemove for disk backend removal qemu: hotplug: Simplify removal of managed PR infrastructure on unplug qemu: hotplug: Refactor/simplify PR managed addition to VM qemu: hotplug: Extract legacy disk media changing bits qemu: hotplug: Make qemuHotplugWaitForTrayEject reusable src/qemu/qemu_hotplug.c | 508 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 358 insertions(+), 150 deletions(-) -- 2.16.2
qemuHotplugPrepareDiskAccess can be used to tear down disk access so we can replace the open-coded version collecting the same function calls. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2b6633a998..ee26fb4f52 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3830,17 +3830,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, qemuDomainReleaseDeviceAddress(vm, &disk->info, src); - if (qemuSecurityRestoreDiskLabel(driver, vm, disk) < 0) - VIR_WARN("Unable to restore security label on %s", src); - - if (qemuTeardownDiskCgroup(vm, disk) < 0) - VIR_WARN("Failed to tear down cgroup for disk path %s", src); - - if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) - VIR_WARN("Unable to release lock on %s", src); - - if (qemuDomainNamespaceTeardownDisk(vm, disk->src) < 0) - VIR_WARN("Unable to remove /dev entry for %s", src); + /* tear down disk security access */ + qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true); dev.type = VIR_DOMAIN_DEVICE_DISK; dev.data.disk = disk; -- 2.16.2
On Tue, Jul 17, 2018 at 02:14:21PM +0200, Peter Krempa wrote:
qemuHotplugPrepareDiskAccess can be used to tear down disk access so we can replace the open-coded version collecting the same function calls.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Now that there's only one use of it, replace it directly by the code filling it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ee26fb4f52..20efb01dbf 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3757,7 +3757,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, virDomainDeviceDef dev; virObjectEventPtr event; size_t i; - const char *src = virDomainDiskGetSource(disk); qemuDomainObjPrivatePtr priv = vm->privateData; char *drivestr; bool prManaged = priv->prDaemonRunning; @@ -3828,7 +3827,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, if (prManaged && !prUsed) qemuProcessKillManagedPRDaemon(vm); - qemuDomainReleaseDeviceAddress(vm, &disk->info, src); + qemuDomainReleaseDeviceAddress(vm, &disk->info, virDomainDiskGetSource(disk)); /* tear down disk security access */ qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true); -- 2.16.2
On Tue, Jul 17, 2018 at 02:14:22PM +0200, Peter Krempa wrote:
Now that there's only one use of it, replace it directly by the code filling it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The warning messages which include the disk source could potentially format NULL using %s as virDomainDiskGetSource may return NULL for e.g. NBD disks. As most of the APIs are NOOP for remote disks the usage of the source string only should be fine for now. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 20efb01dbf..624341baa1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -126,21 +126,21 @@ qemuHotplugPrepareDiskAccess(virQEMUDriverPtr driver, rollback_cgroup: if (qemuTeardownDiskCgroup(vm, disk) < 0) VIR_WARN("Unable to tear down cgroup access on %s", - virDomainDiskGetSource(disk)); + NULLSTR(virDomainDiskGetSource(disk))); rollback_label: if (qemuSecurityRestoreDiskLabel(driver, vm, disk) < 0) VIR_WARN("Unable to restore security label on %s", - virDomainDiskGetSource(disk)); + NULLSTR(virDomainDiskGetSource(disk))); rollback_namespace: if (qemuDomainNamespaceTeardownDisk(vm, disk->src) < 0) VIR_WARN("Unable to remove /dev entry for %s", - virDomainDiskGetSource(disk)); + NULLSTR(virDomainDiskGetSource(disk))); rollback_lock: if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) VIR_WARN("Unable to release lock on %s", - virDomainDiskGetSource(disk)); + NULLSTR(virDomainDiskGetSource(disk))); cleanup: if (origsrc) -- 2.16.2
On Tue, Jul 17, 2018 at 02:14:23PM +0200, Peter Krempa wrote:
The warning messages which include the disk source could potentially format NULL using %s as virDomainDiskGetSource may return NULL for e.g. NBD disks. As most of the APIs are NOOP for remote disks the usage of the source string only should be fine for now.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Similarly to how we've intergrated data belonging to a single virStorageSource for purposes of attaching it to a qemu instance we will need to agregate data relevant for the whole disk. With blockdev there will be some disk-wide backing chain members such as the copy-on-read handler. Introduce qemuHotplugDiskSourceData which agregates the backing chain and other data relevant for the disk and functions which generate it and apply and rollback it. In addition to disk hotplug this will also be reused for media changing where we need to exchange the full disk backend. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 118 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 109 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 624341baa1..2ddcc19cc8 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -350,6 +350,110 @@ qemuDomainDiskAttachManagedPR(virDomainObjPtr vm, } +struct _qemuHotplugDiskSourceData { + qemuBlockStorageSourceAttachDataPtr *backends; + size_t nbackends; +}; +typedef struct _qemuHotplugDiskSourceData qemuHotplugDiskSourceData; +typedef qemuHotplugDiskSourceData *qemuHotplugDiskSourceDataPtr; + + +static void +qemuHotplugDiskSourceDataFree(qemuHotplugDiskSourceDataPtr data) +{ + size_t i; + + if (!data) + return; + + for (i = 0; i < data->nbackends; i++) + qemuBlockStorageSourceAttachDataFree(data->backends[i]); + + VIR_FREE(data); +} + + +/** + * qemuHotplugDiskSourceAttachPrepare: + * @disk: disk to generate attachment data for + * @qemuCaps: capabilities of the qemu process + * + * Prepares and returns qemuHotplugDiskSourceData structure filled with all data + * which will fully attach the source backend of the disk to a given VM. + */ +static qemuHotplugDiskSourceDataPtr +qemuHotplugDiskSourceAttachPrepare(virDomainDiskDefPtr disk, + virQEMUCapsPtr qemuCaps) +{ + qemuBlockStorageSourceAttachDataPtr backend; + qemuHotplugDiskSourceDataPtr data; + qemuHotplugDiskSourceDataPtr ret = NULL; + + if (VIR_ALLOC(data) < 0) + return NULL; + + if (!(backend = qemuBuildStorageSourceAttachPrepareDrive(disk, qemuCaps, false))) + goto cleanup; + + if (qemuBuildStorageSourceAttachPrepareCommon(disk->src, backend, qemuCaps) < 0) + goto cleanup; + + if (VIR_APPEND_ELEMENT(data->backends, data->nbackends, backend) < 0) + goto cleanup; + + VIR_STEAL_PTR(ret, data); + + cleanup: + qemuBlockStorageSourceAttachDataFree(backend); + qemuHotplugDiskSourceDataFree(data); + return ret; +} + + +/** + * qemuHotplugDiskSourceAttach: + * @mon: monitor object + * @data: disk backend data object describing what to remove + * + * Attach a disk source backend with all relevant pieces. Caller must enter the + * monitor context for @mon. + */ +static int +qemuHotplugDiskSoureceAttach(qemuMonitorPtr mon, + qemuHotplugDiskSourceDataPtr data) +{ + size_t i; + + for (i = data->nbackends; i > 0; i--) { + if (qemuBlockStorageSourceAttachApply(mon, data->backends[i - 1]) < 0) + return -1; + } + + return 0; +} + + +/** + * qemuHotplugDiskSourceRemove: + * @mon: monitor object + * @data: disk backend data object describing what to remove + * + * Remove a disk source backend with all relevant pieces. This function + * preserves the error which was set prior to calling it. Caller must enter the + * monitor context for @mon. + */ +static void +qemuHotplugDiskSourceRemove(qemuMonitorPtr mon, + qemuHotplugDiskSourceDataPtr data) + +{ + size_t i; + + for (i = 0; i < data->nbackends; i++) + qemuBlockStorageSourceAttachRollback(mon, data->backends[i]); +} + + /** * qemuDomainAttachDiskGeneric: * @@ -362,7 +466,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - qemuBlockStorageSourceAttachDataPtr data = NULL; + qemuHotplugDiskSourceDataPtr diskdata = NULL; virErrorPtr orig_err; char *devstr = NULL; char *managedPrmgrAlias = NULL; @@ -381,11 +485,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuDomainDiskAttachManagedPR(vm, disk, &managedPrmgrProps) < 0) goto error; - if (!(data = qemuBuildStorageSourceAttachPrepareDrive(disk, priv->qemuCaps, - false))) - goto error; - - if (qemuBuildStorageSourceAttachPrepareCommon(disk->src, data, priv->qemuCaps) < 0) + if (!(diskdata = qemuHotplugDiskSourceAttachPrepare(disk, priv->qemuCaps))) goto error; if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, 0, priv->qemuCaps))) @@ -400,7 +500,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, qemuMonitorAddObject(priv->mon, &managedPrmgrProps, &managedPrmgrAlias) < 0) goto exit_monitor; - if (qemuBlockStorageSourceAttachApply(priv->mon, data) < 0) + if (qemuHotplugDiskSoureceAttach(priv->mon, diskdata) < 0) goto exit_monitor; if (qemuMonitorAddDevice(priv->mon, devstr) < 0) @@ -417,7 +517,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, ret = 0; cleanup: - qemuBlockStorageSourceAttachDataFree(data); + qemuHotplugDiskSourceDataFree(diskdata); virJSONValueFree(managedPrmgrProps); qemuDomainSecretDiskDestroy(disk); VIR_FREE(managedPrmgrAlias); @@ -426,7 +526,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, return ret; exit_monitor: - qemuBlockStorageSourceAttachRollback(priv->mon, data); + qemuHotplugDiskSourceRemove(priv->mon, diskdata); virErrorPreserveLast(&orig_err); if (managedPrmgrAlias) -- 2.16.2
On Tue, Jul 17, 2018 at 14:14:24 +0200, Peter Krempa wrote:
Similarly to how we've intergrated data belonging to a single virStorageSource for purposes of attaching it to a qemu instance we will need to agregate data relevant for the whole disk. With blockdev there will be some disk-wide backing chain members such as the copy-on-read handler.
Introduce qemuHotplugDiskSourceData which agregates the backing chain and other data relevant for the disk and functions which generate it and apply and rollback it.
In addition to disk hotplug this will also be reused for media changing where we need to exchange the full disk backend.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 118 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 109 insertions(+), 9 deletions(-)
[...]
+ * qemuHotplugDiskSourceAttach: + * @mon: monitor object + * @data: disk backend data object describing what to remove + * + * Attach a disk source backend with all relevant pieces. Caller must enter the + * monitor context for @mon. + */ +static int +qemuHotplugDiskSoureceAttach(qemuMonitorPtr mon, + qemuHotplugDiskSourceDataPtr data) +{
I forgot to squash in a patch that fixes the typo in the name. I've done the following modification to my private branch: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5f3ff0f7ba..4251c79bf0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -532,8 +532,8 @@ qemuHotplugDiskSourceAttachPrepare(virDomainDiskDefPtr disk, * monitor context for @mon. */ static int -qemuHotplugDiskSoureceAttach(qemuMonitorPtr mon, - qemuHotplugDiskSourceDataPtr data) +qemuHotplugDiskSourceAttach(qemuMonitorPtr mon, + qemuHotplugDiskSourceDataPtr data) { size_t i; @@ -658,7 +658,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); - if (qemuHotplugDiskSoureceAttach(priv->mon, diskdata) < 0) + if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0) goto exit_monitor; if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
On Tue, Jul 17, 2018 at 02:14:24PM +0200, Peter Krempa wrote:
Similarly to how we've intergrated data belonging to a single virStorageSource for purposes of attaching it to a qemu instance we will need to agregate data relevant for the whole disk. With blockdev there will be some disk-wide backing chain members such as the copy-on-read handler.
Introduce qemuHotplugDiskSourceData which agregates the backing chain and other data relevant for the disk and functions which generate it and apply and rollback it.
In addition to disk hotplug this will also be reused for media changing where we need to exchange the full disk backend.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 118 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 109 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
qemuDomainRemoveDiskDevice would leak the disk to be removed if the VM crashed since it was removed from the definition but not freed. Broken in commit 105bcdde76b which moved the removal from the definition earlier. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2ddcc19cc8..f1b18fcc7d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3863,6 +3863,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, bool prUsed = false; const char *authAlias = NULL; const char *encAlias = NULL; + int ret = -1; VIR_DEBUG("Removing disk %s from domain %p %s", disk->info.alias, vm, vm->def->name); @@ -3917,7 +3918,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, ignore_value(qemuMonitorDelObject(priv->mon, qemuDomainGetManagedPRAlias())); if (qemuDomainObjExitMonitor(driver, vm) < 0) - return -1; + goto cleanup; virDomainAuditDisk(vm, disk->src, NULL, "detach", true); @@ -3937,8 +3938,11 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, ignore_value(qemuRemoveSharedDevice(driver, &dev, vm->def->name)); virDomainUSBAddressRelease(priv->usbaddrs, &disk->info); + ret = 0; + + cleanup: virDomainDiskDefFree(disk); - return 0; + return ret; } -- 2.16.2
On Tue, Jul 17, 2018 at 02:14:25PM +0200, Peter Krempa wrote:
qemuDomainRemoveDiskDevice would leak the disk to be removed if the VM crashed since it was removed from the definition but not freed.
Broken in commit 105bcdde76b which moved the removal from the definition earlier.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Add code which will convert a disk definition into qemuHotplugDiskSourceData and then reuse qemuHotplugDiskSourceRemove to remove all the backend related objects. This unifies the detach code as much as possible with the already existing helpers and will allow reuse this infrastructure when changing removable disk media. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 129 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 94 insertions(+), 35 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f1b18fcc7d..3ee74c8e40 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -373,6 +373,96 @@ qemuHotplugDiskSourceDataFree(qemuHotplugDiskSourceDataPtr data) } +/** + * qemuDomainRemoveDiskStorageSourcePrepareData: + * @src: disk source structure + * @driveAlias: Alias of the -drive backend, the pointer is always consumed + * + * Prepare qemuBlockStorageSourceAttachDataPtr for detaching a single source + * from a VM. If @driveAlias is NULL -blockdev is assumed. + */ +static qemuBlockStorageSourceAttachDataPtr +qemuHotplugRemoveStorageSourcePrepareData(virStorageSourcePtr src, + char *driveAlias) + +{ + qemuDomainStorageSourcePrivatePtr srcpriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); + qemuBlockStorageSourceAttachDataPtr data; + qemuBlockStorageSourceAttachDataPtr ret = NULL; + + if (VIR_ALLOC(data) < 0) + goto cleanup; + + if (driveAlias) { + VIR_STEAL_PTR(data->driveAlias, driveAlias); + data->driveAdded = true; + } else { + data->formatNodeName = src->nodeformat; + data->formatAttached = true; + data->storageNodeName = src->nodestorage; + data->storageAttached = true; + } + + if (src->pr && + !virStoragePRDefIsManaged(src->pr) && + VIR_STRDUP(data->prmgrAlias, src->pr->mgralias) < 0) + goto cleanup; + + if (VIR_STRDUP(data->tlsAlias, src->tlsAlias) < 0) + goto cleanup; + + if (srcpriv) { + if (srcpriv->secinfo && + srcpriv->secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES && + VIR_STRDUP(data->authsecretAlias, srcpriv->secinfo->s.aes.alias) < 0) + goto cleanup; + + if (srcpriv->encinfo && + srcpriv->encinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES && + VIR_STRDUP(data->encryptsecretAlias, srcpriv->encinfo->s.aes.alias) < 0) + goto cleanup; + } + + VIR_STEAL_PTR(ret, data); + + cleanup: + VIR_FREE(driveAlias); + qemuBlockStorageSourceAttachDataFree(data); + return ret; +} + + +static qemuHotplugDiskSourceDataPtr +qemuHotplugDiskSourceRemovePrepare(virDomainDiskDefPtr disk, + virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED) +{ + qemuBlockStorageSourceAttachDataPtr backend; + qemuHotplugDiskSourceDataPtr data; + qemuHotplugDiskSourceDataPtr ret = NULL; + char *drivealias = NULL; + + if (VIR_ALLOC(data) < 0) + return NULL; + + if (!(drivealias = qemuAliasDiskDriveFromDisk(disk))) + goto cleanup; + + if (!(backend = qemuHotplugRemoveStorageSourcePrepareData(disk->src, + drivealias))) + goto cleanup; + + if (VIR_APPEND_ELEMENT(data->backends, data->nbackends, backend) < 0) + goto cleanup; + + VIR_STEAL_PTR(ret, data); + + cleanup: + qemuBlockStorageSourceAttachDataFree(backend); + qemuHotplugDiskSourceDataFree(data); + return ret; +} + + /** * qemuHotplugDiskSourceAttachPrepare: * @disk: disk to generate attachment data for @@ -3853,36 +3943,21 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { - qemuDomainStorageSourcePrivatePtr diskPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); + qemuHotplugDiskSourceDataPtr diskbackend = NULL; virDomainDeviceDef dev; virObjectEventPtr event; size_t i; qemuDomainObjPrivatePtr priv = vm->privateData; - char *drivestr; bool prManaged = priv->prDaemonRunning; bool prUsed = false; - const char *authAlias = NULL; - const char *encAlias = NULL; int ret = -1; VIR_DEBUG("Removing disk %s from domain %p %s", disk->info.alias, vm, vm->def->name); - /* build the actual drive id string as the disk->info.alias doesn't - * contain the QEMU_DRIVE_HOST_PREFIX that is passed to qemu */ - if (!(drivestr = qemuAliasDiskDriveFromDisk(disk))) + if (!(diskbackend = qemuHotplugDiskSourceRemovePrepare(disk, priv->qemuCaps))) return -1; - if (diskPriv) { - if (diskPriv->secinfo && - diskPriv->secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) - authAlias = diskPriv->secinfo->s.aes.alias; - - if (diskPriv->encinfo && - diskPriv->encinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) - encAlias = diskPriv->encinfo->s.aes.alias; - } - for (i = 0; i < vm->def->ndisks; i++) { if (vm->def->disks[i] == disk) { virDomainDiskRemove(vm->def, i); @@ -3895,24 +3970,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); - qemuMonitorDriveDel(priv->mon, drivestr); - VIR_FREE(drivestr); - - /* If it fails, then so be it - it was a best shot */ - if (authAlias) - ignore_value(qemuMonitorDelObject(priv->mon, authAlias)); - - /* If it fails, then so be it - it was a best shot */ - if (encAlias) - ignore_value(qemuMonitorDelObject(priv->mon, encAlias)); - - /* If it fails, then so be it - it was a best shot */ - if (disk->src->pr && - !virStoragePRDefIsManaged(disk->src->pr)) - ignore_value(qemuMonitorDelObject(priv->mon, disk->src->pr->mgralias)); - - if (disk->src->tlsAlias) - ignore_value(qemuMonitorDelObject(priv->mon, disk->src->tlsAlias)); + qemuHotplugDiskSourceRemove(priv->mon, diskbackend); if (prManaged && !prUsed) ignore_value(qemuMonitorDelObject(priv->mon, qemuDomainGetManagedPRAlias())); @@ -3941,6 +3999,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, ret = 0; cleanup: + qemuHotplugDiskSourceDataFree(diskbackend); virDomainDiskDefFree(disk); return ret; } -- 2.16.2
On Tue, Jul 17, 2018 at 02:14:26PM +0200, Peter Krempa wrote:
Add code which will convert a disk definition into qemuHotplugDiskSourceData and then reuse qemuHotplugDiskSourceRemove to remove all the backend related objects.
This unifies the detach code as much as possible with the already existing helpers and will allow reuse this infrastructure when changing removable disk media.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 129 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 94 insertions(+), 35 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f1b18fcc7d..3ee74c8e40 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -373,6 +373,96 @@ qemuHotplugDiskSourceDataFree(qemuHotplugDiskSourceDataPtr data) }
+/** + * qemuDomainRemoveDiskStorageSourcePrepareData: + * @src: disk source structure + * @driveAlias: Alias of the -drive backend, the pointer is always consumed + * + * Prepare qemuBlockStorageSourceAttachDataPtr for detaching a single source + * from a VM. If @driveAlias is NULL -blockdev is assumed. + */ +static qemuBlockStorageSourceAttachDataPtr +qemuHotplugRemoveStorageSourcePrepareData(virStorageSourcePtr src, + char *driveAlias) + +{ + qemuDomainStorageSourcePrivatePtr srcpriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); + qemuBlockStorageSourceAttachDataPtr data; + qemuBlockStorageSourceAttachDataPtr ret = NULL; + + if (VIR_ALLOC(data) < 0) + goto cleanup; + + if (driveAlias) { + VIR_STEAL_PTR(data->driveAlias, driveAlias); + data->driveAdded = true; + } else { + data->formatNodeName = src->nodeformat; + data->formatAttached = true; + data->storageNodeName = src->nodestorage; + data->storageAttached = true; + } + + if (src->pr && + !virStoragePRDefIsManaged(src->pr) && + VIR_STRDUP(data->prmgrAlias, src->pr->mgralias) < 0) + goto cleanup; + + if (VIR_STRDUP(data->tlsAlias, src->tlsAlias) < 0) + goto cleanup; + + if (srcpriv) { + if (srcpriv->secinfo && + srcpriv->secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES && + VIR_STRDUP(data->authsecretAlias, srcpriv->secinfo->s.aes.alias) < 0) + goto cleanup; + + if (srcpriv->encinfo && + srcpriv->encinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES && + VIR_STRDUP(data->encryptsecretAlias, srcpriv->encinfo->s.aes.alias) < 0) + goto cleanup; + } + + VIR_STEAL_PTR(ret, data); + + cleanup: + VIR_FREE(driveAlias); + qemuBlockStorageSourceAttachDataFree(data); + return ret; +} + + +static qemuHotplugDiskSourceDataPtr +qemuHotplugDiskSourceRemovePrepare(virDomainDiskDefPtr disk, + virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED) +{ + qemuBlockStorageSourceAttachDataPtr backend; + qemuHotplugDiskSourceDataPtr data; + qemuHotplugDiskSourceDataPtr ret = NULL; + char *drivealias = NULL; + + if (VIR_ALLOC(data) < 0) + return NULL; + + if (!(drivealias = qemuAliasDiskDriveFromDisk(disk))) + goto cleanup; +
qemu/qemu_hotplug.c:447:9: error: variable 'backend' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] if (!(drivealias = qemuAliasDiskDriveFromDisk(disk))) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ if (!(backend = qemuHotplugRemoveStorageSourcePrepareData(disk->src, + drivealias))) + goto cleanup; +
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Extract the (possible) removal of the PR backend and daemon into a separate helper which enters monitor on it's own. This simplifies the code and allows reuse of this function in the future e.g. for blockjobs where removing a image with PR may result into PR not being necessary. Since the PR is not used often the overhead of entering monitor again should be negligible. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 49 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 3ee74c8e40..57ab753974 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -350,6 +350,41 @@ qemuDomainDiskAttachManagedPR(virDomainObjPtr vm, } +/** + * qemuHotplugRemoveManagedPR: + * @driver: QEMU driver object + * @vm: domain object + * @asyncJob: asynchronous job identifier + * + * Removes the managed PR object from @vm if the configuration does not require + * it any more. + */ +static int +qemuHotplugRemoveManagedPR(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virErrorPtr orig_err; + virErrorPreserveLast(&orig_err); + + if (!priv->prDaemonRunning || + virDomainDefHasManagedPR(vm->def)) + return 0; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + ignore_value(qemuMonitorDelObject(priv->mon, qemuDomainGetManagedPRAlias())); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; + + qemuProcessKillManagedPRDaemon(vm); + virErrorRestore(&orig_err); + + return 0; +} + + struct _qemuHotplugDiskSourceData { qemuBlockStorageSourceAttachDataPtr *backends; size_t nbackends; @@ -3948,8 +3983,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, virObjectEventPtr event; size_t i; qemuDomainObjPrivatePtr priv = vm->privateData; - bool prManaged = priv->prDaemonRunning; - bool prUsed = false; int ret = -1; VIR_DEBUG("Removing disk %s from domain %p %s", @@ -3965,16 +3998,10 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, } } - /* check if the last disk with managed PR was just removed */ - prUsed = virDomainDefHasManagedPR(vm->def); - qemuDomainObjEnterMonitor(driver, vm); qemuHotplugDiskSourceRemove(priv->mon, diskbackend); - if (prManaged && !prUsed) - ignore_value(qemuMonitorDelObject(priv->mon, qemuDomainGetManagedPRAlias())); - if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -3983,9 +4010,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, event = virDomainEventDeviceRemovedNewFromObj(vm, disk->info.alias); virObjectEventStateQueue(driver->domainEventState, event); - if (prManaged && !prUsed) - qemuProcessKillManagedPRDaemon(vm); - qemuDomainReleaseDeviceAddress(vm, &disk->info, virDomainDiskGetSource(disk)); /* tear down disk security access */ @@ -3996,6 +4020,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, ignore_value(qemuRemoveSharedDevice(driver, &dev, vm->def->name)); virDomainUSBAddressRelease(priv->usbaddrs, &disk->info); + if (qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + goto cleanup; + ret = 0; cleanup: -- 2.16.2
On Tue, Jul 17, 2018 at 02:14:27PM +0200, Peter Krempa wrote:
Extract the (possible) removal of the PR backend and daemon into a separate helper which enters monitor on it's own. This simplifies the
its
code and allows reuse of this function in the future e.g. for blockjobs where removing a image with PR may result into PR not being necessary.
Since the PR is not used often the overhead of entering monitor again should be negligible.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 49 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 11 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 07/17/2018 08:14 AM, Peter Krempa wrote:
Extract the (possible) removal of the PR backend and daemon into a separate helper which enters monitor on it's own. This simplifies the code and allows reuse of this function in the future e.g. for blockjobs where removing a image with PR may result into PR not being necessary.
Since the PR is not used often the overhead of entering monitor again should be negligible.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 49 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 3ee74c8e40..57ab753974 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -350,6 +350,41 @@ qemuDomainDiskAttachManagedPR(virDomainObjPtr vm, }
+/** + * qemuHotplugRemoveManagedPR: + * @driver: QEMU driver object + * @vm: domain object + * @asyncJob: asynchronous job identifier + * + * Removes the managed PR object from @vm if the configuration does not require + * it any more. + */ +static int +qemuHotplugRemoveManagedPR(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virErrorPtr orig_err; + virErrorPreserveLast(&orig_err);
Coverity points out that orig_err is leaked at each subsequent return before the virErrorRestore below John
+ + if (!priv->prDaemonRunning || + virDomainDefHasManagedPR(vm->def)) + return 0; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + ignore_value(qemuMonitorDelObject(priv->mon, qemuDomainGetManagedPRAlias())); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; + + qemuProcessKillManagedPRDaemon(vm); + virErrorRestore(&orig_err); + + return 0; +} + +
[...]
Similarly to qemuDomainDiskRemoveManagedPR make it enter monitor on it's own so that it can be reused. Future users will be in the snapshot code and in removable media change code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 55 ++++++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 57ab753974..c751660403 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -310,29 +310,31 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, /** - * qemuDomainMaybeStartPRDaemon: + * qemuHotplugAttachManagedPR: + * @driver: QEMU driver object * @vm: domain object - * @disk: disk to hotplug - * @retProps: properties of the managed pr-manager-helper object which needs - * to be added to the running vm + * @src: new disk source to be attached to @vm + * @asyncJob: asynchronous job identifier * * Checks if it's needed to start qemu-pr-helper and add the corresponding * pr-manager-helper object. * - * Returns: 0 on success, -1 on error. If @retProps is populated the - * qemu-pr-helper daemon was started. + * Returns: 0 on success, -1 on error. */ static int -qemuDomainDiskAttachManagedPR(virDomainObjPtr vm, - virDomainDiskDefPtr disk, - virJSONValuePtr *retProps) +qemuHotplugAttachManagedPR(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src, + qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; virJSONValuePtr props = NULL; + bool daemonStarted = false; int ret = -1; + int rc; if (priv->prDaemonRunning || - !virStorageSourceChainHasManagedPR(disk->src)) + !virStorageSourceChainHasManagedPR(src)) return 0; if (!(props = qemuBuildPRManagedManagerInfoProps(priv))) @@ -341,10 +343,21 @@ qemuDomainDiskAttachManagedPR(virDomainObjPtr vm, if (qemuProcessStartManagedPRDaemon(vm) < 0) goto cleanup; - VIR_STEAL_PTR(*retProps, props); + daemonStarted = true; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + goto cleanup; + + rc = qemuMonitorAddObject(priv->mon, &props, NULL); + + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) + goto cleanup; + ret = 0; cleanup: + if (ret < 0 && daemonStarted) + qemuProcessKillManagedPRDaemon(vm); virJSONValueFree(props); return ret; } @@ -592,7 +605,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; qemuHotplugDiskSourceDataPtr diskdata = NULL; - virErrorPtr orig_err; char *devstr = NULL; char *managedPrmgrAlias = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -607,9 +619,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) goto error; - if (qemuDomainDiskAttachManagedPR(vm, disk, &managedPrmgrProps) < 0) - goto error; - if (!(diskdata = qemuHotplugDiskSourceAttachPrepare(disk, priv->qemuCaps))) goto error; @@ -619,11 +628,10 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks + 1) < 0) goto error; - qemuDomainObjEnterMonitor(driver, vm); + if (qemuHotplugAttachManagedPR(driver, vm, disk->src, QEMU_ASYNC_JOB_NONE) < 0) + goto error; - if (managedPrmgrProps && - qemuMonitorAddObject(priv->mon, &managedPrmgrProps, &managedPrmgrAlias) < 0) - goto exit_monitor; + qemuDomainObjEnterMonitor(driver, vm); if (qemuHotplugDiskSoureceAttach(priv->mon, diskdata) < 0) goto exit_monitor; @@ -653,20 +661,15 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, exit_monitor: qemuHotplugDiskSourceRemove(priv->mon, diskdata); - virErrorPreserveLast(&orig_err); - if (managedPrmgrAlias) - ignore_value(qemuMonitorDelObject(priv->mon, managedPrmgrAlias)); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -2; - virErrorRestore(&orig_err); + if (qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + ret = -2; virDomainAuditDisk(vm, NULL, disk->src, "attach", false); error: ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true)); - if (priv->prDaemonRunning && - !virDomainDefHasManagedPR(vm->def)) - qemuProcessKillManagedPRDaemon(vm); goto cleanup; } -- 2.16.2
On Tue, Jul 17, 2018 at 02:14:28PM +0200, Peter Krempa wrote:
Similarly to qemuDomainDiskRemoveManagedPR make it enter monitor on it's own so that it can be reused. Future users will be in the snapshot
its
code and in removable media change code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 55 ++++++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 26 deletions(-)
@@ -619,11 +628,10 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks + 1) < 0) goto error;
- qemuDomainObjEnterMonitor(driver, vm); + if (qemuHotplugAttachManagedPR(driver, vm, disk->src, QEMU_ASYNC_JOB_NONE) < 0) + goto error;
- if (managedPrmgrProps && - qemuMonitorAddObject(priv->mon, &managedPrmgrProps, &managedPrmgrAlias) < 0)
Both variables are unused after this change.
- goto exit_monitor; + qemuDomainObjEnterMonitor(driver, vm);
if (qemuHotplugDiskSoureceAttach(priv->mon, diskdata) < 0) goto exit_monitor;
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Prepare for the -blockdev implementation of ejectable media changing by splitting up the old bits. Additionally since both callers make sure that the device is a cdrom or floppy the check is no longer necessary. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 99 +++++++++++++++++++++++++++++++------------------ 1 file changed, 62 insertions(+), 37 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c751660403..1a7d8f0ca3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -192,7 +192,7 @@ qemuHotplugWaitForTrayEject(virQEMUDriverPtr driver, /** - * qemuDomainChangeEjectableMedia: + * qemuDomainChangeMediaLegacy: * @driver: qemu driver structure * @vm: domain definition * @disk: disk definition to change the source of @@ -206,12 +206,12 @@ qemuHotplugWaitForTrayEject(virQEMUDriverPtr driver, * * Returns 0 on success, -1 on error and reports libvirt error */ -int -qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk, - virStorageSourcePtr newsrc, - bool force) +static int +qemuDomainChangeMediaLegacy(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk, + virStorageSourcePtr newsrc, + bool force) { int ret = -1, rc; char *driveAlias = NULL; @@ -231,19 +231,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (srcPriv) secinfo = srcPriv->secinfo; - if (disk->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && - disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Removable media not supported for %s device"), - virDomainDiskDeviceTypeToString(disk->device)); - goto cleanup; - } - - if (qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, false) < 0) - goto cleanup; - if (!(driveAlias = qemuAliasDiskDriveFromDisk(disk))) - goto error; + goto cleanup; qemuDomainObjEnterMonitor(driver, vm); rc = qemuMonitorEjectMedia(priv->mon, driveAlias, force); @@ -255,16 +244,16 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_TRAY_MOVED)) { rc = qemuHotplugWaitForTrayEject(driver, vm, disk, driveAlias); if (rc < 0) - goto error; + goto cleanup; } else { /* otherwise report possible errors from the attempt to eject the media*/ if (rc < 0) - goto error; + goto cleanup; } if (!virStorageSourceIsEmpty(newsrc)) { if (qemuGetDriveSourceString(newsrc, secinfo, &sourcestr) < 0) - goto error; + goto cleanup; if (virStorageSourceGetActualType(newsrc) != VIR_STORAGE_TYPE_DIR) { if (newsrc->format > 0) { @@ -283,29 +272,15 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, goto cleanup; } - virDomainAuditDisk(vm, disk->src, newsrc, "update", rc >= 0); - if (rc < 0) - goto error; - - /* remove the old source from shared device list */ - ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); - ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true)); + goto cleanup; - virStorageSourceFree(disk->src); - disk->src = newsrc; - newsrc = NULL; ret = 0; cleanup: VIR_FREE(driveAlias); VIR_FREE(sourcestr); return ret; - - error: - virDomainAuditDisk(vm, disk->src, newsrc, "update", false); - ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, true)); - goto cleanup; } @@ -592,6 +567,56 @@ qemuHotplugDiskSourceRemove(qemuMonitorPtr mon, } +/** + * qemuDomainChangeEjectableMedia: + * @driver: qemu driver structure + * @vm: domain definition + * @disk: disk definition to change the source of + * @newsrc: new disk source to change to + * @force: force the change of media + * + * Change the media in an ejectable device to the one described by + * @newsrc. This function also removes the old source from the + * shared device table if appropriate. Note that newsrc is consumed + * on success and the old source is freed on success. + * + * Returns 0 on success, -1 on error and reports libvirt error + */ +int +qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk, + virStorageSourcePtr newsrc, + bool force) +{ + int ret = -1; + int rc; + + if (qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, false) < 0) + goto cleanup; + + rc = qemuDomainChangeMediaLegacy(driver, vm, disk, newsrc, force); + + virDomainAuditDisk(vm, disk->src, newsrc, "update", rc >= 0); + + if (rc < 0) { + ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, true)); + goto cleanup; + } + + /* remove the old source from shared device list */ + ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); + ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true)); + + virStorageSourceFree(disk->src); + VIR_STEAL_PTR(disk->src, newsrc); + ret = 0; + + cleanup: + return ret; +} + + /** * qemuDomainAttachDiskGeneric: * -- 2.16.2
On Tue, Jul 17, 2018 at 02:14:29PM +0200, Peter Krempa wrote:
Prepare for the -blockdev implementation of ejectable media changing by splitting up the old bits.
Additionally since both callers make sure that the device is a cdrom or floppy the check is no longer necessary.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 99 +++++++++++++++++++++++++++++++------------------ 1 file changed, 62 insertions(+), 37 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Remove the issue of the monitor command to the caller so that the function can be used with the modern approach. Additionally improve the error message. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1a7d8f0ca3..774ee25286 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -155,10 +155,8 @@ qemuHotplugPrepareDiskAccess(virQEMUDriverPtr driver, static int -qemuHotplugWaitForTrayEject(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk, - const char *driveAlias) +qemuHotplugWaitForTrayEject(virDomainObjPtr vm, + virDomainDiskDefPtr disk) { unsigned long long now; int rc; @@ -174,19 +172,14 @@ qemuHotplugWaitForTrayEject(virQEMUDriverPtr driver, /* the caller called qemuMonitorEjectMedia which usually reports an * error. Report the failure in an off-chance that it didn't. */ if (virGetLastErrorCode() == VIR_ERR_OK) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("timed out waiting for disk tray status update")); + virReportError(VIR_ERR_OPERATION_FAILED, + _("timed out waiting to open tray of '%s'"), + disk->dst); } return -1; } } - /* re-issue ejection command to pop out the media */ - qemuDomainObjEnterMonitor(driver, vm); - rc = qemuMonitorEjectMedia(qemuDomainGetMonitor(vm), driveAlias, false); - if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) - return -1; - return 0; } @@ -242,9 +235,16 @@ qemuDomainChangeMediaLegacy(virQEMUDriverPtr driver, /* If the tray is present and tray change event is supported wait for it to open. */ if (!force && diskPriv->tray && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_TRAY_MOVED)) { - rc = qemuHotplugWaitForTrayEject(driver, vm, disk, driveAlias); + rc = qemuHotplugWaitForTrayEject(vm, disk); if (rc < 0) goto cleanup; + + /* re-issue ejection command to pop out the media */ + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorEjectMedia(priv->mon, driveAlias, false); + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) + goto cleanup; + } else { /* otherwise report possible errors from the attempt to eject the media*/ if (rc < 0) -- 2.16.2
On Tue, Jul 17, 2018 at 02:14:30PM +0200, Peter Krempa wrote:
Remove the issue of the monitor command to the caller so that the function can be used with the modern approach.
Additionally improve the error message.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
When changing cdrom media we did not handle the managed PR objects thus we'd either have a stale PR object left behind or the media change would fail. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- Opps this one was also left behind in the blockdev-add series but belongs to this posting. src/qemu/qemu_hotplug.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f416db2c60..a1e523013d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -595,6 +595,9 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (qemuHotplugPrepareDiskAccess(driver, vm, disk, newsrc, false) < 0) goto cleanup; + if (qemuHotplugAttachManagedPR(driver, vm, newsrc, QEMU_ASYNC_JOB_NONE) < 0) + goto cleanup; + rc = qemuDomainChangeMediaLegacy(driver, vm, disk, newsrc, force); virDomainAuditDisk(vm, disk->src, newsrc, "update", rc >= 0); @@ -610,6 +613,9 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, virStorageSourceFree(disk->src); VIR_STEAL_PTR(disk->src, newsrc); + + ignore_value(qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE)); + ret = 0; cleanup: -- 2.16.2
On Wed, Jul 18, 2018 at 01:36:54PM +0200, Peter Krempa wrote:
When changing cdrom media we did not handle the managed PR objects thus we'd either have a stale PR object left behind or the media change would fail.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
Opps this one was also left behind in the blockdev-add series but belongs to this posting.
src/qemu/qemu_hotplug.c | 6 ++++++ 1 file changed, 6 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
John Ferlan -
Ján Tomko -
Peter Krempa