Re: [libvirt] [PATCHv2 17/16] blockjob: refactor qemu disk chain permission grants

On 10/17/2012 06:09 PM, Laine Stump wrote: > On 10/16/2012 06:03 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. >> >> + 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(driver, 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); > This feels like a bit of a hackish way to get around an inadequate > security driver (and lock and cgroup?) API by hijacking what's > available. But then I haven't looked at just how deep the changes would > need to be to make it work properly, so I won't dismiss it out of hand. > It seems like something that should require a promise of later cleanup > though :-) This is just code motion; this code was previously here: >> @@ -10881,13 +10917,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(driver, 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); Basically, after granting rights, if you want to then revoke rights because some later step failed, you want to log if the revoke fails, but you're already on the failure path, so there's nothing more we can do than logging. I'm not sure what you are proposing would be a more adequate security manager API. Or is that you are really complaining about my mixing of the existing virDomainDiskDefPtr (which stores the read-only and read-write labels to be used by the security manager for all files in that disk's chain) with the hack of temporarily modifying the disk to only point to the one file that I'm adding or removing from the chain?