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

[Eric Blake]

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?

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121017/7aa1e7f9/attachment-0001.sig>