[libvirt] [PATCHv3 18/19] blockjob: refactor qemu disk chain permission grants
Laine Stump
laine at laine.org
Thu Oct 18 18:50:51 UTC 2012
On 10/17/2012 06:30 PM, Eric Blake wrote:
> 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);
ACK, with qualifying remarks from the review of the previous version of
the patch (PATCHv2 17/16)
More information about the libvir-list
mailing list