Index: /trunk/include/VBox/settings.h =================================================================== --- /trunk/include/VBox/settings.h (revision 54947) +++ /trunk/include/VBox/settings.h (revision 54948) @@ -18,5 +18,5 @@ /* - * Copyright (C) 2007-2014 Oracle Corporation + * Copyright (C) 2007-2015 Oracle Corporation * * This file is part of VirtualBox Open Source Edition (OSE), as @@ -52,8 +52,18 @@ /** + * 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,4 +154,6 @@ }; +extern const struct Medium g_MediumEmpty; + /** * A media registry. Starting with VirtualBox 3.3, this can appear in both the @@ -229,10 +241,12 @@ ~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 ×tamp, 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,6 +254,6 @@ 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,8 +267,8 @@ 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,4 +1165,9 @@ struct Snapshot { + Snapshot() + { + RTTimeSpecSetNano(×tamp, 0); + } + bool operator==(const Snapshot &s) const; @@ -1225,4 +1244,6 @@ com::Utf8Str ovIcon; }; + +extern const struct Snapshot g_SnapshotEmpty; /** Index: /trunk/src/VBox/Main/include/MediumImpl.h =================================================================== --- /trunk/src/VBox/Main/include/MediumImpl.h (revision 54947) +++ /trunk/src/VBox/Main/include/MediumImpl.h (revision 54948) @@ -1,6 +1,4 @@ /* $Id$ */ - /** @file - * * VirtualBox COM class implementation */ @@ -72,4 +70,9 @@ // 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,5 +80,6 @@ const Guid &uuidMachineRegistry, const settings::Medium &data, - const Utf8Str &strMachineFolder); + const Utf8Str &strMachineFolder, + AutoWriteLock &mediaTreeLock); // initializer for host floppy/DVD @@ -109,7 +113,11 @@ /* 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,9 +143,14 @@ HRESULT i_updatePath(const Utf8Str &strOldPath, const Utf8Str &strNewPath); + /* handles caller/locking itself */ ComObjPtr 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); Index: /trunk/src/VBox/Main/include/SnapshotImpl.h =================================================================== --- /trunk/src/VBox/Main/include/SnapshotImpl.h (revision 54947) +++ /trunk/src/VBox/Main/include/SnapshotImpl.h (revision 54948) @@ -1,11 +1,9 @@ /* $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,7 +90,12 @@ 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 &llFilenames); HRESULT i_uninitRecursively(AutoWriteLock &writeLock, CleanupMode_T cleanupMode, Index: /trunk/src/VBox/Main/include/VirtualBoxImpl.h =================================================================== --- /trunk/src/VBox/Main/include/VirtualBoxImpl.h (revision 54947) +++ /trunk/src/VBox/Main/include/VirtualBoxImpl.h (revision 54948) @@ -93,5 +93,5 @@ HRESULT initMachines(); HRESULT initMedia(const Guid &uuidMachineRegistry, - const settings::MediaRegistry mediaRegistry, + const settings::MediaRegistry &mediaRegistry, const Utf8Str &strMachineFolder); void uninit(); @@ -230,4 +230,5 @@ HRESULT i_saveSettings(); void i_markRegistryModified(const Guid &uuid); + void i_unmarkRegistryModified(const Guid &uuid); void i_saveModifiedRegistries(); static const com::Utf8Str &i_getVersionNormalized(); Index: /trunk/src/VBox/Main/src-server/MachineImpl.cpp =================================================================== --- /trunk/src/VBox/Main/src-server/MachineImpl.cpp (revision 54947) +++ /trunk/src/VBox/Main/src-server/MachineImpl.cpp (revision 54948) @@ -4273,4 +4273,7 @@ } + /* 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,4 +4356,7 @@ 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,4 +4800,7 @@ 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,6 +5127,4 @@ 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,5 +9285,5 @@ 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,12 +9970,10 @@ 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,5 +10478,5 @@ 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,6 +10485,11 @@ 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); + } } } Index: /trunk/src/VBox/Main/src-server/MachineImplCloneVM.cpp =================================================================== --- /trunk/src/VBox/Main/src-server/MachineImplCloneVM.cpp (revision 54947) +++ /trunk/src/VBox/Main/src-server/MachineImplCloneVM.cpp (revision 54948) @@ -1342,5 +1342,5 @@ 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 Index: /trunk/src/VBox/Main/src-server/MediumImpl.cpp =================================================================== --- /trunk/src/VBox/Main/src-server/MediumImpl.cpp (revision 54947) +++ /trunk/src/VBox/Main/src-server/MediumImpl.cpp (revision 54948) @@ -1213,5 +1213,5 @@ /** * 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,10 +1219,9 @@ * 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,22 +1229,11 @@ * @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,6 +1246,6 @@ // 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,4 +1357,49 @@ 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,10 +1423,9 @@ 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,8 +3105,8 @@ 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,9 +3159,9 @@ 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,11 +3505,8 @@ * 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,33 +3540,50 @@ 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,4 +3594,5 @@ 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,4 +3601,5 @@ if ((*it) == id) { + // getting away with this as the iterator isn't used after m->llRegistryIDs.erase(it); fRemove = true; @@ -3560,17 +3608,31 @@ } - 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,6 +3648,7 @@ * @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,6 +3895,6 @@ 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,4 +4044,34 @@ 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 pVirtualBox(m->pVirtualBox); + if (!pVirtualBox) + return 1; + + /* we access mParent */ + AutoReadLock treeLock(pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS); + + uint32_t cDepth = 0; + ComObjPtr pMedium(this); + while (!pMedium.isNull()) + { + AutoCaller autoCaller(this); + AssertReturn(autoCaller.isOk(), cDepth + 1); + + pMedium = pMedium->m->pParent; + cDepth++; + } + + return cDepth; } @@ -4041,21 +4134,13 @@ /** - * 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,14 +4202,43 @@ 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,4 +6234,13 @@ 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,6 +6476,5 @@ { // 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,4 +7468,12 @@ 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,12 +7597,18 @@ 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 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,4 +7684,13 @@ 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,13 +7847,10 @@ 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,4 +7885,5 @@ pMedium->i_deparent(); // removes pMedium from source + // no depth check, reduces depth pMedium->i_setParent(pTarget); } @@ -7859,4 +8002,13 @@ 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,5 +8135,5 @@ } - /** @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,8 +8205,7 @@ 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,4 +8843,13 @@ 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,5 +8959,4 @@ } - /** @todo r=klaus target isn't locked, race getting the state */ vrc = VDCopy(hdd, VD_LAST_IMAGE, @@ -8848,8 +9007,7 @@ 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 Index: /trunk/src/VBox/Main/src-server/SnapshotImpl.cpp =================================================================== --- /trunk/src/VBox/Main/src-server/SnapshotImpl.cpp (revision 54947) +++ /trunk/src/VBox/Main/src-server/SnapshotImpl.cpp (revision 54948) @@ -1,5 +1,4 @@ /* $Id$ */ /** @file - * * COM class implementation for Snapshot and SnapshotMachine in VBoxSVC. */ @@ -175,4 +174,10 @@ 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 pSnapshot(this); if (m->pParent) i_deparent(); @@ -186,4 +191,8 @@ delete m; m = NULL; + + autoUninitSpan.setSucceeded(); + // see above, now the refcount may reach 0 + pSnapshot.setNull(); } @@ -725,12 +734,10 @@ /** - * 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,7 +747,4 @@ data.timestamp = m->timeStamp; data.strDescription = m->strDescription; - - if (aAttrsOnly) - return S_OK; // state file (only if this snapshot is online) @@ -756,27 +760,36 @@ 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,17 +799,15 @@ /** - * 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 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,15 +815,7 @@ * 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,42 +827,14 @@ * @return */ -HRESULT Snapshot::i_uninitRecursively(AutoWriteLock &writeLock, - CleanupMode_T cleanupMode, - MediaList &llMedia, - std::list &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 &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,12 +859,73 @@ } - 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 &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; } Index: /trunk/src/VBox/Main/src-server/VirtualBoxImpl.cpp =================================================================== --- /trunk/src/VBox/Main/src-server/VirtualBoxImpl.cpp (revision 54947) +++ /trunk/src/VBox/Main/src-server/VirtualBoxImpl.cpp (revision 54948) @@ -650,5 +650,5 @@ */ HRESULT VirtualBox::initMedia(const Guid &uuidRegistry, - const settings::MediaRegistry mediaRegistry, + const settings::MediaRegistry &mediaRegistry, const Utf8Str &strMachineFolder) { @@ -674,5 +674,6 @@ uuidRegistry, xmlHD, // XML data; this recurses to processes the children - strMachineFolder); + strMachineFolder, + treeLock); if (FAILED(rc)) return rc; @@ -694,5 +695,6 @@ uuidRegistry, xmlDvd, - strMachineFolder); + strMachineFolder, + treeLock); if (FAILED(rc)) return rc; @@ -714,5 +716,6 @@ uuidRegistry, xmlFloppy, - strMachineFolder); + strMachineFolder, + treeLock); if (FAILED(rc)) return rc; @@ -4014,8 +4017,11 @@ 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,5 +4488,5 @@ 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,5 +4497,5 @@ { // 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,5 +4503,5 @@ i_markRegistryModified(*puuidBetter); tlock.acquire(); - mlock.release(); + mlock.acquire(); } } @@ -4533,4 +4539,51 @@ 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 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(); + } + } } } Index: /trunk/src/VBox/Main/xml/Settings.cpp =================================================================== --- /trunk/src/VBox/Main/xml/Settings.cpp (revision 54947) +++ /trunk/src/VBox/Main/xml/Settings.cpp (revision 54948) @@ -55,5 +55,5 @@ /* - * Copyright (C) 2007-2014 Oracle Corporation + * Copyright (C) 2007-2015 Oracle Corporation * * This file is part of VirtualBox Open Source Edition (OSE), as @@ -120,4 +120,7 @@ /** 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,4 +380,25 @@ /** + * 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,5 +490,5 @@ * @return */ -com::Utf8Str ConfigFileBase::makeString(const RTTIMESPEC &stamp) +com::Utf8Str ConfigFileBase::stringifyTimestamp(const RTTIMESPEC &stamp) const { RTTIME time; @@ -566,13 +590,12 @@ * @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) { // - settings::Medium med; + Utf8Str strUUID; if (!elmMedium.getAttributeValue("uuid", strUUID)) @@ -726,32 +749,59 @@ 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,22 +838,22 @@ 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,16 +1136,20 @@ * 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,7 +1160,7 @@ 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,11 +1178,11 @@ // 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,8 +1204,8 @@ { // 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,5 +1232,5 @@ ++it) { - buildMedium(*pelmHardDisks, DeviceType_HardDisk, *it, 0); + buildMedium(HardDisk, 1, *pelmHardDisks, *it); } @@ -1186,5 +1240,5 @@ ++it) { - buildMedium(*pelmDVDImages, DeviceType_DVD, *it, 0); + buildMedium(DVDImage, 1, *pelmDVDImages, *it); } @@ -1194,5 +1248,5 @@ ++it) { - buildMedium(*pelmFloppyImages, DeviceType_Floppy, *it, 0); + buildMedium(FloppyImage, 1, *pelmFloppyImages, *it); } } @@ -3767,5 +3821,5 @@ { 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,13 +3868,10 @@ 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,5 +5200,5 @@ 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,5 +5307,5 @@ if (!fCurrentStateModified) elmMachine.setAttribute("currentStateModified", fCurrentStateModified); - elmMachine.setAttribute("lastStateChange", makeString(timeLastStateChange)); + elmMachine.setAttribute("lastStateChange", stringifyTimestamp(timeLastStateChange)); if (fAborted) elmMachine.setAttribute("aborted", fAborted);