Changeset 75801 in vbox

Timestamp:

Nov 29, 2018 1:54:46 AM (6 years ago)

Author:

vboxsync

Message:

VBoxGuestControl: Optimizing message handling - part 2. bugref:9313

Location:

trunk

Files:

• include/VBox/HostServices/GuestControlSvc.h (modified) (1 diff)
• src/VBox/HostServices/GuestControl/service.cpp (modified) (2 diffs)
• src/VBox/Main/src-client/GuestCtrlImpl.cpp (modified) (5 diffs)
• src/VBox/Main/src-client/GuestCtrlPrivate.cpp (modified) (3 diffs)
• src/VBox/Main/src-client/GuestSessionImpl.cpp (modified) (1 diff)

trunk/include/VBox/HostServices/GuestControlSvc.h

@@ -253 +253 @@
 
 /**
- * The service functions which are called by guest. The numbers may not change,
- * so we hardcode them.
+ * The service functions which are called by guest.
+ *
+ * @note The function numbers cannot be changed.  Please use the first non-zero
+ *       number that's not in use when adding new functions.
+ *
+ * @note Remember to update service.cpp when adding new functions/events for
+ *       Main, as it validates all incoming commands before passing them on.
  */
 enum eGuestFn

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

@@ -2070 +2070 @@
 
         /*
-         * For all other regular commands we call our hostCallback
-         * function. If the current command does not support notifications,
-         * notifyHost will return VERR_NOT_SUPPORTED.
+         * Stuff the goes to various main objects:
          */
-        default:
+        case GUEST_DISCONNECTED:
+        case GUEST_MSG_REPLY:
+        case GUEST_MSG_PROGRESS_UPDATE:
+        case GUEST_SESSION_NOTIFY:
+        case GUEST_EXEC_OUTPUT:
+        case GUEST_EXEC_STATUS:
+        case GUEST_EXEC_INPUT_STATUS:
+        case GUEST_EXEC_IO_NOTIFY:
+        case GUEST_DIR_NOTIFY:
+        case GUEST_FILE_NOTIFY:
             rc = pThis->hostCallback(idFunction, cParms, paParms);
+            Assert(rc != VINF_HGCM_ASYNC_EXECUTE);
             break;
 
@@ -2101 +2109 @@
             LogFlowFunc(("[Client %RU32] GUEST_MSG_FILTER_UNSET\n", idClient));
             rc = VERR_NOT_IMPLEMENTED;
+            break;
+
+        /*
+         * Anything else shall return fail with invalid function.
+         *
+         * Note! We used to return VINF_SUCCESS for these.  See bugref:9313
+         *       and Guest::i_notifyCtrlDispatcher().
+         */
+        default:
+            ASSERT_GUEST_MSG_FAILED(("idFunction=%d (%#x)\n", idFunction, idFunction));
+            rc = VERR_INVALID_FUNCTION;
             break;
     }

trunk/src/VBox/Main/src-client/GuestCtrlImpl.cpp

@@ -45 +45 @@
 #include <iprt/path.h>
 #include <VBox/vmm/pgm.h>
+#include <VBox/AssertGuest.h>
 
 #include <memory>
@@ -67 +68 @@
  * @todo
  *
+ * @todo    r=bird: This code mostly returned VINF_SUCCESS with the comment
+ *          "Never return any errors back to the guest here." attached to the
+ *          return locations.  However, there is no explaination for this attitude
+ *          thowards error handling.   Further, it creates a slight problem since
+ *          the service would route all function calls it didn't recognize here,
+ *          thereby making any undefined functions confusingly return VINF_SUCCESS.
+ *
+ *          In my humble opinion, if the guest gives us incorrect input it should
+ *          expect and deal with error statuses.  If there is unimplemented
+ *          features I expect there to have been sufficient forethought by the
+ *          coder that these return sensible status codes.
+ *
+ *          It would be much appreciated if the esteemed card house builder could
+ *          please step in and explain this confusing state of affairs.
  */
 /* static */
 DECLCALLBACK(int) Guest::i_notifyCtrlDispatcher(void    *pvExtension,
-                                                uint32_t u32Function,
+                                                uint32_t idFunction,
                                                 void    *pvData,
                                                 uint32_t cbData)
@@ -80 +95 @@
      * changes to the object state.
      */
-    Log2Func(("pvExtension=%p, u32Function=%RU32, pvParms=%p, cbParms=%RU32\n",
-              pvExtension, u32Function, pvData, cbData));
+    Log2Func(("pvExtension=%p, idFunction=%RU32, pvParms=%p, cbParms=%RU32\n", pvExtension, idFunction, pvData, cbData));
 
     ComObjPtr<Guest> pGuest = reinterpret_cast<Guest *>(pvExtension);
-    Assert(!pGuest.isNull());
+    AssertReturn(pGuest.isNotNull(), VERR_WRONG_ORDER);
+
+    /*
+     * The data packet should ever be a problem, but check to be sure.
+     */
+    AssertMsgReturn(cbData == sizeof(VBOXGUESTCTRLHOSTCALLBACK),
+                    ("Guest control host callback data has wrong size (expected %zu, got %zu) - buggy host service!\n",
+                     sizeof(VBOXGUESTCTRLHOSTCALLBACK), cbData), VERR_INVALID_PARAMETER);
+    PVBOXGUESTCTRLHOSTCALLBACK pSvcCb = (PVBOXGUESTCTRLHOSTCALLBACK)pvData;
+    AssertPtrReturn(pSvcCb, VERR_INVALID_POINTER);
 
     /*
@@ -92 +115 @@
      * - Extract the session ID out of the context ID
      * - Dispatch the whole stuff to the appropriate session (if still exists)
+     *
+     * At least context ID parameter must always be present.
      */
-    if (cbData != sizeof(VBOXGUESTCTRLHOSTCALLBACK))
-    {
-        AssertMsgFailed(("Guest control host callback data has wrong size (expected %zu, got %zu)\n",
-                         sizeof(VBOXGUESTCTRLHOSTCALLBACK), cbData));
-        return VINF_SUCCESS; /* Never return any errors back to the guest here. */
-    }
-
-    const PVBOXGUESTCTRLHOSTCALLBACK pSvcCb = (PVBOXGUESTCTRLHOSTCALLBACK)pvData;
-    AssertPtr(pSvcCb);
-
-    if (pSvcCb->mParms) /* At least context ID must be present. */
-    {
-        uint32_t uContextID;
-        int rc = HGCMSvcGetU32(&pSvcCb->mpaParms[0], &uContextID);
-        AssertMsgRCReturn(rc, ("Unable to extract callback context ID, pvData=%p\n", pSvcCb),
-                          VINF_SUCCESS /* Never return any errors back to the guest here */);
-
-        VBOXGUESTCTRLHOSTCBCTX ctxCb = { u32Function, uContextID };
-        rc = pGuest->i_dispatchToSession(&ctxCb, pSvcCb);
-
-        Log2Func(("CID=%RU32, uSession=%RU32, uObject=%RU32, uCount=%RU32, rc=%Rrc\n",
-                  uContextID,
-                  VBOX_GUESTCTRL_CONTEXTID_GET_SESSION(uContextID),
-                  VBOX_GUESTCTRL_CONTEXTID_GET_OBJECT(uContextID),
-                  VBOX_GUESTCTRL_CONTEXTID_GET_COUNT(uContextID), rc));
-    }
-
-    /* Never return any errors back to the guest here. */
-    return VINF_SUCCESS;
+    ASSERT_GUEST_RETURN(pSvcCb->mParms > 0, VERR_WRONG_PARAMETER_COUNT);
+    ASSERT_GUEST_MSG_RETURN(pSvcCb->mpaParms[0].type == VBOX_HGCM_SVC_PARM_32BIT,
+                            ("type=%d\n", pSvcCb->mpaParms[0].type), VERR_WRONG_PARAMETER_TYPE);
+    uint32_t const idContext = pSvcCb->mpaParms[0].u.uint32;
+
+    VBOXGUESTCTRLHOSTCBCTX CtxCb = { idFunction, idContext };
+    int rc = pGuest->i_dispatchToSession(&CtxCb, pSvcCb);
+
+    Log2Func(("CID=%#x, idSession=%RU32, uObject=%RU32, uCount=%RU32, rc=%Rrc\n",
+              idContext, VBOX_GUESTCTRL_CONTEXTID_GET_SESSION(idContext), VBOX_GUESTCTRL_CONTEXTID_GET_OBJECT(idContext),
+              VBOX_GUESTCTRL_CONTEXTID_GET_COUNT(idContext), rc));
+    return rc;
 }
 
@@ -215 +223 @@
         }
         else
-            rc = VERR_NOT_FOUND;
+            rc = VERR_INVALID_FUNCTION;
     }
     else
-        rc = VERR_NOT_FOUND;
+        rc = VERR_INVALID_SESSION_ID;
 
     LogFlowFuncLeaveRC(rc);

trunk/src/VBox/Main/src-client/GuestCtrlPrivate.cpp

@@ -730 +730 @@
     AssertPtrReturn(pSvcCb, VERR_INVALID_POINTER);
 
-    int vrc = VINF_SUCCESS;
+    int vrc;
 
     try
@@ -739 +739 @@
         {
             case GUEST_MSG_PROGRESS_UPDATE:
+                vrc = VINF_SUCCESS;
                 break;
 
@@ -752 +753 @@
                     vrc = HGCMSvcGetU32(&pSvcCb->mpaParms[idx++], &dataCb.rc);
                     AssertRCReturn(vrc, vrc);
-                    vrc = HGCMSvcGetPv(&pSvcCb->mpaParms[idx++], &dataCb.pvPayload,
-                                       &dataCb.cbPayload);
+                    vrc = HGCMSvcGetPv(&pSvcCb->mpaParms[idx++], &dataCb.pvPayload, &dataCb.cbPayload);
                     AssertRCReturn(vrc, vrc);
 
-                    GuestWaitEventPayload evPayload(dataCb.uType, dataCb.pvPayload, dataCb.cbPayload);
+                    GuestWaitEventPayload evPayload(dataCb.uType, dataCb.pvPayload, dataCb.cbPayload); /* This bugger throws int. */
                     vrc = signalWaitEventInternal(pCtxCb, dataCb.rc, &evPayload);
                 }

trunk/src/VBox/Main/src-client/GuestSessionImpl.cpp

@@ -1230 +1230 @@
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
 
-    const uint32_t uObjectID = VBOX_GUESTCTRL_CONTEXTID_GET_OBJECT(pCtxCb->uContextID);
-
+    /*
+     * Find the object.
+     */
     int rc = VERR_NOT_FOUND;
-
-    SessionObjects::const_iterator itObjs = mData.mObjects.find(uObjectID);
-
-    if (itObjs == mData.mObjects.end())
-        return rc;
-
-    /* Set protocol version so that pSvcCb can be interpreted right. */
-    pCtxCb->uProtocol = mData.mProtocolVersion;
-
-    switch (itObjs->second.enmType)
-    {
-        case SESSIONOBJECTTYPE_ANONYMOUS:
-            rc = VERR_NOT_SUPPORTED;
-            break;
-
-        case SESSIONOBJECTTYPE_SESSION:
-        {
-            alock.release();
-
-            rc = i_dispatchToThis(pCtxCb, pSvcCb);
-            break;
-        }
-        case SESSIONOBJECTTYPE_DIRECTORY:
-        {
-            SessionDirectories::const_iterator itDir = mData.mDirectories.find(uObjectID);
-            if (itDir != mData.mDirectories.end())
+    const uint32_t idObject = VBOX_GUESTCTRL_CONTEXTID_GET_OBJECT(pCtxCb->uContextID);
+    SessionObjects::const_iterator itObjs = mData.mObjects.find(idObject);
+    if (itObjs != mData.mObjects.end())
+    {
+        /* Set protocol version so that pSvcCb can be interpreted right. */
+        pCtxCb->uProtocol = mData.mProtocolVersion;
+
+        switch (itObjs->second.enmType)
+        {
+            case SESSIONOBJECTTYPE_ANONYMOUS:
+                rc = VERR_NOT_SUPPORTED;
+                break;
+
+            case SESSIONOBJECTTYPE_SESSION:
             {
-                ComObjPtr<GuestDirectory> pDirectory(itDir->second);
-                Assert(!pDirectory.isNull());
-
                 alock.release();
 
-                rc = pDirectory->i_callbackDispatcher(pCtxCb, pSvcCb);
+                rc = i_dispatchToThis(pCtxCb, pSvcCb);
+                break;
             }
-            break;
-        }
-        case SESSIONOBJECTTYPE_FILE:
-        {
-            SessionFiles::const_iterator itFile = mData.mFiles.find(uObjectID);
-            if (itFile != mData.mFiles.end())
+            case SESSIONOBJECTTYPE_DIRECTORY:
             {
-                ComObjPtr<GuestFile> pFile(itFile->second);
-                Assert(!pFile.isNull());
-
-                alock.release();
-
-                rc = pFile->i_callbackDispatcher(pCtxCb, pSvcCb);
+                SessionDirectories::const_iterator itDir = mData.mDirectories.find(idObject);
+                if (itDir != mData.mDirectories.end())
+                {
+                    ComObjPtr<GuestDirectory> pDirectory(itDir->second);
+                    Assert(!pDirectory.isNull());
+
+                    alock.release();
+
+                    rc = pDirectory->i_callbackDispatcher(pCtxCb, pSvcCb);
+                }
+                break;
             }
-            break;
-        }
-        case SESSIONOBJECTTYPE_PROCESS:
-        {
-            SessionProcesses::const_iterator itProc = mData.mProcesses.find(uObjectID);
-            if (itProc != mData.mProcesses.end())
+            case SESSIONOBJECTTYPE_FILE:
             {
-                ComObjPtr<GuestProcess> pProcess(itProc->second);
-                Assert(!pProcess.isNull());
-
-                alock.release();
-
-                rc = pProcess->i_callbackDispatcher(pCtxCb, pSvcCb);
+                SessionFiles::const_iterator itFile = mData.mFiles.find(idObject);
+                if (itFile != mData.mFiles.end())
+                {
+                    ComObjPtr<GuestFile> pFile(itFile->second);
+                    Assert(!pFile.isNull());
+
+                    alock.release();
+
+                    rc = pFile->i_callbackDispatcher(pCtxCb, pSvcCb);
+                }
+                break;
             }
-            break;
-        }
-        default:
-            AssertFailed();
-            break;
+            case SESSIONOBJECTTYPE_PROCESS:
+            {
+                SessionProcesses::const_iterator itProc = mData.mProcesses.find(idObject);
+                if (itProc != mData.mProcesses.end())
+                {
+                    ComObjPtr<GuestProcess> pProcess(itProc->second);
+                    Assert(!pProcess.isNull());
+
+                    alock.release();
+
+                    rc = pProcess->i_callbackDispatcher(pCtxCb, pSvcCb);
+                }
+                break;
+            }
+            default:
+                AssertMsgFailed(("%d\n", itObjs->second.enmType));
+                rc = VERR_INTERNAL_ERROR_4;
+                break;
+        }
     }