Changeset 29220 in vbox for trunk/src/VBox/Main

Timestamp:

May 7, 2010 3:09:53 PM (15 years ago)

Author:

vboxsync

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

61300

Message:

Guest Control/Main/HostService: Deal with stale clients, avoid deadlocks when doing massive output traffic.

Location:

trunk/src/VBox/Main

Files:

• GuestImpl.cpp (modified) (12 diffs)
• include/GuestImpl.h (modified) (1 diff)

trunk/src/VBox/Main/GuestImpl.cpp

@@ -117 +117 @@
     CallbackListIter it;
     for (it = mCallbackList.begin(); it != mCallbackList.end(); it++)
-        removeCtrlCallbackContext(it);
+        destroyCtrlCallbackContext(it);
 
     /* Clear process list. */
@@ -467 +467 @@
     int rc = VINF_SUCCESS;
 
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
     AssertPtr(pData);
     CallbackListIter it = getCtrlCallbackContextByID(pData->hdr.u32ContextID);
@@ -473 +475 @@
     if (it != mCallbackList.end())
     {
-        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
         PHOSTEXECCALLBACKDATA pCBData = (HOSTEXECCALLBACKDATA*)it->pvData;
         AssertPtr(pCBData);
@@ -581 +581 @@
     int rc = VINF_SUCCESS;
 
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
     AssertPtr(pData);
     CallbackListIter it = getCtrlCallbackContextByID(pData->hdr.u32ContextID);
@@ -589 +591 @@
         AssertPtr(pCBData);
 
-        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
         pCBData->u32PID = pData->u32PID;
         pCBData->u32HandleId = pData->u32HandleId;
@@ -601 +601 @@
             /* Allocate data buffer and copy it */
             pCBData->pvData = RTMemAlloc(pData->cbData);
+            pCBData->cbData = pData->cbData;
+
             AssertReturn(pCBData->pvData, VERR_NO_MEMORY);
             memcpy(pCBData->pvData, pData->pvData, pData->cbData);
-            pCBData->cbData = pData->cbData;
         }
         else
@@ -646 +647 @@
 }
 
-void Guest::removeCtrlCallbackContext(Guest::CallbackListIter it)
+/* No locking here; */
+void Guest::destroyCtrlCallbackContext(Guest::CallbackListIter it)
 {
     LogFlowFuncEnter();
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
     if (it->pvData)
     {
@@ -678 +679 @@
 {
     LogFlowFuncEnter();
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
     uint32_t uNewContext = ASMAtomicIncU32(&mNextContextID);
     if (uNewContext == UINT32_MAX)
         ASMAtomicUoWriteU32(&mNextContextID, 1000);
 
+    /** @todo Put this stuff into a constructor! */
     CallbackContext context;
     context.mContextID = uNewContext;
@@ -691 +692 @@
     context.pProgress = pProgress;
 
-    mCallbackList.push_back(context);
-    if (mCallbackList.size() > 256) /* Don't let the container size get too big! */
+    uint32_t nCallbacks;
+    {
+        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+        mCallbackList.push_back(context);
+        nCallbacks = mCallbackList.size();
+    }    
+
+#if 0
+    if (nCallbacks > 256) /* Don't let the container size get too big! */
     {
         Guest::CallbackListIter it = mCallbackList.begin();
-        removeCtrlCallbackContext(it);
-        mCallbackList.erase(it);
-    }
+        destroyCtrlCallbackContext(it);
+        {
+            AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+            mCallbackList.erase(it);
+        }
+    }
+#endif
 
     LogFlowFuncLeave();
@@ -823 +835 @@
                     paParms[i++].setUInt32(aTimeoutMS);
 
-                    /* Make sure mParent is valid, so set a read lock in this scope. */
-                    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-                    /* Forward the information to the VMM device. */
-                    AssertPtr(mParent);
-                    VMMDev *vmmDev = mParent->getVMMDev();
+                    VMMDev *vmmDev;
+                    {
+                        /* Make sure mParent is valid, so set the read lock while using. 
+                         * Do not keep this lock while doing the actual call, because in the meanwhile
+                         * another thread could request a write lock which would be a bad idea ... */
+                        AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
+    
+                        /* Forward the information to the VMM device. */
+                        AssertPtr(mParent);
+                        vmmDev = mParent->getVMMDev();
+                    }
+
                     if (vmmDev)
                     {
@@ -1065 +1083 @@
     
         {
-            /* Make sure mParent is valid, so set the read lock while using. */
-            AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-    
-            /* Forward the information to the VMM device. */
-            AssertPtr(mParent);
-            VMMDev *vmmDev = mParent->getVMMDev();
+            VMMDev *vmmDev;
+            {
+                /* Make sure mParent is valid, so set the read lock while using. 
+                 * Do not keep this lock while doing the actual call, because in the meanwhile
+                 * another thread could request a write lock which would be a bad idea ... */
+                AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
+        
+                /* Forward the information to the VMM device. */
+                AssertPtr(mParent);
+                vmmDev = mParent->getVMMDev();
+            }
+
             if (vmmDev)
             {
@@ -1171 +1195 @@
                     }
                 }
+
+                {
+                    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+                    destroyCtrlCallbackContext(it);
+                }
             }
             else /* PID lookup failed. */

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

@@ -151 +151 @@
     CallbackListIter getCtrlCallbackContextByID(uint32_t u32ContextID);
     GuestProcessIter getProcessByPID(uint32_t u32PID);
-    void removeCtrlCallbackContext(CallbackListIter it);
+    void destroyCtrlCallbackContext(CallbackListIter it);
     uint32_t addCtrlCallbackContext(eVBoxGuestCtrlCallbackType enmType, void *pvData, uint32_t cbData, Progress* pProgress);
 # endif