From f0dbb878337082d3f581874c12e6df2f4659a464 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Fri, 20 Jan 2012 18:10:40 -0500
Subject: Per Richard, replace LLProcessLauncher with LLProcess.
 LLProcessLauncher had the somewhat fuzzy mandate of (1) accumulating
 parameters with which to launch a child process and (2) sometimes tracking
 the lifespan of the ensuing child process. But a valid LLProcessLauncher
 object might or might not have ever been associated with an actual child
 process. LLProcess specifically tracks a child process. In effect, it's a
 fairly thin wrapper around a process HANDLE (on Windows) or pid_t
 (elsewhere), with lifespan management thrown in. A static LLProcess::create()
 method launches a new child; create() accepts an LLSD bundle with child
 parameters. So building up a parameter bundle is deferred to LLSD rather than
 conflated with the process management object. Reconcile all known
 LLProcessLauncher consumers in the viewer code base, notably the class unit
 tests.

---
 indra/llcommon/llprocess.cpp | 338 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 338 insertions(+)
 create mode 100644 indra/llcommon/llprocess.cpp

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
new file mode 100644
index 0000000000..8c0caca680
--- /dev/null
+++ b/indra/llcommon/llprocess.cpp
@@ -0,0 +1,338 @@
+/** 
+ * @file llprocess.cpp
+ * @brief Utility class for launching, terminating, and tracking the state of processes.
+ *
+ * $LicenseInfo:firstyear=2008&license=viewerlgpl$
+ * Second Life Viewer Source Code
+ * Copyright (C) 2010, Linden Research, Inc.
+ * 
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License only.
+ * 
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ * 
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
+ * 
+ * Linden Research, Inc., 945 Battery Street, San Francisco, CA  94111  USA
+ * $/LicenseInfo$
+ */
+
+#include "linden_common.h"
+#include "llprocess.h"
+#include "llsd.h"
+#include "llsdserialize.h"
+#include "stringize.h"
+
+#include <boost/foreach.hpp>
+#include <iostream>
+#include <stdexcept>
+
+/// Need an exception to avoid constructing an invalid LLProcess object, but
+/// internal use only
+struct LLProcessError: public std::runtime_error
+{
+	LLProcessError(const std::string& msg): std::runtime_error(msg) {}
+};
+
+LLProcessPtr LLProcess::create(const LLSD& params)
+{
+	try
+	{
+		return LLProcessPtr(new LLProcess(params));
+	}
+	catch (const LLProcessError& e)
+	{
+		LL_WARNS("LLProcess") << e.what() << LL_ENDL;
+		return LLProcessPtr();
+	}
+}
+
+LLProcess::LLProcess(const LLSD& params):
+	mProcessID(0),
+	mAutokill(params["autokill"].asBoolean())
+{
+	// nonstandard default bool value
+	if (! params.has("autokill"))
+		mAutokill = true;
+	if (! params.has("executable"))
+	{
+		throw LLProcessError(STRINGIZE("not launched: missing 'executable'\n"
+									   << LLSDNotationStreamer(params)));
+	}
+
+	launch(params);
+}
+
+LLProcess::~LLProcess()
+{
+	if (mAutokill)
+	{
+		kill();
+	}
+}
+
+bool LLProcess::isRunning(void)
+{
+	mProcessID = isRunning(mProcessID);
+	return (mProcessID != 0);
+}
+
+#if LL_WINDOWS
+
+static std::string quote(const std::string& str)
+{
+	std::string::size_type len(str.length());
+	// If the string is already quoted, assume user knows what s/he's doing.
+	if (len >= 2 && str[0] == '"' && str[len-1] == '"')
+	{
+		return str;
+	}
+
+	// Not already quoted: do it.
+	std::string result("\"");
+	for (std::string::const_iterator ci(str.begin()), cend(str.end()); ci != cend; ++ci)
+	{
+		if (*ci == '"')
+		{
+			result.append("\\");
+		}
+		result.push_back(*ci);
+	}
+	return result + "\"";
+}
+
+void LLProcess::launch(const LLSD& params)
+{
+	PROCESS_INFORMATION pinfo;
+	STARTUPINFOA sinfo;
+	memset(&sinfo, 0, sizeof(sinfo));
+	
+	std::string args = quote(params["executable"]);
+	BOOST_FOREACH(const std::string& arg, llsd::inArray(params["args"]))
+	{
+		args += " ";
+		args += quote(arg);
+	}
+	
+	// So retarded.  Windows requires that the second parameter to
+	// CreateProcessA be a writable (non-const) string...
+	std::vector<char> args2(args.begin(), args.end());
+	args2.push_back('\0');
+
+	// Convert wrapper to a real std::string so we can use c_str(); but use a
+	// named variable instead of a temporary so c_str() pointer remains valid.
+	std::string cwd(params["cwd"]);
+	const char * working_directory = 0;
+	if (! cwd.empty())
+		working_directory = cwd.c_str();
+	if( ! CreateProcessA( NULL, &args2[0], NULL, NULL, FALSE, 0, NULL, working_directory, &sinfo, &pinfo ) )
+	{
+		int result = GetLastError();
+
+		LPTSTR error_str = 0;
+		if(
+			FormatMessage( FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM,
+				NULL,
+				result,
+				0,
+				(LPTSTR)&error_str,
+				0,
+				NULL) 
+			!= 0) 
+		{
+			char message[256];
+			wcstombs(message, error_str, sizeof(message));
+			message[sizeof(message)-1] = 0;
+			LocalFree(error_str);
+			throw LLProcessError(STRINGIZE("CreateProcessA failed (" << result << "): "
+										   << message));
+		}
+		throw LLProcessError(STRINGIZE("CreateProcessA failed (" << result
+									   << "), but FormatMessage() did not explain"));
+	}
+
+	// foo = pinfo.dwProcessId; // get your pid here if you want to use it later on
+	// CloseHandle(pinfo.hProcess); // stops leaks - nothing else
+	mProcessID = pinfo.hProcess;
+	CloseHandle(pinfo.hThread); // stops leaks - nothing else
+}
+
+LLProcess::id LLProcess::isRunning(id handle)
+{
+	if (! handle)
+		return 0;
+
+	DWORD waitresult = WaitForSingleObject(handle, 0);
+	if(waitresult == WAIT_OBJECT_0)
+	{
+		// the process has completed.
+		return 0;
+	}
+
+	return handle;
+}
+
+bool LLProcess::kill(void)
+{
+	if (! mProcessID)
+		return false;
+
+	TerminateProcess(mProcessID, 0);
+	return ! isRunning();
+}
+
+#else // Mac and linux
+
+#include <signal.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <sys/wait.h>
+
+// Attempt to reap a process ID -- returns true if the process has exited and been reaped, false otherwise.
+static bool reap_pid(pid_t pid)
+{
+	pid_t wait_result = ::waitpid(pid, NULL, WNOHANG);
+	if (wait_result == pid)
+	{
+		return true;
+	}
+	if (wait_result == -1 && errno == ECHILD)
+	{
+		// No such process -- this may mean we're ignoring SIGCHILD.
+		return true;
+	}
+	
+	return false;
+}
+
+void LLProcess::launch(const LLSD& params)
+{
+	// flush all buffers before the child inherits them
+	::fflush(NULL);
+
+	pid_t child = vfork();
+	if (child == 0)
+	{
+		// child process
+
+		std::string cwd(params["cwd"]);
+		if (! cwd.empty())
+		{
+			// change to the desired child working directory
+			if (::chdir(cwd.c_str()))
+			{
+				// chdir failed
+				LL_WARNS("LLProcess") << "could not chdir(\"" << cwd << "\")" << LL_ENDL;
+				// pointless to throw; this is child process...
+				_exit(248);
+			}
+		}
+
+		// create an argv vector for the child process
+		std::vector<const char*> fake_argv;
+
+		// add the executable path
+		std::string executable(params["executable"]);
+		fake_argv.push_back(executable.c_str());
+
+		// and any arguments
+		const LLSD& params_args(params["args"]);
+		std::vector<std::string> args(params_args.beginArray(), params_args.endArray());
+		BOOST_FOREACH(const std::string& arg, args)
+		{
+			fake_argv.push_back(arg.c_str());
+		}
+
+		// terminate with a null pointer
+		fake_argv.push_back(NULL);
+
+		::execv(executable.c_str(), const_cast<char* const*>(&fake_argv[0]));
+
+		// If we reach this point, the exec failed.
+		LL_WARNS("LLProcess") << "failed to launch: ";
+		BOOST_FOREACH(const char* arg, fake_argv)
+		{
+			LL_CONT << arg << ' ';
+		}
+		LL_CONT << LL_ENDL;
+		// Use _exit() instead of exit() per the vfork man page. Exit with a
+		// distinctive rc: someday soon we'll be able to retrieve it, and it
+		// would be nice to be able to tell that the child process failed!
+		_exit(249);
+	}
+
+	// parent process
+	mProcessID = child;
+}
+
+LLProcess::id LLProcess::isRunning(id pid)
+{
+	if (! pid)
+		return 0;
+
+	// Check whether the process has exited, and reap it if it has.
+	if(reap_pid(pid))
+	{
+		// the process has exited.
+		return 0;
+	}
+
+	return pid;
+}
+
+bool LLProcess::kill(void)
+{
+	if (! mProcessID)
+		return false;
+
+	// Try to kill the process. We'll do approximately the same thing whether
+	// the kill returns an error or not, so we ignore the result.
+	(void)::kill(mProcessID, SIGTERM);
+
+	// This will have the side-effect of reaping the zombie if the process has exited.
+	return ! isRunning();
+}
+
+/*==========================================================================*|
+static std::list<pid_t> sZombies;
+
+void LLProcess::orphan(void)
+{
+	// Disassociate the process from this object
+	if(mProcessID != 0)
+	{	
+		// We may still need to reap the process's zombie eventually
+		sZombies.push_back(mProcessID);
+	
+		mProcessID = 0;
+	}
+}
+
+// static 
+void LLProcess::reap(void)
+{
+	// Attempt to real all saved process ID's.
+	
+	std::list<pid_t>::iterator iter = sZombies.begin();
+	while(iter != sZombies.end())
+	{
+		if(reap_pid(*iter))
+		{
+			iter = sZombies.erase(iter);
+		}
+		else
+		{
+			iter++;
+		}
+	}
+}
+|*==========================================================================*/
+
+#endif
-- 
cgit v1.2.3


From 6e214960ce203d1d50d7bd6bd04eedee3afd0fa3 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Fri, 20 Jan 2012 20:19:50 -0500
Subject: Define LLProcess::Params; accept create(const
 LLSDParamAdapter<Params>&). This allows callers to pass either LLSD formatted
 as before -- which all callers still do -- or an actual LLProcess::Params
 block.

---
 indra/llcommon/llprocess.cpp | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 8c0caca680..dfb2ed69e9 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -26,7 +26,6 @@
 
 #include "linden_common.h"
 #include "llprocess.h"
-#include "llsd.h"
 #include "llsdserialize.h"
 #include "stringize.h"
 
@@ -41,7 +40,7 @@ struct LLProcessError: public std::runtime_error
 	LLProcessError(const std::string& msg): std::runtime_error(msg) {}
 };
 
-LLProcessPtr LLProcess::create(const LLSD& params)
+LLProcessPtr LLProcess::create(const LLSDParamAdapter<Params>& params)
 {
 	try
 	{
@@ -54,16 +53,13 @@ LLProcessPtr LLProcess::create(const LLSD& params)
 	}
 }
 
-LLProcess::LLProcess(const LLSD& params):
+LLProcess::LLProcess(const LLSDParamAdapter<Params>& params):
 	mProcessID(0),
-	mAutokill(params["autokill"].asBoolean())
+	mAutokill(params.autokill)
 {
-	// nonstandard default bool value
-	if (! params.has("autokill"))
-		mAutokill = true;
-	if (! params.has("executable"))
+	if (! params.validateBlock(true))
 	{
-		throw LLProcessError(STRINGIZE("not launched: missing 'executable'\n"
+		throw LLProcessError(STRINGIZE("not launched: failed parameter validation\n"
 									   << LLSDNotationStreamer(params)));
 	}
 
@@ -108,14 +104,14 @@ static std::string quote(const std::string& str)
 	return result + "\"";
 }
 
-void LLProcess::launch(const LLSD& params)
+void LLProcess::launch(const LLSDParamAdapter<Params>& params)
 {
 	PROCESS_INFORMATION pinfo;
 	STARTUPINFOA sinfo;
 	memset(&sinfo, 0, sizeof(sinfo));
 	
-	std::string args = quote(params["executable"]);
-	BOOST_FOREACH(const std::string& arg, llsd::inArray(params["args"]))
+	std::string args = quote(params.executable);
+	BOOST_FOREACH(const std::string& arg, params.args)
 	{
 		args += " ";
 		args += quote(arg);
@@ -128,7 +124,7 @@ void LLProcess::launch(const LLSD& params)
 
 	// Convert wrapper to a real std::string so we can use c_str(); but use a
 	// named variable instead of a temporary so c_str() pointer remains valid.
-	std::string cwd(params["cwd"]);
+	std::string cwd(params.cwd);
 	const char * working_directory = 0;
 	if (! cwd.empty())
 		working_directory = cwd.c_str();
@@ -212,7 +208,7 @@ static bool reap_pid(pid_t pid)
 	return false;
 }
 
-void LLProcess::launch(const LLSD& params)
+void LLProcess::launch(const LLSDParamAdapter<Params>& params)
 {
 	// flush all buffers before the child inherits them
 	::fflush(NULL);
@@ -222,7 +218,7 @@ void LLProcess::launch(const LLSD& params)
 	{
 		// child process
 
-		std::string cwd(params["cwd"]);
+		std::string cwd(params.cwd);
 		if (! cwd.empty())
 		{
 			// change to the desired child working directory
@@ -239,12 +235,11 @@ void LLProcess::launch(const LLSD& params)
 		std::vector<const char*> fake_argv;
 
 		// add the executable path
-		std::string executable(params["executable"]);
+		std::string executable(params.executable);
 		fake_argv.push_back(executable.c_str());
 
 		// and any arguments
-		const LLSD& params_args(params["args"]);
-		std::vector<std::string> args(params_args.beginArray(), params_args.endArray());
+		std::vector<std::string> args(params.args.begin(), params.args.end());
 		BOOST_FOREACH(const std::string& arg, args)
 		{
 			fake_argv.push_back(arg.c_str());
-- 
cgit v1.2.3


From b9a03b95aafb07eb32a8f99a671f2216acce96d4 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Sun, 22 Jan 2012 09:16:11 -0500
Subject: On Windows, introduce viewer Job Object and assign children to it.
 The idea is that, with the right flag settings, this will cause the OS to
 terminate remaining viewer child processes when the viewer terminates --
 whether or not it terminates intentionally. Of course, if LLProcess's caller
 specifies autokill=false, e.g. to run the viewer updater, that asserts that
 we WANT the child to persist beyond the viewer session itself.

---
 indra/llcommon/llprocess.cpp | 167 +++++++++++++++++++++++++++++++++----------
 1 file changed, 128 insertions(+), 39 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index dfb2ed69e9..d30d87411d 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -27,6 +27,7 @@
 #include "linden_common.h"
 #include "llprocess.h"
 #include "llsdserialize.h"
+#include "llsingleton.h"
 #include "stringize.h"
 
 #include <boost/foreach.hpp>
@@ -80,43 +81,84 @@ bool LLProcess::isRunning(void)
 	return (mProcessID != 0);
 }
 
+/*****************************************************************************
+*   Windows specific
+*****************************************************************************/
 #if LL_WINDOWS
 
-static std::string quote(const std::string& str)
+static std::string WindowsErrorString(const std::string& operation);
+static std::string quote(const std::string&);
+
+/**
+ * Wrap a Windows Job Object for use in managing child-process lifespan.
+ *
+ * On Windows, we use a Job Object to constrain the lifespan of any
+ * autokill=true child process to the viewer's own lifespan:
+ * http://stackoverflow.com/questions/53208/how-do-i-automatically-destroy-child-processes-in-windows
+ * (thanks Richard!).
+ *
+ * We manage it using an LLSingleton for a couple of reasons:
+ *
+ * # Lazy initialization: if some viewer session never launches a child
+ *   process, we should never have to create a Job Object.
+ * # Cross-DLL support: be wary of C++ statics when multiple DLLs are
+ *   involved.
+ */
+class LLJob: public LLSingleton<LLJob>
 {
-	std::string::size_type len(str.length());
-	// If the string is already quoted, assume user knows what s/he's doing.
-	if (len >= 2 && str[0] == '"' && str[len-1] == '"')
+public:
+	void assignProcess(const std::string& prog, HANDLE hProcess)
 	{
-		return str;
+		// If we never managed to initialize this Job Object, can't use it --
+		// but don't keep spamming the log, we already emitted warnings when
+		// we first tried to create.
+		if (! mJob)
+			return;
+
+		if (! AssignProcessToJobObject(mJob, hProcess))
+		{
+			LL_WARNS("LLProcess") << WindowsErrorString(STRINGIZE("AssignProcessToJobObject(\""
+																  << prog << "\")")) << LL_ENDL;
+		}
 	}
 
-	// Not already quoted: do it.
-	std::string result("\"");
-	for (std::string::const_iterator ci(str.begin()), cend(str.end()); ci != cend; ++ci)
+private:
+	LLJob():
+		mJob(0)
 	{
-		if (*ci == '"')
+		mJob = CreateJobObject(NULL, NULL);
+		if (! mJob)
 		{
-			result.append("\\");
+			LL_WARNS("LLProcess") << WindowsErrorString("CreateJobObject()") << LL_ENDL;
+			return;
+		}
+
+		JOBOBJECT_EXTENDED_LIMIT_INFORMATION jeli = { 0 };
+
+		// Configure all child processes associated with this new job object
+		// to terminate when the calling process (us!) terminates.
+		jeli.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
+		if (! SetInformationJobObject(mJob, JobObjectExtendedLimitInformation, &jeli, sizeof(jeli)))
+		{
+			LL_WARNS("LLProcess") << WindowsErrorString("SetInformationJobObject()") << LL_ENDL;
 		}
-		result.push_back(*ci);
 	}
-	return result + "\"";
-}
+
+	HANDLE mJob;
+};
 
 void LLProcess::launch(const LLSDParamAdapter<Params>& params)
 {
 	PROCESS_INFORMATION pinfo;
-	STARTUPINFOA sinfo;
-	memset(&sinfo, 0, sizeof(sinfo));
-	
+	STARTUPINFOA sinfo = { sizeof(sinfo) };
+
 	std::string args = quote(params.executable);
 	BOOST_FOREACH(const std::string& arg, params.args)
 	{
 		args += " ";
 		args += quote(arg);
 	}
-	
+
 	// So retarded.  Windows requires that the second parameter to
 	// CreateProcessA be a writable (non-const) string...
 	std::vector<char> args2(args.begin(), args.end());
@@ -130,28 +172,14 @@ void LLProcess::launch(const LLSDParamAdapter<Params>& params)
 		working_directory = cwd.c_str();
 	if( ! CreateProcessA( NULL, &args2[0], NULL, NULL, FALSE, 0, NULL, working_directory, &sinfo, &pinfo ) )
 	{
-		int result = GetLastError();
-
-		LPTSTR error_str = 0;
-		if(
-			FormatMessage( FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM,
-				NULL,
-				result,
-				0,
-				(LPTSTR)&error_str,
-				0,
-				NULL) 
-			!= 0) 
-		{
-			char message[256];
-			wcstombs(message, error_str, sizeof(message));
-			message[sizeof(message)-1] = 0;
-			LocalFree(error_str);
-			throw LLProcessError(STRINGIZE("CreateProcessA failed (" << result << "): "
-										   << message));
-		}
-		throw LLProcessError(STRINGIZE("CreateProcessA failed (" << result
-									   << "), but FormatMessage() did not explain"));
+		throw LLProcessError(WindowsErrorString("CreateProcessA"));
+	}
+
+	// Now associate the new child process with our Job Object -- unless
+	// autokill is false, i.e. caller asserts the child should persist.
+	if (params.autokill)
+	{
+		LLJob::instance().assignProcess(params.executable, pinfo.hProcess);
 	}
 
 	// foo = pinfo.dwProcessId; // get your pid here if you want to use it later on
@@ -184,6 +212,67 @@ bool LLProcess::kill(void)
 	return ! isRunning();
 }
 
+/**
+ * Double-quote an argument string, unless it's already double-quoted. If we
+ * quote it, escape any embedded double-quote with backslash.
+ *
+ * LLProcess::create()'s caller passes a Unix-style array of strings for
+ * command-line arguments. Our caller can and should expect that these will be
+ * passed to the child process as individual arguments, regardless of content
+ * (e.g. embedded spaces). But because Windows invokes any child process with
+ * a single command-line string, this means we must quote each argument behind
+ * the scenes.
+ */
+static std::string quote(const std::string& str)
+{
+	std::string::size_type len(str.length());
+	// If the string is already quoted, assume user knows what s/he's doing.
+	if (len >= 2 && str[0] == '"' && str[len-1] == '"')
+	{
+		return str;
+	}
+
+	// Not already quoted: do it.
+	std::string result("\"");
+	for (std::string::const_iterator ci(str.begin()), cend(str.end()); ci != cend; ++ci)
+	{
+		if (*ci == '"')
+		{
+			result.append("\\");
+		}
+		result.push_back(*ci);
+	}
+	return result + "\"";
+}
+
+/// GetLastError()/FormatMessage() boilerplate
+static std::string WindowsErrorString(const std::string& operation)
+{
+	int result = GetLastError();
+
+	LPTSTR error_str = 0;
+	if (FormatMessage( FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM,
+					   NULL,
+					   result,
+					   0,
+					   (LPTSTR)&error_str,
+					   0,
+					   NULL)
+		!= 0) 
+	{
+		char message[256];
+		wcstombs(message, error_str, sizeof(message));
+		message[sizeof(message)-1] = 0;
+		LocalFree(error_str);
+		return STRINGIZE(operation << " failed (" << result << "): " << message);
+	}
+	return STRINGIZE(operation << " failed (" << result
+					 << "), but FormatMessage() did not explain");
+}
+
+/*****************************************************************************
+*   Non-Windows specific
+*****************************************************************************/
 #else // Mac and linux
 
 #include <signal.h>
-- 
cgit v1.2.3


From aa1bbe3277842a9a6e7db5227b35f1fbea50b7a6 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Sun, 22 Jan 2012 10:58:16 -0500
Subject: Make LLProcess::Params streamable; use that in LLExternalEditor.

---
 indra/llcommon/llprocess.cpp | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index d30d87411d..9d6c19f1dd 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -81,6 +81,21 @@ bool LLProcess::isRunning(void)
 	return (mProcessID != 0);
 }
 
+std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params)
+{
+	std::string cwd(params.cwd);
+	if (! cwd.empty())
+	{
+		out << "cd '" << cwd << "': ";
+	}
+	out << '"' << std::string(params.executable) << '"';
+	BOOST_FOREACH(const std::string& arg, params.args)
+	{
+		out << " \"" << arg << '"';
+	}
+	return out;
+}
+
 /*****************************************************************************
 *   Windows specific
 *****************************************************************************/
-- 
cgit v1.2.3


From 748d1b311fdecf123df40bd7d22dd7e19afaca84 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Sun, 22 Jan 2012 11:56:38 -0500
Subject: Add LLProcess logging on launch(), kill(), isRunning(). Much as I
 dislike viewer log spam, seems to me starting a child process, killing it and
 observing its termination are noteworthy events. New logging makes
 LLExternalEditor launch message redundant; removed.

---
 indra/llcommon/llprocess.cpp | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 9d6c19f1dd..6d329a3fa1 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -77,7 +77,7 @@ LLProcess::~LLProcess()
 
 bool LLProcess::isRunning(void)
 {
-	mProcessID = isRunning(mProcessID);
+	mProcessID = isRunning(mProcessID, mDesc);
 	return (mProcessID != 0);
 }
 
@@ -190,20 +190,23 @@ void LLProcess::launch(const LLSDParamAdapter<Params>& params)
 		throw LLProcessError(WindowsErrorString("CreateProcessA"));
 	}
 
+	// foo = pinfo.dwProcessId; // get your pid here if you want to use it later on
+	// CloseHandle(pinfo.hProcess); // stops leaks - nothing else
+	mProcessID = pinfo.hProcess;
+	CloseHandle(pinfo.hThread); // stops leaks - nothing else
+
+	mDesc = STRINGIZE('"' << std::string(params.executable) << "\" (" << pinfo.dwProcessId << ')');
+	LL_INFOS("LLProcess") << "Launched " << params << " (" << pinfo.dwProcessId << ")" << LL_ENDL;
+
 	// Now associate the new child process with our Job Object -- unless
 	// autokill is false, i.e. caller asserts the child should persist.
 	if (params.autokill)
 	{
-		LLJob::instance().assignProcess(params.executable, pinfo.hProcess);
+		LLJob::instance().assignProcess(mDesc, mProcessID);
 	}
-
-	// foo = pinfo.dwProcessId; // get your pid here if you want to use it later on
-	// CloseHandle(pinfo.hProcess); // stops leaks - nothing else
-	mProcessID = pinfo.hProcess;
-	CloseHandle(pinfo.hThread); // stops leaks - nothing else
 }
 
-LLProcess::id LLProcess::isRunning(id handle)
+LLProcess::id LLProcess::isRunning(id handle, const std::string& desc)
 {
 	if (! handle)
 		return 0;
@@ -212,6 +215,10 @@ LLProcess::id LLProcess::isRunning(id handle)
 	if(waitresult == WAIT_OBJECT_0)
 	{
 		// the process has completed.
+		if (! desc.empty())
+		{
+			LL_INFOS("LLProcess") << desc << " terminated" << LL_ENDL;
+		}
 		return 0;
 	}
 
@@ -223,6 +230,7 @@ bool LLProcess::kill(void)
 	if (! mProcessID)
 		return false;
 
+	LL_INFOS("LLProcess") << "killing " << mDesc << LL_ENDL;
 	TerminateProcess(mProcessID, 0);
 	return ! isRunning();
 }
@@ -369,9 +377,12 @@ void LLProcess::launch(const LLSDParamAdapter<Params>& params)
 
 	// parent process
 	mProcessID = child;
+
+	mDesc = STRINGIZE('"' << std::string(params.executable) << "\" (" << mProcessID << ')');
+	LL_INFOS("LLProcess") << "Launched " << params << " (" << mProcessID << ")" << LL_ENDL;
 }
 
-LLProcess::id LLProcess::isRunning(id pid)
+LLProcess::id LLProcess::isRunning(id pid, const std::string& desc)
 {
 	if (! pid)
 		return 0;
@@ -380,6 +391,10 @@ LLProcess::id LLProcess::isRunning(id pid)
 	if(reap_pid(pid))
 	{
 		// the process has exited.
+		if (! desc.empty())
+		{
+			LL_INFOS("LLProcess") << desc << " terminated" << LL_ENDL;
+		}
 		return 0;
 	}
 
@@ -393,6 +408,7 @@ bool LLProcess::kill(void)
 
 	// Try to kill the process. We'll do approximately the same thing whether
 	// the kill returns an error or not, so we ignore the result.
+	LL_INFOS("LLProcess") << "killing " << mDesc << LL_ENDL;
 	(void)::kill(mProcessID, SIGTERM);
 
 	// This will have the side-effect of reaping the zombie if the process has exited.
-- 
cgit v1.2.3


From 738483e6302af5a9ad00fa3df17efe5336a03a41 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Sun, 22 Jan 2012 13:05:34 -0500
Subject: Every singleton needs a friend...

---
 indra/llcommon/llprocess.cpp | 1 +
 1 file changed, 1 insertion(+)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 6d329a3fa1..eb7ce4129b 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -138,6 +138,7 @@ public:
 	}
 
 private:
+	friend class LLSingleton<LLJob>;
 	LLJob():
 		mJob(0)
 	{
-- 
cgit v1.2.3


From 507e136f9a25179992b2093e10ade1093cc71698 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Mon, 23 Jan 2012 16:24:33 -0500
Subject: Per Richard: close unusable Job Object; move quote() to LLStringUtil.
 If LLProcess can't set the right flag on a Windows Job Object, the object
 isn't useful to us, so we might as well discard it. quote() is sufficiently
 general that it belongs in LLStringUtil instead of buried as a static helper
 function in llprocess.cpp.

---
 indra/llcommon/llprocess.cpp | 49 ++++++++++++--------------------------------
 1 file changed, 13 insertions(+), 36 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index eb7ce4129b..2c7512419d 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -28,6 +28,7 @@
 #include "llprocess.h"
 #include "llsdserialize.h"
 #include "llsingleton.h"
+#include "llstring.h"
 #include "stringize.h"
 
 #include <boost/foreach.hpp>
@@ -102,7 +103,6 @@ std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params)
 #if LL_WINDOWS
 
 static std::string WindowsErrorString(const std::string& operation);
-static std::string quote(const std::string&);
 
 /**
  * Wrap a Windows Job Object for use in managing child-process lifespan.
@@ -157,6 +157,10 @@ private:
 		if (! SetInformationJobObject(mJob, JobObjectExtendedLimitInformation, &jeli, sizeof(jeli)))
 		{
 			LL_WARNS("LLProcess") << WindowsErrorString("SetInformationJobObject()") << LL_ENDL;
+			// This Job Object is useless to us
+			CloseHandle(mJob);
+			// prevent assignProcess() from trying to use it
+			mJob = 0;
 		}
 	}
 
@@ -168,11 +172,17 @@ void LLProcess::launch(const LLSDParamAdapter<Params>& params)
 	PROCESS_INFORMATION pinfo;
 	STARTUPINFOA sinfo = { sizeof(sinfo) };
 
-	std::string args = quote(params.executable);
+	// LLProcess::create()'s caller passes a Unix-style array of strings for
+	// command-line arguments. Our caller can and should expect that these will be
+	// passed to the child process as individual arguments, regardless of content
+	// (e.g. embedded spaces). But because Windows invokes any child process with
+	// a single command-line string, this means we must quote each argument behind
+	// the scenes.
+	std::string args = LLStringUtil::quote(params.executable);
 	BOOST_FOREACH(const std::string& arg, params.args)
 	{
 		args += " ";
-		args += quote(arg);
+		args += LLStringUtil::quote(arg);
 	}
 
 	// So retarded.  Windows requires that the second parameter to
@@ -236,39 +246,6 @@ bool LLProcess::kill(void)
 	return ! isRunning();
 }
 
-/**
- * Double-quote an argument string, unless it's already double-quoted. If we
- * quote it, escape any embedded double-quote with backslash.
- *
- * LLProcess::create()'s caller passes a Unix-style array of strings for
- * command-line arguments. Our caller can and should expect that these will be
- * passed to the child process as individual arguments, regardless of content
- * (e.g. embedded spaces). But because Windows invokes any child process with
- * a single command-line string, this means we must quote each argument behind
- * the scenes.
- */
-static std::string quote(const std::string& str)
-{
-	std::string::size_type len(str.length());
-	// If the string is already quoted, assume user knows what s/he's doing.
-	if (len >= 2 && str[0] == '"' && str[len-1] == '"')
-	{
-		return str;
-	}
-
-	// Not already quoted: do it.
-	std::string result("\"");
-	for (std::string::const_iterator ci(str.begin()), cend(str.end()); ci != cend; ++ci)
-	{
-		if (*ci == '"')
-		{
-			result.append("\\");
-		}
-		result.push_back(*ci);
-	}
-	return result + "\"";
-}
-
 /// GetLastError()/FormatMessage() boilerplate
 static std::string WindowsErrorString(const std::string& operation)
 {
-- 
cgit v1.2.3


From 27df0a84564d3a886661aae0faae74c2157cd31b Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Fri, 27 Jan 2012 23:46:00 -0500
Subject: On Windows, only quote LLProcess arguments if they seem to need it.
 On Posix platforms, the OS argument mechanism makes quoting/reparsing
 unnecessary anyway, so this only affects Windows. Add optional 'triggers'
 parameter to LLStringUtils::quote() (default: space and double-quote). Only
 if the passed string contains a character in 'triggers' will it be
 double-quoted. This is observed to fix a Windows-specific problem in which
 plugin child process would fail to start because it wasn't expecting a quoted
 number. Use LLStringUtils::quote() more consistently in LLProcess
 implementation for logging.

---
 indra/llcommon/llprocess.cpp | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 2c7512419d..2b7a534fb3 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -87,12 +87,12 @@ std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params)
 	std::string cwd(params.cwd);
 	if (! cwd.empty())
 	{
-		out << "cd '" << cwd << "': ";
+		out << "cd " << LLStringUtil::quote(cwd) << ": ";
 	}
-	out << '"' << std::string(params.executable) << '"';
+	out << LLStringUtil::quote(params.executable);
 	BOOST_FOREACH(const std::string& arg, params.args)
 	{
-		out << " \"" << arg << '"';
+		out << ' ' << LLStringUtil::quote(arg);
 	}
 	return out;
 }
@@ -132,8 +132,8 @@ public:
 
 		if (! AssignProcessToJobObject(mJob, hProcess))
 		{
-			LL_WARNS("LLProcess") << WindowsErrorString(STRINGIZE("AssignProcessToJobObject(\""
-																  << prog << "\")")) << LL_ENDL;
+			LL_WARNS("LLProcess") << WindowsErrorString(STRINGIZE("AssignProcessToJobObject("
+																  << prog << ")")) << LL_ENDL;
 		}
 	}
 
@@ -206,7 +206,7 @@ void LLProcess::launch(const LLSDParamAdapter<Params>& params)
 	mProcessID = pinfo.hProcess;
 	CloseHandle(pinfo.hThread); // stops leaks - nothing else
 
-	mDesc = STRINGIZE('"' << std::string(params.executable) << "\" (" << pinfo.dwProcessId << ')');
+	mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << pinfo.dwProcessId << ')');
 	LL_INFOS("LLProcess") << "Launched " << params << " (" << pinfo.dwProcessId << ")" << LL_ENDL;
 
 	// Now associate the new child process with our Job Object -- unless
@@ -356,7 +356,7 @@ void LLProcess::launch(const LLSDParamAdapter<Params>& params)
 	// parent process
 	mProcessID = child;
 
-	mDesc = STRINGIZE('"' << std::string(params.executable) << "\" (" << mProcessID << ')');
+	mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << mProcessID << ')');
 	LL_INFOS("LLProcess") << "Launched " << params << " (" << mProcessID << ")" << LL_ENDL;
 }
 
-- 
cgit v1.2.3


From 803acbc5efde19c0acacfc7fe4990841dbf31a3e Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Mon, 30 Jan 2012 10:14:10 -0500
Subject: Trim trailing "\r\n" from Windows FormatMessage() string for logging.

---
 indra/llcommon/llprocess.cpp | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 2b7a534fb3..8c0e8fe65e 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -261,11 +261,15 @@ static std::string WindowsErrorString(const std::string& operation)
 					   NULL)
 		!= 0) 
 	{
+        // convert from wide-char string to multi-byte string
 		char message[256];
 		wcstombs(message, error_str, sizeof(message));
 		message[sizeof(message)-1] = 0;
 		LocalFree(error_str);
-		return STRINGIZE(operation << " failed (" << result << "): " << message);
+		// convert to std::string to trim trailing whitespace
+		std::string mbsstr(message);
+		mbsstr.erase(mbsstr.find_last_not_of(" \t\r\n"));
+		return STRINGIZE(operation << " failed (" << result << "): " << mbsstr);
 	}
 	return STRINGIZE(operation << " failed (" << result
 					 << "), but FormatMessage() did not explain");
-- 
cgit v1.2.3


From 85581eefa63d8f8e8c5132c4cd7e137f6cb88869 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Mon, 30 Jan 2012 12:11:44 -0500
Subject: Expose 'handle' as well as 'id' on LLProcess objects. On Posix, these
 and the corresponding getProcessID()/getProcessHandle() accessors produce the
 same pid_t value; but on Windows, it's useful to distinguish an int-like 'id'
 useful to human log readers versus an opaque 'handle' for passing to
 platform-specific API functions. So make the distinction in a
 platform-independent way.

---
 indra/llcommon/llprocess.cpp | 50 +++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 19 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 8c0e8fe65e..a7bafb8cb0 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -42,7 +42,7 @@ struct LLProcessError: public std::runtime_error
 	LLProcessError(const std::string& msg): std::runtime_error(msg) {}
 };
 
-LLProcessPtr LLProcess::create(const LLSDParamAdapter<Params>& params)
+LLProcessPtr LLProcess::create(const LLSDOrParams& params)
 {
 	try
 	{
@@ -55,8 +55,9 @@ LLProcessPtr LLProcess::create(const LLSDParamAdapter<Params>& params)
 	}
 }
 
-LLProcess::LLProcess(const LLSDParamAdapter<Params>& params):
+LLProcess::LLProcess(const LLSDOrParams& params):
 	mProcessID(0),
+	mProcessHandle(0),
 	mAutokill(params.autokill)
 {
 	if (! params.validateBlock(true))
@@ -78,8 +79,18 @@ LLProcess::~LLProcess()
 
 bool LLProcess::isRunning(void)
 {
-	mProcessID = isRunning(mProcessID, mDesc);
-	return (mProcessID != 0);
+	mProcessHandle = isRunning(mProcessHandle, mDesc);
+	return (mProcessHandle != 0);
+}
+
+LLProcess::id LLProcess::getProcessID() const
+{
+	return mProcessID;
+}
+
+LLProcess::handle LLProcess::getProcessHandle() const
+{
+	return mProcessHandle;
 }
 
 std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params)
@@ -122,7 +133,7 @@ static std::string WindowsErrorString(const std::string& operation);
 class LLJob: public LLSingleton<LLJob>
 {
 public:
-	void assignProcess(const std::string& prog, HANDLE hProcess)
+	void assignProcess(const std::string& prog, handle hProcess)
 	{
 		// If we never managed to initialize this Job Object, can't use it --
 		// but don't keep spamming the log, we already emitted warnings when
@@ -164,10 +175,10 @@ private:
 		}
 	}
 
-	HANDLE mJob;
+	handle mJob;
 };
 
-void LLProcess::launch(const LLSDParamAdapter<Params>& params)
+void LLProcess::launch(const LLSDOrParams& params)
 {
 	PROCESS_INFORMATION pinfo;
 	STARTUPINFOA sinfo = { sizeof(sinfo) };
@@ -201,28 +212,28 @@ void LLProcess::launch(const LLSDParamAdapter<Params>& params)
 		throw LLProcessError(WindowsErrorString("CreateProcessA"));
 	}
 
-	// foo = pinfo.dwProcessId; // get your pid here if you want to use it later on
 	// CloseHandle(pinfo.hProcess); // stops leaks - nothing else
-	mProcessID = pinfo.hProcess;
+	mProcessID = pinfo.dwProcessId;
+	mProcessHandle = pinfo.hProcess;
 	CloseHandle(pinfo.hThread); // stops leaks - nothing else
 
-	mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << pinfo.dwProcessId << ')');
-	LL_INFOS("LLProcess") << "Launched " << params << " (" << pinfo.dwProcessId << ")" << LL_ENDL;
+	mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << mProcessID << ')');
+	LL_INFOS("LLProcess") << "Launched " << params << " (" << mProcessID << ")" << LL_ENDL;
 
 	// Now associate the new child process with our Job Object -- unless
 	// autokill is false, i.e. caller asserts the child should persist.
 	if (params.autokill)
 	{
-		LLJob::instance().assignProcess(mDesc, mProcessID);
+		LLJob::instance().assignProcess(mDesc, mProcessHandle);
 	}
 }
 
-LLProcess::id LLProcess::isRunning(id handle, const std::string& desc)
+LLProcess::handle LLProcess::isRunning(handle h, const std::string& desc)
 {
-	if (! handle)
+	if (! h)
 		return 0;
 
-	DWORD waitresult = WaitForSingleObject(handle, 0);
+	DWORD waitresult = WaitForSingleObject(h, 0);
 	if(waitresult == WAIT_OBJECT_0)
 	{
 		// the process has completed.
@@ -233,16 +244,16 @@ LLProcess::id LLProcess::isRunning(id handle, const std::string& desc)
 		return 0;
 	}
 
-	return handle;
+	return h;
 }
 
 bool LLProcess::kill(void)
 {
-	if (! mProcessID)
+	if (! mProcessHandle)
 		return false;
 
 	LL_INFOS("LLProcess") << "killing " << mDesc << LL_ENDL;
-	TerminateProcess(mProcessID, 0);
+	TerminateProcess(mProcessHandle, 0);
 	return ! isRunning();
 }
 
@@ -302,7 +313,7 @@ static bool reap_pid(pid_t pid)
 	return false;
 }
 
-void LLProcess::launch(const LLSDParamAdapter<Params>& params)
+void LLProcess::launch(const LLSDOrParams& params)
 {
 	// flush all buffers before the child inherits them
 	::fflush(NULL);
@@ -359,6 +370,7 @@ void LLProcess::launch(const LLSDParamAdapter<Params>& params)
 
 	// parent process
 	mProcessID = child;
+	mProcessHandle = child;
 
 	mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << mProcessID << ')');
 	LL_INFOS("LLProcess") << "Launched " << params << " (" << mProcessID << ")" << LL_ENDL;
-- 
cgit v1.2.3


From 60a777d2e3f9bec7a20e5de2df7cb3ecd2455b98 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Mon, 30 Jan 2012 12:32:05 -0500
Subject: LLProcess::handle must be qualified when used in LLJob class.

---
 indra/llcommon/llprocess.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index a7bafb8cb0..1e27f8ce1d 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -133,7 +133,7 @@ static std::string WindowsErrorString(const std::string& operation);
 class LLJob: public LLSingleton<LLJob>
 {
 public:
-	void assignProcess(const std::string& prog, handle hProcess)
+	void assignProcess(const std::string& prog, LLProcess::handle hProcess)
 	{
 		// If we never managed to initialize this Job Object, can't use it --
 		// but don't keep spamming the log, we already emitted warnings when
@@ -175,7 +175,7 @@ private:
 		}
 	}
 
-	handle mJob;
+	LLProcess::handle mJob;
 };
 
 void LLProcess::launch(const LLSDOrParams& params)
-- 
cgit v1.2.3


From 491cd825561be1cf4a6f428a535811cbe0e3f179 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Mon, 30 Jan 2012 17:47:57 -0500
Subject: Set bit flag on CreateProcess() to allow AssignProcessToJobObject().
 Windows 7 and friends tend to create a process already implicitly allocated
 to a job object, and a process can only belong to a single job object.
 Passing CREATE_BREAKAWAY_FROM_JOB in CreateProcessA()'s dwCreationFlags seems
 to bypass the access-denied error observed with AssignProcessToJobObject()
 otherwise. This change should (!) enable OS lifespan management for
 SLVoice.exe et al.

---
 indra/llcommon/llprocess.cpp | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 1e27f8ce1d..8611d67f25 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -207,7 +207,26 @@ void LLProcess::launch(const LLSDOrParams& params)
 	const char * working_directory = 0;
 	if (! cwd.empty())
 		working_directory = cwd.c_str();
-	if( ! CreateProcessA( NULL, &args2[0], NULL, NULL, FALSE, 0, NULL, working_directory, &sinfo, &pinfo ) )
+
+	// It's important to pass CREATE_BREAKAWAY_FROM_JOB because Windows 7 et
+	// al. tend to implicitly launch new processes already bound to a job. From
+	// http://msdn.microsoft.com/en-us/library/windows/desktop/ms681949%28v=vs.85%29.aspx :
+	// "The process must not already be assigned to a job; if it is, the
+	// function fails with ERROR_ACCESS_DENIED." ...
+	// "If the process is being monitored by the Program Compatibility
+	// Assistant (PCA), it is placed into a compatibility job. Therefore, the
+	// process must be created using CREATE_BREAKAWAY_FROM_JOB before it can
+	// be placed in another job."
+	if( ! CreateProcessA(NULL,      // lpApplicationName
+                         &args2[0], // lpCommandLine
+                         NULL,      // lpProcessAttributes
+                         NULL,      // lpThreadAttributes
+                         FALSE,     // bInheritHandles
+                         CREATE_BREAKAWAY_FROM_JOB, // dwCreationFlags
+                         NULL,      // lpEnvironment
+                         working_directory, // lpCurrentDirectory
+                         &sinfo,            // lpStartupInfo
+                         &pinfo ) )         // lpProcessInformation
 	{
 		throw LLProcessError(WindowsErrorString("CreateProcessA"));
 	}
@@ -225,7 +244,7 @@ void LLProcess::launch(const LLSDOrParams& params)
 	if (params.autokill)
 	{
 		LLJob::instance().assignProcess(mDesc, mProcessHandle);
-	}
+}
 }
 
 LLProcess::handle LLProcess::isRunning(handle h, const std::string& desc)
@@ -287,7 +306,7 @@ static std::string WindowsErrorString(const std::string& operation)
 }
 
 /*****************************************************************************
-*   Non-Windows specific
+*   Posix specific
 *****************************************************************************/
 #else // Mac and linux
 
@@ -444,4 +463,4 @@ void LLProcess::reap(void)
 }
 |*==========================================================================*/
 
-#endif
+#endif  // Posix
-- 
cgit v1.2.3


From aafb03b29f5166e8978931ad8b717be32d942836 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Tue, 7 Feb 2012 10:53:23 -0500
Subject: Convert LLProcess implementation from platform-specific to using APR.
 Include logic to engage Linden apr_procattr_autokill_set() extension: on
 Windows, magic CreateProcess() flag must be pushed down into
 apr_proc_create() level. When using an APR package without that extension,
 present implementation should lock (e.g.) SLVoice.exe lifespan to viewer's on
 Windows XP but probably won't on Windows 7: need magic flag on
 CreateProcess(). Using APR child-termination callback requires us to define
 state (e.g. LLProcess::RUNNING). Take the opportunity to present Status,
 capturing state and (if terminated) rc or signal number; but since most of
 the time all caller really wants is to log the outcome, also present status
 string, encapsulating logic to examine state and describe exited-with-rc vs.
 killed-by-signal. New Status logic may report clearer results in the case of
 a Windows child process killed by exception. Clarify that static
 LLProcess::isRunning(handle) overload is only for use when the original
 LLProcess object has been destroyed: really only for unit tests. We
 necessarily retain our original platform-specific implementations for just
 that one method. (Nonstatic isRunning() no longer calls static method.)
 Clarify log output from llprocess_test.cpp in a couple places.

---
 indra/llcommon/llprocess.cpp | 552 +++++++++++++++++++++++++++++++------------
 1 file changed, 395 insertions(+), 157 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 8611d67f25..bc27002701 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -30,11 +30,15 @@
 #include "llsingleton.h"
 #include "llstring.h"
 #include "stringize.h"
+#include "llapr.h"
 
 #include <boost/foreach.hpp>
 #include <iostream>
 #include <stdexcept>
 
+static std::string empty;
+static LLProcess::Status interpret_status(int status);
+
 /// Need an exception to avoid constructing an invalid LLProcess object, but
 /// internal use only
 struct LLProcessError: public std::runtime_error
@@ -55,9 +59,14 @@ LLProcessPtr LLProcess::create(const LLSDOrParams& params)
 	}
 }
 
+/// Call an apr function returning apr_status_t. On failure, log warning and
+/// throw LLProcessError mentioning the function call that produced that
+/// result.
+#define chkapr(func)                            \
+    if (ll_apr_warn_status(func))               \
+        throw LLProcessError(#func " failed")
+
 LLProcess::LLProcess(const LLSDOrParams& params):
-	mProcessID(0),
-	mProcessHandle(0),
 	mAutokill(params.autokill)
 {
 	if (! params.validateBlock(true))
@@ -66,31 +75,298 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 									   << LLSDNotationStreamer(params)));
 	}
 
-	launch(params);
+	apr_procattr_t *procattr = NULL;
+	chkapr(apr_procattr_create(&procattr, gAPRPoolp));
+
+	// For which of stdin, stdout, stderr should we create a pipe to the
+	// child? In the viewer, there are only a couple viable
+	// apr_procattr_io_set() alternatives: inherit the viewer's own stdxxx
+	// handle (APR_NO_PIPE, e.g. for stdout, stderr), or create a pipe that's
+	// blocking on the child end but nonblocking at the viewer end
+	// (APR_CHILD_BLOCK). The viewer can't block for anything: the parent end
+	// MUST be nonblocking. As the APR documentation itself points out, it
+	// makes very little sense to set nonblocking I/O for the child end of a
+	// pipe: only a specially-written child could deal with that.
+	// Other major options could include explicitly creating a single APR pipe
+	// and passing it as both stdout and stderr (apr_procattr_child_out_set(),
+	// apr_procattr_child_err_set()), or accepting a filename, opening it and
+	// passing that apr_file_t (simple <, >, 2> redirect emulation).
+//	chkapr(apr_procattr_io_set(procattr, APR_CHILD_BLOCK, APR_CHILD_BLOCK, APR_CHILD_BLOCK));
+	chkapr(apr_procattr_io_set(procattr, APR_NO_PIPE, APR_NO_PIPE, APR_NO_PIPE));
+
+	// Thumbs down on implicitly invoking the shell to invoke the child. From
+	// our point of view, the other major alternative to APR_PROGRAM_PATH
+	// would be APR_PROGRAM_ENV: still copy environment, but require full
+	// executable pathname. I don't see a downside to searching the PATH,
+	// though: if our caller wants (e.g.) a specific Python interpreter, s/he
+	// can still pass the full pathname.
+	chkapr(apr_procattr_cmdtype_set(procattr, APR_PROGRAM_PATH));
+	// YES, do extra work if necessary to report child exec() failures back to
+	// parent process.
+	chkapr(apr_procattr_error_check_set(procattr, 1));
+	// Do not start a non-autokill child in detached state. On Posix
+	// platforms, this setting attempts to daemonize the new child, closing
+	// std handles and the like, and that's a bit more detachment than we
+	// want. autokill=false just means not to implicitly kill the child when
+	// the parent terminates!
+//	chkapr(apr_procattr_detach_set(procattr, params.autokill? 0 : 1));
+
+	if (params.autokill)
+	{
+#if defined(APR_HAS_PROCATTR_AUTOKILL_SET)
+		apr_status_t ok = apr_procattr_autokill_set(procattr, 1);
+# if LL_WINDOWS
+		// As of 2012-02-02, we only expect this to be implemented on Windows.
+		// Avoid spamming the log with warnings we fully expect.
+		ll_apr_warn_status(ok);
+# endif // LL_WINDOWS
+#else
+		LL_WARNS("LLProcess") << "This version of APR lacks Linden apr_procattr_autokill_set() extension" << LL_ENDL;
+#endif
+	}
+
+	// Have to instantiate named std::strings for string params items so their
+	// c_str() values persist.
+	std::string cwd(params.cwd);
+	if (! cwd.empty())
+	{
+		chkapr(apr_procattr_dir_set(procattr, cwd.c_str()));
+	}
+
+	// create an argv vector for the child process
+	std::vector<const char*> argv;
+
+	// add the executable path
+	std::string executable(params.executable);
+	argv.push_back(executable.c_str());
+
+	// and any arguments
+	std::vector<std::string> args(params.args.begin(), params.args.end());
+	BOOST_FOREACH(const std::string& arg, args)
+	{
+		argv.push_back(arg.c_str());
+	}
+
+	// terminate with a null pointer
+	argv.push_back(NULL);
+
+	// Launch! The NULL would be the environment block, if we were passing one.
+	chkapr(apr_proc_create(&mProcess, argv[0], &argv[0], NULL, procattr, gAPRPoolp));    
+
+	// arrange to call status_callback()
+	apr_proc_other_child_register(&mProcess, &LLProcess::status_callback, this, mProcess.in,
+								  gAPRPoolp);
+	mStatus.mState = RUNNING;
+
+	mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << mProcess.pid << ')');
+	LL_INFOS("LLProcess") << "Launched " << params << " (" << mProcess.pid << ")" << LL_ENDL;
+
+	// Unless caller explicitly turned off autokill (child should persist),
+	// take steps to terminate the child. This is all suspenders-and-belt: in
+	// theory our destructor should kill an autokill child, but in practice
+	// that doesn't always work (e.g. VWR-21538).
+	if (params.autokill)
+	{
+		// Tie the lifespan of this child process to the lifespan of our APR
+		// pool: on destruction of the pool, forcibly kill the process. Tell
+		// APR to try SIGTERM and wait 3 seconds. If that didn't work, use
+		// SIGKILL.
+		apr_pool_note_subprocess(gAPRPoolp, &mProcess, APR_KILL_AFTER_TIMEOUT);
+
+		// On Windows, associate the new child process with our Job Object.
+		autokill();
+	}
 }
 
 LLProcess::~LLProcess()
 {
+	// Only in state RUNNING are we registered for callback. In UNSTARTED we
+	// haven't yet registered. And since receiving the callback is the only
+	// way we detect child termination, we only change from state RUNNING at
+	// the same time we unregister.
+	if (mStatus.mState == RUNNING)
+	{
+		// We're still registered for a callback: unregister. Do it before
+		// we even issue the kill(): even if kill() somehow prompted an
+		// instantaneous callback (unlikely), this object is going away! Any
+		// information updated in this object by such a callback is no longer
+		// available to any consumer anyway.
+		apr_proc_other_child_unregister(this);
+	}
+
 	if (mAutokill)
 	{
-		kill();
+		kill("destructor");
+	}
+}
+
+bool LLProcess::kill(const std::string& who)
+{
+	if (isRunning())
+	{
+		LL_INFOS("LLProcess") << who << " killing " << mDesc << LL_ENDL;
+
+#if LL_WINDOWS
+		int sig = -1;
+#else  // Posix
+		int sig = SIGTERM;
+#endif
+
+		ll_apr_warn_status(apr_proc_kill(&mProcess, sig));
 	}
+
+	return ! isRunning();
 }
 
 bool LLProcess::isRunning(void)
 {
-	mProcessHandle = isRunning(mProcessHandle, mDesc);
-	return (mProcessHandle != 0);
+	return getStatus().mState == RUNNING;
+}
+
+LLProcess::Status LLProcess::getStatus()
+{
+	// Only when mState is RUNNING might the status change dynamically. For
+	// any other value, pointless to attempt to update status: it won't
+	// change.
+	if (mStatus.mState == RUNNING)
+	{
+		// Tell APR to sense whether the child is still running and call
+		// handle_status() appropriately. We should be able to get the same
+		// info from an apr_proc_wait(APR_NOWAIT) call; but at least in APR
+		// 1.4.2, testing suggests that even with APR_NOWAIT, apr_proc_wait()
+		// blocks the caller. We can't have that in the viewer. Hence the
+		// callback rigmarole. Once we update APR, it's probably worth testing
+		// again. Also -- although there's an apr_proc_other_child_refresh()
+		// call, i.e. get that information for one specific child, it accepts
+		// an 'apr_other_child_rec_t*' that's mentioned NOWHERE else in the
+		// documentation or header files! I would use the specific call if I
+		// knew how. As it is, each call to this method will call callbacks
+		// for ALL still-running child processes. Sigh...
+		apr_proc_other_child_refresh_all(APR_OC_REASON_RUNNING);
+	}
+
+	return mStatus;
+}
+
+std::string LLProcess::getStatusString()
+{
+	return getStatusString(getStatus());
+}
+
+std::string LLProcess::getStatusString(const Status& status)
+{
+	return getStatusString(mDesc, status);
+}
+
+//static
+std::string LLProcess::getStatusString(const std::string& desc, const Status& status)
+{
+	if (status.mState == UNSTARTED)
+		return desc + " was never launched";
+
+	if (status.mState == RUNNING)
+		return desc + " running";
+
+	if (status.mState == EXITED)
+		return STRINGIZE(desc << " exited with code " << status.mData);
+
+	if (status.mState == KILLED)
+#if LL_WINDOWS
+		return STRINGIZE(desc << " killed with exception " << std::hex << status.mData);
+#else
+		return STRINGIZE(desc << " killed by signal " << status.mData);
+#endif
+
+
+	return STRINGIZE(desc << " in unknown state " << status.mState << " (" << status.mData << ")");
+}
+
+// Classic-C-style APR callback
+void LLProcess::status_callback(int reason, void* data, int status)
+{
+	// Our only role is to bounce this static method call back into object
+	// space.
+	static_cast<LLProcess*>(data)->handle_status(reason, status);
+}
+
+#define tabent(symbol) { symbol, #symbol }
+static struct ReasonCode
+{
+	int code;
+	const char* name;
+} reasons[] =
+{
+	tabent(APR_OC_REASON_DEATH),
+	tabent(APR_OC_REASON_UNWRITABLE),
+	tabent(APR_OC_REASON_RESTART),
+	tabent(APR_OC_REASON_UNREGISTER),
+	tabent(APR_OC_REASON_LOST),
+	tabent(APR_OC_REASON_RUNNING)
+};
+#undef tabent
+
+// Object-oriented callback
+void LLProcess::handle_status(int reason, int status)
+{
+	{
+		// This odd appearance of LL_DEBUGS is just to bracket a lookup that will
+		// only be performed if in fact we're going to produce the log message.
+		LL_DEBUGS("LLProcess") << empty;
+		std::string reason_str;
+		BOOST_FOREACH(const ReasonCode& rcp, reasons)
+		{
+			if (reason == rcp.code)
+			{
+				reason_str = rcp.name;
+				break;
+			}
+		}
+		if (reason_str.empty())
+		{
+			reason_str = STRINGIZE("unknown reason " << reason);
+		}
+		LL_CONT << mDesc << ": handle_status(" << reason_str << ", " << status << ")" << LL_ENDL;
+	}
+
+	if (! (reason == APR_OC_REASON_DEATH || reason == APR_OC_REASON_LOST))
+	{
+		// We're only interested in the call when the child terminates.
+		return;
+	}
+
+	// Somewhat oddly, APR requires that you explicitly unregister even when
+	// it already knows the child has terminated. We must pass the same 'data'
+	// pointer as for the register() call, which was our 'this'.
+	apr_proc_other_child_unregister(this);
+	// We overload mStatus.mState to indicate whether the child is registered
+	// for APR callback: only RUNNING means registered. Track that we've
+	// unregistered. We know the child has terminated; might be EXITED or
+	// KILLED; refine below.
+	mStatus.mState = EXITED;
+
+//	wi->rv = apr_proc_wait(wi->child, &wi->rc, &wi->why, APR_NOWAIT);
+	// It's just wrong to call apr_proc_wait() here. The only way APR knows to
+	// call us with APR_OC_REASON_DEATH is that it's already reaped this child
+	// process, so calling wait() will only produce "huh?" from the OS. We
+	// must rely on the status param passed in, which unfortunately comes
+	// straight from the OS wait() call, which means we have to decode it by
+	// hand.
+	mStatus = interpret_status(status);
+	LL_INFOS("LLProcess") << getStatusString() << LL_ENDL;
 }
 
 LLProcess::id LLProcess::getProcessID() const
 {
-	return mProcessID;
+	return mProcess.pid;
 }
 
 LLProcess::handle LLProcess::getProcessHandle() const
 {
-	return mProcessHandle;
+#if LL_WINDOWS
+	return mProcess.hproc;
+#else
+	return mProcess.pid;
+#endif
 }
 
 std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params)
@@ -178,77 +454,15 @@ private:
 	LLProcess::handle mJob;
 };
 
-void LLProcess::launch(const LLSDOrParams& params)
+void LLProcess::autokill()
 {
-	PROCESS_INFORMATION pinfo;
-	STARTUPINFOA sinfo = { sizeof(sinfo) };
-
-	// LLProcess::create()'s caller passes a Unix-style array of strings for
-	// command-line arguments. Our caller can and should expect that these will be
-	// passed to the child process as individual arguments, regardless of content
-	// (e.g. embedded spaces). But because Windows invokes any child process with
-	// a single command-line string, this means we must quote each argument behind
-	// the scenes.
-	std::string args = LLStringUtil::quote(params.executable);
-	BOOST_FOREACH(const std::string& arg, params.args)
-	{
-		args += " ";
-		args += LLStringUtil::quote(arg);
-	}
-
-	// So retarded.  Windows requires that the second parameter to
-	// CreateProcessA be a writable (non-const) string...
-	std::vector<char> args2(args.begin(), args.end());
-	args2.push_back('\0');
-
-	// Convert wrapper to a real std::string so we can use c_str(); but use a
-	// named variable instead of a temporary so c_str() pointer remains valid.
-	std::string cwd(params.cwd);
-	const char * working_directory = 0;
-	if (! cwd.empty())
-		working_directory = cwd.c_str();
-
-	// It's important to pass CREATE_BREAKAWAY_FROM_JOB because Windows 7 et
-	// al. tend to implicitly launch new processes already bound to a job. From
-	// http://msdn.microsoft.com/en-us/library/windows/desktop/ms681949%28v=vs.85%29.aspx :
-	// "The process must not already be assigned to a job; if it is, the
-	// function fails with ERROR_ACCESS_DENIED." ...
-	// "If the process is being monitored by the Program Compatibility
-	// Assistant (PCA), it is placed into a compatibility job. Therefore, the
-	// process must be created using CREATE_BREAKAWAY_FROM_JOB before it can
-	// be placed in another job."
-	if( ! CreateProcessA(NULL,      // lpApplicationName
-                         &args2[0], // lpCommandLine
-                         NULL,      // lpProcessAttributes
-                         NULL,      // lpThreadAttributes
-                         FALSE,     // bInheritHandles
-                         CREATE_BREAKAWAY_FROM_JOB, // dwCreationFlags
-                         NULL,      // lpEnvironment
-                         working_directory, // lpCurrentDirectory
-                         &sinfo,            // lpStartupInfo
-                         &pinfo ) )         // lpProcessInformation
-	{
-		throw LLProcessError(WindowsErrorString("CreateProcessA"));
-	}
-
-	// CloseHandle(pinfo.hProcess); // stops leaks - nothing else
-	mProcessID = pinfo.dwProcessId;
-	mProcessHandle = pinfo.hProcess;
-	CloseHandle(pinfo.hThread); // stops leaks - nothing else
-
-	mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << mProcessID << ')');
-	LL_INFOS("LLProcess") << "Launched " << params << " (" << mProcessID << ")" << LL_ENDL;
-
-	// Now associate the new child process with our Job Object -- unless
-	// autokill is false, i.e. caller asserts the child should persist.
-	if (params.autokill)
-	{
-		LLJob::instance().assignProcess(mDesc, mProcessHandle);
-}
+	LLJob::instance().assignProcess(mDesc, mProcess.hproc);
 }
 
 LLProcess::handle LLProcess::isRunning(handle h, const std::string& desc)
 {
+	// This direct Windows implementation is because we have no access to the
+	// apr_proc_t struct: we expect it's been destroyed.
 	if (! h)
 		return 0;
 
@@ -258,22 +472,44 @@ LLProcess::handle LLProcess::isRunning(handle h, const std::string& desc)
 		// the process has completed.
 		if (! desc.empty())
 		{
-			LL_INFOS("LLProcess") << desc << " terminated" << LL_ENDL;
+			DWORD status = 0;
+			if (! GetExitCodeProcess(h, &status))
+			{
+				LL_WARNS("LLProcess") << desc << " terminated, but "
+									  << WindowsErrorString("GetExitCodeProcess()") << LL_ENDL;
+			}
+			{
+				LL_INFOS("LLProcess") << getStatusString(desc, interpret_status(status))
+									  << LL_ENDL;
+			}
 		}
+		CloseHandle(h);
 		return 0;
 	}
 
 	return h;
 }
 
-bool LLProcess::kill(void)
+static LLProcess::Status interpret_status(int status)
 {
-	if (! mProcessHandle)
-		return false;
+	LLProcess::Status result;
+
+	// This bit of code is cribbed from apr/threadproc/win32/proc.c, a
+	// function (unfortunately static) called why_from_exit_code():
+	/* See WinNT.h STATUS_ACCESS_VIOLATION and family for how
+	 * this class of failures was determined
+	 */
+	if ((status & 0xFFFF0000) == 0xC0000000)
+	{
+		result.mState = KILLED;
+	}
+	else
+	{
+		result.mState = EXITED;
+	}
+	result.mData = status;
 
-	LL_INFOS("LLProcess") << "killing " << mDesc << LL_ENDL;
-	TerminateProcess(mProcessHandle, 0);
-	return ! isRunning();
+	return result;
 }
 
 /// GetLastError()/FormatMessage() boilerplate
@@ -315,98 +551,91 @@ static std::string WindowsErrorString(const std::string& operation)
 #include <errno.h>
 #include <sys/wait.h>
 
+void LLProcess::autokill()
+{
+	// What we ought to do here is to:
+	// 1. create a unique process group and run all autokill children in that
+	//    group (see https://jira.secondlife.com/browse/SWAT-563);
+	// 2. figure out a way to intercept control when the viewer exits --
+	//    gracefully or not; 
+	// 3. when the viewer exits, kill off the aforementioned process group.
+
+	// It's point 2 that's troublesome. Although I've seen some signal-
+	// handling logic in the Posix viewer code, I haven't yet found any bit of
+	// code that's run no matter how the viewer exits (a try/finally for the
+	// whole process, as it were).
+}
+
 // Attempt to reap a process ID -- returns true if the process has exited and been reaped, false otherwise.
-static bool reap_pid(pid_t pid)
+static bool reap_pid(pid_t pid, LLProcess::Status* pstatus=NULL)
 {
-	pid_t wait_result = ::waitpid(pid, NULL, WNOHANG);
+	LLProcess::Status dummy;
+	if (! pstatus)
+	{
+		// If caller doesn't want to see Status, give us a target anyway so we
+		// don't have to have a bunch of conditionals.
+		pstatus = &dummy;
+	}
+
+	int status = 0;
+	pid_t wait_result = ::waitpid(pid, &status, WNOHANG);
 	if (wait_result == pid)
 	{
+		*pstatus = interpret_status(status);
 		return true;
 	}
-	if (wait_result == -1 && errno == ECHILD)
+	if (wait_result == 0)
 	{
-		// No such process -- this may mean we're ignoring SIGCHILD.
-		return true;
+		pstatus->mState = LLProcess::RUNNING;
+		pstatus->mData	= 0;
+		return false;
 	}
-	
-	return false;
-}
 
-void LLProcess::launch(const LLSDOrParams& params)
-{
-	// flush all buffers before the child inherits them
-	::fflush(NULL);
+	// Clear caller's Status block; caller must interpret UNSTARTED to mean
+	// "if this PID was ever valid, it no longer is."
+	*pstatus = LLProcess::Status();
 
-	pid_t child = vfork();
-	if (child == 0)
+	// We've dealt with the success cases: we were able to reap the child
+	// (wait_result == pid) or it's still running (wait_result == 0). It may
+	// be that the child terminated but didn't hang around long enough for us
+	// to reap. In that case we still have no Status to report, but we can at
+	// least state that it's not running.
+	if (wait_result == -1 && errno == ECHILD)
 	{
-		// child process
-
-		std::string cwd(params.cwd);
-		if (! cwd.empty())
-		{
-			// change to the desired child working directory
-			if (::chdir(cwd.c_str()))
-			{
-				// chdir failed
-				LL_WARNS("LLProcess") << "could not chdir(\"" << cwd << "\")" << LL_ENDL;
-				// pointless to throw; this is child process...
-				_exit(248);
-			}
-		}
-
-		// create an argv vector for the child process
-		std::vector<const char*> fake_argv;
-
-		// add the executable path
-		std::string executable(params.executable);
-		fake_argv.push_back(executable.c_str());
-
-		// and any arguments
-		std::vector<std::string> args(params.args.begin(), params.args.end());
-		BOOST_FOREACH(const std::string& arg, args)
-		{
-			fake_argv.push_back(arg.c_str());
-		}
-
-		// terminate with a null pointer
-		fake_argv.push_back(NULL);
-
-		::execv(executable.c_str(), const_cast<char* const*>(&fake_argv[0]));
-
-		// If we reach this point, the exec failed.
-		LL_WARNS("LLProcess") << "failed to launch: ";
-		BOOST_FOREACH(const char* arg, fake_argv)
-		{
-			LL_CONT << arg << ' ';
-		}
-		LL_CONT << LL_ENDL;
-		// Use _exit() instead of exit() per the vfork man page. Exit with a
-		// distinctive rc: someday soon we'll be able to retrieve it, and it
-		// would be nice to be able to tell that the child process failed!
-		_exit(249);
+		// No such process -- this may mean we're ignoring SIGCHILD.
+		return true;
 	}
 
-	// parent process
-	mProcessID = child;
-	mProcessHandle = child;
-
-	mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << mProcessID << ')');
-	LL_INFOS("LLProcess") << "Launched " << params << " (" << mProcessID << ")" << LL_ENDL;
+	// Uh, should never happen?!
+	LL_WARNS("LLProcess") << "LLProcess::reap_pid(): waitpid(" << pid << ") returned "
+						  << wait_result << "; not meaningful?" << LL_ENDL;
+	// If caller is looping until this pid terminates, and if we can't find
+	// out, better to break the loop than to claim it's still running.
+	return true;
 }
 
 LLProcess::id LLProcess::isRunning(id pid, const std::string& desc)
 {
+	// This direct Posix implementation is because we have no access to the
+	// apr_proc_t struct: we expect it's been destroyed.
 	if (! pid)
 		return 0;
 
 	// Check whether the process has exited, and reap it if it has.
-	if(reap_pid(pid))
+	LLProcess::Status status;
+	if(reap_pid(pid, &status))
 	{
 		// the process has exited.
 		if (! desc.empty())
 		{
-			LL_INFOS("LLProcess") << desc << " terminated" << LL_ENDL;
+			std::string statstr(desc + " apparently terminated: no status available");
+			// We don't just pass UNSTARTED to getStatusString() because, in
+			// the context of reap_pid(), that state has special meaning.
+			if (status.mState != UNSTARTED)
+			{
+				statstr = getStatusString(desc, status);
+			}
+			LL_INFOS("LLProcess") << statstr << LL_ENDL;
 		}
 		return 0;
 	}
@@ -414,18 +643,27 @@ LLProcess::id LLProcess::isRunning(id pid, const std::string& desc)
 	return pid;
 }
 
-bool LLProcess::kill(void)
+static LLProcess::Status interpret_status(int status)
 {
-	if (! mProcessID)
-		return false;
+	LLProcess::Status result;
 
-	// Try to kill the process. We'll do approximately the same thing whether
-	// the kill returns an error or not, so we ignore the result.
-	LL_INFOS("LLProcess") << "killing " << mDesc << LL_ENDL;
-	(void)::kill(mProcessID, SIGTERM);
+	if (WIFEXITED(status))
+	{
+		result.mState = LLProcess::EXITED;
+		result.mData  = WEXITSTATUS(status);
+	}
+	else if (WIFSIGNALED(status))
+	{
+		result.mState = LLProcess::KILLED;
+		result.mData  = WTERMSIG(status);
+	}
+	else                            // uh, shouldn't happen?
+	{
+		result.mState = LLProcess::EXITED;
+		result.mData  = status;     // someone else will have to decode
+	}
 
-	// This will have the side-effect of reaping the zombie if the process has exited.
-	return ! isRunning();
+	return result;
 }
 
 /*==========================================================================*|
-- 
cgit v1.2.3


From 219a010aaf4891fc2ab8480a2121b1244191c24d Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Tue, 7 Feb 2012 11:17:04 -0500
Subject: LLProcess::Status enum values need qualification in helper function.

---
 indra/llcommon/llprocess.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index bc27002701..1305c1d764 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -501,11 +501,11 @@ static LLProcess::Status interpret_status(int status)
 	 */
 	if ((status & 0xFFFF0000) == 0xC0000000)
 	{
-		result.mState = KILLED;
+		result.mState = LLProcess::KILLED;
 	}
 	else
 	{
-		result.mState = EXITED;
+		result.mState = LLProcess::EXITED;
 	}
 	result.mData = status;
 
-- 
cgit v1.2.3


From 90a1b67cc43792e1ca0047eaca51530aa14e134c Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Thu, 9 Feb 2012 15:22:24 -0500
Subject: Remove LLJob class: apr_procattr_autokill_set() should now handle.
 LLJob was vestigial code from before migrating Job Object support into APR.
 Also add APR signal-name string to getStatusString() output.

---
 indra/llcommon/llprocess.cpp | 70 +++-----------------------------------------
 1 file changed, 4 insertions(+), 66 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 1305c1d764..7ccbdeed01 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -31,6 +31,7 @@
 #include "llstring.h"
 #include "stringize.h"
 #include "llapr.h"
+#include "apr_signal.h"
 
 #include <boost/foreach.hpp>
 #include <iostream>
@@ -274,10 +275,10 @@ std::string LLProcess::getStatusString(const std::string& desc, const Status& st
 #if LL_WINDOWS
 		return STRINGIZE(desc << " killed with exception " << std::hex << status.mData);
 #else
-		return STRINGIZE(desc << " killed by signal " << status.mData);
+		return STRINGIZE(desc << " killed by signal " << status.mData
+						 << " (" << apr_signal_description_get(status.mData) << ")");
 #endif
 
-
 	return STRINGIZE(desc << " in unknown state " << status.mState << " (" << status.mData << ")");
 }
 
@@ -391,72 +392,9 @@ std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params)
 
 static std::string WindowsErrorString(const std::string& operation);
 
-/**
- * Wrap a Windows Job Object for use in managing child-process lifespan.
- *
- * On Windows, we use a Job Object to constrain the lifespan of any
- * autokill=true child process to the viewer's own lifespan:
- * http://stackoverflow.com/questions/53208/how-do-i-automatically-destroy-child-processes-in-windows
- * (thanks Richard!).
- *
- * We manage it using an LLSingleton for a couple of reasons:
- *
- * # Lazy initialization: if some viewer session never launches a child
- *   process, we should never have to create a Job Object.
- * # Cross-DLL support: be wary of C++ statics when multiple DLLs are
- *   involved.
- */
-class LLJob: public LLSingleton<LLJob>
-{
-public:
-	void assignProcess(const std::string& prog, LLProcess::handle hProcess)
-	{
-		// If we never managed to initialize this Job Object, can't use it --
-		// but don't keep spamming the log, we already emitted warnings when
-		// we first tried to create.
-		if (! mJob)
-			return;
-
-		if (! AssignProcessToJobObject(mJob, hProcess))
-		{
-			LL_WARNS("LLProcess") << WindowsErrorString(STRINGIZE("AssignProcessToJobObject("
-																  << prog << ")")) << LL_ENDL;
-		}
-	}
-
-private:
-	friend class LLSingleton<LLJob>;
-	LLJob():
-		mJob(0)
-	{
-		mJob = CreateJobObject(NULL, NULL);
-		if (! mJob)
-		{
-			LL_WARNS("LLProcess") << WindowsErrorString("CreateJobObject()") << LL_ENDL;
-			return;
-		}
-
-		JOBOBJECT_EXTENDED_LIMIT_INFORMATION jeli = { 0 };
-
-		// Configure all child processes associated with this new job object
-		// to terminate when the calling process (us!) terminates.
-		jeli.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
-		if (! SetInformationJobObject(mJob, JobObjectExtendedLimitInformation, &jeli, sizeof(jeli)))
-		{
-			LL_WARNS("LLProcess") << WindowsErrorString("SetInformationJobObject()") << LL_ENDL;
-			// This Job Object is useless to us
-			CloseHandle(mJob);
-			// prevent assignProcess() from trying to use it
-			mJob = 0;
-		}
-	}
-
-	LLProcess::handle mJob;
-};
-
 void LLProcess::autokill()
 {
-	LLJob::instance().assignProcess(mDesc, mProcess.hproc);
+	// hopefully now handled by apr_procattr_autokill_set()
 }
 
 LLProcess::handle LLProcess::isRunning(handle h, const std::string& desc)
-- 
cgit v1.2.3


From 0f2882ec95fc6cd2649fbe4e952ce1bc586bb853 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Mon, 13 Feb 2012 09:41:50 -0500
Subject: Suppress a specific unused-var warning on Posix platforms.

---
 indra/llcommon/llprocess.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 7ccbdeed01..de71595f16 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -120,7 +120,9 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 		// As of 2012-02-02, we only expect this to be implemented on Windows.
 		// Avoid spamming the log with warnings we fully expect.
 		ll_apr_warn_status(ok);
-# endif // LL_WINDOWS
+#else   // ! LL_WINDOWS
+		(void)ok;                   // suppress 'unused' warning
+# endif // ! LL_WINDOWS
 #else
 		LL_WARNS("LLProcess") << "This version of APR lacks Linden apr_procattr_autokill_set() extension" << LL_ENDL;
 #endif
-- 
cgit v1.2.3


From aae61392be822218cabcab91d95eb1e75d471764 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Mon, 13 Feb 2012 17:38:25 -0500
Subject: Use per-frame ticks on "mainloop" LLEventPump to update LLProcess.
 When we reimplemented LLProcess on APR, necessitating APR's funny callback
 mechanism to sense child-process status, every isRunning() or getStatus()
 call called the APR poll function that calls ALL registered LLProcess
 callbacks. In other words, every time any consumer called any
 LLProcess::isRunning() method, all LLProcess callbacks were redundantly
 fired. Change that so that the single APR poll function is called once per
 frame, courtesy of the "mainloop" LLEventPump. Once per viewer frame should
 be well within the realtime duration in which it's reasonable to expect
 child-process status to change. In effect, this changes LLProcess's public
 API to introduce a dependency on "mainloop" ticks. Add such ticks to
 llprocess_test.cpp as well.

---
 indra/llcommon/llprocess.cpp | 96 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 76 insertions(+), 20 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index de71595f16..b13e8eb8e0 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -32,8 +32,10 @@
 #include "stringize.h"
 #include "llapr.h"
 #include "apr_signal.h"
+#include "llevents.h"
 
 #include <boost/foreach.hpp>
+#include <boost/bind.hpp>
 #include <iostream>
 #include <stdexcept>
 
@@ -47,6 +49,74 @@ struct LLProcessError: public std::runtime_error
 	LLProcessError(const std::string& msg): std::runtime_error(msg) {}
 };
 
+/**
+ * Ref-counted "mainloop" listener. As long as there are still outstanding
+ * LLProcess objects, keep listening on "mainloop" so we can keep polling APR
+ * for process status.
+ */
+class LLProcessListener
+{
+	LOG_CLASS(LLProcessListener);
+public:
+	LLProcessListener():
+		mCount(0)
+	{}
+
+	void addPoll(const LLProcess&)
+	{
+		// Unconditionally increment mCount. If it was zero before
+		// incrementing, listen on "mainloop".
+		if (mCount++ == 0)
+		{
+			LL_DEBUGS("LLProcess") << "listening on \"mainloop\"" << LL_ENDL;
+			mConnection = LLEventPumps::instance().obtain("mainloop")
+				.listen("LLProcessListener", boost::bind(&LLProcessListener::tick, this, _1));
+		}
+	}
+
+	void dropPoll(const LLProcess&)
+	{
+		// Unconditionally decrement mCount. If it's zero after decrementing,
+		// stop listening on "mainloop".
+		if (--mCount == 0)
+		{
+			LL_DEBUGS("LLProcess") << "disconnecting from \"mainloop\"" << LL_ENDL;
+			mConnection.disconnect();
+		}
+	}
+
+private:
+	/// called once per frame by the "mainloop" LLEventPump
+	bool tick(const LLSD&)
+	{
+		// Tell APR to sense whether each registered LLProcess is still
+		// running and call handle_status() appropriately. We should be able
+		// to get the same info from an apr_proc_wait(APR_NOWAIT) call; but at
+		// least in APR 1.4.2, testing suggests that even with APR_NOWAIT,
+		// apr_proc_wait() blocks the caller. We can't have that in the
+		// viewer. Hence the callback rigmarole. (Once we update APR, it's
+		// probably worth testing again.) Also -- although there's an
+		// apr_proc_other_child_refresh() call, i.e. get that information for
+		// one specific child, it accepts an 'apr_other_child_rec_t*' that's
+		// mentioned NOWHERE else in the documentation or header files! I
+		// would use the specific call in LLProcess::getStatus() if I knew
+		// how. As it is, each call to apr_proc_other_child_refresh_all() will
+		// call callbacks for ALL still-running child processes. That's why we
+		// centralize such calls, using "mainloop" to ensure it happens once
+		// per frame, and refcounting running LLProcess objects to remain
+		// registered only while needed.
+		LL_DEBUGS("LLProcess") << "calling apr_proc_other_child_refresh_all()" << LL_ENDL;
+		apr_proc_other_child_refresh_all(APR_OC_REASON_RUNNING);
+		return false;
+	}
+
+	/// If this object is destroyed before mCount goes to zero, stop
+	/// listening on "mainloop" anyway.
+	LLTempBoundListener mConnection;
+	unsigned mCount;
+};
+static LLProcessListener sProcessListener;
+
 LLProcessPtr LLProcess::create(const LLSDOrParams& params)
 {
 	try
@@ -159,6 +229,8 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 	// arrange to call status_callback()
 	apr_proc_other_child_register(&mProcess, &LLProcess::status_callback, this, mProcess.in,
 								  gAPRPoolp);
+	// and make sure we poll it once per "mainloop" tick
+	sProcessListener.addPoll(*this);
 	mStatus.mState = RUNNING;
 
 	mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << mProcess.pid << ')');
@@ -195,6 +267,8 @@ LLProcess::~LLProcess()
 		// information updated in this object by such a callback is no longer
 		// available to any consumer anyway.
 		apr_proc_other_child_unregister(this);
+		// One less LLProcess to poll for
+		sProcessListener.dropPoll(*this);
 	}
 
 	if (mAutokill)
@@ -228,26 +302,6 @@ bool LLProcess::isRunning(void)
 
 LLProcess::Status LLProcess::getStatus()
 {
-	// Only when mState is RUNNING might the status change dynamically. For
-	// any other value, pointless to attempt to update status: it won't
-	// change.
-	if (mStatus.mState == RUNNING)
-	{
-		// Tell APR to sense whether the child is still running and call
-		// handle_status() appropriately. We should be able to get the same
-		// info from an apr_proc_wait(APR_NOWAIT) call; but at least in APR
-		// 1.4.2, testing suggests that even with APR_NOWAIT, apr_proc_wait()
-		// blocks the caller. We can't have that in the viewer. Hence the
-		// callback rigmarole. Once we update APR, it's probably worth testing
-		// again. Also -- although there's an apr_proc_other_child_refresh()
-		// call, i.e. get that information for one specific child, it accepts
-		// an 'apr_other_child_rec_t*' that's mentioned NOWHERE else in the
-		// documentation or header files! I would use the specific call if I
-		// knew how. As it is, each call to this method will call callbacks
-		// for ALL still-running child processes. Sigh...
-		apr_proc_other_child_refresh_all(APR_OC_REASON_RUNNING);
-	}
-
 	return mStatus;
 }
 
@@ -341,6 +395,8 @@ void LLProcess::handle_status(int reason, int status)
 	// it already knows the child has terminated. We must pass the same 'data'
 	// pointer as for the register() call, which was our 'this'.
 	apr_proc_other_child_unregister(this);
+	// don't keep polling for a terminated process
+	sProcessListener.dropPoll(*this);
 	// We overload mStatus.mState to indicate whether the child is registered
 	// for APR callback: only RUNNING means registered. Track that we've
 	// unregistered. We know the child has terminated; might be EXITED or
-- 
cgit v1.2.3


From e239cad1f509e3d96011acb61614f2481c46af38 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Wed, 15 Feb 2012 10:07:09 -0500
Subject: Preliminary pipe support for LLProcess. Add LLProcess::FileParam to
 specify how to construct each child's standard file slot, with lots of
 comments about features designed but not yet implemented. The point is to
 design it with enough flexibility to be able to extend to foreseeable use
 cases. Add LLProcess::Params::files to collect up to 3 FileParam items.
 Naturally this extends the accepted LLSD syntax as well. Implement type=""
 (child inherits parent file descriptor) and "pipe" (parent constructs
 anonymous pipe to pass to child). Add LLProcess::FILESLOT enum, plus methods:
 getReadPipe(FILESLOT), getOptReadPipe(FILESLOT) getWritePipe(),
 getOptWritePipe() getPipeName(FILESLOT): placeholder implementation for now
 Add LLProcess::ReadPipe and WritePipe classes, as returned by get*Pipe().
 WritePipe supports get_ostream() method for streaming to child stdin.
 ReadPipe supports get_istream() method for reading from child stdout/stderr.
 It also provides getPump() returning LLEventPump& so interested parties can
 listen for arrival of new data on the aforementioned std::istream. For "pipe"
 slots, instantiate appropriate *Pipe class. ReadPipe and WritePipe classes
 are pure virtual bases for ReadPipeImpl and WritePipeImpl, respectively: all
 implementation data are hidden in the latter classes, visible only in
 llprocess.cpp. In fact each *PipeImpl class registers itself for "mainloop"
 ticks, attempting nonblocking I/O to the underlying apr_file_t on each tick.
 Data are buffered in a boost::asio::streambuf, which bridges between
 std::[io]stream and the APR I/O calls. Sanity-test ReadPipeImpl by using a
 pipe to absorb the Python "SyntaxError" output from the successful
 syntax_error test, rather than alarming the user. Add first few unit tests
 for validating FileParam. More tests coming!

---
 indra/llcommon/llprocess.cpp | 321 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 305 insertions(+), 16 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index b13e8eb8e0..55eb7e69d3 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -26,6 +26,7 @@
 
 #include "linden_common.h"
 #include "llprocess.h"
+#include "llsdutil.h"
 #include "llsdserialize.h"
 #include "llsingleton.h"
 #include "llstring.h"
@@ -36,19 +37,20 @@
 
 #include <boost/foreach.hpp>
 #include <boost/bind.hpp>
+#include <boost/asio/streambuf.hpp>
+#include <boost/asio/buffers_iterator.hpp>
 #include <iostream>
 #include <stdexcept>
+#include <limits>
+#include <algorithm>
+#include <vector>
+#include <typeinfo>
+#include <utility>
 
+static const char* whichfile[] = { "stdin", "stdout", "stderr" };
 static std::string empty;
 static LLProcess::Status interpret_status(int status);
 
-/// Need an exception to avoid constructing an invalid LLProcess object, but
-/// internal use only
-struct LLProcessError: public std::runtime_error
-{
-	LLProcessError(const std::string& msg): std::runtime_error(msg) {}
-};
-
 /**
  * Ref-counted "mainloop" listener. As long as there are still outstanding
  * LLProcess objects, keep listening on "mainloop" so we can keep polling APR
@@ -117,6 +119,154 @@ private:
 };
 static LLProcessListener sProcessListener;
 
+LLProcess::BasePipe::~BasePipe() {}
+
+class WritePipeImpl: public LLProcess::WritePipe
+{
+public:
+	WritePipeImpl(const std::string& desc, apr_file_t* pipe):
+		mDesc(desc),
+		mPipe(pipe),
+		// Essential to initialize our std::ostream with our special streambuf!
+		mStream(&mStreambuf)
+	{
+		mConnection = LLEventPumps::instance().obtain("mainloop")
+			.listen(LLEventPump::inventName("WritePipe"),
+					boost::bind(&WritePipeImpl::tick, this, _1));
+	}
+
+	virtual std::ostream& get_ostream() { return mStream; }
+
+	bool tick(const LLSD&)
+	{
+		// If there's anything to send, try to send it.
+		if (mStreambuf.size())
+		{
+			// Copy data out from mStreambuf to a flat, contiguous buffer to
+			// write -- but only up to a certain size.
+			std::streamsize total(mStreambuf.size());
+			std::streamsize bufsize((std::min)(4096, total));
+			boost::asio::streambuf::const_buffers_type bufs = mStreambuf.data();
+			std::vector<char> buffer(
+				boost::asio::buffers_begin(bufs),
+				boost::asio::buffers_begin(bufs) + bufsize);
+			apr_size_t written(bufsize);
+			ll_apr_warn_status(apr_file_write(mPipe, &buffer[0], &written));
+			// 'written' is modified to reflect the number of bytes actually
+			// written. Since they've been sent, remove them from the
+			// streambuf so we don't keep trying to send them. This could be
+			// anywhere from 0 up to mStreambuf.size(); anything we haven't
+			// yet sent, we'll try again next tick() call.
+			mStreambuf.consume(written);
+			LL_DEBUGS("LLProcess") << "wrote " << written << " of " << bufsize
+								   << " bytes to " << mDesc
+								   << " (original " << total << "), "
+								   << mStreambuf.size() << " remaining" << LL_ENDL;
+		}
+		return false;
+	}
+
+private:
+	std::string mDesc;
+	apr_file_t* mPipe;
+	LLTempBoundListener mConnection;
+	boost::asio::streambuf mStreambuf;
+	std::ostream mStream;
+};
+
+class ReadPipeImpl: public LLProcess::ReadPipe
+{
+public:
+	ReadPipeImpl(const std::string& desc, apr_file_t* pipe):
+		mDesc(desc),
+		mPipe(pipe),
+		// Essential to initialize our std::istream with our special streambuf!
+		mStream(&mStreambuf),
+		mPump("ReadPipe"),
+		// use funky syntax to call max() to avoid blighted max() macros
+		mLimit((std::numeric_limits<size_t>::max)())
+	{
+		mConnection = LLEventPumps::instance().obtain("mainloop")
+			.listen(LLEventPump::inventName("ReadPipe"),
+					boost::bind(&ReadPipeImpl::tick, this, _1));
+	}
+
+	// Much of the implementation is simply connecting the abstract virtual
+	// methods with implementation data concealed from the base class.
+	virtual std::istream& get_istream() { return mStream; }
+	virtual LLEventPump& getPump() { return mPump; }
+	virtual void setLimit(size_t limit) { mLimit = limit; }
+	virtual size_t getLimit() const { return mLimit; }
+
+private:
+	bool tick(const LLSD&)
+	{
+		// Allocate a buffer and try, every time, to read into it.
+		std::vector<char> buffer(4096);
+		apr_size_t gotten(buffer.size());
+		apr_status_t err = apr_file_read(mPipe, &buffer[0], &gotten);
+		if (err == APR_EOF)
+		{
+			// Handle EOF specially: it's part of normal-case processing.
+			LL_DEBUGS("LLProcess") << "EOF on " << mDesc << LL_ENDL;
+			// We won't need any more tick() calls.
+			mConnection.disconnect();
+		}
+		else if (! ll_apr_warn_status(err)) // validate anything but EOF
+		{
+			// 'gotten' was modified to reflect the number of bytes actually
+			// received. If nonzero, add them to the streambuf and notify
+			// interested parties.
+			if (gotten)
+			{
+				boost::asio::streambuf::mutable_buffers_type mbufs = mStreambuf.prepare(gotten);
+				std::copy(buffer.begin(), buffer.begin() + gotten,
+						  boost::asio::buffers_begin(mbufs));
+				// Don't forget to "commit" the data! The sequence (prepare(),
+				// commit()) is obviously intended to allow us to allocate
+				// buffer space, then read directly into some portion of it,
+				// then commit only as much as we managed to obtain. But the
+				// only official (documented) way I can find to populate a
+				// mutable_buffers_type is to use buffers_begin(). It Would Be
+				// Nice if we were permitted to directly read into
+				// mutable_buffers_type (not to mention writing directly from
+				// const_buffers_type in WritePipeImpl; APR even supports an
+				// apr_file_writev() function for writing from discontiguous
+				// buffers) -- but as of 2012-02-14, this copying appears to
+				// be the safest tactic.
+				mStreambuf.commit(gotten);
+				LL_DEBUGS("LLProcess") << "read " << gotten << " of " << buffer.size()
+									   << " bytes from " << mDesc << ", new total "
+									   << mStreambuf.size() << LL_ENDL;
+
+				// Now that we've received new data, publish it on our
+				// LLEventPump as advertised. Constrain it by mLimit.
+				std::streamsize datasize((std::min)(mLimit, mStreambuf.size()));
+				boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data();
+				mPump.post(LLSDMap("data", LLSD::String(
+									   boost::asio::buffers_begin(cbufs),
+									   boost::asio::buffers_begin(cbufs) + datasize)));
+			}
+		}
+		return false;
+	}
+
+	std::string mDesc;
+	apr_file_t* mPipe;
+	LLTempBoundListener mConnection;
+	boost::asio::streambuf mStreambuf;
+	std::istream mStream;
+	LLEventStream mPump;
+	size_t mLimit;
+};
+
+/// Need an exception to avoid constructing an invalid LLProcess object, but
+/// internal use only
+struct LLProcessError: public std::runtime_error
+{
+	LLProcessError(const std::string& msg): std::runtime_error(msg) {}
+};
+
 LLProcessPtr LLProcess::create(const LLSDOrParams& params)
 {
 	try
@@ -134,12 +284,23 @@ LLProcessPtr LLProcess::create(const LLSDOrParams& params)
 /// throw LLProcessError mentioning the function call that produced that
 /// result.
 #define chkapr(func)                            \
-    if (ll_apr_warn_status(func))               \
-        throw LLProcessError(#func " failed")
+	if (ll_apr_warn_status(func))				\
+		throw LLProcessError(#func " failed")
 
 LLProcess::LLProcess(const LLSDOrParams& params):
-	mAutokill(params.autokill)
+	mAutokill(params.autokill),
+	mPipes(NSLOTS)
 {
+	// Hmm, when you construct a ptr_vector with a size, it merely reserves
+	// space, it doesn't actually make it that big. Explicitly make it bigger.
+	// Because of ptr_vector's odd semantics, have to push_back(0) the right
+	// number of times! resize() wants to default-construct new BasePipe
+	// instances, which fails because it's pure virtual. But because of the
+	// constructor call, these push_back() calls should require no new
+	// allocation.
+	for (size_t i = 0; i < mPipes.capacity(); ++i)
+		mPipes.push_back(0);
+
 	if (! params.validateBlock(true))
 	{
 		throw LLProcessError(STRINGIZE("not launched: failed parameter validation\n"
@@ -154,16 +315,46 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 	// apr_procattr_io_set() alternatives: inherit the viewer's own stdxxx
 	// handle (APR_NO_PIPE, e.g. for stdout, stderr), or create a pipe that's
 	// blocking on the child end but nonblocking at the viewer end
-	// (APR_CHILD_BLOCK). The viewer can't block for anything: the parent end
-	// MUST be nonblocking. As the APR documentation itself points out, it
-	// makes very little sense to set nonblocking I/O for the child end of a
-	// pipe: only a specially-written child could deal with that.
+	// (APR_CHILD_BLOCK).
 	// Other major options could include explicitly creating a single APR pipe
 	// and passing it as both stdout and stderr (apr_procattr_child_out_set(),
 	// apr_procattr_child_err_set()), or accepting a filename, opening it and
 	// passing that apr_file_t (simple <, >, 2> redirect emulation).
-//	chkapr(apr_procattr_io_set(procattr, APR_CHILD_BLOCK, APR_CHILD_BLOCK, APR_CHILD_BLOCK));
-	chkapr(apr_procattr_io_set(procattr, APR_NO_PIPE, APR_NO_PIPE, APR_NO_PIPE));
+	std::vector<FileParam> fparams(params.files.begin(), params.files.end());
+	// By default, pass APR_NO_PIPE for each slot.
+	std::vector<apr_int32_t> select(LL_ARRAY_SIZE(whichfile), APR_NO_PIPE);
+	for (size_t i = 0; i < (std::min)(LL_ARRAY_SIZE(whichfile), fparams.size()); ++i)
+	{
+		if (std::string(fparams[i].type).empty()) // inherit our file descriptor
+		{
+			select[i] = APR_NO_PIPE;
+		}
+		else if (std::string(fparams[i].type) == "pipe") // anonymous pipe
+		{
+			if (! std::string(fparams[i].name).empty())
+			{
+				LL_WARNS("LLProcess") << "For " << std::string(params.executable)
+									  << ": internal names for reusing pipes ('"
+									  << std::string(fparams[i].name) << "' for " << whichfile[i]
+									  << ") are not yet supported -- creating distinct pipe"
+									  << LL_ENDL;
+			}
+			// The viewer can't block for anything: the parent end MUST be
+			// nonblocking. As the APR documentation itself points out, it
+			// makes very little sense to set nonblocking I/O for the child
+			// end of a pipe: only a specially-written child could deal with
+			// that.
+			select[i] = APR_CHILD_BLOCK;
+		}
+		else
+		{
+			throw LLProcessError(STRINGIZE("For " << std::string(params.executable)
+										   << ": unsupported FileParam for " << whichfile[i]
+										   << ": type='" << std::string(fparams[i].type)
+										   << "', name='" << std::string(fparams[i].name) << "'"));
+		}
+	}
+	chkapr(apr_procattr_io_set(procattr, select[STDIN], select[STDOUT], select[STDERR]));
 
 	// Thumbs down on implicitly invoking the shell to invoke the child. From
 	// our point of view, the other major alternative to APR_PROGRAM_PATH
@@ -251,6 +442,27 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 		// On Windows, associate the new child process with our Job Object.
 		autokill();
 	}
+
+	// Instantiate the proper pipe I/O machinery
+	// want to be able to point to apr_proc_t::in, out, err by index
+	typedef apr_file_t* apr_proc_t::*apr_proc_file_ptr;
+	static apr_proc_file_ptr members[] =
+		{ &apr_proc_t::in, &apr_proc_t::out, &apr_proc_t::err };
+	for (size_t i = 0; i < NSLOTS; ++i)
+	{
+		if (select[i] != APR_CHILD_BLOCK)
+			continue;
+		if (i == STDIN)
+		{
+			mPipes.replace(i, new WritePipeImpl(whichfile[i], mProcess.*(members[i])));
+		}
+		else
+		{
+			mPipes.replace(i, new ReadPipeImpl(whichfile[i], mProcess.*(members[i])));
+		}
+		LL_DEBUGS("LLProcess") << "Instantiating " << typeid(mPipes[i]).name()
+							   << "('" << whichfile[i] << "')" << LL_ENDL;
+	}
 }
 
 LLProcess::~LLProcess()
@@ -428,6 +640,83 @@ LLProcess::handle LLProcess::getProcessHandle() const
 #endif
 }
 
+std::string LLProcess::getPipeName(FILESLOT)
+{
+	// LLProcess::FileParam::type "npipe" is not yet implemented
+	return "";
+}
+
+template<class PIPETYPE>
+PIPETYPE* LLProcess::getPipePtr(std::string& error, FILESLOT slot)
+{
+    if (slot >= NSLOTS)
+    {
+        error = STRINGIZE(mDesc << " has no slot " << slot);
+        return NULL;
+    }
+    if (mPipes.is_null(slot))
+    {
+        error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a monitored pipe");
+        return NULL;
+    }
+    // Make sure we dynamic_cast in pointer domain so we can test, rather than
+    // accepting runtime's exception.
+    PIPETYPE* ppipe = dynamic_cast<PIPETYPE*>(&mPipes[slot]);
+    if (! ppipe)
+    {
+        error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a " << typeid(PIPETYPE).name());
+        return NULL;
+    }
+
+    error.clear();
+    return ppipe;
+}
+
+template <class PIPETYPE>
+PIPETYPE& LLProcess::getPipe(FILESLOT slot)
+{
+    std::string error;
+    PIPETYPE* wp = getPipePtr<PIPETYPE>(error, slot);
+    if (! wp)
+    {
+        throw NoPipe(error);
+    }
+    return *wp;
+}
+
+template <class PIPETYPE>
+boost::optional<PIPETYPE&> LLProcess::getOptPipe(FILESLOT slot)
+{
+    std::string error;
+    PIPETYPE* wp = getPipePtr<PIPETYPE>(error, slot);
+    if (! wp)
+    {
+        LL_DEBUGS("LLProcess") << error << LL_ENDL;
+        return boost::optional<PIPETYPE&>();
+    }
+    return *wp;
+}
+
+LLProcess::WritePipe& LLProcess::getWritePipe(FILESLOT slot)
+{
+    return getPipe<WritePipe>(slot);
+}
+
+boost::optional<LLProcess::WritePipe&> LLProcess::getOptWritePipe(FILESLOT slot)
+{
+    return getOptPipe<WritePipe>(slot);
+}
+
+LLProcess::ReadPipe& LLProcess::getReadPipe(FILESLOT slot)
+{
+    return getPipe<ReadPipe>(slot);
+}
+
+boost::optional<LLProcess::ReadPipe&> LLProcess::getOptReadPipe(FILESLOT slot)
+{
+    return getOptPipe<ReadPipe>(slot);
+}
+
 std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params)
 {
 	std::string cwd(params.cwd);
-- 
cgit v1.2.3


From 9b02f483ffe6f313ec86af3f29fa858fa0cb22e4 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Wed, 15 Feb 2012 14:04:46 -0500
Subject: VS2010 doesn't know how to compute min(4096, size_t)  :-P

---
 indra/llcommon/llprocess.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 55eb7e69d3..d7c297b952 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -144,8 +144,8 @@ public:
 		{
 			// Copy data out from mStreambuf to a flat, contiguous buffer to
 			// write -- but only up to a certain size.
-			std::streamsize total(mStreambuf.size());
-			std::streamsize bufsize((std::min)(4096, total));
+			std::size_t total(mStreambuf.size());
+			std::size_t bufsize((std::min)(std::size_t(4096), total));
 			boost::asio::streambuf::const_buffers_type bufs = mStreambuf.data();
 			std::vector<char> buffer(
 				boost::asio::buffers_begin(bufs),
@@ -241,7 +241,7 @@ private:
 
 				// Now that we've received new data, publish it on our
 				// LLEventPump as advertised. Constrain it by mLimit.
-				std::streamsize datasize((std::min)(mLimit, mStreambuf.size()));
+				std::size_t datasize((std::min)(mLimit, mStreambuf.size()));
 				boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data();
 				mPump.post(LLSDMap("data", LLSD::String(
 									   boost::asio::buffers_begin(cbufs),
-- 
cgit v1.2.3


From 56d931216e67a3e59199669bba022c65a9617bb5 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Wed, 15 Feb 2012 15:47:03 -0500
Subject: Add LLProcess::ReadPipe::size(), peek(), contains(). Also add "len"
 key to event data on LLProcess::getPump(). If you've used setLimit(),
 event["data"].length() may not reflect the length of the accumulated data in
 the ReadPipe. Add unit test with stdin/stdout handshake with child process.

---
 indra/llcommon/llprocess.cpp | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index d7c297b952..1481bf571f 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -197,6 +197,25 @@ public:
 	virtual LLEventPump& getPump() { return mPump; }
 	virtual void setLimit(size_t limit) { mLimit = limit; }
 	virtual size_t getLimit() const { return mLimit; }
+    virtual std::size_t size() { return mStreambuf.size(); }
+
+	virtual std::string peek(std::size_t offset=0,
+							 std::size_t len=(std::numeric_limits<std::size_t>::max)())
+	{
+		// Constrain caller's offset and len to overlap actual buffer content.
+		std::size_t real_offset = (std::min)(mStreambuf.size(), offset);
+		std::size_t real_end	= (std::min)(mStreambuf.size(), real_offset + len);
+		boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data();
+		return std::string(boost::asio::buffers_begin(cbufs) + real_offset,
+						   boost::asio::buffers_begin(cbufs) + real_end);
+	}
+
+	virtual bool contains(const std::string& seek, std::size_t offset=0)
+	{
+		// There may be a more efficient way to search mStreambuf contents,
+		// but this is far the easiest...
+		return peek(offset).find(seek) != std::string::npos;
+	}
 
 private:
 	bool tick(const LLSD&)
@@ -240,12 +259,13 @@ private:
 									   << mStreambuf.size() << LL_ENDL;
 
 				// Now that we've received new data, publish it on our
-				// LLEventPump as advertised. Constrain it by mLimit.
+				// LLEventPump as advertised. Constrain it by mLimit. But show
+				// listener the actual accumulated buffer size, regardless of
+				// mLimit.
 				std::size_t datasize((std::min)(mLimit, mStreambuf.size()));
-				boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data();
-				mPump.post(LLSDMap("data", LLSD::String(
-									   boost::asio::buffers_begin(cbufs),
-									   boost::asio::buffers_begin(cbufs) + datasize)));
+				mPump.post(LLSDMap
+						   ("data", peek(0, datasize))
+						   ("len", LLSD::Integer(mStreambuf.size())));
 			}
 		}
 		return false;
@@ -985,5 +1005,4 @@ void LLProcess::reap(void)
 	}
 }
 |*==========================================================================*/
-
 #endif  // Posix
-- 
cgit v1.2.3


From e92c3113545dd60fb76e115da201163e340c730c Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Thu, 16 Feb 2012 16:05:04 -0500
Subject: Add LLProcess::ReadPipe::find() methods, with corresponding npos. If
 it's useful to have contains() to tell you whether incoming data contains a
 particular substring, and if it's useful for contains() and peek() to accept
 an offset within that data, then it's useful to allow you to get the offset
 of a desired substring within that data. But of course a find() returning
 offset needs something like std::string::npos for "not found"; borrow that
 convention. Support both find(const std::string&) and find(char); the latter
 permits a more efficient implementation. In fact, make find(string) recognize
 a string of length 1 and leverage the find(char) implementation. Given that,
 reimplement contains(mumble) as shorthand for find(mumble) != npos. Implement
 find() overloads using std::search() and std::find() on
 boost::asio::streambuf character iterators, rather than copying to
 std::string and then using string search like previous contains()
 implementation. Reimplement WritePipeImpl::tick() and ReadPipeImpl::tick() to
 write/read directly from/to boost::asio::streambuf data, instead of copying
 to/from a temporary flat buffer. As long as ReadPipeImpl::tick() keeps
 successfully filling buffers, keep reading. Previous implementation would
 only handle a long child write over successive tick() calls. Stop on read
 error or when we come up short.

---
 indra/llcommon/llprocess.cpp | 259 ++++++++++++++++++++++++++++++-------------
 1 file changed, 179 insertions(+), 80 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 1481bf571f..aa22b3f805 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -120,9 +120,12 @@ private:
 static LLProcessListener sProcessListener;
 
 LLProcess::BasePipe::~BasePipe() {}
+const LLProcess::BasePipe::size_type
+	  LLProcess::BasePipe::npos((std::numeric_limits<LLProcess::BasePipe::size_type>::max)());
 
 class WritePipeImpl: public LLProcess::WritePipe
 {
+    LOG_CLASS(WritePipeImpl);
 public:
 	WritePipeImpl(const std::string& desc, apr_file_t* pipe):
 		mDesc(desc),
@@ -139,30 +142,53 @@ public:
 
 	bool tick(const LLSD&)
 	{
+		typedef boost::asio::streambuf::const_buffers_type const_buffer_sequence;
 		// If there's anything to send, try to send it.
-		if (mStreambuf.size())
+		std::size_t total(mStreambuf.size()), consumed(0);
+		if (total)
 		{
-			// Copy data out from mStreambuf to a flat, contiguous buffer to
-			// write -- but only up to a certain size.
-			std::size_t total(mStreambuf.size());
-			std::size_t bufsize((std::min)(std::size_t(4096), total));
-			boost::asio::streambuf::const_buffers_type bufs = mStreambuf.data();
-			std::vector<char> buffer(
-				boost::asio::buffers_begin(bufs),
-				boost::asio::buffers_begin(bufs) + bufsize);
-			apr_size_t written(bufsize);
-			ll_apr_warn_status(apr_file_write(mPipe, &buffer[0], &written));
-			// 'written' is modified to reflect the number of bytes actually
-			// written. Since they've been sent, remove them from the
+			const_buffer_sequence bufs = mStreambuf.data();
+			// In general, our streambuf might contain a number of different
+			// physical buffers; iterate over those.
+			for (const_buffer_sequence::const_iterator bufi(bufs.begin()), bufend(bufs.end());
+				 bufi != bufend; ++bufi)
+			{
+				// http://www.boost.org/doc/libs/1_49_0_beta1/doc/html/boost_asio/reference/buffer.html#boost_asio.reference.buffer.accessing_buffer_contents
+				std::size_t towrite(boost::asio::buffer_size(*bufi));
+				apr_size_t written(towrite);
+				apr_status_t err = apr_file_write(mPipe,
+												  boost::asio::buffer_cast<const void*>(*bufi),
+												  &written);
+				// EAGAIN is exactly what we want from a nonblocking pipe.
+				// Rather than waiting for data, it should return immediately.
+				if (! (err == APR_SUCCESS || APR_STATUS_IS_EAGAIN(err)))
+				{
+					LL_WARNS("LLProcess") << "apr_file_write(" << towrite << ") on " << mDesc
+										  << " got " << err << ":" << LL_ENDL;
+					ll_apr_warn_status(err);
+				}
+
+				// 'written' is modified to reflect the number of bytes actually
+				// written. Make sure we consume those later. (Don't consume them
+				// now, that would invalidate the buffer iterator sequence!)
+				consumed += written;
+				LL_DEBUGS("LLProcess") << "wrote " << written << " of " << towrite
+									   << " bytes to " << mDesc
+									   << " (original " << total << ")" << LL_ENDL;
+
+				// The parent end of this pipe is nonblocking. If we weren't able
+				// to write everything we wanted, don't keep banging on it -- that
+				// won't change until the child reads some. Wait for next tick().
+				if (written < towrite)
+					break;
+			}
+			// In all, we managed to write 'consumed' bytes. Remove them from the
 			// streambuf so we don't keep trying to send them. This could be
-			// anywhere from 0 up to mStreambuf.size(); anything we haven't
-			// yet sent, we'll try again next tick() call.
-			mStreambuf.consume(written);
-			LL_DEBUGS("LLProcess") << "wrote " << written << " of " << bufsize
-								   << " bytes to " << mDesc
-								   << " (original " << total << "), "
-								   << mStreambuf.size() << " remaining" << LL_ENDL;
+			// anywhere from 0 up to mStreambuf.size(); anything we haven't yet
+			// sent, we'll try again later.
+			mStreambuf.consume(consumed);
 		}
+
 		return false;
 	}
 
@@ -176,6 +202,7 @@ private:
 
 class ReadPipeImpl: public LLProcess::ReadPipe
 {
+    LOG_CLASS(ReadPipeImpl);
 public:
 	ReadPipeImpl(const std::string& desc, apr_file_t* pipe):
 		mDesc(desc),
@@ -184,7 +211,7 @@ public:
 		mStream(&mStreambuf),
 		mPump("ReadPipe"),
 		// use funky syntax to call max() to avoid blighted max() macros
-		mLimit((std::numeric_limits<size_t>::max)())
+		mLimit(npos)
 	{
 		mConnection = LLEventPumps::instance().obtain("mainloop")
 			.listen(LLEventPump::inventName("ReadPipe"),
@@ -195,79 +222,149 @@ public:
 	// methods with implementation data concealed from the base class.
 	virtual std::istream& get_istream() { return mStream; }
 	virtual LLEventPump& getPump() { return mPump; }
-	virtual void setLimit(size_t limit) { mLimit = limit; }
-	virtual size_t getLimit() const { return mLimit; }
-    virtual std::size_t size() { return mStreambuf.size(); }
+	virtual void setLimit(size_type limit) { mLimit = limit; }
+	virtual size_type getLimit() const { return mLimit; }
+	virtual size_type size() const { return mStreambuf.size(); }
 
-	virtual std::string peek(std::size_t offset=0,
-							 std::size_t len=(std::numeric_limits<std::size_t>::max)())
+	virtual std::string peek(size_type offset=0, size_type len=npos) const
 	{
 		// Constrain caller's offset and len to overlap actual buffer content.
-		std::size_t real_offset = (std::min)(mStreambuf.size(), offset);
-		std::size_t real_end	= (std::min)(mStreambuf.size(), real_offset + len);
+		std::size_t real_offset = (std::min)(mStreambuf.size(), std::size_t(offset));
+		std::size_t real_end	= (std::min)(mStreambuf.size(), std::size_t(real_offset + len));
 		boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data();
 		return std::string(boost::asio::buffers_begin(cbufs) + real_offset,
 						   boost::asio::buffers_begin(cbufs) + real_end);
 	}
 
-	virtual bool contains(const std::string& seek, std::size_t offset=0)
+	virtual size_type find(const std::string& seek, size_type offset=0) const
 	{
-		// There may be a more efficient way to search mStreambuf contents,
-		// but this is far the easiest...
-		return peek(offset).find(seek) != std::string::npos;
+		// If we're passing a string of length 1, use find(char), which can
+		// use an O(n) std::find() rather than the O(n^2) std::search().
+		if (seek.length() == 1)
+		{
+			return find(seek[0], offset);
+		}
+
+		// If offset is beyond the whole buffer, can't even construct a valid
+		// iterator range; can't possibly find the string we seek.
+		if (offset > mStreambuf.size())
+		{
+			return npos;
+		}
+
+		boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data();
+		boost::asio::buffers_iterator<boost::asio::streambuf::const_buffers_type>
+			begin(boost::asio::buffers_begin(cbufs)),
+			end	 (boost::asio::buffers_end(cbufs)),
+			found(std::search(begin + offset, end, seek.begin(), seek.end()));
+		return (found == end)? npos : (found - begin);
 	}
 
-private:
-	bool tick(const LLSD&)
+	virtual size_type find(char seek, size_type offset=0) const
 	{
-		// Allocate a buffer and try, every time, to read into it.
-		std::vector<char> buffer(4096);
-		apr_size_t gotten(buffer.size());
-		apr_status_t err = apr_file_read(mPipe, &buffer[0], &gotten);
-		if (err == APR_EOF)
+		// If offset is beyond the whole buffer, can't even construct a valid
+		// iterator range; can't possibly find the char we seek.
+		if (offset > mStreambuf.size())
 		{
-			// Handle EOF specially: it's part of normal-case processing.
-			LL_DEBUGS("LLProcess") << "EOF on " << mDesc << LL_ENDL;
-			// We won't need any more tick() calls.
-			mConnection.disconnect();
+			return npos;
 		}
-		else if (! ll_apr_warn_status(err)) // validate anything but EOF
+
+		boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data();
+		boost::asio::buffers_iterator<boost::asio::streambuf::const_buffers_type>
+			begin(boost::asio::buffers_begin(cbufs)),
+			end	 (boost::asio::buffers_end(cbufs)),
+			found(std::find(begin + offset, end, seek));
+		return (found == end)? npos : (found - begin);
+	}
+
+private:
+	bool tick(const LLSD&)
+	{
+		typedef boost::asio::streambuf::mutable_buffers_type mutable_buffer_sequence;
+		// Try, every time, to read into our streambuf. In fact, we have no
+		// idea how much data the child might be trying to send: keep trying
+		// until we're convinced we've temporarily exhausted the pipe.
+		bool exhausted = false;
+		std::size_t committed(0);
+		do
 		{
-			// 'gotten' was modified to reflect the number of bytes actually
-			// received. If nonzero, add them to the streambuf and notify
-			// interested parties.
-			if (gotten)
+			// attempt to read an arbitrary size
+			mutable_buffer_sequence bufs = mStreambuf.prepare(4096);
+			// In general, the mutable_buffer_sequence returned by prepare() might
+			// contain a number of different physical buffers; iterate over those.
+			std::size_t tocommit(0);
+			for (mutable_buffer_sequence::const_iterator bufi(bufs.begin()), bufend(bufs.end());
+				 bufi != bufend; ++bufi)
 			{
-				boost::asio::streambuf::mutable_buffers_type mbufs = mStreambuf.prepare(gotten);
-				std::copy(buffer.begin(), buffer.begin() + gotten,
-						  boost::asio::buffers_begin(mbufs));
-				// Don't forget to "commit" the data! The sequence (prepare(),
-				// commit()) is obviously intended to allow us to allocate
-				// buffer space, then read directly into some portion of it,
-				// then commit only as much as we managed to obtain. But the
-				// only official (documented) way I can find to populate a
-				// mutable_buffers_type is to use buffers_begin(). It Would Be
-				// Nice if we were permitted to directly read into
-				// mutable_buffers_type (not to mention writing directly from
-				// const_buffers_type in WritePipeImpl; APR even supports an
-				// apr_file_writev() function for writing from discontiguous
-				// buffers) -- but as of 2012-02-14, this copying appears to
-				// be the safest tactic.
-				mStreambuf.commit(gotten);
-				LL_DEBUGS("LLProcess") << "read " << gotten << " of " << buffer.size()
-									   << " bytes from " << mDesc << ", new total "
-									   << mStreambuf.size() << LL_ENDL;
-
-				// Now that we've received new data, publish it on our
-				// LLEventPump as advertised. Constrain it by mLimit. But show
-				// listener the actual accumulated buffer size, regardless of
-				// mLimit.
-				std::size_t datasize((std::min)(mLimit, mStreambuf.size()));
-				mPump.post(LLSDMap
-						   ("data", peek(0, datasize))
-						   ("len", LLSD::Integer(mStreambuf.size())));
+				// http://www.boost.org/doc/libs/1_49_0_beta1/doc/html/boost_asio/reference/buffer.html#boost_asio.reference.buffer.accessing_buffer_contents
+				std::size_t toread(boost::asio::buffer_size(*bufi));
+				apr_size_t gotten(toread);
+				apr_status_t err = apr_file_read(mPipe,
+												 boost::asio::buffer_cast<void*>(*bufi),
+												 &gotten);
+				// EAGAIN is exactly what we want from a nonblocking pipe.
+				// Rather than waiting for data, it should return immediately.
+				if (! (err == APR_SUCCESS || APR_STATUS_IS_EAGAIN(err)))
+				{
+					// Handle EOF specially: it's part of normal-case processing.
+					if (err == APR_EOF)
+					{
+						LL_DEBUGS("LLProcess") << "EOF on " << mDesc << LL_ENDL;
+					}
+					else
+					{
+						LL_WARNS("LLProcess") << "apr_file_read(" << toread << ") on " << mDesc
+											  << " got " << err << ":" << LL_ENDL;
+						ll_apr_warn_status(err);
+					}
+					// Either way, though, we won't need any more tick() calls.
+					mConnection.disconnect();
+					exhausted = true; // also break outer retry loop
+					break;
+				}
+
+				// 'gotten' was modified to reflect the number of bytes actually
+				// received. Make sure we commit those later. (Don't commit them
+				// now, that would invalidate the buffer iterator sequence!)
+				tocommit += gotten;
+				LL_DEBUGS("LLProcess") << "read " << gotten << " of " << toread
+									   << " bytes from " << mDesc << LL_ENDL;
+
+				// The parent end of this pipe is nonblocking. If we weren't even
+				// able to fill this buffer, don't loop to try to fill the next --
+				// that won't change until the child writes more. Wait for next
+				// tick().
+				if (gotten < toread)
+				{
+					// break outer retry loop too
+					exhausted = true;
+					break;
+				}
 			}
+
+			// Don't forget to "commit" the data!
+			mStreambuf.commit(tocommit);
+			committed += tocommit;
+
+			// 'exhausted' is set when we can't fill any one buffer of the
+			// mutable_buffer_sequence established by the current prepare()
+			// call -- whether due to error or not enough bytes. That is,
+			// 'exhausted' is still false when we've filled every physical
+			// buffer in the mutable_buffer_sequence. In that case, for all we
+			// know, the child might have still more data pending -- go for it!
+		} while (! exhausted);
+
+		if (committed)
+		{
+			// If we actually received new data, publish it on our LLEventPump
+			// as advertised. Constrain it by mLimit. But show listener the
+			// actual accumulated buffer size, regardless of mLimit.
+			size_type datasize((std::min)(mLimit, size_type(mStreambuf.size())));
+			mPump.post(LLSDMap
+					   ("data", peek(0, datasize))
+					   ("len", LLSD::Integer(mStreambuf.size())));
 		}
+
 		return false;
 	}
 
@@ -277,7 +374,7 @@ private:
 	boost::asio::streambuf mStreambuf;
 	std::istream mStream;
 	LLEventStream mPump;
-	size_t mLimit;
+	size_type mLimit;
 };
 
 /// Need an exception to avoid constructing an invalid LLProcess object, but
@@ -472,16 +569,18 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 	{
 		if (select[i] != APR_CHILD_BLOCK)
 			continue;
+		std::string desc(STRINGIZE(mDesc << ' ' << whichfile[i]));
+		apr_file_t* pipe(mProcess.*(members[i]));
 		if (i == STDIN)
 		{
-			mPipes.replace(i, new WritePipeImpl(whichfile[i], mProcess.*(members[i])));
+			mPipes.replace(i, new WritePipeImpl(desc, pipe));
 		}
 		else
 		{
-			mPipes.replace(i, new ReadPipeImpl(whichfile[i], mProcess.*(members[i])));
+			mPipes.replace(i, new ReadPipeImpl(desc, pipe));
 		}
 		LL_DEBUGS("LLProcess") << "Instantiating " << typeid(mPipes[i]).name()
-							   << "('" << whichfile[i] << "')" << LL_ENDL;
+							   << "('" << desc << "')" << LL_ENDL;
 	}
 }
 
-- 
cgit v1.2.3


From a06ba836c76ea8b35aeca9d09bd7d3b043a4c962 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Thu, 16 Feb 2012 17:35:34 -0500
Subject: Fix bug in LLProcess::ReadPipe::peek() substring computation. Add
 unit tests for peek() with substring args, reimplemented contains(), various
 forms of find(). (yay unit tests)

---
 indra/llcommon/llprocess.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index aa22b3f805..add1649ba5 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -230,7 +230,8 @@ public:
 	{
 		// Constrain caller's offset and len to overlap actual buffer content.
 		std::size_t real_offset = (std::min)(mStreambuf.size(), std::size_t(offset));
-		std::size_t real_end	= (std::min)(mStreambuf.size(), std::size_t(real_offset + len));
+		size_type	want_end	= (len == npos)? npos : (real_offset + len);
+		std::size_t real_end	= (std::min)(mStreambuf.size(), std::size_t(want_end));
 		boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data();
 		return std::string(boost::asio::buffers_begin(cbufs) + real_offset,
 						   boost::asio::buffers_begin(cbufs) + real_end);
-- 
cgit v1.2.3


From e98438bda70f92c1caa0621b7e467b72c7e484ad Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Sat, 18 Feb 2012 12:00:13 -0500
Subject: Fix subtle bug in ReadPipeImpl: wouldn't tolerate multiple instances.
 That is, trying to instantiate a ReadPipeImpl while another already existed
 would throw an LLEventPump::DupPumpName exception. Fortunately this behavior
 is easily bypassed.

---
 indra/llcommon/llprocess.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index add1649ba5..d6a5a18565 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -209,7 +209,7 @@ public:
 		mPipe(pipe),
 		// Essential to initialize our std::istream with our special streambuf!
 		mStream(&mStreambuf),
-		mPump("ReadPipe"),
+		mPump("ReadPipe", true),    // tweak name as needed to avoid collisions
 		// use funky syntax to call max() to avoid blighted max() macros
 		mLimit(npos)
 	{
-- 
cgit v1.2.3


From 8b5d5f9652499103b966524e1c0ceef869e29eeb Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Mon, 20 Feb 2012 12:40:38 -0500
Subject: Make LLProcess post termination event to specified pump if desired.
 This way a caller need not spin on isRunning(); we can just listen for the
 requested termination event. Post a similar event containing error message if
 for any reason LLProcess::create() failed to launch the child. Add unit tests
 for both cases.

---
 indra/llcommon/llprocess.cpp | 118 +++++++++++++++++++++++++++----------------
 1 file changed, 75 insertions(+), 43 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index d6a5a18565..9799ed1938 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -125,7 +125,7 @@ const LLProcess::BasePipe::size_type
 
 class WritePipeImpl: public LLProcess::WritePipe
 {
-    LOG_CLASS(WritePipeImpl);
+	LOG_CLASS(WritePipeImpl);
 public:
 	WritePipeImpl(const std::string& desc, apr_file_t* pipe):
 		mDesc(desc),
@@ -202,7 +202,7 @@ private:
 
 class ReadPipeImpl: public LLProcess::ReadPipe
 {
-    LOG_CLASS(ReadPipeImpl);
+	LOG_CLASS(ReadPipeImpl);
 public:
 	ReadPipeImpl(const std::string& desc, apr_file_t* pipe):
 		mDesc(desc),
@@ -394,6 +394,23 @@ LLProcessPtr LLProcess::create(const LLSDOrParams& params)
 	catch (const LLProcessError& e)
 	{
 		LL_WARNS("LLProcess") << e.what() << LL_ENDL;
+
+		// If caller is requesting an event on process termination, send one
+		// indicating bad launch. This may prevent someone waiting forever for
+		// a termination post that can't arrive because the child never
+		// started.
+		if (! std::string(params.postend).empty())
+		{
+			LLEventPumps::instance().obtain(params.postend)
+				.post(LLSDMap
+					  // no "id"
+					  ("desc", std::string(params.executable))
+					  ("state", LLProcess::UNSTARTED)
+					  // no "data"
+					  ("string", e.what())
+					 );
+		}
+
 		return LLProcessPtr();
 	}
 }
@@ -425,6 +442,8 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 									   << LLSDNotationStreamer(params)));
 	}
 
+	mPostend = params.postend;
+
 	apr_procattr_t *procattr = NULL;
 	chkapr(apr_procattr_create(&procattr, gAPRPoolp));
 
@@ -744,6 +763,19 @@ void LLProcess::handle_status(int reason, int status)
 	// hand.
 	mStatus = interpret_status(status);
 	LL_INFOS("LLProcess") << getStatusString() << LL_ENDL;
+
+	// If caller requested notification on child termination, send it.
+	if (! mPostend.empty())
+	{
+		LLEventPumps::instance().obtain(mPostend)
+			.post(LLSDMap
+				  ("id",     getProcessID())
+				  ("desc",   mDesc)
+				  ("state",  mStatus.mState)
+				  ("data",   mStatus.mData)
+				  ("string", getStatusString())
+				 );
+	}
 }
 
 LLProcess::id LLProcess::getProcessID() const
@@ -769,72 +801,72 @@ std::string LLProcess::getPipeName(FILESLOT)
 template<class PIPETYPE>
 PIPETYPE* LLProcess::getPipePtr(std::string& error, FILESLOT slot)
 {
-    if (slot >= NSLOTS)
-    {
-        error = STRINGIZE(mDesc << " has no slot " << slot);
-        return NULL;
-    }
-    if (mPipes.is_null(slot))
-    {
-        error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a monitored pipe");
-        return NULL;
-    }
-    // Make sure we dynamic_cast in pointer domain so we can test, rather than
-    // accepting runtime's exception.
-    PIPETYPE* ppipe = dynamic_cast<PIPETYPE*>(&mPipes[slot]);
-    if (! ppipe)
-    {
-        error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a " << typeid(PIPETYPE).name());
-        return NULL;
-    }
-
-    error.clear();
-    return ppipe;
+	if (slot >= NSLOTS)
+	{
+		error = STRINGIZE(mDesc << " has no slot " << slot);
+		return NULL;
+	}
+	if (mPipes.is_null(slot))
+	{
+		error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a monitored pipe");
+		return NULL;
+	}
+	// Make sure we dynamic_cast in pointer domain so we can test, rather than
+	// accepting runtime's exception.
+	PIPETYPE* ppipe = dynamic_cast<PIPETYPE*>(&mPipes[slot]);
+	if (! ppipe)
+	{
+		error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a " << typeid(PIPETYPE).name());
+		return NULL;
+	}
+
+	error.clear();
+	return ppipe;
 }
 
 template <class PIPETYPE>
 PIPETYPE& LLProcess::getPipe(FILESLOT slot)
 {
-    std::string error;
-    PIPETYPE* wp = getPipePtr<PIPETYPE>(error, slot);
-    if (! wp)
-    {
-        throw NoPipe(error);
-    }
-    return *wp;
+	std::string error;
+	PIPETYPE* wp = getPipePtr<PIPETYPE>(error, slot);
+	if (! wp)
+	{
+		throw NoPipe(error);
+	}
+	return *wp;
 }
 
 template <class PIPETYPE>
 boost::optional<PIPETYPE&> LLProcess::getOptPipe(FILESLOT slot)
 {
-    std::string error;
-    PIPETYPE* wp = getPipePtr<PIPETYPE>(error, slot);
-    if (! wp)
-    {
-        LL_DEBUGS("LLProcess") << error << LL_ENDL;
-        return boost::optional<PIPETYPE&>();
-    }
-    return *wp;
+	std::string error;
+	PIPETYPE* wp = getPipePtr<PIPETYPE>(error, slot);
+	if (! wp)
+	{
+		LL_DEBUGS("LLProcess") << error << LL_ENDL;
+		return boost::optional<PIPETYPE&>();
+	}
+	return *wp;
 }
 
 LLProcess::WritePipe& LLProcess::getWritePipe(FILESLOT slot)
 {
-    return getPipe<WritePipe>(slot);
+	return getPipe<WritePipe>(slot);
 }
 
 boost::optional<LLProcess::WritePipe&> LLProcess::getOptWritePipe(FILESLOT slot)
 {
-    return getOptPipe<WritePipe>(slot);
+	return getOptPipe<WritePipe>(slot);
 }
 
 LLProcess::ReadPipe& LLProcess::getReadPipe(FILESLOT slot)
 {
-    return getPipe<ReadPipe>(slot);
+	return getPipe<ReadPipe>(slot);
 }
 
 boost::optional<LLProcess::ReadPipe&> LLProcess::getOptReadPipe(FILESLOT slot)
 {
-    return getOptPipe<ReadPipe>(slot);
+	return getOptPipe<ReadPipe>(slot);
 }
 
 std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params)
@@ -932,7 +964,7 @@ static std::string WindowsErrorString(const std::string& operation)
 					   NULL)
 		!= 0) 
 	{
-        // convert from wide-char string to multi-byte string
+		// convert from wide-char string to multi-byte string
 		char message[256];
 		wcstombs(message, error_str, sizeof(message));
 		message[sizeof(message)-1] = 0;
-- 
cgit v1.2.3


From 999484a60896b11df1af9a44e58ccae6fa6ecbed Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Mon, 20 Feb 2012 14:22:32 -0500
Subject: Let LLProcess consumer specify desired description for logging. If
 caller runs (e.g.) a Python script, it's not very helpful to a human log
 reader to keep seeing LLProcess instances logged as /pathname/to/python
 (pid). If caller is aware, the code can at least use the script name as the
 desc -- or maybe even a hint as to the script's purpose. If caller doesn't
 explicitly pass a desc, at least shorten to just the basename of the
 executable.

---
 indra/llcommon/llprocess.cpp | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 9799ed1938..b4c6a647d7 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -50,6 +50,7 @@
 static const char* whichfile[] = { "stdin", "stdout", "stderr" };
 static std::string empty;
 static LLProcess::Status interpret_status(int status);
+static std::string getDesc(const LLProcess::Params& params);
 
 /**
  * Ref-counted "mainloop" listener. As long as there are still outstanding
@@ -404,7 +405,7 @@ LLProcessPtr LLProcess::create(const LLSDOrParams& params)
 			LLEventPumps::instance().obtain(params.postend)
 				.post(LLSDMap
 					  // no "id"
-					  ("desc", std::string(params.executable))
+					  ("desc", getDesc(params))
 					  ("state", LLProcess::UNSTARTED)
 					  // no "data"
 					  ("string", e.what())
@@ -561,8 +562,8 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 	sProcessListener.addPoll(*this);
 	mStatus.mState = RUNNING;
 
-	mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << mProcess.pid << ')');
-	LL_INFOS("LLProcess") << "Launched " << params << " (" << mProcess.pid << ")" << LL_ENDL;
+	mDesc = STRINGIZE(getDesc(params) << " (" << mProcess.pid << ')');
+	LL_INFOS("LLProcess") << mDesc << ": launched " << params << LL_ENDL;
 
 	// Unless caller explicitly turned off autokill (child should persist),
 	// take steps to terminate the child. This is all suspenders-and-belt: in
@@ -604,6 +605,29 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 	}
 }
 
+// Helper to obtain a description string, given a Params block
+static std::string getDesc(const LLProcess::Params& params)
+{
+	// If caller specified a description string, by all means use it.
+	std::string desc(params.desc);
+	if (! desc.empty())
+		return desc;
+
+	// Caller didn't say. Use the executable name -- but use just the filename
+	// part. On Mac, for instance, full pathnames get cumbersome.
+	// If there are Linden utility functions to manipulate pathnames, I
+	// haven't found them -- and for this usage, Boost.Filesystem seems kind
+	// of heavyweight.
+	std::string executable(params.executable);
+	std::string::size_type delim = executable.find_last_of("\\/");
+	// If executable contains no pathname delimiters, return the whole thing.
+	if (delim == std::string::npos)
+		return executable;
+
+	// Return just the part beyond the last delimiter.
+	return executable.substr(delim + 1);
+}
+
 LLProcess::~LLProcess()
 {
 	// Only in state RUNNING are we registered for callback. In UNSTARTED we
-- 
cgit v1.2.3


From 14ddc6474a0ae83db8d034b00138289fb15e41b7 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Thu, 23 Feb 2012 13:41:26 -0500
Subject: Tighten up LLProcess pipe support, per Richard's code review. Clarify
 wording in some of the doc comments; be a bit more explicit about some of the
 parameter fields. Make some query methods 'const'. Change default
 LLProcess::ReadPipe::getLimit() value to 0: don't post any incoming data with
 notification event unless caller requests it. But do post pertinent FILESLOT
 in case caller reuses same listener for both stdout and stderr. Use more
 idiomatic, readable syntax for accessing LLProcess::Params data.

---
 indra/llcommon/llprocess.cpp | 119 ++++++++++++++++++++++++++-----------------
 1 file changed, 72 insertions(+), 47 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index b4c6a647d7..3b17b819bd 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -47,11 +47,18 @@
 #include <typeinfo>
 #include <utility>
 
-static const char* whichfile[] = { "stdin", "stdout", "stderr" };
+static const char* whichfile_[] = { "stdin", "stdout", "stderr" };
 static std::string empty;
 static LLProcess::Status interpret_status(int status);
 static std::string getDesc(const LLProcess::Params& params);
 
+static std::string whichfile(LLProcess::FILESLOT index)
+{
+	if (index < LL_ARRAY_SIZE(whichfile_))
+		return whichfile_[index];
+	return STRINGIZE("file slot " << index);
+}
+
 /**
  * Ref-counted "mainloop" listener. As long as there are still outstanding
  * LLProcess objects, keep listening on "mainloop" so we can keep polling APR
@@ -122,6 +129,7 @@ static LLProcessListener sProcessListener;
 
 LLProcess::BasePipe::~BasePipe() {}
 const LLProcess::BasePipe::size_type
+	  // use funky syntax to call max() to avoid blighted max() macros
 	  LLProcess::BasePipe::npos((std::numeric_limits<LLProcess::BasePipe::size_type>::max)());
 
 class WritePipeImpl: public LLProcess::WritePipe
@@ -205,14 +213,14 @@ class ReadPipeImpl: public LLProcess::ReadPipe
 {
 	LOG_CLASS(ReadPipeImpl);
 public:
-	ReadPipeImpl(const std::string& desc, apr_file_t* pipe):
+	ReadPipeImpl(const std::string& desc, apr_file_t* pipe, LLProcess::FILESLOT index):
 		mDesc(desc),
 		mPipe(pipe),
+		mIndex(index),
 		// Essential to initialize our std::istream with our special streambuf!
 		mStream(&mStreambuf),
 		mPump("ReadPipe", true),    // tweak name as needed to avoid collisions
-		// use funky syntax to call max() to avoid blighted max() macros
-		mLimit(npos)
+		mLimit(0)
 	{
 		mConnection = LLEventPumps::instance().obtain("mainloop")
 			.listen(LLEventPump::inventName("ReadPipe"),
@@ -364,7 +372,10 @@ private:
 			size_type datasize((std::min)(mLimit, size_type(mStreambuf.size())));
 			mPump.post(LLSDMap
 					   ("data", peek(0, datasize))
-					   ("len", LLSD::Integer(mStreambuf.size())));
+					   ("len", LLSD::Integer(mStreambuf.size()))
+					   ("index", LLSD::Integer(mIndex))
+					   ("name", whichfile(mIndex))
+					   ("desc", mDesc));
 		}
 
 		return false;
@@ -372,6 +383,7 @@ private:
 
 	std::string mDesc;
 	apr_file_t* mPipe;
+	LLProcess::FILESLOT mIndex;
 	LLTempBoundListener mConnection;
 	boost::asio::streambuf mStreambuf;
 	std::istream mStream;
@@ -400,7 +412,7 @@ LLProcessPtr LLProcess::create(const LLSDOrParams& params)
 		// indicating bad launch. This may prevent someone waiting forever for
 		// a termination post that can't arrive because the child never
 		// started.
-		if (! std::string(params.postend).empty())
+		if (params.postend.isProvided())
 		{
 			LLEventPumps::instance().obtain(params.postend)
 				.post(LLSDMap
@@ -458,22 +470,24 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 	// and passing it as both stdout and stderr (apr_procattr_child_out_set(),
 	// apr_procattr_child_err_set()), or accepting a filename, opening it and
 	// passing that apr_file_t (simple <, >, 2> redirect emulation).
-	std::vector<FileParam> fparams(params.files.begin(), params.files.end());
-	// By default, pass APR_NO_PIPE for each slot.
-	std::vector<apr_int32_t> select(LL_ARRAY_SIZE(whichfile), APR_NO_PIPE);
-	for (size_t i = 0; i < (std::min)(LL_ARRAY_SIZE(whichfile), fparams.size()); ++i)
+	std::vector<apr_int32_t> select;
+	BOOST_FOREACH(const FileParam& fparam, params.files)
 	{
-		if (std::string(fparams[i].type).empty()) // inherit our file descriptor
+		// Every iteration, we're going to append an item to 'select'. At the
+		// top of the loop, its size() is, in effect, an index. Use that to
+		// pick a string description for messages.
+		std::string which(whichfile(FILESLOT(select.size())));
+		if (fparam.type().empty())  // inherit our file descriptor
 		{
-			select[i] = APR_NO_PIPE;
+			select.push_back(APR_NO_PIPE);
 		}
-		else if (std::string(fparams[i].type) == "pipe") // anonymous pipe
+		else if (fparam.type() == "pipe") // anonymous pipe
 		{
-			if (! std::string(fparams[i].name).empty())
+			if (! fparam.name().empty())
 			{
-				LL_WARNS("LLProcess") << "For " << std::string(params.executable)
+				LL_WARNS("LLProcess") << "For " << params.executable()
 									  << ": internal names for reusing pipes ('"
-									  << std::string(fparams[i].name) << "' for " << whichfile[i]
+									  << fparam.name() << "' for " << which
 									  << ") are not yet supported -- creating distinct pipe"
 									  << LL_ENDL;
 			}
@@ -482,16 +496,21 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 			// makes very little sense to set nonblocking I/O for the child
 			// end of a pipe: only a specially-written child could deal with
 			// that.
-			select[i] = APR_CHILD_BLOCK;
+			select.push_back(APR_CHILD_BLOCK);
 		}
 		else
 		{
-			throw LLProcessError(STRINGIZE("For " << std::string(params.executable)
-										   << ": unsupported FileParam for " << whichfile[i]
-										   << ": type='" << std::string(fparams[i].type)
-										   << "', name='" << std::string(fparams[i].name) << "'"));
+			throw LLProcessError(STRINGIZE("For " << params.executable()
+										   << ": unsupported FileParam for " << which
+										   << ": type='" << fparam.type()
+										   << "', name='" << fparam.name() << "'"));
 		}
 	}
+	// By default, pass APR_NO_PIPE for unspecified slots.
+	while (select.size() < NSLOTS)
+	{
+		select.push_back(APR_NO_PIPE);
+	}
 	chkapr(apr_procattr_io_set(procattr, select[STDIN], select[STDOUT], select[STDERR]));
 
 	// Thumbs down on implicitly invoking the shell to invoke the child. From
@@ -527,24 +546,32 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 #endif
 	}
 
-	// Have to instantiate named std::strings for string params items so their
-	// c_str() values persist.
-	std::string cwd(params.cwd);
-	if (! cwd.empty())
+	// In preparation for calling apr_proc_create(), we collect a number of
+	// const char* pointers obtained from std::string::c_str(). Turns out
+	// LLInitParam::Block's helpers Optional, Mandatory, Multiple et al.
+	// guarantee that converting to the wrapped type (std::string in our
+	// case), e.g. by calling operator(), returns a reference to *the same
+	// instance* of the wrapped type that's stored in our Block subclass.
+	// That's important! We know 'params' persists throughout this method
+	// call; but without that guarantee, we'd have to assume that converting
+	// one of its members to std::string might return a different (temp)
+	// instance. Capturing the c_str() from a temporary std::string is Bad Bad
+	// Bad. But armed with this knowledge, when you see params.cwd().c_str(),
+	// grit your teeth and smile and carry on.
+
+	if (params.cwd.isProvided())
 	{
-		chkapr(apr_procattr_dir_set(procattr, cwd.c_str()));
+		chkapr(apr_procattr_dir_set(procattr, params.cwd().c_str()));
 	}
 
 	// create an argv vector for the child process
 	std::vector<const char*> argv;
 
-	// add the executable path
-	std::string executable(params.executable);
-	argv.push_back(executable.c_str());
+	// Add the executable path. See above remarks about c_str().
+	argv.push_back(params.executable().c_str());
 
-	// and any arguments
-	std::vector<std::string> args(params.args.begin(), params.args.end());
-	BOOST_FOREACH(const std::string& arg, args)
+	// Add arguments. See above remarks about c_str().
+	BOOST_FOREACH(const std::string& arg, params.args)
 	{
 		argv.push_back(arg.c_str());
 	}
@@ -590,7 +617,7 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 	{
 		if (select[i] != APR_CHILD_BLOCK)
 			continue;
-		std::string desc(STRINGIZE(mDesc << ' ' << whichfile[i]));
+		std::string desc(STRINGIZE(mDesc << ' ' << whichfile(FILESLOT(i))));
 		apr_file_t* pipe(mProcess.*(members[i]));
 		if (i == STDIN)
 		{
@@ -598,7 +625,7 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 		}
 		else
 		{
-			mPipes.replace(i, new ReadPipeImpl(desc, pipe));
+			mPipes.replace(i, new ReadPipeImpl(desc, pipe, FILESLOT(i)));
 		}
 		LL_DEBUGS("LLProcess") << "Instantiating " << typeid(mPipes[i]).name()
 							   << "('" << desc << "')" << LL_ENDL;
@@ -609,9 +636,8 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 static std::string getDesc(const LLProcess::Params& params)
 {
 	// If caller specified a description string, by all means use it.
-	std::string desc(params.desc);
-	if (! desc.empty())
-		return desc;
+	if (params.desc.isProvided())
+		return params.desc;
 
 	// Caller didn't say. Use the executable name -- but use just the filename
 	// part. On Mac, for instance, full pathnames get cumbersome.
@@ -670,22 +696,22 @@ bool LLProcess::kill(const std::string& who)
 	return ! isRunning();
 }
 
-bool LLProcess::isRunning(void)
+bool LLProcess::isRunning() const
 {
 	return getStatus().mState == RUNNING;
 }
 
-LLProcess::Status LLProcess::getStatus()
+LLProcess::Status LLProcess::getStatus() const
 {
 	return mStatus;
 }
 
-std::string LLProcess::getStatusString()
+std::string LLProcess::getStatusString() const
 {
 	return getStatusString(getStatus());
 }
 
-std::string LLProcess::getStatusString(const Status& status)
+std::string LLProcess::getStatusString(const Status& status) const
 {
 	return getStatusString(mDesc, status);
 }
@@ -816,7 +842,7 @@ LLProcess::handle LLProcess::getProcessHandle() const
 #endif
 }
 
-std::string LLProcess::getPipeName(FILESLOT)
+std::string LLProcess::getPipeName(FILESLOT) const
 {
 	// LLProcess::FileParam::type "npipe" is not yet implemented
 	return "";
@@ -832,7 +858,7 @@ PIPETYPE* LLProcess::getPipePtr(std::string& error, FILESLOT slot)
 	}
 	if (mPipes.is_null(slot))
 	{
-		error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a monitored pipe");
+		error = STRINGIZE(mDesc << ' ' << whichfile(slot) << " not a monitored pipe");
 		return NULL;
 	}
 	// Make sure we dynamic_cast in pointer domain so we can test, rather than
@@ -840,7 +866,7 @@ PIPETYPE* LLProcess::getPipePtr(std::string& error, FILESLOT slot)
 	PIPETYPE* ppipe = dynamic_cast<PIPETYPE*>(&mPipes[slot]);
 	if (! ppipe)
 	{
-		error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a " << typeid(PIPETYPE).name());
+		error = STRINGIZE(mDesc << ' ' << whichfile(slot) << " not a " << typeid(PIPETYPE).name());
 		return NULL;
 	}
 
@@ -895,10 +921,9 @@ boost::optional<LLProcess::ReadPipe&> LLProcess::getOptReadPipe(FILESLOT slot)
 
 std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params)
 {
-	std::string cwd(params.cwd);
-	if (! cwd.empty())
+	if (params.cwd.isProvided())
 	{
-		out << "cd " << LLStringUtil::quote(cwd) << ": ";
+		out << "cd " << LLStringUtil::quote(params.cwd) << ": ";
 	}
 	out << LLStringUtil::quote(params.executable);
 	BOOST_FOREACH(const std::string& arg, params.args)
-- 
cgit v1.2.3


From 3649eda62ad3a04203e6c562e78815a95896bbd4 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Wed, 29 Feb 2012 17:10:19 -0500
Subject: Guarantee LLProcess::Params::postend listener any ReadPipe data.
 Previously one might get process-terminated notification but still have to
 wait for the child process's final data to arrive on one or more ReadPipes.
 That required complex consumer timing logic to handle incomplete pending
 ReadPipe data, e.g. a partial last line with no terminating newline. New code
 guarantees that by the time LLProcess sends process-terminated notification,
 all pending pipe data will have been buffered in ReadPipes. Document
 LLProcess::ReadPipe::getPump() notification event; add "eof" key. Add
 LLProcess::ReadPipe::getline() and read() convenience methods. Add static
 LLProcess::getline() and basename() convenience methods, publishing logic
 already present elsewhere. Use ReadPipe::getline() and read() in unit tests.
 Add unit test for "eof" event on ReadPipe::getPump(). Add unit test verifying
 that final data have been buffered by termination notification event.

---
 indra/llcommon/llprocess.cpp | 107 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 88 insertions(+), 19 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 3b17b819bd..edfdebfe87 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -220,7 +220,8 @@ public:
 		// Essential to initialize our std::istream with our special streambuf!
 		mStream(&mStreambuf),
 		mPump("ReadPipe", true),    // tweak name as needed to avoid collisions
-		mLimit(0)
+		mLimit(0),
+		mEOF(false)
 	{
 		mConnection = LLEventPumps::instance().obtain("mainloop")
 			.listen(LLEventPump::inventName("ReadPipe"),
@@ -230,11 +231,25 @@ public:
 	// Much of the implementation is simply connecting the abstract virtual
 	// methods with implementation data concealed from the base class.
 	virtual std::istream& get_istream() { return mStream; }
+	virtual std::string getline() { return LLProcess::getline(mStream); }
 	virtual LLEventPump& getPump() { return mPump; }
 	virtual void setLimit(size_type limit) { mLimit = limit; }
 	virtual size_type getLimit() const { return mLimit; }
 	virtual size_type size() const { return mStreambuf.size(); }
 
+	virtual std::string read(size_type len)
+	{
+		// Read specified number of bytes into a buffer. Make a buffer big
+		// enough.
+		size_type readlen((std::min)(size(), len));
+		std::vector<char> buffer(readlen);
+		mStream.read(&buffer[0], readlen);
+		// Since we've already clamped 'readlen', we can think of no reason
+		// why mStream.read() should read fewer than 'readlen' bytes.
+		// Nonetheless, use the actual retrieved length.
+		return std::string(&buffer[0], mStream.gcount());
+	}
+
 	virtual std::string peek(size_type offset=0, size_type len=npos) const
 	{
 		// Constrain caller's offset and len to overlap actual buffer content.
@@ -287,14 +302,18 @@ public:
 		return (found == end)? npos : (found - begin);
 	}
 
-private:
 	bool tick(const LLSD&)
 	{
+		// Once we've hit EOF, skip all the rest of this.
+		if (mEOF)
+			return false;
+
 		typedef boost::asio::streambuf::mutable_buffers_type mutable_buffer_sequence;
 		// Try, every time, to read into our streambuf. In fact, we have no
 		// idea how much data the child might be trying to send: keep trying
 		// until we're convinced we've temporarily exhausted the pipe.
-		bool exhausted = false;
+		enum PipeState { RETRY, EXHAUSTED, CLOSED };
+		PipeState state = RETRY;
 		std::size_t committed(0);
 		do
 		{
@@ -329,7 +348,9 @@ private:
 					}
 					// Either way, though, we won't need any more tick() calls.
 					mConnection.disconnect();
-					exhausted = true; // also break outer retry loop
+					// Ignore any subsequent calls we might get anyway.
+					mEOF = true;
+					state = CLOSED; // also break outer retry loop
 					break;
 				}
 
@@ -347,7 +368,7 @@ private:
 				if (gotten < toread)
 				{
 					// break outer retry loop too
-					exhausted = true;
+					state = EXHAUSTED;
 					break;
 				}
 			}
@@ -356,15 +377,20 @@ private:
 			mStreambuf.commit(tocommit);
 			committed += tocommit;
 
-			// 'exhausted' is set when we can't fill any one buffer of the
-			// mutable_buffer_sequence established by the current prepare()
-			// call -- whether due to error or not enough bytes. That is,
-			// 'exhausted' is still false when we've filled every physical
+			// state is changed from RETRY when we can't fill any one buffer
+			// of the mutable_buffer_sequence established by the current
+			// prepare() call -- whether due to error or not enough bytes.
+			// That is, if state is still RETRY, we've filled every physical
 			// buffer in the mutable_buffer_sequence. In that case, for all we
 			// know, the child might have still more data pending -- go for it!
-		} while (! exhausted);
-
-		if (committed)
+		} while (state == RETRY);
+
+		// Once we recognize that the pipe is closed, make one more call to
+		// listener. The listener might be waiting for a particular substring
+		// to arrive, or a particular length of data or something. The event
+		// with "eof" == true announces that nothing further will arrive, so
+		// use it or lose it.
+		if (committed || state == CLOSED)
 		{
 			// If we actually received new data, publish it on our LLEventPump
 			// as advertised. Constrain it by mLimit. But show listener the
@@ -373,14 +399,16 @@ private:
 			mPump.post(LLSDMap
 					   ("data", peek(0, datasize))
 					   ("len", LLSD::Integer(mStreambuf.size()))
-					   ("index", LLSD::Integer(mIndex))
+					   ("slot", LLSD::Integer(mIndex))
 					   ("name", whichfile(mIndex))
-					   ("desc", mDesc));
+					   ("desc", mDesc)
+					   ("eof", state == CLOSED));
 		}
 
 		return false;
 	}
 
+private:
 	std::string mDesc;
 	apr_file_t* mPipe;
 	LLProcess::FILESLOT mIndex;
@@ -389,6 +417,7 @@ private:
 	std::istream mStream;
 	LLEventStream mPump;
 	size_type mLimit;
+	bool mEOF;
 };
 
 /// Need an exception to avoid constructing an invalid LLProcess object, but
@@ -641,17 +670,22 @@ static std::string getDesc(const LLProcess::Params& params)
 
 	// Caller didn't say. Use the executable name -- but use just the filename
 	// part. On Mac, for instance, full pathnames get cumbersome.
+	return LLProcess::basename(params.executable);
+}
+
+//static
+std::string LLProcess::basename(const std::string& path)
+{
 	// If there are Linden utility functions to manipulate pathnames, I
 	// haven't found them -- and for this usage, Boost.Filesystem seems kind
 	// of heavyweight.
-	std::string executable(params.executable);
-	std::string::size_type delim = executable.find_last_of("\\/");
-	// If executable contains no pathname delimiters, return the whole thing.
+	std::string::size_type delim = path.find_last_of("\\/");
+	// If path contains no pathname delimiters, return the whole thing.
 	if (delim == std::string::npos)
-		return executable;
+		return path;
 
 	// Return just the part beyond the last delimiter.
-	return executable.substr(delim + 1);
+	return path.substr(delim + 1);
 }
 
 LLProcess::~LLProcess()
@@ -804,6 +838,24 @@ void LLProcess::handle_status(int reason, int status)
 	// KILLED; refine below.
 	mStatus.mState = EXITED;
 
+	// Make last-gasp calls for each of the ReadPipes we have on hand. Since
+	// they're listening on "mainloop", we can be sure they'll eventually
+	// collect all pending data from the child. But we want to be able to
+	// guarantee to our consumer that by the time we post on the "postend"
+	// LLEventPump, our ReadPipes are already buffering all the data there
+	// will ever be from the child. That lets the "postend" listener decide
+	// what to do with that final data.
+	for (size_t i = 0; i < mPipes.size(); ++i)
+	{
+		std::string error;
+		ReadPipeImpl* ppipe = getPipePtr<ReadPipeImpl>(error, FILESLOT(i));
+		if (ppipe)
+		{
+			static LLSD trivial;
+			ppipe->tick(trivial);
+		}
+	}
+
 //	wi->rv = apr_proc_wait(wi->child, &wi->rc, &wi->why, APR_NOWAIT);
 	// It's just wrong to call apr_proc_wait() here. The only way APR knows to
 	// call us with APR_OC_REASON_DEATH is that it's already reaped this child
@@ -919,6 +971,23 @@ boost::optional<LLProcess::ReadPipe&> LLProcess::getOptReadPipe(FILESLOT slot)
 	return getOptPipe<ReadPipe>(slot);
 }
 
+//static
+std::string LLProcess::getline(std::istream& in)
+{
+	std::string line;
+	std::getline(in, line);
+	// Blur the distinction between "\r\n" and plain "\n". std::getline() will
+	// have eaten the "\n", but we could still end up with a trailing "\r".
+	std::string::size_type lastpos = line.find_last_not_of("\r");
+	if (lastpos != std::string::npos)
+	{
+		// Found at least one character that's not a trailing '\r'. SKIP OVER
+		// IT and erase the rest of the line.
+		line.erase(lastpos+1);
+	}
+	return line;
+}
+
 std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params)
 {
 	if (params.cwd.isProvided())
-- 
cgit v1.2.3


From 40dc3e0d3bee6ff70fb68d9ba7f0a2ee9da96f68 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Wed, 29 Feb 2012 19:40:18 -0500
Subject: When constructing a pipe to child stdin on Posix, ignore SIGPIPE. We
 can't count on every child process reading everything we try to write to it.
 And if the child terminates with WritePipe data still pending, unless we
 explicitly suppress it, Posix will hit us with SIGPIPE. That would terminate
 the calling process, boom. "Ignoring" it means APR gets the correct errno,
 passes it back to us, we log it, etc.

---
 indra/llcommon/llprocess.cpp | 9 +++++++++
 1 file changed, 9 insertions(+)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index edfdebfe87..8ccd39152b 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -145,6 +145,15 @@ public:
 		mConnection = LLEventPumps::instance().obtain("mainloop")
 			.listen(LLEventPump::inventName("WritePipe"),
 					boost::bind(&WritePipeImpl::tick, this, _1));
+
+#if ! LL_WINDOWS
+		// We can't count on every child process reading everything we try to
+		// write to it. And if the child terminates with WritePipe data still
+		// pending, unless we explicitly suppress it, Posix will hit us with
+		// SIGPIPE. That would terminate the viewer, boom. "Ignoring" it means
+		// APR gets the correct errno, passes it back to us, we log it, etc.
+		signal(SIGPIPE, SIG_IGN);
+#endif
 	}
 
 	virtual std::ostream& get_ostream() { return mStream; }
-- 
cgit v1.2.3


From 22fcb563ce45e64f23c9911bdcd07b0086bc892a Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Thu, 1 Mar 2012 14:27:32 -0500
Subject: Log better error message in case of apr_proc_create() failure. We
 were using uniform macro to report the APR function and its C++ parameter
 expressions. But specifically for apr_proc_create() failure, better to report
 the command we're attempting to execute.

---
 indra/llcommon/llprocess.cpp | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 8ccd39152b..9c49517598 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -617,8 +617,14 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 	// terminate with a null pointer
 	argv.push_back(NULL);
 
-	// Launch! The NULL would be the environment block, if we were passing one.
-	chkapr(apr_proc_create(&mProcess, argv[0], &argv[0], NULL, procattr, gAPRPoolp));    
+	// Launch! The NULL would be the environment block, if we were passing
+	// one. Hand-expand chkapr() macro so we can fill in the actual command
+	// string instead of the variable names.
+	if (ll_apr_warn_status(apr_proc_create(&mProcess, argv[0], &argv[0], NULL, procattr,
+										   gAPRPoolp)))
+	{
+		throw LLProcessError(STRINGIZE(params << " failed"));
+	}
 
 	// arrange to call status_callback()
 	apr_proc_other_child_register(&mProcess, &LLProcess::status_callback, this, mProcess.in,
-- 
cgit v1.2.3


From 260883c11f051853876d34bf1c9448d0063f9f5a Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Fri, 2 Mar 2012 10:40:15 -0500
Subject: Clarify LLProcess debug log message about reading from child pipe.
 Previous "read N of M bytes" wording implied that the child had M bytes to
 send, but we only read N of them. In reality we have no idea how many bytes
 the child is trying to send, only how many the OS is willing to deliver at
 this moment. To me, "filled N of M bytes" more clearly implies that M is the
 buffer size.

---
 indra/llcommon/llprocess.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 9c49517598..ad8e3a930e 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -367,7 +367,7 @@ public:
 				// received. Make sure we commit those later. (Don't commit them
 				// now, that would invalidate the buffer iterator sequence!)
 				tocommit += gotten;
-				LL_DEBUGS("LLProcess") << "read " << gotten << " of " << toread
+				LL_DEBUGS("LLProcess") << "filled " << gotten << " of " << toread
 									   << " bytes from " << mDesc << LL_ENDL;
 
 				// The parent end of this pipe is nonblocking. If we weren't even
-- 
cgit v1.2.3


From 75f412549242c949851938ffeac65b9e7145de5e Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Sat, 3 Mar 2012 06:45:19 -0500
Subject: Break large buffer into chunks to write to LLProcess child pipe. On
 Windows we ran into trouble trying to write a biggish (~1 MB) buffer of data
 to the child process's stdin pipe with a single apr_file_write() call. The
 child actually received corrupted data -- suggesting a possible bug in either
 APR or Windows pipes; the same test driving the same logic worked fine on Mac
 and Linux. Empirically, iterating over chunks of the buffered data is more
 robust.

---
 indra/llcommon/llprocess.cpp | 74 ++++++++++++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 27 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index ad8e3a930e..d926791e9e 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -168,38 +168,58 @@ public:
 			const_buffer_sequence bufs = mStreambuf.data();
 			// In general, our streambuf might contain a number of different
 			// physical buffers; iterate over those.
+			bool keepwriting = true;
 			for (const_buffer_sequence::const_iterator bufi(bufs.begin()), bufend(bufs.end());
-				 bufi != bufend; ++bufi)
+				 bufi != bufend && keepwriting; ++bufi)
 			{
 				// http://www.boost.org/doc/libs/1_49_0_beta1/doc/html/boost_asio/reference/buffer.html#boost_asio.reference.buffer.accessing_buffer_contents
-				std::size_t towrite(boost::asio::buffer_size(*bufi));
-				apr_size_t written(towrite);
-				apr_status_t err = apr_file_write(mPipe,
-												  boost::asio::buffer_cast<const void*>(*bufi),
-												  &written);
-				// EAGAIN is exactly what we want from a nonblocking pipe.
-				// Rather than waiting for data, it should return immediately.
-				if (! (err == APR_SUCCESS || APR_STATUS_IS_EAGAIN(err)))
+				// Although apr_file_write() accepts const void*, we
+				// manipulate const char* so we can increment the pointer.
+				const char* remainptr = boost::asio::buffer_cast<const char*>(*bufi);
+				std::size_t remainlen = boost::asio::buffer_size(*bufi);
+				while (remainlen)
 				{
-					LL_WARNS("LLProcess") << "apr_file_write(" << towrite << ") on " << mDesc
-										  << " got " << err << ":" << LL_ENDL;
-					ll_apr_warn_status(err);
-				}
+					// Tackle the current buffer in discrete chunks. On
+					// Windows, we've observed strange failures when trying to
+					// write big lengths (~1 MB) in a single operation.
+					std::size_t towrite((std::min)(remainlen, std::size_t(32*1024)));
+					apr_size_t written(towrite);
+					apr_status_t err = apr_file_write(mPipe, remainptr, &written);
+					// EAGAIN is exactly what we want from a nonblocking pipe.
+					// Rather than waiting for data, it should return immediately.
+					if (! (err == APR_SUCCESS || APR_STATUS_IS_EAGAIN(err)))
+					{
+						LL_WARNS("LLProcess") << "apr_file_write(" << towrite << ") on " << mDesc
+											  << " got " << err << ":" << LL_ENDL;
+						ll_apr_warn_status(err);
+					}
 
-				// 'written' is modified to reflect the number of bytes actually
-				// written. Make sure we consume those later. (Don't consume them
-				// now, that would invalidate the buffer iterator sequence!)
-				consumed += written;
-				LL_DEBUGS("LLProcess") << "wrote " << written << " of " << towrite
-									   << " bytes to " << mDesc
-									   << " (original " << total << ")" << LL_ENDL;
-
-				// The parent end of this pipe is nonblocking. If we weren't able
-				// to write everything we wanted, don't keep banging on it -- that
-				// won't change until the child reads some. Wait for next tick().
-				if (written < towrite)
-					break;
-			}
+					// 'written' is modified to reflect the number of bytes actually
+					// written. Make sure we consume those later. (Don't consume them
+					// now, that would invalidate the buffer iterator sequence!)
+					consumed += written;
+					// don't forget to advance to next chunk of current buffer
+					remainptr += written;
+					remainlen -= written;
+
+					char msgbuf[512];
+					LL_DEBUGS("LLProcess") << "wrote " << written << " of " << towrite
+										   << " bytes to " << mDesc
+										   << " (original " << total << "),"
+										   << " code " << err << ": "
+										   << apr_strerror(err, msgbuf, sizeof(msgbuf))
+										   << LL_ENDL;
+
+					// The parent end of this pipe is nonblocking. If we weren't able
+					// to write everything we wanted, don't keep banging on it -- that
+					// won't change until the child reads some. Wait for next tick().
+					if (written < towrite)
+					{
+						keepwriting = false; // break outer loop over buffers too
+						break;
+					}
+				} // next chunk of current buffer
+			}     // next buffer
 			// In all, we managed to write 'consumed' bytes. Remove them from the
 			// streambuf so we don't keep trying to send them. This could be
 			// anywhere from 0 up to mStreambuf.size(); anything we haven't yet
-- 
cgit v1.2.3


From e7ceb82e71ed88354758c6f16525aa051d47bdec Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Mon, 5 Mar 2012 19:00:23 -0500
Subject: Further reduce the block size that LLProcess writes to child pipe. It
 seems that on Windows, even 32K is too big: one in three load-test runs fails
 with a duplicated block. Empirically, reducing it to 4K makes it much more
 stable -- at least we can run successfully 100 consecutive times, which is a
 step in the right direction.

---
 indra/llcommon/llprocess.cpp | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index d926791e9e..178ec064c0 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -181,8 +181,15 @@ public:
 				{
 					// Tackle the current buffer in discrete chunks. On
 					// Windows, we've observed strange failures when trying to
-					// write big lengths (~1 MB) in a single operation.
-					std::size_t towrite((std::min)(remainlen, std::size_t(32*1024)));
+					// write big lengths (~1 MB) in a single operation. Even a
+					// 32K chunk seems too large. At some point along the way
+					// apr_file_write() returns 11 (Resource temporarily
+					// unavailable, i.e. EAGAIN) and says it wrote 0 bytes --
+					// even though it did write the chunk! Our next write
+					// attempt retries with the same chunk, resulting in the
+					// chunk being duplicated at the child end. Using smaller
+					// chunks is empirically more reliable.
+					std::size_t towrite((std::min)(remainlen, std::size_t(4*1024)));
 					apr_size_t written(towrite);
 					apr_status_t err = apr_file_write(mPipe, remainptr, &written);
 					// EAGAIN is exactly what we want from a nonblocking pipe.
-- 
cgit v1.2.3


From ab7fb5944a2c5d851944fec59a86c8b7e0df77d3 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Tue, 13 Mar 2012 14:40:46 -0400
Subject: Protect LLProcess destructor when run after APR shutdown. A static
 LLProcessPtr variable won't be destroyed until after procedural code has shut
 down APR. The trouble is that LLProcess's destructor unregisters itself from
 APR -- and, for an autokill LLProcess, attempts to kill the child process.
 All that is ill-advised after APR shutdown. Disable use of
 apr_pool_note_subprocess() mechanism. This should be another viable way of
 coping with static autokill LLProcessPtr variables: when the designated APR
 pool is cleaned up, APR promises to kill the child process. But whether it's
 an APR bug or a calling error, the present (now disabled) call in LLProcess
 results in OUR process, the viewer, getting SIGTERM when it asks to clean up
 the global APR pool.

---
 indra/llcommon/llprocess.cpp | 62 ++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 34 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 178ec064c0..bd08c3ab51 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -47,6 +47,9 @@
 #include <typeinfo>
 #include <utility>
 
+/*****************************************************************************
+*   Helpers
+*****************************************************************************/
 static const char* whichfile_[] = { "stdin", "stdout", "stderr" };
 static std::string empty;
 static LLProcess::Status interpret_status(int status);
@@ -127,6 +130,9 @@ private:
 };
 static LLProcessListener sProcessListener;
 
+/*****************************************************************************
+*   WritePipe and ReadPipe
+*****************************************************************************/
 LLProcess::BasePipe::~BasePipe() {}
 const LLProcess::BasePipe::size_type
 	  // use funky syntax to call max() to avoid blighted max() macros
@@ -456,6 +462,9 @@ private:
 	bool mEOF;
 };
 
+/*****************************************************************************
+*   LLProcess itself
+*****************************************************************************/
 /// Need an exception to avoid constructing an invalid LLProcess object, but
 /// internal use only
 struct LLProcessError: public std::runtime_error
@@ -669,11 +678,23 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 	// that doesn't always work (e.g. VWR-21538).
 	if (params.autokill)
 	{
+/*==========================================================================*|
+		// NO: There may be an APR bug, not sure -- but at least on Mac, when
+		// gAPRPoolp is destroyed, OUR process receives SIGTERM! Apparently
+		// either our own PID is getting into the list of processes to kill()
+		// (unlikely), or somehow one of those PIDs is getting zeroed first,
+		// so that kill() sends SIGTERM to the whole process group -- this
+		// process included. I'd have to build and link with a debug version
+		// of APR to know for sure. It's too bad: this mechanism would be just
+		// right for dealing with static autokill LLProcessPtr variables,
+		// which aren't destroyed until after APR is no longer available.
+
 		// Tie the lifespan of this child process to the lifespan of our APR
 		// pool: on destruction of the pool, forcibly kill the process. Tell
 		// APR to try SIGTERM and wait 3 seconds. If that didn't work, use
 		// SIGKILL.
 		apr_pool_note_subprocess(gAPRPoolp, &mProcess, APR_KILL_AFTER_TIMEOUT);
+|*==========================================================================*/
 
 		// On Windows, associate the new child process with our Job Object.
 		autokill();
@@ -732,6 +753,13 @@ std::string LLProcess::basename(const std::string& path)
 
 LLProcess::~LLProcess()
 {
+	// In the Linden viewer, there's at least one static LLProcessPtr. Its
+	// destructor will be called *after* ll_cleanup_apr(). In such a case,
+	// unregistering is pointless (and fatal!) -- and kill(), which also
+	// relies on APR, is impossible.
+	if (! gAPRPoolp)
+		return;
+
 	// Only in state RUNNING are we registered for callback. In UNSTARTED we
 	// haven't yet registered. And since receiving the callback is the only
 	// way we detect child termination, we only change from state RUNNING at
@@ -1263,38 +1291,4 @@ static LLProcess::Status interpret_status(int status)
 	return result;
 }
 
-/*==========================================================================*|
-static std::list<pid_t> sZombies;
-
-void LLProcess::orphan(void)
-{
-	// Disassociate the process from this object
-	if(mProcessID != 0)
-	{	
-		// We may still need to reap the process's zombie eventually
-		sZombies.push_back(mProcessID);
-	
-		mProcessID = 0;
-	}
-}
-
-// static 
-void LLProcess::reap(void)
-{
-	// Attempt to real all saved process ID's.
-	
-	std::list<pid_t>::iterator iter = sZombies.begin();
-	while(iter != sZombies.end())
-	{
-		if(reap_pid(*iter))
-		{
-			iter = sZombies.erase(iter);
-		}
-		else
-		{
-			iter++;
-		}
-	}
-}
-|*==========================================================================*/
 #endif  // Posix
-- 
cgit v1.2.3


From 4edf34ed01611d75bdcd98aa065a2b286845ebd9 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Thu, 15 Mar 2012 23:30:36 -0400
Subject: Promote LLProcess::ReadPipe::size() to BasePipe (hence WritePipe).
 Certain use cases need to know whether the WritePipe buffer has been flushed
 to the pipe, or is still pending.

---
 indra/llcommon/llprocess.cpp | 1 +
 1 file changed, 1 insertion(+)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index bd08c3ab51..d4786035ce 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -163,6 +163,7 @@ public:
 	}
 
 	virtual std::ostream& get_ostream() { return mStream; }
+	virtual size_type size() const { return mStreambuf.size(); }
 
 	bool tick(const LLSD&)
 	{
-- 
cgit v1.2.3


From 38e23bb0eb71e160fdfa829398a46ec3db01d7aa Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Wed, 18 Apr 2012 15:43:34 -0400
Subject: IQA-463: Make LLProcess call apr_procattr_inherit_set() extension. On
 Windows, Bad Things happen when apr_proc_create() is allowed to pass TRUE to
 CreateProcess(bInheritHandles). For instance, the open handle for a new
 installer executable file being downloaded by the background updater gets
 inadvertently passed to a couple slplugin.exe instances. When the viewer
 finishes downloading, closes the file and tries to remove it, Windows balks
 because the file is still open by another process. Require an apr_suite
 package that includes the new Linden apr_procattr_inherit_set() extension,
 and call it to turn off CreateProcess(bInheritHandles).

---
 indra/llcommon/llprocess.cpp | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index d4786035ce..e96b328365 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -535,6 +535,24 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 	apr_procattr_t *procattr = NULL;
 	chkapr(apr_procattr_create(&procattr, gAPRPoolp));
 
+#if ! defined(APR_HAS_PROCATTR_INHERIT_SET)
+	// Our special preprocessor symbol isn't even defined -- wrong APR
+	LL_WARNS("LLProcess") << "This version of APR lacks Linden apr_procattr_inherit_set() extension" << LL_ENDL;
+#elif ! APR_HAS_PROCATTR_INHERIT_SET
+	// Symbol is defined, but to 0: expect apr_procattr_inherit_set() to
+	// return APR_ENOTIMPL.
+	LL_DEBUGS("LLProcess") << "apr_procattr_inherit_set() not supported on this platform" << LL_ENDL;
+#else  // APR_HAS_PROCATTR_INHERIT_SET nonzero
+	// As of 2012-04-17, the original Windows implementation of
+	// apr_proc_create() unconditionally passes TRUE for bInheritHandles. That
+	// seems to assume that all files are opened by APR so you can
+	// individually control whether each is inherited by a child process. But
+	// we've been burned by having surprising open file handles inherited by
+	// our child processes. Turn that OFF for us!
+	LL_DEBUGS("LLProcess") << "Setting apr_procattr_inherit_set(0)" << LL_ENDL;
+	ll_apr_warn_status(apr_procattr_inherit_set(procattr, 0));
+#endif
+
 	// For which of stdin, stdout, stderr should we create a pipe to the
 	// child? In the viewer, there are only a couple viable
 	// apr_procattr_io_set() alternatives: inherit the viewer's own stdxxx
@@ -607,17 +625,14 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 
 	if (params.autokill)
 	{
-#if defined(APR_HAS_PROCATTR_AUTOKILL_SET)
-		apr_status_t ok = apr_procattr_autokill_set(procattr, 1);
-# if LL_WINDOWS
-		// As of 2012-02-02, we only expect this to be implemented on Windows.
-		// Avoid spamming the log with warnings we fully expect.
-		ll_apr_warn_status(ok);
-#else   // ! LL_WINDOWS
-		(void)ok;                   // suppress 'unused' warning
-# endif // ! LL_WINDOWS
-#else
+#if ! defined(APR_HAS_PROCATTR_AUTOKILL_SET)
+		// Our special preprocessor symbol isn't even defined -- wrong APR
 		LL_WARNS("LLProcess") << "This version of APR lacks Linden apr_procattr_autokill_set() extension" << LL_ENDL;
+#elif ! APR_HAS_PROCATTR_AUTOKILL_SET
+		// Symbol is defined, but to 0: expect apr_procattr_autokill_set() to
+		// return APR_ENOTIMPL.
+#else   // APR_HAS_PROCATTR_AUTOKILL_SET nonzero
+		ll_apr_warn_status(apr_procattr_autokill_set(procattr, 1));
 #endif
 	}
 
-- 
cgit v1.2.3


From 78816bb1561190ac5a882caa90da2865d4aaa353 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Mon, 23 Apr 2012 12:01:15 -0400
Subject: IQA-463: Use APR file I/O for downloaded viewer installer .exe. On
 Windows, calling CreateProcess(bInheritHandles=FALSE) is the wrong idea. In
 that case, CreateProcess() passes NO handles -- even the files you've
 explicitly designated as the child's stdin, stdout, stderr in the STARTUPINFO
 struct! Remove LLProcess code to tweak bInheritHandles; we should also remove
 the corresponding (useless) APR extension. Instead, given that the Windows
 file-locking problem we've observed is specific to the viewer installer .exe
 file downloaded by the background updater logic, use APR file I/O for that
 specific file. Empirically, both llofstream and std::ofstream seem to make
 the open file handle inheritable; but apr_file_open() documentation says: "By
 default, the returned file descriptor will not be inherited by child
 processes created by apr_proc_create()." And indeed, it does appear to
 sidestep the locking problem.

---
 indra/llcommon/llprocess.cpp | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index e96b328365..760be6da9b 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -282,9 +282,13 @@ public:
 
 	virtual std::string read(size_type len)
 	{
-		// Read specified number of bytes into a buffer. Make a buffer big
-		// enough.
+		// Read specified number of bytes into a buffer.
 		size_type readlen((std::min)(size(), len));
+		// Formally, &buffer[0] is invalid for a vector of size() 0. Exit
+		// early in that situation.
+		if (! readlen)
+			return "";
+		// Make a buffer big enough.
 		std::vector<char> buffer(readlen);
 		mStream.read(&buffer[0], readlen);
 		// Since we've already clamped 'readlen', we can think of no reason
@@ -535,24 +539,6 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 	apr_procattr_t *procattr = NULL;
 	chkapr(apr_procattr_create(&procattr, gAPRPoolp));
 
-#if ! defined(APR_HAS_PROCATTR_INHERIT_SET)
-	// Our special preprocessor symbol isn't even defined -- wrong APR
-	LL_WARNS("LLProcess") << "This version of APR lacks Linden apr_procattr_inherit_set() extension" << LL_ENDL;
-#elif ! APR_HAS_PROCATTR_INHERIT_SET
-	// Symbol is defined, but to 0: expect apr_procattr_inherit_set() to
-	// return APR_ENOTIMPL.
-	LL_DEBUGS("LLProcess") << "apr_procattr_inherit_set() not supported on this platform" << LL_ENDL;
-#else  // APR_HAS_PROCATTR_INHERIT_SET nonzero
-	// As of 2012-04-17, the original Windows implementation of
-	// apr_proc_create() unconditionally passes TRUE for bInheritHandles. That
-	// seems to assume that all files are opened by APR so you can
-	// individually control whether each is inherited by a child process. But
-	// we've been burned by having surprising open file handles inherited by
-	// our child processes. Turn that OFF for us!
-	LL_DEBUGS("LLProcess") << "Setting apr_procattr_inherit_set(0)" << LL_ENDL;
-	ll_apr_warn_status(apr_procattr_inherit_set(procattr, 0));
-#endif
-
 	// For which of stdin, stdout, stderr should we create a pipe to the
 	// child? In the viewer, there are only a couple viable
 	// apr_procattr_io_set() alternatives: inherit the viewer's own stdxxx
-- 
cgit v1.2.3


From d29f920c22ca67b13f42680c432b689b80909f42 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Wed, 9 May 2012 19:55:26 -0400
Subject: CHOP-900: Use new apr_procattr_constrain_handle_set() extension. Now
 LLProcess explicitly requests APR to limit the handles passed to any child
 process, instead of wantonly passing whatever happens to be lying around the
 parent process at the time. This requires the latest APR build. Also revert
 LLUpdateDownloader::Implementation::mDownloadStream to llofstream (as in rev
 1878a57aebd7) instead of apr_file_t*. Using APR for that file was a Band-Aid
 -- a single whacked mole -- for the problem more systemically addressed by
 apr_procattr_constrain_handle_set().

---
 indra/llcommon/llprocess.cpp | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

(limited to 'indra/llcommon/llprocess.cpp')

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 760be6da9b..9667e4e033 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -539,6 +539,23 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 	apr_procattr_t *procattr = NULL;
 	chkapr(apr_procattr_create(&procattr, gAPRPoolp));
 
+	// IQA-490, CHOP-900: On Windows, ask APR to jump through hoops to
+	// constrain the set of handles passed to the child process. Before we
+	// changed to APR, the Windows implementation of LLProcessLauncher called
+	// CreateProcess(bInheritHandles=FALSE), meaning to pass NO open handles
+	// to the child process. Now that we support pipes, though, we must allow
+	// apr_proc_create() to pass bInheritHandles=TRUE. But without taking
+	// special pains, that causes trouble in a number of ways, due to the fact
+	// that the viewer is constantly opening and closing files -- most of
+	// which CreateProcess() passes to every child process!
+#if ! defined(APR_HAS_PROCATTR_CONSTRAIN_HANDLE_SET)
+	// Our special preprocessor symbol isn't even defined -- wrong APR
+	LL_WARNS("LLProcess") << "This version of APR lacks Linden "
+						  << "apr_procattr_constrain_handle_set() extension" << LL_ENDL;
+#else
+	chkapr(apr_procattr_constrain_handle_set(procattr, 1));
+#endif
+
 	// For which of stdin, stdout, stderr should we create a pipe to the
 	// child? In the viewer, there are only a couple viable
 	// apr_procattr_io_set() alternatives: inherit the viewer's own stdxxx
-- 
cgit v1.2.3