Changeset 29220 in vbox

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

Files:

• HostServices/GuestControl/service.cpp (modified) (13 diffs)
• Main/GuestImpl.cpp (modified) (12 diffs)
• Main/include/GuestImpl.h (modified) (1 diff)

trunk/src/VBox/HostServices/GuestControl/service.cpp

@@ -28 +28 @@
 
 #include <VBox/log.h>
-#include <iprt/asm.h>
+#include <iprt/asm.h> /* For ASMBreakpoint(). */
 #include <iprt/assert.h>
 #include <iprt/cpp/autores.h>
@@ -65 +65 @@
 struct GuestCall
 {
+    /** Client ID; a client can have multiple handles! */
+    uint32_t mClientID;
     /** The call handle */
     VBOXHGCMCALLHANDLE mHandle;
@@ -73 +75 @@
 
     /** The standard constructor */
-    GuestCall() : mHandle(0), mParms(NULL), mNumParms(0) {}
+    GuestCall() : mClientID(0), mHandle(0), mParms(NULL), mNumParms(0) {}
     /** The normal contructor */
-    GuestCall(VBOXHGCMCALLHANDLE aHandle, VBOXHGCMSVCPARM aParms[], int cParms)
-              : mHandle(aHandle), mParms(aParms), mNumParms(cParms) {}
+    GuestCall(uint32_t aClientID, VBOXHGCMCALLHANDLE aHandle, 
+              VBOXHGCMSVCPARM aParms[], uint32_t cParms)
+              : mClientID(aClientID), mHandle(aHandle), mParms(aParms), mNumParms(cParms) {}
 };
 /** The guest call list type */
-typedef std::list <GuestCall> CallList;
+typedef std::list< GuestCall > CallList;
+typedef std::list< GuestCall >::iterator CallListIter;
+typedef std::list< GuestCall >::const_iterator CallListIterConst;
 
 /**
@@ -91 +96 @@
     /** HGCM helper functions. */
     PVBOXHGCMSVCHELPERS mpHelpers;
-    /** @todo we should have classes for thread and request handler thread */
-    /** Queue of outstanding property change notifications */
-    RTREQQUEUE *mReqQueue;
-    /** Request that we've left pending in a call to flushNotifications. */
-    PRTREQ mPendingDummyReq;
-    /** Thread for processing the request queue */
-    RTTHREAD mReqThread;
-    /** Tell the thread that it should exit */
-    bool volatile mfExitThread;
     /** Callback function supplied by the host for notification of updates
      * to properties */
@@ -113 +109 @@
     explicit Service(PVBOXHGCMSVCHELPERS pHelpers)
         : mpHelpers(pHelpers)
-        , mPendingDummyReq(NULL)
-        , mfExitThread(false)
         , mpfnHostCallback(NULL)
         , mpvHostData(NULL)
     {
-        int rc = RTReqCreateQueue(&mReqQueue);
-#ifndef VBOX_GUEST_CTRL_TEST_NOTHREAD
-        if (RT_SUCCESS(rc))
-            rc = RTThreadCreate(&mReqThread, reqThreadFn, this, 0,
-                                RTTHREADTYPE_MSG_PUMP, RTTHREADFLAGS_WAITABLE,
-                                "GuestCtrlReq");
-#endif
-        if (RT_FAILURE(rc))
-            throw rc;
     }
 
@@ -231 +216 @@
     int paramBufferAssign(PVBOXGUESTCTRPARAMBUFFER pBuf, uint32_t cParms, VBOXHGCMSVCPARM paParms[]);
     int prepareExecute(uint32_t cParms, VBOXHGCMSVCPARM paParms[]);
-    static DECLCALLBACK(int) reqThreadFn(RTTHREAD ThreadSelf, void *pvUser);
     int clientConnect(uint32_t u32ClientID, void *pvClient);
     int clientDisconnect(uint32_t u32ClientID, void *pvClient);
     int sendHostCmdToGuest(HostCmd *pCmd, VBOXHGCMCALLHANDLE callHandle, uint32_t cParms, VBOXHGCMSVCPARM paParms[]);
-    int retrieveNextHostCmd(VBOXHGCMCALLHANDLE callHandle, uint32_t cParms, VBOXHGCMSVCPARM paParms[]);
+    int retrieveNextHostCmd(uint32_t u32ClientID, VBOXHGCMCALLHANDLE callHandle, uint32_t cParms, VBOXHGCMSVCPARM paParms[]);
     int notifyHost(uint32_t eFunction, uint32_t cParms, VBOXHGCMSVCPARM paParms[]);
     int processHostCmd(uint32_t eFunction, uint32_t cParms, VBOXHGCMSVCPARM paParms[]);
@@ -246 +230 @@
 
 
-/**
- * Thread function for processing the request queue
- * @copydoc FNRTTHREAD
- */
-/* static */
-DECLCALLBACK(int) Service::reqThreadFn(RTTHREAD ThreadSelf, void *pvUser)
-{
-    SELF *pSelf = reinterpret_cast<SELF *>(pvUser);
-    while (!pSelf->mfExitThread)
-        RTReqProcess(pSelf->mReqQueue, RT_INDEFINITE_WAIT);
-    return VINF_SUCCESS;
-}
-
 /** @todo Write some nice doc headers! */
 /* Stores a HGCM request in an internal buffer (pEx). Needs to be freed later using execBufferFree(). */
@@ -265 +236 @@
     AssertPtr(pBuf);
     int rc = VINF_SUCCESS;
+
+    /* Paranoia. */
+    if (cParms > 256)
+        cParms = 256;
 
     /*
@@ -392 +367 @@
 {
     LogFlowFunc(("Client (%ld) disconnected\n", u32ClientID));
+    /*
+     * Throw out all stale clients.
+     */
+    CallListIter it = mClientList.begin();
+    while (it != mClientList.end())
+    {
+        if (it->mClientID == u32ClientID)
+            it = mClientList.erase(it);
+        else
+            it++;
+    }
     return VINF_SUCCESS;
 }
@@ -424 +410 @@
  * defer the guest call until we have something from the host.
  */
-int Service::retrieveNextHostCmd(VBOXHGCMCALLHANDLE callHandle, uint32_t cParms, VBOXHGCMSVCPARM paParms[])
+int Service::retrieveNextHostCmd(uint32_t u32ClientID, VBOXHGCMCALLHANDLE callHandle, 
+                                 uint32_t cParms, VBOXHGCMSVCPARM paParms[])
 {
     int rc = VINF_SUCCESS;
@@ -435 +422 @@
     if (mHostCmds.empty()) /* If command list is empty, defer ... */
     {
-        mClientList.push_back(GuestCall(callHandle, paParms, cParms));
+        mClientList.push_back(GuestCall(u32ClientID, callHandle, paParms, cParms));
         rc = VINF_HGCM_ASYNC_EXECUTE;
     }
@@ -535 +522 @@
         if (!fProcessed)
         {
-            mHostCmds.push_back(newCmd);
-    
+            mHostCmds.push_back(newCmd);    
+#if 0
             /* Limit list size by deleting oldest element. */
             if (mHostCmds.size() > 256) /** @todo Use a define! */
                 mHostCmds.pop_front();
+#endif
         }
     }
@@ -568 +556 @@
             case GUEST_GET_HOST_MSG:
                 LogFlowFunc(("GUEST_GET_HOST_MSG\n"));
-                rc = retrieveNextHostCmd(callHandle, cParms, paParms);
+                rc = retrieveNextHostCmd(u32ClientID, callHandle, cParms, paParms);
                 break;
 

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