[LLVMdev] [PATCH 2/2] Make Program::ExecuteNoWait return a process ID.
Daniel Dunbar
daniel at zuster.org
Thu Jul 16 19:27:37 PDT 2009
I don't think this is the right direction for the system library to
go. The System library is supposed to expose generic OS independent
interfaces, not be a generic way for clients to get OS information.
Ultimately I think a better API would be to provide a generic class
which represents an executed operating system process, and includes
operations to wait for its completion, redirect its IO, communicate
with it, etc. This would be a big improvement over the current
monolithic function.
That said, designing and implementing that is more work. However, I'd
rather either see that done, or clients do their own process stuff,
than have the llvm::sys API expose operating system dependent
information.
- Daniel
On Thu, Jul 16, 2009 at 6:04 PM, Mikhail Glushenkov<foldr at codedgers.com> wrote:
> ---
> include/llvm/System/Program.h | 14 ++++++++++----
> lib/System/Unix/Program.inc | 17 +++++++++--------
> lib/System/Win32/Program.inc | 16 +++++++++-------
> 3 files changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/include/llvm/System/Program.h b/include/llvm/System/Program.h
> index 14f9e9e..05c73ac 100644
> --- a/include/llvm/System/Program.h
> +++ b/include/llvm/System/Program.h
> @@ -15,6 +15,7 @@
> #define LLVM_SYSTEM_PROGRAM_H
>
> #include "llvm/System/Path.h"
> +#include "llvm/Config/config.h"
>
> namespace llvm {
> namespace sys {
> @@ -88,6 +89,12 @@ namespace sys {
> static bool ChangeStdoutToBinary();
> /// @}
>
> +#ifdef LLVM_ON_WINDOWS
> + typedef unsigned ProcessID;
> +#else
> + typedef int ProcessID;
> +#endif
> +
> /// This function executes the program using the \p arguments provided and
> /// waits for the program to exit. This function will block the current
> /// program until the invoked program exits. The invoked program will
> @@ -95,12 +102,11 @@ namespace sys {
> /// environment and other configuration settings of the invoking program.
> /// If Path::executable() does not return true when this function is
> /// called then a std::string is thrown.
> - /// @returns an integer result code indicating the status of the program.
> - /// A zero or positive value indicates the result code of the program. A
> - /// negative value is the signal number on which it terminated.
> + /// @returns the child process's process ID on success, or 0 if the
> + /// invocation failed.
> /// @see FindProgrambyName
> /// @brief Executes the program with the given set of \p args.
> - static void ExecuteNoWait(
> + static ProcessID ExecuteNoWait(
> const Path& path, ///< sys::Path object providing the path of the
> ///< program to be executed. It is presumed this is the result of
> ///< the FindProgramByName method.
> diff --git a/lib/System/Unix/Program.inc b/lib/System/Unix/Program.inc
> index 342b45c..15f6e3f 100644
> --- a/lib/System/Unix/Program.inc
> +++ b/lib/System/Unix/Program.inc
> @@ -274,7 +274,7 @@ Program::ExecuteAndWait(const Path& path,
>
> }
>
> -void
> +Program::ProcessID
> Program::ExecuteNoWait(const Path& path,
> const char** args,
> const char** envp,
> @@ -285,36 +285,36 @@ Program::ExecuteNoWait(const Path& path,
> if (!path.canExecute()) {
> if (ErrMsg)
> *ErrMsg = path.toString() + " is not executable";
> - return;
> + return 0;
> }
>
> // Create a child process.
> - int child = fork();
> + ProcessID child = fork();
> switch (child) {
> // An error occured: Return to the caller.
> case -1:
> MakeErrMsg(ErrMsg, "Couldn't fork");
> - return;
> + return 0;
>
> // Child process: Execute the program.
> case 0: {
> // Redirect file descriptors...
> if (redirects) {
> // Redirect stdin
> - if (RedirectIO(redirects[0], 0, ErrMsg)) { return; }
> + if (RedirectIO(redirects[0], 0, ErrMsg)) { return 0; }
> // Redirect stdout
> - if (RedirectIO(redirects[1], 1, ErrMsg)) { return; }
> + if (RedirectIO(redirects[1], 1, ErrMsg)) { return 0; }
> if (redirects[1] && redirects[2] &&
> *(redirects[1]) == *(redirects[2])) {
> // If stdout and stderr should go to the same place, redirect stderr
> // to the FD already open for stdout.
> if (-1 == dup2(1,2)) {
> MakeErrMsg(ErrMsg, "Can't redirect stderr to stdout");
> - return;
> + return 0;
> }
> } else {
> // Just redirect stderr
> - if (RedirectIO(redirects[2], 2, ErrMsg)) { return; }
> + if (RedirectIO(redirects[2], 2, ErrMsg)) { return 0; }
> }
> }
>
> @@ -344,6 +344,7 @@ Program::ExecuteNoWait(const Path& path,
> fsync(1);
> fsync(2);
>
> + return child;
> }
>
> bool Program::ChangeStdinToBinary(){
> diff --git a/lib/System/Win32/Program.inc b/lib/System/Win32/Program.inc
> index 71f686c..4577737 100644
> --- a/lib/System/Win32/Program.inc
> +++ b/lib/System/Win32/Program.inc
> @@ -303,7 +303,7 @@ Program::ExecuteAndWait(const Path& path,
> return status;
> }
>
> -void
> +Program::ProcessID
> Program::ExecuteNoWait(const Path& path,
> const char** args,
> const char** envp,
> @@ -313,7 +313,7 @@ Program::ExecuteNoWait(const Path& path,
> if (!path.canExecute()) {
> if (ErrMsg)
> *ErrMsg = "program not executable";
> - return;
> + return 0;
> }
>
> // Windows wants a command line, not an array of args, to pass to the new
> @@ -388,13 +388,13 @@ Program::ExecuteNoWait(const Path& path,
> si.hStdInput = RedirectIO(redirects[0], 0, ErrMsg);
> if (si.hStdInput == INVALID_HANDLE_VALUE) {
> MakeErrMsg(ErrMsg, "can't redirect stdin");
> - return;
> + return 0;
> }
> si.hStdOutput = RedirectIO(redirects[1], 1, ErrMsg);
> if (si.hStdOutput == INVALID_HANDLE_VALUE) {
> CloseHandle(si.hStdInput);
> MakeErrMsg(ErrMsg, "can't redirect stdout");
> - return;
> + return 0;
> }
> if (redirects[1] && redirects[2] && *(redirects[1]) == *(redirects[2])) {
> // If stdout and stderr should go to the same place, redirect stderr
> @@ -409,7 +409,7 @@ Program::ExecuteNoWait(const Path& path,
> CloseHandle(si.hStdInput);
> CloseHandle(si.hStdOutput);
> MakeErrMsg(ErrMsg, "can't redirect stderr");
> - return;
> + return 0;
> }
> }
> }
> @@ -435,7 +435,7 @@ Program::ExecuteNoWait(const Path& path,
> SetLastError(err);
> MakeErrMsg(ErrMsg, std::string("Couldn't execute program '") +
> path.toString() + "'");
> - return;
> + return 0;
> }
>
> // Make sure these get closed no matter what.
> @@ -463,9 +463,11 @@ Program::ExecuteNoWait(const Path& path,
> MakeErrMsg(ErrMsg, std::string("Unable to set memory limit"));
> TerminateProcess(pi.hProcess, 1);
> WaitForSingleObject(pi.hProcess, INFINITE);
> - return;
> + return 0;
> }
> }
> +
> + return pi.dwProcessId;
> }
>
> bool Program::ChangeStdinToBinary(){
> --
> 1.6.3.3
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
More information about the llvm-dev
mailing list