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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

* src/qemu/qemu_driver.c (qemuDomainPrepareDiskChainElement): New
function.
(qemuDomainSnapshotCreateSingleDiskActive)
(qemuDomainSnapshotUndoSingleDiskActive): Use it.
---
 src/qemu/qemu_driver.c | 101 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 66 insertions(+), 35 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 04b28a1..59c475e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10452,6 +10452,65 @@ 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.  */
+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->chain;
+    bool origreadonly = disk->readonly;
+    int ret = -1;
+
+    disk->src = file;
+    disk->format = VIR_STORAGE_FILE_RAW;
+    disk->chain = 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(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);
+    } else if (virDomainLockDiskAttach(driver->lockManager, driver->uri,
+                                       vm, disk) < 0 ||
+               (cgroup && qemuSetupDiskCgroup(driver, vm, cgroup, disk) < 0) ||
+               virSecurityManagerSetImageLabel(driver->securityManager,
+                                               vm->def, disk) < 0) {
+        goto cleanup;
+    }
+
+    ret = 0;
+
+cleanup:
+    disk->src = origsrc;
+    disk->format = origformat;
+    disk->chain = origchain;
+    disk->readonly = origreadonly;
+    return ret;
+}
+
+
 static bool
 qemuDomainSnapshotIsAllowed(virDomainObjPtr vm)
 {
@@ -10742,6 +10801,7 @@ cleanup:
     return ret;
 }

+
 /* The domain is expected to hold monitor lock.  */
 static int
 qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver,
@@ -10761,8 +10821,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) {
@@ -10789,10 +10847,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->chain if
      * successful, so we nuke the existing chain so that future
      * commands will recompute it.  Better would be storing the chain
@@ -10802,27 +10856,13 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver,
     virStorageFileFreeMetadata(disk->chain);
     disk->chain = NULL;

-    if (virDomainLockDiskAttach(driver->lockManager, driver->uri,
-                                vm, disk) < 0)
-        goto cleanup;
-    if (cgroup && qemuSetupDiskCgroup(driver, 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(driver, 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);
@@ -10846,10 +10886,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);
@@ -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);
     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);
-- 
1.7.11.7

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]