Changeset 91743 in vbox for trunk

Timestamp:

Oct 14, 2021 8:04:27 PM (3 years ago)

Author:

vboxsync

Message:

Main/Machine: Release lock while waiting in Machine::i_ensureNoStateDependencies. Needs passing the lock from the caller (and a bit of multiple lock untangling). Regression introduced in r76471. bugref:10121

Location:

trunk/src/VBox/Main

Files:

• include/MachineImpl.h (modified) (2 diffs)
• src-server/MachineImpl.cpp (modified) (17 diffs)
• src-server/MachineImplCloneVM.cpp (modified) (1 diff)
• src-server/RecordingSettingsImpl.cpp (modified) (2 diffs)
• src-server/SnapshotImpl.cpp (modified) (6 diffs)
• src-server/VirtualBoxImpl.cpp (modified) (1 diff)

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

@@ -632 +632 @@
     Machine *i_getMachine();
 
-    void i_ensureNoStateDependencies();
+    void i_ensureNoStateDependencies(AutoWriteLock &alock);
 
     virtual HRESULT i_setMachineState(MachineState_T aMachineState);
@@ -682 +682 @@
 
     HRESULT i_prepareSaveSettings(bool *pfNeedsGlobalSaveSettings);
-    HRESULT i_saveSettings(bool *pfNeedsGlobalSaveSettings, int aFlags = 0);
+    HRESULT i_saveSettings(bool *pfNeedsGlobalSaveSettings, AutoWriteLock &alock, int aFlags = 0);
 
     void i_copyMachineDataToSettings(settings::MachineConfigFile &config);

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

@@ -829 +829 @@
         {
             AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-            i_saveSettings(NULL, Machine::SaveS_Force);
+            i_saveSettings(NULL, alock, Machine::SaveS_Force);
         }
     }
@@ -1610 +1610 @@
     /** Save settings if online - @todo why is this required? -- @bugref{6818} */
     if (Global::IsOnline(mData->mMachineState))
-        i_saveSettings(NULL);
+        i_saveSettings(NULL, alock);
 
     return S_OK;
@@ -2664 +2664 @@
     /** Save settings if online - @todo why is this required? -- @bugref{6818} */
     if (Global::IsOnline(mData->mMachineState))
-        i_saveSettings(NULL);
+        i_saveSettings(NULL, alock);
 
     return S_OK;
@@ -2695 +2695 @@
     /** Save settings if online - @todo why is this required? -- @bugref{6818} */
     if (Global::IsOnline(mData->mMachineState))
-        i_saveSettings(NULL);
+        i_saveSettings(NULL, alock);
 
     return S_OK;
@@ -2727 +2727 @@
     /** Save settings if online - @todo why is this required? -- @bugref{6818} */
     if (Global::IsOnline(mData->mMachineState))
-        i_saveSettings(NULL);
+        i_saveSettings(NULL, alock);
 
     return S_OK;
@@ -4801 +4801 @@
         // extradata items at all (unlike all other settings), so the old/new
         // settings comparison would give a wrong result!
-        i_saveSettings(&fNeedsGlobalSaveSettings, SaveS_Force);
+        i_saveSettings(&fNeedsGlobalSaveSettings, alock, SaveS_Force);
 
         if (fNeedsGlobalSaveSettings)
@@ -4838 +4838 @@
     /* save all VM data excluding snapshots */
     bool fNeedsGlobalSaveSettings = false;
-    rc = i_saveSettings(&fNeedsGlobalSaveSettings);
+    rc = i_saveSettings(&fNeedsGlobalSaveSettings, mlock);
     mlock.release();
 
@@ -4931 +4931 @@
 
     // wait for state dependents to drop to zero
-    i_ensureNoStateDependencies();
+    i_ensureNoStateDependencies(alock);
 
     if (!mData->mAccessible)
@@ -6411 +6411 @@
     /** Save settings if online - @todo why is this required? -- @bugref{6818} */
     if (Global::IsOnline(mData->mMachineState))
-        i_saveSettings(NULL);
+        i_saveSettings(NULL, alock);
 
     return S_OK;
@@ -6448 +6448 @@
     /** Save settings if online - @todo why is this required? -- @bugref{6818} */
     if (Global::IsOnline(mData->mMachineState))
-        i_saveSettings(NULL);
+        i_saveSettings(NULL, alock);
 
     return S_OK;
@@ -7883 +7883 @@
 
     /* wait for state dependents to drop to zero */
-    i_ensureNoStateDependencies();
+    i_ensureNoStateDependencies(alock);
 
     if (!mData->mAccessible)
@@ -7907 +7907 @@
        )
     {
-        rc = i_saveSettings(NULL);
+        rc = i_saveSettings(NULL, alock);
                 // no need to check whether VirtualBox.xml needs saving too since
                 // we can't have a machine XML file rename pending
@@ -8420 +8420 @@
  * control to the caller.
  *
- * @note Locks this object for writing. The lock will be released while waiting
- *       (if necessary).
+ * @note Receives a lock to this object for writing. The lock will be released
+ *       while waiting (if necessary).
  *
  * @warning To be used only in methods that change the machine state!
  */
-void Machine::i_ensureNoStateDependencies()
+void Machine::i_ensureNoStateDependencies(AutoWriteLock &alock)
 {
     AssertReturnVoid(isWriteLockOnCurrentThread());
-
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
     /* Wait for all state dependents if necessary */
@@ -8477 +8475 @@
 
     /* wait for state dependents to drop to zero */
-    i_ensureNoStateDependencies();
+    i_ensureNoStateDependencies(alock);
 
     MachineState_T const enmOldState = mData->mMachineState;
@@ -9866 +9864 @@
  */
 HRESULT Machine::i_saveSettings(bool *pfNeedsGlobalSaveSettings,
+                                AutoWriteLock &alock,
                                 int  aFlags /*= 0*/)
 {
@@ -9874 +9873 @@
     /* make sure child objects are unable to modify the settings while we are
      * saving them */
-    i_ensureNoStateDependencies();
+    i_ensureNoStateDependencies(alock);
 
     AssertReturn(!i_isSnapshotMachine(),
@@ -12999 +12998 @@
 
             /* save all VM settings */
-            rc = i_saveSettings(NULL);
+            rc = i_saveSettings(NULL, alock);
                     // no need to check whether VirtualBox.xml needs saving also since
                     // we can't have a name change pending at this point

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

@@ -1600 +1600 @@
             /* save all VM data */
             bool fNeedsGlobalSaveSettings = false;
-            rc = d->pTrgMachine->i_saveSettings(&fNeedsGlobalSaveSettings, Machine::SaveS_Force);
+            rc = d->pTrgMachine->i_saveSettings(&fNeedsGlobalSaveSettings, trgLock, Machine::SaveS_Force);
             if (FAILED(rc)) throw rc;
             /* Release all locks */

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

@@ -239 +239 @@
         else
         {
-            AutoWriteLock mlock(m->pMachine COMMA_LOCKVAL_SRC_POS);  // mParent is const, needs no locking
+            AutoWriteLock mlock(m->pMachine COMMA_LOCKVAL_SRC_POS);  // pMachine is const, needs no locking
             m->pMachine->i_setModified(Machine::IsModified_Recording);
 
@@ -248 +248 @@
             /** Save settings if online - @todo why is this required? -- @bugref{6818} */
             if (Global::IsOnline(m->pMachine->i_getMachineState()))
-                rc = m->pMachine->i_saveSettings(NULL);
+                rc = m->pMachine->i_saveSettings(NULL, mlock);
         }
     }

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

@@ -1395 +1395 @@
 HRESULT SnapshotMachine::i_onSnapshotChange(Snapshot *aSnapshot)
 {
-    AutoMultiWriteLock2 mlock(this, aSnapshot COMMA_LOCKVAL_SRC_POS);
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+    AutoWriteLock slock(aSnapshot COMMA_LOCKVAL_SRC_POS);
     Guid uuidMachine(mData->mUuid),
          uuidSnapshot(aSnapshot->i_getId());
@@ -1404 +1405 @@
      * related to the current state. */
     mMachine->i_setModified(Machine::IsModified_Snapshots, false /* fAllowStateModification */);
+    slock.release();
     HRESULT rc = mMachine->i_saveSettings(&fNeedsGlobalSaveSettings,
+                                          alock,
                                           SaveS_Force);        // we know we need saving, no need to check
-    mlock.release();
+    alock.release();
 
     if (SUCCEEDED(rc) && fNeedsGlobalSaveSettings)
@@ -1754 +1757 @@
         /* save settings to ensure current changes are committed and
          * hard disks are fixed up */
-        rc = i_saveSettings(NULL);
+        rc = i_saveSettings(NULL, alock);
             // no need to check for whether VirtualBox.xml needs changing since
             // we can't have a machine XML rename pending at this point
@@ -2109 +2112 @@
             flSaveSettings |= SaveS_ResetCurStateModified;
 
-        rc = i_saveSettings(NULL, flSaveSettings);
+        rc = i_saveSettings(NULL, alock, flSaveSettings);
     }
 
@@ -2508 +2511 @@
         // save machine settings, reset the modified flag and commit;
         bool fNeedsGlobalSaveSettings = false;
-        rc = i_saveSettings(&fNeedsGlobalSaveSettings,
+        rc = i_saveSettings(&fNeedsGlobalSaveSettings, alock,
                             SaveS_ResetCurStateModified);
         if (FAILED(rc))
@@ -2720 +2723 @@
         if (mData->flModifications)
         {
-            rc = i_saveSettings(NULL);
+            snapshotLock.release();
+            rc = i_saveSettings(NULL, alock);
+            snapshotLock.acquire();
                 // no need to change for whether VirtualBox.xml needs saving since
                 // we can't have a machine XML rename pending at this point

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

@@ -5480 +5480 @@
                     continue;
                 AutoWriteLock mlock(pMachine COMMA_LOCKVAL_SRC_POS);
-                rc = pMachine->i_saveSettings(&fNeedsGlobalSettings,
+                rc = pMachine->i_saveSettings(&fNeedsGlobalSettings, mlock,
                                               Machine::SaveS_Force);           // caller said save, so stop arguing
             }