Re: [libvirt] [PATCHv3 18/19] blockjob: refactor qemu disk chain permission grants

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);