Index: /trunk/src/VBox/Main/idl/VirtualBox.xidl =================================================================== --- /trunk/src/VBox/Main/idl/VirtualBox.xidl (revision 35902) +++ /trunk/src/VBox/Main/idl/VirtualBox.xidl (revision 35903) @@ -5230,6 +5230,6 @@ - Unregisters the machine, which must have been previously registered using - , and optionally do additional + Unregisters a machine previously registered with + and optionally do additional cleanup before the machine is unregistered. @@ -5246,5 +5246,5 @@
  • With "UnregisterOnly", the machine will only be unregistered, but no additional cleanup will be performed. The call will fail if the machine is in "Saved" state - or has any snapshots or any media attached (see . + or has any snapshots or any media attached (see ). It is the responsibility of the caller to delete all such configuration in this mode. In this mode, the API behaves like the former @c IVirtualBox::unregisterMachine() API @@ -5252,6 +5252,8 @@
  • With "DetachAllReturnNone", the call will succeed even if the machine is in "Saved" state or if it has snapshots or media attached. All media attached to the current machine - state or in snapshots will be detached. No medium objects will be returned; all of the - machine's media will remain open.
    • + state or in snapshots will be detached. For machines created with VirtualBox 4.0 or + later, media which are registered in this machine's media registry which are still + attached to another machine besides this machine will be moved a another machine's + registry. No medium objects will be returned; all of the machine's media will remain open.
  • With "DetachAllReturnHardDisksOnly", the call will behave like with "DetachAllReturnNone", except that all the hard disk medium objects which were detached from the machine will Index: /trunk/src/VBox/Main/include/MediumImpl.h =================================================================== --- /trunk/src/VBox/Main/include/MediumImpl.h (revision 35902) +++ /trunk/src/VBox/Main/include/MediumImpl.h (revision 35903) @@ -176,4 +176,5 @@ bool addRegistry(const Guid& id); + bool removeRegistry(const Guid& id); bool isInRegistry(const Guid& id); bool getFirstRegistryMachineId(Guid &uuid) const; Index: /trunk/src/VBox/Main/include/VirtualBoxImpl.h =================================================================== --- /trunk/src/VBox/Main/include/VirtualBoxImpl.h (revision 35902) +++ /trunk/src/VBox/Main/include/VirtualBoxImpl.h (revision 35903) @@ -275,5 +275,5 @@ HRESULT saveSettings(); - void addGuidToListUniquely(GuidList &llRegistriesThatNeedSaving, Guid uuid); + void addGuidToListUniquely(GuidList &llRegistriesThatNeedSaving, const Guid &uuid); HRESULT saveRegistries(const GuidList &llRegistriesThatNeedSaving); Index: /trunk/src/VBox/Main/src-server/MachineImpl.cpp =================================================================== --- /trunk/src/VBox/Main/src-server/MachineImpl.cpp (revision 35902) +++ /trunk/src/VBox/Main/src-server/MachineImpl.cpp (revision 35903) @@ -4316,14 +4316,13 @@ mUserData->s.strName.c_str(), cSnapshots); - // this list collects the medium objects from all medium attachments - // which got detached from the machine and its snapshots, in the following - // order: - // 1) media from machine attachments (these have the "leaf" attachments with snapshots - // and must be closed first, or closing the parents will fail because they will - // children); + // This list collects the medium objects from all medium attachments + // which we will detach from the machine and its snapshots, in a specific + // order which allows for closing all media without getting "media in use" + // errors, simply by going through the list from the front to the back: + // 1) first media from machine attachments (these have the "leaf" attachments with snapshots + // and must be closed before the parent media from the snapshots, or closing the parents + // will fail because they still have children); // 2) media from the youngest snapshots followed by those from the parent snapshots until - // the root ("first") snapshot of the machine - // This order allows for closing the media on this list from the beginning to the end - // without getting "media in use" errors. + // the root ("first") snapshot of the machine. MediaList llMedia; @@ -7749,4 +7748,8 @@ if (!medium.isNull()) { + AutoCaller medCaller(medium); + if (FAILED(medCaller.rc())) return medCaller.rc(); + AutoWriteLock mlock(medium COMMA_LOCKVAL_SRC_POS); + if (isSnapshotMachine()) rc = medium->addBackReference(mData->mUuid, *puuidSnapshot); @@ -8777,4 +8780,8 @@ uuid = mParent->getGlobalRegistryId(); // VirtualBox global registry UUID + AutoCaller autoCaller(pMedium); + if (FAILED(autoCaller.rc())) return; + AutoWriteLock alock(pMedium COMMA_LOCKVAL_SRC_POS); + if (pMedium->addRegistry(uuid)) // registry actually changed: @@ -9034,6 +9041,5 @@ * called from #fixupMedia() when the changes are rolled back. * - * @param pfNeedsSaveSettings Optional pointer to a bool that must have been initialized to false and that will be set to true - * by this function if the caller should invoke VirtualBox::saveSettings() because the global settings have changed. + * @param pllRegistriesThatNeedSaving Optional pointer to a list of UUIDs to receive the registry IDs that need saving * * @note Locks this object for writing. @@ -9220,6 +9226,5 @@ * @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. + * @param pllRegistriesThatNeedSaving Optional pointer to a list of UUIDs to receive the registry IDs that need saving * @return */ @@ -9251,5 +9256,6 @@ writeLock.release(); - HRESULT rc = oldmedium->deleteStorage(NULL /*aProgress*/, true /*aWait*/, + HRESULT rc = oldmedium->deleteStorage(NULL /*aProgress*/, + true /*aWait*/, pllRegistriesThatNeedSaving); @@ -9283,12 +9289,19 @@ /** - * 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, - * depending on cleanupMode. + * Goes thru all media of the given list and + * + * 1) calls detachDevice() on each of them for this machine and + * 2) adds all Medium objects found in the process to the given list, + * depending on cleanupMode. + * + * If cleanupMode is CleanupMode_DetachAllReturnHardDisksOnly, this only + * adds hard disks to the list. If it is CleanupMode_Full, this adds all + * media to the 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. + * Requires caller and locking. The machine lock must be passed in because it + * will be passed on to detachDevice which needs it for temporary unlocking. * * @param writeLock Machine lock from top-level caller; this gets passed to detachDevice. @@ -9316,5 +9329,5 @@ ++it) { - ComObjPtr pAttach = *it; + ComObjPtr &pAttach = *it; ComObjPtr pMedium = pAttach->getMedium(); Index: /trunk/src/VBox/Main/src-server/MediumImpl.cpp =================================================================== --- /trunk/src/VBox/Main/src-server/MediumImpl.cpp (revision 35902) +++ /trunk/src/VBox/Main/src-server/MediumImpl.cpp (revision 35903) @@ -1626,5 +1626,6 @@ // we access mParent and members - AutoMultiWriteLock2 mlock(&m->pVirtualBox->getMediaTreeLockHandle(), this->lockHandle() COMMA_LOCKVAL_SRC_POS); + AutoMultiWriteLock2 mlock(&m->pVirtualBox->getMediaTreeLockHandle(), + this->lockHandle() COMMA_LOCKVAL_SRC_POS); switch (m->state) @@ -3076,4 +3077,6 @@ * See getFirstRegistryMachineId() for details. * + * Must have caller + locking! + * * @param id * @return true if the registry was added; false if the given id was already on the list. @@ -3081,9 +3084,5 @@ bool Medium::addRegistry(const Guid& id) { - AutoCaller autoCaller(this); - if (FAILED(autoCaller.rc())) return false; - - AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - + // hard disks cannot be in more than one registry if ( m->devType == DeviceType_HardDisk && m->llRegistryIDs.size() > 0 @@ -3105,5 +3104,33 @@ /** + * Removes the given UUID from the list of media registry UUIDs. Returns true + * if found or false if not. + * + * Must have caller + locking! + * + * @param id + * @return + */ +bool Medium::removeRegistry(const Guid& id) +{ + for (GuidList::iterator it = m->llRegistryIDs.begin(); + it != m->llRegistryIDs.end(); + ++it) + { + if ((*it) == id) + { + m->llRegistryIDs.erase(it); + return true; + } + } + + return false; +} + +/** * Returns true if id is in the list of media registries for this medium. + * + * Must have caller + locking! + * * @param id * @return Index: /trunk/src/VBox/Main/src-server/SnapshotImpl.cpp =================================================================== --- /trunk/src/VBox/Main/src-server/SnapshotImpl.cpp (revision 35902) +++ /trunk/src/VBox/Main/src-server/SnapshotImpl.cpp (revision 35903) @@ -812,5 +812,5 @@ * cannot be closed if they have children. * - * This calls uninit() on itself, so the snapshots tree becomes invalid after this. + * 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). * @@ -898,5 +898,5 @@ uninit(); - + BaseFinalRelease(); } Index: /trunk/src/VBox/Main/src-server/VirtualBoxImpl.cpp =================================================================== --- /trunk/src/VBox/Main/src-server/VirtualBoxImpl.cpp (revision 35902) +++ /trunk/src/VBox/Main/src-server/VirtualBoxImpl.cpp (revision 35903) @@ -345,5 +345,5 @@ HRESULT rc = init(); - + BaseFinalConstruct(); @@ -3192,50 +3192,40 @@ } + struct { + MediaOList &llSource; + settings::MediaList &llTarget; + } s[] = + { + // hard disks + { m->allHardDisks, mediaRegistry.llHardDisks }, + // CD/DVD images + { m->allDVDImages, mediaRegistry.llDvdImages }, + // floppy images + { m->allFloppyImages, mediaRegistry.llFloppyImages } + }; + HRESULT rc; - // hard disks - mediaRegistry.llHardDisks.clear(); - for (MediaList::const_iterator it = m->allHardDisks.begin(); - it != m->allHardDisks.end(); - ++it) - { - Medium *pMedium = *it; - if (pMedium->isInRegistry(uuidRegistry)) - { - settings::Medium med; - rc = pMedium->saveSettings(med, strMachineFolder); // this recurses into its children - if (FAILED(rc)) throw rc; - mediaRegistry.llHardDisks.push_back(med); - } - } - - // CD/DVD images - mediaRegistry.llDvdImages.clear(); - for (MediaList::const_iterator it = m->allDVDImages.begin(); - it != m->allDVDImages.end(); - ++it) - { - Medium *pMedium = *it; - if (pMedium->isInRegistry(uuidRegistry)) - { - settings::Medium med; - rc = pMedium->saveSettings(med, strMachineFolder); - if (FAILED(rc)) throw rc; - mediaRegistry.llDvdImages.push_back(med); - } - } - - // floppy images - mediaRegistry.llFloppyImages.clear(); - for (MediaList::const_iterator it = m->allFloppyImages.begin(); - it != m->allFloppyImages.end(); - ++it) - { - Medium *pMedium = *it; - if (pMedium->isInRegistry(uuidRegistry)) - { - settings::Medium med; - rc = pMedium->saveSettings(med, strMachineFolder); - if (FAILED(rc)) throw rc; - mediaRegistry.llFloppyImages.push_back(med); + + for (size_t i = 0; i < RT_ELEMENTS(s); ++i) + { + MediaOList &llSource = s[i].llSource; + settings::MediaList &llTarget = s[i].llTarget; + llTarget.clear(); + for (MediaList::const_iterator it = llSource.begin(); + it != llSource.end(); + ++it) + { + Medium *pMedium = *it; + AutoCaller autoCaller(pMedium); + if (FAILED(autoCaller.rc())) throw autoCaller.rc(); + AutoReadLock mlock(pMedium COMMA_LOCKVAL_SRC_POS); + + if (pMedium->isInRegistry(uuidRegistry)) + { + settings::Medium med; + rc = pMedium->saveSettings(med, strMachineFolder); // this recurses into child hard disks + if (FAILED(rc)) throw rc; + llTarget.push_back(med); + } } } @@ -3664,5 +3654,6 @@ { - AutoWriteLock mlock(getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS); + AutoWriteLock tlock(getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS); + for (MediaOList::iterator it = m->allHardDisks.getList().begin(); it != m->allHardDisks.getList().end(); @@ -3670,4 +3661,7 @@ { ComObjPtr pMedium = *it; + AutoCaller medCaller(pMedium); + if (FAILED(medCaller.rc())) return medCaller.rc(); + AutoReadLock medlock(pMedium COMMA_LOCKVAL_SRC_POS); if (pMedium->isInRegistry(uuidMachine)) @@ -3702,13 +3696,49 @@ const Guid &id) { + // remove from the collection of registered machines AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - - // remove from the collection of registered machines m->allMachines.removeChild(pMachine); - // save the global registry HRESULT rc = saveSettings(); - alock.release(); + + /* + * Now go over all known media and checks if they were registered in the + * media registry of the given machine. Each such medium is then moved to + * a different media registry to make sure it doesn't get lost since its + * media registry is about to go away. + * + * This fixes the following use case: Image A.vdi of machine A is also used + * by machine B, but registered in the media registry of machine A. If machine + * A is deleted, A.vdi must be moved to the registry of B, or else B will + * become inaccessible. + */ + GuidList llRegistriesThatNeedSaving; + { + AutoWriteLock tlock(getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS); + for (MediaOList::iterator it = m->allHardDisks.getList().begin(); + it != m->allHardDisks.getList().end(); + ++it) + { + ComObjPtr &pMedium = *it; + AutoCaller medCaller(pMedium); + if (FAILED(medCaller.rc())) return medCaller.rc(); + AutoWriteLock mlock(pMedium COMMA_LOCKVAL_SRC_POS); + + if (pMedium->removeRegistry(id)) + { + // ID was found in medium's registry list: + // add a new one then + const Guid *puuidBetter = pMedium->getFirstMachineBackrefId(); + if (puuidBetter) + { + pMedium->addRegistry(*puuidBetter); + addGuidToListUniquely(llRegistriesThatNeedSaving, *puuidBetter); + } + } + } + } + + saveRegistries(llRegistriesThatNeedSaving); /* fire an event */ @@ -3727,5 +3757,5 @@ */ void VirtualBox::addGuidToListUniquely(GuidList &llRegistriesThatNeedSaving, - Guid uuid) + const Guid &uuid) { for (GuidList::const_iterator it = llRegistriesThatNeedSaving.begin(); @@ -4392,5 +4422,5 @@ AssertReturn(pvUser, VERR_INVALID_POINTER); - + com::Initialize();