On 10/17/2012 09:42 PM, Eric Blake wrote:
On 10/17/2012 06:09 PM, Laine Stump 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
On 10/16/2012 06:03 PM, Eric Blake wrote: 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?
Right, the 2nd thing. Rather than temporarily poking different values into what is essentially an item in a database in order to trick the API into thinking that is reality, it would be cleaner if the API was flexible enough that you could just send the various pieces in as args. But again, I haven't investigated just how much work this would be, and what you have works, so I'm not going to insist on it being done right away (I just can't make myself let it go without mention, though :-)