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/llcommon/llprocess.h | 106 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 indra/llcommon/llprocess.h (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h new file mode 100644 index 0000000000..9a74cfe829 --- /dev/null +++ b/indra/llcommon/llprocess.h @@ -0,0 +1,106 @@ +/** + * @file llprocess.h + * @brief Utility class for launching, terminating, and tracking child 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$ + */ + +#ifndef LL_LLPROCESS_H +#define LL_LLPROCESS_H + +#include +#include + +#if LL_WINDOWS +#define WIN32_LEAN_AND_MEAN +#include +#endif + +class LLSD; + +class LLProcess; +/// LLProcess instances are created on the heap by static factory methods and +/// managed by ref-counted pointers. +typedef boost::shared_ptr LLProcessPtr; + +/** + * LLProcess handles launching external processes with specified command line arguments. + * It also keeps track of whether the process is still running, and can kill it if required. +*/ +class LL_COMMON_API LLProcess: public boost::noncopyable +{ + LOG_CLASS(LLProcess); +public: + + /** + * Factory accepting LLSD::Map. + * MAY RETURN DEFAULT-CONSTRUCTED LLProcessPtr if params invalid! + * + * executable (required, string): executable pathname + * args (optional, string array): extra command-line arguments + * cwd (optional, string, dft no chdir): change to this directory before executing + * autokill (optional, bool, dft true): implicit kill() on ~LLProcess + */ + static LLProcessPtr create(const LLSD& params); + virtual ~LLProcess(); + + // isRunning isn't const because, if child isn't running, it clears stored + // process ID + bool isRunning(void); + + // 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(void); + +#if LL_WINDOWS + typedef HANDLE id; +#else + typedef pid_t id; +#endif + /// Get platform-specific process ID + id getProcessID() const { return mProcessID; }; + + /** + * Test if a process (id obtained from getProcessID()) is still + * running. Return is same nonzero id value if still running, else + * zero, so you can test it like a bool. But if you want to update a + * stored variable as a side effect, you can write code like this: + * @code + * childpid = LLProcess::isRunning(childpid); + * @endcode + * @note This method is intended as a unit-test hook, not as the first of + * a whole set of operations supported on freestanding @c id values. New + * functionality should be added as nonstatic members operating on + * mProcessID. + */ + static id isRunning(id); + +private: + /// constructor is private: use create() instead + LLProcess(const LLSD& params); + void launch(const LLSD& params); + + id mProcessID; + bool mAutokill; +}; + +#endif // LL_LLPROCESS_H -- cgit v1.2.3 From 6e214960ce203d1d50d7bd6bd04eedee3afd0fa3 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 20 Jan 2012 20:19:50 -0500 Subject: Define LLProcess::Params; accept create(const LLSDParamAdapter&). This allows callers to pass either LLSD formatted as before -- which all callers still do -- or an actual LLProcess::Params block. --- indra/llcommon/llprocess.h | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 9a74cfe829..9ea129baf2 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -27,6 +27,8 @@ #ifndef LL_LLPROCESS_H #define LL_LLPROCESS_H +#include "llinitparam.h" +#include "llsdparam.h" #include #include @@ -35,8 +37,6 @@ #include #endif -class LLSD; - class LLProcess; /// LLProcess instances are created on the heap by static factory methods and /// managed by ref-counted pointers. @@ -50,17 +50,38 @@ class LL_COMMON_API LLProcess: public boost::noncopyable { LOG_CLASS(LLProcess); public: + /// Param block definition + struct Params: public LLInitParam::Block + { + Params(): + executable("executable"), + args("args"), + cwd("cwd"), + autokill("autokill", true) + {} + + /// pathname of executable + Mandatory executable; + /// zero or more additional command-line arguments + Multiple args; + /// current working directory, if need it changed + Optional cwd; + /// implicitly kill process on destruction of LLProcess object + Optional autokill; + }; /** - * Factory accepting LLSD::Map. + * Factory accepting either plain LLSD::Map or Params block. * MAY RETURN DEFAULT-CONSTRUCTED LLProcessPtr if params invalid! * + * Redundant with Params definition above? + * * executable (required, string): executable pathname * args (optional, string array): extra command-line arguments * cwd (optional, string, dft no chdir): change to this directory before executing * autokill (optional, bool, dft true): implicit kill() on ~LLProcess */ - static LLProcessPtr create(const LLSD& params); + static LLProcessPtr create(const LLSDParamAdapter& params); virtual ~LLProcess(); // isRunning isn't const because, if child isn't running, it clears stored @@ -96,8 +117,8 @@ public: private: /// constructor is private: use create() instead - LLProcess(const LLSD& params); - void launch(const LLSD& params); + LLProcess(const LLSDParamAdapter& params); + void launch(const LLSDParamAdapter& params); id mProcessID; bool mAutokill; -- cgit v1.2.3 From aa1bbe3277842a9a6e7db5227b35f1fbea50b7a6 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sun, 22 Jan 2012 10:58:16 -0500 Subject: Make LLProcess::Params streamable; use that in LLExternalEditor. --- indra/llcommon/llprocess.h | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 9ea129baf2..7dbdf23679 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -31,6 +31,7 @@ #include "llsdparam.h" #include #include +#include // std::ostream #if LL_WINDOWS #define WIN32_LEAN_AND_MEAN @@ -124,4 +125,7 @@ private: bool mAutokill; }; +/// for logging +LL_COMMON_API std::ostream& operator<<(std::ostream&, const LLProcess::Params&); + #endif // LL_LLPROCESS_H -- cgit v1.2.3 From 748d1b311fdecf123df40bd7d22dd7e19afaca84 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed 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.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 7dbdf23679..019c33592c 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -97,7 +97,7 @@ public: typedef HANDLE id; #else typedef pid_t id; -#endif +#endif /// Get platform-specific process ID id getProcessID() const { return mProcessID; }; @@ -114,13 +114,14 @@ public: * functionality should be added as nonstatic members operating on * mProcessID. */ - static id isRunning(id); - + static id isRunning(id, const std::string& desc=""); + private: /// constructor is private: use create() instead LLProcess(const LLSDParamAdapter& params); void launch(const LLSDParamAdapter& params); + std::string mDesc; id mProcessID; bool mAutokill; }; -- cgit v1.2.3 From 61e98256df80822f9504a2037d5cbb029c39506d Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 23 Jan 2012 17:38:06 -0500 Subject: Clarify that items in LLProcess::Params::args are implicitly quoted. That is, we try to pass through each args entry as a separate child-process arvg[] entry, whitespace and all. --- indra/llcommon/llprocess.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 019c33592c..51c42582ea 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -63,7 +63,16 @@ public: /// pathname of executable Mandatory executable; - /// zero or more additional command-line arguments + /** + * zero or more additional command-line arguments. Arguments are + * passed through as exactly as we can manage, whitespace and all. + * @note On Windows we manage this by implicitly double-quoting each + * argument while assembling the command line. BUT if a given argument + * is already double-quoted, we don't double-quote it again. Try to + * avoid making use of this, though, as on Mac and Linux explicitly + * double-quoted args will be passed to the child process including + * the double quotes. + */ Multiple args; /// current working directory, if need it changed Optional cwd; -- cgit v1.2.3 From 85581eefa63d8f8e8c5132c4cd7e137f6cb88869 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed 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.h | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 51c42582ea..8a842589ec 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -35,7 +35,7 @@ #if LL_WINDOWS #define WIN32_LEAN_AND_MEAN -#include +#include // HANDLE (eye roll) #endif class LLProcess; @@ -79,6 +79,7 @@ public: /// implicitly kill process on destruction of LLProcess object Optional autokill; }; + typedef LLSDParamAdapter LLSDOrParams; /** * Factory accepting either plain LLSD::Map or Params block. @@ -91,7 +92,7 @@ public: * cwd (optional, string, dft no chdir): change to this directory before executing * autokill (optional, bool, dft true): implicit kill() on ~LLProcess */ - static LLProcessPtr create(const LLSDParamAdapter& params); + static LLProcessPtr create(const LLSDOrParams& params); virtual ~LLProcess(); // isRunning isn't const because, if child isn't running, it clears stored @@ -103,35 +104,46 @@ public: bool kill(void); #if LL_WINDOWS - typedef HANDLE id; + typedef int id; ///< as returned by getProcessID() + typedef HANDLE handle; ///< as returned by getProcessHandle() #else - typedef pid_t id; + typedef pid_t id; + typedef pid_t handle; #endif - /// Get platform-specific process ID - id getProcessID() const { return mProcessID; }; + /** + * Get an int-like id value. This is primarily intended for a human reader + * to differentiate processes. + */ + id getProcessID() const; + /** + * Get a "handle" of a kind that you might pass to platform-specific API + * functions to engage features not directly supported by LLProcess. + */ + handle getProcessHandle() const; /** - * Test if a process (id obtained from getProcessID()) is still - * running. Return is same nonzero id value if still running, else + * Test if a process (@c handle obtained from getProcessHandle()) is still + * running. Return same nonzero @c handle value if still running, else * zero, so you can test it like a bool. But if you want to update a * stored variable as a side effect, you can write code like this: * @code - * childpid = LLProcess::isRunning(childpid); + * hchild = LLProcess::isRunning(hchild); * @endcode * @note This method is intended as a unit-test hook, not as the first of - * a whole set of operations supported on freestanding @c id values. New - * functionality should be added as nonstatic members operating on - * mProcessID. + * a whole set of operations supported on freestanding @c handle values. + * New functionality should be added as nonstatic members operating on + * the same data as getProcessHandle(). */ - static id isRunning(id, const std::string& desc=""); + static handle isRunning(handle, const std::string& desc=""); private: /// constructor is private: use create() instead - LLProcess(const LLSDParamAdapter& params); - void launch(const LLSDParamAdapter& params); + LLProcess(const LLSDOrParams& params); + void launch(const LLSDOrParams& params); std::string mDesc; id mProcessID; + handle mProcessHandle; bool mAutokill; }; -- cgit v1.2.3 From aafb03b29f5166e8978931ad8b717be32d942836 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed 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.h | 64 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 57 insertions(+), 7 deletions(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 8a842589ec..689f8aedab 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -29,6 +29,7 @@ #include "llinitparam.h" #include "llsdparam.h" +#include "apr_thread_proc.h" #include #include #include // std::ostream @@ -95,13 +96,52 @@ public: static LLProcessPtr create(const LLSDOrParams& params); virtual ~LLProcess(); - // isRunning isn't const because, if child isn't running, it clears stored - // process ID + // isRunning() isn't const because, when child terminates, it sets stored + // Status bool isRunning(void); - + + /** + * State of child process + */ + enum state + { + UNSTARTED, ///< initial value, invisible to consumer + RUNNING, ///< child process launched + EXITED, ///< child process terminated voluntarily + KILLED ///< child process terminated involuntarily + }; + + /** + * Status info + */ + struct Status + { + Status(): + mState(UNSTARTED), + mData(0) + {} + + state mState; ///< @see state + /** + * - for mState == EXITED: mData is exit() code + * - for mState == KILLED: mData is signal number (Posix) + * - otherwise: mData is undefined + */ + int mData; + }; + + /// Status query + Status getStatus(); + /// English Status string query, for logging etc. + std::string getStatusString(); + /// English Status string query for previously-captured Status + std::string getStatusString(const Status& status); + /// static English Status string query + static std::string getStatusString(const std::string& desc, const Status& status); + // 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(void); + bool kill(const std::string& who=""); #if LL_WINDOWS typedef int id; ///< as returned by getProcessID() @@ -133,18 +173,28 @@ public: * a whole set of operations supported on freestanding @c handle values. * 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() + * 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 + * launching LLProcess object has been destroyed. */ static handle isRunning(handle, const std::string& desc=""); private: /// constructor is private: use create() instead LLProcess(const LLSDOrParams& params); - void launch(const LLSDOrParams& params); + void autokill(); + // Classic-C-style APR callback + static void status_callback(int reason, void* data, int status); + // Object-oriented callback + void handle_status(int reason, int status); std::string mDesc; - id mProcessID; - handle mProcessHandle; + apr_proc_t mProcess; bool mAutokill; + Status mStatus; }; /// for logging -- cgit v1.2.3 From 5d2bb536324a906078b224bdc9697b368e96a6b6 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 7 Feb 2012 12:06:03 -0500 Subject: On Linux, #undef Status: we use that name for nested LLProcess struct. Apparently something in the Linux system header chain #defines a macro Status as 'int'. That's just Bad in C++ land. It should at the very least be a typedef! #undefining it in llprocess.h permits the viewer to build. --- indra/llcommon/llprocess.h | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 689f8aedab..0de033c15b 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -37,6 +37,10 @@ #if LL_WINDOWS #define WIN32_LEAN_AND_MEAN #include // HANDLE (eye roll) +#elif LL_LINUX +#if defined(Status) +#undef Status +#endif #endif class LLProcess; -- cgit v1.2.3 From aae61392be822218cabcab91d95eb1e75d471764 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed 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.h | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 0de033c15b..b95ae55701 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -49,9 +49,17 @@ class LLProcess; typedef boost::shared_ptr LLProcessPtr; /** - * LLProcess handles launching external processes with specified command line arguments. - * It also keeps track of whether the process is still running, and can kill it if required. -*/ + * LLProcess handles launching an external process with specified command line + * arguments. It also keeps track of whether the process is still running, and + * can kill it if required. + * + * LLProcess relies on periodic post() calls on the "mainloop" LLEventPump: an + * LLProcess object's Status won't update until the next "mainloop" tick. The + * viewer's main loop already posts to that LLEventPump once per iteration + * (hence the name). See indra/llcommon/tests/llprocess_test.cpp for an + * example of waiting for child-process termination in a standalone test + * context. + */ class LL_COMMON_API LLProcess: public boost::noncopyable { LOG_CLASS(LLProcess); @@ -72,11 +80,7 @@ public: * zero or more additional command-line arguments. Arguments are * passed through as exactly as we can manage, whitespace and all. * @note On Windows we manage this by implicitly double-quoting each - * argument while assembling the command line. BUT if a given argument - * is already double-quoted, we don't double-quote it again. Try to - * avoid making use of this, though, as on Mac and Linux explicitly - * double-quoted args will be passed to the child process including - * the double quotes. + * argument while assembling the command line. */ Multiple args; /// current working directory, if need it changed -- cgit v1.2.3 From e239cad1f509e3d96011acb61614f2481c46af38 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed 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.h | 235 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 227 insertions(+), 8 deletions(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index b95ae55701..448a88f4c0 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -31,8 +31,11 @@ #include "llsdparam.h" #include "apr_thread_proc.h" #include +#include +#include #include #include // std::ostream +#include #if LL_WINDOWS #define WIN32_LEAN_AND_MEAN @@ -43,6 +46,8 @@ #endif #endif +class LLEventPump; + class LLProcess; /// LLProcess instances are created on the heap by static factory methods and /// managed by ref-counted pointers. @@ -64,6 +69,87 @@ class LL_COMMON_API LLProcess: public boost::noncopyable { LOG_CLASS(LLProcess); public: + /** + * Specify what to pass for each of child stdin, stdout, stderr. + * @see LLProcess::Params::files. + */ + struct FileParam: public LLInitParam::Block + { + /** + * type of file handle to pass to child process + * + * - "" (default): let the child inherit the same file handle used by + * this process. For instance, if passed as stdout, child stdout + * will be interleaved with stdout from this process. In this case, + * @a name is moot and should be left "". + * + * - "file": open an OS filesystem file with the specified @a name. + * Not yet implemented. + * + * - "pipe" or "tpipe" or "npipe": depends on @a name + * + * - @a name.empty(): construct an OS pipe used only for this slot + * of the forthcoming child process. + * + * - ! @a name.empty(): in a global registry, find or create (using + * the specified @a name) an OS pipe. The point of the (purely + * internal) @a name is that passing the same @a name in more than + * one slot for a given LLProcess -- or for slots in different + * LLProcess instances -- means the same pipe. For example, you + * might pass the same @a name value as both stdout and stderr to + * make the child process produce both on the same actual pipe. Or + * you might pass the same @a name as the stdout for one LLProcess + * and the stdin for another to connect the two child processes. + * Use LLProcess::getPipeName() to generate a unique name + * guaranteed not to already exist in the registry. Not yet + * implemented. + * + * The difference between "pipe", "tpipe" and "npipe" is as follows. + * + * - "pipe": direct LLProcess to monitor the parent end of the pipe, + * pumping nonblocking I/O every frame. The expectation (at least + * for stdout or stderr) is that the caller will listen for + * incoming data and consume it as it arrives. It's important not + * to neglect such a pipe, because it's buffered in viewer memory. + * If you suspect the child may produce a great volume of output + * between viewer frames, consider directing the child to write to + * a filesystem file instead, then read the file later. + * + * - "tpipe": do not engage LLProcess machinery to monitor the + * parent end of the pipe. A "tpipe" is used only to connect + * different child processes. As such, it makes little sense to + * pass an empty @a name. Not yet implemented. + * + * - "npipe": like "tpipe", but use an OS named pipe with a + * generated name. Note that @a name is the @em internal name of + * the pipe in our global registry -- it doesn't necessarily have + * anything to do with the pipe's name in the OS filesystem. Use + * LLProcess::getPipeName() to obtain the named pipe's OS + * filesystem name, e.g. to pass it as the @a name to another + * LLProcess instance using @a type "file". This supports usage + * like bash's <(subcommand...) or >(subcommand...) + * constructs. Not yet implemented. + * + * In all cases the open mode (read, write) is determined by the child + * slot you're filling. Child stdin means select the "read" end of a + * pipe, or open a filesystem file for reading; child stdout or stderr + * means select the "write" end of a pipe, or open a filesystem file + * for writing. + * + * Confusion such as passing the same pipe as the stdin of two + * processes (rather than stdout for one and stdin for the other) is + * explicitly permitted: it's up to the caller to construct meaningful + * LLProcess pipe graphs. + */ + Optional type; + Optional name; + + FileParam(const std::string& tp="", const std::string& nm=""): + type("type", tp), + name("name", nm) + {} + }; + /// Param block definition struct Params: public LLInitParam::Block { @@ -71,7 +157,8 @@ public: executable("executable"), args("args"), cwd("cwd"), - autokill("autokill", true) + autokill("autokill", true), + files("files") {} /// pathname of executable @@ -87,19 +174,22 @@ public: Optional cwd; /// implicitly kill process on destruction of LLProcess object Optional autokill; + /** + * Up to three FileParam items: for child stdin, stdout, stderr. + * Passing two FileParam entries means default treatment for stderr, + * and so forth. + * + * @note While it's theoretically plausible to pass additional open + * file handles to a child specifically written to expect them, our + * underlying implementation library doesn't support that. + */ + Multiple files; }; typedef LLSDParamAdapter LLSDOrParams; /** * Factory accepting either plain LLSD::Map or Params block. * MAY RETURN DEFAULT-CONSTRUCTED LLProcessPtr if params invalid! - * - * Redundant with Params definition above? - * - * executable (required, string): executable pathname - * args (optional, string array): extra command-line arguments - * cwd (optional, string, dft no chdir): change to this directory before executing - * autokill (optional, bool, dft true): implicit kill() on ~LLProcess */ static LLProcessPtr create(const LLSDOrParams& params); virtual ~LLProcess(); @@ -190,6 +280,125 @@ public: */ static handle isRunning(handle, const std::string& desc=""); + /// Provide symbolic access to child's file slots + enum FILESLOT { STDIN=0, STDOUT=1, STDERR=2, NSLOTS=3 }; + + /** + * For a pipe constructed with @a type "npipe", obtain the generated OS + * filesystem name for the specified pipe. Otherwise returns the empty + * string. @see LLProcess::FileParam::type + */ + std::string getPipeName(FILESLOT); + + /// base of ReadPipe, WritePipe + class BasePipe + { + public: + virtual ~BasePipe() = 0; + }; + + /// As returned by getWritePipe() or getOptWritePipe() + class WritePipe: public BasePipe + { + public: + /** + * Get ostream& on which to write to child's stdin. + * + * @usage + * @code + * myProcess->getWritePipe().get_ostream() << "Hello, child!" << std::endl; + * @endcode + */ + virtual std::ostream& get_ostream() = 0; + }; + + /// As returned by getReadPipe() or getOptReadPipe() + class ReadPipe: public BasePipe + { + public: + /** + * Get istream& on which to read from child's stdout or stderr. + * + * @usage + * @code + * std::string stuff; + * myProcess->getReadPipe().get_istream() >> stuff; + * @endcode + * + * You should be sure in advance that the ReadPipe in question can + * fill the request. @see getPump() + */ + virtual std::istream& get_istream() = 0; + + /** + * Get LLEventPump& on which to listen for incoming data. The posted + * LLSD::Map event will contain a key "data" whose value is an + * LLSD::String containing (part of) the data accumulated in the + * buffer. + * + * If the child sends "abc", and this ReadPipe posts "data"="abc", but + * you don't consume it by reading the std::istream returned by + * get_istream(), and the child next sends "def", ReadPipe will post + * "data"="abcdef". + */ + virtual LLEventPump& getPump() = 0; + + /** + * Set maximum length of buffer data that will be posted in the LLSD + * announcing arrival of new data from the child. If you call + * setLimit(5), and the child sends "abcdef", the LLSD event will + * contain "data"="abcde". However, you may still read the entire + * "abcdef" from get_istream(): this limit affects only the size of + * the data posted with the LLSD event. If you don't call this method, + * all pending data will be posted. + */ + virtual void setLimit(size_t limit) = 0; + + /** + * Query the current setLimit() limit. + */ + virtual size_t getLimit() const = 0; + }; + + /// Exception thrown by getWritePipe(), getReadPipe() if you didn't ask to + /// create a pipe at the corresponding FILESLOT. + struct NoPipe: public std::runtime_error + { + NoPipe(const std::string& what): std::runtime_error(what) {} + }; + + /** + * Get a reference to the (only) WritePipe for this LLProcess. @a slot, if + * specified, must be STDIN. Throws NoPipe if you did not request a "pipe" + * for child stdin. Use this method when you know how you created the + * LLProcess in hand. + */ + WritePipe& getWritePipe(FILESLOT slot=STDIN); + + /** + * Get a boost::optional to the (only) WritePipe for this + * LLProcess. @a slot, if specified, must be STDIN. The return value is + * empty if you did not request a "pipe" for child stdin. Use this method + * for inspecting an LLProcess you did not create. + */ + boost::optional getOptWritePipe(FILESLOT slot=STDIN); + + /** + * Get a reference to one of the ReadPipes for this LLProcess. @a slot, if + * specified, must be STDOUT or STDERR. Throws NoPipe if you did not + * request a "pipe" for child stdout or stderr. Use this method when you + * know how you created the LLProcess in hand. + */ + ReadPipe& getReadPipe(FILESLOT slot); + + /** + * Get a boost::optional to one of the ReadPipes for this + * LLProcess. @a slot, if specified, must be STDOUT or STDERR. The return + * value is empty if you did not request a "pipe" for child stdout or + * stderr. Use this method for inspecting an LLProcess you did not create. + */ + boost::optional getOptReadPipe(FILESLOT slot); + private: /// constructor is private: use create() instead LLProcess(const LLSDOrParams& params); @@ -198,11 +407,21 @@ private: static void status_callback(int reason, void* data, int status); // Object-oriented callback void handle_status(int reason, int status); + // implementation for get[Opt][Read|Write]Pipe() + template + PIPETYPE& getPipe(FILESLOT slot); + template + boost::optional getOptPipe(FILESLOT slot); + template + PIPETYPE* getPipePtr(std::string& error, FILESLOT slot); std::string mDesc; apr_proc_t mProcess; bool mAutokill; Status mStatus; + // explicitly want this ptr_vector to be able to store NULLs + typedef boost::ptr_vector< boost::nullable > PipeVector; + PipeVector mPipes; }; /// for logging -- cgit v1.2.3 From 56d931216e67a3e59199669bba022c65a9617bb5 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed 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.h | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 448a88f4c0..bf0517600d 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -330,6 +330,31 @@ public: */ virtual std::istream& get_istream() = 0; + /** + * Get accumulated buffer length. + * Often we need to refrain from actually reading the std::istream + * returned by get_istream() until we've accumulated enough data to + * make it worthwhile. For instance, if we're expecting a number from + * the child, but the child happens to flush "12" before emitting + * "3\n", get_istream() >> myint could return 12 rather than 123! + */ + virtual std::size_t size() = 0; + + /** + * Peek at accumulated buffer data without consuming it. Optional + * parameters give you substr() functionality. + * + * @note You can discard buffer data using get_istream().ignore(n). + */ + virtual std::string peek(std::size_t offset=0, + std::size_t len=(std::numeric_limits::max)()) = 0; + + /** + * Search accumulated buffer data without retrieving it. Optional + * offset allows you to start at specified position. + */ + virtual bool contains(const std::string& seek, std::size_t offset=0) = 0; + /** * Get LLEventPump& on which to listen for incoming data. The posted * LLSD::Map event will contain a key "data" whose value is an -- cgit v1.2.3 From e92c3113545dd60fb76e115da201163e340c730c Mon Sep 17 00:00:00 2001 From: Nat Goodspeed 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.h | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index bf0517600d..2c6951b562 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -295,6 +295,9 @@ public: { public: virtual ~BasePipe() = 0; + + typedef std::size_t size_type; + static const size_type npos; }; /// As returned by getWritePipe() or getOptWritePipe() @@ -338,7 +341,7 @@ public: * the child, but the child happens to flush "12" before emitting * "3\n", get_istream() >> myint could return 12 rather than 123! */ - virtual std::size_t size() = 0; + virtual size_type size() const = 0; /** * Peek at accumulated buffer data without consuming it. Optional @@ -346,14 +349,32 @@ public: * * @note You can discard buffer data using get_istream().ignore(n). */ - virtual std::string peek(std::size_t offset=0, - std::size_t len=(std::numeric_limits::max)()) = 0; + virtual std::string peek(size_type offset=0, size_type len=npos) const = 0; + + /** + * Detect presence of a substring (or char) in accumulated buffer data + * without retrieving it. Optional offset allows you to search from + * specified position. + */ + template + bool contains(SEEK seek, size_type offset=0) const + { return find(seek, offset) != npos; } + + /** + * Search for a substring in accumulated buffer data without + * retrieving it. Returns size_type position at which found, or npos + * meaning not found. Optional offset allows you to search from + * specified position. + */ + virtual size_type find(const std::string& seek, size_type offset=0) const = 0; /** - * Search accumulated buffer data without retrieving it. Optional - * offset allows you to start at specified position. + * Search for a char in accumulated buffer data without retrieving it. + * Returns size_type position at which found, or npos meaning not + * found. Optional offset allows you to search from specified + * position. */ - virtual bool contains(const std::string& seek, std::size_t offset=0) = 0; + virtual size_type find(char seek, size_type offset=0) const = 0; /** * Get LLEventPump& on which to listen for incoming data. The posted @@ -377,12 +398,12 @@ public: * the data posted with the LLSD event. If you don't call this method, * all pending data will be posted. */ - virtual void setLimit(size_t limit) = 0; + virtual void setLimit(size_type limit) = 0; /** * Query the current setLimit() limit. */ - virtual size_t getLimit() const = 0; + virtual size_type getLimit() const = 0; }; /// Exception thrown by getWritePipe(), getReadPipe() if you didn't ask to -- cgit v1.2.3 From d6ed77a598a0009d386ce3cd6c49d0f1c4b422e8 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 16 Feb 2012 17:39:56 -0500 Subject: Attempt to fix Windows link error for LLProcess::BasePipe::npos. --- indra/llcommon/llprocess.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 2c6951b562..06be0954c0 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -291,7 +291,7 @@ public: std::string getPipeName(FILESLOT); /// base of ReadPipe, WritePipe - class BasePipe + class LL_COMMON_API BasePipe { public: virtual ~BasePipe() = 0; -- cgit v1.2.3 From 8b5d5f9652499103b966524e1c0ceef869e29eeb Mon Sep 17 00:00:00 2001 From: Nat Goodspeed 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.h | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 06be0954c0..96a3dce5b3 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -158,7 +158,8 @@ public: args("args"), cwd("cwd"), autokill("autokill", true), - files("files") + files("files"), + postend("postend") {} /// pathname of executable @@ -184,6 +185,20 @@ public: * underlying implementation library doesn't support that. */ Multiple files; + /** + * On child-process termination, if this LLProcess object still + * exists, post LLSD event to LLEventPump with specified name (default + * no event). Event contains at least: + * + * - "id" as obtained from getProcessID() + * - "desc" short string description of child (executable + pid) + * - "state" @c state enum value, from Status.mState + * - "data" if "state" is EXITED, exit code; if KILLED, on Posix, + * signal number + * - "string" English text describing "state" and "data" (e.g. "exited + * with code 0") + */ + Optional postend; }; typedef LLSDParamAdapter LLSDOrParams; @@ -462,6 +477,7 @@ private: PIPETYPE* getPipePtr(std::string& error, FILESLOT slot); std::string mDesc; + std::string mPostend; apr_proc_t mProcess; bool mAutokill; Status mStatus; -- cgit v1.2.3 From 999484a60896b11df1af9a44e58ccae6fa6ecbed Mon Sep 17 00:00:00 2001 From: Nat Goodspeed 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.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 96a3dce5b3..d005847e18 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -159,7 +159,8 @@ public: cwd("cwd"), autokill("autokill", true), files("files"), - postend("postend") + postend("postend"), + desc("desc") {} /// pathname of executable @@ -199,6 +200,13 @@ public: * with code 0") */ Optional postend; + /** + * Description of child process for logging purposes. It need not be + * unique; the logged description string will contain the PID as well. + * If this is omitted, a description will be derived from the + * executable name. + */ + Optional desc; }; typedef LLSDParamAdapter LLSDOrParams; -- cgit v1.2.3 From 14ddc6474a0ae83db8d034b00138289fb15e41b7 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed 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.h | 63 ++++++++++++++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 22 deletions(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index d005847e18..637b7e2f9c 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -58,12 +58,16 @@ typedef boost::shared_ptr LLProcessPtr; * arguments. It also keeps track of whether the process is still running, and * can kill it if required. * + * In discussing LLProcess, we use the term "parent" to refer to this process + * (the process invoking LLProcess), versus "child" to refer to the process + * spawned by LLProcess. + * * LLProcess relies on periodic post() calls on the "mainloop" LLEventPump: an - * LLProcess object's Status won't update until the next "mainloop" tick. The - * viewer's main loop already posts to that LLEventPump once per iteration - * (hence the name). See indra/llcommon/tests/llprocess_test.cpp for an - * example of waiting for child-process termination in a standalone test - * context. + * LLProcess object's Status won't update until the next "mainloop" tick. For + * instance, the Second Life viewer's main loop already posts to an + * LLEventPump by that name once per iteration. See + * indra/llcommon/tests/llprocess_test.cpp for an example of waiting for + * child-process termination in a standalone test context. */ class LL_COMMON_API LLProcess: public boost::noncopyable { @@ -110,10 +114,10 @@ public: * pumping nonblocking I/O every frame. The expectation (at least * for stdout or stderr) is that the caller will listen for * incoming data and consume it as it arrives. It's important not - * to neglect such a pipe, because it's buffered in viewer memory. - * If you suspect the child may produce a great volume of output - * between viewer frames, consider directing the child to write to - * a filesystem file instead, then read the file later. + * to neglect such a pipe, because it's buffered in memory. If you + * suspect the child may produce a great volume of output between + * frames, consider directing the child to write to a filesystem + * file instead, then read the file later. * * - "tpipe": do not engage LLProcess machinery to monitor the * parent end of the pipe. A "tpipe" is used only to connect @@ -145,9 +149,14 @@ public: Optional name; FileParam(const std::string& tp="", const std::string& nm=""): - type("type", tp), - name("name", nm) - {} + type("type"), + name("name") + { + // If caller wants to specify values, use explicit assignment to + // set them rather than initialization. + if (! tp.empty()) type = tp; + if (! nm.empty()) name = nm; + } }; /// Param block definition @@ -175,17 +184,28 @@ public: /// current working directory, if need it changed Optional cwd; /// implicitly kill process on destruction of LLProcess object + /// (default true) Optional autokill; /** * Up to three FileParam items: for child stdin, stdout, stderr. * Passing two FileParam entries means default treatment for stderr, * and so forth. * + * @note LLInitParam::Block permits usage like this: + * @code + * LLProcess::Params params; + * ... + * params.files + * .add(LLProcess::FileParam()) // stdin + * .add(LLProcess::FileParam().type("pipe") // stdout + * .add(LLProcess::FileParam().type("file").name("error.log")); + * @endcode + * * @note While it's theoretically plausible to pass additional open * file handles to a child specifically written to expect them, our - * underlying implementation library doesn't support that. + * underlying implementation doesn't yet support that. */ - Multiple files; + Multiple > files; /** * On child-process termination, if this LLProcess object still * exists, post LLSD event to LLEventPump with specified name (default @@ -217,9 +237,8 @@ public: static LLProcessPtr create(const LLSDOrParams& params); virtual ~LLProcess(); - // isRunning() isn't const because, when child terminates, it sets stored - // Status - bool isRunning(void); + /// Is child process still running? + bool isRunning() const; /** * State of child process @@ -252,11 +271,11 @@ public: }; /// Status query - Status getStatus(); + Status getStatus() const; /// English Status string query, for logging etc. - std::string getStatusString(); + std::string getStatusString() const; /// English Status string query for previously-captured Status - std::string getStatusString(const Status& status); + std::string getStatusString(const Status& status) const; /// static English Status string query static std::string getStatusString(const std::string& desc, const Status& status); @@ -311,7 +330,7 @@ public: * filesystem name for the specified pipe. Otherwise returns the empty * string. @see LLProcess::FileParam::type */ - std::string getPipeName(FILESLOT); + std::string getPipeName(FILESLOT) const; /// base of ReadPipe, WritePipe class LL_COMMON_API BasePipe @@ -419,7 +438,7 @@ public: * contain "data"="abcde". However, you may still read the entire * "abcdef" from get_istream(): this limit affects only the size of * the data posted with the LLSD event. If you don't call this method, - * all pending data will be posted. + * @em no data will be posted: the default is 0 bytes. */ virtual void setLimit(size_type limit) = 0; -- cgit v1.2.3 From 3649eda62ad3a04203e6c562e78815a95896bbd4 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed 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.h | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 637b7e2f9c..06ada83698 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -156,7 +156,7 @@ public: // set them rather than initialization. if (! tp.empty()) type = tp; if (! nm.empty()) name = nm; - } + } }; /// Param block definition @@ -375,6 +375,18 @@ public: */ virtual std::istream& get_istream() = 0; + /** + * Like std::getline(get_istream(), line), but trims off trailing '\r' + * to make calling code less platform-sensitive. + */ + virtual std::string getline() = 0; + + /** + * Like get_istream().read(buffer, n), but returns std::string rather + * than requiring caller to construct a buffer, etc. + */ + virtual std::string read(size_type len) = 0; + /** * Get accumulated buffer length. * Often we need to refrain from actually reading the std::istream @@ -420,9 +432,15 @@ public: /** * Get LLEventPump& on which to listen for incoming data. The posted - * LLSD::Map event will contain a key "data" whose value is an - * LLSD::String containing (part of) the data accumulated in the - * buffer. + * LLSD::Map event will contain: + * + * - "data" part of pending data; see setLimit() + * - "len" entire length of pending data, regardless of setLimit() + * - "slot" this ReadPipe's FILESLOT, e.g. LLProcess::STDOUT + * - "name" e.g. "stdout" + * - "desc" e.g. "SLPlugin (pid) stdout" + * - "eof" @c true means there no more data will arrive on this pipe, + * therefore no more events on this pump * * If the child sends "abc", and this ReadPipe posts "data"="abc", but * you don't consume it by reading the std::istream returned by @@ -487,6 +505,11 @@ public: */ boost::optional getOptReadPipe(FILESLOT slot); + /// little utilities that really should already be somewhere else in the + /// code base + static std::string basename(const std::string& path); + static std::string getline(std::istream&); + private: /// constructor is private: use create() instead LLProcess(const LLSDOrParams& params); -- cgit v1.2.3 From 4edf34ed01611d75bdcd98aa065a2b286845ebd9 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed 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.h | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 06ada83698..51010966f9 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -340,6 +340,20 @@ public: typedef std::size_t size_type; static const size_type npos; + + /** + * Get accumulated buffer length. + * + * For WritePipe, is there still pending data to send to child? + * + * For ReadPipe, we often need to refrain from actually reading the + * std::istream returned by get_istream() until we've accumulated + * enough data to make it worthwhile. For instance, if we're expecting + * a number from the child, but the child happens to flush "12" before + * emitting "3\n", get_istream() >> myint could return 12 rather than + * 123! + */ + virtual size_type size() const = 0; }; /// As returned by getWritePipe() or getOptWritePipe() @@ -387,16 +401,6 @@ public: */ virtual std::string read(size_type len) = 0; - /** - * Get accumulated buffer length. - * Often we need to refrain from actually reading the std::istream - * returned by get_istream() until we've accumulated enough data to - * make it worthwhile. For instance, if we're expecting a number from - * the child, but the child happens to flush "12" before emitting - * "3\n", get_istream() >> myint could return 12 rather than 123! - */ - virtual size_type size() const = 0; - /** * Peek at accumulated buffer data without consuming it. Optional * parameters give you substr() functionality. -- cgit v1.2.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.h | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) (limited to 'indra/llcommon/llprocess.h') 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=""); -- cgit v1.2.3