[libvirt] [PATCH 00/12] qemu: Refactor image permission/lock setting (blockdev-add saga)
With new blockjob handling we'll need to modify permissions for chains and individual images. The individual image code was universally accessible but the chain setting code reimplemented it mostly only in qemu_hotplug.h. Refactor the handling by moving the code to qemu_domain.c and making it universal. Peter Krempa (12): qemu: Rename qemuDomainDiskChainElement(Revoke|Prepare) qemu: Move and rename qemuHotplugPrepareDiskSourceAccess qemu: Split entry points to qemuDomainStorageSourceChainAccessPrepare qemu: domain: Rename qemuDomainStorageSourceChainAccessPrepare qemu: Convert boolean flags to enum flags in qemuDomainStorageSourceAccessModify qemu: Allow using qemuDomainStorageSourceAccessModify on singe images qemu: Refactor/simplify qemuDomainStorageSourceAccessRevoke qemu: Allow forcing read-only mode in qemuDomainStorageSourceAccessModify qemu: Use bools rather than labels in qemuDomainStorageSourceAccessModify qemu: Allow skipping the revoke step in qemuDomainStorageSourceAccessModify qemu: Mark when modifying access to existing source in qemuDomainStorageSourceAccessModify qemu: Refactor/simplify qemuDomainStorageSourceAccessAllow src/qemu/qemu_domain.c | 212 +++++++++++++++++++++++++++++++--------- src/qemu/qemu_domain.h | 23 +++-- src/qemu/qemu_driver.c | 24 ++--- src/qemu/qemu_hotplug.c | 80 ++------------- 4 files changed, 200 insertions(+), 139 deletions(-) -- 2.20.1
Use qemuDomainStorageSourceAccess(Allow|Revoke) instead. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 20 ++++++++++---------- src/qemu/qemu_domain.h | 16 ++++++++-------- src/qemu/qemu_driver.c | 24 ++++++++++++------------ 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 960aaff3c7..ce8f4a0dca 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9212,15 +9212,15 @@ qemuDomainDiskGetBackendAlias(virDomainDiskDefPtr disk, /** - * qemuDomainDiskChainElementRevoke: + * qemuDomainStorageSourceAccessRevoke: * * Revoke access to a single backing chain element. This restores the labels, * removes cgroup ACLs for devices and removes locks. */ void -qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virStorageSourcePtr elem) +qemuDomainStorageSourceAccessRevoke(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr elem) { if (qemuTeardownImageCgroup(vm, elem) < 0) VIR_WARN("Failed to teardown cgroup for disk path %s", @@ -9238,7 +9238,7 @@ qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver, /** - * qemuDomainDiskChainElementPrepare: + * qemuDomainStorageSourceAccessAllow: * @driver: qemu driver data * @vm: domain object * @elem: source structure to set access for @@ -9253,11 +9253,11 @@ qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver, * backing chain) @newSource needs to be set to false. */ int -qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virStorageSourcePtr elem, - bool readonly, - bool newSource) +qemuDomainStorageSourceAccessAllow(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr elem, + bool readonly, + bool newSource) { bool was_readonly = elem->readonly; virQEMUDriverConfigPtr cfg = NULL; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 06640a9510..9a48b5b69d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -770,14 +770,14 @@ int qemuDomainDiskGetBackendAlias(virDomainDiskDefPtr disk, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; -void qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virStorageSourcePtr elem); -int qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virStorageSourcePtr elem, - bool readonly, - bool newSource); +void qemuDomainStorageSourceAccessRevoke(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr elem); +int qemuDomainStorageSourceAccessAllow(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr elem, + bool readonly, + bool newSource); int qemuDomainCleanupAdd(virDomainObjPtr vm, qemuDomainCleanupCallback cb); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c443c881d5..b038bfc360 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15025,7 +15025,7 @@ struct _qemuDomainSnapshotDiskData { virStorageSourcePtr src; bool initialized; /* @src was initialized in the storage driver */ bool created; /* @src was created by the snapshot code */ - bool prepared; /* @src was prepared using qemuDomainDiskChainElementPrepare */ + bool prepared; /* @src was prepared using qemuDomainStorageSourceAccessAllow */ virDomainDiskDefPtr disk; char *relPath; /* relative path component to fill into original disk */ @@ -15056,7 +15056,7 @@ qemuDomainSnapshotDiskDataFree(qemuDomainSnapshotDiskDataPtr data, virStorageFileDeinit(data[i].src); if (data[i].prepared) - qemuDomainDiskChainElementRevoke(driver, vm, data[i].src); + qemuDomainStorageSourceAccessRevoke(driver, vm, data[i].src); virObjectUnref(data[i].src); } @@ -15216,8 +15216,8 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, } /* set correct security, cgroup and locking options on the new image */ - if (qemuDomainDiskChainElementPrepare(driver, vm, dd->src, false, true) < 0) { - qemuDomainDiskChainElementRevoke(driver, vm, dd->src); + if (qemuDomainStorageSourceAccessAllow(driver, vm, dd->src, false, true) < 0) { + qemuDomainStorageSourceAccessRevoke(driver, vm, dd->src); goto cleanup; } @@ -15314,7 +15314,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, continue; if (diskdata[i].prepared) - qemuDomainDiskChainElementRevoke(driver, vm, diskdata[i].src); + qemuDomainStorageSourceAccessRevoke(driver, vm, diskdata[i].src); if (diskdata[i].created && virStorageFileUnlink(diskdata[i].src) < 0) @@ -17752,8 +17752,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, qemuSecuritySetImageLabel(driver, vm, mirror, true) < 0) goto endjob; } else { - if (qemuDomainDiskChainElementPrepare(driver, vm, mirror, false, true) < 0) { - qemuDomainDiskChainElementRevoke(driver, vm, mirror); + if (qemuDomainStorageSourceAccessAllow(driver, vm, mirror, false, true) < 0) { + qemuDomainStorageSourceAccessRevoke(driver, vm, mirror); goto endjob; } } @@ -17774,7 +17774,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, ret = -1; if (ret < 0) { monitor_error = virSaveLastError(); - qemuDomainDiskChainElementRevoke(driver, vm, mirror); + qemuDomainStorageSourceAccessRevoke(driver, vm, mirror); goto endjob; } @@ -18149,9 +18149,9 @@ qemuDomainBlockCommit(virDomainPtr dom, * operation succeeds, but doing that requires tracking the * operation in XML across libvirtd restarts. */ clean_access = true; - if (qemuDomainDiskChainElementPrepare(driver, vm, baseSource, false, false) < 0 || + if (qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, false, false) < 0 || (top_parent && top_parent != disk->src && - qemuDomainDiskChainElementPrepare(driver, vm, top_parent, false, false) < 0)) + qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, false, false) < 0)) goto endjob; if (!(job = qemuBlockJobDiskNew(disk, jobtype, device))) @@ -18192,9 +18192,9 @@ qemuDomainBlockCommit(virDomainPtr dom, if (ret < 0 && clean_access) { virErrorPtr orig_err = virSaveLastError(); /* Revert access to read-only, if possible. */ - qemuDomainDiskChainElementPrepare(driver, vm, baseSource, true, false); + qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, true, false); if (top_parent && top_parent != disk->src) - qemuDomainDiskChainElementPrepare(driver, vm, top_parent, true, false); + qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, true, false); if (orig_err) { virSetError(orig_err); -- 2.20.1
On Thu, Apr 18, 2019 at 04:42:56PM +0200, Peter Krempa wrote:
Use qemuDomainStorageSourceAccess(Allow|Revoke) instead.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 20 ++++++++++---------- src/qemu/qemu_domain.h | 16 ++++++++-------- src/qemu/qemu_driver.c | 24 ++++++++++++------------ 3 files changed, 30 insertions(+), 30 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Move it to qemu_domain.c and call it qemuDomainStorageSourceChainAccessPrepare. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 68 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 5 +++ src/qemu/qemu_hotplug.c | 80 ++++------------------------------------- 3 files changed, 79 insertions(+), 74 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ce8f4a0dca..ab96688a65 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9211,6 +9211,74 @@ qemuDomainDiskGetBackendAlias(virDomainDiskDefPtr disk, } +/** + * qemuDomainStorageSourceChainAccessPrepare: + * @driver: qemu driver struct + * @vm: domain object + * @src: Source to prepare + * @teardown: Teardown the access to @src instead of adding it to a vm + * + * Setup the locks, cgroups and security permissions on a disk source and its + * backing chain. If @teardown is true, then the labels and cgroups are removed + * instead. + * + * Returns 0 on success and -1 on error. Reports libvirt error. + */ +int +qemuDomainStorageSourceChainAccessPrepare(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src, + bool teardown) +{ + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); + const char *srcstr = NULLSTR(src->path); + int ret = -1; + virErrorPtr orig_err = NULL; + + /* just tear down the disk access */ + if (teardown) { + virErrorPreserveLast(&orig_err); + ret = 0; + goto rollback_cgroup; + } + + if (virDomainLockImageAttach(driver->lockManager, cfg->uri, vm, src) < 0) + goto cleanup; + + if (qemuDomainNamespaceSetupDisk(vm, src) < 0) + goto rollback_lock; + + if (qemuSecuritySetImageLabel(driver, vm, src, true) < 0) + goto rollback_namespace; + + if (qemuSetupImageChainCgroup(vm, src) < 0) + goto rollback_label; + + ret = 0; + goto cleanup; + + rollback_cgroup: + if (qemuTeardownImageChainCgroup(vm, src) < 0) + VIR_WARN("Unable to tear down cgroup access on %s", srcstr); + rollback_label: + if (qemuSecurityRestoreImageLabel(driver, vm, src, true) < 0) + VIR_WARN("Unable to restore security label on %s", srcstr); + + rollback_namespace: + if (qemuDomainNamespaceTeardownDisk(vm, src) < 0) + VIR_WARN("Unable to remove /dev entry for %s", srcstr); + + rollback_lock: + if (virDomainLockImageDetach(driver->lockManager, vm, src) < 0) + VIR_WARN("Unable to release lock on %s", srcstr); + + cleanup: + virErrorRestore(&orig_err); + + return ret; +} + + /** * qemuDomainStorageSourceAccessRevoke: * diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 9a48b5b69d..65b0e8c39d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -770,6 +770,11 @@ int qemuDomainDiskGetBackendAlias(virDomainDiskDefPtr disk, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; +int qemuDomainStorageSourceChainAccessPrepare(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src, + bool teardown); + void qemuDomainStorageSourceAccessRevoke(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr elem); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a4f7d111b1..dd5571aea3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -132,74 +132,6 @@ qemuDomainDeleteDevice(virDomainObjPtr vm, } -/** - * qemuHotplugPrepareDiskSourceAccess: - * @driver: qemu driver struct - * @vm: domain object - * @src: Source to prepare - * @teardown: Teardown the access to @src instead of adding it to a vm - * - * Setup the locks, cgroups and security permissions on a disk source and its - * backing chain. If @teardown is true, then the labels and cgroups are removed - * instead. - * - * Returns 0 on success and -1 on error. Reports libvirt error. - */ -static int -qemuHotplugPrepareDiskSourceAccess(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virStorageSourcePtr src, - bool teardown) -{ - VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); - const char *srcstr = NULLSTR(src->path); - int ret = -1; - virErrorPtr orig_err = NULL; - - /* just tear down the disk access */ - if (teardown) { - virErrorPreserveLast(&orig_err); - ret = 0; - goto rollback_cgroup; - } - - if (virDomainLockImageAttach(driver->lockManager, cfg->uri, vm, src) < 0) - goto cleanup; - - if (qemuDomainNamespaceSetupDisk(vm, src) < 0) - goto rollback_lock; - - if (qemuSecuritySetImageLabel(driver, vm, src, true) < 0) - goto rollback_namespace; - - if (qemuSetupImageChainCgroup(vm, src) < 0) - goto rollback_label; - - ret = 0; - goto cleanup; - - rollback_cgroup: - if (qemuTeardownImageChainCgroup(vm, src) < 0) - VIR_WARN("Unable to tear down cgroup access on %s", srcstr); - rollback_label: - if (qemuSecurityRestoreImageLabel(driver, vm, src, true) < 0) - VIR_WARN("Unable to restore security label on %s", srcstr); - - rollback_namespace: - if (qemuDomainNamespaceTeardownDisk(vm, src) < 0) - VIR_WARN("Unable to remove /dev entry for %s", srcstr); - - rollback_lock: - if (virDomainLockImageDetach(driver->lockManager, vm, src) < 0) - VIR_WARN("Unable to release lock on %s", srcstr); - - cleanup: - virErrorRestore(&orig_err); - - return ret; -} - - static int qemuDomainAttachZPCIDevice(qemuMonitorPtr mon, virDomainDeviceInfoPtr info) @@ -877,7 +809,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) goto cleanup; - if (qemuHotplugPrepareDiskSourceAccess(driver, vm, newsrc, false) < 0) + if (qemuDomainStorageSourceChainAccessPrepare(driver, vm, newsrc, false) < 0) goto cleanup; if (qemuHotplugAttachManagedPR(driver, vm, newsrc, QEMU_ASYNC_JOB_NONE) < 0) @@ -896,7 +828,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, /* remove the old source from shared device list */ disk->src = oldsrc; ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); - ignore_value(qemuHotplugPrepareDiskSourceAccess(driver, vm, oldsrc, true)); + ignore_value(qemuDomainStorageSourceChainAccessPrepare(driver, vm, oldsrc, true)); /* media was changed, so we can remove the old media definition now */ virObjectUnref(oldsrc); @@ -911,7 +843,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (sharedAdded) ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); - ignore_value(qemuHotplugPrepareDiskSourceAccess(driver, vm, newsrc, true)); + ignore_value(qemuDomainStorageSourceChainAccessPrepare(driver, vm, newsrc, true)); } /* remove PR manager object if unneeded */ @@ -941,7 +873,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, char *devstr = NULL; VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); - if (qemuHotplugPrepareDiskSourceAccess(driver, vm, disk->src, false) < 0) + if (qemuDomainStorageSourceChainAccessPrepare(driver, vm, disk->src, false) < 0) goto cleanup; if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) @@ -1003,7 +935,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, virDomainAuditDisk(vm, NULL, disk->src, "attach", false); error: - ignore_value(qemuHotplugPrepareDiskSourceAccess(driver, vm, disk->src, true)); + ignore_value(qemuDomainStorageSourceChainAccessPrepare(driver, vm, disk->src, true)); goto cleanup; } @@ -4532,7 +4464,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, qemuDomainReleaseDeviceAddress(vm, &disk->info); /* tear down disk security access */ - qemuHotplugPrepareDiskSourceAccess(driver, vm, disk->src, true); + qemuDomainStorageSourceChainAccessPrepare(driver, vm, disk->src, true); dev.type = VIR_DOMAIN_DEVICE_DISK; dev.data.disk = disk; -- 2.20.1
On Thu, Apr 18, 2019 at 04:42:57PM +0200, Peter Krempa wrote:
Move it to qemu_domain.c and call it qemuDomainStorageSourceChainAccessPrepare.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 68 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 5 +++ src/qemu/qemu_hotplug.c | 80 ++++------------------------------------- 3 files changed, 79 insertions(+), 74 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Introduce qemuDomainStorageSourceChainAccess(Allow|Revoke) as entry points to qemuDomainStorageSourceChainAccessPrepare for symmetry with the functions for single backing chain elements. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 20 +++++++++++++++++++- src/qemu/qemu_domain.h | 10 ++++++---- src/qemu/qemu_hotplug.c | 12 ++++++------ 3 files changed, 31 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ab96688a65..85e43027d5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9224,7 +9224,7 @@ qemuDomainDiskGetBackendAlias(virDomainDiskDefPtr disk, * * Returns 0 on success and -1 on error. Reports libvirt error. */ -int +static int qemuDomainStorageSourceChainAccessPrepare(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src, @@ -9279,6 +9279,24 @@ qemuDomainStorageSourceChainAccessPrepare(virQEMUDriverPtr driver, } +int +qemuDomainStorageSourceChainAccessAllow(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src) +{ + return qemuDomainStorageSourceChainAccessPrepare(driver, vm, src, false); +} + + +int +qemuDomainStorageSourceChainAccessRevoke(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src) +{ + return qemuDomainStorageSourceChainAccessPrepare(driver, vm, src, true); +} + + /** * qemuDomainStorageSourceAccessRevoke: * diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 65b0e8c39d..f92f0dbc27 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -770,10 +770,12 @@ int qemuDomainDiskGetBackendAlias(virDomainDiskDefPtr disk, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; -int qemuDomainStorageSourceChainAccessPrepare(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virStorageSourcePtr src, - bool teardown); +int qemuDomainStorageSourceChainAccessAllow(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src); +int qemuDomainStorageSourceChainAccessRevoke(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src); void qemuDomainStorageSourceAccessRevoke(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index dd5571aea3..ce5ca015ab 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -809,7 +809,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) goto cleanup; - if (qemuDomainStorageSourceChainAccessPrepare(driver, vm, newsrc, false) < 0) + if (qemuDomainStorageSourceChainAccessAllow(driver, vm, newsrc) < 0) goto cleanup; if (qemuHotplugAttachManagedPR(driver, vm, newsrc, QEMU_ASYNC_JOB_NONE) < 0) @@ -828,7 +828,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, /* remove the old source from shared device list */ disk->src = oldsrc; ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); - ignore_value(qemuDomainStorageSourceChainAccessPrepare(driver, vm, oldsrc, true)); + ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, oldsrc)); /* media was changed, so we can remove the old media definition now */ virObjectUnref(oldsrc); @@ -843,7 +843,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (sharedAdded) ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); - ignore_value(qemuDomainStorageSourceChainAccessPrepare(driver, vm, newsrc, true)); + ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, newsrc)); } /* remove PR manager object if unneeded */ @@ -873,7 +873,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, char *devstr = NULL; VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); - if (qemuDomainStorageSourceChainAccessPrepare(driver, vm, disk->src, false) < 0) + if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0) goto cleanup; if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) @@ -935,7 +935,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, virDomainAuditDisk(vm, NULL, disk->src, "attach", false); error: - ignore_value(qemuDomainStorageSourceChainAccessPrepare(driver, vm, disk->src, true)); + ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, disk->src)); goto cleanup; } @@ -4464,7 +4464,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, qemuDomainReleaseDeviceAddress(vm, &disk->info); /* tear down disk security access */ - qemuDomainStorageSourceChainAccessPrepare(driver, vm, disk->src, true); + qemuDomainStorageSourceChainAccessRevoke(driver, vm, disk->src); dev.type = VIR_DOMAIN_DEVICE_DISK; dev.data.disk = disk; -- 2.20.1
On Thu, Apr 18, 2019 at 04:42:58PM +0200, Peter Krempa wrote:
Introduce qemuDomainStorageSourceChainAccess(Allow|Revoke) as entry points to qemuDomainStorageSourceChainAccessPrepare for symmetry with the functions for single backing chain elements.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 20 +++++++++++++++++++- src/qemu/qemu_domain.h | 10 ++++++---- src/qemu/qemu_hotplug.c | 12 ++++++------ 3 files changed, 31 insertions(+), 11 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Thank you, Jano
The function will be able to deal with non-chains too so drop 'Chain' and also change the suffix to 'Modify' as it's used both for setup and teardown. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 85e43027d5..b4c59b9a74 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9212,7 +9212,7 @@ qemuDomainDiskGetBackendAlias(virDomainDiskDefPtr disk, /** - * qemuDomainStorageSourceChainAccessPrepare: + * qemuDomainStorageSourceAccessModify: * @driver: qemu driver struct * @vm: domain object * @src: Source to prepare @@ -9225,10 +9225,10 @@ qemuDomainDiskGetBackendAlias(virDomainDiskDefPtr disk, * Returns 0 on success and -1 on error. Reports libvirt error. */ static int -qemuDomainStorageSourceChainAccessPrepare(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virStorageSourcePtr src, - bool teardown) +qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src, + bool teardown) { VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); const char *srcstr = NULLSTR(src->path); @@ -9284,7 +9284,7 @@ qemuDomainStorageSourceChainAccessAllow(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src) { - return qemuDomainStorageSourceChainAccessPrepare(driver, vm, src, false); + return qemuDomainStorageSourceAccessModify(driver, vm, src, false); } @@ -9293,7 +9293,7 @@ qemuDomainStorageSourceChainAccessRevoke(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src) { - return qemuDomainStorageSourceChainAccessPrepare(driver, vm, src, true); + return qemuDomainStorageSourceAccessModify(driver, vm, src, true); } -- 2.20.1
On Thu, Apr 18, 2019 at 04:42:59PM +0200, Peter Krempa wrote:
The function will be able to deal with non-chains too so drop 'Chain' and also change the suffix to 'Modify' as it's used both for setup and teardown.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Upcoming patches will add a few more flags. Add an enum to collect them so that we don't end up with multiple bools. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b4c59b9a74..6644c1418b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9211,16 +9211,21 @@ qemuDomainDiskGetBackendAlias(virDomainDiskDefPtr disk, } +typedef enum { + /* revoke access to the image instead of allowing it */ + QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE = 1 << 0, +} qemuDomainStorageSourceAccessFlags; + + /** * qemuDomainStorageSourceAccessModify: * @driver: qemu driver struct * @vm: domain object * @src: Source to prepare - * @teardown: Teardown the access to @src instead of adding it to a vm + * @flags: bitwise or of qemuDomainStorageSourceAccessFlags * * Setup the locks, cgroups and security permissions on a disk source and its - * backing chain. If @teardown is true, then the labels and cgroups are removed - * instead. + * backing chain. * * Returns 0 on success and -1 on error. Reports libvirt error. */ @@ -9228,7 +9233,7 @@ static int qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src, - bool teardown) + qemuDomainStorageSourceAccessFlags flags) { VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); const char *srcstr = NULLSTR(src->path); @@ -9236,7 +9241,7 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, virErrorPtr orig_err = NULL; /* just tear down the disk access */ - if (teardown) { + if (flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE) { virErrorPreserveLast(&orig_err); ret = 0; goto rollback_cgroup; @@ -9284,7 +9289,9 @@ qemuDomainStorageSourceChainAccessAllow(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src) { - return qemuDomainStorageSourceAccessModify(driver, vm, src, false); + qemuDomainStorageSourceAccessFlags flags = 0; + + return qemuDomainStorageSourceAccessModify(driver, vm, src, flags); } @@ -9293,7 +9300,9 @@ qemuDomainStorageSourceChainAccessRevoke(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src) { - return qemuDomainStorageSourceAccessModify(driver, vm, src, true); + qemuDomainStorageSourceAccessFlags flags = QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE; + + return qemuDomainStorageSourceAccessModify(driver, vm, src, flags); } -- 2.20.1
On Thu, Apr 18, 2019 at 04:43:00PM +0200, Peter Krempa wrote:
Upcoming patches will add a few more flags. Add an enum to collect them so that we don't end up with multiple bools.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Add a new flag QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN to select whether to work on single image or full chain. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6644c1418b..30acba3c47 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9214,6 +9214,8 @@ qemuDomainDiskGetBackendAlias(virDomainDiskDefPtr disk, typedef enum { /* revoke access to the image instead of allowing it */ QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE = 1 << 0, + /* operate on full backing chain rather than single image */ + QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN = 1 << 1, } qemuDomainStorageSourceAccessFlags; @@ -9239,6 +9241,8 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, const char *srcstr = NULLSTR(src->path); int ret = -1; virErrorPtr orig_err = NULL; + bool chain = flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN; + int rc; /* just tear down the disk access */ if (flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE) { @@ -9253,20 +9257,30 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, if (qemuDomainNamespaceSetupDisk(vm, src) < 0) goto rollback_lock; - if (qemuSecuritySetImageLabel(driver, vm, src, true) < 0) + if (qemuSecuritySetImageLabel(driver, vm, src, chain) < 0) goto rollback_namespace; - if (qemuSetupImageChainCgroup(vm, src) < 0) + if (chain) + rc = qemuSetupImageChainCgroup(vm, src); + else + rc = qemuSetupImageCgroup(vm, src); + + if (rc < 0) goto rollback_label; ret = 0; goto cleanup; rollback_cgroup: - if (qemuTeardownImageChainCgroup(vm, src) < 0) + if (chain) + rc = qemuTeardownImageChainCgroup(vm, src); + else + rc = qemuTeardownImageCgroup(vm, src); + + if (rc < 0) VIR_WARN("Unable to tear down cgroup access on %s", srcstr); rollback_label: - if (qemuSecurityRestoreImageLabel(driver, vm, src, true) < 0) + if (qemuSecurityRestoreImageLabel(driver, vm, src, chain) < 0) VIR_WARN("Unable to restore security label on %s", srcstr); rollback_namespace: @@ -9289,7 +9303,7 @@ qemuDomainStorageSourceChainAccessAllow(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src) { - qemuDomainStorageSourceAccessFlags flags = 0; + qemuDomainStorageSourceAccessFlags flags = QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN; return qemuDomainStorageSourceAccessModify(driver, vm, src, flags); } @@ -9300,7 +9314,8 @@ qemuDomainStorageSourceChainAccessRevoke(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src) { - qemuDomainStorageSourceAccessFlags flags = QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE; + qemuDomainStorageSourceAccessFlags flags = QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE | + QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN; return qemuDomainStorageSourceAccessModify(driver, vm, src, flags); } -- 2.20.1
On Thu, Apr 18, 2019 at 04:43:01PM +0200, Peter Krempa wrote:
Add a new flag QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN to select whether to work on single image or full chain.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use qemuDomainStorageSourceAccessModify instead of the individual calls. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 30acba3c47..f7d1250f09 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9332,18 +9332,9 @@ qemuDomainStorageSourceAccessRevoke(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr elem) { - if (qemuTeardownImageCgroup(vm, elem) < 0) - VIR_WARN("Failed to teardown cgroup for disk path %s", - NULLSTR(elem->path)); + qemuDomainStorageSourceAccessFlags flags = QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE; - if (qemuSecurityRestoreImageLabel(driver, vm, elem, false) < 0) - VIR_WARN("Unable to restore security label on %s", NULLSTR(elem->path)); - - if (qemuDomainNamespaceTeardownDisk(vm, elem) < 0) - VIR_WARN("Unable to remove /dev entry for %s", NULLSTR(elem->path)); - - if (virDomainLockImageDetach(driver->lockManager, vm, elem) < 0) - VIR_WARN("Unable to release lock on %s", NULLSTR(elem->path)); + ignore_value(qemuDomainStorageSourceAccessModify(driver, vm, elem, flags)); } -- 2.20.1
On Thu, Apr 18, 2019 at 04:43:02PM +0200, Peter Krempa wrote:
Use qemuDomainStorageSourceAccessModify instead of the individual calls.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Add a new flag which will set the image as read-only even if the image data allows writing. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f7d1250f09..e8f6ee548a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9216,6 +9216,8 @@ typedef enum { QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE = 1 << 0, /* operate on full backing chain rather than single image */ QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN = 1 << 1, + /* force permissions to read-only when allowing */ + QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_READ_ONLY = 1 << 2, } qemuDomainStorageSourceAccessFlags; @@ -9243,6 +9245,10 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, virErrorPtr orig_err = NULL; bool chain = flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN; int rc; + bool was_readonly = src->readonly; + + if (flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_READ_ONLY) + src->readonly = true; /* just tear down the disk access */ if (flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE) { @@ -9292,6 +9298,7 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, VIR_WARN("Unable to release lock on %s", srcstr); cleanup: + src->readonly = was_readonly; virErrorRestore(&orig_err); return ret; -- 2.20.1
On Thu, Apr 18, 2019 at 04:43:03PM +0200, Peter Krempa wrote:
Add a new flag which will set the image as read-only even if the image data allows writing.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 7 +++++++ 1 file changed, 7 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Rather than jumping to the correct label use a set of booleans to determine which operation needs to be rolled back. This will allow more flexibility when e.g. rollback after a failed operation will not be necessary/desired. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 68 ++++++++++++++++++++++++++++-------------- 1 file changed, 45 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e8f6ee548a..aec094d8f9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9246,6 +9246,10 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, bool chain = flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN; int rc; bool was_readonly = src->readonly; + bool revoke_cgroup = false; + bool revoke_label = false; + bool revoke_namespace = false; + bool revoke_lockspace = false; if (flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_READ_ONLY) src->readonly = true; @@ -9253,18 +9257,28 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, /* just tear down the disk access */ if (flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE) { virErrorPreserveLast(&orig_err); + revoke_cgroup = true; + revoke_label = true; + revoke_namespace = true; + revoke_lockspace = true; ret = 0; - goto rollback_cgroup; + goto revoke; } if (virDomainLockImageAttach(driver->lockManager, cfg->uri, vm, src) < 0) - goto cleanup; + goto revoke; + + revoke_lockspace = true; if (qemuDomainNamespaceSetupDisk(vm, src) < 0) - goto rollback_lock; + goto revoke; + + revoke_namespace = true; if (qemuSecuritySetImageLabel(driver, vm, src, chain) < 0) - goto rollback_namespace; + goto revoke; + + revoke_label = true; if (chain) rc = qemuSetupImageChainCgroup(vm, src); @@ -9272,30 +9286,38 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, rc = qemuSetupImageCgroup(vm, src); if (rc < 0) - goto rollback_label; + goto revoke; + + revoke_cgroup = true; ret = 0; goto cleanup; - rollback_cgroup: - if (chain) - rc = qemuTeardownImageChainCgroup(vm, src); - else - rc = qemuTeardownImageCgroup(vm, src); + revoke: + if (revoke_cgroup) { + if (chain) + rc = qemuTeardownImageChainCgroup(vm, src); + else + rc = qemuTeardownImageCgroup(vm, src); - if (rc < 0) - VIR_WARN("Unable to tear down cgroup access on %s", srcstr); - rollback_label: - if (qemuSecurityRestoreImageLabel(driver, vm, src, chain) < 0) - VIR_WARN("Unable to restore security label on %s", srcstr); - - rollback_namespace: - if (qemuDomainNamespaceTeardownDisk(vm, src) < 0) - VIR_WARN("Unable to remove /dev entry for %s", srcstr); - - rollback_lock: - if (virDomainLockImageDetach(driver->lockManager, vm, src) < 0) - VIR_WARN("Unable to release lock on %s", srcstr); + if (rc < 0) + VIR_WARN("Unable to tear down cgroup access on %s", srcstr); + } + + if (revoke_label) { + if (qemuSecurityRestoreImageLabel(driver, vm, src, chain) < 0) + VIR_WARN("Unable to restore security label on %s", srcstr); + } + + if (revoke_namespace) { + if (qemuDomainNamespaceTeardownDisk(vm, src) < 0) + VIR_WARN("Unable to remove /dev entry for %s", srcstr); + } + + if (revoke_lockspace) { + if (virDomainLockImageDetach(driver->lockManager, vm, src) < 0) + VIR_WARN("Unable to release lock on %s", srcstr); + } cleanup: src->readonly = was_readonly; -- 2.20.1
On Thu, Apr 18, 2019 at 04:43:04PM +0200, Peter Krempa wrote:
Rather than jumping to the correct label use a set of booleans to determine which operation needs to be rolled back. This will allow more flexibility when e.g. rollback after a failed operation will not be necessary/desired.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 68 ++++++++++++++++++++++++++++-------------- 1 file changed, 45 insertions(+), 23 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
In some cases when we need to modify access permissions for a storage source which is already used by the VM we should not revoke all permissions on a failure. Allow this in qemuDomainStorageSourceAccessModify by adding a new flag. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index aec094d8f9..82ebf3c324 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9218,6 +9218,8 @@ typedef enum { QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN = 1 << 1, /* force permissions to read-only when allowing */ QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_READ_ONLY = 1 << 2, + /* don't revoke permissions when modification has failed */ + QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE = 1 << 3, } qemuDomainStorageSourceAccessFlags; @@ -9294,6 +9296,9 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, goto cleanup; revoke: + if (flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE) + goto cleanup; + if (revoke_cgroup) { if (chain) rc = qemuTeardownImageChainCgroup(vm, src); -- 2.20.1
On Thu, Apr 18, 2019 at 04:43:05PM +0200, Peter Krempa wrote:
In some cases when we need to modify access permissions for a storage source which is already used by the VM we should not revoke all permissions on a failure. Allow this in qemuDomainStorageSourceAccessModify by adding a new flag.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 5 +++++ 1 file changed, 5 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Some operations e.g. namespace setup are not necessary when modifying access to a file which the VM can already access. Add a flag which allows to skip them. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 82ebf3c324..75d0b34e42 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9220,6 +9220,8 @@ typedef enum { QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_READ_ONLY = 1 << 2, /* don't revoke permissions when modification has failed */ QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE = 1 << 3, + /* VM already has access to the source and we are just modifying it */ + QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS = 1 << 4, } qemuDomainStorageSourceAccessFlags; @@ -9272,10 +9274,13 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver, revoke_lockspace = true; - if (qemuDomainNamespaceSetupDisk(vm, src) < 0) - goto revoke; + /* When modifying access of existing @src namespace does not need update */ + if (!(flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS)) { + if (qemuDomainNamespaceSetupDisk(vm, src) < 0) + goto revoke; - revoke_namespace = true; + revoke_namespace = true; + } if (qemuSecuritySetImageLabel(driver, vm, src, chain) < 0) goto revoke; -- 2.20.1
On Thu, Apr 18, 2019 at 04:43:06PM +0200, Peter Krempa wrote:
Some operations e.g. namespace setup are not necessary when modifying access to a file which the VM can already access. Add a flag which allows to skip them.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use qemuDomainStorageSourceAccessModify with correct flags to do the job. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 30 ++++++------------------------ 1 file changed, 6 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 75d0b34e42..c2b96b825e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9399,33 +9399,15 @@ qemuDomainStorageSourceAccessAllow(virQEMUDriverPtr driver, bool readonly, bool newSource) { - bool was_readonly = elem->readonly; - virQEMUDriverConfigPtr cfg = NULL; - int ret = -1; - - cfg = virQEMUDriverGetConfig(driver); - - elem->readonly = readonly; - - if (virDomainLockImageAttach(driver->lockManager, cfg->uri, vm, elem) < 0) - goto cleanup; - - if (newSource && - qemuDomainNamespaceSetupDisk(vm, elem) < 0) - goto cleanup; - - if (qemuSetupImageCgroup(vm, elem) < 0) - goto cleanup; + qemuDomainStorageSourceAccessFlags flags = QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE; - if (qemuSecuritySetImageLabel(driver, vm, elem, false) < 0) - goto cleanup; + if (readonly) + flags &= QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_READ_ONLY; - ret = 0; + if (!newSource) + flags &= QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS; - cleanup: - elem->readonly = was_readonly; - virObjectUnref(cfg); - return ret; + return qemuDomainStorageSourceAccessModify(driver, vm, elem, flags); } -- 2.20.1
On Thu, Apr 18, 2019 at 04:43:07PM +0200, Peter Krempa wrote:
Use qemuDomainStorageSourceAccessModify with correct flags to do the job.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 30 ++++++------------------------ 1 file changed, 6 insertions(+), 24 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 4/18/19 9:42 AM, Peter Krempa wrote:
With new blockjob handling we'll need to modify permissions for chains and individual images. The individual image code was universally accessible but the chain setting code reimplemented it mostly only in qemu_hotplug.h.
Refactor the handling by moving the code to qemu_domain.c and making it universal.
Oh fun - competes with my work to get incremental backup in. But from the subject lines alone, it seems reasonable; I can see how hard it is to rebase my code on top of yours. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Thu, Apr 18, 2019 at 14:30:04 -0500, Eric Blake wrote:
On 4/18/19 9:42 AM, Peter Krempa wrote:
With new blockjob handling we'll need to modify permissions for chains and individual images. The individual image code was universally accessible but the chain setting code reimplemented it mostly only in qemu_hotplug.h.
Refactor the handling by moving the code to qemu_domain.c and making it universal.
Oh fun - competes with my work to get incremental backup in. But from the subject lines alone, it seems reasonable; I can see how hard it is to rebase my code on top of yours.
These are the necessary changes which are needed in: backup: Wire up qemu full pull backup commands over QMP diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c9f1996b2a..d3307bfc4f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17796,7 +17796,7 @@ qemuDomainBackupDiskCleanup(virQEMUDriverPtr driver, virDomainObjPtr vm, ret = -1; } if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_LABEL) - qemuDomainDiskChainElementRevoke(driver, vm, disk->store); + qemuDomainStorageSourceAccessRevoke(driver, vm, disk->store); if ((!push || !completed) && disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_CREATED && disk->store->detected && unlink(disk->store->path) < 0) { @@ -17997,8 +17997,8 @@ qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml, } disk->state = VIR_DOMAIN_BACKUP_DISK_STATE_CREATED; } - if (qemuDomainDiskChainElementPrepare(driver, vm, disk->store, false, - true) < 0) + if (qemuDomainStorageSourceAccessAllow(driver, vm, disk->store, false, + true) < 0) goto endmon; disk->state = VIR_DOMAIN_BACKUP_DISK_STATE_LABEL; if (disk->store->detected) {
On Thu, Apr 18, 2019 at 16:42:55 +0200, Peter Krempa wrote:
With new blockjob handling we'll need to modify permissions for chains and individual images. The individual image code was universally accessible but the chain setting code reimplemented it mostly only in qemu_hotplug.h.
Refactor the handling by moving the code to qemu_domain.c and making it universal.
Ping?
participants (3)
-
Eric Blake -
Ján Tomko -
Peter Krempa