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

[Laine Stump]

On 10/17/2012 09:42 PM, Eric Blake wrote:
> 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?

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