Changeset 94598 in vbox

Timestamp:

Apr 13, 2022 9:50:00 PM (3 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

150959

Message:

Main/Machine+Medium+Snapshot+VirtualBox: Recursion elimination to save stack space. Caused trouble with the current settings limits already in ASAN builds, now much higher limits would be possible, but that's not urgent. Also fix the behavior of forgetting medium objects when unregistering VMs (was previously not doing what the API documentation said in the CleanupMode_UnregisterOnly case). bugref:7717

Settings.cpp: Recursion elimination and make the handling of settings reading and writing more similar.

ValidationKit/tests/api/tdTreeDepth1.py: Improve testcase. Make sure that VM unregistering does the right thing with the associated medium objects.

Location:

trunk

Files:

• include/VBox/settings.h (modified) (6 diffs)
• src/VBox/Main/include/MachineImpl.h (modified) (2 diffs)
• src/VBox/Main/include/MediumImpl.h (modified) (3 diffs)
• src/VBox/Main/include/SnapshotImpl.h (modified) (2 diffs)
• src/VBox/Main/include/VirtualBoxImpl.h (modified) (1 diff)
• src/VBox/Main/src-server/MachineImpl.cpp (modified) (11 diffs)
• src/VBox/Main/src-server/MachineImplMoveVM.cpp (modified) (1 diff)
• src/VBox/Main/src-server/MediumImpl.cpp (modified) (13 diffs)
• src/VBox/Main/src-server/SnapshotImpl.cpp (modified) (18 diffs)
• src/VBox/Main/src-server/VirtualBoxImpl.cpp (modified) (8 diffs)
• src/VBox/Main/xml/Settings.cpp (modified) (24 diffs)
• src/VBox/ValidationKit/tests/api/tdTreeDepth1.py (modified) (6 diffs)

trunk/include/VBox/settings.h

@@ -59 +59 @@
  * XPCOM has a relatively low stack size for its workers, and we have
  * to avoid crashes due to exceeding the limit both on reading and
- * writing config files.
+ * writing config files. The bottleneck is in libxml2.
+ * Data point: a release and asan build could both handle 3800 on Debian 10.
  */
 #define SETTINGS_MEDIUM_DEPTH_MAX 300
@@ -68 +69 @@
  * to avoid crashes due to exceeding the limit both on reading and
  * writing config files. The bottleneck is reading config files with
- * deep snapshot nesting, as libxml2 needs quite some stack space,
- * so with the current stack size the margin isn't big.
+ * deep snapshot nesting, as libxml2 needs quite some stack space.
+ * Data point: a release and asan build could both handle 1300 on Debian 10.
  */
 #define SETTINGS_SNAPSHOT_DEPTH_MAX 250
@@ -257 +258 @@
                               USBDeviceFiltersList &ll);
     void readMediumOne(MediaType t, const xml::ElementNode &elmMedium, Medium &med);
-    void readMedium(MediaType t, uint32_t depth, const xml::ElementNode &elmMedium, Medium &med);
+    void readMedium(MediaType t, const xml::ElementNode &elmMedium, Medium &med);
     void readMediaRegistry(const xml::ElementNode &elmMediaRegistry, MediaRegistry &mr);
     void readNATForwardRulesMap(const xml::ElementNode  &elmParent, NATRulesMap &mapRules);
@@ -271 +272 @@
                                bool fHostMode);
     void buildMedium(MediaType t,
-                     uint32_t depth,
                      xml::ElementNode &elmMedium,
-                     const Medium &mdm);
+                     const Medium &med);
     void buildMediaRegistry(xml::ElementNode &elmParent,
                             const MediaRegistry &mr);
@@ -1409 +1409 @@
     void readStorageControllers(const xml::ElementNode &elmStorageControllers, Storage &strg);
     void readDVDAndFloppies_pre1_9(const xml::ElementNode &elmHardware, Storage &strg);
-    void readTeleporter(const xml::ElementNode *pElmTeleporter, MachineUserData *pUserData);
-    void readDebugging(const xml::ElementNode *pElmDbg, Debugging *pDbg);
-    void readAutostart(const xml::ElementNode *pElmAutostart, Autostart *pAutostart);
-    void readGroups(const xml::ElementNode *elmGroups, StringsList *pllGroups);
-    bool readSnapshot(const com::Guid &curSnapshotUuid, uint32_t depth, const xml::ElementNode &elmSnapshot, Snapshot &snap);
+    void readTeleporter(const xml::ElementNode &elmTeleporter, MachineUserData &userData);
+    void readDebugging(const xml::ElementNode &elmDbg, Debugging &dbg);
+    void readAutostart(const xml::ElementNode &elmAutostart, Autostart &autostrt);
+    void readGroups(const xml::ElementNode &elmGroups, StringsList &llGroups);
+    bool readSnapshot(const com::Guid &curSnapshotUuid, const xml::ElementNode &elmSnapshot, Snapshot &snap);
     void convertOldOSType_pre1_5(com::Utf8Str &str);
     void readMachine(const xml::ElementNode &elmMachine);
@@ -1423 +1423 @@
                                     bool fSkipRemovableMedia,
                                     std::list<xml::ElementNode*> *pllElementsWithUuidAttributes);
-    void buildDebuggingXML(xml::ElementNode *pElmParent, const Debugging *pDbg);
-    void buildAutostartXML(xml::ElementNode *pElmParent, const Autostart *pAutostart);
-    void buildGroupsXML(xml::ElementNode *pElmParent, const StringsList *pllGroups);
-    void buildSnapshotXML(uint32_t depth, xml::ElementNode &elmParent, const Snapshot &snap);
+    void buildDebuggingXML(xml::ElementNode &elmParent, const Debugging &dbg);
+    void buildAutostartXML(xml::ElementNode &elmParent, const Autostart &autostrt);
+    void buildGroupsXML(xml::ElementNode &elmParent, const StringsList &llGroups);
+    void buildSnapshotXML(xml::ElementNode &elmParent, const Snapshot &snap);
 
     void bumpSettingsVersionIfNeeded();

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

@@ -24 +24 @@
 #include "AuthLibrary.h"
 #include "VirtualBoxBase.h"
-#include "SnapshotImpl.h"
 #include "ProgressImpl.h"
 #include "VRDEServerImpl.h"
@@ -645 +644 @@
                                           const Guid *puuidRegistry);
     HRESULT i_loadSnapshot(const settings::Snapshot &data,
-                           const Guid &aCurSnapshotId,
-                           Snapshot *aParentSnapshot);
+                           const Guid &aCurSnapshotId);
     HRESULT i_loadHardware(const Guid *puuidRegistry,
                            const Guid *puuidSnapshot,

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

@@ -77 +77 @@
                     DeviceType_T aDeviceType,
                     const Guid &uuidMachineRegistry,
-                    const settings::Medium &data,
-                    const Utf8Str &strMachineFolder);
-    HRESULT initFromSettings(VirtualBox *aVirtualBox,
-                             Medium *aParent,
-                             DeviceType_T aDeviceType,
-                             const Guid &uuidMachineRegistry,
-                             const settings::Medium &data,
-                             const Utf8Str &strMachineFolder,
-                             AutoWriteLock &mediaTreeLock,
-                             ComObjPtr<Medium> *ppRegistered);
+                    const Utf8Str &strMachineFolder,
+                    const settings::Medium &data);
+    static HRESULT initFromSettings(VirtualBox *aVirtualBox,
+                                    DeviceType_T aDeviceType,
+                                    const Guid &uuidMachineRegistry,
+                                    const Utf8Str &strMachineFolder,
+                                    const settings::Medium &data,
+                                    AutoWriteLock &mediaTreeLock,
+                                    std::list<std::pair<Guid, DeviceType_T> > &uIdsForNotify);
 
     // initializer for host floppy/DVD
@@ -122 +121 @@
     bool i_addRegistryNoCallerCheck(const Guid &id);
     /* handles caller/locking itself, caller is responsible for tree lock */
-    bool i_addRegistryRecursive(const Guid &id);
+    bool i_addRegistryAll(const Guid &id);
     /* handles caller/locking itself */
     bool i_removeRegistry(const Guid& id);
     /* handles caller/locking itself, caller is responsible for tree lock */
-    bool i_removeRegistryRecursive(const Guid& id);
+    bool i_removeRegistryAll(const Guid& id);
     bool i_isInRegistry(const Guid& id);
     bool i_getFirstRegistryMachineId(Guid &uuid) const;
@@ -140 +139 @@
 
     const Guid* i_getFirstMachineBackrefId() const;
-    const Guid* i_getAnyMachineBackref() const;
+    const Guid* i_getAnyMachineBackref(const Guid &aId) const;
     const Guid* i_getFirstMachineBackrefSnapshotId() const;
     size_t i_getMachineBackRefCount() const;

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

@@ -74 +74 @@
     ULONG i_getChildrenCount();
     ULONG i_getAllChildrenCount();
-    ULONG i_getAllChildrenCountImpl();
 
     const ComObjPtr<SnapshotMachine>& i_getSnapshotMachine() const;
@@ -98 +97 @@
                                 const Utf8Str &strNewPath);
 
+    HRESULT i_saveSnapshotOne(settings::Snapshot &data) const;
     HRESULT i_saveSnapshot(settings::Snapshot &data) const;
-    HRESULT i_saveSnapshotImpl(settings::Snapshot &data) const;
-    HRESULT i_saveSnapshotImplOne(settings::Snapshot &data) const;
 
-    HRESULT i_uninitOne(AutoWriteLock &writeLock,
+    HRESULT i_uninitAll(AutoWriteLock &writeLock,
                         CleanupMode_T cleanupMode,
                         MediaList &llMedia,
                         std::list<Utf8Str> &llFilenames);
-    HRESULT i_uninitRecursively(AutoWriteLock &writeLock,
-                                CleanupMode_T cleanupMode,
-                                MediaList &llMedia,
-                                std::list<Utf8Str> &llFilenames);
 
 

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

@@ -271 +271 @@
                              AutoWriteLock &mediaTreeLock, bool fCalledFromMediumInit = false);
     HRESULT i_unregisterMedium(Medium *pMedium);
-    void i_pushMediumToListWithChildren(MediaList &llMedia, Medium *pMedium);
     HRESULT i_unregisterMachineMedia(const Guid &id);
-    HRESULT i_unregisterMachine(Machine *pMachine, const Guid &id);
+    HRESULT i_unregisterMachine(Machine *pMachine, CleanupMode_T aCleanupMode, const Guid &id);
     void i_rememberMachineNameChangeForMedia(const Utf8Str &strOldConfigDir,
                                              const Utf8Str &strNewConfigDir);

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

@@ -29 +29 @@
 #include "VirtualBoxImpl.h"
 #include "MachineImpl.h"
+#include "SnapshotImpl.h"
 #include "ClientToken.h"
 #include "ProgressImpl.h"
@@ -4949 +4950 @@
         uninit();
 
-        mParent->i_unregisterMachine(this, id);
+        mParent->i_unregisterMachine(this, CleanupMode_UnregisterOnly, id);
             // calls VirtualBox::i_saveSettings()
 
@@ -4986 +4987 @@
     if (mData->mFirstSnapshot)
     {
-        // add the media from the medium attachments of the snapshots to llMedia
-        // as well, after the "main" machine media; Snapshot::uninitRecursively()
-        // calls Machine::detachAllMedia() for the snapshot machine, recursing
-        // into the children first
+        // add the media from the medium attachments of the snapshots to
+        // llMedia as well, after the "main" machine media;
+        // Snapshot::uninitAll() calls Machine::detachAllMedia() for each
+        // snapshot machine, depth first.
 
         // Snapshot::beginDeletingSnapshot() asserts if the machine state is not this
@@ -5001 +5002 @@
 
         // GO!
-        pFirstSnapshot->i_uninitRecursively(alock, aCleanupMode, llMedia, mData->llFilesToDelete);
+        pFirstSnapshot->i_uninitAll(alock, aCleanupMode, llMedia, mData->llFilesToDelete);
 
         mData->mMachineState = oldState;
@@ -5034 +5035 @@
         (*it).queryInterfaceTo(aMedia[i].asOutParam());
 
-    mParent->i_unregisterMachine(this, id);
+    mParent->i_unregisterMachine(this, aCleanupMode, id);
             // calls VirtualBox::i_saveSettings() and VirtualBox::saveModifiedRegistries()
+
+    autoCaller.release();
+    uninit();
 
     return S_OK;
@@ -8402 +8406 @@
     if (!i_isSessionMachine() && !i_isSnapshotMachine())
     {
-        // clean up the snapshots list (Snapshot::uninit() will handle the snapshot's children recursively)
+        // clean up the snapshots list (Snapshot::uninit() will handle the snapshot's children)
         if (mData->mFirstSnapshot)
         {
-            // snapshots tree is protected by machine write lock; strictly
-            // this isn't necessary here since we're deleting the entire
-            // machine, but otherwise we assert in Snapshot::uninit()
+            // Snapshots tree is protected by machine write lock.
+            // Otherwise we assert in Snapshot::uninit()
             AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
             mData->mFirstSnapshot->uninit();
@@ -8653 +8656 @@
         // there must be only one root snapshot
         Assert(cRootSnapshots == 1);
-
         const settings::Snapshot &snap = config.llFirstSnapshot.front();
 
         rc = i_loadSnapshot(snap,
-                            config.uuidCurrentSnapshot,
-                            NULL);        // no parent == first snapshot
+                            config.uuidCurrentSnapshot);
         if (FAILED(rc)) return rc;
     }
@@ -8699 +8700 @@
 
 /**
- *  Recursively loads all snapshots starting from the given.
+ *  Loads all snapshots starting from the given settings.
  *
  *  @param data             snapshot settings.
  *  @param aCurSnapshotId   Current snapshot ID from the settings file.
- *  @param aParentSnapshot  Parent snapshot.
  */
 HRESULT Machine::i_loadSnapshot(const settings::Snapshot &data,
-                                const Guid &aCurSnapshotId,
-                                Snapshot *aParentSnapshot)
+                                const Guid &aCurSnapshotId)
 {
     AssertReturn(!i_isSnapshotMachine(), E_FAIL);
@@ -8714 +8713 @@
     HRESULT rc = S_OK;
 
-    Utf8Str strStateFile;
-    if (!data.strStateFile.isEmpty())
-    {
-        /* optional */
-        strStateFile = data.strStateFile;
-        int vrc = i_calculateFullPath(strStateFile, strStateFile);
-        if (RT_FAILURE(vrc))
-            return setErrorBoth(E_FAIL, vrc,
-                                tr("Invalid saved state file path '%s' (%Rrc)"),
-                                strStateFile.c_str(),
-                                vrc);
-    }
-
-    /* create a snapshot machine object */
-    ComObjPtr<SnapshotMachine> pSnapshotMachine;
-    pSnapshotMachine.createObject();
-    rc = pSnapshotMachine->initFromSettings(this,
-                                            data.hardware,
-                                            &data.debugging,
-                                            &data.autostart,
-                                            data.uuid.ref(),
-                                            strStateFile);
-    if (FAILED(rc)) return rc;
-
-    /* create a snapshot object */
-    ComObjPtr<Snapshot> pSnapshot;
-    pSnapshot.createObject();
-    /* initialize the snapshot */
-    rc = pSnapshot->init(mParent, // VirtualBox object
-                         data.uuid,
-                         data.strName,
-                         data.strDescription,
-                         data.timestamp,
-                         pSnapshotMachine,
-                         aParentSnapshot);
-    if (FAILED(rc)) return rc;
-
-    /* memorize the first snapshot if necessary */
-    if (!mData->mFirstSnapshot)
-        mData->mFirstSnapshot = pSnapshot;
-
-    /* memorize the current snapshot when appropriate */
-    if (    !mData->mCurrentSnapshot
-         && pSnapshot->i_getId() == aCurSnapshotId
-       )
-        mData->mCurrentSnapshot = pSnapshot;
-
-    // now create the children
-    for (settings::SnapshotsList::const_iterator
-         it = data.llChildSnapshots.begin();
-         it != data.llChildSnapshots.end();
-         ++it)
-    {
-        const settings::Snapshot &childData = *it;
-        // recurse
-        rc = i_loadSnapshot(childData,
-                            aCurSnapshotId,
-                            pSnapshot);       // parent = the one we created above
-        if (FAILED(rc)) return rc;
+    std::list<const settings::Snapshot *> llSettingsTodo;
+    llSettingsTodo.push_back(&data);
+    std::list<Snapshot *> llParentsTodo;
+    llParentsTodo.push_back(NULL);
+
+    while (llSettingsTodo.size() > 0)
+    {
+        const settings::Snapshot *current = llSettingsTodo.front();
+        llSettingsTodo.pop_front();
+        Snapshot *pParent = llParentsTodo.front();
+        llParentsTodo.pop_front();
+
+        Utf8Str strStateFile;
+        if (!current->strStateFile.isEmpty())
+        {
+            /* optional */
+            strStateFile = current->strStateFile;
+            int vrc = i_calculateFullPath(strStateFile, strStateFile);
+            if (RT_FAILURE(vrc))
+            {
+                setErrorBoth(E_FAIL, vrc,
+                             tr("Invalid saved state file path '%s' (%Rrc)"),
+                             strStateFile.c_str(), vrc);
+            }
+        }
+
+        /* create a snapshot machine object */
+        ComObjPtr<SnapshotMachine> pSnapshotMachine;
+        pSnapshotMachine.createObject();
+        rc = pSnapshotMachine->initFromSettings(this,
+                                                current->hardware,
+                                                &current->debugging,
+                                                &current->autostart,
+                                                current->uuid.ref(),
+                                                strStateFile);
+        if (FAILED(rc)) break;
+
+        /* create a snapshot object */
+        ComObjPtr<Snapshot> pSnapshot;
+        pSnapshot.createObject();
+        /* initialize the snapshot */
+        rc = pSnapshot->init(mParent, // VirtualBox object
+                             current->uuid,
+                             current->strName,
+                             current->strDescription,
+                             current->timestamp,
+                             pSnapshotMachine,
+                             pParent);
+        if (FAILED(rc)) break;
+
+        /* memorize the first snapshot if necessary */
+        if (!mData->mFirstSnapshot)
+        {
+            Assert(pParent == NULL);
+            mData->mFirstSnapshot = pSnapshot;
+        }
+
+        /* memorize the current snapshot when appropriate */
+        if (    !mData->mCurrentSnapshot
+             && pSnapshot->i_getId() == aCurSnapshotId
+           )
+            mData->mCurrentSnapshot = pSnapshot;
+
+        /* create all children */
+        std::list<settings::Snapshot>::const_iterator itBegin = current->llChildSnapshots.begin();
+        std::list<settings::Snapshot>::const_iterator itEnd = current->llChildSnapshots.end();
+        for (std::list<settings::Snapshot>::const_iterator it = itBegin; it != itEnd; ++it)
+        {
+            llSettingsTodo.push_back(&*it);
+            llParentsTodo.push_back(pSnapshot);
+        }
     }
 
@@ -10654 +10665 @@
     if (pMedium != pBase)
     {
-        /* Tree lock needed by Medium::addRegistry when recursing. */
+        /* Tree lock needed by Medium::addRegistryAll. */
         AutoReadLock treeLock(&mParent->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-        if (fCanHaveOwnMediaRegistry && pMedium->i_removeRegistryRecursive(mParent->i_getGlobalRegistryId()))
+        if (fCanHaveOwnMediaRegistry && pBase->i_removeRegistryAll(mParent->i_getGlobalRegistryId()))
         {
             treeLock.release();
@@ -10662 +10673 @@
             treeLock.acquire();
         }
-        if (pBase->i_addRegistryRecursive(uuid))
+        if (pBase->i_addRegistryAll(uuid))
         {
             treeLock.release();

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

@@ -26 +26 @@
 
 #include "MachineImplMoveVM.h"
+#include "SnapshotImpl.h"
 #include "MediumFormatImpl.h"
 #include "VirtualBoxImpl.h"

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

@@ -1213 +1213 @@
                         DeviceType_T aDeviceType,
                         const Guid &uuidMachineRegistry,
-                        const settings::Medium &data,
-                        const Utf8Str &strMachineFolder)
+                        const Utf8Str &strMachineFolder,
+                        const settings::Medium &data)
 {
     HRESULT rc;
@@ -1356 +1356 @@
  *
  * @param aVirtualBox   VirtualBox object.
- * @param aParent       Parent medium disk or NULL for a root (base) medium.
  * @param aDeviceType   Device type of the medium.
  * @param uuidMachineRegistry The registry to which this medium should be added
  *                      (global registry UUID or machine UUID).
- * @param data          Configuration settings.
  * @param strMachineFolder The machine folder with which to resolve relative
  *                      paths; if empty, then we use the VirtualBox home directory
+ * @param data          Configuration settings.
  * @param mediaTreeLock Autolock.
- * @param ppRegistered  Where to return the registered Medium object.  This is
- *                      for handling the case where the medium object we're
- *                      constructing from settings already has a registered
- *                      instance that should be used instead of this one.
- *
- * @note Locks the medium tree for writing.
+ * @param uIdsForNotify List to be updated with newly registered media.
+ *
+ * @note Assumes that the medium tree lock is held for writing. May release
+ * and lock it again. At the end it is always held.
  */
+/* static */
 HRESULT Medium::initFromSettings(VirtualBox *aVirtualBox,
-                                 Medium *aParent,
                                  DeviceType_T aDeviceType,
                                  const Guid &uuidMachineRegistry,
+                                 const Utf8Str &strMachineFolder,
                                  const settings::Medium &data,
-                                 const Utf8Str &strMachineFolder,
                                  AutoWriteLock &mediaTreeLock,
-                                 ComObjPtr<Medium> *ppRegistered)
-{
-    using namespace settings;
-
+                                 std::list<std::pair<Guid, DeviceType_T> > &uIdsForNotify)
+{
     Assert(aVirtualBox->i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
     AssertReturn(aVirtualBox, E_INVALIDARG);
 
-    /*
-     * Enclose the state transition NotReady->InInit->Ready.
-     *
-     * Note! This has to be scoped so that we can temporarily drop the
-     *       mediaTreeLock ownership on failure.
-     */
-    HRESULT rc;
-    bool    fRetakeLock = false;
-    {
-        AutoInitSpan autoInitSpan(this);
-        AssertReturn(autoInitSpan.isOk(), E_FAIL);
-
-        unconst(m->pVirtualBox) = aVirtualBox;
-        ComObjPtr<Medium> pActualThis = this;
-
-        // Do not inline this method call, as the purpose of having this separate
-        // is to save on stack size. Less local variables are the key for reaching
-        // deep recursion levels with small stack (XPCOM/g++ without optimization).
-        rc = initOne(aParent, aDeviceType, uuidMachineRegistry, data, strMachineFolder);
-        if (SUCCEEDED(rc))
-        {
-            /* In order to avoid duplicate instances of the medium object, we need
-               to register the parent before loading any children.  */
-            rc = m->pVirtualBox->i_registerMedium(pActualThis, &pActualThis, mediaTreeLock, true /*fCalledFromMediumInit*/);
-            if (SUCCEEDED(rc))
-            {
-                /* Don't call Medium::i_queryInfo for registered media to prevent the calling
-                 * thread (i.e. the VirtualBox server startup thread) from an unexpected
-                 * freeze but mark it as initially inaccessible instead. The vital UUID,
-                 * location and format properties are read from the registry file above; to
-                 * get the actual state and the rest of the data, the user will have to call
-                 * COMGETTER(State). */
-
-                /* load all children */
-                for (settings::MediaList::const_iterator it = data.llChildren.begin();
-                     it != data.llChildren.end();
-                     ++it)
-                {
-                    const settings::Medium &med = *it;
-
-                    ComObjPtr<Medium> pMedium;
-                    rc = pMedium.createObject();
-                    if (FAILED(rc)) break;
-
-                    rc = pMedium->initFromSettings(aVirtualBox,
-                                                   pActualThis,       // parent
-                                                   aDeviceType,
-                                                   uuidMachineRegistry,
-                                                   med,               // child data
-                                                   strMachineFolder,
-                                                   mediaTreeLock,
-                                                   NULL /*ppRegistered - must _not_ be &pMedium*/);
-                    if (FAILED(rc)) break;
-                }
-            }
-        }
-
-        /* Confirm a successful initialization when it's the case.  Do not confirm duplicates. */
-        if (SUCCEEDED(rc) && (Medium *)pActualThis == this)
-            autoInitSpan.setSucceeded();
-        /* Otherwise we have release the media tree lock so that Medium::uninit()
-           doesn't freak out when it is called by AutoInitSpan::~AutoInitSpan().
-           In the case of duplicates, the uninit will i_deparent this object. */
-        else
-        {
-            /** @todo Non-duplicate: Deregister? What about child load errors, should they
-             *        affect the parents? */
-            mediaTreeLock.release();
-            fRetakeLock = true;
-        }
-
-        /* Must be done after the mediaTreeLock.release above. */
-        if (ppRegistered)
-            *ppRegistered = pActualThis;
-    }
-
-    if (fRetakeLock)
+    HRESULT rc = S_OK;
+
+    MediaList llMediaTocleanup;
+
+    std::list<const settings::Medium *> llSettingsTodo;
+    llSettingsTodo.push_back(&data);
+    MediaList llParentsTodo;
+    llParentsTodo.push_back(NULL);
+
+    while (llSettingsTodo.size() > 0)
+    {
+        const settings::Medium *current = llSettingsTodo.front();
+        llSettingsTodo.pop_front();
+        ComObjPtr<Medium> pParent = llParentsTodo.front();
+        llParentsTodo.pop_front();
+
+        bool fReleasedMediaTreeLock = false;
+        ComObjPtr<Medium> pMedium;
+        rc = pMedium.createObject();
+        if (FAILED(rc))
+            break;
+        ComObjPtr<Medium> pActualMedium(pMedium);
+
+        {
+            AutoInitSpan autoInitSpan(pMedium);
+            AssertBreakStmt(autoInitSpan.isOk(), rc = E_FAIL);
+
+            unconst(pMedium->m->pVirtualBox) = aVirtualBox;
+            rc = pMedium->initOne(pParent, aDeviceType, uuidMachineRegistry, strMachineFolder, *current);
+            if (FAILED(rc))
+                break;
+            rc = aVirtualBox->i_registerMedium(pActualMedium, &pActualMedium, mediaTreeLock, true /*fCalledFromMediumInit*/);
+            if (FAILED(rc))
+                break;
+
+            if (pActualMedium == pMedium)
+            {
+                /* It is a truly new medium, remember details for cleanup. */
+                autoInitSpan.setSucceeded();
+                llMediaTocleanup.push_front(pMedium);
+            }
+            else
+            {
+                /* Since the newly created medium was replaced by an already
+                 * known one when merging medium trees, we can immediately mark
+                 * it as failed. */
+                autoInitSpan.setFailed();
+                mediaTreeLock.release();
+                fReleasedMediaTreeLock = true;
+            }
+        }
+        if (fReleasedMediaTreeLock)
+        {
+            /* With the InitSpan out of the way it's safe to let the refcount
+             * drop to 0 without causing uninit trouble. */
+            pMedium.setNull();
+            mediaTreeLock.acquire();
+        }
+
+        /* create all children */
+        std::list<settings::Medium>::const_iterator itBegin = current->llChildren.begin();
+        std::list<settings::Medium>::const_iterator itEnd = current->llChildren.end();
+        for (std::list<settings::Medium>::const_iterator it = itBegin; it != itEnd; ++it)
+        {
+            llSettingsTodo.push_back(&*it);
+            llParentsTodo.push_back(pActualMedium);
+        }
+    }
+
+    if (SUCCEEDED(rc))
+    {
+        /* Check for consistency. */
+        Assert(llSettingsTodo.size() == 0);
+        Assert(llParentsTodo.size() == 0);
+        /* Create the list of notifications, parent first. */
+        MediaList::const_reverse_iterator itBegin = llMediaTocleanup.rbegin();
+        MediaList::const_reverse_iterator itEnd = llMediaTocleanup.rend();
+        for (MediaList::const_reverse_iterator it = itBegin; it != itEnd; --it)
+        {
+            ComObjPtr<Medium> pMedium = *it;
+            AutoCaller mediumCaller(pMedium);
+            if (FAILED(mediumCaller.rc())) continue;
+            const Guid &id = pMedium->i_getId();
+            uIdsForNotify.push_back(std::pair<Guid, DeviceType_T>(id, aDeviceType));
+        }
+    }
+    else
+    {
+        /* Forget state of the settings processing. */
+        llSettingsTodo.clear();
+        llParentsTodo.clear();
+        /* Unregister all accumulated medium objects in the right order (last
+         * created to first created, avoiding config leftovers). */
+        MediaList::const_iterator itBegin = llMediaTocleanup.begin();
+        MediaList::const_iterator itEnd = llMediaTocleanup.end();
+        for (MediaList::const_iterator it = itBegin; it != itEnd; ++it)
+        {
+            ComObjPtr<Medium> pMedium = *it;
+            pMedium->i_unregisterWithVirtualBox();
+        }
+        /* Forget the only references to all newly created medium objects,
+         * triggering freeing (uninit happened in unregistering above). */
+        mediaTreeLock.release();
+        llMediaTocleanup.clear();
         mediaTreeLock.acquire();
+    }
 
     return rc;
@@ -1533 +1556 @@
  * Called either from FinalRelease() or by the parent when it gets destroyed.
  *
- * @note All children of this medium get uninitialized by calling their
- *       uninit() methods.
+ * @note All children of this medium get uninitialized, too, in a stack
+ *       friendly manner.
  */
 void Medium::uninit()
@@ -1552 +1575 @@
         return;
 
-    /* Caller must not hold the object or media tree lock over uninit(). */
-    Assert(!isWriteLockOnCurrentThread());
+    /* Caller must not hold the object (checked below) or media tree lock. */
     Assert(!pVirtualBox->i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
 
     AutoWriteLock treeLock(pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+
+    MediaList llMediaTodo;
+    llMediaTodo.push_back(this);
+
+    while (!llMediaTodo.empty())
+    {
+        /* This also guarantees that the refcount doesn't actually drop to 0
+         * again while the uninit is already ongoing. */
+        ComObjPtr<Medium> pMedium = llMediaTodo.front();
+        llMediaTodo.pop_front();
+
+        /* Enclose the state transition Ready->InUninit->NotReady */
+        AutoUninitSpan autoUninitSpan(pMedium);
+        if (autoUninitSpan.uninitDone())
+            continue;
+
+        Assert(!pMedium->isWriteLockOnCurrentThread());
 #ifdef DEBUG
-    if (!m->backRefs.empty())
-        i_dumpBackRefs();
+        if (!pMedium->m->backRefs.empty())
+            pMedium->i_dumpBackRefs();
 #endif
-    Assert(m->backRefs.empty());
-
-    /* Enclose the state transition Ready->InUninit->NotReady */
-    AutoUninitSpan autoUninitSpan(this);
-    if (autoUninitSpan.uninitDone())
-        return;
-
-    if (!m->formatObj.isNull())
-        m->formatObj.setNull();
-
-    if (m->state == MediumState_Deleting)
-    {
-        /* This medium has been already deleted (directly or as part of a
-         * merge).  Reparenting has already been done. */
-        Assert(m->pParent.isNull());
-    }
-    else
-    {
-        MediaList llChildren(m->llChildren);
-        m->llChildren.clear();
+        Assert(pMedium->m->backRefs.empty());
+
+        pMedium->m->formatObj.setNull();
+
+        if (m->state == MediumState_Deleting)
+        {
+            /* This medium has been already deleted (directly or as part of a
+             * merge).  Reparenting has already been done. */
+            Assert(m->pParent.isNull());
+            Assert(m->llChildren.empty());
+            continue;
+        }
+
+        //Assert(!pMedium->m->pParent);
+        /** @todo r=klaus Should not be necessary, since the caller should be
+         * doing the deparenting. No time right now to test everything. */
+        if (pMedium == this && pMedium->m->pParent)
+            pMedium->i_deparent();
+
+        /* Process all children */
+        MediaList::const_iterator itBegin = pMedium->m->llChildren.begin();
+        MediaList::const_iterator itEnd = pMedium->m->llChildren.end();
+        for (MediaList::const_iterator it = itBegin; it != itEnd; ++it)
+        {
+            Medium *pChild = *it;
+            pChild->m->pParent.setNull();
+            llMediaTodo.push_back(pChild);
+        }
+
+        /* Children information obsolete, will be processed anyway. */
+        pMedium->m->llChildren.clear();
+
+        unconst(pMedium->m->pVirtualBox) = NULL;
+
         autoUninitSpan.setSucceeded();
-
-        while (!llChildren.empty())
-        {
-            ComObjPtr<Medium> pChild = llChildren.front();
-            llChildren.pop_front();
-            pChild->m->pParent.setNull();
-            treeLock.release();
-            pChild->uninit();
-            treeLock.acquire();
-        }
-
-        if (m->pParent)
-        {
-            // this is a differencing disk: then remove it from the parent's children list
-            i_deparent();
-        }
-    }
-
-    unconst(m->pVirtualBox) = NULL;
+    }
 }
 
@@ -3622 +3657 @@
         ComObjPtr<VirtualBox> pVirtualBox(m->pVirtualBox);
 
-        /* canClose() needs the tree lock */
+        /* i_canClose() needs the tree lock */
         AutoMultiWriteLock2 multilock(!pVirtualBox.isNull() ? &pVirtualBox->i_getMediaTreeLockHandle() : NULL,
                                       this->lockHandle()
@@ -4223 +4258 @@
  * medium should be registered. The UUID can either be a machine UUID,
  * to add a machine registry, or the global registry UUID as returned by
- * VirtualBox::getGlobalRegistryId(). This recurses over all children.
+ * VirtualBox::getGlobalRegistryId(). Thisis applied to all children.
  *
  * Note that for hard disks, this method does nothing if the medium is
@@ -4235 +4270 @@
  * @return true if the registry was added; false if the given id was already on the list.
  */
-bool Medium::i_addRegistryRecursive(const Guid &id)
-{
-    AutoCaller autoCaller(this);
-    if (FAILED(autoCaller.rc()))
-        return false;
-
-    bool fAdd = i_addRegistryNoCallerCheck(id);
-
-    // protected by the medium tree lock held by our original caller
-    for (MediaList::const_iterator it = i_getChildren().begin();
-         it != i_getChildren().end();
-         ++it)
-    {
-        Medium *pChild = *it;
-        fAdd |= pChild->i_addRegistryRecursive(id);
+bool Medium::i_addRegistryAll(const Guid &id)
+{
+    MediaList llMediaTodo;
+    llMediaTodo.push_back(this);
+
+    bool fAdd = false;
+
+    while (llMediaTodo.size() > 0)
+    {
+        ComObjPtr<Medium> pMedium = llMediaTodo.front();
+        llMediaTodo.pop_front();
+
+        AutoCaller mediumCaller(pMedium);
+        if (FAILED(mediumCaller.rc())) return mediumCaller.rc();
+
+        fAdd |= pMedium->i_addRegistryNoCallerCheck(id);
+
+        // protected by the medium tree lock held by our original caller
+        MediaList::const_iterator itBegin = pMedium->i_getChildren().begin();
+        MediaList::const_iterator itEnd = pMedium->i_getChildren().end();
+        for (MediaList::const_iterator it = itBegin; it != itEnd; ++it)
+            llMediaTodo.push_back(*it);
     }
 
@@ -4289 +4331 @@
 /**
  * Removes the given UUID from the list of media registry UUIDs, for this
- * medium and all its children recursively.
+ * medium and all its children.
  *
  * @note the caller must hold the media tree lock for reading.
@@ -4296 +4338 @@
  * @return true if the UUID was found or false if not.
  */
-bool Medium::i_removeRegistryRecursive(const Guid &id)
-{
-    AutoCaller autoCaller(this);
-    if (FAILED(autoCaller.rc()))
-        return false;
-
-    bool fRemove = i_removeRegistry(id);
-
-    // protected by the medium tree lock held by our original caller
-    for (MediaList::const_iterator it = i_getChildren().begin();
-         it != i_getChildren().end();
-         ++it)
-    {
-        Medium *pChild = *it;
-        fRemove |= pChild->i_removeRegistryRecursive(id);
+bool Medium::i_removeRegistryAll(const Guid &id)
+{
+    MediaList llMediaTodo;
+    llMediaTodo.push_back(this);
+
+    bool fRemove = false;
+
+    while (llMediaTodo.size() > 0)
+    {
+        ComObjPtr<Medium> pMedium = llMediaTodo.front();
+        llMediaTodo.pop_front();
+
+        AutoCaller mediumCaller(pMedium);
+        if (FAILED(mediumCaller.rc())) return mediumCaller.rc();
+
+        fRemove |= pMedium->i_removeRegistry(id);
+
+        // protected by the medium tree lock held by our original caller
+        MediaList::const_iterator itBegin = pMedium->i_getChildren().begin();
+        MediaList::const_iterator itEnd = pMedium->i_getChildren().end();
+        for (MediaList::const_iterator it = itBegin; it != itEnd; ++it)
+            llMediaTodo.push_back(*it);
     }
 
@@ -4597 +4646 @@
  *
  * Must have caller + locking, *and* caller must hold the media tree lock!
+ * @param aId   Id to ignore when looking for backrefs.
  * @return
  */
-const Guid* Medium::i_getAnyMachineBackref() const
-{
-    if (m->backRefs.size())
-        return &m->backRefs.front().machineId;
-
-    for (MediaList::const_iterator it = i_getChildren().begin();
-         it != i_getChildren().end();
-         ++it)
-    {
-        Medium *pChild = *it;
-        // recurse for this child
-        const Guid* puuid;
-        if ((puuid = pChild->i_getAnyMachineBackref()))
-            return puuid;
+const Guid* Medium::i_getAnyMachineBackref(const Guid &aId) const
+{
+    std::list<const Medium *> llMediaTodo;
+    llMediaTodo.push_back(this);
+
+    while (llMediaTodo.size() > 0)
+    {
+        const Medium *pMedium = llMediaTodo.front();
+        llMediaTodo.pop_front();
+
+        if (pMedium->m->backRefs.size())
+        {
+            if (pMedium->m->backRefs.front().machineId != aId)
+                return &pMedium->m->backRefs.front().machineId;
+            if (pMedium->m->backRefs.size() > 1)
+            {
+                BackRefList::const_iterator it = pMedium->m->backRefs.begin();
+                ++it;
+                return &it->machineId;
+            }
+        }
+
+        MediaList::const_iterator itBegin = pMedium->i_getChildren().begin();
+        MediaList::const_iterator itEnd = pMedium->i_getChildren().end();
+        for (MediaList::const_iterator it = itBegin; it != itEnd; ++it)
+            llMediaTodo.push_back(*it);
     }
 
@@ -4936 +4998 @@
 /**
  * Saves medium data by putting it into the provided data structure.
- * Recurses over all children to save their settings, too.
+ * The settings of all children is saved, too.
  *
  * @param data      Settings struct to be updated.
@@ -4952 +5014 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    i_saveSettingsOne(data, strHardDiskFolder);
-
-    /* save all children */
-    settings::MediaList &llSettingsChildren = data.llChildren;
-    for (MediaList::const_iterator it = i_getChildren().begin();
-         it != i_getChildren().end();
-         ++it)
-    {
-        // Use the element straight in the list to reduce both unnecessary
-        // deep copying (when unwinding the recursion the entire medium
-        // settings sub-tree is copied) and the stack footprint (the settings
-        // need almost 1K, and there can be VMs with long image chains.
-        llSettingsChildren.push_back(settings::Medium::Empty);
-        HRESULT rc = (*it)->i_saveSettings(llSettingsChildren.back(), strHardDiskFolder);
-        if (FAILED(rc))
-        {
-            llSettingsChildren.pop_back();
-            return rc;
+    MediaList llMediaTodo;
+    llMediaTodo.push_back(this);
+    std::list<settings::Medium *> llSettingsTodo;
+    llSettingsTodo.push_back(&data);
+
+    while (llMediaTodo.size() > 0)
+    {
+        ComObjPtr<Medium> pMedium = llMediaTodo.front();
+        llMediaTodo.pop_front();
+        settings::Medium *current = llSettingsTodo.front();
+        llSettingsTodo.pop_front();
+
+        AutoCaller mediumCaller(pMedium);
+        if (FAILED(mediumCaller.rc())) return mediumCaller.rc();
+
+        pMedium->i_saveSettingsOne(*current, strHardDiskFolder);
+
+        /* save all children */
+        MediaList::const_iterator itBegin = pMedium->i_getChildren().begin();
+        MediaList::const_iterator itEnd = pMedium->i_getChildren().end();
+        for (MediaList::const_iterator it = itBegin; it != itEnd; ++it)
+        {
+            llMediaTodo.push_back(*it);
+            current->llChildren.push_back(settings::Medium::Empty);
+            llSettingsTodo.push_back(&current->llChildren.back());
         }
     }
@@ -5402 +5471 @@
     try
     {
-        /* we're accessing the media tree, and canClose() needs it too */
+        /* we're accessing the media tree, and i_canClose() needs it too */
         AutoWriteLock treelock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 

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

@@ -157 +157 @@
  *  Since this manipulates the snapshots tree, the caller must hold the
  *  machine lock in write mode (which protects the snapshots tree)!
+ *
+ * @note All children of this snapshot get uninitialized, too, in a stack
+ *       friendly manner.
  */
 void Snapshot::uninit()
@@ -162 +165 @@
     LogFlowThisFunc(("\n"));
 
-    /* Enclose the state transition Ready->InUninit->NotReady */
-    AutoUninitSpan autoUninitSpan(this);
-    if (autoUninitSpan.uninitDone())
-        return;
-
-    Assert(m->pMachine->isWriteLockOnCurrentThread());
-
-    // uninit all children
-    SnapshotsList::iterator it;
-    for (it = m->llChildren.begin();
-         it != m->llChildren.end();
-         ++it)
-    {
-        Snapshot *pChild = *it;
-        pChild->m->pParent.setNull();
-        pChild->uninit();
-    }
-    m->llChildren.clear();          // this unsets all the ComPtrs and probably calls delete
-
-    // since there is no guarantee anyone holds a reference to us except the
-    // list of children in our parent, make sure that the reference count
-    // will not drop to 0 before we've declared ourselves as uninitialized,
-    // otherwise there will be another uninit call which causes a self-deadlock
-    // because this uninit isn't complete yet.
-    ComObjPtr<Snapshot> pSnapshot(this);
-    if (m->pParent)
-        i_deparent();
-
-    if (m->pMachine)
-    {
-        m->pMachine->uninit();
-        m->pMachine.setNull();
-    }
-
-    delete m;
-    m = NULL;
-
-    autoUninitSpan.setSucceeded();
-    // see above, now the refcount may reach 0
-    pSnapshot.setNull();
+    {
+        /* If "this" is already uninitialized or was never initialized, skip
+         * all activity since it makes no sense. Also would cause asserts with
+         * the automatic refcount updating with SnapshotList/ComPtr. Also,
+         * make sure that the possible fake error is undone. */
+        ErrorInfoKeeper eik;
+        AutoLimitedCaller autoCaller(this);
+        if (FAILED(autoCaller.rc()))
+            return;
+    }
+
+    SnapshotsList llSnapshotsTodo;
+    llSnapshotsTodo.push_back(this);
+    SnapshotsList llSnapshotsAll;
+
+    while (llSnapshotsTodo.size() > 0)
+    {
+        /* This also guarantees that the refcount doesn't actually drop to 0
+         * again while the uninit is already ongoing. */
+        ComObjPtr<Snapshot> pSnapshot = llSnapshotsTodo.front();
+        llSnapshotsTodo.pop_front();
+
+        /* Enclose the state transition Ready->InUninit->NotReady */
+        AutoUninitSpan autoUninitSpan(pSnapshot);
+        if (autoUninitSpan.uninitDone())
+            continue;
+
+        /* Remember snapshots (depth first), for associated SnapshotMachine
+         * uninitialization, which must be done in dept first order, otherwise
+         * the Medium object uninit is done in the wrong order. */
+        llSnapshotsAll.push_front(pSnapshot);
+
+        Assert(pSnapshot->m->pMachine->isWriteLockOnCurrentThread());
+
+        /* Paranoia. Shouldn't be set any more at processing time. */
+        pSnapshot->m->pParent.setNull();
+
+        /* Process all children */
+        SnapshotsList::const_iterator itBegin = pSnapshot->m->llChildren.begin();
+        SnapshotsList::const_iterator itEnd = pSnapshot->m->llChildren.end();
+        for (SnapshotsList::const_iterator it = itBegin; it != itEnd; ++it)
+        {
+            Snapshot *pChild = *it;
+            pChild->m->pParent.setNull();
+            llSnapshotsTodo.push_back(pChild);
+        }
+
+        /* Children information obsolete, will be processed anyway. */
+        pSnapshot->m->llChildren.clear();
+
+        autoUninitSpan.setSucceeded();
+    }
+
+    /* Now handle SnapshotMachine uninit and free memory. */
+    while (llSnapshotsAll.size() > 0)
+    {
+        ComObjPtr<Snapshot> pSnapshot = llSnapshotsAll.front();
+        llSnapshotsAll.pop_front();
+
+        if (pSnapshot->m->pMachine)
+        {
+            pSnapshot->m->pMachine->uninit();
+            pSnapshot->m->pMachine.setNull();
+        }
+
+        delete pSnapshot->m;
+        pSnapshot->m = NULL;
+    }
 }
 
@@ -278 +309 @@
 /**
  * Internal helper that removes "this" from the list of children of its
- * parent. Used in uninit() and other places when reparenting is necessary.
+ * parent. Used in places when reparenting is necessary.
  *
  * The caller must hold the machine lock in write mode (which protects the snapshots tree)!
@@ -488 +519 @@
 /**
  * Returns the number of direct child snapshots, without grandchildren.
- * Does not recurse.
  * @return
  */
@@ -503 +533 @@
 
 /**
- * Implementation method for getAllChildrenCount() so we request the
- * tree lock only once before recursing. Don't call directly.
+ * Returns the number of child snapshots including all grandchildren.
  * @return
  */
-ULONG Snapshot::i_getAllChildrenCountImpl()
+ULONG Snapshot::i_getAllChildrenCount()
 {
     AutoCaller autoCaller(this);
     AssertComRC(autoCaller.rc());
 
-    ULONG count = (ULONG)m->llChildren.size();
-    for (SnapshotsList::const_iterator it = m->llChildren.begin();
-         it != m->llChildren.end();
-         ++it)
-    {
-        count += (*it)->i_getAllChildrenCountImpl();
-    }
-
-    return count;
-}
-
-/**
- * Returns the number of child snapshots including all grandchildren.
- * Recurses into the snapshots tree.
- * @return
- */
-ULONG Snapshot::i_getAllChildrenCount()
-{
-    AutoCaller autoCaller(this);
-    AssertComRC(autoCaller.rc());
-
     // snapshots tree is protected by machine lock
     AutoReadLock alock(m->pMachine COMMA_LOCKVAL_SRC_POS);
 
-    return i_getAllChildrenCountImpl();
+    std::list<const Snapshot *> llSnapshotsTodo;
+    llSnapshotsTodo.push_back(this);
+
+    ULONG cChildren = 0;
+
+    while (llSnapshotsTodo.size() > 0)
+    {
+        const Snapshot *pSnapshot = llSnapshotsTodo.front();
+        llSnapshotsTodo.pop_front();
+
+        cChildren += (ULONG)pSnapshot->m->llChildren.size();
+
+        /* count all children */
+        SnapshotsList::const_iterator itBegin = pSnapshot->m->llChildren.begin();
+        SnapshotsList::const_iterator itEnd = pSnapshot->m->llChildren.end();
+        for (SnapshotsList::const_iterator it = itBegin; it != itEnd; ++it)
+            llSnapshotsTodo.push_back(*it);
+    }
+
+    return cChildren;
 }
 
@@ -720 +747 @@
     const Utf8Str &path = m->pMachine->mSSData->strStateFilePath;
 
-    if (!pSnapshotToIgnore || pSnapshotToIgnore != this)
-        if (path.isNotEmpty())
+    if (path.isEmpty())
+        return false;
+
+    std::list<const Snapshot *> llSnapshotsTodo;
+    llSnapshotsTodo.push_back(this);
+
+    while (llSnapshotsTodo.size() > 0)
+    {
+        const Snapshot *pSnapshot = llSnapshotsTodo.front();
+        llSnapshotsTodo.pop_front();
+
+        if (!pSnapshotToIgnore || pSnapshotToIgnore != this)
             if (path == strPath)
-                return true;        // no need to recurse then
-
-    // but otherwise we must check children
-    for (SnapshotsList::const_iterator it = m->llChildren.begin();
-         it != m->llChildren.end();
-         ++it)
-    {
-        Snapshot *pChild = *it;
-        if (!pSnapshotToIgnore || pSnapshotToIgnore != pChild)
-            if (pChild->i_sharesSavedStateFile(strPath, pSnapshotToIgnore))
                 return true;
+
+        /* check all children */
+        SnapshotsList::const_iterator itBegin = pSnapshot->m->llChildren.begin();
+        SnapshotsList::const_iterator itEnd = pSnapshot->m->llChildren.end();
+        for (SnapshotsList::const_iterator it = itBegin; it != itEnd; ++it)
+            llSnapshotsTodo.push_back(*it);
     }
 
@@ -805 +838 @@
  * @return
  */
-HRESULT Snapshot::i_saveSnapshotImplOne(settings::Snapshot &data) const
+HRESULT Snapshot::i_saveSnapshotOne(settings::Snapshot &data) const
 {
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
@@ -827 +860 @@
 
 /**
- * Internal implementation for Snapshot::saveSnapshot (below). Caller has
- * requested the snapshots tree (machine) lock.
- *
- * @param data      Target for saving snapshot settings.
- * @return
- */
-HRESULT Snapshot::i_saveSnapshotImpl(settings::Snapshot &data) const
-{
-    HRESULT rc = i_saveSnapshotImplOne(data);
-    if (FAILED(rc))
-        return rc;
-
-    settings::SnapshotsList &llSettingsChildren = data.llChildSnapshots;
-    for (SnapshotsList::const_iterator it = m->llChildren.begin();
-         it != m->llChildren.end();
-         ++it)
-    {
-        // Use the heap (indirectly through the list container) to reduce the
-        // stack footprint, avoiding local settings objects on the stack which
-        // need a lot of stack space. There can be VMs with deeply nested
-        // snapshots. The stack can be quite small, especially with XPCOM.
-        llSettingsChildren.push_back(settings::Snapshot::Empty);
-        Snapshot *pSnap = *it;
-        rc = pSnap->i_saveSnapshotImpl(llSettingsChildren.back());
-        if (FAILED(rc))
-        {
-            llSettingsChildren.pop_back();
-            return rc;
-        }
-    }
-
-    return S_OK;
-}
-
-/**
  * Saves the given snapshot and all its children.
  * It is assumed that the given node is empty.
@@ -872 +870 @@
     AutoReadLock alock(m->pMachine COMMA_LOCKVAL_SRC_POS);
 
-    return i_saveSnapshotImpl(data);
+    std::list<const Snapshot *> llSnapshotsTodo;
+    llSnapshotsTodo.push_back(this);
+    std::list<settings::Snapshot *> llSettingsTodo;
+    llSettingsTodo.push_back(&data);
+
+    while (llSnapshotsTodo.size() > 0)
+    {
+        const Snapshot *pSnapshot = llSnapshotsTodo.front();
+        llSnapshotsTodo.pop_front();
+        settings::Snapshot *current = llSettingsTodo.front();
+        llSettingsTodo.pop_front();
+
+        HRESULT rc = pSnapshot->i_saveSnapshotOne(*current);
+        if (FAILED(rc))
+            return rc;
+
+        /* save all children */
+        SnapshotsList::const_iterator itBegin = pSnapshot->m->llChildren.begin();
+        SnapshotsList::const_iterator itEnd = pSnapshot->m->llChildren.end();
+        for (SnapshotsList::const_iterator it = itBegin; it != itEnd; ++it)
+        {
+            llSnapshotsTodo.push_back(*it);
+            current->llChildSnapshots.push_back(settings::Snapshot::Empty);
+            llSettingsTodo.push_back(&current->llChildSnapshots.back());
+        }
+    }
+
+    return S_OK;
 }
 
@@ -879 +904 @@
  *
  * This removes all medium attachments from the snapshot's machine and returns
- * the snapshot's saved state file name, if any, and then calls uninit() on
- * "this" itself.
+ * the snapshot's saved state file name, if any, and then calls uninit().
+ *
+ * This processes children depth first, so the given MediaList receives child
+ * media first before their parents. If the caller wants to close all media,
+ * they should go thru the list from the beginning to the end because media
+ * cannot be closed if they have children.
+ *
+ * This calls uninit() on itself, so the snapshots tree (beginning with a machine's pFirstSnapshot) becomes invalid after this.
+ * It does not alter the main machine's snapshot pointers (pFirstSnapshot, pCurrentSnapshot).
  *
  * Caller must hold the machine write lock (which protects the snapshots tree!)
@@ -890 +922 @@
  * @return
  */
-HRESULT Snapshot::i_uninitOne(AutoWriteLock &writeLock,
+HRESULT Snapshot::i_uninitAll(AutoWriteLock &writeLock,
                               CleanupMode_T cleanupMode,
                               MediaList &llMedia,
                               std::list<Utf8Str> &llFilenames)
 {
-    // now call detachAllMedia on the snapshot machine
-    HRESULT rc = m->pMachine->i_detachAllMedia(writeLock,
-                                               this /* pSnapshot */,
-                                               cleanupMode,
-                                               llMedia);
-    if (FAILED(rc))
-        return rc;
-
-    // report the saved state file if it's not on the list yet
-    if (m->pMachine->mSSData->strStateFilePath.isNotEmpty())
-    {
-        bool fFound = false;
-        for (std::list<Utf8Str>::const_iterator it = llFilenames.begin();
-             it != llFilenames.end();
-             ++it)
-        {
-            const Utf8Str &str = *it;
-            if (str == m->pMachine->mSSData->strStateFilePath)
+    Assert(m->pMachine->isWriteLockOnCurrentThread());
+
+    HRESULT rc = S_OK;
+
+    SnapshotsList llSnapshotsTodo;
+    llSnapshotsTodo.push_front(this);
+    SnapshotsList llSnapshotsAll;
+
+    /* Enumerate all snapshots depth first, avoids trouble with updates. */
+    while (llSnapshotsTodo.size() > 0)
+    {
+        ComObjPtr<Snapshot> pSnapshot = llSnapshotsTodo.front();
+        llSnapshotsTodo.pop_front();
+
+        llSnapshotsAll.push_front(pSnapshot);
+
+        /* Process all children */
+        SnapshotsList::const_iterator itBegin = pSnapshot->m->llChildren.begin();
+        SnapshotsList::const_iterator itEnd = pSnapshot->m->llChildren.end();
+        for (SnapshotsList::const_iterator it = itBegin; it != itEnd; ++it)
+        {
+            Snapshot *pChild = *it;
+            pChild->m->pParent.setNull();
+            llSnapshotsTodo.push_front(pChild);
+        }
+    }
+
+    /* Process all snapshots in enumeration order. */
+    while (llSnapshotsAll.size() > 0)
+    {
+        /* This also guarantees that the refcount doesn't actually drop to 0
+         * again while the uninit is already ongoing. */
+        ComObjPtr<Snapshot> pSnapshot = llSnapshotsAll.front();
+        llSnapshotsAll.pop_front();
+
+        rc = pSnapshot->m->pMachine->i_detachAllMedia(writeLock,
+                                                      pSnapshot,
+                                                      cleanupMode,
+                                                      llMedia);
+        if (SUCCEEDED(rc))
+        {
+            Utf8Str strFile;
+
+            // report the saved state file if it's not on the list yet
+            strFile = pSnapshot->m->pMachine->mSSData->strStateFilePath;
+            if (strFile.isNotEmpty())
             {
-                fFound = true;
-                break;
+                std::list<Utf8Str>::const_iterator itFound = find(llFilenames.begin(), llFilenames.end(), strFile);
+
+                if (itFound != llFilenames.end())
+                    llFilenames.push_back(strFile);
             }
-        }
-        if (!fFound)
-            llFilenames.push_back(m->pMachine->mSSData->strStateFilePath);
-    }
-
-    Utf8Str strNVRAMFile = m->pMachine->mNvramStore->i_getNonVolatileStorageFile();
-    if (strNVRAMFile.isNotEmpty() && RTFileExists(strNVRAMFile.c_str()))
-        llFilenames.push_back(strNVRAMFile);
-
-    i_beginSnapshotDelete();
-    uninit();
+
+            strFile = pSnapshot->m->pMachine->mNvramStore->i_getNonVolatileStorageFile();
+            if (strFile.isNotEmpty() && RTFileExists(strFile.c_str()))
+                llFilenames.push_back(strFile);
+        }
+
+        pSnapshot->m->pParent.setNull();
+        pSnapshot->m->llChildren.clear();
+        pSnapshot->uninit();
+    }
 
     return S_OK;
-}
-
-/**
- * Part of the cleanup engine of Machine::Unregister().
- *
- * This recursively removes all medium attachments from the snapshot's machine
- * and returns the snapshot's saved state file name, if any, and then calls
- * uninit() on "this" itself.
- *
- * This recurses into children first, so the given MediaList receives child
- * media first before their parents. If the caller wants to close all media,
- * they should go thru the list from the beginning to the end because media
- * cannot be closed if they have children.
- *
- * This calls uninit() on itself, so the snapshots tree (beginning with a machine's pFirstSnapshot) becomes invalid after this.
- * It does not alter the main machine's snapshot pointers (pFirstSnapshot, pCurrentSnapshot).
- *
- * Caller must hold the machine write lock (which protects the snapshots tree!)
- *
- * @param writeLock Machine write lock, which can get released temporarily here.
- * @param cleanupMode Cleanup mode; see Machine::detachAllMedia().
- * @param llMedia List of media returned to caller, depending on cleanupMode.
- * @param llFilenames
- * @return
- */
-HRESULT Snapshot::i_uninitRecursively(AutoWriteLock &writeLock,
-                                      CleanupMode_T cleanupMode,
-                                      MediaList &llMedia,
-                                      std::list<Utf8Str> &llFilenames)
-{
-    Assert(m->pMachine->isWriteLockOnCurrentThread());
-
-    HRESULT rc = S_OK;
-
-    // make a copy of the Guid for logging before we uninit ourselves
-#ifdef LOG_ENABLED
-    Guid uuid = i_getId();
-    Utf8Str name = i_getName();
-    LogFlowThisFunc(("Entering for snapshot '%s' {%RTuuid}\n", name.c_str(), uuid.raw()));
-#endif
-
-    // Recurse into children first so that the child media appear on the list
-    // first; this way caller can close the media from the beginning to the end
-    // because parent media can't be closed if they have children and
-    // additionally it postpones the uninit() call until we no longer need
-    // anything from the list. Oh, and remember that the child removes itself
-    // from the list, so keep the iterator at the beginning.
-    for (SnapshotsList::const_iterator it = m->llChildren.begin();
-         it != m->llChildren.end();
-         it = m->llChildren.begin())
-    {
-        Snapshot *pChild = *it;
-        rc = pChild->i_uninitRecursively(writeLock, cleanupMode, llMedia, llFilenames);
-        if (FAILED(rc))
-            break;
-    }
-
-    if (SUCCEEDED(rc))
-        rc = i_uninitOne(writeLock, cleanupMode, llMedia, llFilenames);
-
-#ifdef LOG_ENABLED
-    LogFlowThisFunc(("Leaving for snapshot '%s' {%RTuuid}: %Rhrc\n", name.c_str(), uuid.raw(), rc));
-#endif
-
-    return rc;
 }
 
@@ -1731 +1727 @@
     }
 
+    LogRel(("Taking snapshot %s\n", task.m_strName.c_str()));
+
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
@@ -1758 +1756 @@
         /* save settings to ensure current changes are committed and
          * hard disks are fixed up */
-        rc = i_saveSettings(NULL, alock);
+        rc = i_saveSettings(NULL, alock); /******************1 */
             // no need to check for whether VirtualBox.xml needs changing since
             // we can't have a machine XML rename pending at this point
@@ -1934 +1932 @@
          * machine state to the state it had at the beginning.
          */
-        rc = i_finishTakingSnapshot(task, alock, true /*aSuccess*/);
+        rc = i_finishTakingSnapshot(task, alock, true /*aSuccess*/); /*******************2+3 */
         // do not throw rc here because we can't call i_finishTakingSnapshot() twice
         LogFlowThisFunc(("i_finishTakingSnapshot -> %Rhrc [mMachineState=%s]\n", rc, ::stringifyMachineState(mData->mMachineState)));
@@ -1978 +1976 @@
         {
             if (!task.m_fTakingSnapshotOnline)
-                i_setMachineState(task.m_machineStateBackup);
+                i_setMachineState(task.m_machineStateBackup); /**************** 4 Machine::i_saveStateSettings*/
             else
             {
@@ -2048 +2046 @@
         }
     }
+    LogRel(("Finished taking snapshot %s\n", task.m_strName.c_str()));
     LogFlowThisFuncLeave();
 }
@@ -2112 +2111 @@
             flSaveSettings |= SaveS_ResetCurStateModified;
 
-        rc = i_saveSettings(NULL, alock, flSaveSettings);
+        rc = i_saveSettings(NULL, alock, flSaveSettings); /******************2 */
     }
 
@@ -2150 +2149 @@
 
     /* alock has been released already */
-    mParent->i_saveModifiedRegistries();
+    mParent->i_saveModifiedRegistries(); /**************3 */
 
     alock.acquire();

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

@@ -935 +935 @@
         const settings::Medium &xmlHD = *it;
 
-        ComObjPtr<Medium> pHardDisk;
-        rc = pHardDisk.createObject();
+        rc = Medium::initFromSettings(this,
+                                      DeviceType_HardDisk,
+                                      uuidRegistry,
+                                      strMachineFolder,
+                                      xmlHD,
+                                      treeLock,
+                                      uIdsForNotify);
         if (FAILED(rc)) return rc;
-        ComObjPtr<Medium> pHardDiskActual(pHardDisk);
-        rc = pHardDisk->initFromSettings(this,
-                                         NULL,          // parent
-                                         DeviceType_HardDisk,
-                                         uuidRegistry,
-                                         xmlHD,         // XML data; this recurses to processes the children
-                                         strMachineFolder,
-                                         treeLock,
-                                         &pHardDiskActual /*never &pHardDisk!*/);
-        if (SUCCEEDED(rc))
-        {
-            /** @todo r=bird: should we really do notifications for duplicates?
-             *  ((Medium *)pHardDisk != (Medium *)pHardDiskActual)
-             * The problem with that, though, is that for the children we don't quite know
-             * which are duplicates and which aren't.   The above initFromSettings is
-             * essentially poforming a merge operation now, so in the duplicate case, we may
-             * just have added a new (grand)child.  Why don't we just pass uIdsForNotify
-             * down to initFromSettings, that'll save us this extra walking? */
-
-            uIdsForNotify.push_back(std::pair<Guid, DeviceType_T>(pHardDiskActual->i_getId(), DeviceType_HardDisk));
-            // Add children IDs to notification using non-recursive children enumeration.
-            std::vector<std::pair<MediaList::const_iterator, ComObjPtr<Medium> > > llEnumStack;
-            const MediaList& mediaList = pHardDiskActual->i_getChildren();
-            llEnumStack.push_back(std::pair<MediaList::const_iterator, ComObjPtr<Medium> >(mediaList.begin(), pHardDiskActual));
-            while (!llEnumStack.empty())
-            {
-                if (llEnumStack.back().first == llEnumStack.back().second->i_getChildren().end())
-                {
-                    llEnumStack.pop_back();
-                    if (!llEnumStack.empty())
-                        ++llEnumStack.back().first;
-                    continue;
-                }
-                uIdsForNotify.push_back(std::pair<Guid, DeviceType_T>((*llEnumStack.back().first)->i_getId(), DeviceType_HardDisk));
-                const MediaList& childMediaList = (*llEnumStack.back().first)->i_getChildren();
-                if (!childMediaList.empty())
-                {
-                    llEnumStack.push_back(std::pair<MediaList::const_iterator, ComObjPtr<Medium> >(childMediaList.begin(),
-                                                                                             *llEnumStack.back().first));
-                    continue;
-                }
-                ++llEnumStack.back().first;
-            }
-        }
-        // Avoid trouble with lock/refcount, before returning or not.
-        treeLock.release();
-        pHardDisk.setNull();
-        pHardDiskActual.setNull();
-        if (FAILED(rc)) return rc;
-        treeLock.acquire();
     }
 
@@ -996 +951 @@
         const settings::Medium &xmlDvd = *it;
 
-        ComObjPtr<Medium> pImage;
-        rc = pImage.createObject();
-        if (FAILED(rc)) return rc;
-
-        ComObjPtr<Medium> pImageActually = pImage;
-        rc = pImage->initFromSettings(this,
-                                      NULL,
+        rc = Medium::initFromSettings(this,
                                       DeviceType_DVD,
                                       uuidRegistry,
+                                      strMachineFolder,
                                       xmlDvd,
-                                      strMachineFolder,
                                       treeLock,
-                                      &pImageActually);
-        if (SUCCEEDED(rc))
-            uIdsForNotify.push_back(std::pair<Guid, DeviceType_T>(pImageActually->i_getId(), DeviceType_DVD));
-
-        // Avoid trouble with lock/refcount, before returning or not.
-        treeLock.release();
-        pImage.setNull();
-        pImageActually.setNull();
+                                      uIdsForNotify);
         if (FAILED(rc)) return rc;
-        treeLock.acquire();
     }
 
@@ -1026 +967 @@
         const settings::Medium &xmlFloppy = *it;
 
-        ComObjPtr<Medium> pImage;
-        rc = pImage.createObject();
-        if (FAILED(rc)) return rc;
-
-        ComObjPtr<Medium> pImageActually = pImage;
-        rc = pImage->initFromSettings(this,
-                                      NULL,
+        rc = Medium::initFromSettings(this,
                                       DeviceType_Floppy,
                                       uuidRegistry,
+                                      strMachineFolder,
                                       xmlFloppy,
-                                      strMachineFolder,
                                       treeLock,
-                                      &pImageActually);
-        if (SUCCEEDED(rc))
-            uIdsForNotify.push_back(std::pair<Guid, DeviceType_T>(pImage->i_getId(), DeviceType_Floppy));
-
-        // Avoid trouble with lock/refcount, before returning or not.
-        treeLock.release();
-        pImage.setNull();
-        pImageActually.setNull();
+                                      uIdsForNotify);
         if (FAILED(rc)) return rc;
-        treeLock.acquire();
-    }
-
-    if (SUCCEEDED(rc))
-    {
-        for (std::list<std::pair<Guid, DeviceType_T> >::const_iterator itItem = uIdsForNotify.begin();
-             itItem != uIdsForNotify.end();
-             ++itItem)
-        {
-            i_onMediumRegistered(itItem->first, itItem->second, TRUE);
-        }
+    }
+
+    for (std::list<std::pair<Guid, DeviceType_T> >::const_iterator itItem = uIdsForNotify.begin();
+         itItem != uIdsForNotify.end();
+         ++itItem)
+    {
+        i_onMediumRegistered(itItem->first, itItem->second, TRUE);
     }
 
@@ -5238 +5162 @@
 
 /**
- * Little helper called from unregisterMachineMedia() to recursively add media to the given list,
- * with children appearing before their parents.
- * @param llMedia
- * @param pMedium
- */
-void VirtualBox::i_pushMediumToListWithChildren(MediaList &llMedia, Medium *pMedium)
-{
-    // recurse first, then add ourselves; this way children end up on the
-    // list before their parents
-
-    const MediaList &llChildren = pMedium->i_getChildren();
-    for (MediaList::const_iterator it = llChildren.begin();
-         it != llChildren.end();
-         ++it)
-    {
-        Medium *pChild = *it;
-        i_pushMediumToListWithChildren(llMedia, pChild);
-    }
-
-    Log(("Pushing medium %RTuuid\n", pMedium->i_getId().raw()));
-    llMedia.push_back(pMedium);
-}
-
-/**
  * Unregisters all Medium objects which belong to the given machine registry.
  * Gets called from Machine::uninit() just before the machine object dies
@@ -5293 +5193 @@
             if (FAILED(medCaller.rc())) return medCaller.rc();
             AutoReadLock medlock(pMedium COMMA_LOCKVAL_SRC_POS);
-
-            if (pMedium->i_isInRegistry(uuidMachine))
-                // recursively with children first
-                i_pushMediumToListWithChildren(llMedia2Close, pMedium);
+            LogRel(("Looking at medium %RTuuid\n", pMedium->i_getId().raw()));
+
+            /* If the medium is still in the registry then either some code is
+             * seriously buggy (unregistering a VM removes it automatically),
+             * or the reference to a Machine object is destroyed without ever
+             * being registered. The second condition checks if a medium is
+             * in no registry, which indicates (set by unregistering) that a
+             * medium is not used by any other VM and thus can be closed. */
+            Guid dummy;
+            if (   pMedium->i_isInRegistry(uuidMachine)
+                || !pMedium->i_getFirstRegistryMachineId(dummy))
+            {
+                /* Collect all medium objects into llMedia2Close,
+                 * in right order for closing. */
+                MediaList llMediaTodo;
+                llMediaTodo.push_back(pMedium);
+
+                while (llMediaTodo.size() > 0)
+                {
+                    ComObjPtr<Medium> pCurrent = llMediaTodo.front();
+                    llMediaTodo.pop_front();
+
+                    /* Add to front, order must be children then parent. */
+                    LogRel(("Pushing medium %RTuuid (front)\n", pCurrent->i_getId().raw()));
+                    llMedia2Close.push_front(pCurrent);
+
+                    /* process all children */
+                    MediaList::const_iterator itBegin = pCurrent->i_getChildren().begin();
+                    MediaList::const_iterator itEnd = pCurrent->i_getChildren().end();
+                    for (MediaList::const_iterator it2 = itBegin; it2 != itEnd; ++it2)
+                        llMediaTodo.push_back(*it2);
+                }
+            }
         }
     }
@@ -5305 +5234 @@
     {
         ComObjPtr<Medium> pMedium = *it;
-        Log(("Closing medium %RTuuid\n", pMedium->i_getId().raw()));
+        LogRel(("Closing medium %RTuuid\n", pMedium->i_getId().raw()));
         AutoCaller mac(pMedium);
         pMedium->i_close(mac);
@@ -5319 +5248 @@
  * Called from Machine::Unregister().
  * @param pMachine
+ * @param aCleanupMode  How to handle medium attachments. For
+ *      CleanupMode_UnregisterOnly the associated medium objects will be
+ *      closed when the Machine object is uninitialized, otherwise they will
+ *      go to the global registry if no better registry is found.
  * @param id  UUID of the machine. Must be passed by caller because machine may be dead by this time.
  * @return
  */
 HRESULT VirtualBox::i_unregisterMachine(Machine *pMachine,
+                                        CleanupMode_T aCleanupMode,
                                         const Guid &id)
 {
@@ -5355 +5289 @@
             AutoWriteLock mlock(pMedium COMMA_LOCKVAL_SRC_POS);
 
-            if (pMedium->i_removeRegistryRecursive(id))
+            if (pMedium->i_removeRegistryAll(id))
             {
                 // machine ID was found in base medium's registry list:
                 // move this base image and all its children to another registry then
                 // 1) first, find a better registry to add things to
-                const Guid *puuidBetter = pMedium->i_getAnyMachineBackref();
+                const Guid *puuidBetter = pMedium->i_getAnyMachineBackref(id);
                 if (puuidBetter)
                 {
                     // 2) better registry found: then use that
-                    pMedium->i_addRegistryRecursive(*puuidBetter);
+                    pMedium->i_addRegistryAll(*puuidBetter);
                     // 3) and make sure the registry is saved below
                     mlock.release();
                     tlock.release();
                     i_markRegistryModified(*puuidBetter);
+                    tlock.acquire();
+                    mlock.acquire();
+                }
+                else if (aCleanupMode != CleanupMode_UnregisterOnly)
+                {
+                    pMedium->i_addRegistryAll(i_getGlobalRegistryId());
+                    mlock.release();
+                    tlock.release();
+                    i_markRegistryModified(i_getGlobalRegistryId());
                     tlock.acquire();
                     mlock.acquire();

trunk/src/VBox/Main/xml/Settings.cpp

@@ -851 +851 @@
 
 /**
- * Reads a media registry entry from the main VirtualBox.xml file and recurses
- * into children where applicable.
+ * Reads a media registry entry from the main VirtualBox.xml file and
+ * likewise for all children where applicable.
  *
  * @param t
- * @param depth
  * @param elmMedium
  * @param med
  */
 void ConfigFileBase::readMedium(MediaType t,
-                                uint32_t depth,
-                                const xml::ElementNode &elmMedium,  // HardDisk node if root; if recursing,
-                                                                    // child HardDisk node or DiffHardDisk node for pre-1.4
-                                Medium &med)                        // medium settings to fill out
-{
-    if (depth > SETTINGS_MEDIUM_DEPTH_MAX)
-        throw ConfigFileError(this, &elmMedium, N_("Maximum medium tree depth of %u exceeded"), SETTINGS_MEDIUM_DEPTH_MAX);
-
-    // Do not inline this method call, as the purpose of having this separate
-    // is to save on stack size. Less local variables are the key for reaching
-    // deep recursion levels with small stack (XPCOM/g++ without optimization).
-    readMediumOne(t, elmMedium, med);
-
-    if (t != HardDisk)
-        return;
-
-    // recurse to handle children
-    MediaList &llSettingsChildren = med.llChildren;
-    xml::NodesLoop nl2(elmMedium, m->sv >= SettingsVersion_v1_4 ? "HardDisk" : "DiffHardDisk");
-    const xml::ElementNode *pelmHDChild;
-    while ((pelmHDChild = nl2.forAllNodes()))
-    {
-        // recurse with this element and put the child at the end of the list.
-        // XPCOM has very small stack, avoid big local variables and use the
-        // list element.
-        llSettingsChildren.push_back(Medium::Empty);
-        readMedium(t,
-                   depth + 1,
-                   *pelmHDChild,
-                   llSettingsChildren.back());
+                                const xml::ElementNode &elmMedium,
+                                Medium &med)
+{
+    std::list<const xml::ElementNode *> llElementsTodo;
+    llElementsTodo.push_back(&elmMedium);
+    std::list<Medium *> llSettingsTodo;
+    llSettingsTodo.push_back(&med);
+    std::list<uint32_t> llDepthsTodo;
+    llDepthsTodo.push_back(1);
+
+    while (llElementsTodo.size() > 0)
+    {
+        const xml::ElementNode *pElement = llElementsTodo.front();
+        llElementsTodo.pop_front();
+        Medium *pMed = llSettingsTodo.front();
+        llSettingsTodo.pop_front();
+        uint32_t depth = llDepthsTodo.front();
+        llDepthsTodo.pop_front();
+
+        if (depth > SETTINGS_MEDIUM_DEPTH_MAX)
+            throw ConfigFileError(this, pElement, N_("Maximum medium tree depth of %u exceeded"), SETTINGS_MEDIUM_DEPTH_MAX);
+
+        readMediumOne(t, *pElement, *pMed);
+
+        if (t != HardDisk)
+            return;
+
+        // load all children
+        xml::NodesLoop nl2(*pElement, m->sv >= SettingsVersion_v1_4 ? "HardDisk" : "DiffHardDisk");
+        const xml::ElementNode *pelmHDChild;
+        while ((pelmHDChild = nl2.forAllNodes()))
+        {
+            llElementsTodo.push_back(pelmHDChild);
+            pMed->llChildren.push_back(Medium::Empty);
+            llSettingsTodo.push_back(&pMed->llChildren.back());
+            llDepthsTodo.push_back(depth + 1);
+        }
     }
 }
@@ -930 +936 @@
             {
                 mr.llHardDisks.push_back(Medium::Empty);
-                readMedium(t, 1, *pelmMedium, mr.llHardDisks.back());
+                readMedium(t, *pelmMedium, mr.llHardDisks.back());
             }
             else if (   t == DVDImage
@@ -936 +942 @@
             {
                 mr.llDvdImages.push_back(Medium::Empty);
-                readMedium(t, 1, *pelmMedium, mr.llDvdImages.back());
+                readMedium(t, *pelmMedium, mr.llDvdImages.back());
             }
             else if (   t == FloppyImage
@@ -942 +948 @@
             {
                 mr.llFloppyImages.push_back(Medium::Empty);
-                readMedium(t, 1, *pelmMedium, mr.llFloppyImages.back());
+                readMedium(t, *pelmMedium, mr.llFloppyImages.back());
             }
         }
@@ -1265 +1271 @@
 /**
  * Creates a single \<HardDisk\> element for the given Medium structure
- * and recurses to write the child hard disks underneath. Called from
- * MainConfigFile::write().
+ * and all child hard disks underneath. Called from MainConfigFile::write().
  *
  * @param t
- * @param depth
  * @param elmMedium
- * @param mdm
+ * @param med
  */
 void ConfigFileBase::buildMedium(MediaType t,
-                                 uint32_t depth,
                                  xml::ElementNode &elmMedium,
-                                 const Medium &mdm)
-{
-    if (depth > SETTINGS_MEDIUM_DEPTH_MAX)
-        throw ConfigFileError(this, &elmMedium, N_("Maximum medium tree depth of %u exceeded"), SETTINGS_MEDIUM_DEPTH_MAX);
-
-    xml::ElementNode *pelmMedium;
-
-    if (t == HardDisk)
-        pelmMedium = elmMedium.createChild("HardDisk");
-    else
-        pelmMedium = elmMedium.createChild("Image");
-
-    pelmMedium->setAttribute("uuid", mdm.uuid.toStringCurly());
-
-    pelmMedium->setAttributePath("location", mdm.strLocation);
-
-    if (t == HardDisk || RTStrICmp(mdm.strFormat.c_str(), "RAW"))
-        pelmMedium->setAttribute("format", mdm.strFormat);
-    if (   t == HardDisk
-        && mdm.fAutoReset)
-        pelmMedium->setAttribute("autoReset", mdm.fAutoReset);
-    if (mdm.strDescription.length())
-        pelmMedium->createChild("Description")->addContent(mdm.strDescription);
-
-    for (StringsMap::const_iterator it = mdm.properties.begin();
-         it != mdm.properties.end();
-         ++it)
-    {
-        xml::ElementNode *pelmProp = pelmMedium->createChild("Property");
-        pelmProp->setAttribute("name", it->first);
-        pelmProp->setAttribute("value", it->second);
-    }
-
-    // only for base hard disks, save the type
-    if (depth == 1)
-    {
-        // no need to save the usual DVD/floppy medium types
-        if (   (   t != DVDImage
-                || (   mdm.hdType != MediumType_Writethrough // shouldn't happen
-                    && mdm.hdType != MediumType_Readonly))
-            && (   t != FloppyImage
-                || mdm.hdType != MediumType_Writethrough))
-        {
-            const char *pcszType =
-                mdm.hdType == MediumType_Normal ? "Normal" :
-                mdm.hdType == MediumType_Immutable ? "Immutable" :
-                mdm.hdType == MediumType_Writethrough ? "Writethrough" :
-                mdm.hdType == MediumType_Shareable ? "Shareable" :
-                mdm.hdType == MediumType_Readonly ? "Readonly" :
-                mdm.hdType == MediumType_MultiAttach ? "MultiAttach" :
-                "INVALID";
-            pelmMedium->setAttribute("type", pcszType);
-        }
-    }
-
-    for (MediaList::const_iterator it = mdm.llChildren.begin();
-         it != mdm.llChildren.end();
-         ++it)
-    {
-        // recurse for children
-        buildMedium(t,              // device type
-                    depth + 1,      // depth
-                    *pelmMedium,    // parent
-                    *it);           // settings::Medium
+                                 const Medium &med)
+{
+    std::list<const Medium *> llSettingsTodo;
+    llSettingsTodo.push_back(&med);
+    std::list<xml::ElementNode *> llElementsTodo;
+    llElementsTodo.push_back(&elmMedium);
+    std::list<uint32_t> llDepthsTodo;
+    llDepthsTodo.push_back(1);
+
+    while (llSettingsTodo.size() > 0)
+    {
+        const Medium *pMed = llSettingsTodo.front();
+        llSettingsTodo.pop_front();
+        xml::ElementNode *pElement = llElementsTodo.front();
+        llElementsTodo.pop_front();
+        uint32_t depth = llDepthsTodo.front();
+        llDepthsTodo.pop_front();
+
+        if (depth > SETTINGS_MEDIUM_DEPTH_MAX)
+            throw ConfigFileError(this, pElement, N_("Maximum medium tree depth of %u exceeded"), SETTINGS_MEDIUM_DEPTH_MAX);
+
+        xml::ElementNode *pelmMedium;
+
+        if (t == HardDisk)
+            pelmMedium = pElement->createChild("HardDisk");
+        else
+            pelmMedium = pElement->createChild("Image");
+
+        pelmMedium->setAttribute("uuid", pMed->uuid.toStringCurly());
+
+        pelmMedium->setAttributePath("location", pMed->strLocation);
+
+        if (t == HardDisk || RTStrICmp(pMed->strFormat.c_str(), "RAW"))
+            pelmMedium->setAttribute("format", pMed->strFormat);
+        if (   t == HardDisk
+            && pMed->fAutoReset)
+            pelmMedium->setAttribute("autoReset", pMed->fAutoReset);
+        if (pMed->strDescription.length())
+            pelmMedium->createChild("Description")->addContent(pMed->strDescription);
+
+        for (StringsMap::const_iterator it = pMed->properties.begin();
+             it != pMed->properties.end();
+             ++it)
+        {
+            xml::ElementNode *pelmProp = pelmMedium->createChild("Property");
+            pelmProp->setAttribute("name", it->first);
+            pelmProp->setAttribute("value", it->second);
+        }
+
+        // only for base hard disks, save the type
+        if (depth == 1)
+        {
+            // no need to save the usual DVD/floppy medium types
+            if (   (   t != DVDImage
+                    || (   pMed->hdType != MediumType_Writethrough // shouldn't happen
+                        && pMed->hdType != MediumType_Readonly))
+                && (   t != FloppyImage
+                    || pMed->hdType != MediumType_Writethrough))
+            {
+                const char *pcszType =
+                    pMed->hdType == MediumType_Normal ? "Normal" :
+                    pMed->hdType == MediumType_Immutable ? "Immutable" :
+                    pMed->hdType == MediumType_Writethrough ? "Writethrough" :
+                    pMed->hdType == MediumType_Shareable ? "Shareable" :
+                    pMed->hdType == MediumType_Readonly ? "Readonly" :
+                    pMed->hdType == MediumType_MultiAttach ? "MultiAttach" :
+                    "INVALID";
+                pelmMedium->setAttribute("type", pcszType);
+            }
+        }
+
+        /* save all children */
+        MediaList::const_iterator itBegin = pMed->llChildren.begin();
+        MediaList::const_iterator itEnd = pMed->llChildren.end();
+        for (MediaList::const_iterator it = itBegin; it != itEnd; ++it)
+        {
+            llSettingsTodo.push_back(&*it);
+            llElementsTodo.push_back(pelmMedium);
+            llDepthsTodo.push_back(depth + 1);
+        }
     }
 }
@@ -1369 +1388 @@
              ++it)
         {
-            buildMedium(HardDisk, 1, *pelmHardDisks, *it);
+            buildMedium(HardDisk, *pelmHardDisks, *it);
         }
     }
@@ -1380 +1399 @@
              ++it)
         {
-            buildMedium(DVDImage, 1, *pelmDVDImages, *it);
+            buildMedium(DVDImage, *pelmDVDImages, *it);
         }
     }
@@ -1391 +1410 @@
              ++it)
         {
-            buildMedium(FloppyImage, 1, *pelmFloppyImages, *it);
+            buildMedium(FloppyImage, *pelmFloppyImages, *it);
         }
     }
@@ -2589 +2608 @@
 
     clearDocument();
+    LogRel(("Finished saving settings file \"%s\"\n", m->strFilename.c_str()));
 }
 
@@ -5750 +5770 @@
  * Called for reading the \<Teleporter\> element under \<Machine\>.
  */
-void MachineConfigFile::readTeleporter(const xml::ElementNode *pElmTeleporter,
-                                       MachineUserData *pUserData)
-{
-    pElmTeleporter->getAttributeValue("enabled", pUserData->fTeleporterEnabled);
-    pElmTeleporter->getAttributeValue("port", pUserData->uTeleporterPort);
-    pElmTeleporter->getAttributeValue("address", pUserData->strTeleporterAddress);
-    pElmTeleporter->getAttributeValue("password", pUserData->strTeleporterPassword);
-
-    if (   pUserData->strTeleporterPassword.isNotEmpty()
-        && !VBoxIsPasswordHashed(&pUserData->strTeleporterPassword))
-        VBoxHashPassword(&pUserData->strTeleporterPassword);
+void MachineConfigFile::readTeleporter(const xml::ElementNode &elmTeleporter,
+                                       MachineUserData &userData)
+{
+    elmTeleporter.getAttributeValue("enabled", userData.fTeleporterEnabled);
+    elmTeleporter.getAttributeValue("port", userData.uTeleporterPort);
+    elmTeleporter.getAttributeValue("address", userData.strTeleporterAddress);
+    elmTeleporter.getAttributeValue("password", userData.strTeleporterPassword);
+
+    if (   userData.strTeleporterPassword.isNotEmpty()
+        && !VBoxIsPasswordHashed(&userData.strTeleporterPassword))
+        VBoxHashPassword(&userData.strTeleporterPassword);
 }
 
@@ -5766 +5786 @@
  * Called for reading the \<Debugging\> element under \<Machine\> or \<Snapshot\>.
  */
-void MachineConfigFile::readDebugging(const xml::ElementNode *pElmDebugging, Debugging *pDbg)
-{
-    if (!pElmDebugging || m->sv < SettingsVersion_v1_13)
+void MachineConfigFile::readDebugging(const xml::ElementNode &elmDebugging, Debugging &dbg)
+{
+    if (m->sv < SettingsVersion_v1_13)
         return;
 
-    const xml::ElementNode * const pelmTracing = pElmDebugging->findChildElement("Tracing");
+    const xml::ElementNode *pelmTracing = elmDebugging.findChildElement("Tracing");
     if (pelmTracing)
     {
-        pelmTracing->getAttributeValue("enabled", pDbg->fTracingEnabled);
-        pelmTracing->getAttributeValue("allowTracingToAccessVM", pDbg->fAllowTracingToAccessVM);
-        pelmTracing->getAttributeValue("config", pDbg->strTracingConfig);
+        pelmTracing->getAttributeValue("enabled", dbg.fTracingEnabled);
+        pelmTracing->getAttributeValue("allowTracingToAccessVM", dbg.fAllowTracingToAccessVM);
+        pelmTracing->getAttributeValue("config", dbg.strTracingConfig);
     }
 }
@@ -5783 +5803 @@
  * Called for reading the \<Autostart\> element under \<Machine\> or \<Snapshot\>.
  */
-void MachineConfigFile::readAutostart(const xml::ElementNode *pElmAutostart, Autostart *pAutostart)
+void MachineConfigFile::readAutostart(const xml::ElementNode &elmAutostart, Autostart &autostrt)
 {
     Utf8Str strAutostop;
 
-    if (!pElmAutostart || m->sv < SettingsVersion_v1_13)
+    if (m->sv < SettingsVersion_v1_13)
         return;
 
-    pElmAutostart->getAttributeValue("enabled", pAutostart->fAutostartEnabled);
-    pElmAutostart->getAttributeValue("delay", pAutostart->uAutostartDelay);
-    pElmAutostart->getAttributeValue("autostop", strAutostop);
+    elmAutostart.getAttributeValue("enabled", autostrt.fAutostartEnabled);
+    elmAutostart.getAttributeValue("delay", autostrt.uAutostartDelay);
+    elmAutostart.getAttributeValue("autostop", strAutostop);
     if (strAutostop == "Disabled")
-        pAutostart->enmAutostopType = AutostopType_Disabled;
+        autostrt.enmAutostopType = AutostopType_Disabled;
     else if (strAutostop == "SaveState")
-        pAutostart->enmAutostopType = AutostopType_SaveState;
+        autostrt.enmAutostopType = AutostopType_SaveState;
     else if (strAutostop == "PowerOff")
-        pAutostart->enmAutostopType = AutostopType_PowerOff;
+        autostrt.enmAutostopType = AutostopType_PowerOff;
     else if (strAutostop == "AcpiShutdown")
-        pAutostart->enmAutostopType = AutostopType_AcpiShutdown;
+        autostrt.enmAutostopType = AutostopType_AcpiShutdown;
     else
-        throw ConfigFileError(this, pElmAutostart, N_("Invalid value '%s' for Autostart/@autostop attribute"), strAutostop.c_str());
+        throw ConfigFileError(this, &elmAutostart, N_("Invalid value '%s' for Autostart/@autostop attribute"), strAutostop.c_str());
 }
 
@@ -5808 +5828 @@
  * Called for reading the \<Groups\> element under \<Machine\>.
  */
-void MachineConfigFile::readGroups(const xml::ElementNode *pElmGroups, StringsList *pllGroups)
-{
-    pllGroups->clear();
-    if (!pElmGroups || m->sv < SettingsVersion_v1_13)
-    {
-        pllGroups->push_back("/");
+void MachineConfigFile::readGroups(const xml::ElementNode &elmGroups, StringsList &llGroups)
+{
+    llGroups.clear();
+    if (m->sv < SettingsVersion_v1_13)
+    {
+        llGroups.push_back("/");
         return;
     }
 
-    xml::NodesLoop nlGroups(*pElmGroups);
+    xml::NodesLoop nlGroups(elmGroups);
     const xml::ElementNode *pelmGroup;
     while ((pelmGroup = nlGroups.forAllNodes()))
@@ -5826 +5846 @@
             if (!pelmGroup->getAttributeValue("name", strGroup))
                 throw ConfigFileError(this, pelmGroup, N_("Required Group/@name attribute is missing"));
-            pllGroups->push_back(strGroup);
+            llGroups.push_back(strGroup);
         }
     }
@@ -5834 +5854 @@
  * Called initially for the \<Snapshot\> element under \<Machine\>, if present,
  * to store the snapshot's data into the given Snapshot structure (which is
- * then the one in the Machine struct). This might then recurse if
- * a \<Snapshots\> (plural) element is found in the snapshot, which should
- * contain a list of child snapshots; such lists are maintained in the
- * Snapshot structure.
+ * then the one in the Machine struct). This might process further elements
+ * of the snapshot tree if a \<Snapshots\> (plural) element is found in the
+ * snapshot, which should contain a list of child snapshots; such lists are
+ * maintained in the Snapshot structure.
  *
  * @param curSnapshotUuid
- * @param depth
  * @param elmSnapshot
  * @param snap
@@ -5846 +5865 @@
  */
 bool MachineConfigFile::readSnapshot(const Guid &curSnapshotUuid,
-                                     uint32_t depth,
                                      const xml::ElementNode &elmSnapshot,
                                      Snapshot &snap)
 {
-    if (depth > SETTINGS_SNAPSHOT_DEPTH_MAX)
-        throw ConfigFileError(this, &elmSnapshot, N_("Maximum snapshot tree depth of %u exceeded"), SETTINGS_SNAPSHOT_DEPTH_MAX);
-
-    Utf8Str strTemp;
-
-    if (!elmSnapshot.getAttributeValue("uuid", strTemp))
-        throw ConfigFileError(this, &elmSnapshot, N_("Required Snapshot/@uuid attribute is missing"));
-    parseUUID(snap.uuid, strTemp, &elmSnapshot);
-    bool foundCurrentSnapshot = (snap.uuid == curSnapshotUuid);
-
-    if (!elmSnapshot.getAttributeValue("name", snap.strName))
-        throw ConfigFileError(this, &elmSnapshot, N_("Required Snapshot/@name attribute is missing"));
-
-    // 3.1 dev builds added Description as an attribute, read it silently
-    // and write it back as an element
-    elmSnapshot.getAttributeValue("Description", snap.strDescription);
-
-    if (!elmSnapshot.getAttributeValue("timeStamp", strTemp))
-        throw ConfigFileError(this, &elmSnapshot, N_("Required Snapshot/@timeStamp attribute is missing"));
-    parseTimestamp(snap.timestamp, strTemp, &elmSnapshot);
-
-    elmSnapshot.getAttributeValuePath("stateFile", snap.strStateFile);      // online snapshots only
-
-    // parse Hardware before the other elements because other things depend on it
-    const xml::ElementNode *pelmHardware;
-    if (!(pelmHardware = elmSnapshot.findChildElement("Hardware")))
-        throw ConfigFileError(this, &elmSnapshot, N_("Required Snapshot/@Hardware element is missing"));
-    readHardware(*pelmHardware, snap.hardware);
-
-    xml::NodesLoop nlSnapshotChildren(elmSnapshot);
-    const xml::ElementNode *pelmSnapshotChild;
-    while ((pelmSnapshotChild = nlSnapshotChildren.forAllNodes()))
-    {
-        if (pelmSnapshotChild->nameEquals("Description"))
-            snap.strDescription = pelmSnapshotChild->getValue();
-        else if (   m->sv < SettingsVersion_v1_7
-                 && pelmSnapshotChild->nameEquals("HardDiskAttachments"))
-            readHardDiskAttachments_pre1_7(*pelmSnapshotChild, snap.hardware.storage);
-        else if (   m->sv >= SettingsVersion_v1_7
-                 && pelmSnapshotChild->nameEquals("StorageControllers"))
-            readStorageControllers(*pelmSnapshotChild, snap.hardware.storage);
-        else if (pelmSnapshotChild->nameEquals("Snapshots"))
-        {
-            xml::NodesLoop nlChildSnapshots(*pelmSnapshotChild);
+    std::list<const xml::ElementNode *> llElementsTodo;
+    llElementsTodo.push_back(&elmSnapshot);
+    std::list<Snapshot *> llSettingsTodo;
+    llSettingsTodo.push_back(&snap);
+    std::list<uint32_t> llDepthsTodo;
+    llDepthsTodo.push_back(1);
+
+    bool foundCurrentSnapshot = false;
+
+    while (llElementsTodo.size() > 0)
+    {
+        const xml::ElementNode *pElement = llElementsTodo.front();
+        llElementsTodo.pop_front();
+        Snapshot *pSnap = llSettingsTodo.front();
+        llSettingsTodo.pop_front();
+        uint32_t depth = llDepthsTodo.front();
+        llDepthsTodo.pop_front();
+
+        if (depth > SETTINGS_SNAPSHOT_DEPTH_MAX)
+            throw ConfigFileError(this, pElement, N_("Maximum snapshot tree depth of %u exceeded"), SETTINGS_SNAPSHOT_DEPTH_MAX);
+
+        Utf8Str strTemp;
+        if (!pElement->getAttributeValue("uuid", strTemp))
+            throw ConfigFileError(this, pElement, N_("Required Snapshot/@uuid attribute is missing"));
+        parseUUID(pSnap->uuid, strTemp, pElement);
+        foundCurrentSnapshot |= (pSnap->uuid == curSnapshotUuid);
+
+        if (!pElement->getAttributeValue("name", pSnap->strName))
+            throw ConfigFileError(this, pElement, N_("Required Snapshot/@name attribute is missing"));
+
+        // 3.1 dev builds added Description as an attribute, read it silently
+        // and write it back as an element
+        pElement->getAttributeValue("Description", pSnap->strDescription);
+
+        if (!pElement->getAttributeValue("timeStamp", strTemp))
+            throw ConfigFileError(this, pElement, N_("Required Snapshot/@timeStamp attribute is missing"));
+        parseTimestamp(pSnap->timestamp, strTemp, pElement);
+
+        pElement->getAttributeValuePath("stateFile", pSnap->strStateFile);      // online snapshots only
+
+        // parse Hardware before the other elements because other things depend on it
+        const xml::ElementNode *pelmHardware;
+        if (!(pelmHardware = pElement->findChildElement("Hardware")))
+            throw ConfigFileError(this, pElement, N_("Required Snapshot/@Hardware element is missing"));
+        readHardware(*pelmHardware, pSnap->hardware);
+
+        const xml::ElementNode *pelmSnapshots = NULL;
+
+        xml::NodesLoop nlSnapshotChildren(*pElement);
+        const xml::ElementNode *pelmSnapshotChild;
+        while ((pelmSnapshotChild = nlSnapshotChildren.forAllNodes()))
+        {
+            if (pelmSnapshotChild->nameEquals("Description"))
+                pSnap->strDescription = pelmSnapshotChild->getValue();
+            else if (   m->sv < SettingsVersion_v1_7
+                    && pelmSnapshotChild->nameEquals("HardDiskAttachments"))
+                readHardDiskAttachments_pre1_7(*pelmSnapshotChild, pSnap->hardware.storage);
+            else if (   m->sv >= SettingsVersion_v1_7
+                    && pelmSnapshotChild->nameEquals("StorageControllers"))
+                readStorageControllers(*pelmSnapshotChild, pSnap->hardware.storage);
+            else if (pelmSnapshotChild->nameEquals("Snapshots"))
+            {
+                if (pelmSnapshots)
+                    throw ConfigFileError(this, pelmSnapshotChild, N_("Just a single Snapshots element is allowed"));
+                pelmSnapshots = pelmSnapshotChild;
+            }
+        }
+
+        if (m->sv < SettingsVersion_v1_9)
+            // go through Hardware once more to repair the settings controller structures
+            // with data from old DVDDrive and FloppyDrive elements
+            readDVDAndFloppies_pre1_9(*pelmHardware, pSnap->hardware.storage);
+
+        const xml::ElementNode *pelmDebugging = elmSnapshot.findChildElement("Debugging");
+        if (pelmDebugging)
+            readDebugging(*pelmDebugging, pSnap->debugging);
+        const xml::ElementNode *pelmAutostart = elmSnapshot.findChildElement("Autostart");
+        if (pelmAutostart)
+            readAutostart(*pelmAutostart, pSnap->autostart);
+        // note: Groups exist only for Machine, not for Snapshot
+
+        // process all child snapshots
+        if (pelmSnapshots)
+        {
+            xml::NodesLoop nlChildSnapshots(*pelmSnapshots);
             const xml::ElementNode *pelmChildSnapshot;
             while ((pelmChildSnapshot = nlChildSnapshots.forAllNodes()))
@@ -5899 +5958 @@
                 if (pelmChildSnapshot->nameEquals("Snapshot"))
                 {
-                    // recurse with this element and put the child at the
-                    // end of the list. XPCOM has very small stack, avoid
-                    // big local variables and use the list element.
-                    snap.llChildSnapshots.push_back(Snapshot::Empty);
-                    bool found = readSnapshot(curSnapshotUuid, depth + 1, *pelmChildSnapshot, snap.llChildSnapshots.back());
-                    foundCurrentSnapshot = foundCurrentSnapshot || found;
+                    llElementsTodo.push_back(pelmChildSnapshot);
+                    pSnap->llChildSnapshots.push_back(Snapshot::Empty);
+                    llSettingsTodo.push_back(&pSnap->llChildSnapshots.back());
+                    llDepthsTodo.push_back(depth + 1);
                 }
             }
         }
     }
-
-    if (m->sv < SettingsVersion_v1_9)
-        // go through Hardware once more to repair the settings controller structures
-        // with data from old DVDDrive and FloppyDrive elements
-        readDVDAndFloppies_pre1_9(*pelmHardware, snap.hardware.storage);
-
-    readDebugging(elmSnapshot.findChildElement("Debugging"), &snap.debugging);
-    readAutostart(elmSnapshot.findChildElement("Autostart"), &snap.autostart);
-    // note: Groups exist only for Machine, not for Snapshot
 
     return foundCurrentSnapshot;
@@ -6062 +6110 @@
                     throw ConfigFileError(this, &elmMachine, N_("Snapshots present but required Machine/@currentSnapshot attribute is missing"));
                 bool foundCurrentSnapshot = false;
-                Snapshot snap;
-                // this will recurse into child snapshots, if necessary
-                foundCurrentSnapshot = readSnapshot(uuidCurrentSnapshot, 1, *pelmMachineChild, snap);
+                // Work directly with the target list, because otherwise
+                // the entire snapshot settings tree will need to be copied,
+                // and the usual STL implementation needs a lot of stack space.
+                llFirstSnapshot.push_back(Snapshot::Empty);
+                // this will also read all child snapshots
+                foundCurrentSnapshot = readSnapshot(uuidCurrentSnapshot, *pelmMachineChild, llFirstSnapshot.back());
                 if (!foundCurrentSnapshot)
                     throw ConfigFileError(this, &elmMachine, N_("Snapshots present but none matches the UUID in the Machine/@currentSnapshot attribute"));
-                llFirstSnapshot.push_back(snap);
             }
             else if (pelmMachineChild->nameEquals("Description"))
                 machineUserData.strDescription = pelmMachineChild->getValue();
             else if (pelmMachineChild->nameEquals("Teleporter"))
-                readTeleporter(pelmMachineChild, &machineUserData);
+                readTeleporter(*pelmMachineChild, machineUserData);
             else if (pelmMachineChild->nameEquals("MediaRegistry"))
                 readMediaRegistry(*pelmMachineChild, mediaRegistry);
             else if (pelmMachineChild->nameEquals("Debugging"))
-                readDebugging(pelmMachineChild, &debugging);
+                readDebugging(*pelmMachineChild, debugging);
             else if (pelmMachineChild->nameEquals("Autostart"))
-                readAutostart(pelmMachineChild, &autostart);
+                readAutostart(*pelmMachineChild, autostart);
             else if (pelmMachineChild->nameEquals("Groups"))
-                readGroups(pelmMachineChild, &machineUserData.llGroups);
+                readGroups(*pelmMachineChild, machineUserData.llGroups);
         }
 
@@ -7604 +7654 @@
  * keys under that. Called for both the \<Machine\> node and for snapshots.
  *
- * @param pElmParent    Pointer to the parent element.
- * @param pDbg          Pointer to the debugging settings.
- */
-void MachineConfigFile::buildDebuggingXML(xml::ElementNode *pElmParent, const Debugging *pDbg)
-{
-    if (m->sv < SettingsVersion_v1_13 || pDbg->areDefaultSettings())
+ * @param elmParent     Parent element.
+ * @param dbg           Debugging settings.
+ */
+void MachineConfigFile::buildDebuggingXML(xml::ElementNode &elmParent, const Debugging &dbg)
+{
+    if (m->sv < SettingsVersion_v1_13 || dbg.areDefaultSettings())
         return;
 
-    xml::ElementNode *pElmDebugging = pElmParent->createChild("Debugging");
+    xml::ElementNode *pElmDebugging = elmParent.createChild("Debugging");
     xml::ElementNode *pElmTracing   = pElmDebugging->createChild("Tracing");
-    pElmTracing->setAttribute("enabled", pDbg->fTracingEnabled);
-    pElmTracing->setAttribute("allowTracingToAccessVM", pDbg->fAllowTracingToAccessVM);
-    pElmTracing->setAttribute("config", pDbg->strTracingConfig);
+    pElmTracing->setAttribute("enabled", dbg.fTracingEnabled);
+    pElmTracing->setAttribute("allowTracingToAccessVM", dbg.fAllowTracingToAccessVM);
+    pElmTracing->setAttribute("config", dbg.strTracingConfig);
 }
 
@@ -7623 +7673 @@
  * keys under that. Called for both the \<Machine\> node and for snapshots.
  *
- * @param pElmParent    Pointer to the parent element.
- * @param pAutostart    Pointer to the autostart settings.
- */
-void MachineConfigFile::buildAutostartXML(xml::ElementNode *pElmParent, const Autostart *pAutostart)
+ * @param elmParent     Parent element.
+ * @param autostrt      Autostart settings.
+ */
+void MachineConfigFile::buildAutostartXML(xml::ElementNode &elmParent, const Autostart &autostrt)
 {
     const char *pcszAutostop = NULL;
 
-    if (m->sv < SettingsVersion_v1_13 || pAutostart->areDefaultSettings())
+    if (m->sv < SettingsVersion_v1_13 || autostrt.areDefaultSettings())
         return;
 
-    xml::ElementNode *pElmAutostart = pElmParent->createChild("Autostart");
-    pElmAutostart->setAttribute("enabled", pAutostart->fAutostartEnabled);
-    pElmAutostart->setAttribute("delay", pAutostart->uAutostartDelay);
-
-    switch (pAutostart->enmAutostopType)
+    xml::ElementNode *pElmAutostart = elmParent.createChild("Autostart");
+    pElmAutostart->setAttribute("enabled", autostrt.fAutostartEnabled);
+    pElmAutostart->setAttribute("delay", autostrt.uAutostartDelay);
+
+    switch (autostrt.enmAutostopType)
     {
         case AutostopType_Disabled:     pcszAutostop = "Disabled";     break;
@@ -7652 +7702 @@
  * keys under that. Called for the \<Machine\> node only.
  *
- * @param pElmParent    Pointer to the parent element.
- * @param pllGroups     Pointer to the groups list.
- */
-void MachineConfigFile::buildGroupsXML(xml::ElementNode *pElmParent, const StringsList *pllGroups)
-{
-    if (   m->sv < SettingsVersion_v1_13 || pllGroups->size() == 0
-        || (pllGroups->size() == 1 && pllGroups->front() == "/"))
+ * @param elmParent     Parent element.
+ * @param llGroups      Groups list.
+ */
+void MachineConfigFile::buildGroupsXML(xml::ElementNode &elmParent, const StringsList &llGroups)
+{
+    if (   m->sv < SettingsVersion_v1_13 || llGroups.size() == 0
+        || (llGroups.size() == 1 && llGroups.front() == "/"))
         return;
 
-    xml::ElementNode *pElmGroups = pElmParent->createChild("Groups");
-    for (StringsList::const_iterator it = pllGroups->begin();
-         it != pllGroups->end();
+    xml::ElementNode *pElmGroups = elmParent.createChild("Groups");
+    for (StringsList::const_iterator it = llGroups.begin();
+         it != llGroups.end();
          ++it)
     {
@@ -7673 +7723 @@
 
 /**
- * Writes a single snapshot into the DOM tree. Initially this gets called from MachineConfigFile::write()
- * for the root snapshot of a machine, if present; elmParent then points to the \<Snapshots\> node under the
- * \<Machine\> node to which \<Snapshot\> must be added. This may then recurse for child snapshots.
+ * Writes a single snapshot into the DOM tree. Initially this gets called from
+ * MachineConfigFile::write() for the root snapshot of a machine, if present;
+ * elmParent then points to the \<Snapshots\> node under the \<Machine\> node
+ * to which \<Snapshot\> must be added. This may then continue processing the
+ * child snapshots.
  *
- * @param depth
  * @param elmParent
  * @param snap
  */
-void MachineConfigFile::buildSnapshotXML(uint32_t depth,
-                                         xml::ElementNode &elmParent,
+void MachineConfigFile::buildSnapshotXML(xml::ElementNode &elmParent,
                                          const Snapshot &snap)
 {
-    if (depth > SETTINGS_SNAPSHOT_DEPTH_MAX)
-        throw ConfigFileError(this, NULL, N_("Maximum snapshot tree depth of %u exceeded"), SETTINGS_SNAPSHOT_DEPTH_MAX);
-
-    xml::ElementNode *pelmSnapshot = elmParent.createChild("Snapshot");
-
-    pelmSnapshot->setAttribute("uuid", snap.uuid.toStringCurly());
-    pelmSnapshot->setAttribute("name", snap.strName);
-    pelmSnapshot->setAttribute("timeStamp", stringifyTimestamp(snap.timestamp));
-
-    if (snap.strStateFile.length())
-        pelmSnapshot->setAttributePath("stateFile", snap.strStateFile);
-
-    if (snap.strDescription.length())
-        pelmSnapshot->createChild("Description")->addContent(snap.strDescription);
-
-    // We only skip removable media for OVF, but OVF never includes snapshots.
-    buildHardwareXML(*pelmSnapshot, snap.hardware, 0 /* fl */, NULL /* pllElementsWithUuidAttributes */);
-    buildDebuggingXML(pelmSnapshot, &snap.debugging);
-    buildAutostartXML(pelmSnapshot, &snap.autostart);
-    // note: Groups exist only for Machine, not for Snapshot
-
-    if (snap.llChildSnapshots.size())
-    {
-        xml::ElementNode *pelmChildren = pelmSnapshot->createChild("Snapshots");
-        for (SnapshotsList::const_iterator it = snap.llChildSnapshots.begin();
-             it != snap.llChildSnapshots.end();
-             ++it)
-        {
-            const Snapshot &child = *it;
-            buildSnapshotXML(depth + 1, *pelmChildren, child);
+    std::list<const Snapshot *> llSettingsTodo;
+    llSettingsTodo.push_back(&snap);
+    std::list<xml::ElementNode *> llElementsTodo;
+    llElementsTodo.push_back(&elmParent);
+    std::list<uint32_t> llDepthsTodo;
+    llDepthsTodo.push_back(1);
+
+    while (llSettingsTodo.size() > 0)
+    {
+        const Snapshot *pSnap = llSettingsTodo.front();
+        llSettingsTodo.pop_front();
+        xml::ElementNode *pElement = llElementsTodo.front();
+        llElementsTodo.pop_front();
+        uint32_t depth = llDepthsTodo.front();
+        llDepthsTodo.pop_front();
+
+        if (depth > SETTINGS_SNAPSHOT_DEPTH_MAX)
+            throw ConfigFileError(this, NULL, N_("Maximum snapshot tree depth of %u exceeded"), SETTINGS_SNAPSHOT_DEPTH_MAX);
+
+        xml::ElementNode *pelmSnapshot = pElement->createChild("Snapshot");
+
+        pelmSnapshot->setAttribute("uuid", pSnap->uuid.toStringCurly());
+        pelmSnapshot->setAttribute("name", pSnap->strName);
+        pelmSnapshot->setAttribute("timeStamp", stringifyTimestamp(pSnap->timestamp));
+
+        if (pSnap->strStateFile.length())
+            pelmSnapshot->setAttributePath("stateFile", pSnap->strStateFile);
+
+        if (pSnap->strDescription.length())
+            pelmSnapshot->createChild("Description")->addContent(pSnap->strDescription);
+
+        // We only skip removable media for OVF, but OVF never includes snapshots.
+        buildHardwareXML(*pelmSnapshot, pSnap->hardware, 0 /* fl */, NULL /* pllElementsWithUuidAttributes */);
+        buildDebuggingXML(*pelmSnapshot, pSnap->debugging);
+        buildAutostartXML(*pelmSnapshot, pSnap->autostart);
+        // note: Groups exist only for Machine, not for Snapshot
+
+        if (pSnap->llChildSnapshots.size())
+        {
+            xml::ElementNode *pelmChildren = pelmSnapshot->createChild("Snapshots");
+            for (SnapshotsList::const_iterator it = pSnap->llChildSnapshots.begin();
+                it != pSnap->llChildSnapshots.end();
+                ++it)
+            {
+                llSettingsTodo.push_back(&*it);
+                llElementsTodo.push_back(pelmChildren);
+                llDepthsTodo.push_back(depth + 1);
+            }
         }
     }
@@ -7850 +7918 @@
     if (    (fl & BuildMachineXML_IncludeSnapshots)
          && llFirstSnapshot.size())
-        buildSnapshotXML(1, elmMachine, llFirstSnapshot.front());
+        buildSnapshotXML(elmMachine, llFirstSnapshot.front());
 
     buildHardwareXML(elmMachine, hardwareMachine, fl, pllElementsWithUuidAttributes);
-    buildDebuggingXML(&elmMachine, &debugging);
-    buildAutostartXML(&elmMachine, &autostart);
-    buildGroupsXML(&elmMachine, &machineUserData.llGroups);
+    buildDebuggingXML(elmMachine, debugging);
+    buildAutostartXML(elmMachine, autostart);
+    buildGroupsXML(elmMachine, machineUserData.llGroups);
 }
 
@@ -8650 +8718 @@
         m->fFileExists = true;
         clearDocument();
+        LogRel(("Finished saving settings file \"%s\"\n", m->strFilename.c_str()));
     }
     catch (...)
     {
         clearDocument();
+        LogRel(("Finished saving settings file \"%s\" with failure\n", m->strFilename.c_str()));
         throw;
     }

trunk/src/VBox/ValidationKit/tests/api/tdTreeDepth1.py

@@ -34 +34 @@
 import os
 import sys
+import random
 
 # Only the main script needs to modify the path.
@@ -73 +74 @@
 
         try:
+            oVBox = self.oTstDrv.oVBoxMgr.getVirtualBox()
             oVM = self.oTstDrv.createTestVM('test-medium', 1, None, 4)
             assert oVM is not None
@@ -79 +81 @@
             fRc = True
             oSession = self.oTstDrv.openSession(oVM)
-            for i in range(1, 301):
+            cImages = 38 #00
+            for i in range(1, cImages + 1):
                 sHddPath = os.path.join(self.oTstDrv.sScratchPath, 'Test' + str(i) + '.vdi')
                 if i == 1:
@@ -93 +96 @@
             fRc = fRc and oSession.saveSettings()
             fRc = oSession.close() and fRc
-
-            # unregister and re-register to test loading of settings
+            ## @todo r=klaus: count known hard disk images, should be cImages
+
+            # unregister, making sure the images are closed
             sSettingsFile = oVM.settingsFilePath
-            reporter.log('unregistering VM')
-            oVM.unregister(vboxcon.CleanupMode_DetachAllReturnNone)
-            oVBox = self.oTstDrv.oVBoxMgr.getVirtualBox()
+            fDetachAll = random.choice([False, True])
+            if fDetachAll:
+                reporter.log('unregistering VM, DetachAll style')
+            else:
+                reporter.log('unregistering VM, UnregisterOnly style')
+            self.oTstDrv.forgetTestMachine(oVM)
+            if fDetachAll:
+                aoHDs = oVM.unregister(vboxcon.CleanupMode_DetachAllReturnHardDisksOnly)
+                for oHD in aoHDs:
+                    oHD.close()
+                aoHDs = None
+            else:
+                oVM.unregister(vboxcon.CleanupMode_UnregisterOnly)
+            oVM = None
+            
+            # If there is no base image (expected) then there are no leftover
+            # child images either. Can be changed later once the todos above
+            # and below are resolved.
+            cBaseImages = len(self.oTstDrv.oVBoxMgr.getArray(oVBox, 'hardDisks'))
+            reporter.log('API reports %i base images' % (cBaseImages))
+            fRc = fRc and cBaseImages == 0
+
+            # re-register to test loading of settings
             reporter.log('opening VM %s, testing config reading' % (sSettingsFile))
             oVM = oVBox.openMachine(sSettingsFile)
+            ## @todo r=klaus: count known hard disk images, should be cImages
+
+            reporter.log('unregistering VM')
+            oVM.unregister(vboxcon.CleanupMode_UnregisterOnly)
+            oVM = None
+
+            cBaseImages = len(self.oTstDrv.oVBoxMgr.getArray(oVBox, 'hardDisks'))
+            reporter.log('API reports %i base images' % (cBaseImages))
+            fRc = fRc and cBaseImages == 0
 
             assert fRc is True
@@ -115 +148 @@
 
         try:
+            oVBox = self.oTstDrv.oVBoxMgr.getVirtualBox()
             oVM = self.oTstDrv.createTestVM('test-snap', 1, None, 4)
             assert oVM is not None
@@ -126 +160 @@
 
             # take 250 snapshots (snapshot tree depth limit)
-            for i in range(1, 251):
+            cSnapshots = 13 #00
+            for i in range(1, cSnapshots + 1):
                 fRc = fRc and oSession.takeSnapshot('Snapshot ' + str(i))
             fRc = oSession.close() and fRc
-
-            # unregister and re-register to test loading of settings
+            oSession = None
+            reporter.log('API reports %i snapshots' % (oVM.snapshotCount))
+            fRc = fRc and oVM.snapshotCount == cSnapshots
+
+            assert fRc is True
+
+            # unregister, making sure the images are closed
             sSettingsFile = oVM.settingsFilePath
-            reporter.log('unregistering VM')
-            oVM.unregister(vboxcon.CleanupMode_DetachAllReturnNone)
-            oVBox = self.oTstDrv.oVBoxMgr.getVirtualBox()
+            fDetachAll = random.choice([False, True])
+            if fDetachAll:
+                reporter.log('unregistering VM, DetachAll style')
+            else:
+                reporter.log('unregistering VM, UnregisterOnly style')
+            self.oTstDrv.forgetTestMachine(oVM)
+            if fDetachAll:
+                aoHDs = oVM.unregister(vboxcon.CleanupMode_DetachAllReturnHardDisksOnly)
+                for oHD in aoHDs:
+                    oHD.close()
+                aoHDs = None
+            else:
+                oVM.unregister(vboxcon.CleanupMode_UnregisterOnly)
+            oVM = None
+
+            # If there is no base image (expected) then there are no leftover
+            # child images either. Can be changed later once the todos above
+            # and below are resolved.
+            cBaseImages = len(self.oTstDrv.oVBoxMgr.getArray(oVBox, 'hardDisks'))
+            reporter.log('API reports %i base images' % (cBaseImages))
+            fRc = fRc and cBaseImages == 0
+
+            # re-register to test loading of settings
             reporter.log('opening VM %s, testing config reading' % (sSettingsFile))
             oVM = oVBox.openMachine(sSettingsFile)
+            reporter.log('API reports %i snapshots' % (oVM.snapshotCount))
+            fRc = fRc and oVM.snapshotCount == cSnapshots
+
+            reporter.log('unregistering VM')
+            oVM.unregister(vboxcon.CleanupMode_UnregisterOnly)
+            oVM = None
+
+            cBaseImages = len(self.oTstDrv.oVBoxMgr.getArray(oVBox, 'hardDisks'))
+            reporter.log('API reports %i base images' % (cBaseImages))
+            fRc = fRc and cBaseImages == 0
 
             assert fRc is True