Changeset 75720 in vbox for trunk/src/VBox/Additions/common/VBoxService

Timestamp:

Nov 25, 2018 6:37:51 PM (6 years ago)

Author:

vboxsync

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

126919

Message:

VBoxServiceControlSession.cpp: Pointed out bogus error handling in vgsvcGstCtrlSessionSpawnWorker() for the author to have a look at. Also, don't use doxygen comments inside functions. Did some other cleanups to help follow the code flow.

File:

• trunk/src/VBox/Additions/common/VBoxService/VBoxServiceControlSession.cpp (modified) (11 diffs)

trunk/src/VBox/Additions/common/VBoxService/VBoxServiceControlSession.cpp

@@ -724 +724 @@
                                          startupInfo.szEnv,      &startupInfo.cbEnv,            &startupInfo.uNumEnvVars,
                                          /* Credentials; for hosts with VBox < 4.3 (protocol version 1).
-                                          * For protocl v2 and up the credentials are part of the session
+                                          * For protocol v2 and up the credentials are part of the session
                                           * opening call. */
                                          startupInfo.szUser,     sizeof(startupInfo.szUser),
@@ -1067 +1067 @@
         VGSvcVerbose(3, "Unsupported message (uMsg=%RU32, cParms=%RU32) from host, skipping\n", uMsg, pHostCtx->uNumParms);
 
-        /**
+        /*
          * !!! HACK ALERT BEGIN !!!
          * As peeking for the current message by VbglR3GuestCtrlMsgWaitFor() / GUEST_MSG_WAIT only gives us the message type and
@@ -1073 +1073 @@
          *
          * This is needed in order to reply to the host with the current context ID, without breaking existing clients.
-         * Not doing this isn't fatal, but will make host clients wait longer (timing out) for not implemented messages.
-         ** @todo Get rid of this as soon as we have a protocl bump (v4).
-         */
+         * Not doing this isn't fatal, but will make host clients wait longer (timing out) for not implemented messages. */
+        /** @todo Get rid of this as soon as we have a protocol bump (v4). */
         struct HGCMMsgSkip
         {
@@ -1086 +1085 @@
         VBGL_HGCM_HDR_INIT(&Msg.hdr, pHostCtx->uClientID, GUEST_MSG_WAIT, pHostCtx->uNumParms);
 
-        /* Don't want to drag in VbglHGCMParmUInt32Set(). */
+        /* Don't want to drag in VbglHGCMParmUInt32Set(). */ /** @todo r=bird: Why don't we?  It's an inline function, so little dragging.  */
         Msg.context.type      = VMMDevHGCMParmType_32bit;
         Msg.context.u.value64 = 0; /* init unused bits to 0 */
@@ -1096 +1095 @@
             Msg.context.GetUInt32(&pHostCtx->uContextID);
 
-        /** !!!                !!!
+        /*  !!!                !!!
          *  !!! HACK ALERT END !!!
          *  !!!                !!! */
@@ -1317 +1316 @@
     if (RT_SUCCESS(rc))
     {
-        /* Set session filter. This prevents the guest control
-         * host service to send messages which belong to another
-         * session we don't want to handle. */
+        /* Set session filter. This prevents the guest control host service from
+           sending messages which belong to another session we don't want to handle. */
         uint32_t uFilterAdd = VBOX_GUESTCTRL_FILTER_BY_SESSION(pSession->StartupInfo.uSessionID);
         rc = VbglR3GuestCtrlMsgFilterSet(uClientID,
@@ -1338 +1336 @@
     }
     else
+/** @todo r=bird: This should return to caller, shouldn't it it?!?
+ * Because VbglR3GuestCtrlSessionNotify() below is now using an uninitialized
+ * uClientID, right?  If you check VbglR3GuestCtrlConnect, you can clearly see
+ * that it will not be set, except on success.  So, this is a terrible idea!
+ *
+ * Did this happen because of a weird desire to have a single return statement?
+ * Or was it some other confusion wrt program flow/state?  Indentation fright?
+ * Please figure out why and try not to do it again. :-)
+ */
         VGSvcError("Error connecting to guest control service, rc=%Rrc\n", rc);
 
@@ -1360 +1367 @@
     AssertReturn(RT_IS_POWER_OF_TWO(cbScratchBuf), RTEXITCODE_FAILURE);
     uint8_t *pvScratchBuf = NULL;
-
-    if (RT_SUCCESS(rc))
-    {
-        pvScratchBuf = (uint8_t*)RTMemAlloc(cbScratchBuf);
+    if (RT_SUCCESS(rc))
+    {
+        pvScratchBuf = (uint8_t *)RTMemAlloc(cbScratchBuf);
         if (!pvScratchBuf)
             rc = VERR_NO_MEMORY;
@@ -1379 +1385 @@
             uint32_t cParms = 0;
             rc = VbglR3GuestCtrlMsgWaitFor(uClientID, &uMsg, &cParms);
-            if (rc == VERR_TOO_MUCH_DATA)
+            if (   rc == VINF_SUCCESS
+                || rc == VERR_TOO_MUCH_DATA)
             {
-#ifdef DEBUG
-                VGSvcVerbose(4, "Message requires %RU32 parameters, but only 2 supplied -- retrying request (no error!)...\n",
-                             cParms);
-#endif
-                rc = VINF_SUCCESS; /* Try to get "real" message in next block below. */
+                VGSvcVerbose(4, "Msg=%RU32 (%RU32 parms) retrieved (%Rrc)\n", uMsg, cParms, rc);
+
+                /* Set number of parameters for current host context and pass it on to the
+                   session handler.
+                   Note! Only when handling HOST_SESSION_CLOSE is the rc used. */
+                ctxHost.uNumParms = cParms;
+                rc = VGSvcGstCtrlSessionHandler(pSession, uMsg, &ctxHost, pvScratchBuf, cbScratchBuf, &fShutdown);
+                if (fShutdown)
+                    break;
             }
             else if (RT_FAILURE(rc))
                 VGSvcVerbose(3, "Getting host message failed with %Rrc\n", rc); /* VERR_GEN_IO_FAILURE seems to be normal if ran into timeout. */
-            if (RT_SUCCESS(rc))
-            {
-                VGSvcVerbose(4, "Msg=%RU32 (%RU32 parms) retrieved\n", uMsg, cParms);
-
-                /* Set number of parameters for current host context. */
-                ctxHost.uNumParms = cParms;
-
-                /* ... and pass it on to the session handler. */
-                rc = VGSvcGstCtrlSessionHandler(pSession, uMsg, &ctxHost, pvScratchBuf, cbScratchBuf, &fShutdown);
-            }
-
-            if (fShutdown)
-                break;
 
             /* Let others run ... */
@@ -2209 +2207 @@
     };
 
-    int ch;
-    RTGETOPTUNION ValueUnion;
     RTGETOPTSTATE GetState;
     RTGetOptInit(&GetState, argc, argv,
@@ -2222 +2218 @@
     g_Session.StartupInfo.uSessionID = UINT32_MAX;
 
+    int ch;
+    RTGETOPTUNION ValueUnion;
     while ((ch = RTGetOpt(&GetState, &ValueUnion)) != 0)
     {