mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Clemmer <clemmer.alexan...@gmail.com>
Subject Re: Review Request 55547: Windows: Standardize on win32 environment, transition away from CRT.
Date Sun, 15 Jan 2017 21:44:11 GMT

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

(Updated Jan. 15, 2017, 9:44 p.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


Changes
-------

Updated typos I made copying from my Windows box. :(


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 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