5:30 p.m.
Previously, snapshot code did its own permission granting (lock
manager, cgroup device controller, and security manager labeling)
inline. But now that we are adding block-commit and block-copy
which also have to change permissions, it's better to reuse
common code for the task. While snapshot should fall back to
no access if read-write access failed, block-commit will want to
fall back to read-only access. The common code doesn't know
whether failure to grant read-write access should revert to no
access (snapshot, block-copy) or read-only access (block-commit).
This code can also be used to revoke access to unused files after
block-pull.
* src/qemu/qemu_driver.c (qemuDomainPrepareDiskChainElement): New
function.
(qemuDomainSnapshotCreateSingleDiskActive)
(qemuDomainSnapshotUndoSingleDiskActive): Use it.
---
v3: rebase to earlier patches in series
src/qemu/qemu_driver.c | 101 ++++++++++++++++++++++++++++++++-----------------
1 file changed, 66 insertions(+), 35 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 73804fe..072ec17 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10453,6 +10453,66 @@ cleanup:
return ret;
}
+typedef enum {
+ VIR_DISK_CHAIN_NO_ACCESS,
+ VIR_DISK_CHAIN_READ_ONLY,
+ VIR_DISK_CHAIN_READ_WRITE,
+} qemuDomainDiskChainMode;
+
+/* Several operations end up adding or removing a single element of a
+ * disk backing file chain; this helper function ensures that the lock
+ * manager, cgroup device controller, and security manager labelling
+ * are all aware of each new file before it is added to a chain, and
+ * can revoke access to a file no longer needed in a chain. */
+static int
+qemuDomainPrepareDiskChainElement(struct qemud_driver *driver,
+ virDomainObjPtr vm,
+ virCgroupPtr cgroup,
+ virDomainDiskDefPtr disk,
+ char *file,
+ qemuDomainDiskChainMode mode)
+{
+ /* The easiest way to label a single file with the same
+ * permissions it would have as if part of the disk chain is to
+ * temporarily modify the disk in place. */
+ char *origsrc = disk->src;
+ int origformat = disk->format;
+ virStorageFileMetadataPtr origchain = disk->backingChain;
+ bool origreadonly = disk->readonly;
+ int ret = -1;
+
+ disk->src = file;
+ disk->format = VIR_STORAGE_FILE_RAW;
+ disk->backingChain = NULL;
+ disk->readonly = mode == VIR_DISK_CHAIN_READ_ONLY;
+
+ if (mode == VIR_DISK_CHAIN_NO_ACCESS) {
+ if (virSecurityManagerRestoreImageLabel(driver->securityManager,
+ vm->def, disk) < 0)
+ VIR_WARN("Unable to restore security label on %s", disk->src);
+ if (cgroup && qemuTeardownDiskCgroup(vm, cgroup, disk) < 0)
+ VIR_WARN("Failed to teardown cgroup for disk path %s",
disk->src);
+ if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0)
+ VIR_WARN("Unable to release lock on %s", disk->src);
+ } else if (virDomainLockDiskAttach(driver->lockManager, driver->uri,
+ vm, disk) < 0 ||
+ (cgroup && qemuSetupDiskCgroup(vm, cgroup, disk) < 0) ||
+ virSecurityManagerSetImageLabel(driver->securityManager,
+ vm->def, disk) < 0) {
+ goto cleanup;
+ }
+
+ ret = 0;
+
+cleanup:
+ disk->src = origsrc;
+ disk->format = origformat;
+ disk->backingChain = origchain;
+ disk->readonly = origreadonly;
+ return ret;
+}
+
+
static bool
qemuDomainSnapshotIsAllowed(virDomainObjPtr vm)
{
@@ -10762,8 +10822,6 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver
*driver,
char *persistSource = NULL;
int ret = -1;
int fd = -1;
- char *origsrc = NULL;
- int origdriver;
bool need_unlink = false;
if (snap->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
@@ -10790,10 +10848,6 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver
*driver,
VIR_FORCE_CLOSE(fd);
}
- origsrc = disk->src;
- disk->src = source;
- origdriver = disk->format;
- disk->format = VIR_STORAGE_FILE_RAW; /* Don't want to probe backing files */
/* XXX Here, we know we are about to alter disk->backingChain if
* successful, so we nuke the existing chain so that future
* commands will recompute it. Better would be storing the chain
@@ -10803,27 +10857,13 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver
*driver,
virStorageFileFreeMetadata(disk->backingChain);
disk->backingChain = NULL;
- if (virDomainLockDiskAttach(driver->lockManager, driver->uri,
- vm, disk) < 0)
- goto cleanup;
- if (cgroup && qemuSetupDiskCgroup(vm, cgroup, disk) < 0) {
- if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0)
- VIR_WARN("Unable to release lock on %s", source);
- goto cleanup;
- }
- if (virSecurityManagerSetImageLabel(driver->securityManager, vm->def,
- disk) < 0) {
- if (cgroup && qemuTeardownDiskCgroup(vm, cgroup, disk) < 0)
- VIR_WARN("Failed to teardown cgroup for disk path %s", source);
- if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0)
- VIR_WARN("Unable to release lock on %s", source);
+ if (qemuDomainPrepareDiskChainElement(driver, vm, cgroup, disk, source,
+ VIR_DISK_CHAIN_READ_WRITE) < 0) {
+ qemuDomainPrepareDiskChainElement(driver, vm, cgroup, disk, source,
+ VIR_DISK_CHAIN_NO_ACCESS);
goto cleanup;
}
- disk->src = origsrc;
- origsrc = NULL;
- disk->format = origdriver;
-
/* create the actual snapshot */
if (snap->format)
formatStr = virStorageFileFormatTypeToString(snap->format);
@@ -10847,10 +10887,6 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver
*driver,
}
cleanup:
- if (origsrc) {
- disk->src = origsrc;
- disk->format = origdriver;
- }
if (need_unlink && unlink(source))
VIR_WARN("unable to unlink just-created %s", source);
VIR_FREE(device);
@@ -10882,13 +10918,8 @@ qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver
*driver,
goto cleanup;
}
- if (virSecurityManagerRestoreImageLabel(driver->securityManager,
- vm->def, disk) < 0)
- VIR_WARN("Unable to restore security label on %s", disk->src);
- if (cgroup && qemuTeardownDiskCgroup(vm, cgroup, disk) < 0)
- VIR_WARN("Failed to teardown cgroup for disk path %s", disk->src);
- if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0)
- VIR_WARN("Unable to release lock on %s", disk->src);
+ qemuDomainPrepareDiskChainElement(driver, vm, cgroup, disk, origdisk->src,
+ VIR_DISK_CHAIN_NO_ACCESS);
if (need_unlink && stat(disk->src, &st) == 0 &&
S_ISREG(st.st_mode) && unlink(disk->src) < 0)
VIR_WARN("Unable to remove just-created %s", disk->src);
--
1.7.11.7