Changeset 25903 in vbox

Timestamp:

Jan 18, 2010 6:15:43 PM (15 years ago)

Author:

vboxsync

Message:

Main: make restoreSnapshot() work with the lock validator; take saveSettings() out of a lot of functions and instead return a flag to the caller so the caller can make that call; as a side effect, this no longer calls saveSettings multiple times in several code paths (e.g. restoreSnapshot()) and cleans up locking in medium tasks

Location:

trunk/src/VBox/Main

Files:

• ConsoleImpl.cpp (modified) (2 diffs)
• MachineImpl.cpp (modified) (30 diffs)
• MediumImpl.cpp (modified) (91 diffs)
• SnapshotImpl.cpp (modified) (26 diffs)
• VirtualBoxImpl.cpp (modified) (28 diffs)
• include/MachineImpl.h (modified) (3 diffs)
• include/MediumImpl.h (modified) (9 diffs)
• include/VirtualBoxImpl.h (modified) (2 diffs)

trunk/src/VBox/Main/ConsoleImpl.cpp

@@ -1752 +1752 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    AutoWriteLock alock(this);
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
     if (   mMachineState != MachineState_Running
@@ -1905 +1905 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    AutoWriteLock alock(this);
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
     if (   mMachineState != MachineState_Running

trunk/src/VBox/Main/MachineImpl.cpp

@@ -1129 +1129 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    AutoReadLock alock(this);
+    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
 
     *enabled = mHWData->mCPUHotPlugEnabled;
@@ -1143 +1143 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    AutoWriteLock alock(this);
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
     rc = checkStateDependency(MutableStateDep);
@@ -2426 +2426 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    /* VirtualBox::findHardDisk() and the corresponding other methods for
-     * DVD and floppy media need *write* lock (for getting rid of unneeded
-     * host drives which got enumerated); also we want to make sure the
-     * media object we pick up doesn't get unregistered before we finish. */
-    AutoMultiWriteLock2 alock(mParent, this COMMA_LOCKVAL_SRC_POS);
+    // if this becomes true then we need to call saveSettings in the end
+    // @todo r=dj there is no error handling so far...
+    bool fNeedsSaveSettings = false;
+
+    /* protect the media tree all the while we're in here, as well as our member variables */
+    AutoMultiWriteLock2 alock(this->lockHandle(), &mParent->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     HRESULT rc = checkStateDependency(MutableStateDep);
@@ -2505 +2506 @@
             break;
 
-        case DeviceType_DVD:
+        case DeviceType_DVD: // @todo r=dj eliminate this, replace with findDVDImage
             if (!uuid.isEmpty())
             {
@@ -2538 +2539 @@
             break;
 
-        case DeviceType_Floppy:
+        case DeviceType_Floppy: // @todo r=dj eliminate this, replace with findFloppyImage
             if (!uuid.isEmpty())
             {
@@ -2582 +2583 @@
     AutoWriteLock mediumLock(medium COMMA_LOCKVAL_SRC_POS);
 
-    if (   (pAttachTemp = findAttachment(mMediaData->mAttachments, medium))
-        && !medium.isNull())
-    {
+    if (    (pAttachTemp = findAttachment(mMediaData->mAttachments, medium))
+         && !medium.isNull()
+       )
         return setError(VBOX_E_OBJECT_IN_USE,
                         tr("Medium '%s' is already attached to this virtual machine"),
                         medium->getLocationFull().raw());
-    }
 
     bool indirect = false;
@@ -2774 +2774 @@
                         medium->preferredDiffFormat().raw(),
                         BstrFmt("%ls"RTPATH_SLASH_STR,
-                                 mUserData->mSnapshotFolderFull.raw()).raw());
+                                 mUserData->mSnapshotFolderFull.raw()).raw(),
+                        &fNeedsSaveSettings);
         if (FAILED(rc)) return rc;
 
@@ -2789 +2790 @@
         alock.leave();
 
-        rc = medium->createDiffStorageAndWait(diff, MediumVariant_Standard);
+        rc = medium->createDiffStorageAndWait(diff, MediumVariant_Standard, &fNeedsSaveSettings);
 
         alock.enter();
@@ -2826 +2827 @@
     mMediaData->mAttachments.push_back(attachment);
 
+    if (fNeedsSaveSettings)
+    {
+        mediumLock.release();
+        alock.release();
+
+        AutoWriteLock(mParent COMMA_LOCKVAL_SRC_POS);
+        mParent->saveSettings();
+    }
+
     return rc;
 }
@@ -2839 +2849 @@
     AutoCaller autoCaller(this);
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
+
+    bool fNeedsSaveSettings = false;
 
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
@@ -2881 +2893 @@
         alock.leave();
 
-        rc = oldmedium->deleteStorageAndWait();
+        rc = oldmedium->deleteStorageAndWait(NULL /*aProgress*/, &fNeedsSaveSettings);
 
         alock.enter();
@@ -2900 +2912 @@
     if (mediumType != DeviceType_HardDisk && !oldmedium.isNull())
         oldmedium->detachFrom(mData->mUuid);
+
+    if (fNeedsSaveSettings)
+    {
+        alock.release();
+        AutoWriteLock vboxlock(this COMMA_LOCKVAL_SRC_POS);
+        saveSettings();
+    }
 
     return S_OK;
@@ -4360 +4379 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    AutoWriteLock alock(this);
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
     if (!mHWData->mCPUHotPlugEnabled)
@@ -4394 +4413 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    AutoWriteLock alock(this);
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
     if (!mHWData->mCPUHotPlugEnabled)
@@ -4437 +4456 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    AutoReadLock alock(this);
+    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
 
     /* If hotplug is enabled the CPU is always enabled. */
@@ -6966 +6985 @@
  *  - SaveS_InformCallbacksAnyway: Callbacks will be informed even if
  *    #isReallyModified() returns false. This is necessary for cases when we
- *    change machine data diectly, not through the backup()/commit() mechanism.
+ *    change machine data directly, not through the backup()/commit() mechanism.
  *
  * @note Must be called from under mParent write lock (sometimes needed by
@@ -7520 +7539 @@
  *                          attached).
  * @param aOnline           Whether the VM was online prior to this operation.
+ * @param pfNeedsSaveSettings 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 The progress object is not marked as completed, neither on success nor
@@ -7529 +7550 @@
                                      IProgress *aProgress,
                                      ULONG aWeight,
-                                     bool aOnline)
+                                     bool aOnline,
+                                     bool *pfNeedsSaveSettings)
 {
     AssertReturn(!aFolder.isEmpty(), E_FAIL);
@@ -7625 +7647 @@
                             medium->preferredDiffFormat().raw(),
                             BstrFmt("%ls"RTPATH_SLASH_STR,
-                                    mUserData->mSnapshotFolderFull.raw()).raw());
+                                    mUserData->mSnapshotFolderFull.raw()).raw(),
+                            pfNeedsSaveSettings);
             if (FAILED(rc)) throw rc;
 
@@ -7633 +7656 @@
             rc = medium->createDiffStorageAndWait(diff,
                                                   MediumVariant_Standard,
-                                                  NULL);
+                                                  pfNeedsSaveSettings);
 
             /** @todo r=bird: How is the locking and diff image cleaned up if we fail before
@@ -7693 +7716 @@
         MultiResultRef mrc (rc);
 
-        mrc = deleteImplicitDiffs();
+        mrc = deleteImplicitDiffs(pfNeedsSaveSettings);
     }
 
@@ -7706 +7729 @@
  * called from #fixupMedia() when the changes are rolled back.
  *
+ * @param pfNeedsSaveSettings 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 Locks this object for writing.
  */
-HRESULT Machine::deleteImplicitDiffs()
+HRESULT Machine::deleteImplicitDiffs(bool *pfNeedsSaveSettings)
 {
     AutoCaller autoCaller(this);
@@ -7783 +7809 @@
             ComObjPtr<Medium> hd = (*it)->getMedium();
 
-            rc = hd->deleteStorageAndWait();
+            rc = hd->deleteStorageAndWait(NULL /*aProgress*/, pfNeedsSaveSettings);
 #if 1 /* HACK ALERT: Just make it kind of work */ /** @todo Fix this hack properly. The LockWrite / UnlockWrite / LockRead changes aren't undone! */
             if (rc == VBOX_E_INVALID_OBJECT_STATE)
@@ -7789 +7815 @@
                 LogFlowFunc(("Applying unlock hack on '%s'! FIXME!\n", (*it)->getLogName()));
                 hd->UnlockWrite(NULL);
-                rc = hd->deleteStorageAndWait();
+                rc = hd->deleteStorageAndWait(NULL /*aProgress*/, pfNeedsSaveSettings);
             }
 #endif
@@ -7890 +7916 @@
 
 /**
- * Perform deferred hard disk detachments on success and deletion of implicitly
- * created diffs on failure.
+ * Perform deferred hard disk detachments.
  *
  * Does nothing if the hard disk attachment data (mMediaData) is not changed (not
  * backed up).
  *
- * When the data is backed up, this method will commit mMediaData if @a aCommit is
- * @c true and rollback it otherwise before returning.
- *
- * If @a aOnline is @c true then this method called with @a aCommit = @c true
- * will also unlock the old hard disks for which the new implicit diffs were
- * created and will lock these new diffs for writing. When @a aCommit is @c
- * false, this argument is ignored.
- *
- * @param aCommit       @c true if called on success.
+ * If @a aOnline is @c true then this method will also unlock the old hard disks
+ * for which the new implicit diffs were created and will lock these new diffs for
+ * writing.
+ *
  * @param aOnline       Whether the VM was online prior to this operation.
  *
  * @note Locks this object for writing!
  */
-void Machine::fixupMedia(bool aCommit, bool aOnline /*= false*/)
+void Machine::commitMedia(bool aOnline /*= false*/)
 {
     AutoCaller autoCaller(this);
@@ -7916 +7936 @@
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
-    LogFlowThisFunc(("Entering, aCommit=%d, aOnline=%d\n", aCommit, aOnline));
+    LogFlowThisFunc(("Entering, aOnline=%d\n", aOnline));
 
     HRESULT rc = S_OK;
@@ -7924 +7944 @@
         return;
 
-    if (aCommit)
-    {
-        MediaData::AttachmentList &oldAtts = mMediaData.backedUpData()->mAttachments;
-
-        /* enumerate new attachments */
-        for (MediaData::AttachmentList::const_iterator it = mMediaData->mAttachments.begin();
-             it != mMediaData->mAttachments.end();
-             ++it)
-        {
-            MediumAttachment *pAttach = *it;
-
-            pAttach->commit();
-
+    MediaData::AttachmentList &oldAtts = mMediaData.backedUpData()->mAttachments;
+
+    /* enumerate new attachments */
+    for (MediaData::AttachmentList::const_iterator it = mMediaData->mAttachments.begin();
+            it != mMediaData->mAttachments.end();
+            ++it)
+    {
+        MediumAttachment *pAttach = *it;
+
+        pAttach->commit();
+
+        Medium* pMedium = pAttach->getMedium();
+        bool fImplicit = pAttach->isImplicit();
+
+        LogFlowThisFunc(("Examining current medium '%s' (implicit: %d)\n",
+                            (pMedium) ? pMedium->getName().raw() : "NULL",
+                            fImplicit));
+
+        /** @todo convert all this Machine-based voodoo to MediumAttachment
+        * based commit logic. */
+        if (fImplicit)
+        {
+            /* convert implicit attachment to normal */
+            pAttach->setImplicit(false);
+
+            if (    aOnline
+                    && pMedium
+                    && pAttach->getType() == DeviceType_HardDisk
+                )
+            {
+                rc = pMedium->LockWrite(NULL);
+                AssertComRC(rc);
+
+                mData->mSession.mLockedMedia.push_back(
+                    Data::Session::LockedMedia::value_type(
+                        ComPtr<IMedium>(pMedium), true));
+
+                /* also, relock the old hard disk which is a base for the
+                * new diff for reading if the VM is online */
+
+                ComObjPtr<Medium> parent = pMedium->getParent();
+                /* make the relock atomic */
+                AutoWriteLock parentLock(parent COMMA_LOCKVAL_SRC_POS);
+                rc = parent->UnlockWrite(NULL);
+                AssertComRC(rc);
+                rc = parent->LockRead(NULL);
+                AssertComRC(rc);
+
+                /* XXX actually we should replace the old entry in that
+                * vector (write lock => read lock) but this would take
+                * some effort. So lets just ignore the error code in
+                * SessionMachine::unlockMedia(). */
+                mData->mSession.mLockedMedia.push_back(
+                    Data::Session::LockedMedia::value_type (
+                        ComPtr<IMedium>(parent), false));
+            }
+
+            continue;
+        }
+
+        if (pMedium)
+        {
+            /* was this medium attached before? */
+            for (MediaData::AttachmentList::iterator oldIt = oldAtts.begin();
+                    oldIt != oldAtts.end();
+                    ++oldIt)
+            {
+                MediumAttachment *pOldAttach = *oldIt;
+                if (pOldAttach->getMedium().equalsTo(pMedium))
+                {
+                    LogFlowThisFunc(("--> medium '%s' was attached before, will not remove\n", pMedium->getName().raw()));
+
+                    /* yes: remove from old to avoid de-association */
+                    oldAtts.erase(oldIt);
+                    break;
+                }
+            }
+        }
+    }
+
+    /* enumerate remaining old attachments and de-associate from the
+        * current machine state */
+    for (MediaData::AttachmentList::const_iterator it = oldAtts.begin();
+            it != oldAtts.end();
+            ++it)
+    {
+        MediumAttachment *pAttach = *it;
+        Medium* pMedium = pAttach->getMedium();
+
+        /* Detach only hard disks, since DVD/floppy media is detached
+            * instantly in MountMedium. */
+        if (pAttach->getType() == DeviceType_HardDisk && pMedium)
+        {
+            LogFlowThisFunc(("detaching medium '%s' from machine\n", pMedium->getName().raw()));
+
+            /* now de-associate from the current machine state */
+            rc = pMedium->detachFrom(mData->mUuid);
+            AssertComRC(rc);
+
+            if (    aOnline
+                    && pAttach->getType() == DeviceType_HardDisk)
+            {
+                /* unlock since not used anymore */
+                MediumState_T state;
+                rc = pMedium->UnlockWrite(&state);
+                /* the disk may be alredy relocked for reading above */
+                Assert (SUCCEEDED(rc) || state == MediumState_LockedRead);
+            }
+        }
+    }
+
+    /* commit the hard disk changes */
+    mMediaData.commit();
+
+    if (getClassID() == clsidSessionMachine)
+    {
+        /* attach new data to the primary machine and reshare it */
+        mPeer->mMediaData.attach(mMediaData);
+    }
+
+    return;
+}
+
+/**
+ * Perform deferred deletion of implicitly created diffs.
+ *
+ * Does nothing if the hard disk attachment data (mMediaData) is not changed (not
+ * backed up).
+ *
+ * @param pfNeedsSaveSettings 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 Locks this object for writing!
+ */
+void Machine::rollbackMedia()
+{
+    AutoCaller autoCaller(this);
+    AssertComRCReturnVoid (autoCaller.rc());
+
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    LogFlowThisFunc(("Entering\n"));
+
+    HRESULT rc = S_OK;
+
+    /* no attach/detach operations -- nothing to do */
+    if (!mMediaData.isBackedUp())
+        return;
+
+    /* enumerate new attachments */
+    for (MediaData::AttachmentList::const_iterator it = mMediaData->mAttachments.begin();
+            it != mMediaData->mAttachments.end();
+            ++it)
+    {
+        MediumAttachment *pAttach = *it;
+        /* Fix up the backrefs for DVD/floppy media. */
+        if (pAttach->getType() != DeviceType_HardDisk)
+        {
             Medium* pMedium = pAttach->getMedium();
-            bool fImplicit = pAttach->isImplicit();
-
-            LogFlowThisFunc(("Examining current medium '%s' (implicit: %d)\n",
-                             (pMedium) ? pMedium->getName().raw() : "NULL",
-                             fImplicit));
-
-            /** @todo convert all this Machine-based voodoo to MediumAttachment
-            * based commit logic. */
-            if (fImplicit)
-            {
-                /* convert implicit attachment to normal */
-                pAttach->setImplicit(false);
-
-                if (    aOnline
-                     && pMedium
-                     && pAttach->getType() == DeviceType_HardDisk
-                   )
-                {
-                    rc = pMedium->LockWrite(NULL);
-                    AssertComRC(rc);
-
-                    mData->mSession.mLockedMedia.push_back(
-                        Data::Session::LockedMedia::value_type(
-                            ComPtr<IMedium>(pMedium), true));
-
-                    /* also, relock the old hard disk which is a base for the
-                    * new diff for reading if the VM is online */
-
-                    ComObjPtr<Medium> parent = pMedium->getParent();
-                    /* make the relock atomic */
-                    AutoWriteLock parentLock(parent COMMA_LOCKVAL_SRC_POS);
-                    rc = parent->UnlockWrite(NULL);
-                    AssertComRC(rc);
-                    rc = parent->LockRead(NULL);
-                    AssertComRC(rc);
-
-                    /* XXX actually we should replace the old entry in that
-                    * vector (write lock => read lock) but this would take
-                    * some effort. So lets just ignore the error code in
-                    * SessionMachine::unlockMedia(). */
-                    mData->mSession.mLockedMedia.push_back(
-                        Data::Session::LockedMedia::value_type (
-                            ComPtr<IMedium>(parent), false));
-                }
-
-                continue;
-            }
-
             if (pMedium)
             {
-                /* was this medium attached before? */
-                for (MediaData::AttachmentList::iterator oldIt = oldAtts.begin();
-                     oldIt != oldAtts.end();
-                     ++oldIt)
-                {
-                    MediumAttachment *pOldAttach = *oldIt;
-                    if (pOldAttach->getMedium().equalsTo(pMedium))
-                    {
-                        LogFlowThisFunc(("--> medium '%s' was attached before, will not remove\n", pMedium->getName().raw()));
-
-                        /* yes: remove from old to avoid de-association */
-                        oldAtts.erase(oldIt);
-                        break;
-                    }
-                }
-            }
-        }
-
-        /* enumerate remaining old attachments and de-associate from the
-         * current machine state */
-        for (MediaData::AttachmentList::const_iterator it = oldAtts.begin();
-             it != oldAtts.end();
-             ++it)
-        {
-            MediumAttachment *pAttach = *it;
-            Medium* pMedium = pAttach->getMedium();
-
-            /* Detach only hard disks, since DVD/floppy media is detached
-             * instantly in MountMedium. */
-            if (pAttach->getType() == DeviceType_HardDisk && pMedium)
-            {
-                LogFlowThisFunc(("detaching medium '%s' from machine\n", pMedium->getName().raw()));
-
-                /* now de-associate from the current machine state */
                 rc = pMedium->detachFrom(mData->mUuid);
                 AssertComRC(rc);
-
-                if (    aOnline
-                     && pAttach->getType() == DeviceType_HardDisk)
-                {
-                    /* unlock since not used anymore */
-                    MediumState_T state;
-                    rc = pMedium->UnlockWrite(&state);
-                    /* the disk may be alredy relocked for reading above */
-                    Assert (SUCCEEDED(rc) || state == MediumState_LockedRead);
-                }
             }
         }
 
-        /* commit the hard disk changes */
-        mMediaData.commit();
-
-        if (getClassID() == clsidSessionMachine)
-        {
-            /* attach new data to the primary machine and reshare it */
-            mPeer->mMediaData.attach(mMediaData);
-        }
-    }
-    else
-    {
-        /* enumerate new attachments */
-        for (MediaData::AttachmentList::const_iterator it = mMediaData->mAttachments.begin();
-             it != mMediaData->mAttachments.end();
-             ++it)
-        {
-            MediumAttachment *pAttach = *it;
-            /* Fix up the backrefs for DVD/floppy media. */
-            if (pAttach->getType() != DeviceType_HardDisk)
+        (*it)->rollback();
+
+        pAttach = *it;
+        /* Fix up the backrefs for DVD/floppy media. */
+        if (pAttach->getType() != DeviceType_HardDisk)
+        {
+            Medium* pMedium = pAttach->getMedium();
+            if (pMedium)
             {
-                Medium* pMedium = pAttach->getMedium();
-                if (pMedium)
-                {
-                    rc = pMedium->detachFrom(mData->mUuid);
-                    AssertComRC(rc);
-                }
+                rc = pMedium->attachTo(mData->mUuid);
+                AssertComRC(rc);
             }
-
-            (*it)->rollback();
-
-            pAttach = *it;
-            /* Fix up the backrefs for DVD/floppy media. */
-            if (pAttach->getType() != DeviceType_HardDisk)
-            {
-                Medium* pMedium = pAttach->getMedium();
-                if (pMedium)
-                {
-                    rc = pMedium->attachTo(mData->mUuid);
-                    AssertComRC(rc);
-                }
-            }
-        }
-
-        /** @todo convert all this Machine-based voodoo to MediumAttachment
-         * based rollback logic. */
-        // @todo r=dj the below totally fails if this gets called from Machine::rollback(),
-        // which gets called if Machine::registeredInit() fails...
-        deleteImplicitDiffs();
-    }
+        }
+    }
+
+    /** @todo convert all this Machine-based voodoo to MediumAttachment
+     * based rollback logic. */
+    // @todo r=dj the below totally fails if this gets called from Machine::rollback(),
+    // which gets called if Machine::registeredInit() fails...
+    deleteImplicitDiffs(NULL /*pfNeedsSaveSettings*/);
 
     return;
@@ -8307 +8351 @@
 
     if (mMediaData.isBackedUp())
-        fixupMedia(false /* aCommit */);
+        rollbackMedia();
 
     /* check for changes in child objects */
@@ -8402 +8446 @@
 
     if (mMediaData.isBackedUp())
-        fixupMedia(true /* aCommit */);
+        commitMedia();
 
     mBIOSSettings->commit();
@@ -8913 +8957 @@
     {
         LogWarningThisFunc(("canceling failed save state request!\n"));
-        endSavingState (FALSE /* aSuccess  */);
+        endSavingState(FALSE /* aSuccess  */);
     }
     else if (!mSnapshotData.mSnapshot.isNull())
     {
         LogWarningThisFunc(("canceling untaken snapshot!\n"));
-        endTakingSnapshot (FALSE /* aSuccess  */);
+
+        /* delete all differencing hard disks created (this will also attach
+         * their parents back by rolling back mMediaData) */
+        rollbackMedia();
+        /* delete the saved state file (it might have been already created) */
+        if (mSnapshotData.mSnapshot->stateFilePath().length())
+            RTFileDelete(mSnapshotData.mSnapshot->stateFilePath().c_str());
+
+        mSnapshotData.mSnapshot->uninit();
     }
 

trunk/src/VBox/Main/MediumImpl.cpp

@@ -118 +118 @@
     Utf8Str strLastAccessError;
 
-    // pParent and llChildren are protected by VirtualBox::hardDiskTreeLockHandle()
+    // pParent and llChildren are protected by VirtualBox::getMediaTreeLockHandle()
     ComObjPtr<Medium> pParent;
     MediaList llChildren;           // to add a child, just call push_back; to remove a child, call child->deparent() which does a lookup
@@ -205 +205 @@
     HRESULT m_rc;
 
+    // Whether the caller needs to call VirtualBox::saveSettings() after
+    // the task function returns. Only used in synchronous (wait) mode;
+    // otherwise the task will save the settings itself.
+    bool *m_pfNeedsSaveSettings;
+
     Task(Medium *aThat,
          Progress *aProgress,
@@ -212 +217 @@
           m_pProgress(aProgress),
           m_operation(aOperation),
-          m_rc(S_OK)
-    {}
+          m_rc(S_OK),
+          m_pfNeedsSaveSettings(NULL)
+    { }
 
     ~Task();
@@ -258 +264 @@
 
     HRESULT startThread();
-    HRESULT runNow();
+    HRESULT runNow(bool *pfNeedsSaveSettings);
 
     struct Data
@@ -279 +285 @@
 
         /** Media to open, in {parent,child} order */
-        std::auto_ptr <ImageChain> source;
+        std::auto_ptr<ImageChain> source;
         /** Media which are parent of target, in {parent,child} order */
-        std::auto_ptr <ImageChain> parent;
+        std::auto_ptr<ImageChain> parent;
         /** The to-be parent medium object */
         ComObjPtr<Medium> parentDisk;
@@ -288 +294 @@
 
         /** Media to merge, in {parent,child} order */
-        std::auto_ptr <MergeChain> chain;
+        std::auto_ptr<MergeChain> chain;
 
         /* Compact */
 
         /** Media to open, in {parent,child} order */
-        std::auto_ptr <ImageChain> images;
-    }
-    d;
+        std::auto_ptr<ImageChain> images;
+    } d;
 
 protected:
@@ -354 +359 @@
  *       complete the progress object in this case.
  */
-HRESULT Medium::Task::runNow()
-{
+HRESULT Medium::Task::runNow(bool *pfNeedsSaveSettings)
+{
+    m_pfNeedsSaveSettings = pfNeedsSaveSettings;
+
     /* NIL_RTTHREAD indicates synchronous call. */
     Medium::taskThread(NIL_RTTHREAD, this);
@@ -880 +887 @@
  * @param aVirtualBox   VirtualBox object.
  * @param aLocation     Storage unit location.
+ * @param pfNeedsSaveSettings 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.
  */
 HRESULT Medium::init(VirtualBox *aVirtualBox,
                      CBSTR aFormat,
-                     CBSTR aLocation)
+                     CBSTR aLocation,
+                     bool *pfNeedsSaveSettings)
 {
     AssertReturn(aVirtualBox != NULL, E_FAIL);
@@ -917 +927 @@
         rc = setLocation(aLocation);
         if (FAILED(rc)) return rc;
-
-        /// @todo later we may want to use a pfnComposeLocation backend info
-        /// callback to generate a well-formed location value (based on the hard
-        /// disk properties we have) rather than allowing each caller to invent
-        /// its own (pseudo-)location.
-    }
-
-    if (!(m->formatObj->capabilities() &
-          (MediumFormatCapabilities_CreateFixed |
-           MediumFormatCapabilities_CreateDynamic)))
+    }
+
+    if (!(m->formatObj->capabilities() & (   MediumFormatCapabilities_CreateFixed
+                                           | MediumFormatCapabilities_CreateDynamic))
+       )
     {
         /* storage for hard disks of this format can neither be explicitly
@@ -933 +938 @@
         m->state = MediumState_Created;
         unconst(m->id).create();
-        rc = m->pVirtualBox->registerHardDisk(this);
-
-        /// @todo later we may want to use a pfnIsConfigSufficient backend info
-        /// callback that would tell us when we have enough properties to work
-        /// with the hard disk and this information could be used to actually
-        /// move such hard disks from NotCreated to Created state. Instead of
-        /// pfnIsConfigSufficient we can use MediumFormat property
-        /// descriptions to see which properties are mandatory
+
+        AutoWriteLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+        rc = m->pVirtualBox->registerHardDisk(this, pfNeedsSaveSettings);
     }
 
@@ -1078 +1078 @@
     {
         // differencing image: add to parent
-        AutoWriteLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+        AutoWriteLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
         m->pParent = aParent;
         aParent->m->llChildren.push_back(this);
@@ -1160 +1160 @@
      * get the actual state and the rest of the data, the user will have to call
      * COMGETTER(State). */
+
+    AutoWriteLock treeLock(aVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     /* load all children */
@@ -1541 +1543 @@
 
     /* we access mParent & children() */
-    AutoReadLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoReadLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     /* cannot change the type of a differencing hard disk */
@@ -1594 +1596 @@
 
     /* we access mParent */
-    AutoReadLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoReadLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     m->pParent.queryInterfaceTo(aParent);
@@ -1610 +1612 @@
 
     /* we access children */
-    AutoReadLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoReadLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     SafeIfaceArray<IMedium> children(this->getChildren());
@@ -1654 +1656 @@
 
         /* we access mParent */
-        AutoReadLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+        AutoReadLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
@@ -2033 +2035 @@
         return S_OK;
 
-    /* unregisterWithVirtualBox() is assumed to always need a write mVirtualBox
-     * lock as it is intenede to modify its internal structires. Also, we want
-     * to unregister ourselves atomically after detecting that closure is
-     * possible to make sure that we don't do that after another thread has done
-     * VirtualBox::find*() but before it starts using us (provided that it holds
-     * a mVirtualBox lock of course). */
-
-    AutoWriteLock vboxLock(m->pVirtualBox COMMA_LOCKVAL_SRC_POS);
+    // we're accessing parent/child and backrefs, so lock the tree first, then ourselves
+    AutoMultiWriteLock2 multilock(&m->pVirtualBox->getMediaTreeLockHandle(),
+                                  this->lockHandle()
+                                  COMMA_LOCKVAL_SRC_POS);
 
     bool wasCreated = true;
+    bool fNeedsSaveSettings = false;
 
     switch (m->state)
@@ -2070 +2069 @@
          * uninitialization (to keep the media registry consistent on
          * failure to do so) */
-        rc = unregisterWithVirtualBox();
+        rc = unregisterWithVirtualBox(&fNeedsSaveSettings);
         if (FAILED(rc)) return rc;
+    }
+
+    multilock.release();
+
+    if (fNeedsSaveSettings)
+    {
+        AutoWriteLock vboxlock(m->pVirtualBox COMMA_LOCKVAL_SRC_POS);
+        m->pVirtualBox->saveSettings();
     }
 
@@ -2389 +2396 @@
 
         /* we walk the source tree */
-        AutoReadLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+        AutoReadLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
         for (Medium *hd = this;
              hd;
@@ -2491 +2498 @@
 
         /* we walk the image tree */
-        AutoReadLock srcTreeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+        AutoReadLock srcTreeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
         for (Medium *hd = this;
              hd;
@@ -2556 +2563 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+    /* canClose() needs the tree lock */
+    AutoMultiWriteLock2 multilock(&m->pVirtualBox->getMediaTreeLockHandle(),
+                                  this->lockHandle()
+                                  COMMA_LOCKVAL_SRC_POS);
 
     LogFlowThisFunc(("ENTER for medium %s\n", m->strLocationFull.c_str()));
@@ -2577 +2587 @@
         progress.createObject();
         rc = progress->init(m->pVirtualBox,
-                            static_cast <IMedium *>(this),
+                            static_cast<IMedium*>(this),
                             BstrFmt(tr("Resetting differencing hard disk '%s'"), m->strLocationFull.raw()),
                             FALSE /* aCancelable */);
@@ -2584 +2594 @@
         /* setup task object and thread to carry out the operation
          * asynchronously */
-
         std::auto_ptr<Task> task(new Task(this, progress, Task::Reset));
         AssertComRCThrowRC(task->m_autoCaller.rc());
@@ -2932 +2941 @@
 
     /* we access children() */
-    AutoReadLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoReadLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     updatePath(aOldPath, aNewPath);
@@ -2966 +2975 @@
 
     /* we access mParent */
-    AutoReadLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoReadLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     pBase = this;
@@ -3005 +3014 @@
 
     /* we access children */
-    AutoReadLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoReadLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
@@ -3052 +3061 @@
 
     /* we access mParent */
-    AutoReadLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoReadLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
@@ -3181 +3190 @@
 
     /* we access mParent & children() */
-    AutoReadLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoReadLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     AssertReturn(m->type == MediumType_Normal, E_FAIL);
@@ -3298 +3307 @@
  *       reading or writing.
  */
-HRESULT Medium::discard(ComObjPtr<Progress> &aProgress, ULONG ulWeight, MergeChain *aChain)
+HRESULT Medium::discard(ComObjPtr<Progress> &aProgress,
+                        ULONG ulWeight,
+                        MergeChain *aChain,
+                        bool *pfNeedsSaveSettings)
 {
     AssertReturn(!aProgress.isNull(), E_FAIL);
@@ -3315 +3327 @@
         if (aChain == NULL)
         {
-            AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
             /* we access mParent & children() */
-            AutoReadLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+            AutoMultiWriteLock2 mLock(&m->pVirtualBox->getMediaTreeLockHandle(), this->lockHandle() COMMA_LOCKVAL_SRC_POS);
 
             Assert(getChildren().size() == 0);
@@ -3340 +3350 @@
             hdFrom = this;
 
-            // deleteStorageAndWait calls unregisterWithVirtualBox which gets
-            // a write tree lock, so don't deadlock
-            treeLock.release();
-            alock.release();
-
-            rc = deleteStorageAndWait(&aProgress);
+            rc = deleteStorageAndWait(&aProgress, pfNeedsSaveSettings);
         }
         else
@@ -3351 +3356 @@
             hdFrom = aChain->source();
 
-            rc = hdFrom->mergeToAndWait(aChain, &aProgress);
+            rc = hdFrom->mergeToAndWait(aChain, &aProgress, pfNeedsSaveSettings);
         }
     }
@@ -3386 +3391 @@
 
         /* we access mParent & children() */
-        AutoReadLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+        AutoReadLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
         Assert(getChildren().size() == 0);
@@ -3852 +3857 @@
 
                     /* we set mParent & children() */
-                    AutoWriteLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+                    AutoWriteLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
                     Assert(m->pParent.isNull());
@@ -3861 +3866 @@
                 {
                     /* we access mParent */
-                    AutoReadLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+                    AutoReadLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
                     /* check that parent UUIDs match. Note that there's no need
@@ -4044 +4049 @@
  * @param aWait         @c true if this method should block instead of creating
  *                      an asynchronous thread.
+ * @param pfNeedsSaveSettings 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.
+ *                This only works in "wait" mode; otherwise saveSettings gets called automatically by the thread that was created,
+ *                and this parameter is ignored.
  *
  * @note Locks mVirtualBox and this object for writing. Locks getTreeLock() for
  *       writing.
  */
-HRESULT Medium::deleteStorage(ComObjPtr <Progress> *aProgress, bool aWait)
+HRESULT Medium::deleteStorage(ComObjPtr<Progress> *aProgress,
+                              bool aWait,
+                              bool *pfNeedsSaveSettings)
 {
     AssertReturn(aProgress != NULL || aWait == true, E_FAIL);
 
-    /* unregisterWithVirtualBox() needs a write lock. We want to unregister
-     * ourselves atomically after detecting that deletion is possible to make
-     * sure that we don't do that after another thread has done
-     * VirtualBox::findHardDisk() but before it starts using us (provided that
-     * it holds a mVirtualBox lock too of course). */
-
-    AutoMultiWriteLock2 alock(m->pVirtualBox->lockHandle(), this->lockHandle() COMMA_LOCKVAL_SRC_POS);
+    /* we're accessing the media tree, and canClose() needs the tree lock too */
+    AutoMultiWriteLock2 multilock(&m->pVirtualBox->getMediaTreeLockHandle(),
+                                  this->lockHandle()
+                                  COMMA_LOCKVAL_SRC_POS);
     LogFlowThisFunc(("aWait=%RTbool locationFull=%s\n", aWait, getLocationFull().c_str() ));
 
@@ -4111 +4119 @@
     m->state = MediumState_Deleting;
 
-    /* we need to leave this object's write lock now because of
-     * unregisterWithVirtualBox() that locks getTreeLock() for writing */
-    alock.leave();
-
     /* try to remove from the list of known hard disks before performing actual
      * deletion (we favor the consistency of the media registry in the first
      * place which would have been broken if unregisterWithVirtualBox() failed
      * after we successfully deleted the storage) */
-
-    rc = unregisterWithVirtualBox();
-
-    alock.enter();
+    rc = unregisterWithVirtualBox(pfNeedsSaveSettings);
 
     /* restore the state because we may fail below; we will set it later again*/
@@ -4148 +4149 @@
     }
 
-    std::auto_ptr <Task> task(new Task(this, progress, Task::Delete));
+    std::auto_ptr<Task> task(new Task(this, progress, Task::Delete));
     AssertComRCReturnRC(task->m_autoCaller.rc());
 
@@ -4156 +4157 @@
         m->state = MediumState_Deleting;
 
-        rc = task->runNow();
+        rc = task->runNow(NULL /* pfNeedsSaveSettings*/ );        // there is no save settings to do in taskThreadDelete()
     }
     else
@@ -4211 +4212 @@
  * @param aWait         @c true if this method should block instead of creating
  *                      an asynchronous thread.
+ * @param pfNeedsSaveSettings 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.
+ *                This only works in "wait" mode; otherwise saveSettings gets called automatically by the thread that was created,
+ *                and this parameter is ignored.
  *
  * @note Locks this object and @a aTarget for writing.
@@ -4217 +4222 @@
                                   MediumVariant_T aVariant,
                                   ComObjPtr<Progress> *aProgress,
-                                  bool aWait)
+                                  bool aWait,
+                                  bool *pfNeedsSaveSettings)
 {
     AssertReturn(!aTarget.isNull(), E_FAIL);
@@ -4233 +4239 @@
 
     /* Note: MediumState_LockedWrite is ok when taking an online snapshot */
-    AssertReturn(m->state == MediumState_LockedRead ||
-                  m->state == MediumState_LockedWrite, E_FAIL);
+    AssertReturn(    m->state == MediumState_LockedRead
+                  || m->state == MediumState_LockedWrite, E_FAIL);
 
     if (aTarget->m->state != MediumState_NotCreated)
@@ -4243 +4249 @@
     /* check that the hard disk is not attached to any VM in the current state*/
     for (BackRefList::const_iterator it = m->backRefs.begin();
-         it != m->backRefs.end(); ++ it)
+         it != m->backRefs.end();
+         ++it)
     {
         if (it->fInCurState)
@@ -4257 +4264 @@
 
             if (it->llSnapshotIds.size() == 0)
-            {
                 return setError(VBOX_E_INVALID_OBJECT_STATE,
                                 tr("Hard disk '%s' is attached to a virtual machine with UUID {%RTuuid}. No differencing hard disks based on it may be created until it is detached"),
                                 m->strLocationFull.raw(), it->machineId.raw());
-            }
 
             Assert(it->llSnapshotIds.size() == 1);
@@ -4267 +4272 @@
     }
 
-    ComObjPtr <Progress> progress;
+    ComObjPtr<Progress> progress;
 
     if (aProgress != NULL)
@@ -4286 +4291 @@
     }
 
-    /* setup task object and thread to carry out the operation
+    /* set up task object and thread to carry out the operation
      * asynchronously */
 
-    std::auto_ptr <Task> task(new Task(this, progress, Task::CreateDiff));
+    std::auto_ptr<Task> task(new Task(this, progress, Task::CreateDiff));
     AssertComRCReturnRC(task->m_autoCaller.rc());
 
@@ -4301 +4306 @@
     if (aWait)
     {
-        /* go to Creating state before starting the task */
+        // go to Creating state before starting the task
         aTarget->m->state = MediumState_Creating;
 
-        rc = task->runNow();
+        // release the locks because the task function will acquire other locks;
+        // this is safe because both this and the target are not protected by
+        // their states; we have asserted above that *this* is locked read or write!
+        alock.release();
+
+        rc = task->runNow(pfNeedsSaveSettings);
     }
     else
@@ -4365 +4375 @@
 
     /* we walk the tree */
-    AutoReadLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    AutoReadLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     HRESULT rc = S_OK;
@@ -4495 +4505 @@
  * @param aWait         @c true if this method should block instead of creating
  *                      an asynchronous thread.
+ * @param pfNeedsSaveSettings 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.
+ *                This only works in "wait" mode; otherwise saveSettings gets called automatically by the thread that was created,
+ *                and this parameter is ignored.
  *
  * @note Locks the branch lock for writing. Locks the hard disks from the chain
@@ -4501 +4515 @@
 HRESULT Medium::mergeTo(MergeChain *aChain,
                         ComObjPtr <Progress> *aProgress,
-                        bool aWait)
+                        bool aWait,
+                        bool *pfNeedsSaveSettings)
 {
     AssertReturn(aChain != NULL, E_FAIL);
@@ -4549 +4564 @@
     if (aWait)
     {
-        rc = task->runNow();
+        rc = task->runNow(pfNeedsSaveSettings);
     }
     else
@@ -4640 +4655 @@
  * @note Also reused by Medium::Reset().
  *
- * @note Locks getTreeLock() for reading.
+ * @note Caller must hold the media tree write lock!
  */
 HRESULT Medium::canClose()
 {
-    /* we access children */
-    AutoReadLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    Assert(m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
 
     if (getChildren().size() != 0)
@@ -4656 +4670 @@
 
 /**
+ * Calls either VirtualBox::unregisterImage or VirtualBox::unregisterHardDisk depending
+ * on the device type of this medium.
+ *
+ * @param pfNeedsSaveSettings 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 Called from within this object's AutoMayUninitSpan (or AutoCaller) and
  *       from under mVirtualBox write lock.
  *
- * @note Locks getTreeLock() for writing.
- */
-HRESULT Medium::unregisterWithVirtualBox()
+ * @note Caller must have locked the media tree lock for writing!
+ */
+HRESULT Medium::unregisterWithVirtualBox(bool *pfNeedsSaveSettings)
 {
     /* Note that we need to de-associate ourselves from the parent to let
@@ -4667 +4687 @@
 
     /* we modify mParent and access children */
-    AutoWriteLock treeLock(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    Assert(m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
 
     Medium *pParentBackup = m->pParent;
-
     AssertReturn(getChildren().size() == 0, E_FAIL);
-
     if (m->pParent)
         deparent();
@@ -4680 +4698 @@
     {
         case DeviceType_DVD:
-            rc = m->pVirtualBox->unregisterImage(this, DeviceType_DVD);
-            break;
+            rc = m->pVirtualBox->unregisterImage(this, DeviceType_DVD, pfNeedsSaveSettings);
+        break;
+
         case DeviceType_Floppy:
-            rc = m->pVirtualBox->unregisterImage(this, DeviceType_Floppy);
-            break;
+            rc = m->pVirtualBox->unregisterImage(this, DeviceType_Floppy, pfNeedsSaveSettings);
+        break;
+
         case DeviceType_HardDisk:
-            rc = m->pVirtualBox->unregisterHardDisk(this);
-            break;
+            rc = m->pVirtualBox->unregisterHardDisk(this, pfNeedsSaveSettings);
+        break;
+
         default:
-            break;
+        break;
     }
 
@@ -4867 +4888 @@
 /**
  * Implementation code called from Medium::taskThread for the "create base" task.
+ *
+ * This only gets started from Medium::CreateBaseStorage() and always runs
+ * asynchronously. As a result, we always save the VirtualBox.xml file when we're
+ * done here.
+ *
  * @param task
  * @param pvdOperationIfaces
@@ -4877 +4903 @@
     PVDINTERFACE vdOperationIfaces = (PVDINTERFACE)pvdOperationIfaces;
 
-    /* The lock is also used as a signal from the task initiator (which
-    * releases it only after RTThreadCreate()) that we can start the job */
-    AutoWriteLock thisLock(this COMMA_LOCKVAL_SRC_POS);
-
     /* these parameters we need after creation */
     uint64_t size = 0, logicalSize = 0;
-
-    /* The object may request a specific UUID (through a special form of
-    * the setLocation() argument). Otherwise we have to generate it */
-    Guid id = m->id;
-    bool generateUuid = id.isEmpty();
-    if (generateUuid)
-    {
-        id.create();
-        /* VirtualBox::registerHardDisk() will need UUID */
-        unconst(m->id) = id;
-    }
+    bool fGenerateUuid = false;
 
     try
     {
+        /* The lock is also used as a signal from the task initiator (which
+        * releases it only after RTThreadCreate()) that we can start the job */
+        AutoWriteLock thisLock(this COMMA_LOCKVAL_SRC_POS);
+
+        /* The object may request a specific UUID (through a special form of
+        * the setLocation() argument). Otherwise we have to generate it */
+        Guid id = m->id;
+        fGenerateUuid = id.isEmpty();
+        if (fGenerateUuid)
+        {
+            id.create();
+            /* VirtualBox::registerHardDisk() will need UUID */
+            unconst(m->id) = id;
+        }
+
         PVBOXHDD hdd;
         int vrc = VDCreate(m->vdDiskIfaces, &hdd);
@@ -4948 +4975 @@
     {
         /* register with mVirtualBox as the last step and move to
-        * Created state only on success (leaving an orphan file is
-        * better than breaking media registry consistency) */
-        rc = m->pVirtualBox->registerHardDisk(this);
-    }
-
-    thisLock.maybeEnter();
+         * Created state only on success (leaving an orphan file is
+         * better than breaking media registry consistency) */
+        bool fNeedsSaveSettings = false;
+        AutoWriteLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+        rc = m->pVirtualBox->registerHardDisk(this, &fNeedsSaveSettings);
+        treeLock.release();
+
+        if (fNeedsSaveSettings)
+        {
+            AutoWriteLock vboxlock(m->pVirtualBox COMMA_LOCKVAL_SRC_POS);
+            m->pVirtualBox->saveSettings();
+        }
+    }
+
+    // reenter the lock before changing state
+    AutoWriteLock thisLock(this COMMA_LOCKVAL_SRC_POS);
 
     if (SUCCEEDED(rc))
@@ -4968 +5005 @@
 
         /* reset UUID to prevent it from being reused next time */
-        if (generateUuid)
+        if (fGenerateUuid)
             unconst(m->id).clear();
     }
@@ -4977 +5014 @@
 /**
  * Implementation code called from Medium::taskThread for the "create diff" task.
+ *
+ * This task always gets started from Medium::createDiffStorage() and can run
+ * synchronously or asynchrously depending on the "wait" parameter passed to that
+ * function. If we run synchronously, the caller expects the bool *pfNeedsSaveSettings
+ * to be set before returning; otherwise (in asynchronous mode), we save the settings
+ * ourselves.
+ *
  * @param task
  * @param pvdOperationIfaces
@@ -4987 +5031 @@
     PVDINTERFACE vdOperationIfaces = (PVDINTERFACE)pvdOperationIfaces;
 
+    bool fNeedsSaveSettings = false;
+
     ComObjPtr<Medium> &pTarget = task.d.target;
 
-    /* Lock both in {parent,child} order. The lock is also used as a
-     * signal from the task initiator (which releases it only after
-     * RTThreadCreate()) that we can start the job*/
-    AutoMultiWriteLock2 thisLock(this, pTarget COMMA_LOCKVAL_SRC_POS);
-
     uint64_t size = 0, logicalSize = 0;
-
-    /* The object may request a specific UUID (through a special form of
-     * the setLocation() argument). Otherwise we have to generate it */
-    Guid targetId = pTarget->m->id;
-    bool generateUuid = targetId.isEmpty();
-    if (generateUuid)
-    {
-        targetId.create();
-        /* VirtualBox::registerHardDisk() will need UUID */
-        unconst(pTarget->m->id) = targetId;
-    }
+    bool fGenerateUuid = false;
 
     try
     {
+        /* Lock both in {parent,child} order. The lock is also used as a
+        * signal from the task initiator (which releases it only after
+        * RTThreadCreate()) that we can start the job*/
+        AutoMultiWriteLock2 mediaLock(this, pTarget COMMA_LOCKVAL_SRC_POS);
+
+        /* The object may request a specific UUID (through a special form of
+        * the setLocation() argument). Otherwise we have to generate it */
+        Guid targetId = pTarget->m->id;
+        fGenerateUuid = targetId.isEmpty();
+        if (fGenerateUuid)
+        {
+            targetId.create();
+            /* VirtualBox::registerHardDisk() will need UUID */
+            unconst(pTarget->m->id) = targetId;
+        }
+
         PVBOXHDD hdd;
         int vrc = VDCreate(m->vdDiskIfaces, &hdd);
@@ -5027 +5074 @@
                 || m->state == MediumState_LockedWrite);
 
-        /* unlock before the potentially lengthy operation */
-        thisLock.leave();
+        /* the two media are now protected by their non-default states;
+           unlock the media before the potentially lengthy operation */
+        mediaLock.leave();
 
         try
@@ -5074 +5122 @@
     if (SUCCEEDED(rc))
     {
-        /* we set mParent & children() (note that thatLock is released
-         * here), but lock VirtualBox first to follow the rule */
-        AutoWriteLock alock1(m->pVirtualBox COMMA_LOCKVAL_SRC_POS);
-        AutoWriteLock alock2(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+        AutoWriteLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
         Assert(pTarget->m->pParent.isNull());
@@ -5095 +5140 @@
          * Created state only on success (leaving an orphan file is
          * better than breaking media registry consistency) */
-        rc = m->pVirtualBox->registerHardDisk(pTarget);
+        rc = m->pVirtualBox->registerHardDisk(pTarget, &fNeedsSaveSettings);
 
         if (FAILED(rc))
@@ -5102 +5147 @@
     }
 
-    thisLock.maybeEnter();
+    AutoMultiWriteLock2 mediaLock(this, pTarget COMMA_LOCKVAL_SRC_POS);
 
     if (SUCCEEDED(rc))
@@ -5119 +5164 @@
 
         /* reset UUID to prevent it from being reused next time */
-        if (generateUuid)
+        if (fGenerateUuid)
             unconst(pTarget->m->id).clear();
     }
@@ -5132 +5177 @@
             AssertComRC(rc2);
         }
-    }
+
+        if (fNeedsSaveSettings)
+        {
+            mediaLock.leave();
+            AutoWriteLock vboxlock(m->pVirtualBox COMMA_LOCKVAL_SRC_POS);
+            m->pVirtualBox->saveSettings();
+        }
+    }
+    else
+        // synchronous mode: report save settings result to caller
+        if (task.m_pfNeedsSaveSettings)
+            *task.m_pfNeedsSaveSettings = fNeedsSaveSettings;
 
     /* deregister the task registered in createDiffStorage() */
@@ -5146 +5202 @@
 /**
  * Implementation code called from Medium::taskThread for the "merge" task.
+ *
+ * This task always gets started from Medium::mergeTo() and can run
+ * synchronously or asynchrously depending on the "wait" parameter passed to that
+ * function. If we run synchronously, the caller expects the bool *pfNeedsSaveSettings
+ * to be set before returning; otherwise (in asynchronous mode), we save the settings
+ * ourselves.
+ *
  * @param task
  * @param pvdOperationIfaces
@@ -5182 +5245 @@
 
             for (MergeChain::const_iterator it = chain->begin();
-                    it != chain->end(); ++ it)
+                 it != chain->end();
+                 ++it)
             {
                 /* complex sanity (sane complexity) */
@@ -5201 +5265 @@
                 /* open the first image with VDOPEN_FLAGS_INFO because
                  * it's not necessarily the base one */
-                vrc = VDOpen(hdd, (*it)->m->strFormat.c_str(),
-                                (*it)->m->strLocationFull.c_str(),
-                                it == chain->begin() ?
-                                    VD_OPEN_FLAGS_INFO : 0,
-                                (*it)->m->vdDiskIfaces);
+                vrc = VDOpen(hdd,
+                             (*it)->m->strFormat.c_str(),
+                             (*it)->m->strLocationFull.c_str(),
+                             it == chain->begin() ? VD_OPEN_FLAGS_INFO : 0,
+                             (*it)->m->vdDiskIfaces);
                 if (RT_FAILURE(vrc))
                     throw vrc;
@@ -5238 +5302 @@
                 {
                     for (MediaList::const_iterator it = chain->children().begin();
-                            it != chain->children().end();
-                            ++it)
+                         it != chain->children().end();
+                         ++it)
                     {
                         /* VD_OPEN_FLAGS_INFO since UUID is wrong yet */
-                        vrc = VDOpen(hdd, (*it)->m->strFormat.c_str(),
-                                        (*it)->m->strLocationFull.c_str(),
-                                        VD_OPEN_FLAGS_INFO,
-                                        (*it)->m->vdDiskIfaces);
+                        vrc = VDOpen(hdd,
+                                     (*it)->m->strFormat.c_str(),
+                                     (*it)->m->strLocationFull.c_str(),
+                                     VD_OPEN_FLAGS_INFO,
+                                     (*it)->m->vdDiskIfaces);
                         if (RT_FAILURE(vrc))
                             throw vrc;
 
                         vrc = VDSetParentUuid(hdd, 1,
-                                                chain->target()->m->id);
+                                              chain->target()->m->id);
                         if (RT_FAILURE(vrc))
                             throw vrc;
@@ -5277 +5342 @@
     HRESULT rc2;
 
-    bool saveSettingsFailed = false;
-
     if (SUCCEEDED(rc))
     {
@@ -5284 +5347 @@
          * VDMerge; reparent the last one and uninitialize deleted */
 
-        /* we set mParent & children() (note that thatLock is released
-         * here), but lock VirtualBox first to follow the rule */
-        AutoWriteLock alock1(m->pVirtualBox COMMA_LOCKVAL_SRC_POS);
-        AutoWriteLock alock2(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+        AutoWriteLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
         Medium *pSource = chain->source();
@@ -5296 +5356 @@
             /* first, unregister the target since it may become a base
              * hard disk which needs re-registration */
-            rc2 = pTarget->m->pVirtualBox->unregisterHardDisk(pTarget, false /* aSaveSettings */);
+            rc2 = m->pVirtualBox->unregisterHardDisk(pTarget, NULL /*&fNeedsSaveSettings*/);
             AssertComRC(rc2);
 
@@ -5310 +5370 @@
 
             /* then, register again */
-            rc2 = pTarget->m->pVirtualBox->registerHardDisk(pTarget, false /* aSaveSettings */);
+            rc2 = m->pVirtualBox->registerHardDisk(pTarget, NULL /*&fNeedsSaveSettings*/);
             AssertComRC(rc2);
         }
@@ -5331 +5391 @@
 
                 for (MediaList::const_iterator it = children.begin();
-                        it != children.end();
-                        ++it)
+                     it != children.end();
+                     ++it)
                 {
                     AutoWriteLock childLock(*it COMMA_LOCKVAL_SRC_POS);
@@ -5344 +5404 @@
         }
 
-        /* try to save the hard disk registry */
-        rc = m->pVirtualBox->saveSettings();
-
-        if (SUCCEEDED(rc))
-        {
-            /* unregister and uninitialize all hard disks in the chain
-             * but the target */
-
-            for (MergeChain::iterator it = chain->begin();
-                    it != chain->end();)
+        /* unregister and uninitialize all hard disks in the chain but the target */
+        for (MergeChain::iterator it = chain->begin();
+             it != chain->end();
+            )
+        {
+            if (*it == chain->target())
             {
-                if (*it == chain->target())
-                {
-                    ++ it;
-                    continue;
-                }
-
-                rc2 = (*it)->m->pVirtualBox->
-                    unregisterHardDisk(*it, false /* aSaveSettings */);
-                AssertComRC(rc2);
-
-                /* now, uninitialize the deleted hard disk (note that
-                 * due to the Deleting state, uninit() will not touch
-                 * the parent-child relationship so we need to
-                 * uninitialize each disk individually) */
-
-                /* note that the operation initiator hard disk (which is
-                 * normally also the source hard disk) is a special case
-                 * -- there is one more caller added by Task to it which
-                 * we must release. Also, if we are in sync mode, the
-                 * caller may still hold an AutoCaller instance for it
-                 * and therefore we cannot uninit() it (it's therefore
-                 * the caller's responsibility) */
-                if (*it == this)
-                    task.m_autoCaller.release();
-
-                /* release the caller added by MergeChain before
-                 * uninit() */
-                (*it)->releaseCaller();
-
-                if (fIsAsync || *it != this)
-                    (*it)->uninit();
-
-                /* delete (to prevent uninitialization in MergeChain
-                 * dtor) and advance to the next item */
-                it = chain->erase(it);
+                ++it;
+                continue;
             }
 
-            /* Note that states of all other hard disks (target, parent,
-             * children) will be restored by the MergeChain dtor */
-        }
-        else
-        {
-            /* too bad if we fail, but we'll need to rollback everything
-             * we did above to at least keep the HD tree in sync with
-             * the current registry on disk */
-
-            saveSettingsFailed = true;
-
-            /// @todo NEWMEDIA implement a proper undo
-
-            AssertFailed();
-        }
-    }
+            rc2 = (*it)->m->pVirtualBox->unregisterHardDisk(*it, NULL /*pfNeedsSaveSettings*/);
+            AssertComRC(rc2);
+
+            /* now, uninitialize the deleted hard disk (note that
+             * due to the Deleting state, uninit() will not touch
+             * the parent-child relationship so we need to
+             * uninitialize each disk individually) */
+
+            /* note that the operation initiator hard disk (which is
+             * normally also the source hard disk) is a special case
+             * -- there is one more caller added by Task to it which
+             * we must release. Also, if we are in sync mode, the
+             * caller may still hold an AutoCaller instance for it
+             * and therefore we cannot uninit() it (it's therefore
+             * the caller's responsibility) */
+            if (*it == this)
+                task.m_autoCaller.release();
+
+            /* release the caller added by MergeChain before uninit() */
+            (*it)->releaseCaller();
+
+            if (fIsAsync || *it != this)
+                (*it)->uninit();
+
+            /* delete (to prevent uninitialization in MergeChain
+                * dtor) and advance to the next item */
+            it = chain->erase(it);
+        }
+    }
+
+    if (fIsAsync)
+    {
+        // in asynchronous mode, save settings now
+        AutoWriteLock vboxlock(m->pVirtualBox COMMA_LOCKVAL_SRC_POS);
+        m->pVirtualBox->saveSettings();
+    }
+    else
+        // synchronous mode: report save settings result to caller
+        if (task.m_pfNeedsSaveSettings)
+            *task.m_pfNeedsSaveSettings = true;
 
     if (FAILED(rc))
@@ -5424 +5471 @@
         if (!fIsAsync)
             task.d.chain.release();
-
-        NOREF(saveSettingsFailed);
     }
 
@@ -5433 +5478 @@
 /**
  * Implementation code called from Medium::taskThread for the "clone" task.
+ * This only gets started from Medium::CloneTo() and always runs asynchronously.
+ * As a result, we always save the VirtualBox.xml file when we're done here.
+ *
  * @param task
  * @param pvdOperationIfaces
@@ -5446 +5494 @@
     ComObjPtr<Medium> &pParent = task.d.parentDisk;
 
-    /* Lock all in {parent,child} order. The lock is also used as a
-     * signal from the task initiator (which releases it only after
-     * RTThreadCreate()) that we can start the job. */
-    AutoMultiWriteLock3 thisLock(this, pTarget, pParent COMMA_LOCKVAL_SRC_POS);
-
-    ImageChain *srcChain = task.d.source.get();
-    ImageChain *parentChain = task.d.parent.get();
+    bool fCreatingTarget;
 
     uint64_t size = 0, logicalSize = 0;
-
-    /* The object may request a specific UUID (through a special form of
-     * the setLocation() argument). Otherwise we have to generate it */
-    Guid targetId = pTarget->m->id;
-    bool generateUuid = targetId.isEmpty();
-    if (generateUuid)
-    {
-        targetId.create();
-        /* VirtualBox::registerHardDisk() will need UUID */
-        unconst(pTarget->m->id) = targetId;
-    }
+    bool fGenerateUuid = false;
 
     try
     {
+        /* Lock all in {parent,child} order. The lock is also used as a
+         * signal from the task initiator (which releases it only after
+         * RTThreadCreate()) that we can start the job. */
+        AutoMultiWriteLock3 thisLock(this, pTarget, pParent COMMA_LOCKVAL_SRC_POS);
+
+        fCreatingTarget = pTarget->m->state == MediumState_Creating;
+
+        ImageChain *srcChain = task.d.source.get();
+        ImageChain *parentChain = task.d.parent.get();
+
+        /* The object may request a specific UUID (through a special form of
+         * the setLocation() argument). Otherwise we have to generate it */
+        Guid targetId = pTarget->m->id;
+        fGenerateUuid = targetId.isEmpty();
+        if (fGenerateUuid)
+        {
+            targetId.create();
+            /* VirtualBox::registerHardDisk() will need UUID */
+            unconst(pTarget->m->id) = targetId;
+        }
+
         PVBOXHDD hdd;
         int vrc = VDCreate(m->vdDiskIfaces, &hdd);
@@ -5546 +5599 @@
                              targetHdd,
                              targetFormat.c_str(),
-                             pTarget->m->state == MediumState_Creating ? targetLocation.raw() : (char *)NULL,
+                             (fCreatingTarget) ? targetLocation.raw() : (char *)NULL,
                              false,
                              0,
@@ -5573 +5626 @@
 
     /* Only do the parent changes for newly created images. */
-    if (pTarget->m->state == MediumState_Creating)
-    {
-        if (SUCCEEDED(rc))
-        {
-            /* we set mParent & children() (note that thatLock is released
-                * here), but lock VirtualBox first to follow the rule */
-            AutoWriteLock alock1(m->pVirtualBox COMMA_LOCKVAL_SRC_POS);
-            AutoWriteLock alock2(m->pVirtualBox->hardDiskTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-
-            Assert(pTarget->m->pParent.isNull());
-
-            if (pParent)
-            {
-                /* associate the clone with the parent and deassociate
-                    * from VirtualBox */
-                pTarget->m->pParent = pParent;
-                pParent->m->llChildren.push_back(pTarget);
-
-                /* register with mVirtualBox as the last step and move to
-                    * Created state only on success (leaving an orphan file is
-                    * better than breaking media registry consistency) */
-                rc = pParent->m->pVirtualBox->registerHardDisk(pTarget);
-
-                if (FAILED(rc))
-                    /* break parent association on failure to register */
-                    pTarget->deparent();     // removes target from parent
-            }
-            else
-            {
-                /* just register  */
-                rc = m->pVirtualBox->registerHardDisk(pTarget);
-            }
-        }
-    }
-
-    thisLock.maybeEnter();
-
-    if (pTarget->m->state == MediumState_Creating)
-    {
+    if (SUCCEEDED(rc) && fCreatingTarget)
+    {
+        /* we set mParent & children() */
+        AutoWriteLock alock2(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+
+        Assert(pTarget->m->pParent.isNull());
+
+        if (pParent)
+        {
+            /* associate the clone with the parent and deassociate
+             * from VirtualBox */
+            pTarget->m->pParent = pParent;
+            pParent->m->llChildren.push_back(pTarget);
+
+            /* register with mVirtualBox as the last step and move to
+             * Created state only on success (leaving an orphan file is
+             * better than breaking media registry consistency) */
+            rc = pParent->m->pVirtualBox->registerHardDisk(pTarget, NULL /* pfNeedsSaveSettings */);
+
+            if (FAILED(rc))
+                /* break parent association on failure to register */
+                pTarget->deparent();     // removes target from parent
+        }
+        else
+        {
+            /* just register  */
+            rc = m->pVirtualBox->registerHardDisk(pTarget, NULL /* pfNeedsSaveSettings */);
+        }
+    }
+
+    if (fCreatingTarget)
+    {
+        AutoWriteLock mLock(pTarget COMMA_LOCKVAL_SRC_POS);
+
         if (SUCCEEDED(rc))
         {
@@ -5625 +5673 @@
 
             /* reset UUID to prevent it from being reused next time */
-            if (generateUuid)
+            if (fGenerateUuid)
                 unconst(pTarget->m->id).clear();
         }
+    }
+
+    // now, at the end of this task (always asynchronous), save the settings
+    {
+        AutoWriteLock vboxlock(m->pVirtualBox COMMA_LOCKVAL_SRC_POS);
+        m->pVirtualBox->saveSettings();
     }
 
@@ -5643 +5697 @@
 /**
  * Implementation code called from Medium::taskThread for the "delete" task.
+ *
+ * This task always gets started from Medium::deleteStorage() and can run
+ * synchronously or asynchrously depending on the "wait" parameter passed to that
+ * function.
+ *
  * @return
  */
@@ -5649 +5708 @@
     HRESULT rc = S_OK;
 
-    /* The lock is also used as a signal from the task initiator (which
-     * releases it only after RTThreadCreate()) that we can start the job */
-    AutoWriteLock thisLock(this COMMA_LOCKVAL_SRC_POS);
-
     try
     {
+        /* The lock is also used as a signal from the task initiator (which
+         * releases it only after RTThreadCreate()) that we can start the job */
+        AutoWriteLock thisLock(this COMMA_LOCKVAL_SRC_POS);
+
         PVBOXHDD hdd;
         int vrc = VDCreate(m->vdDiskIfaces, &hdd);
@@ -5664 +5723 @@
         /* unlock before the potentially lengthy operation */
         Assert(m->state == MediumState_Deleting);
-        thisLock.leave();
+        thisLock.release();
 
         try
@@ -5688 +5747 @@
     catch (HRESULT aRC) { rc = aRC; }
 
-    thisLock.maybeEnter();
+    AutoWriteLock thisLock(this COMMA_LOCKVAL_SRC_POS);
 
     /* go to the NotCreated state even on failure since the storage
-        * may have been already partially deleted and cannot be used any
-        * more. One will be able to manually re-open the storage if really
-        * needed to re-register it. */
+     * may have been already partially deleted and cannot be used any
+     * more. One will be able to manually re-open the storage if really
+     * needed to re-register it. */
     m->state = MediumState_NotCreated;
 
@@ -5704 +5763 @@
 /**
  * Implementation code called from Medium::taskThread for the "reset" task.
+ *
+ * This always gets started asynchronously from Medium::Reset().
+ *
  * @param task
  * @param pvdOperationIfaces
@@ -5714 +5776 @@
     PVDINTERFACE vdOperationIfaces = (PVDINTERFACE)pvdOperationIfaces;
 
-    /* The lock is also used as a signal from the task initiator (which
-     * releases it only after RTThreadCreate()) that we can start the job */
-    AutoWriteLock thisLock(this COMMA_LOCKVAL_SRC_POS);
-
-    /// @todo Below we use a pair of delete/create operations to reset
-    /// the diff contents but the most efficient way will of course be
-    /// to add a VDResetDiff() API call
-
     uint64_t size = 0, logicalSize = 0;
 
     try
     {
+        /* The lock is also used as a signal from the task initiator (which
+         * releases it only after RTThreadCreate()) that we can start the job */
+        AutoWriteLock thisLock(this COMMA_LOCKVAL_SRC_POS);
+
+        /// @todo Below we use a pair of delete/create operations to reset
+        /// the diff contents but the most efficient way will of course be
+        /// to add a VDResetDiff() API call
+
         PVBOXHDD hdd;
         int vrc = VDCreate(m->vdDiskIfaces, &hdd);
@@ -5742 +5804 @@
 
         /* unlock before the potentially lengthy operation */
-        thisLock.leave();
+        thisLock.release();
 
         try
         {
             /* first, delete the storage unit */
-            vrc = VDOpen(hdd, format.c_str(), location.c_str(),
-                            VD_OPEN_FLAGS_READONLY | VD_OPEN_FLAGS_INFO,
-                            m->vdDiskIfaces);
+            vrc = VDOpen(hdd,
+                         format.c_str(),
+                         location.c_str(),
+                         VD_OPEN_FLAGS_READONLY | VD_OPEN_FLAGS_INFO,
+                         m->vdDiskIfaces);
             if (RT_SUCCESS(vrc))
                 vrc = VDClose(hdd, true /* fDelete */);
 
             if (RT_FAILURE(vrc))
-            {
                 throw setError(E_FAIL,
-                                tr("Could not delete the hard disk storage unit '%s'%s"),
-                                location.raw(), vdError(vrc).raw());
-            }
+                               tr("Could not delete the hard disk storage unit '%s'%s"),
+                               location.raw(), vdError(vrc).raw());
 
             /* next, create it again */
-            vrc = VDOpen(hdd, parentFormat.c_str(), parentLocation.c_str(),
-                            VD_OPEN_FLAGS_READONLY | VD_OPEN_FLAGS_INFO,
-                            m->vdDiskIfaces);
+            vrc = VDOpen(hdd,
+                         parentFormat.c_str(),
+                         parentLocation.c_str(),
+                         VD_OPEN_FLAGS_READONLY | VD_OPEN_FLAGS_INFO,
+                         m->vdDiskIfaces);
             if (RT_FAILURE(vrc))
-            {
                 throw setError(E_FAIL,
                                 tr("Could not open the hard disk storage unit '%s'%s"),
                                 parentLocation.raw(), vdError(vrc).raw());
-            }
-
-            vrc = VDCreateDiff(hdd, format.c_str(), location.c_str(),
-                                /// @todo use the same image variant as before
-                                VD_IMAGE_FLAGS_NONE,
-                                NULL, id.raw(),
-                                parentId.raw(),
-                                VD_OPEN_FLAGS_NORMAL,
-                                m->vdDiskIfaces,
-                                vdOperationIfaces);
+
+            vrc = VDCreateDiff(hdd,
+                               format.c_str(),
+                               location.c_str(),
+                               /// @todo use the same image variant as before
+                               VD_IMAGE_FLAGS_NONE,
+                               NULL,
+                               id.raw(),
+                               parentId.raw(),
+                               VD_OPEN_FLAGS_NORMAL,
+                               m->vdDiskIfaces,
+                               vdOperationIfaces);
             if (RT_FAILURE(vrc))
-            {
                 throw setError(E_FAIL,
                                 tr("Could not create the differencing hard disk storage unit '%s'%s"),
                                 location.raw(), vdError(vrc).raw());
-            }
 
             size = VDGetFileSize(hdd, 1);
@@ -5795 +5858 @@
     catch (HRESULT aRC) { rc = aRC; }
 
-    thisLock.enter();
+    AutoWriteLock thisLock(this COMMA_LOCKVAL_SRC_POS);
 
     m->size = size;

trunk/src/VBox/Main/SnapshotImpl.cpp

@@ -1226 +1226 @@
     AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
 
-    /* saveSettings() needs mParent lock */
-    AutoMultiWriteLock2 alock(mParent, this COMMA_LOCKVAL_SRC_POS);
+    // if this becomes true, we need to call VirtualBox::saveSettings() in the end
+    bool fNeedsSaveSettings = false;
+
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
     AssertReturn(    !Global::IsOnlineOrTransient(mData->mMachineState)
@@ -1241 +1243 @@
         /* save all current settings to ensure current changes are committed and
          * hard disks are fixed up */
+        alock.release();
+
+        AutoWriteLock vboxLock(mParent COMMA_LOCKVAL_SRC_POS);
         HRESULT rc = saveSettings();
         if (FAILED(rc)) return rc;
+
+        alock.acquire();
     }
 
@@ -1305 +1312 @@
                                  aConsoleProgress,
                                  1,            // operation weight; must be the same as in Console::TakeSnapshot()
-                                 !!fTakingSnapshotOnline);
+                                 !!fTakingSnapshotOnline,
+                                 &fNeedsSaveSettings);
         if (FAILED(rc))
             throw rc;
@@ -1320 +1328 @@
                                                1);        // weight
 
-            /* Leave the lock before a lengthy operation (mMachineState is
-            * MachineState_Saving here) */
-            alock.leave();
+            /* Leave the lock before a lengthy operation (machine is protected
+             * by "Saving" machine state now) */
+            alock.release();
 
             /* copy the state file */
@@ -1330 +1338 @@
                                    progressCallback,
                                    aConsoleProgress);
-            alock.enter();
+            alock.acquire();
 
             if (RT_FAILURE(vrc))
-            {
                 /** @todo r=bird: Delete stateTo when appropriate. */
                 throw setError(E_FAIL,
@@ -1340 +1347 @@
                                stateTo.raw(),
                                vrc);
-            }
         }
     }
@@ -1347 +1353 @@
         LogThisFunc(("Caught %Rhrc [%s]\n", hrc, Global::stringifyMachineState(mData->mMachineState) ));
         if (    mSnapshotData.mLastState != mData->mMachineState
-            &&  (  mSnapshotData.mLastState == MachineState_Running
-                 ? mData->mMachineState == MachineState_LiveSnapshotting
-                 : mData->mMachineState == MachineState_Saving)
+             && (   mSnapshotData.mLastState == MachineState_Running
+                  ? mData->mMachineState == MachineState_LiveSnapshotting
+                  : mData->mMachineState == MachineState_Saving)
            )
             setMachineState(mSnapshotData.mLastState);
@@ -1359 +1365 @@
 
         rc = hrc;
+
+        // @todo r=dj what with the implicit diff that we created above? this is never cleaned up
     }
 
@@ -1366 +1374 @@
         *aStateFilePath = NULL;
 
+    // @todo r=dj normally we would need to save the settings if fNeedsSaveSettings was set to true,
+    // but since we have no error handling that cleans up the diff image that might have gotten created,
+    // there's no point in saving the disk registry at this point either... this needs fixing.
+
     LogFlowThisFunc(("LEAVE - %Rhrc [%s]\n", rc, Global::stringifyMachineState(mData->mMachineState) ));
     return rc;
@@ -1379 +1391 @@
  * BeginTakingSnapshot() call, to clean up the server side.
  *
- * @note Locks this object for writing.
+ * @note Locks VirtualBox and this object for writing.
  *
  * @param aSuccess Whether Console was successful with the client-side snapshot things.
@@ -1391 +1403 @@
     AssertComRCReturn (autoCaller.rc(), autoCaller.rc());
 
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+    AutoMultiWriteLock2 alock(mParent, this COMMA_LOCKVAL_SRC_POS);
 
     AssertReturn(   !aSuccess
                  || (    (    mData->mMachineState == MachineState_Saving
-                          ||  mData->mMachineState == MachineState_LiveSnapshotting)
-                     &&  mSnapshotData.mLastState != MachineState_Null
-                     &&  !mSnapshotData.mSnapshot.isNull()
+                           || mData->mMachineState == MachineState_LiveSnapshotting)
+                      && mSnapshotData.mLastState != MachineState_Null
+                      && !mSnapshotData.mSnapshot.isNull()
                     )
                  , E_FAIL);
@@ -1407 +1419 @@
      * all to avoid races.
      */
-    if (   mData->mMachineState != mSnapshotData.mLastState
-        && mSnapshotData.mLastState != MachineState_Running)
+    if (    mData->mMachineState != mSnapshotData.mLastState
+         && mSnapshotData.mLastState != MachineState_Running
+       )
         setMachineState(mSnapshotData.mLastState);
-
-    return endTakingSnapshot(aSuccess);
-}
-
-/**
- * Internal helper method to finalize taking a snapshot. Gets called from
- * SessionMachine::EndTakingSnapshot() to finalize the server-side
- * parts of snapshotting.
- *
- * This also gets called from SessionMachine::uninit() if an untaken
- * snapshot needs cleaning up.
- *
- * Expected to be called after completing *all* the tasks related to
- * taking the snapshot, either successfully or unsuccessfilly.
- *
- * @param aSuccess  TRUE if the snapshot has been taken successfully.
- *
- * @note Locks this objects for writing.
- */
-HRESULT SessionMachine::endTakingSnapshot(BOOL aSuccess)
-{
-    LogFlowThisFuncEnter();
-
-    AutoCaller autoCaller(this);
-    AssertComRCReturn (autoCaller.rc(), autoCaller.rc());
-
-    AutoMultiWriteLock2 alock(mParent, this COMMA_LOCKVAL_SRC_POS);
-            // saveSettings needs VirtualBox lock
-
-    AssertReturn(!mSnapshotData.mSnapshot.isNull(), E_FAIL);
-
-    MultiResult rc(S_OK);
 
     ComObjPtr<Snapshot> pOldFirstSnap = mData->mFirstSnapshot;
@@ -1447 +1428 @@
 
     bool fOnline = Global::IsOnline(mSnapshotData.mLastState);
+
+    HRESULT rc = S_OK;
 
     if (aSuccess)
@@ -1468 +1451 @@
     {
         /* associate old hard disks with the snapshot and do locking/unlocking*/
-        fixupMedia(true /* aCommit */, fOnline);
+        commitMedia(fOnline);
 
         /* inform callbacks */
@@ -1478 +1461 @@
         /* delete all differencing hard disks created (this will also attach
          * their parents back by rolling back mMediaData) */
-        fixupMedia(false /* aCommit */);
+        rollbackMedia();
 
         mData->mFirstSnapshot = pOldFirstSnap;      // might have been changed above
@@ -1494 +1477 @@
     mSnapshotData.mSnapshot.setNull();
 
-    LogFlowThisFuncLeave();
     return rc;
 }
@@ -1663 +1645 @@
     }
 
-    /* saveSettings() needs mParent lock */
-    AutoWriteLock vboxLock(mParent COMMA_LOCKVAL_SRC_POS);
-
-    /* @todo We don't need mParent lock so far so unlock() it. Better is to
-     * provide an AutoWriteLock argument that lets create a non-locking
-     * instance */
-    vboxLock.release();
-
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
@@ -1682 +1656 @@
 
     bool stateRestored = false;
+    bool fNeedsSaveSettings = false;
 
     try
@@ -1724 +1699 @@
                                      aTask.pProgress,
                                      1,
-                                     false /* aOnline */);
+                                     false /* aOnline */,
+                                     &fNeedsSaveSettings);
             if (FAILED(rc)) throw rc;
 
@@ -1809 +1785 @@
 
         int saveFlags = 0;
-
-        /* @todo saveSettings() below needs a VirtualBox write lock and we need
-         * to leave this object's lock to do this to follow the {parent-child}
-         * locking rule. This is the last chance to do that while we are still
-         * in a protective state which allows us to temporarily leave the lock*/
-        alock.release();
-        vboxLock.acquire();
-        alock.acquire();
 
         /* we have already discarded the current state, so set the execution
@@ -1864 +1832 @@
         }
 
+        alock.leave();
+
+        AutoWriteLock vboxLock(mParent COMMA_LOCKVAL_SRC_POS);
+        if (fNeedsSaveSettings)
+            // VirtualBox.xml needs saving too: must not hold machine lock at this point!
+            mParent->saveSettings();
+
+        alock.enter();
+
         // save all settings, reset the modified flag and commit;
         rc = saveSettings(SaveS_ResetCurStateModified | saveFlags);
@@ -1877 +1854 @@
             LogFlowThisFunc(("Deleting old current state in differencing image '%s'\n", pMedium->getName().raw()));
 
-            HRESULT rc2 = pMedium->deleteStorageAndWait();
+            HRESULT rc2 = pMedium->deleteStorageAndWait(NULL /*aProgress*/, NULL /*pfNeedsSaveSettings*/ );
             // ignore errors here because we cannot roll back after saveSettings() above
             if (SUCCEEDED(rc2))
@@ -2144 +2121 @@
     MediumDiscardRecList toDiscard;
 
-    bool settingsChanged = false;
+    bool fMachineSettingsChanged = false;       // Machine
+    bool fNeedsSaveSettings = false;            // VirtualBox.xml
 
     try
@@ -2306 +2284 @@
         /// @todo NEWMEDIA maybe save everything in one operation in place of
         ///  saveSnapshotSettings() above
-        settingsChanged = true;
+        fMachineSettingsChanged = true;
 
         /* third pass: */
@@ -2323 +2301 @@
             rc = it->hd->discard(aTask.pProgress,
                                  (ULONG)(it->hd->getSize() / _1M),          // weight
-                                 it->chain);
+                                 it->chain,
+                                 &fNeedsSaveSettings);
             if (FAILED(rc)) throw rc;
 
@@ -2376 +2355 @@
         updateMachineStateOnClient();
 
-        if (settingsChanged)
+        if (fMachineSettingsChanged || fNeedsSaveSettings)
         {
             // saveSettings needs VirtualBox write lock in addition to our own
             // (parent -> child locking order!)
             AutoWriteLock vboxLock(mParent COMMA_LOCKVAL_SRC_POS);
-            alock.acquire();
-
-            saveSettings(SaveS_InformCallbacksAnyway);
+
+            if (fMachineSettingsChanged)
+            {
+                alock.acquire();
+                saveSettings(SaveS_InformCallbacksAnyway);
+            }
+
+            if (fNeedsSaveSettings)
+                mParent->saveSettings();
         }
     }

trunk/src/VBox/Main/VirtualBoxImpl.cpp

@@ -520 +520 @@
 HRESULT VirtualBox::initMedia()
 {
+    AutoWriteLock treeLock(getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+
     HRESULT rc = S_OK;
     settings::MediaList::const_iterator it;
@@ -536 +538 @@
         if (FAILED(rc)) return rc;
 
-        rc = registerHardDisk(pHardDisk, false /* aSaveRegistry */);
+        rc = registerHardDisk(pHardDisk, NULL /*pfNeedsSaveSettings*/ );
         if (FAILED(rc)) return rc;
     }
@@ -551 +553 @@
         if (FAILED(rc)) return rc;
 
-        rc = registerImage(pImage, DeviceType_DVD, false /* aSaveRegistry */);
+        rc = registerImage(pImage, DeviceType_DVD, NULL /*pfNeedsSaveSettings*/ );
         if (FAILED(rc)) return rc;
     }
@@ -566 +568 @@
         if (FAILED(rc)) return rc;
 
-        rc = registerImage(pImage, DeviceType_Floppy, false /* aSaveRegistry */);
+        rc = registerImage(pImage, DeviceType_Floppy, NULL /*pfNeedsSaveSettings*/ );
         if (FAILED(rc)) return rc;
     }
@@ -1319 +1321 @@
     HRESULT rc = E_FAIL;
 
+    bool fNeedsSaveSettings = false;
+
     ComObjPtr<Medium> hardDisk;
     hardDisk.createObject();
-    rc = hardDisk->init(this, format.raw(), aLocation);
+    rc = hardDisk->init(this, format.raw(), aLocation, &fNeedsSaveSettings);
 
     if (SUCCEEDED(rc))
         hardDisk.queryInterfaceTo(aHardDisk);
+
+    if (fNeedsSaveSettings)
+    {
+        AutoWriteLock vboxlock(this COMMA_LOCKVAL_SRC_POS);
+        saveSettings();
+    }
 
     return rc;
@@ -1362 +1372 @@
                         (accessMode == AccessMode_ReadWrite) ? Medium::OpenReadWrite : Medium::OpenReadOnly,
                         DeviceType_HardDisk,
-                        aSetImageId, imageId,
-                        aSetParentId, parentId);
+                        aSetImageId,
+                        imageId,
+                        aSetParentId,
+                        parentId);
 
     if (SUCCEEDED(rc))
     {
-        rc = registerHardDisk (hardDisk);
+        bool fNeedsSaveSettings = false;
+        rc = registerHardDisk(hardDisk, &fNeedsSaveSettings);
 
         /* Note that it's important to call uninit() on failure to register
@@ -1377 +1390 @@
         else
             hardDisk->uninit();
+
+        if (fNeedsSaveSettings)
+        {
+            AutoWriteLock vboxlock(this COMMA_LOCKVAL_SRC_POS);
+            saveSettings();
+        }
     }
 
@@ -1440 +1459 @@
     if (SUCCEEDED(rc))
     {
-        rc = registerImage(image, DeviceType_DVD);
+        bool fNeedsSaveSettings = false;
+        rc = registerImage(image, DeviceType_DVD, &fNeedsSaveSettings);
 
         if (SUCCEEDED(rc))
             image.queryInterfaceTo(aDVDImage);
+
+        if (fNeedsSaveSettings)
+        {
+            AutoWriteLock vboxlock(this COMMA_LOCKVAL_SRC_POS);
+            saveSettings();
+        }
     }
 
@@ -1507 +1533 @@
     if (SUCCEEDED(rc))
     {
-        rc = registerImage(image, DeviceType_Floppy);
+        bool fNeedsSaveSettings = false;
+        rc = registerImage(image, DeviceType_Floppy, &fNeedsSaveSettings);
 
         if (SUCCEEDED(rc))
             image.queryInterfaceTo(aFloppyImage);
+
+        if (fNeedsSaveSettings)
+        {
+            AutoWriteLock vboxlock(this COMMA_LOCKVAL_SRC_POS);
+            saveSettings();
+        }
     }
 
@@ -2766 +2799 @@
  * @return S_OK when found or E_INVALIDARG when not found.
  *
- * @note Locks this object and hard disk objects for reading.
+ * @note Locks the media tree for reading.
  */
 HRESULT VirtualBox::findHardDisk(const Guid *aId,
@@ -2847 +2880 @@
  * @return S_OK when found or E_INVALIDARG when not found.
  *
- * @note Locks this object and image objects for reading.
+ * @note Locks the media tree for reading.
  */
 HRESULT VirtualBox::findDVDImage(const Guid *aId,
@@ -2925 +2958 @@
  * @return S_OK when found or E_INVALIDARG when not found.
  *
- * @note Locks this object and image objects for reading.
- */
-HRESULT VirtualBox::findFloppyImage(const Guid *aId, CBSTR aLocation,
+ * @note Locks the media tree for reading.
+ */
+HRESULT VirtualBox::findFloppyImage(const Guid *aId,
+                                    CBSTR aLocation,
                                     bool aSetError,
                                     ComObjPtr<Medium> *aImage /* = NULL */)
@@ -3144 +3178 @@
  * @param aConflict     Where to return parameters of the conflicting medium.
  *
- * @note Locks this object and media objects for reading.
+ * @note Locks the media tree and media objects for reading.
  */
 HRESULT VirtualBox::checkMediaForConflicts2(const Guid &aId,
@@ -3154 +3188 @@
     AssertReturn(!aId.isEmpty() && !aLocation.isEmpty(), E_FAIL);
 
-    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
+    AutoReadLock alock(getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
     HRESULT rc = S_OK;
@@ -3176 +3210 @@
     {
         ComObjPtr<Medium> image;
-        rc = findDVDImage (&aId, bstrLocation, false /* aSetError */, &image);
+        rc = findDVDImage(&aId, bstrLocation, false /* aSetError */, &image);
         if (SUCCEEDED(rc))
         {
@@ -3394 +3428 @@
  *
  * @param aHardDisk     Hard disk object to remember.
- * @param aSaveRegistry Whether to save the registry after adding the new disk; this is @c false when this gets called during VirtualBox initialization.
- *
- * 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 remembered. It is therefore the responsibility of the caller to
- * call this method as the last step of some action that requires registration
- * in order to make sure that only fully functional hard disk objects get
- * registered.
- *
- * @note Locks this object for writing and @a aHardDisk for reading.
+ * @param pfNeedsSaveSettings 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::registerHardDisk(Medium *aHardDisk,
-                                     bool aSaveRegistry /*= true*/)
+                                     bool *pfNeedsSaveSettings)
 {
     AssertReturn(aHardDisk != NULL, E_INVALIDARG);
@@ -3415 +3443 @@
     AutoCaller hardDiskCaller(aHardDisk);
     AssertComRCReturn(hardDiskCaller.rc(), hardDiskCaller.rc());
+
+    // caller must hold the media tree write lock
+    Assert(getMediaTreeLockHandle().isWriteLockOnCurrentThread());
 
     Guid id;
@@ -3427 +3458 @@
 
     HRESULT rc;
-    {
-        // lock the hard disk lists (list + map) while checking for conflicts
-        AutoWriteLock al(m->ollHardDisks.getLockHandle() COMMA_LOCKVAL_SRC_POS);
-
-        Utf8Str strConflict;
-        rc = checkMediaForConflicts2(id,
-                                     strLocationFull,
-                                     strConflict);
-        if (FAILED(rc)) return rc;
-        if (strConflict.length())
-            return setError(E_INVALIDARG,
-                            tr("Cannot register the hard disk '%s' with UUID {%RTuuid} because a %s already exists in the media registry ('%s')"),
-                            strLocationFull.raw(),
-                            id.raw(),
-                            strConflict.raw(),
-                            m->strSettingsFilePath.raw());
-
-        // store base (root) hard disks in the list
-        if (pParent.isNull())
-            m->ollHardDisks.getList().push_back(aHardDisk);
-                    // access the list directly because we already locked the list above
-
-        // store all hard disks (even differencing images) in the map
-        m->mapHardDisks[id] = aHardDisk;
-    }
-
-    if (aSaveRegistry)
-    {
-        rc = saveSettings();
-        if (FAILED(rc))
-            unregisterHardDisk(aHardDisk, false /* aSaveRegistry */);
-    }
+
+    Utf8Str strConflict;
+    rc = checkMediaForConflicts2(id,
+                                 strLocationFull,
+                                 strConflict);
+    if (FAILED(rc)) return rc;
+
+    if (strConflict.length())
+        return setError(E_INVALIDARG,
+                        tr("Cannot register the hard disk '%s' with UUID {%RTuuid} because a %s already exists in the media registry ('%s')"),
+                        strLocationFull.raw(),
+                        id.raw(),
+                        strConflict.raw(),
+                        m->strSettingsFilePath.raw());
+
+    // store base (root) hard disks in the list
+    if (pParent.isNull())
+        m->ollHardDisks.getList().push_back(aHardDisk);
+                // access the list directly because we already locked the list above
+
+    // store all hard disks (even differencing images) in the map
+    m->mapHardDisks[id] = aHardDisk;
+
+    if (pfNeedsSaveSettings)
+        *pfNeedsSaveSettings = true;
 
     return rc;
@@ -3467 +3491 @@
  *
  * @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.
+ * @param pfNeedsSaveSettings 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,
-                                       bool aSaveRegistry /*= true*/)
+                                       bool *pfNeedsSaveSettings)
 {
     AssertReturn(aHardDisk != NULL, E_INVALIDARG);
@@ -3488 +3506 @@
     AutoCaller hardDiskCaller (aHardDisk);
     AssertComRCReturn(hardDiskCaller.rc(), hardDiskCaller.rc());
+
+    // caller must hold the media tree write lock
+    Assert(getMediaTreeLockHandle().isWriteLockOnCurrentThread());
 
     Guid id;
@@ -3497 +3518 @@
     }
 
-    {
-        // lock the hard disk lists (list + map)
-        AutoWriteLock al(m->ollHardDisks.getLockHandle() COMMA_LOCKVAL_SRC_POS);
-
-        // remove base (root) hard disks from the list
-        if (pParent.isNull())
-            m->ollHardDisks.getList().remove(aHardDisk);
-                    // access the list directly because we already locked the list above
-
-        // remove all hard disks (even differencing images) from map
-        size_t cnt = m->mapHardDisks.erase(id);
-        Assert(cnt == 1);
-        NOREF(cnt);
-    }
-
-    HRESULT rc = S_OK;
-
-    if (aSaveRegistry)
-    {
-        rc = saveSettings();
-        if (FAILED(rc))
-            registerHardDisk(aHardDisk, false /* aSaveRegistry */);
-    }
-
-    return rc;
+    // remove base (root) hard disks from the list
+    if (pParent.isNull())
+        m->ollHardDisks.getList().remove(aHardDisk);
+                // access the list directly because caller must have locked the list
+
+    // remove all hard disks (even differencing images) from map
+    size_t cnt = m->mapHardDisks.erase(id);
+    Assert(cnt == 1);
+    NOREF(cnt);
+
+    if (pfNeedsSaveSettings)
+        *pfNeedsSaveSettings = true;
+
+    return S_OK;
 }
 
@@ -3529 +3539 @@
  * @param argImage      Image object to remember.
  * @param argType       Either DeviceType_DVD or DeviceType_Floppy.
- * @param aSaveRegistry @c true to save the image 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 image object
- * will not be remembered. It is therefore the responsibility of the caller to
- * call this method as the last step of some action that requires registration
- * in order to make sure that only fully functional image objects get
- * registered.
- *
- * @note Locks this object for writing and @a aImage for reading.
+ * @param pfNeedsSaveSettings 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::registerImage(Medium *argImage,
                                   DeviceType_T argType,
-                                  bool argSaveRegistry /*= true*/)
+                                  bool *pfNeedsSaveSettings)
 {
     AssertReturn(argImage != NULL, E_INVALIDARG);
@@ -3551 +3555 @@
     AutoCaller imageCaller(argImage);
     AssertComRCReturn(imageCaller.rc(), imageCaller.rc());
+
+    // caller must hold the media tree write lock
+    Assert(getMediaTreeLockHandle().isWriteLockOnCurrentThread());
 
     Guid id;
@@ -3566 +3573 @@
 
     HRESULT rc;
-    {
-        // lock the images lists (list + map) while checking for conflicts
-        AutoWriteLock al(oll.getLockHandle() COMMA_LOCKVAL_SRC_POS);
-
-        Utf8Str strConflict;
-        rc = checkMediaForConflicts2(id,
-                                     strLocationFull,
-                                     strConflict);
-        if (FAILED(rc)) return rc;
-
-        if (strConflict.length())
-            return setError(VBOX_E_INVALID_OBJECT_STATE,
-                            tr("Cannot register the image '%s' with UUID {%RTuuid} because a %s already exists in the media registry ('%s')"),
-                            strLocationFull.raw(),
-                            id.raw(),
-                            strConflict.raw(),
-                            m->strSettingsFilePath.raw());
-
-        // add to the collection
-        oll.getList().push_back(argImage);
-                // access the list directly because we already locked the list above
-    }
-
-    if (argSaveRegistry)
-    {
-        rc = saveSettings();
-        if (FAILED(rc))
-            unregisterImage(argImage, argType, false /* aSaveRegistry */);
-    }
+    // lock the images lists (list + map) while checking for conflicts
+    AutoWriteLock al(oll.getLockHandle() COMMA_LOCKVAL_SRC_POS);
+
+    Utf8Str strConflict;
+    rc = checkMediaForConflicts2(id,
+                                    strLocationFull,
+                                    strConflict);
+    if (FAILED(rc)) return rc;
+
+    if (strConflict.length())
+        return setError(VBOX_E_INVALID_OBJECT_STATE,
+                        tr("Cannot register the image '%s' with UUID {%RTuuid} because a %s already exists in the media registry ('%s')"),
+                        strLocationFull.raw(),
+                        id.raw(),
+                        strConflict.raw(),
+                        m->strSettingsFilePath.raw());
+
+    // add to the collection
+    oll.getList().push_back(argImage);
+            // access the list directly because we already locked the list above
+
+    if (pfNeedsSaveSettings)
+        *pfNeedsSaveSettings = true;
 
     return rc;
@@ -3604 +3605 @@
  * @param argImage        Image object to remove.
  * @param argType         Either DeviceType_DVD or DeviceType_Floppy.
- * @param argSaveRegistry @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 image 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 aImage.
- *
- * @note Locks this object for writing and @a aImage for reading.
+ * @param pfNeedsSaveSettings 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,
-                                    bool argSaveRegistry /*= true*/)
+                                    bool *pfNeedsSaveSettings)
 {
     AssertReturn(argImage != NULL, E_INVALIDARG);
@@ -3626 +3621 @@
     AutoCaller imageCaller(argImage);
     AssertComRCReturn(imageCaller.rc(), imageCaller.rc());
+
+    // caller must hold the media tree write lock
+    Assert(getMediaTreeLockHandle().isWriteLockOnCurrentThread());
 
     Guid id;
@@ -3638 +3636 @@
     ObjectsList<Medium> &oll = (argType == DeviceType_DVD) ? m->ollDVDImages : m->ollFloppyImages;
 
-    oll.removeChild(argImage);
+    // access the list directly because the caller must have requested the lock
+    oll.getList().remove(argImage);
 
     HRESULT rc = S_OK;
 
-    if (argSaveRegistry)
-    {
-        rc = saveSettings();
-        if (FAILED(rc))
-            registerImage(argImage, argType, false /* aSaveRegistry */);
-    }
+    if (pfNeedsSaveSettings)
+        *pfNeedsSaveSettings = true;
 
     return rc;
@@ -3779 +3774 @@
 
 /**
- * Returns a lock handle used to protect changes to the hard disk hierarchy
- * (e.g. serialize access to the Medium::mParent fields and methods
- * adding/removing children). When using this lock, the following rules must
- * be obeyed:
- *
- * 1. The write lock on this handle must be either held alone on the thread
- *    or requested *after* the VirtualBox object lock. Mixing with other
- *    locks is prohibited.
- *
- * 2. The read lock on this handle may be intermixed with any other lock
- *    with the exception that it must be requested *after* the VirtualBox
- *    object lock.
- */
-RWLockHandle& VirtualBox::hardDiskTreeLockHandle()
-{
-    return m->ollHardDisks.getLockHandle();
+ * Returns the lock handle which protects the media trees (hard disks,
+ * DVDs, floppies). As opposed to version 3.1 and earlier, these lists
+ * are no longer protected by the VirtualBox lock, but by this more
+ * specialized lock. Mind the locking order: always request this lock
+ * after the VirtualBox object lock but before the locks of the media
+ * objects contained in these lists. See AutoLock.h.
+ */
+RWLockHandle& VirtualBox::getMediaTreeLockHandle()
+{
+    return m->lockMedia;
 }
 

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

@@ -889 +889 @@
                                 IProgress *aProgress,
                                 ULONG aWeight,
-                                bool aOnline);
-    HRESULT deleteImplicitDiffs();
+                                bool aOnline,
+                                bool *pfNeedsSaveSettings);
+    HRESULT deleteImplicitDiffs(bool *pfNeedsSaveSettings);
 
     MediumAttachment* findAttachment(const MediaData::AttachmentList &ll,
@@ -901 +902 @@
                                      Guid &id);
 
-    void fixupMedia(bool aCommit, bool aOnline = false);
+    void commitMedia(bool aOnline = false);
+    void rollbackMedia();
 
     bool isInOwnDir(Utf8Str *aSettingsDir = NULL);
@@ -1088 +1090 @@
 
     HRESULT endSavingState(BOOL aSuccess);
-    HRESULT endTakingSnapshot(BOOL aSuccess);
 
     typedef std::map<ComObjPtr<Machine>, MachineState_T> AffectedMachines;

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

@@ -78 +78 @@
     HRESULT init(VirtualBox *aVirtualBox,
                  CBSTR aFormat,
-                 CBSTR aLocation);
+                 CBSTR aLocation,
+                 bool *pfNeedsSaveSettings);
     HRESULT init(VirtualBox *aVirtualBox,
                  CBSTR aLocation,
@@ -193 +194 @@
      */
     HRESULT deleteStorageNoWait(ComObjPtr<Progress> &aProgress)
-    { return deleteStorage(&aProgress, false /* aWait */); }
+    { return deleteStorage(&aProgress, false /* aWait */, NULL /* pfNeedsSaveSettings */); }
 
     /**
@@ -199 +200 @@
      * blocking the current thread.
      */
-    HRESULT deleteStorageAndWait(ComObjPtr<Progress> *aProgress = NULL)
-    { return deleteStorage(aProgress, true /* aWait */); }
+    HRESULT deleteStorageAndWait(ComObjPtr<Progress> *aProgress, bool *pfNeedsSaveSettings)
+    { return deleteStorage(aProgress, true /* aWait */, pfNeedsSaveSettings); }
 
     /**
@@ -209 +210 @@
                                     MediumVariant_T aVariant,
                                     ComObjPtr<Progress> &aProgress)
-    { return createDiffStorage(aTarget, aVariant, &aProgress, false /* aWait */); }
+    { return createDiffStorage(aTarget, aVariant, &aProgress, false /* aWait */, NULL /* pfNeedsSaveSettings*/ ); }
 
     /**
@@ -217 +218 @@
     HRESULT createDiffStorageAndWait(ComObjPtr<Medium> &aTarget,
                                      MediumVariant_T aVariant,
-                                     ComObjPtr<Progress> *aProgress = NULL)
-    { return createDiffStorage(aTarget, aVariant, aProgress, true /* aWait */); }
+                                     bool *pfNeedsSaveSettings)
+    { return createDiffStorage(aTarget, aVariant, NULL /*aProgress*/, true /* aWait */, pfNeedsSaveSettings); }
 
     HRESULT prepareMergeTo(Medium *aTarget, MergeChain * &aChain,
@@ -229 +230 @@
     HRESULT mergeToNoWait(MergeChain *aChain,
                           ComObjPtr<Progress> &aProgress)
-    { return mergeTo(aChain, &aProgress, false /* aWait */); }
+    { return mergeTo(aChain, &aProgress, false /* aWait */, NULL /*pfNeedsSaveSettings*/); }
 
     /**
@@ -236 +237 @@
      */
     HRESULT mergeToAndWait(MergeChain *aChain,
-                           ComObjPtr<Progress> *aProgress = NULL)
-    { return mergeTo(aChain, aProgress, true /* aWait */); }
+                           ComObjPtr<Progress> *aProgress,
+                           bool *pfNeedsSaveSettings)
+    { return mergeTo(aChain, aProgress, true /* aWait */, pfNeedsSaveSettings); }
 
     void cancelMergeTo(MergeChain *aChain);
@@ -244 +246 @@
 
     HRESULT prepareDiscard(MergeChain * &aChain);
-    HRESULT discard(ComObjPtr<Progress> &aProgress, ULONG ulWeight, MergeChain *aChain);
+    HRESULT discard(ComObjPtr<Progress> &aProgress, ULONG ulWeight, MergeChain *aChain, bool *pfNeedsSaveSettings);
     void cancelDiscard(MergeChain *aChain);
 
@@ -273 +275 @@
      * this object's AutoMayUninitSpan and from under mVirtualBox write lock.
      */
-    HRESULT unregisterWithVirtualBox();
+    HRESULT unregisterWithVirtualBox(bool *pfNeedsSaveSettings);
 
     HRESULT setStateError();
 
-    HRESULT deleteStorage(ComObjPtr<Progress> *aProgress, bool aWait);
+    HRESULT deleteStorage(ComObjPtr<Progress> *aProgress, bool aWait, bool *pfNeedsSaveSettings);
 
     HRESULT createDiffStorage(ComObjPtr<Medium> &aTarget,
                               MediumVariant_T aVariant,
                               ComObjPtr<Progress> *aProgress,
-                              bool aWait);
+                              bool aWait,
+                              bool *pfNeedsSaveSettings);
 
     HRESULT mergeTo(MergeChain *aChain,
                     ComObjPtr<Progress> *aProgress,
-                    bool aWait);
+                    bool aWait,
+                    bool *pfNeedsSaveSettings);
 
     HRESULT setLocation(const Utf8Str &aLocation, const Utf8Str &aFormat = Utf8Str());

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

@@ -274 +274 @@
     void calculateRelativePath(const Utf8Str &strPath, Utf8Str &aResult);
 
-    HRESULT registerHardDisk(Medium *aHardDisk, bool aSaveRegistry = true);
-    HRESULT unregisterHardDisk(Medium *aHardDisk, bool aSaveRegistry = true);
-
-    HRESULT registerImage(Medium *aImage, DeviceType_T argType, bool aSaveRegistry = true);
-    HRESULT unregisterImage(Medium *aImage, DeviceType_T argType, bool aSaveRegistry = true);
+    HRESULT registerHardDisk(Medium *aHardDisk, bool *pfNeedsSaveSettings);
+    HRESULT unregisterHardDisk(Medium *aHardDisk, bool *pfNeedsSaveSettings);
+
+    HRESULT registerImage(Medium *aImage, DeviceType_T argType, bool *pfNeedsSaveSettings);
+    HRESULT unregisterImage(Medium *aImage, DeviceType_T argType, bool *pfNeedsSaveSettings);
 
     HRESULT saveSettings();
@@ -289 +289 @@
     const Utf8Str& settingsFilePath();
 
-    RWLockHandle& hardDiskTreeLockHandle();
+    RWLockHandle& getMediaTreeLockHandle();
 
     /* for VirtualBoxSupportErrorInfoImpl */