Changeset 40257 in vbox

Timestamp:

Feb 27, 2012 9:25:12 AM (13 years ago)

Author:

vboxsync

Message:

Main/Medium: rework locking scheme to solve lock order violations and long GUI start up time caused by too much locking
Main/all: Remove the enter and leave methods from write locks, they cause hard to find locking problems. Better solve them explicitly.

Location:

trunk

Files:

• include/VBox/com/AutoLock.h (modified) (3 diffs)
• src/VBox/Main/glue/AutoLock.cpp (modified) (8 diffs)
• src/VBox/Main/include/MediumImpl.h (modified) (1 diff)
• src/VBox/Main/include/VirtualBoxImpl.h (modified) (3 diffs)
• src/VBox/Main/src-all/ProgressImpl.cpp (modified) (5 diffs)
• src/VBox/Main/src-all/VirtualBoxBase.cpp (modified) (5 diffs)
• src/VBox/Main/src-client/ConsoleImpl.cpp (modified) (51 diffs)
• src/VBox/Main/src-client/ConsoleImplTeleporter.cpp (modified) (3 diffs)
• src/VBox/Main/src-client/DisplayImpl.cpp (modified) (14 diffs)
• src/VBox/Main/src-client/SessionImpl.cpp (modified) (3 diffs)
• src/VBox/Main/src-server/BandwidthControlImpl.cpp (modified) (2 diffs)
• src/VBox/Main/src-server/BandwidthGroupImpl.cpp (modified) (2 diffs)
• src/VBox/Main/src-server/HostUSBDeviceImpl.cpp (modified) (7 diffs)
• src/VBox/Main/src-server/MachineImpl.cpp (modified) (36 diffs)
• src/VBox/Main/src-server/MachineImplCloneVM.cpp (modified) (5 diffs)
• src/VBox/Main/src-server/MediumImpl.cpp (modified) (81 diffs)
• src/VBox/Main/src-server/SnapshotImpl.cpp (modified) (27 diffs)
• src/VBox/Main/src-server/VirtualBoxImpl.cpp (modified) (21 diffs)

trunk/include/VBox/com/AutoLock.h

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2006-2011 Oracle Corporation
+ * Copyright (C) 2006-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -401 +401 @@
  * which derive from this.
  *
- * In addition to utility methods for subclasses, this implements the public
- * leave/enter methods, which are common to all
- * write locks.
+ * It has some utility methods for subclasses.
  */
 class AutoWriteLockBase : public AutoLockBase
@@ -427 +425 @@
     virtual void callLockImpl(LockHandle &l);
     virtual void callUnlockImpl(LockHandle &l);
-
-public:
-    void leave();
-    void enter();
 };
 

trunk/src/VBox/Main/glue/AutoLock.cpp

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2006-2011 Oracle Corporation
+ * Copyright (C) 2006-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -71 +71 @@
         { LOCKCLASS_MACHINEOBJECT,      "5-MACHINEOBJECT" },
         { LOCKCLASS_SNAPSHOTOBJECT,     "6-SNAPSHOTOBJECT" },
-        { LOCKCLASS_LISTOFMEDIA,        "7-LISTOFMEDIA" },
-        { LOCKCLASS_LISTOFOTHEROBJECTS, "8-LISTOFOTHEROBJECTS" },
-        { LOCKCLASS_MEDIUMQUERY,        "9-MEDIUMQUERY" },
+        { LOCKCLASS_MEDIUMQUERY,        "7-MEDIUMQUERY" },
+        { LOCKCLASS_LISTOFMEDIA,        "8-LISTOFMEDIA" },
+        { LOCKCLASS_LISTOFOTHEROBJECTS, "9-LISTOFOTHEROBJECTS" },
         { LOCKCLASS_OTHEROBJECT,        "10-OTHEROBJECT" },
         { LOCKCLASS_USBLIST,            "11-USBLIST" },
@@ -303 +303 @@
 
 typedef std::vector<LockHandle*> HandlesVector;
-typedef std::vector<uint32_t> CountsVector;
 
 struct AutoLockBase::Data
@@ -315 +314 @@
         )
         : fIsLocked(false),
-          aHandles(cHandles),       // size of array
-          acUnlockedInLeave(cHandles)
+          aHandles(cHandles)        // size of array
 #ifdef VBOX_WITH_MAIN_LOCK_VALIDATION
           , pcszFile(pcszFile_),
@@ -324 +322 @@
     {
         for (uint32_t i = 0; i < cHandles; ++i)
-        {
-            acUnlockedInLeave[i] = 0;
             aHandles[i] = NULL;
-        }
     }
 
@@ -335 +330 @@
                                         // and AutoReadLock, there will only be one item on the list; with the
                                         // AutoMulti* derivatives, there will be multiple
-    CountsVector    acUnlockedInLeave;  // for each lock handle, how many times the handle was unlocked in leave(); otherwise 0
 
 #ifdef VBOX_WITH_MAIN_LOCK_VALIDATION
@@ -415 +409 @@
  * Destructor implementation that can also be called explicitly, if required.
  * Restores the exact state before the AutoLock was created; that is, unlocks
- * all contained semaphores and might actually lock them again if leave()
- * was called during the AutoLock's lifetime.
+ * all contained semaphores.
  */
 void AutoLockBase::cleanup()
 {
-    bool fAnyUnlockedInLeave = false;
-
-    uint32_t i = 0;
-    for (HandlesVector::iterator it = m->aHandles.begin();
-         it != m->aHandles.end();
-         ++it)
-    {
-        LockHandle *pHandle = *it;
-        if (pHandle)
-        {
-            if (m->acUnlockedInLeave[i])
-            {
-                // there was a leave() before the destruction: then restore the
-                // lock level that might have been set by locks other than our own
-                if (m->fIsLocked)
-                {
-                    --m->acUnlockedInLeave[i];
-                    fAnyUnlockedInLeave = true;
-                }
-                for (; m->acUnlockedInLeave[i]; --m->acUnlockedInLeave[i])
-                    callLockImpl(*pHandle);
-            }
-        }
-        ++i;
-    }
-
-    if (m->fIsLocked && !fAnyUnlockedInLeave)
+    if (m->fIsLocked)
         callUnlockOnAllHandles();
 }
@@ -553 +520 @@
 {
     l.unlockWrite();
-}
-
-/**
- * Causes the current thread to completely release the write lock to make
- * the managed semaphore immediately available for locking by other threads.
- *
- * This implies that all nested write locks on the semaphore will be
- * released, even those that were acquired through the calls to #lock()
- * methods of all other AutoWriteLock/AutoReadLock instances managing the
- * <b>same</b> read/write semaphore.
- *
- * After calling this method, the only method you are allowed to call is
- * #enter(). It will acquire the write lock again and restore the same
- * level of nesting as it had before calling #leave().
- *
- * If this instance is destroyed without calling #enter(), the destructor
- * will try to restore the write lock level that existed when #leave() was
- * called minus the number of nested #lock() calls made on this instance
- * itself. This is done to preserve lock levels of other
- * AutoWriteLock/AutoReadLock instances managing the same semaphore (if
- * any). Tiis also means that the destructor may indefinitely block if a
- * write or a read lock is owned by some other thread by that time.
- */
-void AutoWriteLockBase::leave()
-{
-    AssertMsg(m->fIsLocked, ("m->fIsLocked is false, cannot leave()!"));
-
-    // unlock in reverse order!
-    uint32_t i = m->aHandles.size();
-    for (HandlesVector::reverse_iterator it = m->aHandles.rbegin();
-         it != m->aHandles.rend();
-         ++it)
-    {
-        --i;            // array index is zero based, decrement with every loop since we iterate backwards
-        LockHandle *pHandle = *it;
-        if (pHandle)
-        {
-            AssertMsg(m->acUnlockedInLeave[i] == 0, ("m->cUnlockedInLeave[%d] is %d, must be 0! Called leave() twice?", i, m->acUnlockedInLeave[i]));
-            m->acUnlockedInLeave[i] = pHandle->writeLockLevel();
-            AssertMsg(m->acUnlockedInLeave[i] >= 1, ("m->cUnlockedInLeave[%d] is %d, must be >=1!", i, m->acUnlockedInLeave[i]));
-
-            for (uint32_t left = m->acUnlockedInLeave[i];
-                 left;
-                 --left)
-                callUnlockImpl(*pHandle);
-        }
-    }
-}
-
-/**
- * Causes the current thread to restore the write lock level after the
- * #leave() call. This call will indefinitely block if another thread has
- * successfully acquired a write or a read lock on the same semaphore in
- * between.
- */
-void AutoWriteLockBase::enter()
-{
-    AssertMsg(m->fIsLocked, ("m->fIsLocked is false, cannot enter()!"));
-
-    uint32_t i = 0;
-    for (HandlesVector::iterator it = m->aHandles.begin();
-         it != m->aHandles.end();
-         ++it)
-    {
-        LockHandle *pHandle = *it;
-        if (pHandle)
-        {
-            AssertMsg(m->acUnlockedInLeave[i] != 0, ("m->cUnlockedInLeave[%d] is 0! enter() without leave()?", i));
-
-            for (; m->acUnlockedInLeave[i]; --m->acUnlockedInLeave[i])
-                callLockImpl(*pHandle);
-        }
-        ++i;
-    }
 }
 

trunk/src/VBox/Main/include/MediumImpl.h

@@ -269 +269 @@
 
     HRESULT queryInfo(bool fSetImageId, bool fSetParentId);
+    HRESULT lockRead(MediumState_T *aState, bool fWithinQueryInfo);
+    HRESULT lockWrite(MediumState_T *aState, bool fWithinQueryInfo);
 
     HRESULT canClose();

trunk/src/VBox/Main/include/VirtualBoxImpl.h

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2006-2011 Oracle Corporation
+ * Copyright (C) 2006-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -260 +260 @@
     void copyPathRelativeToConfig(const Utf8Str &strSource, Utf8Str &strTarget);
 
-    HRESULT registerHardDisk(Medium *aHardDisk, GuidList *pllRegistriesThatNeedSaving);
-    HRESULT unregisterHardDisk(Medium *aHardDisk, GuidList *pllRegistriesThatNeedSaving);
-
-    HRESULT registerImage(Medium *aImage, DeviceType_T argType, GuidList *pllRegistriesThatNeedSaving);
-    HRESULT unregisterImage(Medium *aImage, DeviceType_T argType, GuidList *pllRegistriesThatNeedSaving);
+    HRESULT registerMedium(Medium *pMedium, ComObjPtr<Medium> *ppMedium, DeviceType_T argType, GuidList *pllRegistriesThatNeedSaving);
+    HRESULT unregisterMedium(Medium *pMedium, GuidList *pllRegistriesThatNeedSaving);
 
     void pushMediumToListWithChildren(MediaList &llMedia, Medium *pMedium);
@@ -301 +298 @@
                                    const Utf8Str &aLocation,
                                    Utf8Str &aConflictType,
-                                   bool &fIdentical);
+                                   ComObjPtr<Medium> *pDupMedium);
 
     HRESULT registerMachine(Machine *aMachine);

trunk/src/VBox/Main/src-all/ProgressImpl.cpp

@@ -6 +6 @@
 
 /*
- * Copyright (C) 2006-2010 Oracle Corporation
+ * Copyright (C) 2006-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -820 +820 @@
         {
             mWaitersCount++;
-            alock.leave();
+            alock.release();
             vrc = RTSemEventMultiWait(mCompletedSem,
                                       fForever ? RT_INDEFINITE_WAIT : (RTMSINTERVAL)timeLeft);
-            alock.enter();
+            alock.acquire();
             mWaitersCount--;
 
@@ -884 +884 @@
         {
             mWaitersCount ++;
-            alock.leave();
+            alock.release();
             vrc = RTSemEventMultiWait(mCompletedSem,
                                       fForever ? RT_INDEFINITE_WAIT : (unsigned) timeLeft);
-            alock.enter();
+            alock.acquire();
             mWaitersCount--;
 
@@ -1664 +1664 @@
         while (!mCompleted && (forever || timeLeft > 0))
         {
-            alock.leave();
+            alock.release();
             rc = mProgresses.back()->WaitForCompletion(forever ? -1 : (LONG) timeLeft);
-            alock.enter();
+            alock.acquire();
 
             if (SUCCEEDED(rc))
@@ -1748 +1748 @@
                (forever || timeLeft > 0))
         {
-            alock.leave();
+            alock.release();
             /* wait for the appropriate progress operation completion */
             rc = mProgresses[progress]-> WaitForOperationCompletion(operation,
                                                                     forever ? -1 : (LONG) timeLeft);
-            alock.enter();
+            alock.acquire();
 
             if (SUCCEEDED(rc))

trunk/src/VBox/Main/src-all/VirtualBoxBase.cpp

@@ -7 +7 @@
 
 /*
- * Copyright (C) 2006-2010 Oracle Corporation
+ * Copyright (C) 2006-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -209 +209 @@
             LogFlowThisFunc(("Waiting for AutoInitSpan/AutoReinitSpan to finish...\n"));
 
-            stateLock.leave();
+            stateLock.release();
             RTSemEventMultiWait (mInitUninitSem, RT_INDEFINITE_WAIT);
-            stateLock.enter();
+            stateLock.acquire();
 
             if (-- mInitUninitWaiters == 0)
@@ -776 +776 @@
     {
         mObj->setState(VirtualBoxBase::InitFailed);
-        /* leave the lock to prevent nesting when uninit() is called */
-        stateLock.leave();
+        /* release the lock to prevent nesting when uninit() is called */
+        stateLock.acquire();
         /* call uninit() to let the object uninit itself after failed init() */
         mObj->uninit();
@@ -906 +906 @@
                          mObj));
 
-            stateLock.leave();
+            stateLock.release();
             RTSemEventMultiWait(mObj->mInitUninitSem, RT_INDEFINITE_WAIT);
-            stateLock.enter();
+            stateLock.acquire();
 
             if (--mObj->mInitUninitWaiters == 0)
@@ -935 +935 @@
                      mObj, mObj->mCallers));
 
-        stateLock.leave();
+        stateLock.release();
         RTSemEventWait(mObj->mZeroCallersSem, RT_INDEFINITE_WAIT);
     }

trunk/src/VBox/Main/src-client/ConsoleImpl.cpp

@@ -2077 +2077 @@
         return ptrVM.rc();
 
-    /* leave the lock before a VMR3* call (EMT will call us back)! */
-    alock.leave();
+    /* release the lock before a VMR3* call (EMT will call us back)! */
+    alock.release();
 
     int vrc = VMR3Reset(ptrVM);
@@ -2290 +2290 @@
                           this, pVM, aCpu);
 
-    /* leave the lock before a VMR3* call (EMT will call us back)! */
+    /* release the lock before a VMR3* call (EMT will call us back)! */
     alock.release();
 
@@ -2354 +2354 @@
     LogFlowThisFunc(("Sending PAUSE request...\n"));
 
-    /* leave the lock before a VMR3* call (EMT will call us back)! */
-    alock.leave();
+    /* release the lock before a VMR3* call (EMT will call us back)! */
+    alock.release();
 
     int vrc = VMR3Suspend(ptrVM);
@@ -2389 +2389 @@
     LogFlowThisFunc(("Sending RESUME request...\n"));
 
-    /* leave the lock before a VMR3* call (EMT will call us back)! */
-    alock.leave();
+    /* release the lock before a VMR3* call (EMT will call us back)! */
+    alock.release();
 
 #ifdef VBOX_WITH_EXTPACK
@@ -2434 +2434 @@
     if (!ptrVM.isOk())
         return ptrVM.rc();
-/** @todo leave the console lock? */
+
+/** @todo release the console lock? */
 
     /* get the acpi device interface and press the button. */
@@ -2481 +2482 @@
     if (!ptrVM.isOk())
         return ptrVM.rc();
-/** @todo leave the console lock? */
+
+/** @todo release the console lock? */
 
     /* get the acpi device interface and check if the button press was handled. */
@@ -2536 +2538 @@
         return ptrVM.rc();
 
-/** @todo leave the console lock? */
+/** @todo release the console lock? */
 
     /* get the acpi device interface and query the information. */
@@ -2577 +2579 @@
         return ptrVM.rc();
 
-/** @todo leave the console lock? */
+/** @todo release the console lock? */
 
     /* get the acpi device interface and press the sleep button. */
@@ -2679 +2681 @@
         /*
          * If we fail here it means a PowerDown() call happened on another
-         * thread while we were doing Pause() (which leaves the Console lock).
+         * thread while we were doing Pause() (which releases the Console lock).
          * We assign PowerDown() a higher precedence than SaveState(),
          * therefore just return the error to the caller.
@@ -2887 +2889 @@
             tr("The virtual machine does not have a USB controller"));
 
-    /* leave the lock because the USB Proxy service may call us back
+    /* release the lock because the USB Proxy service may call us back
      * (via onUSBDeviceAttach()) */
-    alock.leave();
+    alock.release();
 
     /* Request the device capture */
@@ -2932 +2934 @@
      * Inform the USB device and USB proxy about what's cooking.
      */
-    alock.leave();
+    alock.release();
     HRESULT rc2 = mControl->DetachUSBDevice(aId, false /* aDone */);
     if (FAILED(rc2))
         return rc2;
-    alock.enter();
+    alock.acquire();
 
     /* Request the PDM to detach the USB device. */
@@ -2943 +2945 @@
     if (SUCCEEDED(rc))
     {
-        /* leave the lock since we don't need it any more (note though that
+        /* release the lock since we don't need it any more (note though that
          * the USB Proxy service must not call us back here) */
-        alock.leave();
+        alock.release();
 
         /* Request the device release. Even if it fails, the device will
@@ -3269 +3271 @@
         /*
          * If we fail here it means a PowerDown() call happened on another
-         * thread while we were doing Pause() (which leaves the Console lock).
+         * thread while we were doing Pause() (which releases the Console lock).
          * We assign PowerDown() a higher precedence than TakeSnapshot(),
          * therefore just return the error to the caller.
@@ -3595 +3597 @@
                           fForce);
 
-    /* leave the lock before waiting for a result (EMT will call us back!) */
-    alock.leave();
+    /* release the lock before waiting for a result (EMT will call us back!) */
+    alock.release();
 
     if (vrc == VERR_TIMEOUT || RT_SUCCESS(vrc))
@@ -3848 +3850 @@
                           aMediumAttachment);
 
-    /* leave the lock before waiting for a result (EMT will call us back!) */
-    alock.leave();
+    /* release the lock before waiting for a result (EMT will call us back!) */
+    alock.release();
 
     if (vrc == VERR_TIMEOUT || RT_SUCCESS(vrc))
@@ -4089 +4091 @@
                           aMediumAttachment);
 
-    /* leave the lock before waiting for a result (EMT will call us back!) */
-    alock.leave();
+    /* release the lock before waiting for a result (EMT will call us back!) */
+    alock.release();
 
     if (vrc == VERR_TIMEOUT || RT_SUCCESS(vrc))
@@ -4498 +4500 @@
                           this, ptrVM.raw(), pszDevice, uInstance, uLun, aNetworkAdapter);
 
-    /* leave the lock before waiting for a result (EMT will call us back!) */
-    alock.leave();
+    /* release the lock before waiting for a result (EMT will call us back!) */
+    alock.release();
 
     if (vrc == VERR_TIMEOUT || RT_SUCCESS(vrc))
@@ -4833 +4835 @@
         {
             /* VRDP server may call this Console object back from other threads (VRDP INPUT or OUTPUT). */
-            alock.leave();
+            alock.release();
 
             if (vrdpEnabled)
@@ -4852 +4854 @@
             }
 
-            alock.enter();
+            alock.acquire();
         }
     }
@@ -6688 +6690 @@
     /* ----------------------------------------------------------------------
      * DONE with necessary state changes, perform the power down actions (it's
-     * safe to leave the object lock now if needed)
+     * safe to release the object lock now if needed)
      * ---------------------------------------------------------------------- */
 
@@ -6699 +6701 @@
         /* Leave the lock since EMT will call us back as addVMCaller()
          * in updateDisplayData(). */
-        alock.leave();
+        alock.release();
 
         mConsoleVRDPServer->Stop();
 
-        alock.enter();
+        alock.acquire();
     }
 
@@ -6729 +6731 @@
                           mVMCallers));
 
-        alock.leave();
+        alock.release();
 
         RTSemEventWait(mVMZeroCallersSem, RT_INDEFINITE_WAIT);
 
-        alock.enter();
+        alock.acquire();
     }
 
@@ -6753 +6755 @@
     {
         LogFlowThisFunc(("Powering off the VM...\n"));
-        alock.leave();
+        alock.release();
         vrc = VMR3PowerOff(VMR3GetVM(pUVM));
 #ifdef VBOX_WITH_EXTPACK
         mptrExtPackManager->callAllVmPowerOffHooks(this, VMR3GetVM(pUVM));
 #endif
-        alock.enter();
+        alock.acquire();
     }
 
@@ -6772 +6774 @@
 
         /* Leave the lock since EMT will call us back as addVMCaller() */
-        alock.leave();
+        alock.release();
 
         m_pVMMDev->hgcmShutdown();
 
-        alock.enter();
+        alock.acquire();
     }
 
@@ -6805 +6807 @@
 
         /* Now we've got to destroy the VM as well. (mpVM is not valid beyond
-         * this point). We leave the lock before calling VMR3Destroy() because
+         * this point). We release the lock before calling VMR3Destroy() because
          * it will result into calling destructors of drivers associated with
          * Console children which may in turn try to lock Console (e.g. by
@@ -6818 +6820 @@
         LogFlowThisFunc(("Destroying the VM...\n"));
 
-        alock.leave();
+        alock.release();
 
         vrc = VMR3Destroy(VMR3GetVM(pUVM));
 
         /* take the lock again */
-        alock.enter();
+        alock.acquire();
 
         /* advance percent count */
@@ -6930 +6932 @@
              * UpdateState we will require Machine and SessionMachine locks
              * (remember that here we're holding the Console lock here, and also
-             * all locks that have been entered by the thread before calling
+             * all locks that have been acquire by the thread before calling
              * this method).
              */
@@ -7648 +7650 @@
     AssertReturn(isWriteLockOnCurrentThread(), E_FAIL);
 
-    /* still want a lock object because we need to leave it */
+    /* still want a lock object because we need to release it */
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
@@ -7680 +7682 @@
                       Address.c_str(), uuid.raw()));
 
-    /* leave the lock before a VMR3* call (EMT will call us back)! */
-    alock.leave();
+    /* release the lock before a VMR3* call (EMT will call us back)! */
+    alock.release();
 
 /** @todo just do everything here and only wrap the PDMR3Usb call. That'll offload some notification stuff from the EMT thread. */
@@ -7689 +7691 @@
 
     /* restore the lock */
-    alock.enter();
+    alock.acquire();
 
     /* hrc is S_OK here */
@@ -7790 +7792 @@
     AssertReturn(isWriteLockOnCurrentThread(), E_FAIL);
 
-    /* still want a lock object because we need to leave it */
+    /* still want a lock object because we need to release it */
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
@@ -7804 +7806 @@
                      (*aIt)->id().raw()));
 
-    /* leave the lock before a VMR3* call (EMT will call us back)! */
-    alock.leave();
+    /* release the lock before a VMR3* call (EMT will call us back)! */
+    alock.release();
 
 /** @todo just do everything here and only wrap the PDMR3Usb call. That'll offload some notification stuff from the EMT thread. */
@@ -8266 +8268 @@
     if (RT_SUCCESS(vrc))
     {
-        /* leave the lock before calling Host in VBoxSVC since Host may call
+        /* release the lock before calling Host in VBoxSVC since Host may call
          * us back from under its lock (e.g. onUSBDeviceAttach()) which would
          * produce an inter-process dead-lock otherwise. */
         AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-        alock.leave();
+        alock.release();
 
         HRESULT hrc = mControl->AutoCaptureUSBDevices();
@@ -8300 +8302 @@
     mUSBDevices.clear();
 
-    /* leave the lock before calling Host in VBoxSVC since Host may call
+    /* release the lock before calling Host in VBoxSVC since Host may call
      * us back from under its lock (e.g. onUSBDeviceAttach()) which would
      * produce an inter-process dead-lock otherwise. */
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-    alock.leave();
+    alock.release();
 
     mControl->DetachAllUSBDevices(aDone);
@@ -8575 +8577 @@
 
         /* Does VRDP server call Console from the other thread?
-         * Not sure (and can change), so leave the lock just in case.
+         * Not sure (and can change), so release the lock just in case.
          */
-        alock.leave();
+        alock.release();
         vrc = server->Launch();
-        alock.enter();
+        alock.acquire();
 
         if (vrc == VERR_NET_ADDRESS_IN_USE)
@@ -8627 +8629 @@
         PVM pVM;
         /*
-         * leave the lock since EMT will call Console. It's safe because
+         * release the lock since EMT will call Console. It's safe because
          * mMachineState is either Starting or Restoring state here.
          */
-        alock.leave();
+        alock.release();
 
         vrc = VMR3Create(cCpus,
@@ -8640 +8642 @@
                          &pVM);
 
-        alock.enter();
+        alock.acquire();
 
         /* Enable client connections to the server. */
@@ -8677 +8679 @@
                 {
                     /* Does the code below call Console from the other thread?
-                     * Not sure, so leave the lock just in case. */
-                    alock.leave();
+                     * Not sure, so release the lock just in case. */
+                    alock.release();
 
                     for (SharedFolderDataMap::const_iterator it = task->mSharedFolders.begin();
@@ -8698 +8700 @@
                         rc = S_OK;          // do not fail with broken shared folders
 
-                    /* enter the lock again */
-                    alock.enter();
+                    /* acquire the lock again */
+                    alock.acquire();
                 }
 
@@ -8708 +8710 @@
                 if (FAILED(rc)) break;
 
-                /* leave the lock before a lengthy operation */
-                alock.leave();
+                /* release the lock before a lengthy operation */
+                alock.release();
 
                 /* Load saved state? */
@@ -8822 +8824 @@
                 }
 
-                /* enter the lock again */
-                alock.enter();
+                /* acquire the lock again */
+                alock.acquire();
             }
             while (0);
@@ -8835 +8837 @@
                 /* powerDown() will call VMR3Destroy() and do all necessary
                  * cleanup (VRDP, USB devices) */
+                alock.release();
                 HRESULT rc2 = pConsole->powerDown();
+                alock.acquire();
                 AssertComRC(rc2);
             }
@@ -8845 +8849 @@
                  * be sticky but our error callback isn't.
                  */
-                alock.leave();
+                alock.release();
                 VMR3AtErrorDeregister(pVM, Console::genericVMSetErrorCallback, &task->mErrorMsg);
                 /** @todo register another VMSetError callback? */
-                alock.enter();
+                alock.acquire();
             }
         }
@@ -8914 +8918 @@
      */
 
-    /* leave the lock, don't need it any more */
-    alock.leave();
+    /* release the lock, don't need it any more */
+    alock.release();
 
     if (SUCCEEDED(rc))
@@ -9114 +9118 @@
             pTask->mProgress->setCancelCallback(takesnapshotProgressCancelCallback, ptrVM.rawUVM());
 
-            alock.leave();
+            alock.release();
             LogFlowFunc(("VMR3Save...\n"));
             int vrc = VMR3Save(ptrVM,
@@ -9122 +9126 @@
                                static_cast<IProgress *>(pTask->mProgress),
                                &fSuspenededBySave);
-            alock.enter();
+            alock.acquire();
             if (RT_FAILURE(vrc))
                 throw setErrorStatic(E_FAIL,
@@ -9190 +9194 @@
 
                 /*
-                 * don't leave the lock since reconfigureMediumAttachment
+                 * don't release the lock since reconfigureMediumAttachment
                  * isn't going to need the Console lock.
                  */
@@ -9265 +9269 @@
                 LogFlowFunc(("VMR3Resume...\n"));
                 SafeVMPtr ptrVM(that);
-                alock.leave();
+                alock.release();
                 int vrc = VMR3Resume(ptrVM);
-                alock.enter();
+                alock.acquire();
                 if (RT_FAILURE(vrc))
                 {
@@ -9323 +9327 @@
                         LogFlowFunc(("VMR3Resume (on failure)...\n"));
                         SafeVMPtr ptrVM(that);
-                        alock.leave();
+                        alock.release();
                         int vrc = VMR3Resume(ptrVM); AssertLogRelRC(vrc);
-                        alock.enter();
+                        alock.acquire();
                         if (RT_FAILURE(vrc))
                             that->setMachineState(MachineState_Paused);
@@ -9411 +9415 @@
          */
         task->releaseVMCaller();
+        thatLock.release();
         rc = that->powerDown();
+        thatLock.acquire();
     }
 
@@ -9458 +9464 @@
     /* release VM caller to avoid the powerDown() deadlock */
     task->releaseVMCaller();
+
+    thatLock.release();
 
     that->powerDown(task->mServerProgress);

trunk/src/VBox/Main/src-client/ConsoleImplTeleporter.cpp

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2010 Oracle Corporation
+ * Copyright (C) 2010-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -886 +886 @@
                         if (pState->mfSuspendedByUs)
                         {
-                            autoLock.leave();
+                            autoLock.release();
                             int rc = VMR3Resume(VMR3GetVM(pState->mpUVM));
                             AssertLogRelMsgRC(rc, ("VMR3Resume -> %Rrc\n", rc));
-                            autoLock.enter();
+                            autoLock.acquire();
                         }
                     }
@@ -901 +901 @@
         }
     }
-    autoLock.leave();
+    autoLock.release();
 
     /*

trunk/src/VBox/Main/src-client/DisplayImpl.cpp

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2006-2010 Oracle Corporation
+ * Copyright (C) 2006-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -2038 +2038 @@
     if (pVM.isOk())
     {
-        /* Must leave the lock here because the changeFramebuffer will
+        /* Must release the lock here because the changeFramebuffer will
          * also obtain it. */
-        alock.leave ();
+        alock.release();
 
         /* send request to the EMT thread */
@@ -2046 +2046 @@
                                    (PFNRT) changeFramebuffer, 3, this, aFramebuffer, aScreenId);
 
-        alock.enter ();
+        alock.acquire();
 
         ComAssertRCRet (vrc, E_FAIL);
@@ -2064 +2064 @@
                 VMMDev *pVMMDev = mParent->getVMMDev();
 
-                alock.leave ();
+                alock.release();
 
                 if (pVMMDev)
@@ -2070 +2070 @@
                 /*ComAssertRCRet (vrc, E_FAIL);*/
 
-                alock.enter ();
+                alock.acquire();
             }
         }
@@ -2155 +2155 @@
 //        return setError(E_FAIL, tr("Not enough VRAM for the selected video mode"));
 
-    /* Have to leave the lock because the pfnRequestDisplayChange
+    /* Have to release the lock because the pfnRequestDisplayChange
      * will call EMT.  */
-    alock.leave ();
+    alock.release();
 
     VMMDev *pVMMDev = mParent->getVMMDev();
@@ -2176 +2176 @@
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
-    /* Have to leave the lock because the pfnRequestSeamlessChange will call EMT.  */
-    alock.leave ();
+    /* Have to release the lock because the pfnRequestSeamlessChange will call EMT.  */
+    alock.release();
 
     VMMDev *pVMMDev = mParent->getVMMDev();
@@ -2377 +2377 @@
     LogRelFlowFunc (("Sending SCREENSHOT request\n"));
 
-    /* Leave lock because other thread (EMT) is called and it may initiate a resize
+    /* Release lock because other thread (EMT) is called and it may initiate a resize
      * which also needs lock.
      *
      * This method does not need the lock anymore.
      */
-    alock.leave();
+    alock.release();
 
     int vrc = displayTakeScreenshot(pVM, this, mpDrv, aScreenId, address, width, height);
@@ -2430 +2430 @@
     LogRelFlowFunc (("Sending SCREENSHOT request\n"));
 
-    /* Leave lock because other thread (EMT) is called and it may initiate a resize
+    /* Release lock because other thread (EMT) is called and it may initiate a resize
      * which also needs lock.
      *
      * This method does not need the lock anymore.
      */
-    alock.leave();
+    alock.release();
 
     size_t cbData = width * 4 * height;
@@ -2507 +2507 @@
     LogRelFlowFunc (("Sending SCREENSHOT request\n"));
 
-    /* Leave lock because other thread (EMT) is called and it may initiate a resize
+    /* Release lock because other thread (EMT) is called and it may initiate a resize
      * which also needs lock.
      *
      * This method does not need the lock anymore.
      */
-    alock.leave();
+    alock.release();
 
     size_t cbData = width * 4 * height;
@@ -2676 +2676 @@
     if (FAILED(pVM.rc())) return pVM.rc();
 
-    /* Leave lock because the call scheduled on EMT may also try to take it. */
-    alock.leave();
+    /* Release lock because the call scheduled on EMT may also try to take it. */
+    alock.release();
 
     /*
@@ -2800 +2800 @@
     LogRelFlowFunc (("Sending DPYUPDATE request\n"));
 
-    /* Have to leave the lock when calling EMT.  */
-    alock.leave ();
+    /* Have to release the lock when calling EMT.  */
+    alock.release();
 
     /* pdm.h says that this has to be called from the EMT thread */
     int rcVBox = VMR3ReqCallVoidWait(pVM, VMCPUID_ANY, (PFNRT)Display::InvalidateAndUpdateEMT,
                                      1, this);
-    alock.enter ();
+    alock.acquire();
 
     if (RT_FAILURE(rcVBox))
@@ -3003 +3003 @@
 
 #if defined(VBOX_WITH_CROGL)
-        /* Leave the lock, because SHCRGL_HOST_FN_SCREEN_CHANGED will read current framebuffer */
+        /* Release the lock, because SHCRGL_HOST_FN_SCREEN_CHANGED will read current framebuffer */
         {
             BOOL is3denabled;
@@ -3010 +3010 @@
             if (is3denabled)
             {
-                alock.leave ();
+                alock.release();
             }
         }

trunk/src/VBox/Main/src-client/SessionImpl.cpp

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2006-2010 Oracle Corporation
+ * Copyright (C) 2006-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -936 +936 @@
          *  SessionState_Closing here, so it's safe.
          */
-        alock.leave();
+        alock.release();
 
         LogFlowThisFunc(("Calling mControl->OnSessionEnd()...\n"));
@@ -942 +942 @@
         LogFlowThisFunc(("mControl->OnSessionEnd()=%08X\n", rc));
 
-        alock.enter();
+        alock.acquire();
 
         /*

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

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2006-2009 Oracle Corporation
+ * Copyright (C) 2006-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -472 +472 @@
 
     /* inform the direct session if any */
-    alock.leave();
+    alock.release();
     //onStorageControllerChange(); @todo
 

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

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2006-2009 Oracle Corporation
+ * Copyright (C) 2006-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -294 +294 @@
     /* inform direct session if any. */
     ComObjPtr<Machine> pMachine = m->pParent->getMachine();
-    alock.leave();
+    alock.release();
     pMachine->onBandwidthGroupChange(this);
 

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

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2006-2007 Oracle Corporation
+ * Copyright (C) 2006-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -355 +355 @@
  * As a convenience, this method will operate like attachToVM() if the device
  * is already held by the proxy. Note that it will then perform IPC to the VM
- * process, which means it will temporarily leave all locks. (Is this a good idea?)
+ * process, which means it will temporarily release all locks. (Is this a good idea?)
  *
  * @param   aMachine    Machine this device should be attach to.
@@ -483 +483 @@
 
     /*
-     * The VM process will query the object, so grab a reference to ourselves and leave the locks.
+     * The VM process will query the object, so grab a reference to ourselves and release the locks.
      */
     ComPtr<IUSBDevice> d = this;
 
     AutoWriteLock alockSelf(this COMMA_LOCKVAL_SRC_POS);
-    alockSelf.leave();
+    alockSelf.release();
     AutoWriteLock alockProxy(mUSBProxyService COMMA_LOCKVAL_SRC_POS);
-    alockProxy.leave();
+    alockProxy.release();
 
     /*
@@ -504 +504 @@
 
     /*
-     * As we re-enter the lock, we'll have to check if the device was
+     * As we re-acquire the lock, we'll have to check if the device was
      * physically detached while we were busy.
      */
-    alockProxy.enter();
-    alockSelf.enter();
+    alockProxy.acquire();
+    alockSelf.acquire();
 
     if (SUCCEEDED(hrc))
@@ -561 +561 @@
 
     /*
-     * Change the state and abandond the locks. The VM may query
+     * Change the state and abandon the locks. The VM may query
      * data and we don't want to deadlock - the state protects us,
      * so, it's not a bit issue here.
@@ -567 +567 @@
     setState(kHostUSBDeviceState_PhysDetachingFromVM, kHostUSBDeviceState_PhysDetached);
     AutoWriteLock alockSelf(this COMMA_LOCKVAL_SRC_POS);
-    alockSelf.leave();
+    alockSelf.release();
     AutoWriteLock alockProxy(mUSBProxyService COMMA_LOCKVAL_SRC_POS);
-    alockProxy.leave();
+    alockProxy.release();
 
     /*
@@ -584 +584 @@
 
     /*
-     * Re-enter the locks and complete the transition.
-     */
-    alockProxy.enter();
-    alockSelf.enter();
+     * Re-acquire the locks and complete the transition.
+     */
+    alockProxy.acquire();
+    alockSelf.acquire();
     advanceTransition();
 }

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

@@ -827 +827 @@
 
     /* the lock is no more necessary (SessionMachine is uninitialized) */
-    alock.leave();
+    alock.release();
 
     // has machine been modified?
@@ -3064 +3064 @@
 
         /*
-         *  Leave the lock before calling the client process. It's safe here
+         *  Release the lock before calling the client process. It's safe here
          *  since the only thing to do after we get the lock again is to add
          *  the remote control to the list (which doesn't directly influence
          *  anything).
          */
-        alock.leave();
+        alock.release();
 
         // get the console of the session holding the write lock (this is a remote call)
@@ -3092 +3092 @@
             return setError(VBOX_E_VM_ERROR,
                             tr("Failed to assign the machine to the session (%Rrc)"), rc);
-        alock.enter();
-
-        // need to revalidate the state after entering the lock again
+        alock.acquire();
+
+        // need to revalidate the state after acquiring the lock again
         if (mData->mSession.mState != SessionState_Locked)
         {
@@ -3163 +3163 @@
              *  Set the session state to Spawning to protect against subsequent
              *  attempts to open a session and to unregister the machine after
-             *  we leave the lock.
+             *  we release the lock.
              */
             SessionState_T origState = mData->mSession.mState;
@@ -3169 +3169 @@
 
             /*
-             *  Leave the lock before calling the client process -- it will call
-             *  Machine/SessionMachine methods. Leaving the lock here is quite safe
+             *  Release the lock before calling the client process -- it will call
+             *  Machine/SessionMachine methods. Releasing the lock here is quite safe
              *  because the state is Spawning, so that LaunchVMProcess() and
              *  LockMachine() calls will fail. This method, called before we
-             *  enter the lock again, will fail because of the wrong PID.
+             *  acquire the lock again, will fail because of the wrong PID.
              *
              *  Note that mData->mSession.mRemoteControls accessed outside
              *  the lock may not be modified when state is Spawning, so it's safe.
              */
-            alock.leave();
+            alock.release();
 
             LogFlowThisFunc(("Calling AssignMachine()...\n"));
@@ -3227 +3227 @@
             }
 
-            /* enter the lock again */
-            alock.enter();
+            /* acquire the lock again */
+            alock.acquire();
 
             /* Restore the session state */
@@ -3283 +3283 @@
         }
 
-        /* Leave the lock since SessionMachine::uninit() locks VirtualBox which
+        /* Release the lock since SessionMachine::uninit() locks VirtualBox which
          * would break the lock order */
-        alock.leave();
+        alock.release();
 
         /* uninitialize the created session machine on failure */
@@ -3804 +3804 @@
         /* Apply the normal locking logic to the entire chain. */
         MediumLockList *pMediumLockList(new MediumLockList());
+        mediumLock.release();
+        treeLock.release();
         rc = diff->createMediumLockList(true /* fFailIfInaccessible */,
                                         true /* fMediumLockWrite */,
                                         medium,
                                         *pMediumLockList);
+        treeLock.acquire();
+        mediumLock.acquire();
         if (SUCCEEDED(rc))
         {
+            mediumLock.release();
+            treeLock.release();
             rc = pMediumLockList->Lock();
+            treeLock.acquire();
+            mediumLock.acquire();
             if (FAILED(rc))
                 setError(rc,
@@ -3817 +3825 @@
             else
             {
-                /* will leave the lock before the potentially lengthy operation, so
-                 * protect with the special state */
+                /* will release the lock before the potentially lengthy
+                 * operation, so protect with the special state */
                 MachineState_T oldState = mData->mMachineState;
                 setMachineState(MachineState_SettingUp);
 
-                mediumLock.leave();
-                treeLock.leave();
-                alock.leave();
+                mediumLock.release();
+                treeLock.release();
+                alock.release();
 
                 rc = medium->createDiffStorage(diff,
@@ -3833 +3841 @@
                                                &llRegistriesThatNeedSaving);
 
-                alock.enter();
-                treeLock.enter();
-                mediumLock.enter();
+                alock.acquire();
+                treeLock.acquire();
+                mediumLock.acquire();
 
                 setMachineState(oldState);
@@ -3890 +3898 @@
 
     mediumLock.release();
-    treeLock.leave();
+    treeLock.release();
     alock.release();
 
@@ -3958 +3966 @@
     if (fHotplug)
     {
-        alock.leave();
+        alock.release();
         rc = onStorageDeviceChange(pAttach, TRUE /* aRemove */);
-        alock.enter();
+        alock.acquire();
     }
     if (FAILED(rc)) return rc;
@@ -4632 +4640 @@
         // (this state combination is not supported). Note releasing the caller and
         // leaving the lock before calling uninit()
-        alock.leave();
+        alock.release();
         autoCaller.release();
 
@@ -5064 +5072 @@
 
     /* inform the direct session if any */
-    alock.leave();
+    alock.release();
     onSharedFolderChange();
 
@@ -5091 +5099 @@
 
     /* inform the direct session if any */
-    alock.leave();
+    alock.release();
     onSharedFolderChange();
 
@@ -5650 +5658 @@
 
     /* inform the direct session if any */
-    alock.leave();
+    alock.release();
     onStorageControllerChange();
 
@@ -5744 +5752 @@
     {
         /* inform the direct session if any */
-        alock.leave();
+        alock.release();
         onStorageControllerChange();
     }
@@ -5788 +5796 @@
 
     /* inform the direct session if any */
-    alock.leave();
+    alock.release();
     onStorageControllerChange();
 
@@ -6797 +6805 @@
 
     /*
-     *  Note that we don't leave the lock here before calling the client,
+     *  Note that we don't release the lock here before calling the client,
      *  because it doesn't need to call us back if called with a NULL argument.
-     *  Leaving the lock here is dangerous because we didn't prepare the
+     *  Releasing the lock here is dangerous because we didn't prepare the
      *  launch data yet, but the client we've just started may happen to be
      *  too fast and call openSession() that will fail (because of PID, etc.),
@@ -7515 +7523 @@
         RTSemEventMultiReset(mData->mMachineStateDepsSem);
 
-        alock.leave();
+        alock.release();
 
         RTSemEventMultiWait(mData->mMachineStateDepsSem, RT_INDEFINITE_WAIT);
 
-        alock.enter();
+        alock.acquire();
 
         -- mData->mMachineStateChangePending;
@@ -9457 +9465 @@
                               &mParent->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
-    /* must be in a protective state because we leave the lock below */
+    /* must be in a protective state because we release the lock below */
     AssertReturn(   mData->mMachineState == MachineState_Saving
                  || mData->mMachineState == MachineState_LiveSnapshotting
@@ -9490 +9498 @@
 
                     MediumLockList *pMediumLockList(new MediumLockList());
+                    alock.release();
                     rc = pMedium->createMediumLockList(true /* fFailIfInaccessible */,
                                                        false /* fMediumLockWrite */,
                                                        NULL,
                                                        *pMediumLockList);
+                    alock.acquire();
                     if (FAILED(rc))
                     {
@@ -9509 +9519 @@
 
             /* Now lock all media. If this fails, nothing is locked. */
+            alock.release();
             rc = lockedMediaMap->Lock();
+            alock.acquire();
             if (FAILED(rc))
             {
@@ -9583 +9595 @@
 
             /** @todo r=bird: How is the locking and diff image cleaned up if we fail before
-             *        the push_back?  Looks like we're going to leave medium with the
+             *        the push_back?  Looks like we're going to release medium with the
              *        wrong kind of lock (general issue with if we fail anywhere at all)
              *        and an orphaned VDI in the snapshots folder. */
@@ -9593 +9605 @@
             if (aOnline)
             {
+                alock.release();
                 rc = pMediumLockList->Update(pMedium, false);
+                alock.acquire();
                 AssertComRCThrowRC(rc);
             }
 
-            /* leave the locks before the potentially lengthy operation */
+            /* release the locks before the potentially lengthy operation */
             alock.release();
             rc = pMedium->createDiffStorage(diff, MediumVariant_Standard,
@@ -9609 +9623 @@
             rc = lockedMediaMap->Unlock();
             AssertComRCThrowRC(rc);
+            alock.release();
             rc = pMediumLockList->Append(diff, true);
+            alock.acquire();
             AssertComRCThrowRC(rc);
+            alock.release();
             rc = lockedMediaMap->Lock();
+            alock.acquire();
             AssertComRCThrowRC(rc);
 
@@ -9654 +9672 @@
         MultiResult mrc = rc;
 
+        alock.release();
         mrc = deleteImplicitDiffs(pllRegistriesThatNeedSaving);
     }
@@ -9726 +9745 @@
     if (implicitAtts.size() != 0)
     {
-        /* will leave the lock before the potentially lengthy
+        /* will release the lock before the potentially lengthy
          * operation, so protect with the special state (unless already
          * protected) */
@@ -9739 +9758 @@
             setMachineState(MachineState_SettingUp);
 
-        alock.leave();
+        alock.release();
 
         for (MediaData::AttachmentList::const_iterator it = implicitAtts.begin();
@@ -9754 +9773 @@
         }
 
-        alock.enter();
+        alock.acquire();
 
         if (mData->mMachineState == MachineState_SettingUp)
@@ -9869 +9888 @@
         /* attempt to implicitly delete the implicitly created diff */
 
-            /// @todo move the implicit flag from MediumAttachment to Medium
-            /// and forbid any hard disk operation when it is implicit. Or maybe
-            /// a special media state for it to make it even more simple.
+        /// @todo move the implicit flag from MediumAttachment to Medium
+        /// and forbid any hard disk operation when it is implicit. Or maybe
+        /// a special media state for it to make it even more simple.
 
         Assert(mMediaData.isBackedUp());
 
-            /* will leave the lock before the potentially lengthy operation, so
-            * protect with the special state */
+        /* will release the lock before the potentially lengthy operation, so
+         * protect with the special state */
         MachineState_T oldState = mData->mMachineState;
         setMachineState(MachineState_SettingUp);
@@ -10390 +10409 @@
         ComObjPtr<Machine> that = this;
         uint32_t flModifications = mData->flModifications;
-        alock.leave();
+        alock.release();
 
         if (flModifications & IsModified_SharedFolders)
@@ -11258 +11277 @@
     mData.free();
 
-#if 1 /** @todo Please review this change! (bird) */
-    /* drop the exclusive lock before setting the below two to NULL */
+    /* release the exclusive lock before setting the below two to NULL */
     multilock.release();
-#else
-    /* leave the exclusive lock before setting the below two to NULL */
-    multilock.leave();
-#endif
 
     unconst(mParent) = NULL;
@@ -11990 +12004 @@
            )
         {
-            alock.leave();
+            alock.release();
 
             mParent->onGuestPropertyChange(mData->mUuid,
@@ -12727 +12741 @@
             bool fIsVitalImage = (devType == DeviceType_HardDisk);
 
+            alock.release();
             mrc = pMedium->createMediumLockList(fIsVitalImage /* fFailIfInaccessible */,
                                                 !fIsReadOnlyLock /* fMediumLockWrite */,
                                                 NULL,
                                                 *pMediumLockList);
+            alock.acquire();
             if (FAILED(mrc))
             {
@@ -12752 +12768 @@
     {
         /* Now lock all media. If this fails, nothing is locked. */
+        alock.release();
         HRESULT rc = mData->mSession.mLockedMedia.Lock();
+        alock.acquire();
         if (FAILED(rc))
         {

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

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2011 Oracle Corporation
+ * Copyright (C) 2011-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -649 +649 @@
     try
     {
-        Bstr bstrSrcId;
-        rc = pParent->COMGETTER(Id)(bstrSrcId.asOutParam());
-        if (FAILED(rc)) throw rc;
+        // check validity of parent object
+        {
+            AutoReadLock alock(pParent COMMA_LOCKVAL_SRC_POS);
+            Bstr bstrSrcId;
+            rc = pParent->COMGETTER(Id)(bstrSrcId.asOutParam());
+            if (FAILED(rc)) throw rc;
+        }
         ComObjPtr<Medium> diff;
         diff.createObject();
@@ -661 +665 @@
         if (FAILED(rc)) throw rc;
 
-        // need tree lock for createMediumLockList
-        AutoWriteLock treeLock(p->getVirtualBox()->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-
         MediumLockList *pMediumLockList(new MediumLockList());
         rc = diff->createMediumLockList(true /* fFailIfInaccessible */,
@@ -672 +673 @@
         rc = pMediumLockList->Lock();
         if (FAILED(rc)) throw rc;
-
-        treeLock.release();
 
         /* this already registers the new diff image */
@@ -1202 +1201 @@
                         {
                             AutoWriteLock tlock(p->mParent->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-                            rc = p->mParent->registerHardDisk(pTarget, NULL /* pllRegistriesThatNeedSaving */);
+                            rc = p->mParent->registerMedium(pTarget, &pTarget,
+                                                            DeviceType_HardDisk,
+                                                            NULL /* pllRegistriesThatNeedSaving */);
                             if (FAILED(rc)) throw rc;
                         }

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

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2008-2011 Oracle Corporation
+ * Copyright (C) 2008-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -125 +125 @@
     MediumState_T preLockState;
 
-    /** Special synchronization for operations which must wait for queryInfo()
-     * in another thread to complete. Using a SemRW is not quite ideal, but at
-     * least it is subject to the lock validator, unlike the SemEventMulti
-     * which we had here for many years. Catching possible deadlocks is more
-     * important than a tiny bit of efficiency. */
+    /** Special synchronization for operations which must wait for
+     * Medium::queryInfo in another thread to complete. Using a SemRW is
+     * not quite ideal, but at least it is subject to the lock validator,
+     * unlike the SemEventMulti which we had here for many years. Catching
+     * possible deadlocks is more important than a tiny bit of efficiency. */
     RWLockHandle queryInfoSem;
     bool queryInfoRunning : 1;
@@ -144 +144 @@
     bool autoReset : 1;
 
-    /** New UUID to be set on the next queryInfo() call. */
+    /** New UUID to be set on the next Medium::queryInfo call. */
     const Guid uuidImage;
-    /** New parent UUID to be set on the next queryInfo() call. */
+    /** New parent UUID to be set on the next Medium::queryInfo call. */
     const Guid uuidParentImage;
 
@@ -956 +956 @@
     m->hostDrive = false;
 
-    /* No storage unit is created yet, no need to queryInfo() */
+    /* No storage unit is created yet, no need to call Medium::queryInfo */
 
     rc = setFormat(aFormat);
@@ -978 +978 @@
 
         AutoWriteLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-        rc = m->pVirtualBox->registerHardDisk(this, pllRegistriesThatNeedSaving);
+        ComObjPtr<Medium> pMedium;
+        rc = m->pVirtualBox->registerMedium(this, &pMedium, DeviceType_HardDisk,
+                                            pllRegistriesThatNeedSaving);
+        Assert(this == pMedium);
     }
 
@@ -1054 +1057 @@
         unconst(m->uuidImage).create();
 
-    {
-        // Medium::queryInfo needs write lock
-        AutoWriteLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-        rc = queryInfo(fForceNewUuid /* fSetImageId */, false /* fSetParentId */);
-    }
+    rc = queryInfo(fForceNewUuid /* fSetImageId */, false /* fSetParentId */);
 
     if (SUCCEEDED(rc))
@@ -1073 +1072 @@
             AssertReturn(!m->id.isEmpty(), E_FAIL);
 
-            /* storage format must be detected by queryInfo() if the medium is accessible */
+            /* storage format must be detected by Medium::queryInfo if the
+             * medium is accessible */
             AssertReturn(!m->strFormat.isEmpty(), E_FAIL);
         }
@@ -1134 +1134 @@
     }
 
-    /* see below why we don't call queryInfo() (and therefore treat the medium
-     * as inaccessible for now */
+    /* see below why we don't call Medium::queryInfo (and therefore treat
+     * the medium as inaccessible for now */
     m->state = MediumState_Inaccessible;
     m->strLastAccessError = tr("Accessibility check was not yet performed");
@@ -1230 +1230 @@
                      m->strLocationFull.c_str(), m->strFormat.c_str(), m->id.raw()));
 
-    /* Don't call queryInfo() for registered media to prevent the calling
+    /* Don't call Medium::queryInfo for registered media to prevent the calling
      * thread (i.e. the VirtualBox server startup thread) from an unexpected
      * freeze but mark it as initially inaccessible instead. The vital UUID,
@@ -1256 +1256 @@
         if (FAILED(rc)) break;
 
-        rc = m->pVirtualBox->registerHardDisk(pHD, NULL /* pllRegistriesThatNeedSaving */ );
+        rc = m->pVirtualBox->registerMedium(pHD, &pHD, DeviceType_HardDisk,
+                                            NULL /* pllRegistriesThatNeedSaving */ );
         if (FAILED(rc)) break;
     }
@@ -1335 +1336 @@
  * @note All children of this medium get uninitialized by calling their
  *       uninit() methods.
- *
- * @note Caller must hold the tree lock of the medium tree this medium is on.
  */
 void Medium::uninit()
@@ -1632 +1631 @@
 
     // we access mParent and members
-    AutoMultiWriteLock2 mlock(&m->pVirtualBox->getMediaTreeLockHandle(),
-                              this->lockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoWriteLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoWriteLock mlock(this COMMA_LOCKVAL_SRC_POS);
 
     switch (m->state)
@@ -1707 +1706 @@
             if (aType == MediumType_Shareable)
             {
-                if (m->state == MediumState_Inaccessible)
-                {
-                    HRESULT rc = queryInfo(false /* fSetImageId */, false /* fSetParentId */);
-                    if (FAILED(rc))
-                        return setError(rc,
-                                        tr("Cannot change type for medium '%s' to 'Shareable' because the medium is inaccessible"),
-                                        m->strLocationFull.c_str());
-                }
-
                 MediumVariant_T variant = getVariant();
                 if (!(variant & MediumVariant_Fixed))
@@ -1762 +1752 @@
     addToRegistryIDList(llRegistriesThatNeedSaving);
     mlock.release();
+    treeLock.release();
     HRESULT rc = m->pVirtualBox->saveRegistries(llRegistriesThatNeedSaving);
 
@@ -1964 +1955 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    AutoMultiWriteLock2 alock(&m->pVirtualBox->getMediaTreeLockHandle(),
-                              this->lockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
     switch (m->state)
@@ -1998 +1988 @@
     unconst(m->uuidParentImage) = parentId;
 
+    // must not hold any locks before calling Medium::queryInfo
+    alock.release();
+
     HRESULT rc = queryInfo(!!aSetImageId /* fSetImageId */,
                            !!aSetParentId /* fSetParentId */);
@@ -2011 +2004 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    /* queryInfo() locks this for writing. */
-    AutoMultiWriteLock2 alock(&m->pVirtualBox->getMediaTreeLockHandle(),
-                              this->lockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
 
     HRESULT rc = S_OK;
@@ -2023 +2014 @@
         case MediumState_LockedRead:
         {
+            // must not hold any locks before calling Medium::queryInfo
+            alock.release();
+
             rc = queryInfo(false /* fSetImageId */, false /* fSetParentId */);
+
+            alock.acquire();
             break;
         }
@@ -2095 +2091 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    /* Wait for a concurrently running queryInfo() to complete */
-    while (m->queryInfoRunning)
-    {
-        alock.leave();
-        {
-            AutoReadLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
-        }
-        alock.enter();
-    }
-
-    /* return the current state before */
-    if (aState)
-        *aState = m->state;
-
-    HRESULT rc = S_OK;
-
-    switch (m->state)
-    {
-        case MediumState_Created:
-        case MediumState_Inaccessible:
-        case MediumState_LockedRead:
-        {
-            ++m->readers;
-
-            ComAssertMsgBreak(m->readers != 0, ("Counter overflow"), rc = E_FAIL);
-
-            /* Remember pre-lock state */
-            if (m->state != MediumState_LockedRead)
-                m->preLockState = m->state;
-
-            LogFlowThisFunc(("Okay - prev state=%d readers=%d\n", m->state, m->readers));
-            m->state = MediumState_LockedRead;
-
-            break;
-        }
-        default:
-        {
-            LogFlowThisFunc(("Failing - state=%d\n", m->state));
-            rc = setStateError();
-            break;
-        }
-    }
-
-    return rc;
+    return lockRead(aState, false /* fWithinQueryInfo */);
 }
 
@@ -2160 +2111 @@
         case MediumState_LockedRead:
         {
-            Assert(m->readers != 0);
+            ComAssertMsgBreak(m->readers != 0, ("Counter underflow"), rc = E_FAIL);
             --m->readers;
 
@@ -2203 +2154 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    /* Wait for a concurrently running queryInfo() to complete */
-    while (m->queryInfoRunning)
-    {
-        alock.leave();
-        {
-            AutoReadLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
-        }
-        alock.enter();
-    }
-
-    /* return the current state before */
-    if (aState)
-        *aState = m->state;
-
-    HRESULT rc = S_OK;
-
-    switch (m->state)
-    {
-        case MediumState_Created:
-        case MediumState_Inaccessible:
-        {
-            m->preLockState = m->state;
-
-            LogFlowThisFunc(("Okay - prev state=%d locationFull=%s\n", m->state, getLocationFull().c_str()));
-            m->state = MediumState_LockedWrite;
-            break;
-        }
-        default:
-        {
-            LogFlowThisFunc(("Failing - state=%d locationFull=%s\n", m->state, getLocationFull().c_str()));
-            rc = setStateError();
-            break;
-        }
-    }
-
-    return rc;
+    return lockWrite(aState, false /* fWithinQueryInfo */);
 }
 
@@ -2557 +2471 @@
     /* Apply the normal locking logic to the entire chain. */
     MediumLockList *pMediumLockList(new MediumLockList());
+    alock.release();
     HRESULT rc = diff->createMediumLockList(true /* fFailIfInaccessible */,
                                             true /* fMediumLockWrite */,
                                             this,
                                             *pMediumLockList);
+    alock.acquire();
     if (FAILED(rc))
     {
@@ -2567 +2483 @@
     }
 
+    alock.release();
     rc = pMediumLockList->Lock();
+    alock.acquire();
     if (FAILED(rc))
     {
@@ -2675 +2593 @@
         /* Build the source lock list. */
         MediumLockList *pSourceMediumLockList(new MediumLockList());
+        alock.release();
         rc = createMediumLockList(true /* fFailIfInaccessible */,
                                   false /* fMediumLockWrite */,
                                   NULL,
                                   *pSourceMediumLockList);
+        alock.acquire();
         if (FAILED(rc))
         {
@@ -2687 +2607 @@
         /* Build the target lock list (including the to-be parent chain). */
         MediumLockList *pTargetMediumLockList(new MediumLockList());
+        alock.release();
         rc = pTarget->createMediumLockList(true /* fFailIfInaccessible */,
                                            true /* fMediumLockWrite */,
                                            pParent,
                                            *pTargetMediumLockList);
+        alock.acquire();
         if (FAILED(rc))
         {
@@ -2698 +2620 @@
         }
 
+        alock.release();
         rc = pSourceMediumLockList->Lock();
+        alock.acquire();
         if (FAILED(rc))
         {
@@ -2707 +2631 @@
                            getLocationFull().c_str());
         }
+        alock.release();
         rc = pTargetMediumLockList->Lock();
+        alock.acquire();
         if (FAILED(rc))
         {
@@ -2770 +2696 @@
     try
     {
-        /* We need to lock both the current object, and the tree lock (would
-         * cause a lock order violation otherwise) for createMediumLockList. */
-        AutoMultiWriteLock2 multilock(&m->pVirtualBox->getMediaTreeLockHandle(),
-                                      this->lockHandle()
-                                      COMMA_LOCKVAL_SRC_POS);
+        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
         /* Build the medium lock list. */
         MediumLockList *pMediumLockList(new MediumLockList());
+        alock.release();
         rc = createMediumLockList(true /* fFailIfInaccessible */ ,
                                   true /* fMediumLockWrite */,
                                   NULL,
                                   *pMediumLockList);
+        alock.acquire();
         if (FAILED(rc))
         {
@@ -2788 +2712 @@
         }
 
+        alock.release();
         rc = pMediumLockList->Lock();
+        alock.acquire();
         if (FAILED(rc))
         {
@@ -2843 +2769 @@
     try
     {
-        /* We need to lock both the current object, and the tree lock (would
-         * cause a lock order violation otherwise) for createMediumLockList. */
-        AutoMultiWriteLock2 multilock(&m->pVirtualBox->getMediaTreeLockHandle(),
-                                      this->lockHandle()
-                                      COMMA_LOCKVAL_SRC_POS);
+        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
         /* Build the medium lock list. */
         MediumLockList *pMediumLockList(new MediumLockList());
+        alock.release();
         rc = createMediumLockList(true /* fFailIfInaccessible */ ,
                                   true /* fMediumLockWrite */,
                                   NULL,
                                   *pMediumLockList);
+        alock.acquire();
         if (FAILED(rc))
         {
@@ -2861 +2785 @@
         }
 
+        alock.release();
         rc = pMediumLockList->Lock();
+        alock.acquire();
         if (FAILED(rc))
         {
@@ -2934 +2860 @@
         /* Build the medium lock list. */
         MediumLockList *pMediumLockList(new MediumLockList());
+        multilock.release();
         rc = createMediumLockList(true /* fFailIfInaccessible */,
                                   true /* fMediumLockWrite */,
                                   NULL,
                                   *pMediumLockList);
+        multilock.acquire();
         if (FAILED(rc))
         {
@@ -2944 +2872 @@
         }
 
-        /* Temporary leave this lock, cause IMedium::LockWrite, will wait for
-         * an running IMedium::queryInfo. If there is one running it might be
-         * it tries to acquire a MediaTreeLock as well -> dead-lock. */
-        multilock.leave();
+        multilock.release();
         rc = pMediumLockList->Lock();
-        multilock.enter();
+        multilock.acquire();
         if (FAILED(rc))
         {
@@ -3775 +3700 @@
  * Constructs a medium lock list for this medium. The lock is not taken.
  *
- * @note Caller must lock the medium tree for writing.
+ * @note Caller MUST NOT hold the media tree or medium lock.
  *
  * @param fFailIfInaccessible If true, this fails with an error if a medium is inaccessible. If false,
@@ -3789 +3714 @@
                                      MediumLockList &mediumLockList)
 {
-    // Medium::queryInfo needs write lock
-    Assert(m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+    Assert(!m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+    Assert(!isWriteLockOnCurrentThread());
 
     AutoCaller autoCaller(this);
@@ -3811 +3736 @@
     while (!pMedium.isNull())
     {
-        // need write lock for queryInfo if medium is inaccessible
-        AutoWriteLock alock(pMedium COMMA_LOCKVAL_SRC_POS);
+        AutoReadLock alock(pMedium COMMA_LOCKVAL_SRC_POS);
 
         /* Accessibility check must be first, otherwise locking interferes
@@ -3820 +3744 @@
         if (mediumState == MediumState_Inaccessible)
         {
+            alock.release();
             rc = pMedium->queryInfo(false /* fSetImageId */, false /* fSetParentId */);
+            alock.acquire();
             if (FAILED(rc)) return rc;
 
@@ -4081 +4007 @@
     }
 
-    // leave the AutoCaller, as otherwise uninit() will simply hang
+    // release the AutoCaller, as otherwise uninit() will simply hang
     autoCaller.release();
 
@@ -4200 +4126 @@
         /* Build the medium lock list. */
         MediumLockList *pMediumLockList(new MediumLockList());
+        multilock.release();
         rc = createMediumLockList(true /* fFailIfInaccessible */,
                                   true /* fMediumLockWrite */,
                                   NULL,
                                   *pMediumLockList);
+        multilock.acquire();
         if (FAILED(rc))
         {
@@ -4210 +4138 @@
         }
 
+        multilock.release();
         rc = pMediumLockList->Lock();
+        multilock.acquire();
         if (FAILED(rc))
         {
@@ -4292 +4222 @@
 HRESULT Medium::markForDeletion()
 {
-    ComAssertRet(this->lockHandle()->isWriteLockOnCurrentThread(), E_FAIL);
+    ComAssertRet(isWriteLockOnCurrentThread(), E_FAIL);
     switch (m->state)
     {
@@ -4312 +4242 @@
 HRESULT Medium::unmarkForDeletion()
 {
-    ComAssertRet(this->lockHandle()->isWriteLockOnCurrentThread(), E_FAIL);
+    ComAssertRet(isWriteLockOnCurrentThread(), E_FAIL);
     switch (m->state)
     {
@@ -4330 +4260 @@
 HRESULT Medium::markLockedForDeletion()
 {
-    ComAssertRet(this->lockHandle()->isWriteLockOnCurrentThread(), E_FAIL);
+    ComAssertRet(isWriteLockOnCurrentThread(), E_FAIL);
     if (   (   m->state == MediumState_LockedRead
             || m->state == MediumState_LockedWrite)
@@ -4349 +4279 @@
 HRESULT Medium::unmarkLockedForDeletion()
 {
-    ComAssertRet(this->lockHandle()->isWriteLockOnCurrentThread(), E_FAIL);
+    ComAssertRet(isWriteLockOnCurrentThread(), E_FAIL);
     if (   (   m->state == MediumState_LockedRead
             || m->state == MediumState_LockedWrite)
@@ -4448 +4378 @@
         /* Build the lock list. */
         aMediumLockList = new MediumLockList();
+        treeLock.release();
         if (fMergeForward)
             rc = pTarget->createMediumLockList(true /* fFailIfInaccessible */,
@@ -4458 +4389 @@
                                       NULL,
                                       *aMediumLockList);
+        treeLock.acquire();
         if (FAILED(rc))
             throw rc;
@@ -4554 +4486 @@
 
         /* Update medium states appropriately */
-        if (m->state == MediumState_Created)
-        {
-            rc = markForDeletion();
-            if (FAILED(rc))
-                throw rc;
-        }
-        else
-        {
-            if (fLockMedia)
-                throw setStateError();
-            else if (   m->state == MediumState_LockedWrite
-                     || m->state == MediumState_LockedRead)
+        {
+            AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+            if (m->state == MediumState_Created)
             {
-                /* Either mark it for deletion in locked state or allow
-                 * others to have done so. */
-                if (m->preLockState == MediumState_Created)
-                    markLockedForDeletion();
-                else if (m->preLockState != MediumState_Deleting)
+                rc = markForDeletion();
+                if (FAILED(rc))
+                    throw rc;
+            }
+            else
+            {
+                if (fLockMedia)
+                    throw setStateError();
+                else if (   m->state == MediumState_LockedWrite
+                         || m->state == MediumState_LockedRead)
+                {
+                    /* Either mark it for deletion in locked state or allow
+                     * others to have done so. */
+                    if (m->preLockState == MediumState_Created)
+                        markLockedForDeletion();
+                    else if (m->preLockState != MediumState_Deleting)
+                        throw setStateError();
+                }
+                else
                     throw setStateError();
             }
-            else
-                throw setStateError();
         }
 
@@ -4581 +4517 @@
         {
             /* we will need parent to reparent target */
-            pParentForTarget = m->pParent;
+            pParentForTarget = getParent();
         }
         else
@@ -4641 +4577 @@
         if (fLockMedia)
         {
+            treeLock.release();
             rc = aMediumLockList->Lock();
+            treeLock.acquire();
             if (FAILED(rc))
             {
@@ -4885 +4823 @@
 HRESULT Medium::fixParentUuidOfChildren(const MediaList &childrenToReparent)
 {
+    Assert(!isWriteLockOnCurrentThread());
+    Assert(!m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
     MediumLockList mediumLockList;
     HRESULT rc = createMediumLockList(true /* fFailIfInaccessible */,
@@ -4994 +4934 @@
     try
     {
-        // locking: we need the tree lock first because we access parent pointers
-        // and we need to write-lock the media involved
-        AutoMultiWriteLock2 alock(&m->pVirtualBox->getMediaTreeLockHandle(),
-                                  this->lockHandle() COMMA_LOCKVAL_SRC_POS);
+        // This needs no extra locks besides what is done in the called methods.
 
         /* Build the source lock list. */
@@ -5091 +5028 @@
         /* Build the target lock list. */
         MediumLockList *pTargetMediumLockList(new MediumLockList());
+        alock.release();
         rc = createMediumLockList(true /* fFailIfInaccessible */,
                                   true /* fMediumLockWrite */,
                                   aParent,
                                   *pTargetMediumLockList);
+        alock.acquire();
         if (FAILED(rc))
         {
@@ -5101 +5040 @@
         }
 
+        alock.release();
         rc = pTargetMediumLockList->Lock();
+        alock.acquire();
         if (FAILED(rc))
         {
@@ -5186 +5127 @@
         /* Build the source lock list. */
         MediumLockList *pSourceMediumLockList(new MediumLockList());
+        alock.release();
         rc = createMediumLockList(true /* fFailIfInaccessible */,
                                   false /* fMediumLockWrite */,
                                   NULL,
                                   *pSourceMediumLockList);
+        alock.acquire();
         if (FAILED(rc))
         {
@@ -5198 +5141 @@
         /* Build the target lock list (including the to-be parent chain). */
         MediumLockList *pTargetMediumLockList(new MediumLockList());
+        alock.release();
         rc = aTarget->createMediumLockList(true /* fFailIfInaccessible */,
                                            true /* fMediumLockWrite */,
                                            aParent,
                                            *pTargetMediumLockList);
+        alock.acquire();
         if (FAILED(rc))
         {
@@ -5209 +5154 @@
         }
 
+        alock.release();
         rc = pSourceMediumLockList->Lock();
+        alock.acquire();
         if (FAILED(rc))
         {
@@ -5218 +5165 @@
                            getLocationFull().c_str());
         }
+        alock.release();
         rc = pTargetMediumLockList->Lock();
+        alock.acquire();
         if (FAILED(rc))
         {
@@ -5276 +5225 @@
 
 /**
+ * @note @a aState may be NULL if the state value is not needed (only for
+ *       in-process calls).
+ */
+HRESULT Medium::lockRead(MediumState_T *aState, bool fWithinQueryInfo)
+{
+    /* Must not hold the object lock, as we need control over it below. */
+    Assert(!isWriteLockOnCurrentThread());
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    /* Wait for a concurrently running Medium::queryInfo to complete. */
+    if (m->queryInfoRunning)
+    {
+        /* Must not hold the media tree lock, as Medium::queryInfo needs this
+         * lock and thus we would run into a deadlock here. */
+        if (!fWithinQueryInfo)
+            Assert(!m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+        while (m->queryInfoRunning)
+        {
+            alock.release();
+            {
+                AutoReadLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
+            }
+            alock.acquire();
+        }
+    }
+
+    /* return the current state before */
+    if (aState)
+        *aState = m->state;
+
+    HRESULT rc = S_OK;
+
+    switch (m->state)
+    {
+        case MediumState_Created:
+        case MediumState_Inaccessible:
+        case MediumState_LockedRead:
+        {
+            ++m->readers;
+
+            ComAssertMsgBreak(m->readers != 0, ("Counter overflow"), rc = E_FAIL);
+
+            /* Remember pre-lock state */
+            if (m->state != MediumState_LockedRead)
+                m->preLockState = m->state;
+
+            LogFlowThisFunc(("Okay - prev state=%d readers=%d\n", m->state, m->readers));
+            m->state = MediumState_LockedRead;
+
+            break;
+        }
+        default:
+        {
+            LogFlowThisFunc(("Failing - state=%d\n", m->state));
+            rc = setStateError();
+            break;
+        }
+    }
+
+    return rc;
+}
+
+/**
+ * @note @a aState may be NULL if the state value is not needed (only for
+ *       in-process calls).
+ */
+HRESULT Medium::lockWrite(MediumState_T *aState, bool fWithinQueryInfo)
+{
+    /* Must not hold the object lock, as we need control over it below. */
+    Assert(!isWriteLockOnCurrentThread());
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    /* Wait for a concurrently running Medium::queryInfo to complete. */
+    if (m->queryInfoRunning)
+    {
+        /* Must not hold the media tree lock, as Medium::queryInfo needs this
+         * lock and thus we would run into a deadlock here. */
+        if (!fWithinQueryInfo)
+            Assert(!m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+        while (m->queryInfoRunning)
+        {
+            alock.release();
+            {
+                AutoReadLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
+            }
+            alock.acquire();
+        }
+    }
+
+    /* return the current state before */
+    if (aState)
+        *aState = m->state;
+
+    HRESULT rc = S_OK;
+
+    switch (m->state)
+    {
+        case MediumState_Created:
+        case MediumState_Inaccessible:
+        {
+            m->preLockState = m->state;
+
+            LogFlowThisFunc(("Okay - prev state=%d locationFull=%s\n", m->state, getLocationFull().c_str()));
+            m->state = MediumState_LockedWrite;
+            break;
+        }
+        default:
+        {
+            LogFlowThisFunc(("Failing - state=%d locationFull=%s\n", m->state, getLocationFull().c_str()));
+            rc = setStateError();
+            break;
+        }
+    }
+
+    return rc;
+}
+
+/**
  * Queries information from the medium.
  *
@@ -5284 +5351 @@
  *       accessibility.
  *
- * @note Caller must hold medium tree for writing.
+ * @note Caller MUST NOT hold the media tree or medium lock.
  *
  * @note Locks mParent for reading. Locks this object for writing.
@@ -5294 +5361 @@
 HRESULT Medium::queryInfo(bool fSetImageId, bool fSetParentId)
 {
-    Assert(m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+    Assert(!isWriteLockOnCurrentThread());
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
@@ -5315 +5382 @@
         while (m->queryInfoRunning)
         {
-            alock.leave();
+            alock.release();
             {
                 AutoReadLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
             }
-            alock.enter();
+            alock.acquire();
         }
 
         return S_OK;
     }
+
+    alock.release();
+    Assert(!isWriteLockOnCurrentThread());
+    Assert(!m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+    AutoWriteLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    alock.acquire();
 
     bool success = false;
@@ -5349 +5422 @@
 
     /* Lock the medium, which makes the behavior much more consistent */
+    alock.release();
     if (uOpenFlags & (VD_OPEN_FLAGS_READONLY | VD_OPEN_FLAGS_SHAREABLE))
-        rc = LockRead(NULL);
+        rc = lockRead(NULL, true /* fWithinQueryInfo */);
     else
-        rc = LockWrite(NULL);
+        rc = lockWrite(NULL, true /* fWithinQueryInfo */);
     if (FAILED(rc)) return rc;
+    alock.acquire();
 
     /* Copies of the input state fields which are not read-only,
@@ -5373 +5448 @@
     bool fRepairImageZeroParentUuid = false;
 
-    /* leave the object lock before a lengthy operation */
+    /* release the object lock before a lengthy operation */
     m->queryInfoRunning = true;
-    alock.leave();
+    alock.release();
+    treeLock.release();
+    Assert(!isWriteLockOnCurrentThread());
+    Assert(!m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+
     /* Note that taking the queryInfoSem after leaving the object lock above
      * can lead to short spinning of the loops waiting for queryInfo() to
@@ -5520 +5599 @@
 
                     /* we set mParent & children() */
-                    AutoWriteLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+                    treeLock.acquire();
 
                     Assert(m->pParent.isNull());
                     m->pParent = pParent;
                     m->pParent->m->llChildren.push_back(this);
+
+                    treeLock.release();
                 }
                 else
                 {
                     /* we access mParent */
-                    AutoReadLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+                    treeLock.acquire();
 
                     /* check that parent UUIDs match. Note that there's no need
@@ -5573 +5654 @@
                     /// accessible while the diff is? Probably nothing. The
                     /// real code will detect the mismatch anyway.
+
+                    treeLock.release();
                 }
             }
@@ -5593 +5676 @@
     }
 
-    alock.enter();
+    treeLock.acquire();
+    alock.acquire();
 
     if (isImport || fSetImageId)
@@ -5642 +5726 @@
         if (FAILED(rc)) return rc;
 
-        alock.leave();
+        alock.release();
 
         try
@@ -5677 +5761 @@
         }
 
-        alock.enter();
+        alock.acquire();
 
         rc = UnlockWrite(NULL);
@@ -5712 +5796 @@
  * Unregisters this medium with mVirtualBox. Called by close() under the medium tree lock.
  *
- * This calls either VirtualBox::unregisterImage or VirtualBox::unregisterHardDisk depending
- * on the device type of this medium.
- *
  * @param pllRegistriesThatNeedSaving Optional pointer to a list of UUIDs that will receive the registry IDs that need saving.
  *
@@ -5732 +5813 @@
         deparent();
 
-    HRESULT rc = E_FAIL;
-    switch (m->devType)
-    {
-        case DeviceType_DVD:
-        case DeviceType_Floppy:
-            rc = m->pVirtualBox->unregisterImage(this,
-                                                 m->devType,
-                                                 pllRegistriesThatNeedSaving);
-        break;
-
-        case DeviceType_HardDisk:
-            rc = m->pVirtualBox->unregisterHardDisk(this, pllRegistriesThatNeedSaving);
-        break;
-
-        default:
-        break;
-    }
-
+    HRESULT rc = m->pVirtualBox->unregisterMedium(this, pllRegistriesThatNeedSaving);
     if (FAILED(rc))
     {
@@ -6429 +6493 @@
         {
             id.create();
-            /* VirtualBox::registerHardDisk() will need UUID */
+            /* VirtualBox::registerMedium() will need UUID */
             unconst(m->id) = id;
         }
@@ -6495 +6559 @@
          * better than breaking media registry consistency) */
         AutoWriteLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-        rc = m->pVirtualBox->registerHardDisk(this, NULL /* pllRegistriesThatNeedSaving */);
-    }
-
-    // reenter the lock before changing state
+        ComObjPtr<Medium> pMedium;
+        rc = m->pVirtualBox->registerMedium(this, &pMedium, DeviceType_HardDisk,
+                                            NULL /* pllRegistriesThatNeedSaving */);
+        Assert(this == pMedium);
+    }
+
+    // re-acquire the lock before changing state
     AutoWriteLock thisLock(this COMMA_LOCKVAL_SRC_POS);
 
@@ -6558 +6625 @@
         {
             targetId.create();
-            /* VirtualBox::registerHardDisk() will need UUID */
+            /* VirtualBox::registerMedium() will need UUID */
             unconst(pTarget->m->id) = targetId;
         }
@@ -6672 +6739 @@
          * Created state only on success (leaving an orphan file is
          * better than breaking media registry consistency) */
-        mrc = m->pVirtualBox->registerHardDisk(pTarget, &llRegistriesThatNeedSaving);
+        ComObjPtr<Medium> pMedium;
+        mrc = m->pVirtualBox->registerMedium(pTarget, &pMedium, DeviceType_HardDisk,
+                                             &llRegistriesThatNeedSaving);
+        Assert(pTarget == pMedium);
 
         if (FAILED(mrc))
@@ -6891 +6961 @@
             /* first, unregister the target since it may become a base
              * medium which needs re-registration */
-            rc2 = m->pVirtualBox->unregisterHardDisk(pTarget, NULL /*&fNeedsGlobalSaveSettings*/);
+            rc2 = m->pVirtualBox->unregisterMedium(pTarget, NULL /*pfNeedsGlobalSaveSettings*/);
             AssertComRC(rc2);
 
@@ -6905 +6975 @@
 
             /* then, register again */
-            rc2 = m->pVirtualBox->registerHardDisk(pTarget, NULL /* pllRegistriesThatNeedSaving */ );
+            ComObjPtr<Medium> pMedium;
+            rc2 = m->pVirtualBox->registerMedium(pTarget, &pMedium,
+                                                 DeviceType_HardDisk,
+                                                 NULL /* pllRegistriesThatNeedSaving */ );
             AssertComRC(rc2);
         }
@@ -6958 +7031 @@
             }
 
-            rc2 = pMedium->m->pVirtualBox->unregisterHardDisk(pMedium,
-                                                              NULL /*pfNeedsGlobalSaveSettings*/);
+            rc2 = pMedium->m->pVirtualBox->unregisterMedium(pMedium,
+                                                            NULL /*pfNeedsGlobalSaveSettings*/);
             AssertComRC(rc2);
 
@@ -7254 +7327 @@
              * better than breaking media registry consistency) */
             eik.restore();
-            mrc = pParent->m->pVirtualBox->registerHardDisk(pTarget, NULL /* pllRegistriesThatNeedSaving */);
+            ComObjPtr<Medium> pMedium;
+            mrc = pParent->m->pVirtualBox->registerMedium(pTarget, &pMedium,
+                                                          DeviceType_HardDisk,
+                                                          NULL /* pllRegistriesThatNeedSaving */);
+            Assert(pTarget == pMedium);
             eik.fetch();
 
@@ -7265 +7342 @@
             /* just register  */
             eik.restore();
-            mrc = m->pVirtualBox->registerHardDisk(pTarget, NULL /* pllRegistriesThatNeedSaving */);
+            ComObjPtr<Medium> pMedium;
+            mrc = m->pVirtualBox->registerMedium(pTarget, &pMedium,
+                                                 DeviceType_HardDisk,
+                                                 NULL /* pllRegistriesThatNeedSaving */);
+            Assert(pTarget == pMedium);
             eik.fetch();
         }
@@ -8029 +8110 @@
              * better than breaking media registry consistency) */
             eik.restore();
-            mrc = pParent->m->pVirtualBox->registerHardDisk(this, NULL /* llRegistriesThatNeedSaving */);
+            ComObjPtr<Medium> pMedium;
+            mrc = pParent->m->pVirtualBox->registerMedium(this, &pMedium,
+                                                          DeviceType_HardDisk,
+                                                          NULL /* llRegistriesThatNeedSaving */);
+            Assert(this == pMedium);
             eik.fetch();
 
@@ -8040 +8125 @@
             /* just register  */
             eik.restore();
-            mrc = m->pVirtualBox->registerHardDisk(this, NULL /* pllRegistriesThatNeedSaving */);
+            ComObjPtr<Medium> pMedium;
+            mrc = m->pVirtualBox->registerMedium(this, &pMedium, DeviceType_HardDisk,
+                                                 NULL /* pllRegistriesThatNeedSaving */);
+            Assert(this == pMedium);
             eik.fetch();
         }

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

@@ -6 +6 @@
 
 /*
- * Copyright (C) 2006-2011 Oracle Corporation
+ * Copyright (C) 2006-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -341 +341 @@
     {
         m->strName = strName;
-        alock.leave(); /* Important! (child->parent locks are forbidden) */
+        alock.release(); /* Important! (child->parent locks are forbidden) */
         rc = m->pMachine->onSnapshotChange(this);
     }
@@ -374 +374 @@
     {
         m->strDescription = strDescription;
-        alock.leave(); /* Important! (child->parent locks are forbidden) */
+        alock.release(); /* Important! (child->parent locks are forbidden) */
         rc = m->pMachine->onSnapshotChange(this);
     }
@@ -1232 +1232 @@
     HRESULT rc = mPeer->saveSettings(&fNeedsGlobalSaveSettings,
                                      SaveS_Force);        // we know we need saving, no need to check
-    mlock.leave();
+    mlock.release();
 
     if (SUCCEEDED(rc) && fNeedsGlobalSaveSettings)
@@ -1834 +1834 @@
             }
 
-            /* leave the locks before the potentially lengthy operation */
+            /* release the locks before the potentially lengthy operation */
             snapshotLock.release();
-            alock.leave();
+            alock.release();
 
             rc = createImplicitDiffs(aTask.pProgress,
@@ -1845 +1845 @@
                 throw rc;
 
-            alock.enter();
+            alock.acquire();
             snapshotLock.acquire();
 
@@ -1951 +1951 @@
 
         // let go of the locks while we're deleting image files below
-        alock.leave();
+        alock.release();
         // from here on we cannot roll back on failure any more
 
@@ -2302 +2302 @@
     {
         /* Locking order:  */
-        AutoMultiWriteLock3 multiLock(this->lockHandle(),                   // machine
-                                      aTask.pSnapshot->lockHandle(),        // snapshot
-                                      &mParent->getMediaTreeLockHandle()    // media tree
+        AutoMultiWriteLock2 multiLock(this->lockHandle(),                   // machine
+                                      aTask.pSnapshot->lockHandle()         // snapshot
                                       COMMA_LOCKVAL_SRC_POS);
-            // once we have this lock, we know that SessionMachine::DeleteSnapshot()
-            // has exited after setting the machine state to MachineState_DeletingSnapshot
+        // once we have this lock, we know that SessionMachine::DeleteSnapshot()
+        // has exited after setting the machine state to MachineState_DeletingSnapshot
+
+        AutoWriteLock treeLock(mParent->getMediaTreeLockHandle()
+                                      COMMA_LOCKVAL_SRC_POS);
 
         ComObjPtr<SnapshotMachine> pSnapMachine = aTask.pSnapshot->getSnapshotMachine();
@@ -2384 +2386 @@
                     fOnlineMergePossible = false;
             }
+
+            // no need to hold the lock any longer
+            attachLock.release();
+
+            treeLock.release();
             rc = prepareDeleteSnapshotMedium(pHD, machineId, snapshotId,
                                              fOnlineMergePossible,
@@ -2391 +2398 @@
                                              fNeedsOnlineMerge,
                                              pMediumLockList);
+            treeLock.acquire();
             if (FAILED(rc))
                 throw rc;
-
-            // no need to hold the lock any longer
-            attachLock.release();
 
             // For simplicity, prepareDeleteSnapshotMedium selects the merge
@@ -2465 +2470 @@
         }
 
-        // we can release the lock now since the machine state is MachineState_DeletingSnapshot
+        // we can release the locks now since the machine state is MachineState_DeletingSnapshot
+        treeLock.release();
         multiLock.release();
 
@@ -2786 +2792 @@
                                                     MediumLockList * &aMediumLockList)
 {
-    Assert(mParent->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+    Assert(!mParent->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
     Assert(!fOnlineMergePossible || VALID_PTR(aVMMALockList));
 
@@ -2812 +2818 @@
             /* lock only, to prevent any usage until the snapshot deletion
              * is completed */
+            alock.release();
             return aHD->LockWrite(NULL);
         }
@@ -2829 +2836 @@
     ComObjPtr<Medium> pChild = aHD->getChildren().front();
 
-    /* we keep this locked, so lock the affected child to make sure the lock
-     * order is correct when calling prepareMergeTo() */
     AutoWriteLock childLock(pChild COMMA_LOCKVAL_SRC_POS);
 
@@ -2843 +2848 @@
             /* backward merge is too tricky, we'll just detach on snapshot
              * deletion, so lock only, to prevent any usage */
+            childLock.release();
+            alock.release();
             return aHD->LockWrite(NULL);
         }
@@ -2857 +2864 @@
 
     HRESULT rc;
+    childLock.release();
+    alock.release();
     rc = aSource->prepareMergeTo(aTarget, &aMachineId, &aSnapshotId,
                                  !fOnlineMergePossible /* fLockMedia */,
                                  aMergeForward, aParentForTarget,
                                  aChildrenToReparent, aMediumLockList);
+    alock.acquire();
+    childLock.acquire();
     if (SUCCEEDED(rc) && fOnlineMergePossible)
     {
@@ -2867 +2878 @@
          * asking the VM to do the merging. Only continue with the online
          * merging preparation if applicable. */
+        childLock.release();
+        alock.release();
         rc = aMediumLockList->Lock();
+        alock.acquire();
+        childLock.acquire();
         if (FAILED(rc) && fOnlineMergePossible)
         {
@@ -2895 +2910 @@
                 }
                 bool fLockReq = (it2->GetLockRequest() || it->GetLockRequest());
+                childLock.release();
+                alock.release();
                 rc = it->UpdateLock(fLockReq);
+                alock.acquire();
+                childLock.acquire();
                 if (FAILED(rc))
                 {
@@ -2912 +2931 @@
                 {
                     ComObjPtr<Medium> pMedium = *it;
+                    AutoReadLock mediumLock(pMedium COMMA_LOCKVAL_SRC_POS);
                     if (pMedium->getState() == MediumState_Created)
                     {
+                        mediumLock.release();
+                        childLock.release();
+                        alock.release();
                         rc = pMedium->LockWrite(NULL);
+                        alock.acquire();
+                        childLock.acquire();
+                        mediumLock.acquire();
                         if (FAILED(rc))
                             throw rc;
@@ -2920 +2946 @@
                     else
                     {
+                        mediumLock.release();
+                        childLock.release();
+                        alock.release();
                         rc = aVMMALockList->Update(pMedium, true);
+                        alock.acquire();
+                        childLock.acquire();
+                        mediumLock.acquire();
                         if (FAILED(rc))
                         {
+                            mediumLock.release();
+                            childLock.release();
+                            alock.release();
                             rc = pMedium->LockWrite(NULL);
+                            alock.acquire();
+                            childLock.acquire();
+                            mediumLock.acquire();
                             if (FAILED(rc))
-                            throw rc;
+                                throw rc;
                         }
                     }
@@ -2933 +2971 @@
             if (fOnlineMergePossible)
             {
+                childLock.release();
+                alock.release();
                 rc = aVMMALockList->Lock();
+                alock.acquire();
+                childLock.acquire();
                 if (FAILED(rc))
                 {
@@ -2967 +3009 @@
                      ++it)
                 {
+                    childLock.release();
+                    alock.release();
                     it->UpdateLock(it == lockListLast);
+                    alock.acquire();
+                    childLock.acquire();
                     ComObjPtr<Medium> pMedium = it->GetMedium();
                     AutoWriteLock mediumLock(pMedium COMMA_LOCKVAL_SRC_POS);
@@ -3058 +3104 @@
                     pMedium->unmarkLockedForDeletion();
                 }
+                mediumLock.release();
                 it->UpdateLock(it == lockListLast);
+                mediumLock.acquire();
             }
         }
@@ -3222 +3270 @@
         // first, unregister the target since it may become a base
         // hard disk which needs re-registration
-        rc = mParent->unregisterHardDisk(pTarget, NULL /*&fNeedsGlobalSaveSettings*/);
+        rc = mParent->unregisterMedium(pTarget, NULL /*&fNeedsGlobalSaveSettings*/);
         AssertComRC(rc);
 
@@ -3233 +3281 @@
 
         // then, register again
-        rc = mParent->registerHardDisk(pTarget, NULL /* pllRegistriesThatNeedSaving */);
+        rc = mParent->registerMedium(pTarget, &pTarget, DeviceType_HardDisk,
+                                     NULL /* pllRegistriesThatNeedSaving */);
         AssertComRC(rc);
     }
@@ -3261 +3310 @@
                 toReparent.push_back(pMedium);
             }
+            treeLock.release();
             pTarget->fixParentUuidOfChildren(toReparent);
+            treeLock.acquire();
 
             // obey {parent,child} lock order
@@ -3304 +3355 @@
         else
         {
-            rc = mParent->unregisterHardDisk(pMedium,
-                                             NULL /*pfNeedsGlobalSaveSettings*/);
+            rc = mParent->unregisterMedium(pMedium, NULL /*pfNeedsGlobalSaveSettings*/);
             AssertComRC(rc);
 

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

@@ -628 +628 @@
         if (FAILED(rc)) return rc;
 
-        rc = registerHardDisk(pHardDisk, NULL /* pllRegistriesThatNeedSaving */);
+        rc = registerMedium(pHardDisk, &pHardDisk, DeviceType_HardDisk,
+                            NULL /* pllRegistriesThatNeedSaving */);
         if (FAILED(rc)) return rc;
     }
@@ -648 +649 @@
         if (FAILED(rc)) return rc;
 
-        rc = registerImage(pImage,
-                           DeviceType_DVD,
-                           NULL /* pllRegistriesThatNeedSaving */);
+        rc = registerMedium(pImage, &pImage, DeviceType_DVD,
+                            NULL /* pllRegistriesThatNeedSaving */);
         if (FAILED(rc)) return rc;
     }
@@ -670 +670 @@
         if (FAILED(rc)) return rc;
 
-        rc = registerImage(pImage,
-                           DeviceType_Floppy,
-                           NULL /* pllRegistriesThatNeedSaving */);
+        rc = registerMedium(pImage, &pImage, DeviceType_Floppy,
+                            NULL /* pllRegistriesThatNeedSaving */);
         if (FAILED(rc)) return rc;
     }
@@ -1713 +1712 @@
     {
         pMedium.createObject();
+        treeLock.release();
         rc = pMedium->init(this,
                            aLocation,
@@ -1718 +1718 @@
                            fForceNewUuid,
                            deviceType);
+        treeLock.acquire();
 
         if (SUCCEEDED(rc))
         {
-            switch (deviceType)
-            {
-                case DeviceType_HardDisk:
-                    rc = registerHardDisk(pMedium, NULL /* pllRegistriesThatNeedSaving */);
-                break;
-
-                case DeviceType_DVD:
-                case DeviceType_Floppy:
-                    rc = registerImage(pMedium,
-                                       deviceType,
-                                       NULL /* pllRegistriesThatNeedSaving */);
-                break;
-            }
+            rc = registerMedium(pMedium, &pMedium, deviceType,
+                                NULL /* pllRegistriesThatNeedSaving */);
 
             treeLock.release();
@@ -3200 +3190 @@
  * @param aLocation     Location to check.
  * @param aConflict     Where to return parameters of the conflicting medium.
+ * @param ppMedium      Medium reference in case this is simply a duplicate.
  *
  * @note Locks the media tree and media objects for reading.
@@ -3206 +3197 @@
                                            const Utf8Str &aLocation,
                                            Utf8Str &aConflict,
-                                           bool &fIdentical)
-{
+                                           ComObjPtr<Medium> *ppMedium)
+{
+    AssertReturn(!aId.isEmpty() && !aLocation.isEmpty(), E_FAIL);
+    AssertReturn(!ppMedium, E_INVALIDARG);
+
     aConflict.setNull();
-
-    AssertReturn(!aId.isEmpty() && !aLocation.isEmpty(), E_FAIL);
+    ppMedium->setNull();
 
     AutoReadLock alock(getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     HRESULT rc = S_OK;
-
-    aConflict.setNull();
-    fIdentical = false;
 
     ComObjPtr<Medium> pMediumFound;
@@ -3254 +3244 @@
              && (idFound == aId)
            )
-            fIdentical = true;
+            *ppMedium = pMediumFound;
 
         aConflict = Utf8StrFmt(tr("%s '%s' with UUID {%RTuuid}"),
@@ -3542 +3532 @@
 
 /**
- * Remembers the given hard disk by storing it in either the global hard disk registry
- * or a machine one.
- *
- * @note Caller must hold the media tree lock for writing; in addition, this locks @a aHardDisk for reading
- *
- * @param aHardDisk Hard disk object to remember.
- * @param uuidMachineRegistry UUID of machine whose registry should be used, or a NULL UUID for the global registry.
+ * Remembers the given medium object by storing it in either the global
+ * medium registry or a machine one.
+ *
+ * @note Caller must hold the media tree lock for writing; in addition, this
+ * locks @a pMedium for reading
+ *
+ * @param pMedium   Hard disk object to remember.
+ * @param ppMedium  Actually stored hard disk object. Can be different if due
+ *                  to an unavoidable race there was a duplicate Medium object
+ *                  created.
+ * @param argType   Either DeviceType_DVD or DeviceType_Floppy.
  * @param pllRegistriesThatNeedSaving Optional pointer to a list of UUIDs of media registries that need saving.
  * @return
  */
-HRESULT VirtualBox::registerHardDisk(Medium *pMedium,
-                                     GuidList *pllRegistriesThatNeedSaving)
+HRESULT VirtualBox::registerMedium(Medium *pMedium,
+                                   ComObjPtr<Medium> *ppMedium,
+                                   DeviceType_T argType,
+                                   GuidList *pllRegistriesThatNeedSaving)
 {
     AssertReturn(pMedium != NULL, E_INVALIDARG);
+    AssertReturn(ppMedium != NULL, E_INVALIDARG);
 
     AutoCaller autoCaller(this);
     AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
 
-    AutoCaller hardDiskCaller(pMedium);
-    AssertComRCReturn(hardDiskCaller.rc(), hardDiskCaller.rc());
+    AutoCaller mediumCaller(pMedium);
+    AssertComRCReturn(mediumCaller.rc(), mediumCaller.rc());
+
+    const char *pszDevType = NULL;
+    ObjectsList<Medium> *pall = NULL;
+    switch (argType)
+    {
+        case DeviceType_HardDisk:
+            pall = &m->allHardDisks;
+            pszDevType = tr("hard disk");
+            break;
+        case DeviceType_DVD:
+            pszDevType = tr("DVD image");
+            pall = &m->allDVDImages;
+            break;
+        case DeviceType_Floppy:
+            pszDevType = tr("floppy image");
+            pall = &m->allFloppyImages;
+            break;
+        default:
+            AssertMsgFailedReturn(("invalid device type %d", argType), E_INVALIDARG);
+    }
 
     // caller must hold the media tree write lock
@@ -3570 +3587 @@
     ComObjPtr<Medium> pParent;
     {
-        AutoReadLock hardDiskLock(pMedium COMMA_LOCKVAL_SRC_POS);
+        AutoReadLock mediumLock(pMedium COMMA_LOCKVAL_SRC_POS);
         id = pMedium->getId();
         strLocationFull = pMedium->getLocationFull();
@@ -3579 +3596 @@
 
     Utf8Str strConflict;
-    bool fIdentical;
+    ComObjPtr<Medium> pDupMedium;
     rc = checkMediaForConflicts(id,
                                 strLocationFull,
                                 strConflict,
-                                fIdentical);
+                                &pDupMedium);
     if (FAILED(rc)) return rc;
 
-    if (!fIdentical)
+    if (pDupMedium.isNull())
     {
         if (strConflict.length())
             return setError(E_INVALIDARG,
-                            tr("Cannot register the hard disk '%s' {%RTuuid} because a %s already exists"),
+                            tr("Cannot register the %s '%s' {%RTuuid} because a %s already exists"),
+                            pszDevType,
                             strLocationFull.c_str(),
                             id.raw(),
@@ -3596 +3614 @@
                             m->strSettingsFilePath.c_str());
 
-        // store base (root) hard disks in the list
+        // add to the collection if it is a base medium
         if (pParent.isNull())
-            m->allHardDisks.getList().push_back(pMedium);
-                    // access the list directly because we already locked the list above
+            pall->getList().push_back(pMedium);
 
         // store all hard disks (even differencing images) in the map
-        m->mapHardDisks[id] = pMedium;
+        if (argType == DeviceType_HardDisk)
+            m->mapHardDisks[id] = pMedium;
 
         if (pllRegistriesThatNeedSaving)
             pMedium->addToRegistryIDList(*pllRegistriesThatNeedSaving);
     }
+    else
+        pMedium = pDupMedium;
+
+    *ppMedium = pMedium;
 
     return rc;
@@ -3612 +3634 @@
 
 /**
- * Removes the given hard disk from the hard disk registry.
- *
- * @param aHardDisk     Hard disk object to remove.
+ * Removes the given medium from the respective registry.
+ *
+ * @param pMedium    Hard disk object to remove.
  * @param pfNeedsGlobalSaveSettings Optional pointer to a bool that must have been initialized to false and that will be set to true
  *                by this function if the caller should invoke VirtualBox::saveSettings() because the global settings have changed.
  *
- * @note Caller must hold the media tree lock for writing; in addition, this locks @a aHardDisk for reading
- */
-HRESULT VirtualBox::unregisterHardDisk(Medium *aHardDisk,
-                                       GuidList *pllRegistriesThatNeedSaving)
-{
-    AssertReturn(aHardDisk != NULL, E_INVALIDARG);
+ * @note Caller must hold the media tree lock for writing; in addition, this locks @a pMedium for reading
+ */
+HRESULT VirtualBox::unregisterMedium(Medium *pMedium,
+                                     GuidList *pllRegistriesThatNeedSaving)
+{
+    AssertReturn(pMedium != NULL, E_INVALIDARG);
 
     AutoCaller autoCaller(this);
     AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
 
-    AutoCaller hardDiskCaller(aHardDisk);
-    AssertComRCReturn(hardDiskCaller.rc(), hardDiskCaller.rc());
+    AutoCaller mediumCaller(pMedium);
+    AssertComRCReturn(mediumCaller.rc(), mediumCaller.rc());
 
     // caller must hold the media tree write lock
@@ -3636 +3658 @@
     Guid id;
     ComObjPtr<Medium> pParent;
-    {
-        AutoReadLock hardDiskLock(aHardDisk COMMA_LOCKVAL_SRC_POS);
-        id = aHardDisk->getId();
-        pParent = aHardDisk->getParent();
-    }
-
-    // remove base (root) hard disks from the list
+    DeviceType_T devType;
+    {
+        AutoReadLock mediumLock(pMedium COMMA_LOCKVAL_SRC_POS);
+        id = pMedium->getId();
+        pParent = pMedium->getParent();
+        devType = pMedium->getDeviceType();
+    }
+
+    ObjectsList<Medium> *pall = NULL;
+    switch (devType)
+    {
+        case DeviceType_HardDisk:
+            pall = &m->allHardDisks;
+            break;
+        case DeviceType_DVD:
+            pall = &m->allDVDImages;
+            break;
+        case DeviceType_Floppy:
+            pall = &m->allFloppyImages;
+            break;
+        default:
+            AssertMsgFailedReturn(("invalid device type %d", devType), E_INVALIDARG);
+    }
+
+    // remove from the collection if it is a base medium
     if (pParent.isNull())
-        m->allHardDisks.getList().remove(aHardDisk);
-                // access the list directly because caller must have locked the list
+        pall->getList().remove(pMedium);
 
     // remove all hard disks (even differencing images) from map
-    size_t cnt = m->mapHardDisks.erase(id);
-    Assert(cnt == 1);
-    NOREF(cnt);
+    if (devType == DeviceType_HardDisk)
+    {
+        size_t cnt = m->mapHardDisks.erase(id);
+        Assert(cnt == 1);
+        NOREF(cnt);
+    }
 
     if (pllRegistriesThatNeedSaving)
-        aHardDisk->addToRegistryIDList(*pllRegistriesThatNeedSaving);
+        pMedium->addToRegistryIDList(*pllRegistriesThatNeedSaving);
 
     return S_OK;
-}
-
-/**
- * Remembers the given image by storing it in the CD/DVD or floppy image registry.
- *
- * @param argImage      Image object to remember.
- * @param argType       Either DeviceType_DVD or DeviceType_Floppy.
- * @param uuidMachineRegistry UUID of machine whose registry should be used, or a NULL UUID for the global registry.
- *
- * @note Caller must hold the media tree lock for writing; in addition, this locks @a argImage for reading
- */
-HRESULT VirtualBox::registerImage(Medium *pMedium,
-                                  DeviceType_T argType,
-                                  GuidList *pllRegistriesThatNeedSaving)
-{
-    AssertReturn(pMedium != NULL, E_INVALIDARG);
-
-    AutoCaller autoCaller(this);
-    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
-
-    AutoCaller imageCaller(pMedium);
-    AssertComRCReturn(imageCaller.rc(), imageCaller.rc());
-
-    // caller must hold the media tree write lock
-    Assert(getMediaTreeLockHandle().isWriteLockOnCurrentThread());
-
-    Guid id;
-    Utf8Str strLocationFull;
-    ComObjPtr<Medium> pParent;
-    {
-        AutoReadLock al(pMedium COMMA_LOCKVAL_SRC_POS);
-        id = pMedium->getId();
-        strLocationFull = pMedium->getLocationFull();
-        pParent = pMedium->getParent();
-    }
-
-    // work on DVDs or floppies list?
-    ObjectsList<Medium> &all = (argType == DeviceType_DVD) ? m->allDVDImages : m->allFloppyImages;
-
-    HRESULT rc;
-    // lock the images lists (list + map) while checking for conflicts
-    AutoWriteLock al(all.getLockHandle() COMMA_LOCKVAL_SRC_POS);
-
-    Utf8Str strConflict;
-    bool fIdentical;
-    rc = checkMediaForConflicts(id,
-                                strLocationFull,
-                                strConflict,
-                                fIdentical);
-    if (FAILED(rc)) return rc;
-
-    if (!fIdentical)
-    {
-        if (strConflict.length())
-            return setError(VBOX_E_INVALID_OBJECT_STATE,
-                            tr("Cannot register the image '%s' with UUID {%RTuuid} because a %s already exists"),
-                            strLocationFull.c_str(),
-                            id.raw(),
-                            strConflict.c_str());
-
-        // add to the collection
-        all.getList().push_back(pMedium);
-                // access the list directly because we already locked the list above
-
-        if (pllRegistriesThatNeedSaving)
-            pMedium->addToRegistryIDList(*pllRegistriesThatNeedSaving);
-    }
-
-    return rc;
-}
-
-/**
- * Removes the given image from the CD/DVD or floppy image registry.
- *
- * @param argImage        Image object to remove.
- * @param argType         Either DeviceType_DVD or DeviceType_Floppy.
- * @param pfNeedsGlobalSaveSettings Optional pointer to a bool that must have been initialized to false and that will be set to true
- *                by this function if the caller should invoke VirtualBox::saveSettings() because the global settings have changed.
- *
- * @note Caller must hold the media tree lock for writing; in addition, this locks @a argImage for reading
- */
-HRESULT VirtualBox::unregisterImage(Medium *argImage,
-                                    DeviceType_T argType,
-                                    GuidList *pllRegistriesThatNeedSaving)
-{
-    AssertReturn(argImage != NULL, E_INVALIDARG);
-
-    AutoCaller autoCaller(this);
-    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
-
-    AutoCaller imageCaller(argImage);
-    AssertComRCReturn(imageCaller.rc(), imageCaller.rc());
-
-    // caller must hold the media tree write lock
-    Assert(getMediaTreeLockHandle().isWriteLockOnCurrentThread());
-
-    Guid id;
-    ComObjPtr<Medium> pParent;
-    {
-        AutoReadLock al(argImage COMMA_LOCKVAL_SRC_POS);
-        id = argImage->getId();
-        pParent = argImage->getParent();
-    }
-
-    // work on DVDs or floppies list?
-    ObjectsList<Medium> &all = (argType == DeviceType_DVD) ? m->allDVDImages : m->allFloppyImages;
-
-    // access the list directly because the caller must have requested the lock
-    all.getList().remove(argImage);
-
-    HRESULT rc = S_OK;
-
-    if (pllRegistriesThatNeedSaving)
-        argImage->addToRegistryIDList(*pllRegistriesThatNeedSaving);
-
-    return rc;
 }
 
@@ -4742 +4668 @@
 
 /**
- * Remembers the given dhcp server by storing it in the hard disk registry.
- *
- * @param aDHCPServer     Dhcp Server object to remember.
- * @param aSaveRegistry @c true to save hard disk registry to disk (default).
- *
- * When @a aSaveRegistry is @c true, this operation may fail because of the
+ * Remembers the given DHCP server in the settings.
+ *
+ * @param aDHCPServer   DHCP server object to remember.
+ * @param aSaveSettings @c true to save settings to disk (default).
+ *
+ * When @a aSaveSettings is @c true, this operation may fail because of the
  * failed #saveSettings() method it calls. In this case, the dhcp server object
  * will not be remembered. It is therefore the responsibility of the caller to
@@ -4757 +4683 @@
  */
 HRESULT VirtualBox::registerDHCPServer(DHCPServer *aDHCPServer,
-                                       bool aSaveRegistry /*= true*/)
+                                       bool aSaveSettings /*= true*/)
 {
     AssertReturn(aDHCPServer != NULL, E_INVALIDARG);
@@ -4781 +4707 @@
     m->allDHCPServers.addChild(aDHCPServer);
 
-    if (aSaveRegistry)
+    if (aSaveSettings)
     {
         AutoWriteLock vboxLock(this COMMA_LOCKVAL_SRC_POS);
@@ -4788 +4714 @@
 
         if (FAILED(rc))
-            unregisterDHCPServer(aDHCPServer, false /* aSaveRegistry */);
+            unregisterDHCPServer(aDHCPServer, false /* aSaveSettings */);
     }
 
@@ -4795 +4721 @@
 
 /**
- * Removes the given hard disk from the hard disk registry.
- *
- * @param aHardDisk     Hard disk object to remove.
- * @param aSaveRegistry @c true to save hard disk registry to disk (default).
- *
- * When @a aSaveRegistry is @c true, this operation may fail because of the
- * failed #saveSettings() method it calls. In this case, the hard disk object
- * will NOT be removed from the registry when this method returns. It is
- * therefore the responsibility of the caller to call this method as the first
- * step of some action that requires unregistration, before calling uninit() on
- * @a aHardDisk.
- *
- * @note Locks this object for writing and @a aHardDisk for reading.
+ * Removes the given DHCP server from the settings.
+ *
+ * @param aDHCPServer   DHCP server object to remove.
+ * @param aSaveSettings @c true to save settings to disk (default).
+ *
+ * When @a aSaveSettings is @c true, this operation may fail because of the
+ * failed #saveSettings() method it calls. In this case, the DHCP server
+ * will NOT be removed from the settingsi when this method returns.
+ *
+ * @note Locks this object for writing.
  */
 HRESULT VirtualBox::unregisterDHCPServer(DHCPServer *aDHCPServer,
-                                         bool aSaveRegistry /*= true*/)
+                                         bool aSaveSettings /*= true*/)
 {
     AssertReturn(aDHCPServer != NULL, E_INVALIDARG);
@@ -4824 +4747 @@
     HRESULT rc = S_OK;
 
-    if (aSaveRegistry)
+    if (aSaveSettings)
     {
         AutoWriteLock vboxLock(this COMMA_LOCKVAL_SRC_POS);
@@ -4831 +4754 @@
 
         if (FAILED(rc))
-            registerDHCPServer(aDHCPServer, false /* aSaveRegistry */);
+            registerDHCPServer(aDHCPServer, false /* aSaveSettings */);
     }