Changeset 57960 in vbox for trunk/src/VBox/Additions/common

Timestamp:

Sep 30, 2015 9:49:32 AM (10 years ago)

Author:

vboxsync

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

102948

Message:

VBoxServiceControlSession.cpp: feedback.

File:

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

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

@@ -1845 +1845 @@
             RTStrPrintf(szParmThreadId, sizeof(szParmThreadId), "--thread-id=%RU32", s_uCtrlSessionThread);
 #endif
-            char szParmUserName[GUESTPROCESS_MAX_USER_LEN + 32];
-
-            int iArgIdx = 0; /* Current index in argument vector. */
-            char const *papszArgs[16];
-
-            papszArgs[iArgIdx++] = pszExeName;
-            papszArgs[iArgIdx++] = "guestsession";
-            papszArgs[iArgIdx++] = szParmSessionID;
-            papszArgs[iArgIdx++] = szParmSessionProto;
-#ifdef DEBUG
-            papszArgs[iArgIdx++] = szParmThreadId;
+            int idxArg = 0; /* Next index in argument vector. */
+            char const *apszArgs[24];
+
+            apszArgs[idxArg++] = pszExeName;
+            apszArgs[idxArg++] = "guestsession";
+            apszArgs[idxArg++] = szParmSessionID;
+            apszArgs[idxArg++] = szParmSessionProto;
+#ifdef DEBUG
+            apszArgs[idxArg++] = szParmThreadId;
 #endif
             if (!fAnonymous) /* Do we need to pass a user name? */
             {
-                RTStrPrintf(szParmUserName, sizeof(szParmUserName), "--user=%s", pSessionThread->StartupInfo.szUser);
-                papszArgs[iArgIdx++] = szParmUserName;
+                apszArgs[idxArg++] = "--user";
+                apszArgs[idxArg++] = pSessionThread->StartupInfo.szUser;
             }
 
             /* Add same verbose flags as parent process. */
+/** @todo r=bird: how does this stuff work when g_cVerbosity = 0?  Also, why do you need rc2 here?  Do we actually care about the RC anyway? */
             int rc2 = VINF_SUCCESS;
             char szParmVerbose[32] = { 0 };
@@ -1875 +1874 @@
             }
             if (RT_SUCCESS(rc2))
-                papszArgs[iArgIdx++] = szParmVerbose;
+                apszArgs[idxArg++] = szParmVerbose;
+
+/** @todo r=bird: As I already mentioned in the review comment you removed in
+ * r102561, the log file name handling is rather mangled.  You have a 4K stack
+ * buffer, but you insist on doing 2-5 heap allocations when constructing the
+ * name.  Either you do it in the stack buffer or you do it on the heap.
+ *
+ * Now, if you make sure szParmLogFile is, say, 128 byte larger than
+ * g_szLogFile, you won't need to consider overflows any more.  You can easily
+ * construct the whole thing with a strcpy, RTPathStripSuffix and a sprintf.
+ */
 
             /* Add log file handling. Each session will have an own
              * log file, naming based on the parent log file. */
+#if 1
             char szParmLogFile[RTPATH_MAX];
             if (   RT_SUCCESS(rc2)
@@ -1926 +1936 @@
                 }
                 if (RT_SUCCESS(rc2))
-                    papszArgs[iArgIdx++] = szParmLogFile;
+                    apszArgs[idxArg++] = szParmLogFile;
 
                 rc = rc2;
@@ -1932 +1942 @@
             else if (RT_FAILURE(rc2))
                 rc = rc2;
+#else
+            char szParmLogFile[sizeof(g_szLogFile) + 128];
+            if (g_szLogFile)
+            {
+                const char *pszSuffix = RTPathSuffix(g_szLogFile);
+                if (!pszSuffix)
+                    pszSuffix = strchr(g_szLogFile, '\0');
+                size_t cchBase = pszSuffix - g_szLogFile;
+#ifndef DEBUG
+                RTStrPrintf(szParmLogFile, sizeof(szParmLogFile), "%.*s-%RU32-%s%s",
+                            cchBase, g_szLogFile, pSessionStartupInfo->uSessionID, pSessionStartupInfo->szUser, pszSuffix);
+#else
+                RTStrPrintf(szParmLogFile, sizeof(szParmLogFile), "%.*s-%RU32-%RU32-%s%s",
+                            cchBase, g_szLogFile, pSessionStartupInfo->uSessionID, s_uCtrlSessionThread,
+                            pSessionStartupInfo->szUser, pszSuffix);
+#endif
+                apszArgs[idxArg++] = "--logfile";
+                apszArgs[idxArg++] = szParmLogFile;
+            }
+#endif /* alternative version */
 #ifdef DEBUG
             VBoxServiceVerbose(4, "Argv building rc=%Rrc, session flags=%x\n", rc, g_Session.fFlags);
@@ -1937 +1967 @@
             {
                 if (g_Session.fFlags & VBOXSERVICECTRLSESSION_FLAG_DUMPSTDOUT)
-                    papszArgs[iArgIdx++] = "--dump-stdout";
+                    apszArgs[idxArg++] = "--dump-stdout";
                 if (g_Session.fFlags & VBOXSERVICECTRLSESSION_FLAG_DUMPSTDERR)
-                    papszArgs[iArgIdx++] = "--dump-stderr";
-            }
-#endif
-            papszArgs[iArgIdx++] = NULL;
+                    apszArgs[idxArg++] = "--dump-stderr";
+            }
+#endif
+            apszArgs[idxArg] = NULL;
+            Assert(idxArg < RT_ELEMENTS(apszArgs));
 
             if (g_cVerbosity > 3)
@@ -1948 +1979 @@
                 VBoxServiceVerbose(4, "Spawning parameters:\n");
 
-                iArgIdx = 0;
-                while (papszArgs[iArgIdx])
-                    VBoxServiceVerbose(4, "\t%s\n", papszArgs[iArgIdx++]);
+                idxArg = 0;
+                while (apszArgs[idxArg])
+                    VBoxServiceVerbose(4, "\t%s\n", apszArgs[idxArg++]);
             }
 
@@ -2025 +2056 @@
 
                     if (RT_SUCCESS(rc))
-                        rc = RTProcCreateEx(pszExeName, papszArgs, hEnv, uProcFlags,
+                        rc = RTProcCreateEx(pszExeName, apszArgs, hEnv, uProcFlags,
                                             pSession->StdIn.phChild, pSession->StdOut.phChild, pSession->StdErr.phChild,
                                             !fAnonymous ? pSession->StartupInfo.szUser : NULL,
@@ -2059 +2090 @@
                     hStdOutAndErr.enmType = RTHANDLETYPE_FILE;
 
-                    rc = RTProcCreateEx(pszExeName, papszArgs, RTENV_DEFAULT, uProcFlags,
+                    rc = RTProcCreateEx(pszExeName, apszArgs, RTENV_DEFAULT, uProcFlags,
                                         &hStdIn, &hStdOutAndErr, &hStdOutAndErr,
                                         !fAnonymous ? pSessionThread->StartupInfo.szUser : NULL,