Changeset 55511 in vbox for trunk/src

Timestamp:

Apr 29, 2015 10:16:01 AM (10 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

99867

Message:

Main/Snapshot: fix handling of saved state with offline snapshots (looked at wrong machine state variable, regression in previous big code structure change), streamline the code path and fix a crash for early failures to take a snapshot. Eliminate a local variable which should always have the same value as the same flag in the task struct, avoiding inconsistent behavior.
Main/Machine: fix sanity check regarding saved state presence when saving the config (resulted in loss of saved state)

Location:

trunk/src/VBox/Main/src-server

Files:

• MachineImpl.cpp (modified) (1 diff)
• SnapshotImpl.cpp (modified) (9 diffs)

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

@@ -9995 +9995 @@
     if (    mData->mMachineState == MachineState_Saved
          || mData->mMachineState == MachineState_Restoring
-            // when deleting a snapshot we may or may not have a saved state in the current state,
-            // so let's not assert here please
-         || (    (   mData->mMachineState == MachineState_DeletingSnapshot
-                  || mData->mMachineState == MachineState_DeletingSnapshotOnline
-                  || mData->mMachineState == MachineState_DeletingSnapshotPaused)
+            // when doing certain snapshot operations we may or may not have
+            // a saved state in the current state, so keep everything as is
+         || (    (   mData->mMachineState == MachineState_Snapshotting
+                  || mData->mMachineState == MachineState_DeletingSnapshot)
               && (!mSSData->strStateFilePath.isEmpty())
             )

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

@@ -1555 +1555 @@
     {
         // @todo: at this point we have to be in the right state!!!!
-        AssertStmt(   !Global::IsOnlineOrTransient(mData->mMachineState)
-                   || mData->mMachineState == MachineState_Snapshotting
+        AssertStmt(   mData->mMachineState == MachineState_Snapshotting
                    || mData->mMachineState == MachineState_OnlineSnapshotting
                    || mData->mMachineState == MachineState_LiveSnapshotting, throw E_FAIL);
@@ -1570 +1569 @@
         }
 
-        if (    !task.m_fTakingSnapshotOnline
-             && mData->mMachineState != MachineState_Saved)
-        {
-            /* save settings to ensure current changes are committed and
-             * hard disks are fixed up */
-            rc = i_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))
-                throw rc;
-        }
+        /* save settings to ensure current changes are committed and
+         * hard disks are fixed up */
+        rc = i_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))
+            throw rc;
 
         /* task.m_strStateFilePath is "" when the machine is offline or saved */
@@ -1592 +1587 @@
                 i_composeSavedStateFilename(task.m_strStateFilePath);
         }
-        else if (mData->mMachineState == MachineState_Saved)
+        else if (task.m_machineStateBackup == MachineState_Saved)
             // taking an offline snapshot from machine in "saved" state: use existing state file
             task.m_strStateFilePath = mSSData->strStateFilePath;
@@ -1652 +1647 @@
 
         fBeganTakingSnapshot = true;
-
-        /* Check sanity: for offline snapshots there must not be a saved state
-         * file name. All other combinations are valid (though online snapshots
-         * without saved state file seems inconsistent - there are exotic use
-         * cases, which need to be explicitly enabled, see the code above. */
-        if (   !task.m_fTakingSnapshotOnline
-            && !task.m_strStateFilePath.isEmpty())
-            throw setError(E_FAIL, "Invalid state of saved state file");
 
         // STEP 3: save the VM state (if online)
@@ -1729 +1716 @@
         // have to postpone this to the end as i_finishTakingSnapshot() needs
         // it for various cleanup steps
-        task.m_pSnapshot->uninit();
-        task.m_pSnapshot.setNull();
+        if (task.m_pSnapshot)
+        {
+            task.m_pSnapshot->uninit();
+            task.m_pSnapshot.setNull();
+        }
     }
     Assert(alock.isWriteLockOnCurrentThread());
@@ -1845 +1835 @@
     ComObjPtr<Snapshot> pOldCurrentSnap = mData->mCurrentSnapshot;
 
-    bool fOnline = Global::IsOnline(task.m_machineStateBackup);
-
     HRESULT rc = S_OK;
 
@@ -1860 +1848 @@
         int flSaveSettings = SaveS_Force;       // do not do a deep compare in machine settings,
                                                 // snapshots change, so we know we need to save
-        if (!fOnline)
+        if (!task.m_fTakingSnapshotOnline)
             /* the machine was powered off or saved when taking a snapshot, so
              * reset the mCurrentStateModified flag */
@@ -1871 +1859 @@
     {
         /* associate old hard disks with the snapshot and do locking/unlocking*/
-        i_commitMedia(fOnline);
+        i_commitMedia(task.m_fTakingSnapshotOnline);
 
         /* inform callbacks */
@@ -1890 +1878 @@
 
         // delete the saved state file (it might have been already created)
-        if (fOnline)
+        if (task.m_fTakingSnapshotOnline)
             // no need to test for whether the saved state file is shared: an online
             // snapshot means that a new saved state file was created, which we must