mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Clemmer <clemmer.alexan...@gmail.com>
Subject Review Request 54415: Stout: Fixed two bugs in `mkdtemp` that block agent tests.
Date Tue, 06 Dec 2016 11:29:59 GMT

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

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