Changeset 22228 in vbox

Timestamp:

Aug 13, 2009 10:26:00 AM (16 years ago)

Author:

vboxsync

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

51030

Message:

Main: fix both SetExtraData() implementations to call onExtraDataCanChange() outside of any locks

Location:

trunk/src/VBox/Main

Files:

• MachineImpl.cpp (modified) (3 diffs)
• VirtualBoxImpl.cpp (modified) (3 diffs)

trunk/src/VBox/Main/MachineImpl.cpp

@@ -2645 +2645 @@
     CheckComArgNotNull(aKey);
 
-    AutoCaller autoCaller (this);
+    AutoCaller autoCaller(this);
     CheckComRCReturnRC (autoCaller.rc());
 
-    Bstr val;
-    if (!aValue)
-        val = (const char *)"";
-    else
-        val = aValue;
-
     Utf8Str strKey(aKey);
-    Bstr oldVal("");
-
-    /* VirtualBox::onExtraDataCanChange() and saveSettings() need mParent
-     * lock (saveSettings() needs a write one). This object's write lock is
-     * also necessary to serialize file access (prevent concurrent reads and
-     * writes). */
-    AutoMultiWriteLock2 alock(mParent, this);
-
-    if (mType == IsSnapshotMachine)
-    {
-        HRESULT rc = checkStateDependency(MutableStateDep);
-        CheckComRCReturnRC (rc);
-    }
-
-    settings::ExtraDataItemsMap::const_iterator it = mData->m_pMachineConfigFile->mapExtraDataItems.find(strKey);
-    if (it != mData->m_pMachineConfigFile->mapExtraDataItems.end())
-    {
-        // key exists:
-        const Utf8Str &strValue = it->second;
-        oldVal = strValue;
-    }
-
-    bool fChanged = (oldVal != val);
-    if (fChanged)
-    {
-        /* ask for permission from all listeners */
+    Utf8Str strValue(aValue);
+    Utf8Str strOldValue;            // empty
+
+    // locking note: we only hold the read lock briefly to look up the old value,
+    // then release it and call the onExtraCanChange callbacks. There is a small
+    // chance of a race insofar as the callback might be called twice if two callers
+    // change the same key at the same time, but that's a much better solution
+    // than the deadlock we had here before. The actual changing of the extradata
+    // is then performed under the write lock and race-free.
+
+    // look up the old value first; if nothing's changed then we need not do anything
+    {
+        AutoReadLock alock(this); // hold read lock only while looking up
+        settings::ExtraDataItemsMap::const_iterator it = mData->m_pMachineConfigFile->mapExtraDataItems.find(strKey);
+        if (it != mData->m_pMachineConfigFile->mapExtraDataItems.end())
+            strOldValue = it->second;
+    }
+
+    bool fChanged;
+    if (fChanged = (strOldValue != strValue))
+    {
+        // ask for permission from all listeners outside the locks;
+        // onExtraDataCanChange() only briefly requests the VirtualBox
+        // lock to copy the list of callbacks to invoke
         Bstr error;
-        if (!mParent->onExtraDataCanChange(mData->mUuid, aKey, val, error))
+        Bstr bstrValue;
+        if (aValue)
+            bstrValue = aValue;
+        else
+            bstrValue = (const char *)"";
+
+        if (!mParent->onExtraDataCanChange(mData->mUuid, aKey, bstrValue, error))
         {
             const char *sep = error.isEmpty() ? "" : ": ";
@@ -2691 +2689 @@
                             tr("Could not set extra data because someone refused the requested change of '%ls' to '%ls'%s%ls"),
                             aKey,
-                            val.raw(),
+                            bstrValue.raw(),
                             sep,
                             err);
         }
 
-        mData->m_pMachineConfigFile->mapExtraDataItems[strKey] = val;
+        // data is changing and change not vetoed: then write it out under the locks
+
+        /* VirtualBox::onExtraDataCanChange() and saveSettings() need mParent
+         * lock (saveSettings() needs a write one). This object's write lock is
+         * also necessary to serialize file access (prevent concurrent reads and
+         * writes). */
+        AutoMultiWriteLock2 alock(mParent, this);
+
+        if (mType == IsSnapshotMachine)
+        {
+            HRESULT rc = checkStateDependency(MutableStateDep);
+            CheckComRCReturnRC (rc);
+        }
+
+        mData->m_pMachineConfigFile->mapExtraDataItems[strKey] = strValue;
+                // creates a new key if needed
 
         /* save settings on success */
@@ -2703 +2716 @@
     }
 
-    /* fire a notification */
+    // fire notification outside the lock
     if (fChanged)
         mParent->onExtraDataChange(mData->mUuid, aKey, aValue);

trunk/src/VBox/Main/VirtualBoxImpl.cpp

@@ -1511 +1511 @@
     CheckComRCReturnRC(autoCaller.rc());
 
-    Guid emptyGuid;
-    Bstr val;
-    if (!aValue)
-        val = (const char *)"";
-    else
-        val = aValue;
-
     Utf8Str strKey(aKey);
-    Bstr oldVal("");
-
-    /* serialize file access (prevent concurrent reads and writes) */
-    AutoWriteLock alock(this);
-
-    settings::ExtraDataItemsMap::const_iterator it = m_pMainConfigFile->mapExtraDataItems.find(strKey);
-    if (it != m_pMainConfigFile->mapExtraDataItems.end())
-    {
-        // key exists:
-        const Utf8Str &strValue = it->second;
-        oldVal = strValue;
-    }
-
-    bool fChanged = (oldVal != val);
-    if (fChanged)
-    {
-        /* ask for permission from all listeners */
+    Utf8Str strValue(aValue);
+    Utf8Str strOldValue;            // empty
+
+    // locking note: we only hold the read lock briefly to look up the old value,
+    // then release it and call the onExtraCanChange callbacks. There is a small
+    // chance of a race insofar as the callback might be called twice if two callers
+    // change the same key at the same time, but that's a much better solution
+    // than the deadlock we had here before. The actual changing of the extradata
+    // is then performed under the write lock and race-free.
+
+    // look up the old value first; if nothing's changed then we need not do anything
+    {
+        AutoReadLock alock(this); // hold read lock only while looking up
+        settings::ExtraDataItemsMap::const_iterator it = m_pMainConfigFile->mapExtraDataItems.find(strKey);
+        if (it != m_pMainConfigFile->mapExtraDataItems.end())
+            strOldValue = it->second;
+    }
+
+    bool fChanged;
+    if (fChanged = (strOldValue != strValue))
+    {
+        // ask for permission from all listeners outside the locks;
+        // onExtraDataCanChange() only briefly requests the VirtualBox
+        // lock to copy the list of callbacks to invoke
         Bstr error;
-        if (!onExtraDataCanChange(Guid::Empty, aKey, val, error))
+        Bstr bstrValue;
+        if (aValue)
+            bstrValue = aValue;
+        else
+            bstrValue = (const char *)"";
+
+        if (!onExtraDataCanChange(Guid::Empty, aKey, bstrValue, error))
         {
             const char *sep = error.isEmpty() ? "" : ": ";
@@ -1546 +1552 @@
                             tr("Could not set extra data because someone refused the requested change of '%ls' to '%ls'%s%ls"),
                             aKey,
-                            val.raw(),
+                            bstrValue.raw(),
                             sep,
                             err);
         }
 
-        m_pMainConfigFile->mapExtraDataItems[strKey] = val;
+        // data is changing and change not vetoed: then write it out under the lock
+
+        AutoWriteLock alock(this);
+
+        m_pMainConfigFile->mapExtraDataItems[strKey] = strValue;
+                // creates a new key if needed
 
         /* save settings on success */
@@ -1558 +1569 @@
     }
 
-    /* fire a notification */
+    // fire notification outside the lock
     if (fChanged)
         onExtraDataChange(Guid::Empty, aKey, aValue);