You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alex Clemmer <cl...@gmail.com> on 2017/02/14 18:47:32 UTC
Re: Review Request 55547: Windows: Standardize on win32 environment,
transition away from CRT.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55547/
-----------------------------------------------------------
(Updated Feb. 14, 2017, 6:47 p.m.)
Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
Changes
-------
Address Joseph's comments.
Bugs: MESOS-5880
https://issues.apache.org/jira/browse/MESOS-5880
Repository: mesos
Description
-------
Windows currently exposes two APIs for managing a process's environment
variables: a CRT API, and a win32 API. This commit will transition Stout
to use only the win32 API, and retire its use of the CRT APIs.
There are many reasons for this, for example:
* Stout currently uses both the CRT and win32 APIs, but they are
incompatible, and this causes real bugs. For example, because
`os::setenv` uses the win32 API, but `os::environment` uses the CRT
API, it is possible to set an environment variable and then later not
see it reflected in the environment. In Mesos this causes many bugs,
such as when we expect to set `LIBPROCESS_PORT` in a parent, then
spawn a health checker, this port doesn't get picked up.
* The CRT API is very old, and essentially unmaintained. It should not
be used unless it is necessary.
* It is generally easier to mirror the most common POSIX semantics of
environment APIs with the win32 API than it is with the CRT API. For
example, the Windows CRT implementation of `setenv` will delete an
environment variable if you pass an empty string as value, instead of
setting the value to be an empty string, like most modern POSIX
implementations. On the other hand, the win32 equivalent,
`SetEnvironmentVariable`, does implement this behavior.
The effort to standardize the Windows code paths essentially involves
two things:
(1) Removing `os::raw::environment` from Stout's Windows API.
`os::raw::environment` is not used by the Windows codepaths, and (for
reasons above) we want to avoid is accidental use of `environ` on
Windows in the future, as well.
While it is possible to simply implement `os::raw::environment` using
the win32 API `GetEnvironmentStrings`, these return fundamentally
different types, so the allocation logic would become more complex, and
the semantics of the function would have to change. Either the user
would have to allocate a `char**` for the environment, or Stout would
have to manage a `static char**`, or the function would have to allocate
memory for the user to `free`. All of these are at odds with the POSIX
semantics, and since this API is only used on POSIX paths, there is no
real advantage in this line of inquiry.
(2) Splitting up the implementation of `os::environment`.
The POSIX `environ` and Windows `GetEnvironmentStrings` are
fundamentally different types, and require mostly different processing
logic to transform them to a `hashmap`. There is no real advantage in
convoluting this processing code to keep the code common between them.
Diffs (updated)
-----
3rdparty/stout/include/Makefile.am 4bde2ef3f466ed91c6922b330f07f5d425398751
3rdparty/stout/include/stout/os/environment.hpp d8c34999829257bee80b0678f2ee096f4798c62b
3rdparty/stout/include/stout/os/posix/environment.hpp PRE-CREATION
3rdparty/stout/include/stout/os/raw/environment.hpp b3e82ac8071b41748aeb098b7d5fcc210a1d3c43
3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION
3rdparty/stout/tests/os_tests.cpp fc359159afcdb60e4406821e123b6358320b6df8
Diff: https://reviews.apache.org/r/55547/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 55547: Windows: Standardize on win32 environment,
transition away from CRT.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55547/#review165625
-----------------------------------------------------------
3rdparty/stout/include/stout/os/windows/environment.hpp (line 27)
<https://reviews.apache.org/r/55547/#comment237490>
Oh, this variable is apparently un-used. I can remove it before committing.
- Joseph Wu
On Feb. 14, 2017, 10:47 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55547/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 10:47 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Bugs: MESOS-5880
> https://issues.apache.org/jira/browse/MESOS-5880
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Windows currently exposes two APIs for managing a process's environment
> variables: a CRT API, and a win32 API. This commit will transition Stout
> to use only the win32 API, and retire its use of the CRT APIs.
>
> There are many reasons for this, for example:
>
> * Stout currently uses both the CRT and win32 APIs, but they are
> incompatible, and this causes real bugs. For example, because
> `os::setenv` uses the win32 API, but `os::environment` uses the CRT
> API, it is possible to set an environment variable and then later not
> see it reflected in the environment. In Mesos this causes many bugs,
> such as when we expect to set `LIBPROCESS_PORT` in a parent, then
> spawn a health checker, this port doesn't get picked up.
> * The CRT API is very old, and essentially unmaintained. It should not
> be used unless it is necessary.
> * It is generally easier to mirror the most common POSIX semantics of
> environment APIs with the win32 API than it is with the CRT API. For
> example, the Windows CRT implementation of `setenv` will delete an
> environment variable if you pass an empty string as value, instead of
> setting the value to be an empty string, like most modern POSIX
> implementations. On the other hand, the win32 equivalent,
> `SetEnvironmentVariable`, does implement this behavior.
>
> The effort to standardize the Windows code paths essentially involves
> two things:
>
> (1) Removing `os::raw::environment` from Stout's Windows API.
>
> `os::raw::environment` is not used by the Windows codepaths, and (for
> reasons above) we want to avoid is accidental use of `environ` on
> Windows in the future, as well.
>
> While it is possible to simply implement `os::raw::environment` using
> the win32 API `GetEnvironmentStrings`, these return fundamentally
> different types, so the allocation logic would become more complex, and
> the semantics of the function would have to change. Either the user
> would have to allocate a `char**` for the environment, or Stout would
> have to manage a `static char**`, or the function would have to allocate
> memory for the user to `free`. All of these are at odds with the POSIX
> semantics, and since this API is only used on POSIX paths, there is no
> real advantage in this line of inquiry.
>
> (2) Splitting up the implementation of `os::environment`.
>
> The POSIX `environ` and Windows `GetEnvironmentStrings` are
> fundamentally different types, and require mostly different processing
> logic to transform them to a `hashmap`. There is no real advantage in
> convoluting this processing code to keep the code common between them.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/Makefile.am 4bde2ef3f466ed91c6922b330f07f5d425398751
> 3rdparty/stout/include/stout/os/environment.hpp d8c34999829257bee80b0678f2ee096f4798c62b
> 3rdparty/stout/include/stout/os/posix/environment.hpp PRE-CREATION
> 3rdparty/stout/include/stout/os/raw/environment.hpp b3e82ac8071b41748aeb098b7d5fcc210a1d3c43
> 3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION
> 3rdparty/stout/tests/os_tests.cpp fc359159afcdb60e4406821e123b6358320b6df8
>
> Diff: https://reviews.apache.org/r/55547/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 55547: Windows: Standardize on win32 environment,
transition away from CRT.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55547/#review165576
-----------------------------------------------------------
Ship it!
A few minor nits below that I will fix before committing.
3rdparty/stout/include/stout/os/raw/environment.hpp (line 36)
<https://reviews.apache.org/r/55547/#comment237428>
I think the capitalization of `win32` should be `Win32`. That's how it appears in the MSDN docs :)
3rdparty/stout/include/stout/os/raw/environment.hpp (line 82)
<https://reviews.apache.org/r/55547/#comment237429>
Even if the condition is `ifndef`, we don't add an exclamation in the comment. The only exception is if we have a block like:
```
#if !defined(__WINDOWS__) || defined(__linux__)
```
In that case, the ending comment would be:
```
#endif // !__WINDOWS || __linux__
```
3rdparty/stout/include/stout/os/raw/environment.hpp (line 104)
<https://reviews.apache.org/r/55547/#comment237430>
Similar here.
3rdparty/stout/include/stout/os/windows/environment.hpp (line 24)
<https://reviews.apache.org/r/55547/#comment237432>
For code-reading sanity, I'm going to add this comment block:
```
// NOTE: The `GetEnvironmentStrings` call returns a pointer to a
// read-only block of memory with the following format (minus newlines):
// Var1=Value1\0
// Var2=Value2\0
// Var3=Value3\0
// ...
// VarN=ValueN\0\0
```
3rdparty/stout/include/stout/os/windows/environment.hpp (line 28)
<https://reviews.apache.org/r/55547/#comment237431>
Missing space after the `for`
3rdparty/stout/include/stout/os/windows/environment.hpp (line 30)
<https://reviews.apache.org/r/55547/#comment237433>
For code-reading sanity:
// Increment past the current environment string and null terminator.
- Joseph Wu
On Feb. 14, 2017, 10:47 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55547/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 10:47 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Bugs: MESOS-5880
> https://issues.apache.org/jira/browse/MESOS-5880
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Windows currently exposes two APIs for managing a process's environment
> variables: a CRT API, and a win32 API. This commit will transition Stout
> to use only the win32 API, and retire its use of the CRT APIs.
>
> There are many reasons for this, for example:
>
> * Stout currently uses both the CRT and win32 APIs, but they are
> incompatible, and this causes real bugs. For example, because
> `os::setenv` uses the win32 API, but `os::environment` uses the CRT
> API, it is possible to set an environment variable and then later not
> see it reflected in the environment. In Mesos this causes many bugs,
> such as when we expect to set `LIBPROCESS_PORT` in a parent, then
> spawn a health checker, this port doesn't get picked up.
> * The CRT API is very old, and essentially unmaintained. It should not
> be used unless it is necessary.
> * It is generally easier to mirror the most common POSIX semantics of
> environment APIs with the win32 API than it is with the CRT API. For
> example, the Windows CRT implementation of `setenv` will delete an
> environment variable if you pass an empty string as value, instead of
> setting the value to be an empty string, like most modern POSIX
> implementations. On the other hand, the win32 equivalent,
> `SetEnvironmentVariable`, does implement this behavior.
>
> The effort to standardize the Windows code paths essentially involves
> two things:
>
> (1) Removing `os::raw::environment` from Stout's Windows API.
>
> `os::raw::environment` is not used by the Windows codepaths, and (for
> reasons above) we want to avoid is accidental use of `environ` on
> Windows in the future, as well.
>
> While it is possible to simply implement `os::raw::environment` using
> the win32 API `GetEnvironmentStrings`, these return fundamentally
> different types, so the allocation logic would become more complex, and
> the semantics of the function would have to change. Either the user
> would have to allocate a `char**` for the environment, or Stout would
> have to manage a `static char**`, or the function would have to allocate
> memory for the user to `free`. All of these are at odds with the POSIX
> semantics, and since this API is only used on POSIX paths, there is no
> real advantage in this line of inquiry.
>
> (2) Splitting up the implementation of `os::environment`.
>
> The POSIX `environ` and Windows `GetEnvironmentStrings` are
> fundamentally different types, and require mostly different processing
> logic to transform them to a `hashmap`. There is no real advantage in
> convoluting this processing code to keep the code common between them.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/Makefile.am 4bde2ef3f466ed91c6922b330f07f5d425398751
> 3rdparty/stout/include/stout/os/environment.hpp d8c34999829257bee80b0678f2ee096f4798c62b
> 3rdparty/stout/include/stout/os/posix/environment.hpp PRE-CREATION
> 3rdparty/stout/include/stout/os/raw/environment.hpp b3e82ac8071b41748aeb098b7d5fcc210a1d3c43
> 3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION
> 3rdparty/stout/tests/os_tests.cpp fc359159afcdb60e4406821e123b6358320b6df8
>
> Diff: https://reviews.apache.org/r/55547/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>