Changeset 38499 in vbox for trunk/src/VBox/Main/src-server

Timestamp:

Aug 19, 2011 12:02:55 PM (13 years ago)

Author:

vboxsync

Message:

Main/Machine+Medium: fix medium registry association when attaching a device or mounting a medium, when there is a chain of parent images which are not associated with any media registry, including the necessary locking adjustments

Location:

trunk/src/VBox/Main/src-server

Files:

• MachineImpl.cpp (modified) (6 diffs)
• MediumImpl.cpp (modified) (7 diffs)

trunk/src/VBox/Main/src-server/MachineImpl.cpp

@@ -3733 +3733 @@
             // creating the diff image for the immutable, and the parent is not yet registered);
             // put the parent in the machine registry then
+            mediumLock.release();
             addMediumToRegistry(medium, llRegistriesThatNeedSaving, &uuidRegistryParent);
+            mediumLock.acquire();
         }
         rc = diff->init(mParent,
@@ -3816 +3818 @@
         if (FAILED(rc)) return rc;
 
+        mediumLock.release();
         addMediumToRegistry(medium,
                             llRegistriesThatNeedSaving,
                             NULL /* Guid *puuid */);
+        mediumLock.acquire();
     }
 
@@ -4190 +4194 @@
             pMedium->addBackReference(mData->mUuid);
 
+            mediumLock.release();
             addMediumToRegistry(pMedium, llRegistriesThatNeedSaving, NULL /* Guid *puuid */ );
+            mediumLock.acquire();
         }
 
@@ -9200 +9206 @@
  * registry.
  *
- * Caller must hold machine read lock!
+ * Caller must hold machine read lock and at least media tree read lock!
+ * Caller must NOT hold any medium locks.
  *
  * @param pMedium
@@ -9210 +9217 @@
                                   Guid *puuid)
 {
+    ComObjPtr<Medium> pBase = pMedium->getBase();
+    /* Paranoia checks: do not hold medium locks. */
+    AssertReturnVoid(!pMedium->isWriteLockOnCurrentThread());
+    AssertReturnVoid(!pBase->isWriteLockOnCurrentThread());
+
     // decide which medium registry to use now that the medium is attached:
     Guid uuid;
@@ -9218 +9230 @@
         uuid = mParent->getGlobalRegistryId(); // VirtualBox global registry UUID
 
-    AutoCaller autoCaller(pMedium);
-    if (FAILED(autoCaller.rc())) return;
-    AutoWriteLock alock(pMedium COMMA_LOCKVAL_SRC_POS);
-
+    bool fAdd = false;
     if (pMedium->addRegistry(uuid, false /* fRecurse */))
+    {
         // registry actually changed:
         VirtualBox::addGuidToListUniquely(llRegistriesThatNeedSaving, uuid);
+        fAdd = true;
+    }
+
+    /* For more complex hard disk structures it can happen that the base
+     * medium isn't yet associated with any medium registry. Do that now. */
+    if (pMedium != pBase)
+    {
+        if (   pBase->addRegistry(uuid, true /* fRecurse */)
+            && !fAdd)
+        {
+            VirtualBox::addGuidToListUniquely(llRegistriesThatNeedSaving, uuid);
+            fAdd = true;
+        }
+    }
 
     if (puuid)

trunk/src/VBox/Main/src-server/MediumImpl.cpp

@@ -3122 +3122 @@
  * See getFirstRegistryMachineId() for details.
  *
- * Must have caller + locking!
- *
- * If fRecurse == true, then additionally the media tree lock must be held for reading.
+ * If fRecurse == true, then the media tree lock must be held for reading.
  *
  * @param id
@@ -3132 +3130 @@
 bool Medium::addRegistry(const Guid& id, bool fRecurse)
 {
+    AutoCaller autoCaller(this);
+    if (FAILED(autoCaller.rc()))
+        return false;
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    bool fAdd = true;
+
     // hard disks cannot be in more than one registry
-    if (    m->devType == DeviceType_HardDisk
-         && m->llRegistryIDs.size() > 0
-       )
-        return false;
+    if (   m->devType == DeviceType_HardDisk
+        && m->llRegistryIDs.size() > 0)
+        fAdd = false;
 
     // no need to add the UUID twice
-    for (GuidList::const_iterator it = m->llRegistryIDs.begin();
-         it != m->llRegistryIDs.end();
-         ++it)
-    {
-        if ((*it) == id)
-            return false;
-    }
-
-    addRegistryImpl(id, fRecurse);
-
-    return true;
-}
-
-/**
- * Private implementation for addRegistry() so we can recurse more efficiently.
- * @param id
- * @param fRecurse
- */
-void Medium::addRegistryImpl(const Guid& id, bool fRecurse)
-{
-    m->llRegistryIDs.push_back(id);
+    if (fAdd)
+    {
+        for (GuidList::const_iterator it = m->llRegistryIDs.begin();
+             it != m->llRegistryIDs.end();
+             ++it)
+        {
+            if ((*it) == id)
+            {
+                fAdd = false;
+                break;
+            }
+        }
+    }
+
+    if (fAdd)
+        m->llRegistryIDs.push_back(id);
 
     if (fRecurse)
     {
-        for (MediaList::iterator it = m->llChildren.begin();
-             it != m->llChildren.end();
+        // Get private list of children and release medium lock straight away.
+        MediaList llChildren(m->llChildren);
+        alock.release();
+
+        for (MediaList::iterator it = llChildren.begin();
+             it != llChildren.end();
              ++it)
         {
             Medium *pChild = *it;
-            // recurse!
-            pChild->addRegistryImpl(id, fRecurse);
-        }
-    }
+            fAdd |= pChild->addRegistry(id, true);
+        }
+    }
+
+    return fAdd;
 }
 
@@ -3178 +3182 @@
  * if found or false if not.
  *
- * Must have caller + locking!
- *
- * If fRecurse == true, then additionally the media tree lock must be held for reading.
+ * If fRecurse == true, then the media tree lock must be held for reading.
  *
  * @param id
@@ -3188 +3190 @@
 bool Medium::removeRegistry(const Guid& id, bool fRecurse)
 {
+    AutoCaller autoCaller(this);
+    if (FAILED(autoCaller.rc()))
+        return false;
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    bool fRemove = false;
+
     for (GuidList::iterator it = m->llRegistryIDs.begin();
          it != m->llRegistryIDs.end();
@@ -3195 +3204 @@
         {
             m->llRegistryIDs.erase(it);
-
-            if (fRecurse)
-            {
-                for (MediaList::iterator it2 = m->llChildren.begin();
-                     it2 != m->llChildren.end();
-                     ++it2)
-                {
-                    Medium *pChild = *it2;
-                    pChild->removeRegistry(id, true);
-                }
-            }
-
-            return true;
-        }
-    }
-
-    return false;
+            fRemove = true;
+            break;
+        }
+    }
+
+    if (fRecurse)
+    {
+        // Get private list of children and release medium lock straight away.
+        MediaList llChildren(m->llChildren);
+        alock.release();
+
+        for (MediaList::iterator it = llChildren.begin();
+             it != llChildren.end();
+             ++it)
+        {
+            Medium *pChild = *it;
+            fRemove |= pChild->removeRegistry(id, true);
+        }
+    }
+
+    return fRemove;
 }
 
@@ -3217 +3230 @@
  * Returns true if id is in the list of media registries for this medium.
  *
- * Must have caller + locking!
+ * Must have caller + read locking!
  *
  * @param id
@@ -3224 +3237 @@
 bool Medium::isInRegistry(const Guid& id)
 {
-    AutoCaller autoCaller(this);
-    if (FAILED(autoCaller.rc())) return false;
-
-    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-
     for (GuidList::const_iterator it = m->llRegistryIDs.begin();
          it != m->llRegistryIDs.end();