Changeset 54948 in vbox

Timestamp:

Mar 25, 2015 4:56:48 PM (10 years ago)

Author:

vboxsync

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

99196

Message:

Main/Medium+Snapshot: make all code recursing over trees (objects containing lists of child objects) use as little stack as possible, and establish safe depth limits to avoid crashes, plus related cleanups in related code areas

Location:

trunk

Files:

• include/VBox/settings.h (modified) (8 diffs)
• src/VBox/Main/include/MediumImpl.h (modified) (5 diffs)
• src/VBox/Main/include/SnapshotImpl.h (modified) (2 diffs)
• src/VBox/Main/include/VirtualBoxImpl.h (modified) (2 diffs)
• src/VBox/Main/src-server/MachineImpl.cpp (modified) (8 diffs)
• src/VBox/Main/src-server/MachineImplCloneVM.cpp (modified) (1 diff)
• src/VBox/Main/src-server/MediumImpl.cpp (modified) (31 diffs)
• src/VBox/Main/src-server/SnapshotImpl.cpp (modified) (10 diffs)
• src/VBox/Main/src-server/VirtualBoxImpl.cpp (modified) (9 diffs)
• src/VBox/Main/xml/Settings.cpp (modified) (18 diffs)

trunk/include/VBox/settings.h

@@ -18 +18 @@
 
 /*
- * Copyright (C) 2007-2014 Oracle Corporation
+ * Copyright (C) 2007-2015 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -52 +52 @@
 
 /**
+ * Maximum depth of a medium tree, to prevent stack overflows.
+ * 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.
+ */
+#define SETTINGS_MEDIUM_DEPTH_MAX 300
+
+/**
  * Maximum depth of the snapshot tree, to prevent stack overflows.
  * 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 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.
  */
 #define SETTINGS_SNAPSHOT_DEPTH_MAX 250
@@ -144 +154 @@
 };
 
+extern const struct Medium g_MediumEmpty;
+
 /**
  * A media registry. Starting with VirtualBox 3.3, this can appear in both the
@@ -229 +241 @@
     ~ConfigFileBase();
 
+    typedef enum {Error, HardDisk, DVDImage, FloppyImage} MediaType;
+
+    static const char *stringifyMediaType(MediaType t);
     void parseUUID(com::Guid &guid,
                    const com::Utf8Str &strUUID) const;
     void parseTimestamp(RTTIMESPEC &timestamp,
                         const com::Utf8Str &str) const;
-
-    com::Utf8Str makeString(const RTTIMESPEC &tm);
+    com::Utf8Str stringifyTimestamp(const RTTIMESPEC &tm) const;
 
     void readExtraData(const xml::ElementNode &elmExtraData,
@@ -240 +254 @@
     void readUSBDeviceFilters(const xml::ElementNode &elmDeviceFilters,
                               USBDeviceFiltersList &ll);
-    typedef enum {Error, HardDisk, DVDImage, FloppyImage} MediaType;
-    void readMedium(MediaType t, const xml::ElementNode &elmMedium, MediaList &llMedia);
+    void readMediumOne(MediaType t, const xml::ElementNode &elmMedium, Medium &med);
+    void readMedium(MediaType t, uint32_t depth, const xml::ElementNode &elmMedium, Medium &med);
     void readMediaRegistry(const xml::ElementNode &elmMediaRegistry, MediaRegistry &mr);
     void readNATForwardRuleList(const xml::ElementNode  &elmParent, NATRuleList &llRules);
@@ -253 +267 @@
                                const USBDeviceFiltersList &ll,
                                bool fHostMode);
-    void buildMedium(xml::ElementNode &elmMedium,
-                     DeviceType_T devType,
-                     const Medium &m,
-                     uint32_t level);
+    void buildMedium(MediaType t,
+                     uint32_t depth,
+                     xml::ElementNode &elmMedium,
+                     const Medium &mdm);
     void buildMediaRegistry(xml::ElementNode &elmParent,
                             const MediaRegistry &mr);
@@ -1151 +1165 @@
 struct Snapshot
 {
+    Snapshot()
+    {
+        RTTimeSpecSetNano(&timestamp, 0);
+    }
+
     bool operator==(const Snapshot &s) const;
 
@@ -1225 +1244 @@
     com::Utf8Str            ovIcon;
 };
+
+extern const struct Snapshot g_SnapshotEmpty;
 
 /**

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

@@ -1 +1 @@
 /* $Id$ */
-
 /** @file
- *
  * VirtualBox COM class implementation
  */
@@ -72 +70 @@
 
     // initializer used when loading settings
+    HRESULT initOne(Medium *aParent,
+                    DeviceType_T aDeviceType,
+                    const Guid &uuidMachineRegistry,
+                    const settings::Medium &data,
+                    const Utf8Str &strMachineFolder);
     HRESULT init(VirtualBox *aVirtualBox,
                  Medium *aParent,
@@ -77 +80 @@
                  const Guid &uuidMachineRegistry,
                  const settings::Medium &data,
-                 const Utf8Str &strMachineFolder);
+                 const Utf8Str &strMachineFolder,
+                 AutoWriteLock &mediaTreeLock);
 
     // initializer for host floppy/DVD
@@ -109 +113 @@
 
     /* handles caller/locking itself */
-    bool i_addRegistry(const Guid& id, bool fRecurse);
+    bool i_addRegistry(const Guid &id);
+    /* handles caller/locking itself, caller is responsible for tree lock */
+    bool i_addRegistryRecursive(const Guid &id);
     /* handles caller/locking itself */
-    bool i_removeRegistry(const Guid& id, bool fRecurse);
+    bool i_removeRegistry(const Guid& id);
+    /* handles caller/locking itself, caller is responsible for tree lock */
+    bool i_removeRegistryRecursive(const Guid& id);
     bool i_isInRegistry(const Guid& id);
     bool i_getFirstRegistryMachineId(Guid &uuid) const;
@@ -135 +143 @@
     HRESULT i_updatePath(const Utf8Str &strOldPath, const Utf8Str &strNewPath);
 
+    /* handles caller/locking itself */
     ComObjPtr<Medium> i_getBase(uint32_t *aLevel = NULL);
+    /* handles caller/locking itself */
+    uint32_t i_getDepth();
 
     bool i_isReadOnly();
     void i_updateId(const Guid &id);
 
+    void i_saveSettingsOne(settings::Medium &data,
+                           const Utf8Str &strHardDiskFolder);
     HRESULT i_saveSettings(settings::Medium &data,
                            const Utf8Str &strHardDiskFolder);

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

@@ -1 +1 @@
 /* $Id$ */
-
 /** @file
- *
  * VirtualBox COM class implementation
  */
 
 /*
- * Copyright (C) 2006-2013 Oracle Corporation
+ * Copyright (C) 2006-2015 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -92 +90 @@
                                 Snapshot *pSnapshotToIgnore);
 
-    HRESULT i_saveSnapshot(settings::Snapshot &data, bool aAttrsOnly);
-    HRESULT i_saveSnapshotImpl(settings::Snapshot &data, bool aAttrsOnly);
+    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,
+                        CleanupMode_T cleanupMode,
+                        MediaList &llMedia,
+                        std::list<Utf8Str> &llFilenames);
     HRESULT i_uninitRecursively(AutoWriteLock &writeLock,
                                 CleanupMode_T cleanupMode,

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

@@ -93 +93 @@
     HRESULT initMachines();
     HRESULT initMedia(const Guid &uuidMachineRegistry,
-                      const settings::MediaRegistry mediaRegistry,
+                      const settings::MediaRegistry &mediaRegistry,
                       const Utf8Str &strMachineFolder);
     void uninit();
@@ -230 +230 @@
     HRESULT i_saveSettings();
     void i_markRegistryModified(const Guid &uuid);
+    void i_unmarkRegistryModified(const Guid &uuid);
     void i_saveModifiedRegistries();
     static const com::Utf8Str &i_getVersionNormalized();

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

@@ -4273 +4273 @@
     }
 
+    /* Save modified registries, but skip this machine as it's the caller's
+     * job to save its settings like all other settings changes. */
+    mParent->i_unmarkRegistryModified(i_getId());
     mParent->i_saveModifiedRegistries();
 
@@ -4353 +4356 @@
     alock.release();
 
+    /* Save modified registries, but skip this machine as it's the caller's
+     * job to save its settings like all other settings changes. */
+    mParent->i_unmarkRegistryModified(i_getId());
     mParent->i_saveModifiedRegistries();
 
@@ -4794 +4800 @@
     multiLock.release();
 
+    /* Save modified registries, but skip this machine as it's the caller's
+     * job to save its settings like all other settings changes. */
+    mParent->i_unmarkRegistryModified(i_getId());
     mParent->i_saveModifiedRegistries();
 
@@ -5118 +5127 @@
     if (cSnapshots)
     {
-        // autoCleanup must be true here, or we would have failed above
-
         // add the media from the medium attachments of the snapshots to llMedia
         // as well, after the "main" machine media; Snapshot::uninitRecursively()
@@ -9278 +9285 @@
             if (puuidRegistry)
                 // caller wants registry ID to be set on all attached media (OVF import case)
-                medium->i_addRegistry(*puuidRegistry, false /* fRecurse */);
+                medium->i_addRegistry(*puuidRegistry);
         }
 
@@ -9963 +9970 @@
         if (mData->mFirstSnapshot)
         {
-            settings::Snapshot snapNew;
-            config.llFirstSnapshot.push_back(snapNew);
-
-            // get reference to the fresh copy of the snapshot on the list and
-            // work on that copy directly to avoid excessive copying later
-            settings::Snapshot &snap = config.llFirstSnapshot.front();
-
-            rc = mData->mFirstSnapshot->i_saveSnapshot(snap, false /*aAttrsOnly*/);
+            // the settings use a list for "the first snapshot"
+            config.llFirstSnapshot.push_back(settings::g_SnapshotEmpty);
+
+            // get reference to the snapshot on the list and work on that
+            // element straight in the list to avoid excessive copying later
+            rc = mData->mFirstSnapshot->i_saveSnapshot(config.llFirstSnapshot.back());
             if (FAILED(rc)) throw rc;
         }
@@ -10473 +10478 @@
         uuid = mParent->i_getGlobalRegistryId(); // VirtualBox global registry UUID
 
-    if (pMedium->i_addRegistry(uuid, false /* fRecurse */))
+    if (pMedium->i_addRegistry(uuid))
         mParent->i_markRegistryModified(uuid);
 
@@ -10480 +10485 @@
     if (pMedium != pBase)
     {
-        if (pBase->i_addRegistry(uuid, true /* fRecurse */))
+        /* Tree lock needed by Medium::addRegistry when recursing. */
+        AutoReadLock treeLock(&mParent->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+        if (pBase->i_addRegistryRecursive(uuid))
+        {
+            treeLock.release();
             mParent->i_markRegistryModified(uuid);
+        }
     }
 }

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

@@ -1342 +1342 @@
                 mlock.acquire();
             }
-            pMedium->i_addRegistry(uuid, false /* fRecurse */);
+            pMedium->i_addRegistry(uuid);
         }
         /* Check if a snapshot folder is necessary and if so doesn't already

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

@@ -1213 +1213 @@
 /**
  * Initializes the medium object by loading its data from the given settings
- * node. In this mode, the medium will always be opened read/write.
+ * node. The medium will always be opened read/write.
  *
  * In this case, since we're loading from a registry, uuidMachineRegistry is
@@ -1219 +1219 @@
  * loading from a per-machine registry.
  *
- * @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 aNode         Configuration settings.
+ * @param data          Configuration settings.
  * @param strMachineFolder The machine folder with which to resolve relative paths;
  *                         if empty, then we use the VirtualBox home directory
@@ -1230 +1229 @@
  * @note Locks the medium tree for writing.
  */
-HRESULT Medium::init(VirtualBox *aVirtualBox,
-                     Medium *aParent,
-                     DeviceType_T aDeviceType,
-                     const Guid &uuidMachineRegistry,
-                     const settings::Medium &data,
-                     const Utf8Str &strMachineFolder)
-{
-    using namespace settings;
-
-    AssertReturn(aVirtualBox, E_INVALIDARG);
-
-    /* Enclose the state transition NotReady->InInit->Ready */
-    AutoInitSpan autoInitSpan(this);
-    AssertReturn(autoInitSpan.isOk(), E_FAIL);
-
-    HRESULT rc = S_OK;
-
-    unconst(m->pVirtualBox) = aVirtualBox;
+HRESULT Medium::initOne(Medium *aParent,
+                        DeviceType_T aDeviceType,
+                        const Guid &uuidMachineRegistry,
+                        const settings::Medium &data,
+                        const Utf8Str &strMachineFolder)
+{
+    HRESULT rc;
 
     if (uuidMachineRegistry.isValid() && !uuidMachineRegistry.isZero())
@@ -1258 +1246 @@
         // differencing medium: add to parent
         AutoWriteLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-        m->pParent = aParent;
-        aParent->m->llChildren.push_back(this);
+        // no need to check maximum depth as settings reading did it
+        i_setParent(aParent);
     }
 
@@ -1369 +1357 @@
                      m->strLocationFull.c_str(), m->strFormat.c_str(), m->id.raw()));
 
+    return S_OK;
+}
+
+/**
+ * Initializes the medium object and its children by loading its data from the
+ * given settings node. The medium will always be opened read/write.
+ *
+ * In this case, since we're loading from a registry, uuidMachineRegistry is
+ * always set: it's either the global registry UUID or a machine UUID when
+ * loading from a per-machine registry.
+ *
+ * @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
+ *
+ * @note Locks the medium tree for writing.
+ */
+HRESULT Medium::init(VirtualBox *aVirtualBox,
+                     Medium *aParent,
+                     DeviceType_T aDeviceType,
+                     const Guid &uuidMachineRegistry,
+                     const settings::Medium &data,
+                     const Utf8Str &strMachineFolder,
+                     AutoWriteLock &mediaTreeLock)
+{
+    using namespace settings;
+
+    Assert(aVirtualBox->i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+    AssertReturn(aVirtualBox, E_INVALIDARG);
+
+    /* Enclose the state transition NotReady->InInit->Ready */
+    AutoInitSpan autoInitSpan(this);
+    AssertReturn(autoInitSpan.isOk(), E_FAIL);
+
+    unconst(m->pVirtualBox) = aVirtualBox;
+
+    // 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).
+    HRESULT rc = initOne(aParent, aDeviceType, uuidMachineRegistry, data, strMachineFolder);
+
+
     /* Don't call Medium::i_queryInfo for registered media to prevent the calling
      * thread (i.e. the VirtualBox server startup thread) from an unexpected
@@ -1390 +1423 @@
                            uuidMachineRegistry,
                            med,               // child data
-                           strMachineFolder);
+                           strMachineFolder,
+                           mediaTreeLock);
         if (FAILED(rc)) break;
 
-        AutoWriteLock treeLock(aVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-
-        rc = m->pVirtualBox->i_registerMedium(pMedium, &pMedium, treeLock);
+        rc = m->pVirtualBox->i_registerMedium(pMedium, &pMedium, mediaTreeLock);
         if (FAILED(rc)) break;
     }
@@ -3073 +3105 @@
                             m->strLocationFull.c_str(), m->backRefs.size());
 
-        if (m->llChildren.size() != 0)
+        if (i_getChildren().size() != 0)
             return setError(VBOX_E_INVALID_OBJECT_STATE,
                             tr("Cannot encrypt medium '%s' because it has %d children"),
-                            m->strLocationFull.c_str(), m->llChildren.size());
+                            m->strLocationFull.c_str(), i_getChildren().size());
 
         /* Build the medium lock list. */
@@ -3127 +3159 @@
                 break;
             }
-            else if (pMedium->m->llChildren.size() > 1)
+            else if (pMedium->i_getChildren().size() > 1)
             {
                 rc = setError(VBOX_E_INVALID_OBJECT_STATE,
                               tr("Cannot encrypt medium '%s' because it has %d children"),
-                              pMedium->m->strLocationFull.c_str(), pMedium->m->llChildren.size());
+                              pMedium->m->strLocationFull.c_str(), pMedium->i_getChildren().size());
                 break;
             }
@@ -3473 +3505 @@
  * See getFirstRegistryMachineId() for details.
  *
- * If fRecurse == true, then the media tree lock must be held for reading.
- *
  * @param id
- * @param fRecurse If true, recurses into child media to make sure the whole tree has registries in sync.
  * @return true if the registry was added; false if the given id was already on the list.
  */
-bool Medium::i_addRegistry(const Guid& id, bool fRecurse)
+bool Medium::i_addRegistry(const Guid& id)
 {
     AutoCaller autoCaller(this);
@@ -3511 +3540 @@
         m->llRegistryIDs.push_back(id);
 
-    if (fRecurse)
-    {
-        // Get private list of children and release medium lock straight away.
-        MediaList llChildren(m->llChildren);
-        alock.release();
-
-        for (MediaList::iterator it = llChildren.begin();
-             it != llChildren.end();
-             ++it)
-        {
-            Medium *pChild = *it;
-            fAdd |= pChild->i_addRegistry(id, true);
-        }
-    }
-
     return fAdd;
 }
 
 /**
- * Removes the given UUID from the list of media registry UUIDs. Returns true
- * if found or false if not.
- *
- * If fRecurse == true, then the media tree lock must be held for reading.
+ * This adds the given UUID to the list of media registries in which this
+ * 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.
+ *
+ * Note that for hard disks, this method does nothing if the medium is
+ * already in another registry to avoid having hard disks in more than
+ * one registry, which causes trouble with keeping diff images in sync.
+ * See getFirstRegistryMachineId() for details.
+ *
+ * @note the caller must hold the media tree lock for reading.
  *
  * @param id
- * @param fRecurse If true, recurses into child media to make sure the whole tree has registries in sync.
- * @return
- */
-bool Medium::i_removeRegistry(const Guid& id, bool fRecurse)
+ * @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_addRegistry(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);
+    }
+
+    return fAdd;
+}
+
+/**
+ * Removes the given UUID from the list of media registry UUIDs of this medium.
+ *
+ * @param id
+ * @return true if the UUID was found or false if not.
+ */
+bool Medium::i_removeRegistry(const Guid &id)
 {
     AutoCaller autoCaller(this);
@@ -3548 +3594 @@
     bool fRemove = false;
 
+    // @todo r=klaus eliminate this code, replace it by using find.
     for (GuidList::iterator it = m->llRegistryIDs.begin();
          it != m->llRegistryIDs.end();
@@ -3554 +3601 @@
         if ((*it) == id)
         {
+            // getting away with this as the iterator isn't used after
             m->llRegistryIDs.erase(it);
             fRemove = true;
@@ -3560 +3608 @@
     }
 
-    if (fRecurse)
-    {
-        // Get private list of children and release medium lock straight away.
-        MediaList llChildren(m->llChildren);
-        alock.release();
-
-        for (MediaList::iterator it = llChildren.begin();
-             it != llChildren.end();
-             ++it)
-        {
-            Medium *pChild = *it;
-            fRemove |= pChild->i_removeRegistry(id, true);
-        }
+    return fRemove;
+}
+
+/**
+ * Removes the given UUID from the list of media registry UUIDs, for this
+ * medium and all its children recursively.
+ *
+ * @note the caller must hold the media tree lock for reading.
+ *
+ * @param id
+ * @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);
     }
 
@@ -3586 +3648 @@
  * @return
  */
-bool Medium::i_isInRegistry(const Guid& id)
-{
+bool Medium::i_isInRegistry(const Guid &id)
+{
+    // @todo r=klaus eliminate this code, replace it by using find.
     for (GuidList::const_iterator it = m->llRegistryIDs.begin();
          it != m->llRegistryIDs.end();
@@ -3832 +3895 @@
         return &m->backRefs.front().machineId;
 
-    for (MediaList::iterator it = m->llChildren.begin();
-         it != m->llChildren.end();
+    for (MediaList::const_iterator it = i_getChildren().begin();
+         it != i_getChildren().end();
          ++it)
     {
@@ -3981 +4044 @@
 
     return pBase;
+}
+
+/**
+ * Returns the depth of this medium in the media chain.
+ *
+ * @note Locks medium tree for reading.
+ */
+uint32_t Medium::i_getDepth()
+{
+    /* it is possible that some previous/concurrent uninit has already cleared
+     * the pVirtualBox reference, and in this case we don't need to continue */
+    ComObjPtr<VirtualBox> pVirtualBox(m->pVirtualBox);
+    if (!pVirtualBox)
+        return 1;
+
+    /* we access mParent */
+    AutoReadLock treeLock(pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+
+    uint32_t cDepth = 0;
+    ComObjPtr<Medium> pMedium(this);
+    while (!pMedium.isNull())
+    {
+        AutoCaller autoCaller(this);
+        AssertReturn(autoCaller.isOk(), cDepth + 1);
+
+        pMedium = pMedium->m->pParent;
+        cDepth++;
+    }
+
+    return cDepth;
 }
 
@@ -4041 +4134 @@
 
 /**
- * Saves medium data by appending a new child node to the given
- * parent XML settings node.
+ * Saves the settings of one medium.
+ *
+ * @note Caller MUST take care of the medium tree lock and caller.
  *
  * @param data      Settings struct to be updated.
  * @param strHardDiskFolder Folder for which paths should be relative.
- *
- * @note Locks this object, medium tree and children for reading.
- */
-HRESULT Medium::i_saveSettings(settings::Medium &data,
-                               const Utf8Str &strHardDiskFolder)
-{
-    AutoCaller autoCaller(this);
-    if (FAILED(autoCaller.rc())) return autoCaller.rc();
-
-    /* we access mParent */
-    AutoReadLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-
+ */
+void Medium::i_saveSettingsOne(settings::Medium &data, const Utf8Str &strHardDiskFolder)
+{
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
 
@@ -4117 +4202 @@
     if (m->pParent.isNull())
         data.hdType = m->type;
+}
+
+/**
+ * Saves medium data by putting it into the provided data structure.
+ * Recurses over all children to save their settings, too.
+ *
+ * @param data      Settings struct to be updated.
+ * @param strHardDiskFolder Folder for which paths should be relative.
+ *
+ * @note Locks this object, medium tree and children for reading.
+ */
+HRESULT Medium::i_saveSettings(settings::Medium &data,
+                               const Utf8Str &strHardDiskFolder)
+{
+    AutoCaller autoCaller(this);
+    if (FAILED(autoCaller.rc())) return autoCaller.rc();
+
+    /* we access mParent */
+    AutoReadLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+
+    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)
     {
-        settings::Medium med;
-        HRESULT rc = (*it)->i_saveSettings(med, strHardDiskFolder);
-        AssertComRCReturnRC(rc);
-        data.llChildren.push_back(med);
+        // 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::g_MediumEmpty);
+        HRESULT rc = (*it)->i_saveSettings(llSettingsChildren.back(), strHardDiskFolder);
+        if (FAILED(rc))
+        {
+            llSettingsChildren.pop_back();
+            return rc;
+        }
     }
 
@@ -6120 +6234 @@
                     if (m->pParent)
                         i_deparent();
+
+                    if (!pParent.isNull())
+                        if (pParent->i_getDepth() >= SETTINGS_MEDIUM_DEPTH_MAX)
+                        {
+                            AutoReadLock plock(pParent COMMA_LOCKVAL_SRC_POS);
+                            throw setError(VBOX_E_INVALID_OBJECT_STATE,
+                                           tr("Cannot open differencing image for medium '%s', because it exceeds the medium tree depth limit. Please merge some images which you no longer need"),
+                                           pParent->m->strLocationFull.c_str());
+                        }
                     i_setParent(pParent);
 
@@ -6353 +6476 @@
         {
             // re-associate with the parent as we are still relatives in the registry
-            m->pParent = pParentBackup;
-            m->pParent->m->llChildren.push_back(this);
+            i_setParent(pParentBackup);
         }
     }
@@ -7346 +7468 @@
     try
     {
+        if (i_getDepth() >= SETTINGS_MEDIUM_DEPTH_MAX)
+        {
+            AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
+            throw setError(VBOX_E_INVALID_OBJECT_STATE,
+                           tr("Cannot create differencing image for medium '%s', because it exceeds the medium tree depth limit. Please merge some images which you no longer need"),
+                           m->strLocationFull.c_str());
+        }
+
         /* Lock both in {parent,child} order. */
         AutoMultiWriteLock2 mediaLock(this, pTarget COMMA_LOCKVAL_SRC_POS);
@@ -7467 +7597 @@
         Assert(pTarget->m->pParent.isNull());
 
-        /* associate the child with the parent */
-        pTarget->m->pParent = this;
-        m->llChildren.push_back(pTarget);
-
-        /** @todo r=klaus neither target nor base() are locked,
-            * potential race! */
+        /* associate child with the parent, maximum depth was checked above */
+        pTarget->i_setParent(this);
+
         /* diffs for immutable media are auto-reset by default */
-        pTarget->m->autoReset = (i_getBase()->m->type == MediumType_Immutable);
+        bool fAutoReset;
+        {
+            ComObjPtr<Medium> pBase = i_getBase();
+            AutoReadLock block(pBase COMMA_LOCKVAL_SRC_POS);
+            fAutoReset = (pBase->m->type == MediumType_Immutable);
+        }
+        {
+            AutoWriteLock tlock(pTarget COMMA_LOCKVAL_SRC_POS);
+            pTarget->m->autoReset = fAutoReset;
+        }
 
         /* register with mVirtualBox as the last step and move to
@@ -7548 +7684 @@
     try
     {
+        if (!task.mParentForTarget.isNull())
+            if (task.mParentForTarget->i_getDepth() >= SETTINGS_MEDIUM_DEPTH_MAX)
+            {
+                AutoReadLock plock(task.mParentForTarget COMMA_LOCKVAL_SRC_POS);
+                throw setError(VBOX_E_INVALID_OBJECT_STATE,
+                               tr("Cannot merge image for medium '%s', because it exceeds the medium tree depth limit. Please merge some images which you no longer need"),
+                               task.mParentForTarget->m->strLocationFull.c_str());
+            }
+
         PVBOXHDD hdd;
         int vrc = VDCreate(m->vdDiskIfaces, i_convertDeviceType(), &hdd);
@@ -7702 +7847 @@
             AssertComRC(rc2);
 
-            /* then, reparent it and disconnect the deleted branch at
-             * both ends (chain->parent() is source's parent) */
+            /* then, reparent it and disconnect the deleted branch at both ends
+             * (chain->parent() is source's parent). Depth check above. */
             pTarget->i_deparent();
-            pTarget->m->pParent = task.mParentForTarget;
-            if (pTarget->m->pParent)
-            {
-                pTarget->m->pParent->m->llChildren.push_back(pTarget);
+            pTarget->i_setParent(task.mParentForTarget);
+            if (task.mParentForTarget)
                 i_deparent();
-            }
 
             /* then, register again */
@@ -7743 +7885 @@
 
                     pMedium->i_deparent();  // removes pMedium from source
+                    // no depth check, reduces depth
                     pMedium->i_setParent(pTarget);
                 }
@@ -7859 +8002 @@
     try
     {
+        if (!pParent.isNull())
+            if (pParent->i_getDepth() >= SETTINGS_MEDIUM_DEPTH_MAX)
+            {
+                AutoReadLock plock(pParent COMMA_LOCKVAL_SRC_POS);
+                throw setError(VBOX_E_INVALID_OBJECT_STATE,
+                               tr("Cannot clone image for medium '%s', because it exceeds the medium tree depth limit. Please merge some images which you no longer need"),
+                               pParent->m->strLocationFull.c_str());
+            }
+
         /* Lock all in {parent,child} order. The lock is also used as a
          * signal from the task initiator (which releases it only after
@@ -7983 +8135 @@
                 }
 
-                /** @todo r=klaus target isn't locked, race getting the state */
+                /* target isn't locked, but no changing data is accessed */
                 if (task.midxSrcImageSame == UINT32_MAX)
                 {
@@ -8053 +8205 @@
         if (pParent)
         {
-            /* associate the clone with the parent and deassociate
-             * from VirtualBox */
-            pTarget->m->pParent = pParent;
-            pParent->m->llChildren.push_back(pTarget);
+            /* Associate the clone with the parent and deassociate
+             * from VirtualBox. Depth check above. */
+            pTarget->i_setParent(pParent);
 
             /* register with mVirtualBox as the last step and move to
@@ -8692 +8843 @@
     try
     {
+        if (!pParent.isNull())
+            if (pParent->i_getDepth() >= SETTINGS_MEDIUM_DEPTH_MAX)
+            {
+                AutoReadLock plock(pParent COMMA_LOCKVAL_SRC_POS);
+                throw setError(VBOX_E_INVALID_OBJECT_STATE,
+                               tr("Cannot import image for medium '%s', because it exceeds the medium tree depth limit. Please merge some images which you no longer need"),
+                               pParent->m->strLocationFull.c_str());
+            }
+
         /* Lock all in {parent,child} order. The lock is also used as a
          * signal from the task initiator (which releases it only after
@@ -8799 +8959 @@
                 }
 
-                /** @todo r=klaus target isn't locked, race getting the state */
                 vrc = VDCopy(hdd,
                              VD_LAST_IMAGE,
@@ -8848 +9007 @@
         if (pParent)
         {
-            /* associate the imported medium with the parent and deassociate
-             * from VirtualBox */
-            m->pParent = pParent;
-            pParent->m->llChildren.push_back(this);
+            /* Associate the imported medium with the parent and deassociate
+             * from VirtualBox. Depth check above. */
+            i_setParent(pParent);
 
             /* register with mVirtualBox as the last step and move to

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

@@ -1 +1 @@
 /* $Id$ */
 /** @file
- *
  * COM class implementation for Snapshot and SnapshotMachine in VBoxSVC.
  */
@@ -175 +174 @@
     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();
@@ -186 +191 @@
     delete m;
     m = NULL;
+
+    autoUninitSpan.setSucceeded();
+    // see above, now the refcount may reach 0
+    pSnapshot.setNull();
 }
 
@@ -725 +734 @@
 
 /**
- * Internal implementation for Snapshot::saveSnapshot (below). Caller has
- * requested the snapshots tree (machine) lock.
- *
- * @param aNode
- * @param aAttrsOnly
+ * Saves the settings attributes of one snapshot.
+ *
+ * @param data      Target for saving snapshot settings.
  * @return
  */
-HRESULT Snapshot::i_saveSnapshotImpl(settings::Snapshot &data, bool aAttrsOnly)
+HRESULT Snapshot::i_saveSnapshotImplOne(settings::Snapshot &data) const
 {
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
@@ -740 +747 @@
     data.timestamp = m->timeStamp;
     data.strDescription = m->strDescription;
-
-    if (aAttrsOnly)
-        return S_OK;
 
     // state file (only if this snapshot is online)
@@ -756 +760 @@
     if (FAILED(rc)) return rc;
 
-    alock.release();
-
-    data.llChildSnapshots.clear();
-
-    if (m->llChildren.size())
-    {
-        for (SnapshotsList::const_iterator it = m->llChildren.begin();
-             it != m->llChildren.end();
-             ++it)
+    return S_OK;
+}
+
+/**
+ * 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::g_SnapshotEmpty);
+        Snapshot *pSnap = *it;
+        rc = pSnap->i_saveSnapshotImpl(llSettingsChildren.back());
+        if (FAILED(rc))
         {
-           // Use the heap to reduce the stack footprint. Each recursion needs
-           // over 1K, and there can be VMs with deeply nested snapshots. The
-           // stack can be quite small, especially with XPCOM.
-
-            settings::Snapshot *snap = new settings::Snapshot();
-            rc = (*it)->i_saveSnapshotImpl(*snap, aAttrsOnly);
-            if (FAILED(rc))
-            {
-                delete snap;
-                return rc;
-            }
-            data.llChildSnapshots.push_back(*snap);
-            delete snap;
+            llSettingsChildren.pop_back();
+            return rc;
         }
     }
@@ -786 +799 @@
 
 /**
- *  Saves the given snapshot and all its children (unless \a aAttrsOnly is true).
- *  It is assumed that the given node is empty (unless \a aAttrsOnly is true).
- *
- *  @param aNode        <Snapshot> node to save the snapshot to.
- *  @param aSnapshot    Snapshot to save.
- *  @param aAttrsOnly   If true, only update user-changeable attrs.
- */
-HRESULT Snapshot::i_saveSnapshot(settings::Snapshot &data, bool aAttrsOnly)
+ * Saves the given snapshot and all its children.
+ * It is assumed that the given node is empty.
+ *
+ * @param data      Target for saving snapshot settings.
+ */
+HRESULT Snapshot::i_saveSnapshot(settings::Snapshot &data) const
 {
     // snapshots tree is protected by machine lock
     AutoReadLock alock(m->pMachine COMMA_LOCKVAL_SRC_POS);
 
-    return i_saveSnapshotImpl(data, aAttrsOnly);
+    return i_saveSnapshotImpl(data);
 }
 
@@ -804 +815 @@
  * 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).
+ * 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.
  *
  * Caller must hold the machine write lock (which protects the snapshots tree!)
@@ -824 +827 @@
  * @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
-
-    // make a copy of the children list since uninit() modifies it
-    SnapshotsList llChildrenCopy(m->llChildren);
-    for (SnapshotsList::iterator it = llChildrenCopy.begin();
-         it != llChildrenCopy.end();
-         ++it)
-    {
-        Snapshot *pChild = *it;
-        rc = pChild->i_uninitRecursively(writeLock, cleanupMode, llMedia, llFilenames);
-        if (FAILED(rc))
-            return rc;
-    }
-
+HRESULT Snapshot::i_uninitOne(AutoWriteLock &writeLock,
+                              CleanupMode_T cleanupMode,
+                              MediaList &llMedia,
+                              std::list<Utf8Str> &llFilenames)
+{
     // now call detachAllMedia on the snapshot machine
-    rc = m->pMachine->i_detachAllMedia(writeLock,
-                                       this /* pSnapshot */,
-                                       cleanupMode,
-                                       llMedia);
+    HRESULT rc = m->pMachine->i_detachAllMedia(writeLock,
+                                               this /* pSnapshot */,
+                                               cleanupMode,
+                                               llMedia);
     if (FAILED(rc))
         return rc;
@@ -884 +859 @@
     }
 
-    this->i_beginSnapshotDelete();
-    this->uninit();
-
+    i_beginSnapshotDelete();
+    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
-    LogFlowThisFunc(("Leaving for snapshot '%s' {%RTuuid}\n", name.c_str(), uuid.raw()));
+    Guid uuid = i_getId();
+    Utf8Str name = i_getName();
+    LogFlowThisFunc(("Entering for snapshot '%s' {%RTuuid}\n", name.c_str(), uuid.raw()));
 #endif
 
-    return S_OK;
+    // 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;
 }
 

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

@@ -650 +650 @@
  */
 HRESULT VirtualBox::initMedia(const Guid &uuidRegistry,
-                              const settings::MediaRegistry mediaRegistry,
+                              const settings::MediaRegistry &mediaRegistry,
                               const Utf8Str &strMachineFolder)
 {
@@ -674 +674 @@
                                  uuidRegistry,
                                  xmlHD,         // XML data; this recurses to processes the children
-                                 strMachineFolder);
+                                 strMachineFolder,
+                                 treeLock);
         if (FAILED(rc)) return rc;
 
@@ -694 +695 @@
                               uuidRegistry,
                               xmlDvd,
-                              strMachineFolder);
+                              strMachineFolder,
+                              treeLock);
         if (FAILED(rc)) return rc;
 
@@ -714 +716 @@
                               uuidRegistry,
                               xmlFloppy,
-                              strMachineFolder);
+                              strMachineFolder,
+                              treeLock);
         if (FAILED(rc)) return rc;
 
@@ -4014 +4017 @@
             if (pMedium->i_isInRegistry(uuidRegistry))
             {
-                settings::Medium med;
-                rc = pMedium->i_saveSettings(med, strMachineFolder);     // this recurses into child hard disks
-                if (FAILED(rc)) throw rc;
-                llTarget.push_back(med);
+                llTarget.push_back(settings::g_MediumEmpty);
+                rc = pMedium->i_saveSettings(llTarget.back(), strMachineFolder);     // this recurses into child hard disks
+                if (FAILED(rc))
+                {
+                    llTarget.pop_back();
+                    throw rc;
+                }
             }
         }
@@ -4482 +4488 @@
             AutoWriteLock mlock(pMedium COMMA_LOCKVAL_SRC_POS);
 
-            if (pMedium->i_removeRegistry(id, true /* fRecurse */))
+            if (pMedium->i_removeRegistryRecursive(id))
             {
                 // machine ID was found in base medium's registry list:
@@ -4491 +4497 @@
                 {
                     // 2) better registry found: then use that
-                    pMedium->i_addRegistry(*puuidBetter, true /* fRecurse */);
+                    pMedium->i_addRegistryRecursive(*puuidBetter);
                     // 3) and make sure the registry is saved below
                     mlock.release();
@@ -4497 +4503 @@
                     i_markRegistryModified(*puuidBetter);
                     tlock.acquire();
-                    mlock.release();
+                    mlock.acquire();
                 }
             }
@@ -4533 +4539 @@
             if (SUCCEEDED(machineCaller.rc()))
                 ASMAtomicIncU64(&pMachine->uRegistryNeedsSaving);
+        }
+    }
+}
+
+/**
+ * Marks the registry for @a uuid as unmodified, so that it's not saved in
+ * a later call to saveModifiedRegistries().
+ *
+ * @param uuid
+ */
+void VirtualBox::i_unmarkRegistryModified(const Guid &uuid)
+{
+    uint64_t uOld;
+    if (uuid == i_getGlobalRegistryId())
+    {
+        for (;;)
+        {
+            uOld = ASMAtomicReadU64(&m->uRegistryNeedsSaving);
+            if (!uOld)
+                break;
+            if (ASMAtomicCmpXchgU64(&m->uRegistryNeedsSaving, 0, uOld))
+                break;
+            ASMNopPause();
+        }
+    }
+    else
+    {
+        ComObjPtr<Machine> pMachine;
+        HRESULT rc = i_findMachine(uuid,
+                                   false /* fPermitInaccessible */,
+                                   false /* aSetError */,
+                                   &pMachine);
+        if (SUCCEEDED(rc))
+        {
+            AutoCaller machineCaller(pMachine);
+            if (SUCCEEDED(machineCaller.rc()))
+            {
+                for (;;)
+                {
+                    uOld = ASMAtomicReadU64(&pMachine->uRegistryNeedsSaving);
+                    if (!uOld)
+                        break;
+                    if (ASMAtomicCmpXchgU64(&pMachine->uRegistryNeedsSaving, 0, uOld))
+                        break;
+                    ASMNopPause();
+                }
+            }
         }
     }

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

@@ -55 +55 @@
 
 /*
- * Copyright (C) 2007-2014 Oracle Corporation
+ * Copyright (C) 2007-2015 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -120 +120 @@
 /** VirtualBox XML settings full version string ("x.y-platform") */
 #define VBOX_XML_VERSION_FULL   VBOX_XML_VERSION "-" VBOX_XML_PLATFORM
+
+const struct Snapshot settings::g_SnapshotEmpty; /* default ctor is OK */
+const struct Medium settings::g_MediumEmpty; /* default ctor is OK */
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -377 +380 @@
 
 /**
+ * Helper function to convert a MediaType enum value into string from.
+ * @param t
+ */
+/*static*/
+const char *ConfigFileBase::stringifyMediaType(MediaType t)
+{
+    switch (t)
+    {
+        case HardDisk:
+            return "hard disk";
+        case DVDImage:
+            return "DVD";
+        case FloppyImage:
+            return "floppy";
+        default:
+            AssertMsgFailed(("media type %d\n", t));
+            return "UNKNOWN";
+    }
+}
+
+/**
  * Helper function that parses a UUID in string form into
  * a com::Guid item. Accepts UUIDs both with and without
@@ -466 +490 @@
  * @return
  */
-com::Utf8Str ConfigFileBase::makeString(const RTTIMESPEC &stamp)
+com::Utf8Str ConfigFileBase::stringifyTimestamp(const RTTIMESPEC &stamp) const
 {
     RTTIME time;
@@ -566 +590 @@
  * @param t
  * @param elmMedium
- * @param llMedia
- */
-void ConfigFileBase::readMedium(MediaType t,
-                                const xml::ElementNode &elmMedium,  // HardDisk node if root; if recursing,
-                                                                    // child HardDisk node or DiffHardDisk node for pre-1.4
-                                MediaList &llMedia)     // list to append medium to (root disk or child list)
+ * @param med
+ */
+void ConfigFileBase::readMediumOne(MediaType t,
+                                   const xml::ElementNode &elmMedium,
+                                   Medium &med)
 {
     // <HardDisk uuid="{5471ecdb-1ddb-4012-a801-6d98e226868b}" location="/mnt/innotek-unix/vdis/Windows XP.vdi" format="VDI" type="Normal">
-    settings::Medium med;
+
     Utf8Str strUUID;
     if (!elmMedium.getAttributeValue("uuid", strUUID))
@@ -726 +749 @@
     elmMedium.getAttributeValue("Description", med.strDescription);       // optional
 
-    // recurse to handle children
-    xml::NodesLoop nl2(elmMedium);
+    // handle medium properties
+    xml::NodesLoop nl2(elmMedium, "Property");
     const xml::ElementNode *pelmHDChild;
     while ((pelmHDChild = nl2.forAllNodes()))
     {
-        if (    t == HardDisk
-             && (    pelmHDChild->nameEquals("HardDisk")
-                  || (    (m->sv < SettingsVersion_v1_4)
-                       && (pelmHDChild->nameEquals("DiffHardDisk"))
-                     )
-                )
-           )
-            // recurse with this element and push the child onto our current children list
-            readMedium(t,
-                        *pelmHDChild,
-                        med.llChildren);
-        else if (pelmHDChild->nameEquals("Property"))
-        {
-            Utf8Str strPropName, strPropValue;
-            if (   pelmHDChild->getAttributeValue("name", strPropName)
-                && pelmHDChild->getAttributeValue("value", strPropValue) )
-                med.properties[strPropName] = strPropValue;
-            else
-                throw ConfigFileError(this, pelmHDChild, N_("Required HardDisk/Property/@name or @value attribute is missing"));
-        }
-    }
-
-    llMedia.push_back(med);
+        Utf8Str strPropName, strPropValue;
+        if (   pelmHDChild->getAttributeValue("name", strPropName)
+            && pelmHDChild->getAttributeValue("value", strPropValue) )
+            med.properties[strPropName] = strPropValue;
+        else
+            throw ConfigFileError(this, pelmHDChild, N_("Required HardDisk/Property/@name or @value attribute is missing"));
+    }
+}
+
+/**
+ * Reads a media registry entry from the main VirtualBox.xml file and recurses
+ * into 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(g_MediumEmpty);
+        readMedium(t,
+                   depth + 1,
+                   *pelmHDChild,
+                   llSettingsChildren.back());
+    }
 }
 
@@ -788 +838 @@
         while ((pelmMedium = nl2.forAllNodes()))
         {
-            if (    t == HardDisk
-                 && (pelmMedium->nameEquals("HardDisk"))
-               )
-                readMedium(t,
-                           *pelmMedium,
-                           mr.llHardDisks);      // list to append hard disk data to: the root list
-            else if (    t == DVDImage
-                      && (pelmMedium->nameEquals("Image"))
-                    )
-                readMedium(t,
-                           *pelmMedium,
-                           mr.llDvdImages);      // list to append dvd images to: the root list
-            else if (    t == FloppyImage
-                      && (pelmMedium->nameEquals("Image"))
-                    )
-                readMedium(t,
-                           *pelmMedium,
-                           mr.llFloppyImages);      // list to append floppy images to: the root list
+            if (   t == HardDisk
+                && (pelmMedium->nameEquals("HardDisk")))
+            {
+                mr.llHardDisks.push_back(g_MediumEmpty);
+                readMedium(t, 1, *pelmMedium, mr.llHardDisks.back());
+            }
+            else if (   t == DVDImage
+                     && (pelmMedium->nameEquals("Image")))
+            {
+                mr.llDvdImages.push_back(g_MediumEmpty);
+                readMedium(t, 1, *pelmMedium, mr.llDvdImages.back());
+            }
+            else if (   t == FloppyImage
+                     && (pelmMedium->nameEquals("Image")))
+            {
+                mr.llFloppyImages.push_back(g_MediumEmpty);
+                readMedium(t, 1, *pelmMedium, mr.llFloppyImages.back());
+            }
         }
     }
@@ -1086 +1136 @@
  * MainConfigFile::write().
  *
+ * @param t
+ * @param depth
  * @param elmMedium
- * @param m
- * @param level
- */
-void ConfigFileBase::buildMedium(xml::ElementNode &elmMedium,
-                                 DeviceType_T devType,
-                                 const Medium &mdm,
-                                 uint32_t level)          // 0 for "root" call, incremented with each recursion
-{
+ * @param mdm
+ */
+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 (devType == DeviceType_HardDisk)
+    if (t == HardDisk)
         pelmMedium = elmMedium.createChild("HardDisk");
     else
@@ -1106 +1160 @@
     pelmMedium->setAttributePath("location", mdm.strLocation);
 
-    if (devType == DeviceType_HardDisk || RTStrICmp(mdm.strFormat.c_str(), "RAW"))
+    if (t == HardDisk || RTStrICmp(mdm.strFormat.c_str(), "RAW"))
         pelmMedium->setAttribute("format", mdm.strFormat);
-    if (   devType == DeviceType_HardDisk
+    if (   t == HardDisk
         && mdm.fAutoReset)
         pelmMedium->setAttribute("autoReset", mdm.fAutoReset);
@@ -1124 +1178 @@
 
     // only for base hard disks, save the type
-    if (level == 0)
+    if (depth == 1)
     {
         // no need to save the usual DVD/floppy medium types
-        if (   (   devType != DeviceType_DVD
+        if (   (   t != DVDImage
                 || (   mdm.hdType != MediumType_Writethrough // shouldn't happen
                     && mdm.hdType != MediumType_Readonly))
-            && (   devType != DeviceType_Floppy
+            && (   t != FloppyImage
                 || mdm.hdType != MediumType_Writethrough))
         {
@@ -1150 +1204 @@
     {
         // recurse for children
-        buildMedium(*pelmMedium, // parent
-                    devType,     // device type
-                    *it,         // settings::Medium
-                    ++level);    // recursion level
+        buildMedium(t,              // device type
+                    depth + 1,      // depth
+                    *pelmMedium,    // parent
+                    *it);           // settings::Medium
     }
 }
@@ -1178 +1232 @@
          ++it)
     {
-        buildMedium(*pelmHardDisks, DeviceType_HardDisk, *it, 0);
+        buildMedium(HardDisk, 1, *pelmHardDisks, *it);
     }
 
@@ -1186 +1240 @@
          ++it)
     {
-        buildMedium(*pelmDVDImages, DeviceType_DVD, *it, 0);
+        buildMedium(DVDImage, 1, *pelmDVDImages, *it);
     }
 
@@ -1194 +1248 @@
          ++it)
     {
-        buildMedium(*pelmFloppyImages, DeviceType_Floppy, *it, 0);
+        buildMedium(FloppyImage, 1, *pelmFloppyImages, *it);
     }
 }
@@ -3767 +3821 @@
 {
     if (depth > SETTINGS_SNAPSHOT_DEPTH_MAX)
-        throw ConfigFileError(this, &elmSnapshot, N_("Maximum snapshot tree depth of %u exceeded"), depth);
+        throw ConfigFileError(this, &elmSnapshot, N_("Maximum snapshot tree depth of %u exceeded"), SETTINGS_SNAPSHOT_DEPTH_MAX);
 
     Utf8Str strTemp;
@@ -3814 +3868 @@
                 if (pelmChildSnapshot->nameEquals("Snapshot"))
                 {
-                    // Use the heap to reduce the stack footprint. Each
-                    // recursion needs over 1K, and there can be VMs with
-                    // deeply nested snapshots. The stack can be quite
-                    // small, especially with XPCOM.
-                    Snapshot *child = new Snapshot();
-                    bool found = readSnapshot(curSnapshotUuid, depth + 1, *pelmChildSnapshot, *child);
+                    // 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(g_SnapshotEmpty);
+                    bool found = readSnapshot(curSnapshotUuid, depth + 1, *pelmChildSnapshot, snap.llChildSnapshots.back());
                     foundCurrentSnapshot = foundCurrentSnapshot || found;
-                    snap.llChildSnapshots.push_back(*child);
-                    delete child;
                 }
             }
@@ -5149 +5200 @@
     pelmSnapshot->setAttribute("uuid", snap.uuid.toStringCurly());
     pelmSnapshot->setAttribute("name", snap.strName);
-    pelmSnapshot->setAttribute("timeStamp", makeString(snap.timestamp));
+    pelmSnapshot->setAttribute("timeStamp", stringifyTimestamp(snap.timestamp));
 
     if (snap.strStateFile.length())
@@ -5256 +5307 @@
     if (!fCurrentStateModified)
         elmMachine.setAttribute("currentStateModified", fCurrentStateModified);
-    elmMachine.setAttribute("lastStateChange", makeString(timeLastStateChange));
+    elmMachine.setAttribute("lastStateChange", stringifyTimestamp(timeLastStateChange));
     if (fAborted)
         elmMachine.setAttribute("aborted", fAborted);