blockjob: refactor qemu disk chain permission grants

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.

It might be nice to clean things up in a future patch by adding
new functions to the lock manager, cgroup manager, and security
manager that takes a single file name and applies context of a
disk to that file, rather than the current semantics of applying
context to the entire chain already associated to a disk.  That
way, we could avoid the games this patch plays of temporarily
swapping out the disk->src and related fields of the disk.  But
that would involve more code changes, so this patch really is
the smallest hack for doing the necessary work; besides, this
patch is more or less code motion (the hack was already employed
by the snapshot creation code, we are just making it reusable).

* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateSingleDiskActive)
(qemuDomainSnapshotUndoSingleDiskActive): Refactor labeling hacks...
(qemuDomainPrepareDiskChainElement): ...into new function.

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 64ca585a8f3cc4b830c1403163348674bf12e888..794c24ff3dd72e7df7fd30ced69669666864092b 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10454,6 +10454,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)
 {
@@ -10763,8 +10823,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) {
@@ -10791,10 +10849,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
@@ -10804,27 +10858,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);
@@ -10848,10 +10888,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);
@@ -10883,13 +10919,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);