Changeset 35903 in vbox

Timestamp:

Feb 8, 2011 4:46:25 PM (14 years ago)

Author:

vboxsync

Message:

Main: before unregistering a machine, move media shared with another machine to that machine's media registry to prevent that machine from becoming inaccessible

Location:

trunk/src/VBox/Main

Files:

• idl/VirtualBox.xidl (modified) (3 diffs)
• include/MediumImpl.h (modified) (1 diff)
• include/VirtualBoxImpl.h (modified) (1 diff)
• src-server/MachineImpl.cpp (modified) (8 diffs)
• src-server/MediumImpl.cpp (modified) (4 diffs)
• src-server/SnapshotImpl.cpp (modified) (2 diffs)
• src-server/VirtualBoxImpl.cpp (modified) (7 diffs)

trunk/src/VBox/Main/idl/VirtualBox.xidl

@@ -5230 +5230 @@
     <method name="unregister">
       <desc>
-        Unregisters the machine, which must have been previously registered using
-        <link to="IVirtualBox::registerMachine"/>, and optionally do additional
+        Unregisters a machine previously registered with
+        <link to="IVirtualBox::registerMachine"/> and optionally do additional
         cleanup before the machine is unregistered.
 
@@ -5246 +5246 @@
           <li>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 <link to="IMediumAttachment" />.
+            or has any snapshots or any media attached (see <link to="IMediumAttachment" />).
             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 +5252 @@
           <li>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.</li>
+            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.</li>
           <li>With "DetachAllReturnHardDisksOnly", the call will behave like with "DetachAllReturnNone",
             except that all the hard disk medium objects which were detached from the machine will

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

@@ -176 +176 @@
 
     bool addRegistry(const Guid& id);
+    bool removeRegistry(const Guid& id);
     bool isInRegistry(const Guid& id);
     bool getFirstRegistryMachineId(Guid &uuid) const;

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

@@ -275 +275 @@
     HRESULT saveSettings();
 
-    void addGuidToListUniquely(GuidList &llRegistriesThatNeedSaving, Guid uuid);
+    void addGuidToListUniquely(GuidList &llRegistriesThatNeedSaving, const Guid &uuid);
     HRESULT saveRegistries(const GuidList &llRegistriesThatNeedSaving);
 

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

@@ -4316 +4316 @@
                            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 +7748 @@
         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 +8780 @@
         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 +9041 @@
  * 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 +9226 @@
  * @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 +9256 @@
         writeLock.release();
 
-        HRESULT rc = oldmedium->deleteStorage(NULL /*aProgress*/, true /*aWait*/,
+        HRESULT rc = oldmedium->deleteStorage(NULL /*aProgress*/,
+                                              true /*aWait*/,
                                               pllRegistriesThatNeedSaving);
 
@@ -9283 +9289 @@
 
 /**
- * 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 +9329 @@
          ++it)
     {
-        ComObjPtr<MediumAttachment> pAttach = *it;
+        ComObjPtr<MediumAttachment> &pAttach = *it;
         ComObjPtr<Medium> pMedium = pAttach->getMedium();
 

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

@@ -1626 +1626 @@
 
     // 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 +3077 @@
  * 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 +3084 @@
 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 +3104 @@
 
 /**
+ * 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

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

@@ -812 +812 @@
  * 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 +898 @@
 
     uninit();
- 
+
     BaseFinalRelease();
 }

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

@@ -345 +345 @@
 
     HRESULT rc = init();
-   
+
     BaseFinalConstruct();
 
@@ -3192 +3192 @@
     }
 
+    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 +3654 @@
 
     {
-        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 +3661 @@
         {
             ComObjPtr<Medium> pMedium = *it;
+            AutoCaller medCaller(pMedium);
+            if (FAILED(medCaller.rc())) return medCaller.rc();
+            AutoReadLock medlock(pMedium COMMA_LOCKVAL_SRC_POS);
 
             if (pMedium->isInRegistry(uuidMachine))
@@ -3702 +3696 @@
                                       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<Medium> &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 +3757 @@
  */
 void VirtualBox::addGuidToListUniquely(GuidList &llRegistriesThatNeedSaving,
-                                       Guid uuid)
+                                       const Guid &uuid)
 {
     for (GuidList::const_iterator it = llRegistriesThatNeedSaving.begin();
@@ -4392 +4422 @@
 
     AssertReturn(pvUser, VERR_INVALID_POINTER);
-    
+
     com::Initialize();