Index: /trunk/src/VBox/Main/MachineImpl.cpp =================================================================== --- /trunk/src/VBox/Main/MachineImpl.cpp (revision 31227) +++ /trunk/src/VBox/Main/MachineImpl.cpp (revision 31228) @@ -3589,5 +3589,5 @@ aDevice, aControllerPort, aControllerName); - rc = detachDevice(pAttach, alock, &fNeedsSaveSettings); + rc = detachDevice(pAttach, alock, NULL /* pSnapshot */, &fNeedsSaveSettings); if (fNeedsSaveSettings) @@ -4080,5 +4080,5 @@ // this list collects the files that should be reported // as to be deleted to the caller in aFiles - std::list llFilesForCaller; + std::list llFilesForCaller; // discard saved state @@ -4087,5 +4087,5 @@ // add the saved state file to the list of files the caller should delete Assert(!mSSData->mStateFilePath.isEmpty()); - llFilesForCaller.push_back(Bstr(mSSData->mStateFilePath)); + llFilesForCaller.push_back(mSSData->mStateFilePath); mSSData->mStateFilePath.setNull(); @@ -4096,12 +4096,32 @@ } - // @todo optionally nuke snapshots size_t snapshotCount = 0; if (mData->mFirstSnapshot) snapshotCount = mData->mFirstSnapshot->getAllChildrenCount() + 1; if (snapshotCount) - return setError(VBOX_E_INVALID_OBJECT_STATE, - tr("Cannot unregister the machine '%ls' because it has %d snapshots"), - mUserData->mName.raw(), snapshotCount); + { + if (fAutoCleanup) + { + // caller wants automatic detachment: then do that and report all media to the array + + // Snapshot::beginDeletingSnapshot() needs the machine state to be this + MachineState_T oldState = mData->mMachineState; + mData->mMachineState = MachineState_DeletingSnapshot; + + // make a copy of the first snapshot so the refcount does not drop to 0 + // in beginDeletingSnapshot, which sets pFirstSnapshot to 0 (that hangs + // because of the AutoCaller voodoo) + ComObjPtr pFirstSnapshot = mData->mFirstSnapshot; + + // go! + pFirstSnapshot->uninitRecursively(alock, llMedia, llFilesForCaller); + + mData->mMachineState = oldState; + } + else + return setError(VBOX_E_INVALID_OBJECT_STATE, + tr("Cannot unregister the machine '%ls' because it has %d snapshots"), + mUserData->mName.raw(), snapshotCount); + } if ( !mMediaData.isNull() // can be NULL if machine is inaccessible @@ -4109,30 +4129,7 @@ ) { - // we have media attachments: + // we have media attachments: detach them all and add the Medium objects to our list if (fAutoCleanup) - { - // caller wants automatic detachment: then do that and report all media to the array - - // make a temporary list because detachDevice invalidates iterators into - // mMediaData->mAttachments - MediaData::AttachmentList llAttachments2 = mMediaData->mAttachments; - - for (MediaData::AttachmentList::iterator it = llAttachments2.begin(); - it != llAttachments2.end(); - ++it) - { - ComObjPtr pAttach = *it; - ComObjPtr pMedium = pAttach->getMedium(); - - if (!pMedium.isNull()) - llMedia.push_back(pMedium); - - rc = detachDevice(pAttach, - alock, - NULL /* pfNeedsSaveSettings */); - if (FAILED(rc)) - break; - }; - } + detachAllMedia(alock, NULL /* pSnapshot */, llMedia); else return setError(VBOX_E_INVALID_OBJECT_STATE, @@ -4163,5 +4160,5 @@ { ComObjPtr pMedium = *it; - Bstr bstrFile = pMedium->getLocationFull(); + Utf8Str strFile = pMedium->getLocationFull(); AutoCaller autoCaller2(pMedium); @@ -4172,4 +4169,6 @@ autoCaller2); // this uninitializes the medium + + LogFlowThisFunc(("Medium::close() on %s yielded rc (%Rhra)\n", strFile.c_str(), rc)); if (rc == VBOX_E_OBJECT_IN_USE) @@ -4180,5 +4179,5 @@ else if (SUCCEEDED(rc)) // report the path to the caller - llFilesForCaller.push_back(bstrFile); + llFilesForCaller.push_back(strFile); } } @@ -4187,8 +4186,8 @@ SafeArray sfaFiles(llFilesForCaller.size()); size_t i = 0; - for (std::list::iterator it = llFilesForCaller.begin(); + for (std::list::iterator it = llFilesForCaller.begin(); it != llFilesForCaller.end(); ++it) - it->detachTo(&sfaFiles[i++]); + Bstr(*it).detachTo(&sfaFiles[i++]); sfaFiles.detachTo(ComSafeArrayOutArg(aFiles)); @@ -8710,4 +8709,5 @@ * @param pAttach Medium attachment to detach. * @param writeLock Machine write lock which the caller must have locked once. This may be released temporarily in here. + * @param pSnapshot If NULL, then the detachment is for the current machine. Otherwise this is for a SnapshotMachine, and this must be its snapshot. * @param pfNeedsSaveSettings Optional pointer to a bool that must have been initialized to false and that will be set to true * by this function if the caller should invoke VirtualBox::saveSettings() because the global settings have changed. @@ -8716,4 +8716,5 @@ HRESULT Machine::detachDevice(MediumAttachment *pAttach, AutoWriteLock &writeLock, + Snapshot *pSnapshot, bool *pfNeedsSaveSettings) { @@ -8751,12 +8752,67 @@ mMediaData.backup(); - /* we cannot use erase (it) below because backup() above will create - * a copy of the list and make this copy active, but the iterator - * still refers to the original and is not valid for the copy */ + // we cannot use erase (it) below because backup() above will create + // a copy of the list and make this copy active, but the iterator + // still refers to the original and is not valid for the copy mMediaData->mAttachments.remove(pAttach); - /* For non-hard disk media, detach straight away. */ - if (mediumType != DeviceType_HardDisk && !oldmedium.isNull()) - oldmedium->detachFrom(mData->mUuid); + if (!oldmedium.isNull()) + { + // if this is from a snapshot, do not defer detachment to commitMedia() + if (pSnapshot) + oldmedium->detachFrom(mData->mUuid, pSnapshot->getId()); + // else if non-hard disk media, do not defer detachment to commitMedia() either + else if (mediumType != DeviceType_HardDisk) + oldmedium->detachFrom(mData->mUuid); + } + + return S_OK; +} + +/** + * Goes thru all medium attachments of the list and calls detachDevice() on each + * of them and attaches all Medium objects found in the process to the given list. + * + * This gets called from Machine::Unregister, both for the actual Machine and + * the SnapshotMachine objects that might be found in the snapshots. + * + * Requires caller and locking. + * + * @param writeLock Machine lock from top-level caller; this gets passed to detachDevice. + * @param llMedia Caller's list to receive Medium objects which got detached so caller can close() them. + * @param pSnapshot Must be NULL when called for a "real" Machine or a snapshot object if called for a SnapshotMachine. + * @return + */ +HRESULT Machine::detachAllMedia(AutoWriteLock &writeLock, + Snapshot *pSnapshot, + MediaList &llMedia) +{ + Assert(isWriteLockOnCurrentThread()); + + HRESULT rc; + + // make a temporary list because detachDevice invalidates iterators into + // mMediaData->mAttachments + MediaData::AttachmentList llAttachments2 = mMediaData->mAttachments; + + for (MediaData::AttachmentList::iterator it = llAttachments2.begin(); + it != llAttachments2.end(); + ++it) + { + ComObjPtr pAttach = *it; + ComObjPtr pMedium = pAttach->getMedium(); + + if (!pMedium.isNull()) + llMedia.push_back(pMedium); + + // real machine: then we need to use the proper method + rc = detachDevice(pAttach, + writeLock, + pSnapshot, + NULL /* pfNeedsSaveSettings */); + + if (FAILED(rc)) + return rc; + } return S_OK; Index: /trunk/src/VBox/Main/SnapshotImpl.cpp =================================================================== --- /trunk/src/VBox/Main/SnapshotImpl.cpp (revision 31227) +++ /trunk/src/VBox/Main/SnapshotImpl.cpp (revision 31228) @@ -251,5 +251,5 @@ /* we've changed the base of the current state so mark it as - * modified as it no longer guaranteed to be its copy */ + * modified as it no longer guaranteed to be its copy */ m->pMachine->mData->mCurrentStateModified = TRUE; } @@ -809,4 +809,66 @@ return saveSnapshotImpl(data, aAttrsOnly); +} + +/** + * 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 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 llFilenames + * @return + */ +HRESULT Snapshot::uninitRecursively(AutoWriteLock &writeLock, + MediaList &llMedia, + std::list &llFilenames) +{ + Assert(m->pMachine->isWriteLockOnCurrentThread()); + + HRESULT rc = 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 + + // 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->uninitRecursively(writeLock, llMedia, llFilenames); + if (FAILED(rc)) + return rc; + } + + // now call detachAllMedia on the snapshot machine + rc = m->pMachine->detachAllMedia(writeLock, + this /* pSnapshot */, + llMedia); + if (FAILED(rc)) + return rc; + + // now report the saved state file + if (!m->pMachine->mSSData->mStateFilePath.isEmpty()) + llFilenames.push_back(m->pMachine->mSSData->mStateFilePath); + + this->beginSnapshotDelete(); + this->uninit(); + + return S_OK; } Index: /trunk/src/VBox/Main/include/MachineImpl.h =================================================================== --- /trunk/src/VBox/Main/include/MachineImpl.h (revision 31227) +++ /trunk/src/VBox/Main/include/MachineImpl.h (revision 31228) @@ -778,5 +778,9 @@ HRESULT detachDevice(MediumAttachment *pAttach, AutoWriteLock &writeLock, + Snapshot *pSnapshot, bool *pfNeedsSaveSettings); + HRESULT detachAllMedia(AutoWriteLock &writeLock, + Snapshot *pSnapshot, + MediaList &llMedia); void commitMedia(bool aOnline = false); Index: /trunk/src/VBox/Main/include/SnapshotImpl.h =================================================================== --- /trunk/src/VBox/Main/include/SnapshotImpl.h (revision 31227) +++ /trunk/src/VBox/Main/include/SnapshotImpl.h (revision 31228) @@ -33,5 +33,5 @@ class ATL_NO_VTABLE Snapshot : - public VirtualBoxBase, // WithTypedChildren, + public VirtualBoxBase, VBOX_SCRIPTABLE_IMPL(ISnapshot) { @@ -124,4 +124,8 @@ HRESULT saveSnapshotImpl(settings::Snapshot &data, bool aAttrsOnly); + HRESULT uninitRecursively(AutoWriteLock &writeLock, + MediaList &llMedia, + std::list &llFilenames); + private: struct Data; // opaque, defined in SnapshotImpl.cpp