From f0dbb878337082d3f581874c12e6df2f4659a464 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed 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/llplugin/llpluginprocessparent.cpp | 47 ++++++++++++++++---------------- 1 file changed, 24 insertions(+), 23 deletions(-) (limited to 'indra/llplugin/llpluginprocessparent.cpp') diff --git a/indra/llplugin/llpluginprocessparent.cpp b/indra/llplugin/llpluginprocessparent.cpp index 110fac0f23..9b225cabb8 100644 --- a/indra/llplugin/llpluginprocessparent.cpp +++ b/indra/llplugin/llpluginprocessparent.cpp @@ -31,6 +31,7 @@ #include "llpluginprocessparent.h" #include "llpluginmessagepipe.h" #include "llpluginmessageclasses.h" +#include "stringize.h" #include "llapr.h" @@ -134,7 +135,10 @@ LLPluginProcessParent::~LLPluginProcessParent() mSharedMemoryRegions.erase(iter); } - mProcess.kill(); + if (mProcess) + { + mProcess->kill(); + } killSockets(); } @@ -159,8 +163,8 @@ void LLPluginProcessParent::errorState(void) void LLPluginProcessParent::init(const std::string &launcher_filename, const std::string &plugin_dir, const std::string &plugin_filename, bool debug) { - mProcess.setExecutable(launcher_filename); - mProcess.setWorkingDirectory(plugin_dir); + mProcessParams["executable"] = launcher_filename; + mProcessParams["cwd"] = plugin_dir; mPluginFile = plugin_filename; mPluginDir = plugin_dir; mCPUUsage = 0.0f; @@ -371,10 +375,8 @@ void LLPluginProcessParent::idle(void) // Launch the plugin process. // Only argument to the launcher is the port number we're listening on - std::stringstream stream; - stream << mBoundPort; - mProcess.addArgument(stream.str()); - if(mProcess.launch() != 0) + mProcessParams["args"].append(stringize(mBoundPort)); + if (! (mProcess = LLProcess::create(mProcessParams))) { errorState(); } @@ -388,19 +390,18 @@ void LLPluginProcessParent::idle(void) // The command we're constructing would look like this on the command line: // osascript -e 'tell application "Terminal"' -e 'set win to do script "gdb -pid 12345"' -e 'do script "continue" in win' -e 'end tell' - std::stringstream cmd; - - mDebugger.setExecutable("/usr/bin/osascript"); - mDebugger.addArgument("-e"); - mDebugger.addArgument("tell application \"Terminal\""); - mDebugger.addArgument("-e"); - cmd << "set win to do script \"gdb -pid " << mProcess.getProcessID() << "\""; - mDebugger.addArgument(cmd.str()); - mDebugger.addArgument("-e"); - mDebugger.addArgument("do script \"continue\" in win"); - mDebugger.addArgument("-e"); - mDebugger.addArgument("end tell"); - mDebugger.launch(); + LLSD params; + params["executable"] = "/usr/bin/osascript"; + params["args"].append("-e"); + params["args"].append("tell application \"Terminal\""); + params["args"].append("-e"); + params["args"].append(STRINGIZE("set win to do script \"gdb -pid " + << mProcess->getProcessID() << "\"")); + params["args"].append("-e"); + params["args"].append("do script \"continue\" in win"); + params["args"].append("-e"); + params["args"].append("end tell"); + mDebugger = LLProcess::create(params); #endif } @@ -470,7 +471,7 @@ void LLPluginProcessParent::idle(void) break; case STATE_EXITING: - if(!mProcess.isRunning()) + if (! mProcess->isRunning()) { setState(STATE_CLEANUP); } @@ -498,7 +499,7 @@ void LLPluginProcessParent::idle(void) break; case STATE_CLEANUP: - mProcess.kill(); + mProcess->kill(); killSockets(); setState(STATE_DONE); break; @@ -1077,7 +1078,7 @@ bool LLPluginProcessParent::pluginLockedUpOrQuit() { bool result = false; - if(!mProcess.isRunning()) + if (! mProcess->isRunning()) { LL_WARNS("Plugin") << "child exited" << LL_ENDL; result = true; -- cgit v1.3 From 47d94757075e338c480ba4d7d24948242a85a9bb Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sat, 21 Jan 2012 11:45:15 -0500 Subject: Convert LLProcess consumers from LLSD to LLProcess::Params block. Using a Params block gives compile-time checking against attribute typos. One might inadvertently set myLLSD["autofill"] = false and only discover it when things behave strangely at runtime; but trying to set myParams.autofill will produce a compile error. However, it's excellent that the same LLProcess::create() method can accept either LLProcess::Params or a properly-constructed LLSD block. --- indra/llcommon/tests/llprocess_test.cpp | 26 ++++++++--------- indra/llcommon/tests/llsdserialize_test.cpp | 6 ++-- indra/llplugin/llpluginprocessparent.cpp | 28 +++++++++--------- indra/llplugin/llpluginprocessparent.h | 12 ++++---- indra/newview/llexternaleditor.cpp | 33 ++++++++++++---------- indra/newview/llexternaleditor.h | 5 ++-- .../updater/llupdateinstaller.cpp | 12 ++++---- 7 files changed, 62 insertions(+), 60 deletions(-) (limited to 'indra/llplugin/llpluginprocessparent.cpp') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 55e22abd81..405540e436 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -107,8 +107,8 @@ struct PythonProcessLauncher const char* PYTHON(getenv("PYTHON")); tut::ensure("Set $PYTHON to the Python interpreter", PYTHON); - mParams["executable"] = PYTHON; - mParams["args"].append(mScript.getName()); + mParams.executable = PYTHON; + mParams.args.add(mScript.getName()); } /// Run Python script and wait for it to complete. @@ -142,13 +142,13 @@ struct PythonProcessLauncher { NamedTempFile out("out", ""); // placeholder // pass name of this temporary file to the script - mParams["args"].append(out.getName()); + mParams.args.add(out.getName()); run(); // assuming the script wrote to that file, read it return readfile(out.getName(), STRINGIZE("from " << mDesc << " script")); } - LLSD mParams; + LLProcess::Params mParams; LLProcessPtr mPy; std::string mDesc; NamedTempFile mScript; @@ -515,7 +515,7 @@ namespace tut "with open(sys.argv[1], 'w') as f:\n" " f.write(os.getcwd())\n"); // Before running, call setWorkingDirectory() - py.mParams["cwd"] = tempdir.getName(); + py.mParams.cwd = tempdir.getName(); ensure_equals("os.getcwd()", py.run_read(), tempdir.getName()); } @@ -531,9 +531,9 @@ namespace tut " for arg in sys.argv[1:]:\n" " print >>f, arg\n"); // We expect that PythonProcessLauncher has already appended - // its own NamedTempFile to mParams["args"] (sys.argv[0]). - py.mParams["args"].append("first arg"); // sys.argv[1] - py.mParams["args"].append("second arg"); // sys.argv[2] + // its own NamedTempFile to mParams.args (sys.argv[0]). + py.mParams.args.add("first arg"); // sys.argv[1] + py.mParams.args.add("second arg"); // sys.argv[2] // run_read() appends() one more argument, hence [3] std::string output(py.run_read()); boost::split_iterator @@ -568,7 +568,7 @@ namespace tut "with open(sys.argv[1], 'w') as f:\n" " f.write('bad')\n"); NamedTempFile out("out", "not started"); - py.mParams["args"].append(out.getName()); + py.mParams.args.add(out.getName()); py.mPy = LLProcess::create(py.mParams); ensure("couldn't launch kill() script", py.mPy); // Wait for the script to wake up and do its first write @@ -611,7 +611,7 @@ namespace tut "# if caller hasn't managed to kill by now, bad\n" "with open(sys.argv[1], 'w') as f:\n" " f.write('bad')\n"); - py.mParams["args"].append(out.getName()); + py.mParams.args.add(out.getName()); py.mPy = LLProcess::create(py.mParams); ensure("couldn't launch kill() script", py.mPy); // Capture id for later @@ -667,9 +667,9 @@ namespace tut "# okay, saw 'go', write 'ack'\n" "with open(sys.argv[1], 'w') as f:\n" " f.write('ack')\n"); - py.mParams["args"].append(from.getName()); - py.mParams["args"].append(to.getName()); - py.mParams["autokill"] = false; + py.mParams.args.add(from.getName()); + py.mParams.args.add(to.getName()); + py.mParams.autokill = false; py.mPy = LLProcess::create(py.mParams); ensure("couldn't launch kill() script", py.mPy); // Capture id for later diff --git a/indra/llcommon/tests/llsdserialize_test.cpp b/indra/llcommon/tests/llsdserialize_test.cpp index 7756ba6226..e625545763 100644 --- a/indra/llcommon/tests/llsdserialize_test.cpp +++ b/indra/llcommon/tests/llsdserialize_test.cpp @@ -1557,9 +1557,9 @@ namespace tut } #else // LL_DARWIN, LL_LINUX - LLSD params; - params["executable"] = PYTHON; - params["args"].append(scriptfile.getName()); + LLProcess::Params params; + params.executable = PYTHON; + params.args.add(scriptfile.getName()); LLProcessPtr py(LLProcess::create(params)); ensure(STRINGIZE("Couldn't launch " << desc << " script"), py); // Implementing timeout would mean messing with alarm() and diff --git a/indra/llplugin/llpluginprocessparent.cpp b/indra/llplugin/llpluginprocessparent.cpp index 9b225cabb8..f10eaee5b4 100644 --- a/indra/llplugin/llpluginprocessparent.cpp +++ b/indra/llplugin/llpluginprocessparent.cpp @@ -163,8 +163,8 @@ void LLPluginProcessParent::errorState(void) void LLPluginProcessParent::init(const std::string &launcher_filename, const std::string &plugin_dir, const std::string &plugin_filename, bool debug) { - mProcessParams["executable"] = launcher_filename; - mProcessParams["cwd"] = plugin_dir; + mProcessParams.executable = launcher_filename; + mProcessParams.cwd = plugin_dir; mPluginFile = plugin_filename; mPluginDir = plugin_dir; mCPUUsage = 0.0f; @@ -375,7 +375,7 @@ void LLPluginProcessParent::idle(void) // Launch the plugin process. // Only argument to the launcher is the port number we're listening on - mProcessParams["args"].append(stringize(mBoundPort)); + mProcessParams.args.add(stringize(mBoundPort)); if (! (mProcess = LLProcess::create(mProcessParams))) { errorState(); @@ -390,17 +390,17 @@ void LLPluginProcessParent::idle(void) // The command we're constructing would look like this on the command line: // osascript -e 'tell application "Terminal"' -e 'set win to do script "gdb -pid 12345"' -e 'do script "continue" in win' -e 'end tell' - LLSD params; - params["executable"] = "/usr/bin/osascript"; - params["args"].append("-e"); - params["args"].append("tell application \"Terminal\""); - params["args"].append("-e"); - params["args"].append(STRINGIZE("set win to do script \"gdb -pid " - << mProcess->getProcessID() << "\"")); - params["args"].append("-e"); - params["args"].append("do script \"continue\" in win"); - params["args"].append("-e"); - params["args"].append("end tell"); + LLProcess::Params params; + params.executable = "/usr/bin/osascript"; + params.args.add("-e"); + params.args.add("tell application \"Terminal\""); + params.args.add("-e"); + params.args.add(STRINGIZE("set win to do script \"gdb -pid " + << mProcess->getProcessID() << "\"")); + params.args.add("-e"); + params.args.add("do script \"continue\" in win"); + params.args.add("-e"); + params.args.add("end tell"); mDebugger = LLProcess::create(params); #endif diff --git a/indra/llplugin/llpluginprocessparent.h b/indra/llplugin/llpluginprocessparent.h index e8bcba75e0..990fc5cbae 100644 --- a/indra/llplugin/llpluginprocessparent.h +++ b/indra/llplugin/llpluginprocessparent.h @@ -140,27 +140,27 @@ private: }; EState mState; void setState(EState state); - + bool pluginLockedUp(); bool pluginLockedUpOrQuit(); bool accept(); - + LLSocket::ptr_t mListenSocket; LLSocket::ptr_t mSocket; U32 mBoundPort; - LLSD mProcessParams; + LLProcess::Params mProcessParams; LLProcessPtr mProcess; - + std::string mPluginFile; std::string mPluginDir; LLPluginProcessParentOwner *mOwner; - + typedef std::map sharedMemoryRegionsType; sharedMemoryRegionsType mSharedMemoryRegions; - + LLSD mMessageClassVersions; std::string mPluginVersionString; diff --git a/indra/newview/llexternaleditor.cpp b/indra/newview/llexternaleditor.cpp index ba58cd8067..3dfebad958 100644 --- a/indra/newview/llexternaleditor.cpp +++ b/indra/newview/llexternaleditor.cpp @@ -60,22 +60,22 @@ LLExternalEditor::EErrorCode LLExternalEditor::setCommand(const std::string& env } // Save command. - mProcessParams["executable"] = bin_path; - mProcessParams["args"].clear(); + mProcessParams = LLProcess::Params(); + mProcessParams.executable = bin_path; for (size_t i = 1; i < tokens.size(); ++i) { - mProcessParams["args"].append(tokens[i]); + mProcessParams.args.add(tokens[i]); } // Add the filename marker if missing. if (cmd.find(sFilenameMarker) == std::string::npos) { - mProcessParams["args"].append(sFilenameMarker); + mProcessParams.args.add(sFilenameMarker); llinfos << "Adding the filename marker (" << sFilenameMarker << ")" << llendl; } llinfos << "Setting command [" << bin_path; - BOOST_FOREACH(const std::string& arg, llsd::inArray(mProcessParams["args"])) + BOOST_FOREACH(const std::string& arg, mProcessParams.args) { llcont << " \"" << arg << "\""; } @@ -86,33 +86,36 @@ LLExternalEditor::EErrorCode LLExternalEditor::setCommand(const std::string& env LLExternalEditor::EErrorCode LLExternalEditor::run(const std::string& file_path) { - if (mProcessParams["executable"].asString().empty() || ! mProcessParams["args"].size()) + // LLInitParams type wrappers don't seem to have empty() or size() + // methods; try determining emptiness by comparing begin/end iterators. + if (std::string(mProcessParams.executable).empty() || + (mProcessParams.args.begin() == mProcessParams.args.end())) { llwarns << "Editor command not set" << llendl; return EC_NOT_SPECIFIED; } // Copy params block so we can replace sFilenameMarker - LLSD params(mProcessParams); + LLProcess::Params params; + params.executable = mProcessParams.executable; // Substitute the filename marker in the command with the actual passed file name. - LLSD& args(params["args"]); - for (LLSD::array_iterator ai(args.beginArray()), aend(args.endArray()); ai != aend; ++ai) + BOOST_FOREACH(const std::string& arg, mProcessParams.args) { - std::string sarg(*ai); - LLStringUtil::replaceString(sarg, sFilenameMarker, file_path); - *ai = sarg; + std::string fixed(arg); + LLStringUtil::replaceString(fixed, sFilenameMarker, file_path); + params.args.add(fixed); } // Run the editor. - llinfos << "Running editor command [" << params["executable"]; - BOOST_FOREACH(const std::string& arg, llsd::inArray(params["args"])) + llinfos << "Running editor command [" << std::string(params.executable); + BOOST_FOREACH(const std::string& arg, params.args) { llcont << " \"" << arg << "\""; } llcont << "]" << llendl; // Prevent killing the process in destructor. - params["autokill"] = false; + params.autokill = false; return LLProcess::create(params) ? EC_SUCCESS : EC_FAILED_TO_RUN; } diff --git a/indra/newview/llexternaleditor.h b/indra/newview/llexternaleditor.h index e81c360c24..fd2c25020c 100644 --- a/indra/newview/llexternaleditor.h +++ b/indra/newview/llexternaleditor.h @@ -27,7 +27,7 @@ #ifndef LL_LLEXTERNALEDITOR_H #define LL_LLEXTERNALEDITOR_H -#include "llsd.h" +#include "llprocess.h" /** * Usage: @@ -97,8 +97,7 @@ private: */ static const std::string sSetting; - - LLSD mProcessParams; + LLProcess::Params mProcessParams; }; #endif // LL_LLEXTERNALEDITOR_H diff --git a/indra/viewer_components/updater/llupdateinstaller.cpp b/indra/viewer_components/updater/llupdateinstaller.cpp index e99fd0af7e..2f87d59373 100644 --- a/indra/viewer_components/updater/llupdateinstaller.cpp +++ b/indra/viewer_components/updater/llupdateinstaller.cpp @@ -78,12 +78,12 @@ int ll_install_update(std::string const & script, llinfos << "UpdateInstaller: installing " << updatePath << " using " << actualScriptPath << LL_ENDL; - LLSD params; - params["executable"] = actualScriptPath; - params["args"].append(updatePath); - params["args"].append(ll_install_failed_marker_path()); - params["args"].append(boost::lexical_cast(required)); - params["autokill"] = false; + LLProcess::Params params; + params.executable = actualScriptPath; + params.args.add(updatePath); + params.args.add(ll_install_failed_marker_path()); + params.args.add(boost::lexical_cast(required)); + params.autokill = false; return LLProcess::create(params)? 0 : -1; } -- cgit v1.3 From 166f75d91f180d0ac45f0b8ee7c3a1b6742209fa Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 6 Jun 2012 16:38:16 -0400 Subject: MAINT-1144: Defend against NULL LLPluginProcessParent::mProcess. The change from LLProcessLauncher to LLProcess introduces the possibility of a NULL (default-constructed) LLProcessPtr. Add certain static LLProcess methods accepting LLProcessPtr, forwarding to nonstatic method when non-NULL but doing something reasonable with NULL. Use these methods in LLPLuginProcessParent. --- indra/llcommon/llprocess.cpp | 38 ++++++++++++++++++++++++++++++++ indra/llcommon/llprocess.h | 11 +++++++-- indra/llplugin/llpluginprocessparent.cpp | 13 +++++------ 3 files changed, 52 insertions(+), 10 deletions(-) (limited to 'indra/llplugin/llpluginprocessparent.cpp') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 9667e4e033..715df36f39 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -819,16 +819,43 @@ bool LLProcess::kill(const std::string& who) return ! isRunning(); } +//static +bool LLProcess::kill(const LLProcessPtr& p, const std::string& who) +{ + if (! p) + return true; // process dead! (was never running) + return p->kill(who); +} + bool LLProcess::isRunning() const { return getStatus().mState == RUNNING; } +//static +bool LLProcess::isRunning(const LLProcessPtr& p) +{ + if (! p) + return false; + return p->isRunning(); +} + LLProcess::Status LLProcess::getStatus() const { return mStatus; } +//static +LLProcess::Status LLProcess::getStatus(const LLProcessPtr& p) +{ + if (! p) + { + // default-constructed Status has mState == UNSTARTED + return Status(); + } + return p->getStatus(); +} + std::string LLProcess::getStatusString() const { return getStatusString(getStatus()); @@ -839,6 +866,17 @@ std::string LLProcess::getStatusString(const Status& status) const return getStatusString(mDesc, status); } +//static +std::string LLProcess::getStatusString(const std::string& desc, const LLProcessPtr& p) +{ + if (! p) + { + // default-constructed Status has mState == UNSTARTED + return getStatusString(desc, Status()); + } + return desc + " " + p->getStatusString(); +} + //static std::string LLProcess::getStatusString(const std::string& desc, const Status& status) { diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 51010966f9..d711ce2f74 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -239,6 +239,10 @@ public: /// Is child process still running? bool isRunning() const; + // static isRunning(LLProcessPtr), getStatus(LLProcessPtr), + // getStatusString(LLProcessPtr), kill(LLProcessPtr) handle the case in + // which the passed LLProcessPtr might be NULL (default-constructed). + static bool isRunning(const LLProcessPtr&); /** * State of child process @@ -272,8 +276,10 @@ public: /// Status query Status getStatus() const; + static Status getStatus(const LLProcessPtr&); /// English Status string query, for logging etc. std::string getStatusString() const; + static std::string getStatusString(const std::string& desc, const LLProcessPtr&); /// English Status string query for previously-captured Status std::string getStatusString(const Status& status) const; /// static English Status string query @@ -282,6 +288,7 @@ public: // Attempt to kill the process -- returns true if the process is no longer running when it returns. // Note that even if this returns false, the process may exit some time after it's called. bool kill(const std::string& who=""); + static bool kill(const LLProcessPtr& p, const std::string& who=""); #if LL_WINDOWS typedef int id; ///< as returned by getProcessID() @@ -314,10 +321,10 @@ public: * New functionality should be added as nonstatic members operating on * the same data as getProcessHandle(). * - * In particular, if child termination is detected by static isRunning() + * In particular, if child termination is detected by this static isRunning() * rather than by nonstatic isRunning(), the LLProcess object won't be * aware of the child's changed status and may encounter OS errors trying - * to obtain it. static isRunning() is only intended for after the + * to obtain it. This static isRunning() is only intended for after the * launching LLProcess object has been destroyed. */ static handle isRunning(handle, const std::string& desc=""); diff --git a/indra/llplugin/llpluginprocessparent.cpp b/indra/llplugin/llpluginprocessparent.cpp index f10eaee5b4..71a6145b58 100644 --- a/indra/llplugin/llpluginprocessparent.cpp +++ b/indra/llplugin/llpluginprocessparent.cpp @@ -134,11 +134,8 @@ LLPluginProcessParent::~LLPluginProcessParent() // and remove it from our map mSharedMemoryRegions.erase(iter); } - - if (mProcess) - { - mProcess->kill(); - } + + LLProcess::kill(mProcess); killSockets(); } @@ -471,7 +468,7 @@ void LLPluginProcessParent::idle(void) break; case STATE_EXITING: - if (! mProcess->isRunning()) + if (! LLProcess::isRunning(mProcess)) { setState(STATE_CLEANUP); } @@ -499,7 +496,7 @@ void LLPluginProcessParent::idle(void) break; case STATE_CLEANUP: - mProcess->kill(); + LLProcess::kill(mProcess); killSockets(); setState(STATE_DONE); break; @@ -1078,7 +1075,7 @@ bool LLPluginProcessParent::pluginLockedUpOrQuit() { bool result = false; - if (! mProcess->isRunning()) + if (! LLProcess::isRunning(mProcess)) { LL_WARNS("Plugin") << "child exited" << LL_ENDL; result = true; -- cgit v1.3