Changeset 55728 in vbox for trunk/src/VBox/Main/src-server

Timestamp:

May 7, 2015 1:58:38 PM (10 years ago)

Author:

vboxsync

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

100140

Message:

Main/Machine+Snapshot+Medium: Remove bogus assertion when locking a machine for shared access. Fix a race between querying medium information and deleting the medium, which was triggered by the GUI medium thread querying the same medium which restoring a snapshot wanted to delete (timing sensitive, happened most with poweroff/restore snapshot). Additionally move the machine state update for restoring a snapshot to the end, when the operation is pretty much done. For taking a snapshot, make sure that the corresponding event is signalled after the operation completed.

Location:

trunk/src/VBox/Main/src-server

Files:

• MachineImpl.cpp (modified) (3 diffs)
• MediumImpl.cpp (modified) (1 diff)
• SnapshotImpl.cpp (modified) (5 diffs)

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

@@ -3208 +3208 @@
 HRESULT Machine::lockMachine(const ComPtr<ISession> &aSession,
                              LockType_T aLockType)
-
 {
     /* check the session state */
@@ -3260 +3259 @@
 
         // copy pointers to W (the write-locking session) before leaving lock (these must not be NULL)
-        ComAssertRet(mData->mSession.mLockType == LockType_Write || mData->mSession.mLockType == LockType_VM, E_FAIL);
         ComPtr<IInternalSessionControl> pSessionW = mData->mSession.mDirectControl;
         ComAssertRet(!pSessionW.isNull(), E_FAIL);
@@ -10008 +10006 @@
             // a saved state in the current state, so keep everything as is
          || (    (   mData->mMachineState == MachineState_Snapshotting
-                  || mData->mMachineState == MachineState_DeletingSnapshot)
+                  || mData->mMachineState == MachineState_DeletingSnapshot
+                  || mData->mMachineState == MachineState_RestoringSnapshot)
               && (!mSSData->strStateFilePath.isEmpty())
             )

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

@@ -4647 +4647 @@
                            tr("Medium format '%s' does not support storage deletion"),
                            m->strFormat.c_str());
+
+        /* Wait for a concurrently running Medium::i_queryInfo to complete. */
+        /** @todo r=klaus would be great if this could be moved to the async
+         * part of the operation as it can take quite a while */
+        if (m->queryInfoRunning)
+        {
+            while (m->queryInfoRunning)
+            {
+                multilock.release();
+                /* Must not hold the media tree lock or the object lock, as
+                 * Medium::i_queryInfo needs this lock and thus we would run
+                 * into a deadlock here. */
+                Assert(!m->pVirtualBox->i_getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+                Assert(!isWriteLockOnCurrentThread());
+                {
+                    AutoReadLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
+                }
+                multilock.acquire();
+            }
+        }
 
         /* Note that we are fine with Inaccessible state too: a) for symmetry

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

@@ -1796 +1796 @@
     task.m_pProgress->i_notifyComplete(rc);
 
+    if (SUCCEEDED(rc))
+        mParent->i_onSnapshotTaken(mData->mUuid,
+                                   task.m_pSnapshot->i_getId());
     LogFlowThisFuncLeave();
 }
@@ -1864 +1867 @@
         /* associate old hard disks with the snapshot and do locking/unlocking*/
         i_commitMedia(task.m_fTakingSnapshotOnline);
-
-        /* inform callbacks */
-        mParent->i_onSnapshotTaken(mData->mUuid,
-                                   task.m_pSnapshot->i_getId());
         alock.release();
     }
@@ -2039 +2038 @@
 
     HRESULT rc = S_OK;
-
-    bool stateRestored = false;
 
     try
@@ -2161 +2158 @@
          * state accordingly no matter of the delete snapshot result */
         if (mSSData->strStateFilePath.isNotEmpty())
-            i_setMachineState(MachineState_Saved);
+            task.modifyBackedUpState(MachineState_Saved);
         else
-            i_setMachineState(MachineState_PoweredOff);
-
-        stateRestored = true;
+            task.modifyBackedUpState(MachineState_PoweredOff);
 
         /* Paranoia: no one must have saved the settings in the mean time. If
@@ -2252 +2247 @@
         i_rollback(false /* aNotify */);
 
-        if (!stateRestored)
-        {
-            /* restore the machine state */
-            i_setMachineState(task.m_machineStateBackup);
-        }
     }
 
     mParent->i_saveModifiedRegistries();
+
+    /* restore the machine state */
+    i_setMachineState(task.m_machineStateBackup);
 
     /* set the result (this will try to fetch current error info on failure) */