[PATCH 2/2] virDomainLockManagerAddImage: Recursively lock backing chain

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

https://bugzilla.redhat.com/show_bug.cgi?id=1192399

It's known fact for a while now that we should not only lock the
top level layers of backing chain but the rest of it too. And
well known too that we are not doing that. Well, up until this
commit. The reason is that while one guest can have for instance
the following backing chain: A (top) -> B -> C (bottom), the
other can be started just over B or C. But libvirt should prevent
that as it will almost certainly mangle the backing chain for the
former guest. On the other hand, we want to allow guest with the
following backing chain: D (top) -> B -> C (bottom), because in
both cases B and C are there just for reading. In order to
achieve that we must lock the rest of backing chain with
VIR_LOCK_MANAGER_RESOURCE_SHARED flag.

Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
 src/locking/domain_lock.c | 53 +++++++++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c
index 705b132..faf73f2 100644
--- a/src/locking/domain_lock.c
+++ b/src/locking/domain_lock.c
@@ -69,35 +69,48 @@ static int virDomainLockManagerAddLease(virLockManagerPtr lock,
 
 
 static int virDomainLockManagerAddImage(virLockManagerPtr lock,
-                                        virStorageSourcePtr src)
+                                        virStorageSourcePtr disk)
 {
     unsigned int diskFlags = 0;
-    int type = virStorageSourceGetActualType(src);
-
-    if (!src->path)
-        return 0;
-
-    if (!(type == VIR_STORAGE_TYPE_BLOCK ||
-          type == VIR_STORAGE_TYPE_FILE ||
-          type == VIR_STORAGE_TYPE_DIR))
-        return 0;
+    virStorageSourcePtr src = disk;
+    int ret = -1;
 
+    /* The top layer of backing chain should have the correct flags set.
+     * The rest should be locked with VIR_LOCK_MANAGER_RESOURCE_SHARED
+     * for it can be shared among other domains. */
     if (src->readonly)
         diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY;
     if (src->shared)
         diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED;
 
-    VIR_DEBUG("Add disk %s", src->path);
-    if (virLockManagerAddResource(lock,
-                                  VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK,
-                                  src->path,
-                                  0,
-                                  NULL,
-                                  diskFlags) < 0) {
-        VIR_DEBUG("Failed add disk %s", src->path);
-        return -1;
+    while (src)  {
+        if (!virStorageSourceIsLocalStorage(src))
+            break;
+
+        if (!src->path)
+            break;
+
+        VIR_DEBUG("Add disk %s", src->path);
+        if (virLockManagerAddResource(lock,
+                                      VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK,
+                                      src->path,
+                                      0,
+                                      NULL,
+                                      diskFlags) < 0) {
+            VIR_DEBUG("Failed add disk %s", src->path);
+            goto cleanup;
+        }
+
+        /* Now that we've added the first disk (top layer of backing chain)
+         * into the lock manager, let's traverse the rest of the backing chain
+         * and lock each layer for RO. This should prevent accidentally
+         * starting a domain with RW disk from the middle of the chain. */
+        diskFlags = VIR_LOCK_MANAGER_RESOURCE_SHARED;
+        src = src->backingStore;
     }
-    return 0;
+    ret = 0;
+ cleanup:
+    return ret;
 }
 
 
-- 
2.4.6

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