mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mesos ReviewBot <revi...@mesos.apache.org>
Subject Re: Review Request 54415: Stout: Fixed two bugs in `mkdtemp` that block agent tests.
Date Tue, 06 Dec 2016 15:40:56 GMT

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



Patch looks great!

Reviews applied: [54324, 53550, 53551, 53552, 54395, 54415]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose'
ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 6, 2016, 11:29 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54415/
> -----------------------------------------------------------
> 
> (Updated Dec. 6, 2016, 11:29 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-6697
>     https://issues.apache.org/jira/browse/MESOS-6697
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Currently there are two bugs in the Windows implementation of
> `os::mkdtemp`, which together cause all agent tests to fail. We describe
> these bugs below and explain the steps this commit will take to address
> them.
> 
> The first concerns a mismatch between the POSIX specification of
> `mkdtemp` and the Windows implementation of `os::mkdtemp`. Specifically,
> the POSIX specification indicates that, if any component of the path
> prefix in the directory pattern passed to `mkdtemp` does not exist, we
> are to set `errno` to `ENOENT` and return a null pointer. In the Stout
> POSIX wrapper, `os::mkdtemp`, we will return an `ErrnoError`. In
> contrast, the Windows implementation will erroneously recursively create
> all components of the path prefix. This commit will address this by
> explicitly setting the `recursive` flag of the call to `os::mkdir` to
> `false`.
> 
> The second concerns a failure to parse Unix-like paths when creating the
> temporary directory. Specifically, both the POSIX and Windows
> implementations of `os::mkdtemp` have a default argument: `/tmp/XXXXXX`.
> When we replace the 'X' characters with random alphanumeric characters
> and pass the result to `os::mkdir`, and when the `recursive` flag is
> errorneously set to `true`, we will fail to parse the path on Windows,
> because `os::mkdir` can only pass Windows-style paths, with the '\'
> character. This causes all Agent tests to fail, e.g., when we try to
> create sandbox directories.
> 
> Technically, this second issue is actually resolved by setting the
> `recursive` flag to `false` in the call to `os::mkdir`, but here we
> observe that `/tmp` is not the "right place" to put temporary files on
> Windows anyway, and so we take the time to clean it up here, by
> replacing the default string literal path `/tmp/XXXXXX` with the
> platform-agnostic `path::join(os::temp(), "XXXXXX)`.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/Makefile.am d5bc728ce378586e84173b98499b0d52047a83e1 
>   3rdparty/stout/include/stout/os.hpp bd085e4e29bbdb2d2baaaeff1d10c0bd95ca65ba 
>   3rdparty/stout/include/stout/os/posix/mkdtemp.hpp 90866dba8e6be206c64f21204d936c1bed808c9a

>   3rdparty/stout/include/stout/os/posix/temp.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/temp.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/mkdtemp.hpp 2cb73bbb996775e3764ad852ccda5076b41aef41

>   3rdparty/stout/include/stout/os/windows/temp.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/posix/os.hpp 1f4d155e4399b955d0ca884f5336c78d8ceb56b5

>   3rdparty/stout/include/stout/windows/os.hpp de9b04ad82443038a0f4408bc72cae1540a1beaf

> 
> Diff: https://reviews.apache.org/r/54415/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>


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