Changeset 27256 in vbox

Timestamp:

Mar 10, 2010 4:50:08 PM (15 years ago)

Author:

vboxsync

Message:

Main: fix regressions with renaming VMs; as a welcome side effect, Machine::saveSettings() no longer implicitly calls VirtualBox::saveSettings() and therefore no longer requires the global VirtualBox lock

Location:

trunk/src/VBox/Main

Files:

• MachineImpl.cpp (modified) (16 diffs)
• MediumImpl.cpp (modified) (1 diff)
• SnapshotImpl.cpp (modified) (7 diffs)
• VirtualBoxImpl.cpp (modified) (8 diffs)
• include/MachineImpl.h (modified) (1 diff)
• include/VirtualBoxImpl.h (modified) (1 diff)

trunk/src/VBox/Main/MachineImpl.cpp

@@ -1719 +1719 @@
     if (FAILED(rc)) return rc;
 
-    BOOL *pb;
-
     switch(property)
     {
@@ -2928 +2926 @@
     if (fNeedsSaveSettings)
     {
-        alock.release();
-        AutoWriteLock vboxlock(this COMMA_LOCKVAL_SRC_POS);
-        saveSettings();
+        bool fNeedsGlobalSaveSettings = false;
+        saveSettings(&fNeedsGlobalSaveSettings);
+
+        if (fNeedsGlobalSaveSettings)
+        {
+            alock.release();
+            AutoWriteLock vboxlock(this COMMA_LOCKVAL_SRC_POS);
+            mParent->saveSettings();
+        }
     }
 
@@ -3306 +3310 @@
         }
 
-        // data is changing and change not vetoed: then write it out under the locks
-
-        // saveSettings() needs VirtualBox write lock
-        AutoMultiWriteLock2 alock(mParent, this COMMA_LOCKVAL_SRC_POS);
+        // data is changing and change not vetoed: then write it out under the lock
+        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
         if (getClassID() == clsidSnapshotMachine)
@@ -3323 +3325 @@
                 // creates a new key if needed
 
-        /* save settings on success */
-        HRESULT rc = saveSettings();
-        if (FAILED(rc)) return rc;
+        bool fNeedsGlobalSaveSettings = false;
+        saveSettings(&fNeedsGlobalSaveSettings);
+
+        if (fNeedsGlobalSaveSettings)
+        {
+            alock.release();
+            AutoWriteLock vboxlock(mParent COMMA_LOCKVAL_SRC_POS);
+            mParent->saveSettings();
+        }
     }
 
@@ -3340 +3348 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    /* saveSettings() needs mParent lock */
-    AutoMultiWriteLock2 alock(mParent, this COMMA_LOCKVAL_SRC_POS);
+    AutoWriteLock mlock(this COMMA_LOCKVAL_SRC_POS);
 
     /* when there was auto-conversion, we want to save the file even if
@@ -3352 +3359 @@
 
     /* save all VM data excluding snapshots */
-    return saveSettings();
+    bool fNeedsGlobalSaveSettings = false;
+    rc = saveSettings(&fNeedsGlobalSaveSettings);
+    mlock.release();
+
+    if (SUCCEEDED(rc) && fNeedsGlobalSaveSettings)
+    {
+        AutoWriteLock vlock(mParent COMMA_LOCKVAL_SRC_POS);
+        rc = mParent->saveSettings();
+    }
+
+    return rc;
 }
 
@@ -5476 +5493 @@
        )
     {
-        rc = saveSettings();
+        rc = saveSettings(NULL);
+                // no need to check whether VirtualBox.xml needs saving too since
+                // we can't have a machine XML file rename pending
         if (FAILED(rc)) return rc;
     }
@@ -6840 +6859 @@
  *  @param aNew     receives |true| if a virgin settings file was created.
  */
-HRESULT Machine::prepareSaveSettings(bool &aRenamed,
-                                     bool &aNew)
-{
-    /* Note: tecnhically, mParent needs to be locked only when the machine is
-     * registered (see prepareSaveSettings() for details) but we don't
-     * currently differentiate it in callers of saveSettings() so we don't
-     * make difference here too.  */
-    AssertReturn(mParent->isWriteLockOnCurrentThread(), E_FAIL);
+HRESULT Machine::prepareSaveSettings(bool *pfNeedsGlobalSaveSettings)
+{
     AssertReturn(isWriteLockOnCurrentThread(), E_FAIL);
 
     HRESULT rc = S_OK;
 
-    aRenamed = false;
-
-    /* if we're ready and isConfigLocked() is FALSE then it means
-     * that no config file exists yet (we will create a virgin one) */
-    aNew = !mData->m_pMachineConfigFile->fileExists();
+    bool fSettingsFileIsNew = !mData->m_pMachineConfigFile->fileExists();
 
     /* attempt to rename the settings file if machine name is changed */
@@ -6864 +6873 @@
        )
     {
-        aRenamed = true;
-
         bool dirRenamed = false;
         bool fileRenamed = false;
@@ -6893 +6900 @@
                  * above and because name != newName */
                 Assert(configDir != newConfigDir);
-                if (!aNew)
+                if (!fSettingsFileIsNew)
                 {
                     /* perform real rename only if the machine is not new */
@@ -6921 +6928 @@
                                         RTPATH_DELIMITER,
                                         RTPathFilename(configFile.c_str()));
-                if (!aNew)
+                if (!fSettingsFileIsNew)
                 {
                     /* perform real rename only if the machine is not new */
@@ -6939 +6946 @@
 
             /* update m_strConfigFileFull amd mConfigFile */
-            Utf8Str oldConfigFileFull = mData->m_strConfigFileFull;
-            Utf8Str oldConfigFile = mData->m_strConfigFile;
             mData->m_strConfigFileFull = newConfigFile;
-            /* try to get the relative path for mConfigFile */
+
+            // compute the relative path too
             Utf8Str path = newConfigFile;
             mParent->calculateRelativePath(path, path);
             mData->m_strConfigFile = path;
 
-            /* last, try to update the global settings with the new path */
-            if (mData->mRegistered)
+            // store the old and new so that VirtualBox::saveSettings() can update
+            // the media registry
+            if (    mData->mRegistered
+                 && configDir != newConfigDir)
             {
-                rc = mParent->updatePathInMediaRegistry(configDir.c_str(), newConfigDir.c_str());
-                if (FAILED(rc))
-                {
-                    /* revert to old values */
-                    mData->m_strConfigFileFull = oldConfigFileFull;
-                    mData->m_strConfigFile = oldConfigFile;
-                    break;
-                }
+                mParent->rememberMachineNameChangeForMedia(configDir, newConfigDir);
+
+                if (pfNeedsGlobalSaveSettings)
+                    *pfNeedsGlobalSaveSettings = true;
             }
 
@@ -7001 +7005 @@
     }
 
-    if (aNew)
+    if (fSettingsFileIsNew)
     {
         /* create a virgin config file */
@@ -7053 +7057 @@
  *    change machine data directly, not through the backup()/commit() mechanism.
  *
- * @note Must be called from under mParent write lock (sometimes needed by
- * #prepareSaveSettings()) and this object's write lock. Locks children for
- * writing. There is one exception when mParent is unused and therefore may be
- * left unlocked: if this machine is an unregistered one.
+ * @note Must be called from under this object's write lock. Locks children for
+ * writing.
+ *
+ * @param pfNeedsGlobalSaveSettings 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 must invoke VirtualBox::saveSettings() because the global
+ *          settings have changed. This will happen if a machine rename has been
+ *          saved and the global machine and media registries will therefore need
+ *          updating.
  */
-HRESULT Machine::saveSettings(int aFlags /*= 0*/)
+HRESULT Machine::saveSettings(bool *pfNeedsGlobalSaveSettings,
+                              int aFlags /*= 0*/)
 {
     LogFlowThisFuncEnter();
 
-    /* Note: tecnhically, mParent needs to be locked only when the machine is
-     * registered (see prepareSaveSettings() for details) but we don't
-     * currently differentiate it in callers of saveSettings() so we don't
-     * make difference here too.  */
-    AssertReturn(mParent->isWriteLockOnCurrentThread(), E_FAIL);
     AssertReturn(isWriteLockOnCurrentThread(), E_FAIL);
 
@@ -7083 +7088 @@
      * settings directory and file if the machine name was changed and about
      * creating a new settings file if this is a new machine. */
-    bool fIsRenamed = false;
-    bool fIsNew = false;
-    rc = prepareSaveSettings(fIsRenamed, fIsNew);
+    rc = prepareSaveSettings(pfNeedsGlobalSaveSettings);
     if (FAILED(rc)) return rc;
 
@@ -10201 +10204 @@
 
         /* save all VM settings */
-        rc = saveSettings();
+        rc = saveSettings(NULL);
+                // no need to check whether VirtualBox.xml needs saving also since
+                // we can't have a name change pending at this point
     }
     else

trunk/src/VBox/Main/MediumImpl.cpp

@@ -2894 +2894 @@
     LogFlowThisFunc(("locationFull.before='%s'\n", m->strLocationFull.raw()));
 
-    Utf8Str path = m->strLocationFull;
-
-    if (RTPathStartsWith(path.c_str(), aOldPath))
-    {
-        Utf8Str newPath = Utf8StrFmt("%s%s", aNewPath,
-                                     path.raw() + strlen(aOldPath));
-        path = newPath;
-
+    const char *pcszMediumPath = m->strLocationFull.c_str();
+
+    if (RTPathStartsWith(pcszMediumPath, aOldPath))
+    {
+        Utf8Str newPath = Utf8StrFmt("%s%s",
+                                     aNewPath,
+                                     pcszMediumPath + strlen(aOldPath));
+        Utf8Str path = newPath;
         m->pVirtualBox->calculateRelativePath(path, path);
-
         unconst(m->strLocationFull) = newPath;
         unconst(m->strLocation) = path;

trunk/src/VBox/Main/SnapshotImpl.cpp

@@ -1260 +1260 @@
         /* save all current settings to ensure current changes are committed and
          * hard disks are fixed up */
-
-        // VirtualBox lock before machine lock
-        alock.release();
-        AutoWriteLock vboxLock(mParent COMMA_LOCKVAL_SRC_POS);
-        alock.acquire();
-        HRESULT rc = saveSettings();
+        HRESULT rc = saveSettings(NULL);
+                // no need to check for whether VirtualBox.xml needs changing since
+                // we can't have a machine XML rename pending at this point
         if (FAILED(rc)) return rc;
     }
@@ -1463 +1460 @@
             mData->mCurrentStateModified = FALSE;
 
-        rc = saveSettings();
+        rc = saveSettings(NULL);
+                // no need to change for whether VirtualBox.xml needs saving since
+                // we can't have a machine XML rename pending at this point
     }
 
@@ -1850 +1849 @@
         }
 
-        alock.leave();
-
-        AutoWriteLock vboxLock(mParent COMMA_LOCKVAL_SRC_POS);
-        alock.enter();
-
         // save machine settings, reset the modified flag and commit;
-        rc = saveSettings(SaveS_ResetCurStateModified | saveFlags);
+        rc = saveSettings(&fNeedsSaveSettings, SaveS_ResetCurStateModified | saveFlags);
         if (FAILED(rc)) throw rc;
 
         // let go of the locks while we're deleting image files below
         alock.leave();
-        vboxLock.release();
-
         // from here on we cannot roll back on failure any more
 
@@ -1881 +1873 @@
         {
             // finally, VirtualBox.xml needs saving too
-            vboxLock.acquire();
+            AutoWriteLock vboxLock(mParent COMMA_LOCKVAL_SRC_POS);
             mParent->saveSettings();
         }
@@ -1952 +1944 @@
     AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
 
-    /* saveSettings() needs mParent lock */
-    AutoMultiWriteLock2 alock(mParent, this COMMA_LOCKVAL_SRC_POS);
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
     // machine must not be running
@@ -1979 +1970 @@
         if (m_flModifications)
         {
-            rc = saveSettings();
+            rc = saveSettings(NULL);
+                // no need to change for whether VirtualBox.xml needs saving since
+                // we can't have a machine XML rename pending at this point
             if (FAILED(rc)) return rc;
         }
@@ -2404 +2397 @@
         if (fMachineSettingsChanged || fNeedsSaveSettings)
         {
-            // saveSettings needs VirtualBox write lock in addition to our own
-            // (parent -> child locking order!)
-            AutoWriteLock vboxLock(mParent COMMA_LOCKVAL_SRC_POS);
-
             if (fMachineSettingsChanged)
             {
                 AutoWriteLock machineLock(this COMMA_LOCKVAL_SRC_POS);
-                saveSettings(SaveS_InformCallbacksAnyway);
+                saveSettings(&fNeedsSaveSettings, SaveS_InformCallbacksAnyway);
             }
 
             if (fNeedsSaveSettings)
+            {
+                AutoWriteLock vboxLock(mParent COMMA_LOCKVAL_SRC_POS);
                 mParent->saveSettings();
+            }
         }
     }

trunk/src/VBox/Main/VirtualBoxImpl.cpp

@@ -223 +223 @@
     // All the media lists are protected by the following locking handle:
     RWLockHandle                        lockMedia;
-    MediaOList                          ollHardDisks,
+    MediaOList                          ollHardDisks,           // base images only!
                                         ollDVDImages,
                                         ollFloppyImages;
@@ -231 +231 @@
     HardDiskMap                         mapHardDisks;
 
+    // list of pending machine renames (also protected by media tree lock;
+    // see VirtualBox::rememberMachineNameChangeForMedia())
+    struct PendingMachineRename
+    {
+        Utf8Str     strConfigDirOld;
+        Utf8Str     strConfigDirNew;
+    };
+    typedef std::list<PendingMachineRename> PendingMachineRenamesList;
+    PendingMachineRenamesList           llPendingMachineRenames;
+
     RWLockHandle                        lockSharedFolders;
     SharedFoldersOList                  ollSharedFolders;
@@ -240 +250 @@
     ProgressMap                         mapProgressOperations;
     CallbackList                        llCallbacks;
-
 
     // the following are data for the client watcher thread
@@ -3245 +3254 @@
 
 /**
+ * Called from Machine::prepareSaveSettings() when it has detected
+ * that a machine has been renamed. Such renames will require
+ * updating the global media registry during the
+ * VirtualBox::saveSettings() that follows later.
+*
+ * When a machine is renamed, there may well be media (in particular,
+ * diff images for snapshots) in the global registry that will need
+ * to have their paths updated. Before 3.2, Machine::saveSettings
+ * used to call VirtualBox::saveSettings implicitly, which was both
+ * unintuitive and caused locking order problems. Now, we remeber
+ * such pending name changes with this method so that
+ * VirtualBox::saveSettings() can process them properly.
+ */
+void VirtualBox::rememberMachineNameChangeForMedia(const Utf8Str &strOldConfigDir,
+                                                   const Utf8Str &strNewConfigDir)
+{
+    AutoWriteLock mediaLock(getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+
+    Data::PendingMachineRename pmr;
+    pmr.strConfigDirOld = strOldConfigDir;
+    pmr.strConfigDirNew = strNewConfigDir;
+    m->llPendingMachineRenames.push_back(pmr);
+}
+
+/**
  *  Helper function to write out the configuration tree.
  *
- *  @note Locks this object for writing and child objects for reading/writing!
+ *  @note Caller must have locked the VirtualBox object for writing!
  */
 HRESULT VirtualBox::saveSettings()
@@ -3254 +3288 @@
     AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
 
+    AssertReturn(isWriteLockOnCurrentThread(), E_FAIL);
     AssertReturn(!m->strSettingsFilePath.isEmpty(), E_FAIL);
 
@@ -3260 +3295 @@
     try
     {
+        // lock the lists while we're here
+        AutoReadLock machinesLock(m->ollMachines.getLockHandle() COMMA_LOCKVAL_SRC_POS);
+
         // machines
         settings::MachinesRegistry machinesTemp;
         {
-            AutoReadLock al(m->ollMachines.getLockHandle() COMMA_LOCKVAL_SRC_POS);
             for (MachinesOList::iterator it = m->ollMachines.begin();
                  it != m->ollMachines.end();
                  ++it)
             {
+                Machine *pMachine = *it;
+                // save actual machine registry entry
                 settings::MachineRegistryEntry mre;
-                rc = (*it)->saveRegistryEntry(mre);
+                rc = pMachine->saveRegistryEntry(mre);
                 machinesTemp.push_back(mre);
             }
         }
 
+        // lock all media for the following; use a write lock because we're
+        // modifying the PendingMachineRenamesList, which is protected by this
+        AutoWriteLock mediaLock(getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+
+        // if a machine was renamed, then we'll need to refresh media paths
+        if (m->llPendingMachineRenames.size())
+        {
+            // make a single list from the three media lists so we don't need three loops
+            MediaList llAllMedia;
+            // with hard disks, we must use the map, not the list, because the list only has base images
+            for (HardDiskMap::iterator it = m->mapHardDisks.begin(); it != m->mapHardDisks.end(); ++it)
+                llAllMedia.push_back(it->second);
+            for (MediaList::iterator it = m->ollDVDImages.begin(); it != m->ollDVDImages.end(); ++it)
+                llAllMedia.push_back(*it);
+            for (MediaList::iterator it = m->ollFloppyImages.begin(); it != m->ollFloppyImages.end(); ++it)
+                llAllMedia.push_back(*it);
+
+            for (MediaList::iterator it = llAllMedia.begin();
+                 it != llAllMedia.end();
+                 ++it)
+            {
+                Medium *pMedium = *it;
+                for (Data::PendingMachineRenamesList::iterator it2 = m->llPendingMachineRenames.begin();
+                     it2 != m->llPendingMachineRenames.end();
+                     ++it2)
+                {
+                    const Data::PendingMachineRename &pmr = *it2;
+                    const char *pcszOld = pmr.strConfigDirOld.c_str();
+                    const char *pcszNew = pmr.strConfigDirNew.c_str();
+                    pMedium->updatePath(pcszOld, pcszNew);
+                }
+            }
+            // done, don't do it again until we have more machine renames
+            m->llPendingMachineRenames.clear();
+        }
+
         // hard disks
         settings::MediaList hardDisksTemp;
-        {
-            AutoReadLock al(m->ollHardDisks.getLockHandle() COMMA_LOCKVAL_SRC_POS);
-            for (MediaList::const_iterator it = m->ollHardDisks.begin();
-                 it != m->ollHardDisks.end();
-                 ++it)
-            {
-                settings::Medium med;
-                rc = (*it)->saveSettings(med);
-                if (FAILED(rc)) throw rc;
-                hardDisksTemp.push_back(med);
-            }
+        for (MediaList::const_iterator it = m->ollHardDisks.begin();
+                it != m->ollHardDisks.end();
+                ++it)
+        {
+            settings::Medium med;
+            rc = (*it)->saveSettings(med);     // this recurses into its children
+            if (FAILED(rc)) throw rc;
+            hardDisksTemp.push_back(med);
         }
 
         /* CD/DVD images */
         settings::MediaList dvdsTemp;
-        {
-            AutoReadLock al(m->ollDVDImages.getLockHandle() COMMA_LOCKVAL_SRC_POS);
-            for (MediaList::const_iterator it = m->ollDVDImages.begin();
-                 it != m->ollDVDImages.end();
-                 ++it)
-            {
-                settings::Medium med;
-                rc = (*it)->saveSettings(med);
-                if (FAILED(rc)) throw rc;
-                dvdsTemp.push_back(med);
-            }
+        for (MediaList::const_iterator it = m->ollDVDImages.begin();
+                it != m->ollDVDImages.end();
+                ++it)
+        {
+            settings::Medium med;
+            rc = (*it)->saveSettings(med);
+            if (FAILED(rc)) throw rc;
+            dvdsTemp.push_back(med);
         }
 
         /* floppy images */
         settings::MediaList floppiesTemp;
-        {
-            AutoReadLock al(m->ollFloppyImages.getLockHandle() COMMA_LOCKVAL_SRC_POS);
-            for (MediaList::const_iterator it = m->ollFloppyImages.begin();
-                 it != m->ollFloppyImages.end();
-                 ++it)
-            {
-                settings::Medium med;
-                rc = (*it)->saveSettings(med);
-                if (FAILED(rc)) throw rc;
-                floppiesTemp.push_back(med);
-            }
+        for (MediaList::const_iterator it = m->ollFloppyImages.begin();
+                it != m->ollFloppyImages.end();
+                ++it)
+        {
+            settings::Medium med;
+            rc = (*it)->saveSettings(med);
+            if (FAILED(rc)) throw rc;
+            floppiesTemp.push_back(med);
         }
 
         settings::DHCPServersList dhcpServersTemp;
         {
-            AutoReadLock al(m->ollDHCPServers.getLockHandle() COMMA_LOCKVAL_SRC_POS);
+            AutoReadLock dhcpLock(m->ollDHCPServers.getLockHandle() COMMA_LOCKVAL_SRC_POS);
             for (DHCPServersOList::const_iterator it = m->ollDHCPServers.begin();
                  it != m->ollDHCPServers.end();
@@ -3333 +3399 @@
         }
 
-        /* finally, lock VirtualBox object for writing */
-        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
         /* now copy the temp data to the config file under the VirtualBox lock */
         m->pMainConfigFile->llMachines = machinesTemp;
@@ -3648 +3711 @@
     if (pfNeedsSaveSettings)
         *pfNeedsSaveSettings = true;
-
-    return rc;
-}
-
-/**
- * Helper to update the global settings file when the name of some machine
- * changes so that file and directory renaming occurs. This method ensures that
- * all affected paths in the disk registry are properly updated.
- *
- * @param aOldPath  Old path (full).
- * @param aNewPath  New path (full).
- *
- * @note Locks this object + DVD, Floppy and HardDisk children for writing.
- */
-HRESULT VirtualBox::updatePathInMediaRegistry(const char *aOldPath, const char *aNewPath)
-{
-    LogFlowThisFunc(("aOldPath={%s} aNewPath={%s}\n", aOldPath, aNewPath));
-
-    AssertReturn(aOldPath, E_INVALIDARG);
-    AssertReturn(aNewPath, E_INVALIDARG);
-
-    AutoCaller autoCaller(this);
-    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
-
-    RWLockHandle tmpLock(LOCKCLASS_LISTOFMEDIA);        // not really necessary but the ObjectsList<> constructor wants one
-    ObjectsList<Medium> ollAll(tmpLock);
-    ollAll.appendOtherList(m->ollDVDImages);
-    ollAll.appendOtherList(m->ollFloppyImages);
-    ollAll.appendOtherList(m->ollHardDisks);
-
-    for (MediaList::iterator it = ollAll.begin();
-         it != ollAll.end();
-         ++ it)
-    {
-        (*it)->updatePath(aOldPath, aNewPath);
-    }
-
-    HRESULT rc = saveSettings();
 
     return rc;

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

@@ -718 +718 @@
     };
 
-    HRESULT prepareSaveSettings(bool &aRenamed, bool &aNew);
-    HRESULT saveSettings(int aFlags = 0);
+    HRESULT prepareSaveSettings(bool *pfNeedsGlobalSaveSettings);
+    HRESULT saveSettings(bool *pfNeedsGlobalSaveSettings, int aFlags = 0);
 
     HRESULT saveAllSnapshots();

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

@@ -278 +278 @@
     HRESULT unregisterImage(Medium *aImage, DeviceType_T argType, bool *pfNeedsSaveSettings);
 
+    void rememberMachineNameChangeForMedia(const Utf8Str &strOldConfigDir,
+                                           const Utf8Str &strNewConfigDir);
+
     HRESULT saveSettings();
-    HRESULT updatePathInMediaRegistry(const char *aOldPath, const char *aNewPath);
 
     static HRESULT ensureFilePathExists(const Utf8Str &strFileName);