mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 55547: Windows: Standardize on win32 environment, transition away from CRT.
Date Tue, 24 Jan 2017 02:23:55 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55547/#review162754
-----------------------------------------------------------



I agree with this change, just a few questions/tweaks:


3rdparty/stout/include/stout/os/raw/environment.hpp (lines 107 - 119)
<https://reviews.apache.org/r/55547/#comment234100>

    Should we also move away from functions like `os::execvpe`?  If so, we would be able to
completely exclude `stout/raw/environment.hpp` from the Windows headers.
    
    `Envp` is currently only used in one location:
    https://github.com/apache/mesos/blob/f1d0cdf1db2a28fa44f7aded0c3760636c0a51de/src/slave/containerizer/mesos/launch.cpp#L659-L684



3rdparty/stout/include/stout/os/windows/environment.hpp (lines 22 - 40)
<https://reviews.apache.org/r/55547/#comment234098>

    We should refactor this code along with some very similar parsing logic here:
    https://github.com/apache/mesos/blob/f1d0cdf1db2a28fa44f7aded0c3760636c0a51de/3rdparty/libprocess/include/process/windows/subprocess.hpp#L90-L114



3rdparty/stout/tests/os_tests.cpp (line 74)
<https://reviews.apache.org/r/55547/#comment234099>

    Remove the disabled macro, since this is now ifdef'd


- Joseph Wu


On Jan. 15, 2017, 1:44 p.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55547/
> -----------------------------------------------------------
> 
> (Updated Jan. 15, 2017, 1:44 p.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 53d04a9b6c4a0d8b35d3c84ef24d619fdb8a2c82 
>   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 30735e28a26ff713469711d63538676ed4e327d9 
> 
> Diff: https://reviews.apache.org/r/55547/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message