You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by jo...@apache.org on 2017/07/11 01:39:21 UTC
[35/50] mesos git commit: Windows: Replaced `CreateProcess()` with
`create_process()` helper.
Windows: Replaced `CreateProcess()` with `create_process()` helper.
This uses the new `ProcessData create_process()` abstraction added to
stout, reducing the use of the Windows API in libprocess. The
`ProcessData` struct eliminates the need to manually call
`CloseHandle()`.
Review: https://reviews.apache.org/r/60329/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/4b28a28c
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/4b28a28c
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/4b28a28c
Branch: refs/heads/master
Commit: 4b28a28c3a56ea96873df7414216a47702a81b61
Parents: c200bec
Author: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
Authored: Mon Jul 10 14:30:16 2017 -0700
Committer: Joseph Wu <jo...@apache.org>
Committed: Mon Jul 10 17:15:37 2017 -0700
----------------------------------------------------------------------
.../libprocess/cmake/ProcessConfigure.cmake | 7 -
.../include/process/subprocess_base.hpp | 7 +-
.../include/process/windows/subprocess.hpp | 197 ++-----------------
3rdparty/libprocess/src/subprocess.cpp | 29 +--
4 files changed, 36 insertions(+), 204 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/4b28a28c/3rdparty/libprocess/cmake/ProcessConfigure.cmake
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/cmake/ProcessConfigure.cmake b/3rdparty/libprocess/cmake/ProcessConfigure.cmake
index 2c46d43..dd3be57 100755
--- a/3rdparty/libprocess/cmake/ProcessConfigure.cmake
+++ b/3rdparty/libprocess/cmake/ProcessConfigure.cmake
@@ -166,13 +166,6 @@ set(PROCESS_LIBS
${HTTP_PARSER_LFLAG}
)
-if (WIN32)
- set(PROCESS_LIBS
- ${PROCESS_LIBS}
- userenv
- )
-endif ()
-
if (NOT ENABLE_LIBEVENT)
set(PROCESS_LIBS ${PROCESS_LIBS} ${LIBEV_LFLAG})
else ()
http://git-wip-us.apache.org/repos/asf/mesos/blob/4b28a28c/3rdparty/libprocess/include/process/subprocess_base.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/include/process/subprocess_base.hpp b/3rdparty/libprocess/include/process/subprocess_base.hpp
index a3edae6..d33be2b 100644
--- a/3rdparty/libprocess/include/process/subprocess_base.hpp
+++ b/3rdparty/libprocess/include/process/subprocess_base.hpp
@@ -318,17 +318,12 @@ private:
if (in.isSome()) { os::close(in.get()); }
if (out.isSome()) { os::close(out.get()); }
if (err.isSome()) { os::close(err.get()); }
-
-#ifdef __WINDOWS__
- CloseHandle(processInformation.hProcess);
- CloseHandle(processInformation.hThread);
-#endif // __WINDOWS__
}
pid_t pid;
#ifdef __WINDOWS__
- PROCESS_INFORMATION processInformation;
+ Option<::internal::windows::ProcessData> process_data;
#endif // __WINDOWS__
// The parent side of the pipe for stdin/stdout/stderr. If the
http://git-wip-us.apache.org/repos/asf/mesos/blob/4b28a28c/3rdparty/libprocess/include/process/windows/subprocess.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/include/process/windows/subprocess.hpp b/3rdparty/libprocess/include/process/windows/subprocess.hpp
index 5459955..0183bb4 100644
--- a/3rdparty/libprocess/include/process/windows/subprocess.hpp
+++ b/3rdparty/libprocess/include/process/windows/subprocess.hpp
@@ -16,6 +16,7 @@
#include <signal.h>
#include <string>
+#include <tuple>
#include <glog/logging.h>
@@ -38,192 +39,34 @@
namespace process {
namespace internal {
-// Retrieves system environment in a `std::map`, ignoring
-// the current process's environment variables.
-inline Option<std::map<std::wstring, std::wstring>> getSystemEnvironment()
-{
- std::map<std::wstring, std::wstring> systemEnvironment;
- wchar_t* environmentEntry = nullptr;
-
- // Get the system environment.
- // The third parameter (bool) tells the function *not* to inherit
- // variables from the current process.
- if (!CreateEnvironmentBlock((LPVOID*)&environmentEntry, nullptr, FALSE)) {
- return None();
- }
-
- // Save the environment block in order to destroy it later.
- wchar_t* environmentBlock = environmentEntry;
-
- while (*environmentEntry != L'\0') {
- // Each environment block contains the environment variables as follows:
- // Var1=Value1\0
- // Var2=Value2\0
- // Var3=Value3\0
- // ...
- // VarN=ValueN\0\0
- // The name of an environment variable cannot include an equal sign (=).
-
- // Construct a string from the pointer up to the first '\0',
- // e.g. "Var1=Value1\0", then split into name and value.
- std::wstring entryString(environmentEntry);
- std::wstring::size_type separator = entryString.find(L"=");
- std::wstring varName(entryString.substr(0, separator));
- std::wstring varVal(entryString.substr(separator + 1));
-
- // Mesos variables are upper case. Convert system variables to
- // match the name provided by the scheduler in case of a collision.
- // This is safe because Windows environment variables are case insensitive.
- std::transform(varName.begin(), varName.end(), varName.begin(), ::towupper);
-
- // The system environment has priority.
- systemEnvironment.insert_or_assign(varName.data(), varVal.data());
-
- // Advance the pointer the length of the entry string plus the '\0'.
- environmentEntry += entryString.length() + 1;
- }
-
- DestroyEnvironmentBlock(environmentBlock);
-
- return systemEnvironment;
-}
-
-
-// Creates a null-terminated array of null-terminated strings that will be
-// passed to `CreateProcessW` as the `lpEnvironment` argument, as described by
-// MSDN[1]. This array needs to be sorted in alphabetical order, but the `map`
-// already takes care of that. Note that this function explicitly handles
-// UTF-16 environments, so it must be used in conjunction with the
-// `CREATE_UNICODE_ENVIRONMENT` flag.
-//
-// NOTE: This function will add the system's environment variables into
-// the returned string. These variables take precedence over the provided
-// `env` and are generally necessary in order to launch things on Windows.
-//
-// [1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx
-inline Option<std::wstring> createProcessEnvironment(
- const Option<std::map<std::string, std::string>>& env)
-{
- if (env.isNone() || (env.isSome() && env.get().size() == 0)) {
- return None();
- }
-
- Option<std::map<std::wstring, std::wstring>> systemEnvironment =
- getSystemEnvironment();
-
- // The system environment must be non-empty.
- // No subprocesses will be able to launch if the system environment is blank.
- CHECK(systemEnvironment.isSome() && systemEnvironment.get().size() > 0);
-
- std::map<std::wstring, std::wstring> combinedEnvironment;
-
- // Populate the combined environment first with the given environment
- // converted to UTF-16 for Windows.
- std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>, wchar_t> converter;
- foreachpair (const std::string& key,
- const std::string& value,
- env.get()) {
- combinedEnvironment[converter.from_bytes(key)] =
- converter.from_bytes(value);
- }
-
- // Add the system environment variables, overwriting the previous.
- foreachpair (const std::wstring& key,
- const std::wstring& value,
- systemEnvironment.get()) {
- combinedEnvironment[key] = value;
- }
-
- std::wstring environmentString;
- foreachpair (const std::wstring& key,
- const std::wstring& value,
- combinedEnvironment) {
- environmentString += key + L'=' + value + L'\0';
- }
-
- // Append final null terminating character.
- environmentString.push_back(L'\0');
- return environmentString;
-}
-
-
// NOTE: We are expecting that components of `argv` that need to be quoted
// (for example, paths with spaces in them like `C:\"Program Files"\foo.exe`)
// to have been already quoted correctly before we generate `command`.
// Incorrectly-quoted command arguments will probably lead the child process
// to terminate with an error. See also NOTE on `process::subprocess`.
-inline Try<PROCESS_INFORMATION> createChildProcess(
+inline Try<::internal::windows::ProcessData> createChildProcess(
const std::string& path,
const std::vector<std::string>& argv,
const Option<std::map<std::string, std::string>>& environment,
+ const std::vector<Subprocess::ParentHook>& parent_hooks,
const InputFileDescriptors stdinfds,
const OutputFileDescriptors stdoutfds,
- const OutputFileDescriptors stderrfds,
- const std::vector<Subprocess::ParentHook>& parent_hooks)
+ const OutputFileDescriptors stderrfds)
{
- // The second argument to `::CreateProcessW` explicitly requries a writable
- // buffer, so we copy the `wstring` data into this `vector`.
- std::wstring command = os::stringify_args(argv);
- std::vector<wchar_t> commandLine(command.begin(), command.end());
-
- // Create the process suspended and with a Unicode environment.
- DWORD creationFlags = CREATE_SUSPENDED | CREATE_UNICODE_ENVIRONMENT;
-
- // Construct the environment that will be passed to `::CreateProcessW`.
- Option<std::wstring> environmentString =
- createProcessEnvironment(environment);
-
- const wchar_t* processEnvironment = environmentString.isNone()
- ? nullptr
- : environmentString.get().data();
-
- STARTUPINFOW startupInfo;
- PROCESS_INFORMATION processInfo;
-
- ::ZeroMemory(&startupInfo, sizeof(STARTUPINFOW));
- ::ZeroMemory(&processInfo, sizeof(PROCESS_INFORMATION));
-
- // Hook up the `stdin`/`stdout`/`stderr` pipes and use the
- // `STARTF_USESTDHANDLES` flag to instruct the child to use them[1]. A more
- // user-friendly example can be found in [2].
- //
- // [1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms686331(v=vs.85).aspx
- // [2] https://msdn.microsoft.com/en-us/library/windows/desktop/ms682499(v=vs.85).aspx
- startupInfo.cb = sizeof(STARTUPINFOW);
- startupInfo.hStdInput = stdinfds.read;
- startupInfo.hStdOutput = stdoutfds.write;
- startupInfo.hStdError = stderrfds.write;
- startupInfo.dwFlags |= STARTF_USESTDHANDLES;
-
- // TODO(hausdorff): Figure out how to map the `path` and `args` arguments of
- // this function into a call to `::CreateProcess` that is more general
- // purpose. In particular, on POSIX, we expect that calls to `subprocess` can
- // be called with relative `path` (e.g., it could simply be `sh`). However,
- // on Windows, unlike the calls to (e.g.) `exec`, `::CreateProcessW` will
- // expect that this argument be a fully-qualified path. In the end, we'd like
- // the calls to `subprocess` to have similar command formats to minimize
- // confusion and mistakes.
- //
- // [1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx
- BOOL createProcessResult = ::CreateProcessW(
- nullptr,
- (LPWSTR)commandLine.data(),
- nullptr, // Default security attributes.
- nullptr, // Default primary thread security attributes.
- TRUE, // Inherited parent process handles.
- creationFlags,
- (LPVOID)processEnvironment,
- nullptr, // Use parent's current directory.
- &startupInfo, // STARTUPINFO pointer.
- &processInfo); // PROCESS_INFORMATION pointer.
-
- if (!createProcessResult) {
- return WindowsError(
- "Failed to call CreateProcess on command '" + stringify(command) + "'");
+ Try<::internal::windows::ProcessData> process_data =
+ ::internal::windows::create_process(
+ path,
+ argv,
+ environment,
+ true, // Create suspended.
+ std::make_tuple(stdinfds.read, stdoutfds.write, stderrfds.write));
+
+ if (process_data.isError()) {
+ return process_data;
}
// Run the parent hooks.
- const pid_t pid = processInfo.dwProcessId;
+ const pid_t pid = process_data.get().pid;
foreach (const Subprocess::ParentHook& hook, parent_hooks) {
Try<Nothing> parentSetup = hook.parent_setup(pid);
@@ -234,23 +77,23 @@ inline Try<PROCESS_INFORMATION> createChildProcess(
// do not need to kill any descendents. We also can't use `os::kill_job`
// because this process is not in a Job Object unless one of the parent
// hooks added it.
- ::TerminateProcess(processInfo.hProcess, 1);
+ ::TerminateProcess(process_data.get().process_handle.get_handle(), 1);
return Error(
"Failed to execute Parent Hook in child '" + stringify(pid) +
- "' with command '" + stringify(command) + "': " +
+ "' with command '" + stringify(argv) + "': " +
parentSetup.error());
}
}
// Start child process.
- if (::ResumeThread(processInfo.hThread) == -1) {
+ if (::ResumeThread(process_data.get().thread_handle.get_handle()) == -1) {
return WindowsError(
"Failed to resume child process with command '" +
- stringify(command) + "'");
+ stringify(argv) + "'");
}
- return processInfo;
+ return process_data;
}
} // namespace internal {
http://git-wip-us.apache.org/repos/asf/mesos/blob/4b28a28c/3rdparty/libprocess/src/subprocess.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/src/subprocess.cpp b/3rdparty/libprocess/src/subprocess.cpp
index 0f1532b..d0aa5f8 100644
--- a/3rdparty/libprocess/src/subprocess.cpp
+++ b/3rdparty/libprocess/src/subprocess.cpp
@@ -380,22 +380,23 @@ Try<Subprocess> subprocess(
process.data->pid = pid.get();
#else
// TODO(joerg84): Consider using the childHooks and parentHooks here.
- Try<PROCESS_INFORMATION> processInformation = internal::createChildProcess(
- path,
- argv,
- environment,
- stdinfds,
- stdoutfds,
- stderrfds,
- parent_hooks);
-
- if (processInformation.isError()) {
+ Try<::internal::windows::ProcessData> process_data =
+ internal::createChildProcess(
+ path,
+ argv,
+ environment,
+ parent_hooks,
+ stdinfds,
+ stdoutfds,
+ stderrfds);
+
+ if (process_data.isError()) {
process::internal::close(stdinfds, stdoutfds, stderrfds);
return Error(
- "Could not launch child process: " + processInformation.error());
+ "Could not launch child process: " + process_data.error());
}
- if (processInformation.get().dwProcessId == -1) {
+ if (process_data.get().pid == -1) {
// Save the errno as 'close' below might overwrite it.
ErrnoError error("Failed to clone");
process::internal::close(stdinfds, stdoutfds, stderrfds);
@@ -408,8 +409,8 @@ Try<Subprocess> subprocess(
// 'createChildProcess' to be consistent with the posix path.
internal::close({stdinfds.read, stdoutfds.write, stderrfds.write});
- process.data->processInformation = processInformation.get();
- process.data->pid = processInformation.get().dwProcessId;
+ process.data->process_data = process_data.get();
+ process.data->pid = process_data.get().pid;
#endif // __WINDOWS__
}