You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by an...@apache.org on 2018/05/02 01:38:19 UTC
[19/31] mesos git commit: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Windows: Removed `FD_CRT` from `WindowsFD` abstraction.
After all the CRT APIs were replaced with Windows APIs, we no longer
needed to support the semantics of an `int` file descriptor in
general (in the sense of opening a CRT handle that's associated with
the actual kernel object for the given `HANDLE`). There are specific
use cases (usually third-party code) which still require a CRT
int-like file descriptor, which the `crt()` function explicitly
allocates (this allocation used to be done in the constructor).
Thus the entire `FD_CRT` type was removed from the `WindowsFD`
abstraction. It still acts like an `int` in the sense that it can be
constructed from one and compared to one. However, construction via
`int` only supports the standard file descriptors 0, 1, and 2 for
`stdin`, `stdout`, and `stderr`. Any other construction creates an
`int_fd` which holds an `INVALID_HANDLE_VALUE`. When being compared to
an `int`, the abstraction simply returns -1 if it is invalid (based on
the result of the `is_valid()` method) or 0 if it is valid. This is to
support the semantics of checking validity by something like
`if (fd < 0)` or `if (fd == -1)`.
With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
APIs that switched on the type were simplified, with the last of the
CRT code deleted.
Thanks to the introduction of the private `int get_valid()` function,
and the removal of the `FD_CRT` type, the comparison operators became
much simpler.
Several unit tests in the `FsTest` suite became cross-platform, with
the `Close` test being simplified to test against an `int_fd`.
Review: https://reviews.apache.org/r/66437
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/d0b055b0
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/d0b055b0
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/d0b055b0
Branch: refs/heads/master
Commit: d0b055b084409c021ded8ed131b16e6a3b568f4a
Parents: 92d340f
Author: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
Authored: Tue Mar 20 20:22:49 2018 -0700
Committer: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
Committed: Tue May 1 18:36:04 2018 -0700
----------------------------------------------------------------------
.../stout/include/stout/os/windows/close.hpp | 33 +-
3rdparty/stout/include/stout/os/windows/dup.hpp | 14 +-
.../stout/include/stout/os/windows/fcntl.hpp | 12 +-
3rdparty/stout/include/stout/os/windows/fd.hpp | 481 ++++++++-----------
.../stout/include/stout/os/windows/pipe.hpp | 2 +-
.../stout/include/stout/os/windows/read.hpp | 8 +-
.../stout/include/stout/os/windows/socket.hpp | 2 +-
.../stout/include/stout/os/windows/write.hpp | 8 +-
3rdparty/stout/tests/os/filesystem_tests.cpp | 131 ++---
3rdparty/stout/tests/os/socket_tests.cpp | 13 +
10 files changed, 284 insertions(+), 420 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/d0b055b0/3rdparty/stout/include/stout/os/windows/close.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/os/windows/close.hpp b/3rdparty/stout/include/stout/os/windows/close.hpp
index fc3a676..1dddae2 100644
--- a/3rdparty/stout/include/stout/os/windows/close.hpp
+++ b/3rdparty/stout/include/stout/os/windows/close.hpp
@@ -13,11 +13,9 @@
#ifndef __STOUT_OS_WINDOWS_CLOSE_HPP__
#define __STOUT_OS_WINDOWS_CLOSE_HPP__
-#include <errno.h>
-
+#include <stout/error.hpp>
#include <stout/nothing.hpp>
#include <stout/try.hpp>
-#include <stout/windows/error.hpp>
#include <stout/os/int_fd.hpp>
@@ -28,29 +26,38 @@ namespace os {
inline Try<Nothing> close(const int_fd& fd)
{
switch (fd.type()) {
- case WindowsFD::FD_CRT:
- case WindowsFD::FD_HANDLE: {
- // We don't need to call `CloseHandle` on `fd.handle`, because calling
- // `_close` on the corresponding CRT FD implicitly invokes `CloseHandle`.
- if (::_close(fd.crt()) < 0) {
- return ErrnoError();
+ case WindowsFD::Type::HANDLE: {
+ if (!fd.is_valid()) {
+ // NOTE: We return early here because
+ // `CloseHandle(INVALID_HANDLE_VALUE)` will not return an error, but
+ // instead (sometimes) triggers the invalid parameter handler, thus
+ // throwing an exception. We'd rather return an error.
+ return WindowsError(ERROR_INVALID_HANDLE);
+ }
+
+ if (::CloseHandle(fd) == FALSE) {
+ return WindowsError();
}
- break;
+
+ return Nothing();
}
- case WindowsFD::FD_SOCKET: {
+ case WindowsFD::Type::SOCKET: {
// NOTE: Since closing an unconnected socket is not an error in POSIX,
// we simply ignore it here.
if (::shutdown(fd, SD_BOTH) == SOCKET_ERROR &&
WSAGetLastError() != WSAENOTCONN) {
return WindowsSocketError("Failed to shutdown a socket");
}
+
if (::closesocket(fd) == SOCKET_ERROR) {
return WindowsSocketError("Failed to close a socket");
}
- break;
+
+ return Nothing();
}
}
- return Nothing();
+
+ UNREACHABLE();
}
} // namespace os {
http://git-wip-us.apache.org/repos/asf/mesos/blob/d0b055b0/3rdparty/stout/include/stout/os/windows/dup.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/os/windows/dup.hpp b/3rdparty/stout/include/stout/os/windows/dup.hpp
index 54b78b1..af98054 100644
--- a/3rdparty/stout/include/stout/os/windows/dup.hpp
+++ b/3rdparty/stout/include/stout/os/windows/dup.hpp
@@ -25,16 +25,7 @@ namespace os {
inline Try<int_fd> dup(const int_fd& fd)
{
switch (fd.type()) {
- // TODO(andschwa): Remove this when `FD_CRT` is removed, MESOS-8675.
- case WindowsFD::FD_CRT: {
- int result = ::_dup(fd.crt());
- if (result == -1) {
- return ErrnoError();
- }
-
- return result;
- }
- case WindowsFD::FD_HANDLE: {
+ case WindowsFD::Type::HANDLE: {
HANDLE duplicate = INVALID_HANDLE_VALUE;
const BOOL result = ::DuplicateHandle(
::GetCurrentProcess(), // Source process == current.
@@ -51,7 +42,7 @@ inline Try<int_fd> dup(const int_fd& fd)
return duplicate;
}
- case WindowsFD::FD_SOCKET: {
+ case WindowsFD::Type::SOCKET: {
WSAPROTOCOL_INFOW info;
const int result =
::WSADuplicateSocketW(fd, ::GetCurrentProcessId(), &info);
@@ -62,6 +53,7 @@ inline Try<int_fd> dup(const int_fd& fd)
return ::WSASocketW(0, 0, 0, &info, 0, 0);
}
}
+
UNREACHABLE();
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/d0b055b0/3rdparty/stout/include/stout/os/windows/fcntl.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/os/windows/fcntl.hpp b/3rdparty/stout/include/stout/os/windows/fcntl.hpp
index bb82676..90cdbfb 100644
--- a/3rdparty/stout/include/stout/os/windows/fcntl.hpp
+++ b/3rdparty/stout/include/stout/os/windows/fcntl.hpp
@@ -45,12 +45,11 @@ inline Try<bool> isCloexec(const int_fd& fd)
inline Try<Nothing> nonblock(const int_fd& fd)
{
switch (fd.type()) {
- case WindowsFD::FD_CRT:
- case WindowsFD::FD_HANDLE: {
+ case WindowsFD::Type::HANDLE: {
/* Do nothing. */
- break;
+ return Nothing();
}
- case WindowsFD::FD_SOCKET: {
+ case WindowsFD::Type::SOCKET: {
const u_long non_block_mode = 1;
u_long mode = non_block_mode;
@@ -58,10 +57,11 @@ inline Try<Nothing> nonblock(const int_fd& fd)
if (result != NO_ERROR) {
return WindowsSocketError();
}
- break;
+ return Nothing();
}
}
- return Nothing();
+
+ UNREACHABLE();
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/d0b055b0/3rdparty/stout/include/stout/os/windows/fd.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/os/windows/fd.hpp b/3rdparty/stout/include/stout/os/windows/fd.hpp
index d7f8cdf..bab16e8 100644
--- a/3rdparty/stout/include/stout/os/windows/fd.hpp
+++ b/3rdparty/stout/include/stout/os/windows/fd.hpp
@@ -13,9 +13,13 @@
#ifndef __STOUT_OS_WINDOWS_FD_HPP__
#define __STOUT_OS_WINDOWS_FD_HPP__
+#include <fcntl.h> // For `O_RDWR`.
+#include <io.h> // For `_open_osfhandle`.
+
#include <array>
#include <memory>
#include <ostream>
+#include <type_traits>
#include <stout/check.hpp>
#include <stout/nothing.hpp>
@@ -37,9 +41,8 @@ namespace os {
// Try<int_fd> fd = os::open(...);
//
// The `WindowsFD` constructs off one of:
-// (1) `int` - from the WinCRT API
-// (2) `HANDLE` - from the Win32 API
-// (3) `SOCKET` - from the WinSock API
+// (1) `HANDLE` - from the Win32 API
+// (2) `SOCKET` - from the WinSock API
//
// The `os::*` functions then take an instance of `WindowsFD`, examines
// the state and dispatches to the appropriate API.
@@ -47,74 +50,112 @@ namespace os {
class WindowsFD
{
public:
- enum Type
+ enum class Type
{
- FD_CRT,
- FD_HANDLE,
- FD_SOCKET
+ HANDLE,
+ SOCKET
};
- WindowsFD() = default;
-
- WindowsFD(int crt)
- : type_(FD_CRT),
- crt_(crt),
- handle_(
- crt < 0 ? INVALID_HANDLE_VALUE
- : reinterpret_cast<HANDLE>(::_get_osfhandle(crt))) {}
-
- // IMPORTANT: The `HANDLE` here is expected to be file handles. Specifically,
- // `HANDLE`s returned by file API such as `CreateFile`. There are
- // APIs that return `HANDLE`s with different error values, and
- // therefore must be handled accordingly. For example, a thread API
- // such as `CreateThread` returns `NULL` as the error value, rather
- // than `INVALID_HANDLE_VALUE`.
- // TODO(mpark): Consider adding a second parameter which tells us what the
- // error values are.
- WindowsFD(HANDLE handle)
- : type_(FD_HANDLE),
- crt_(
- handle == INVALID_HANDLE_VALUE
- ? -1
- : ::_open_osfhandle(reinterpret_cast<intptr_t>(handle), O_RDWR)),
- handle_(handle) {}
-
- WindowsFD(SOCKET socket) : type_(FD_SOCKET), socket_(socket) {}
+ // The `HANDLE` here is expected to be file handles. Specifically,
+ // `HANDLE`s returned by file API such as `CreateFile`. There are
+ // APIs that return `HANDLE`s with different error values, and
+ // therefore must be handled accordingly. For example, a thread API
+ // such as `CreateThread` returns `NULL` as the error value, rather
+ // than `INVALID_HANDLE_VALUE`.
+ //
+ // TODO(mpark): Consider adding a second parameter which tells us
+ // what the error values are.
+ static_assert(
+ std::is_same<HANDLE, void*>::value,
+ "Expected `HANDLE` to be of type `void*`.");
+ explicit WindowsFD(HANDLE handle) : type_(Type::HANDLE), handle_(handle) {}
+
+ // The `SOCKET` here is expected to be Windows sockets, such as that
+ // used by the Windows Sockets 2 library. The only expected error
+ // value is `INVALID_SOCKET`.
+ static_assert(
+ std::is_same<SOCKET, unsigned __int64>::value,
+ "Expected `SOCKET` to be of type `unsigned __int64`.");
+ explicit WindowsFD(SOCKET socket) : type_(Type::SOCKET), socket_(socket) {}
// On Windows, libevent's `evutil_socket_t` is set to `intptr_t`.
- WindowsFD(intptr_t socket)
- : type_(FD_SOCKET),
- socket_(static_cast<SOCKET>(socket)) {}
+ explicit WindowsFD(intptr_t socket) : WindowsFD(static_cast<SOCKET>(socket))
+ {}
+
+ // This constructor is provided in so that the canonical integer
+ // file descriptors representing `stdin` (0), `stdout` (1), and
+ // `stderr` (2), and the invalid value of -1 can be used.
+ //
+ // TODO(andschwa): Consider constraining to the range [-1, 2].
+ WindowsFD(int crt) : WindowsFD(INVALID_HANDLE_VALUE)
+ {
+ if (crt == 0) {
+ handle_ = ::GetStdHandle(STD_INPUT_HANDLE);
+ } else if (crt == 1) {
+ handle_ = ::GetStdHandle(STD_OUTPUT_HANDLE);
+ } else if (crt == 2) {
+ handle_ = ::GetStdHandle(STD_ERROR_HANDLE);
+ }
+ // All others default to `INVALID_HANDLE_VALUE`.
+ }
+
+ // Default construct with invalid handle semantics.
+ WindowsFD() : WindowsFD(INVALID_HANDLE_VALUE) {}
WindowsFD(const WindowsFD&) = default;
WindowsFD(WindowsFD&&) = default;
- ~WindowsFD() = default;
-
WindowsFD& operator=(const WindowsFD&) = default;
WindowsFD& operator=(WindowsFD&&) = default;
+ ~WindowsFD() = default;
+
+ bool is_valid() const
+ {
+ switch (type()) {
+ case Type::HANDLE: {
+ // Remember that both of these values can represent an invalid
+ // handle.
+ return handle_ != nullptr && handle_ != INVALID_HANDLE_VALUE;
+ }
+ case Type::SOCKET: {
+ // Only this value is used for an invalid socket.
+ return socket_ != INVALID_SOCKET;
+ }
+ default: {
+ return false;
+ }
+ }
+ }
+
+ // NOTE: This allocates a C run-time file descriptor and associates
+ // it with the handle. At this point, the `HANDLE` should no longer
+ // be closed via `CloseHandle`, but instead close the returned `int`
+ // with `_close`. This method should almost never be used, and
+ // exists only for compatibility with 3rdparty dependencies.
int crt() const
{
- CHECK((type() == FD_CRT) || (type() == FD_HANDLE));
- return crt_;
+ CHECK_EQ(Type::HANDLE, type());
+ // TODO(andschwa): Consider if we should overwrite `handle_`.
+ return ::_open_osfhandle(reinterpret_cast<intptr_t>(handle_), O_RDWR);
}
operator HANDLE() const
{
- CHECK((type() == FD_CRT) || (type() == FD_HANDLE));
+ CHECK_EQ(Type::HANDLE, type());
return handle_;
}
operator SOCKET() const
{
- CHECK_EQ(FD_SOCKET, type());
+ CHECK_EQ(Type::SOCKET, type());
return socket_;
}
+ // On Windows, libevent's `evutil_socket_t` is set to `intptr_t`.
operator intptr_t() const
{
- CHECK_EQ(FD_SOCKET, type());
+ CHECK_EQ(Type::SOCKET, type());
return static_cast<intptr_t>(socket_);
}
@@ -125,349 +166,211 @@ private:
union
{
- // We keep both a CRT FD as well as a `HANDLE`
- // regardless of whether we were constructed
- // from a file or a handle.
- //
- // This is because once we request for a CRT FD
- // from a `HANDLE`, we're required to close it
- // via `_close`. If we were to do the conversion
- // lazily upon request, the resulting CRT FD
- // would be dangling.
- struct
- {
- int crt_;
- HANDLE handle_;
- };
+ HANDLE handle_;
SOCKET socket_;
};
+
+ // NOTE: This function is provided only for checking validity, thus
+ // it is private. It provides a view of a `WindowsFD` as an `int`.
+ //
+ // TODO(andschwa): Fix all uses of this conversion to use `is_valid`
+ // directly instead, then remove the comparison operators. This
+ // would require writing an `int_fd` class for POSIX too, instead of
+ // just using `int`.
+ int get_valid() const
+ {
+ if (is_valid()) {
+ return 0;
+ } else {
+ return -1;
+ }
+ }
+
+ // NOTE: These operators are used solely to support checking a
+ // `WindowsFD` against e.g. -1 or 0 for validity. Nothing else
+ // should have access to `get_valid()`.
+ friend bool operator<(int left, const WindowsFD& right);
+ friend bool operator<(const WindowsFD& left, int right);
+ friend bool operator>(int left, const WindowsFD& right);
+ friend bool operator>(const WindowsFD& left, int right);
+ friend bool operator<=(int left, const WindowsFD& right);
+ friend bool operator<=(const WindowsFD& left, int right);
+ friend bool operator>=(int left, const WindowsFD& right);
+ friend bool operator>=(const WindowsFD& left, int right);
+ friend bool operator==(int left, const WindowsFD& right);
+ friend bool operator==(const WindowsFD& left, int right);
+ friend bool operator!=(int left, const WindowsFD& right);
+ friend bool operator!=(const WindowsFD& left, int right);
};
-inline std::ostream& operator<<(std::ostream& stream, const WindowsFD& fd)
+inline std::ostream& operator<<(std::ostream& stream, const WindowsFD::Type& fd)
{
- switch (fd.type()) {
- case WindowsFD::FD_CRT: {
- stream << fd.crt();
- break;
+ switch (fd) {
+ case WindowsFD::Type::HANDLE: {
+ stream << "WindowsFD::Type::HANDLE";
+ return stream;
}
- case WindowsFD::FD_HANDLE: {
- stream << static_cast<HANDLE>(fd);
- break;
+ case WindowsFD::Type::SOCKET: {
+ stream << "WindowsFD::Type::SOCKET";
+ return stream;
}
- case WindowsFD::FD_SOCKET: {
- stream << static_cast<SOCKET>(fd);
- break;
+ default: {
+ stream << "WindowsFD::Type::UNKNOWN";
+ return stream;
}
}
- return stream;
}
-// The complexity in this function is due to our effort in trying to support the
-// cases where file descriptors are compared as an `int` on POSIX. For example,
-// we use expressions such as `fd < 0` to check for validity.
-// TODO(mpark): Consider introducing an `is_valid` function for `int_fd`.
-inline bool operator<(const WindowsFD& left, const WindowsFD& right)
+inline std::ostream& operator<<(std::ostream& stream, const WindowsFD& fd)
{
- // In general, when compared against a `WindowsFD` in the `FD_CRT`, we map
- // `INVALID_HANDLE_VALUE` and `INVALID_SOCKET` to `-1` before performing the
- // comparison. The check for `< 0` followed by cast to `HANDLE` or `SOCKET` is
- // due to the fact that `HANDLE` and `SOCKET` are both unsigned.
- switch (left.type()) {
- case WindowsFD::FD_CRT: {
- switch (right.type()) {
- case WindowsFD::FD_CRT: {
- return left.crt() < right.crt();
- }
- case WindowsFD::FD_HANDLE: {
- if (static_cast<HANDLE>(right) == INVALID_HANDLE_VALUE) {
- return left.crt() < -1;
- }
- if (left.crt() < 0) {
- return true;
- }
-#pragma warning(push)
-#pragma warning(disable : 4312)
- // Disable `int`-to-`HANDLE` compiler warning. This is safe to do,
- // see comment above.
- return reinterpret_cast<HANDLE>(left.crt()) <
- static_cast<HANDLE>(right);
-#pragma warning(pop)
- }
- case WindowsFD::FD_SOCKET: {
- if (static_cast<SOCKET>(right) == INVALID_SOCKET) {
- return left.crt() < -1;
- }
- if (left.crt() < 0) {
- return true;
- }
- return static_cast<SOCKET>(left.crt()) < static_cast<SOCKET>(right);
- }
- }
+ stream << fd.type() << "=";
+ switch (fd.type()) {
+ case WindowsFD::Type::HANDLE: {
+ stream << static_cast<HANDLE>(fd);
+ return stream;
}
- case WindowsFD::FD_HANDLE: {
- switch (right.type()) {
- case WindowsFD::FD_CRT: {
- if (static_cast<HANDLE>(left) == INVALID_HANDLE_VALUE) {
- return -1 < right.crt();
- }
- if (right.crt() < 0) {
- return false;
- }
-#pragma warning(push)
-#pragma warning(disable : 4312)
- // Disable `int`-to-`HANDLE` compiler warning. This is safe to do,
- // see comment above.
- return static_cast<HANDLE>(left) <
- reinterpret_cast<HANDLE>(right.crt());
-#pragma warning(pop)
- }
- case WindowsFD::FD_HANDLE: {
- return static_cast<HANDLE>(left) < static_cast<HANDLE>(right);
- }
- case WindowsFD::FD_SOCKET: {
- return static_cast<HANDLE>(left) <
- reinterpret_cast<HANDLE>(static_cast<SOCKET>(right));
- }
- }
+ case WindowsFD::Type::SOCKET: {
+ stream << static_cast<SOCKET>(fd);
+ return stream;
}
- case WindowsFD::FD_SOCKET: {
- switch (right.type()) {
- case WindowsFD::FD_CRT: {
- if (static_cast<SOCKET>(left) == INVALID_SOCKET) {
- return -1 < right.crt();
- }
- if (right.crt() < 0) {
- return false;
- }
- return static_cast<SOCKET>(left) < static_cast<SOCKET>(right.crt());
- }
- case WindowsFD::FD_HANDLE: {
- return reinterpret_cast<HANDLE>(static_cast<SOCKET>(left)) <
- static_cast<HANDLE>(right);
- }
- case WindowsFD::FD_SOCKET: {
- return static_cast<SOCKET>(left) < static_cast<SOCKET>(right);
- }
- }
+ default: {
+ stream << "UNKNOWN";
+ return stream;
}
}
- UNREACHABLE();
}
+// NOTE: The following operators implement all the comparisons
+// possible a `WindowsFD` type and an `int`. The point of this is that
+// the `WindowsFD` type must act like an `int` for compatibility
+// reasons (e.g. checking validity through `fd < 0`), without actually
+// being castable to an `int` to avoid ambiguous types.
inline bool operator<(int left, const WindowsFD& right)
{
- return WindowsFD(left) < right;
+ return left < right.get_valid();
}
inline bool operator<(const WindowsFD& left, int right)
{
- return left < WindowsFD(right);
-}
-
-
-inline bool operator>(const WindowsFD& left, const WindowsFD& right)
-{
- return right < left;
+ return left.get_valid() < right;
}
inline bool operator>(int left, const WindowsFD& right)
{
- return WindowsFD(left) > right;
+ return left > right.get_valid();
}
inline bool operator>(const WindowsFD& left, int right)
{
- return left > WindowsFD(right);
-}
-
-
-inline bool operator<=(const WindowsFD& left, const WindowsFD& right)
-{
- return !(left > right);
+ return left.get_valid() > right;
}
inline bool operator<=(int left, const WindowsFD& right)
{
- return WindowsFD(left) <= right;
+ return left <= right.get_valid();
}
inline bool operator<=(const WindowsFD& left, int right)
{
- return left <= WindowsFD(right);
-}
-
-
-inline bool operator>=(const WindowsFD& left, const WindowsFD& right)
-{
- return !(left < right);
+ return left.get_valid() <= right;
}
inline bool operator>=(int left, const WindowsFD& right)
{
- return WindowsFD(left) >= right;
+ return left >= right.get_valid();
}
inline bool operator>=(const WindowsFD& left, int right)
{
- return left >= WindowsFD(right);
-}
-
-
-// The complexity in this function is due to our effort in trying to support the
-// cases where file descriptors are compared as an `int` on POSIX. For example,
-// we use expressions such as `fd != -1` to check for validity.
-// TODO(mpark): Consider introducing an `is_valid` function for `int_fd`.
-inline bool operator==(const WindowsFD& left, const WindowsFD& right)
-{
- // In general, when compared against a `WindowsFD` in the `FD_CRT`, we map
- // `INVALID_HANDLE_VALUE` and `INVALID_SOCKET` to `-1` before performing the
- // comparison. The check for `< 0` followed by cast to `HANDLE` or `SOCKET` is
- // due to the fact that `HANDLE` and `SOCKET` are both unsigned.
- switch (left.type()) {
- case WindowsFD::FD_CRT: {
- switch (right.type()) {
- case WindowsFD::FD_CRT: {
- return left.crt() == right.crt();
- }
- case WindowsFD::FD_HANDLE: {
- if (static_cast<HANDLE>(right) == INVALID_HANDLE_VALUE) {
- return left.crt() == -1;
- }
- if (left.crt() < 0) {
- return false;
- }
-#pragma warning(push)
-#pragma warning(disable : 4312)
- // Disable `int`-to-`HANDLE` compiler warning. This is safe to do,
- // see comment above.
- return reinterpret_cast<HANDLE>(left.crt()) ==
- static_cast<HANDLE>(right);
-#pragma warning(pop)
- }
- case WindowsFD::FD_SOCKET: {
- if (static_cast<SOCKET>(right) == INVALID_SOCKET) {
- return left.crt() == -1;
- }
- if (left.crt() < 0) {
- return false;
- }
- return static_cast<SOCKET>(left.crt()) == static_cast<SOCKET>(right);
- }
- }
- }
- case WindowsFD::FD_HANDLE: {
- switch (right.type()) {
- case WindowsFD::FD_CRT: {
- if (static_cast<HANDLE>(left) == INVALID_HANDLE_VALUE) {
- return -1 == right.crt();
- }
- if (right.crt() < 0) {
- return false;
- }
-#pragma warning(push)
-#pragma warning(disable : 4312)
- // Disable `int`-to-`HANDLE` compiler warning. This is safe to do,
- // see comment above.
- return static_cast<HANDLE>(left) ==
- reinterpret_cast<HANDLE>(right.crt());
-#pragma warning(pop)
- }
- case WindowsFD::FD_HANDLE: {
- return static_cast<HANDLE>(left) == static_cast<HANDLE>(right);
- }
- case WindowsFD::FD_SOCKET: {
- return static_cast<HANDLE>(left) ==
- reinterpret_cast<HANDLE>(static_cast<SOCKET>(right));
- }
- }
- }
- case WindowsFD::FD_SOCKET: {
- switch (right.type()) {
- case WindowsFD::FD_CRT: {
- if (static_cast<SOCKET>(left) == INVALID_SOCKET) {
- return -1 == right.crt();
- }
- if (right.crt() < 0) {
- return false;
- }
- return static_cast<SOCKET>(left) == static_cast<SOCKET>(right.crt());
- }
- case WindowsFD::FD_HANDLE: {
- return reinterpret_cast<HANDLE>(static_cast<SOCKET>(left)) ==
- static_cast<HANDLE>(right);
- }
- case WindowsFD::FD_SOCKET: {
- return static_cast<SOCKET>(left) == static_cast<SOCKET>(right);
- }
- }
- }
- }
- UNREACHABLE();
+ return left.get_valid() >= right;
}
inline bool operator==(int left, const WindowsFD& right)
{
- return WindowsFD(left) == right;
+ return left == right.get_valid();
}
inline bool operator==(const WindowsFD& left, int right)
{
- return left == WindowsFD(right);
+ return left.get_valid() == right;
}
-inline bool operator!=(const WindowsFD& left, const WindowsFD& right)
+inline bool operator!=(int left, const WindowsFD& right)
{
- return !(left == right);
+ return left != right.get_valid();
}
-inline bool operator!=(int left, const WindowsFD& right)
+inline bool operator!=(const WindowsFD& left, int right)
{
- return WindowsFD(left) != right;
+ return left.get_valid() != right;
}
-inline bool operator!=(const WindowsFD& left, int right)
+// NOTE: This operator exists so that `WindowsFD` can be used in an
+// `unordered_map` (and other STL containers requiring equality).
+inline bool operator==(const WindowsFD& left, const WindowsFD& right)
{
- return left != WindowsFD(right);
+ // This is `true` even if the types mismatch because we want
+ // `WindowsFD(-1)` to compare as equivalent to an invalid `HANDLE`
+ // or `SOCKET`, even though it is technically of type `HANDLE`.
+ if (!left.is_valid() && !right.is_valid()) {
+ return true;
+ }
+
+ // Otherwise mismatched types are not equivalent.
+ if (left.type() != right.type()) {
+ return false;
+ }
+
+ switch (left.type()) {
+ case WindowsFD::Type::HANDLE: {
+ return static_cast<HANDLE>(left) == static_cast<HANDLE>(right);
+ }
+ case WindowsFD::Type::SOCKET: {
+ return static_cast<SOCKET>(left) == static_cast<SOCKET>(right);
+ }
+ }
+
+ UNREACHABLE();
}
} // namespace os {
namespace std {
+// NOTE: This specialization exists so that `WindowsFD` can be used in
+// an `unordered_map` (and other STL containers requiring a hash).
template <>
struct hash<os::WindowsFD>
{
using argument_type = os::WindowsFD;
using result_type = size_t;
- result_type operator()(const argument_type& fd) const
+ result_type operator()(const argument_type& fd) const noexcept
{
switch (fd.type()) {
- case os::WindowsFD::FD_CRT: {
- return static_cast<result_type>(fd.crt());
+ case os::WindowsFD::Type::HANDLE: {
+ return std::hash<HANDLE>{}(static_cast<HANDLE>(fd));
}
- case os::WindowsFD::FD_HANDLE: {
- return reinterpret_cast<result_type>(static_cast<HANDLE>(fd));
- }
- case os::WindowsFD::FD_SOCKET: {
- return static_cast<result_type>(static_cast<SOCKET>(fd));
+ case os::WindowsFD::Type::SOCKET: {
+ return std::hash<SOCKET>{}(static_cast<SOCKET>(fd));
}
}
+
UNREACHABLE();
}
};
http://git-wip-us.apache.org/repos/asf/mesos/blob/d0b055b0/3rdparty/stout/include/stout/os/windows/pipe.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/os/windows/pipe.hpp b/3rdparty/stout/include/stout/os/windows/pipe.hpp
index 8ea89b9..a3574fd 100644
--- a/3rdparty/stout/include/stout/os/windows/pipe.hpp
+++ b/3rdparty/stout/include/stout/os/windows/pipe.hpp
@@ -45,7 +45,7 @@ inline Try<std::array<int_fd, 2>> pipe()
return WindowsError();
}
- return std::array<int_fd, 2>{read_handle, write_handle};
+ return std::array<int_fd, 2>{int_fd(read_handle), int_fd(write_handle)};
}
} // namespace os {
http://git-wip-us.apache.org/repos/asf/mesos/blob/d0b055b0/3rdparty/stout/include/stout/os/windows/read.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/os/windows/read.hpp b/3rdparty/stout/include/stout/os/windows/read.hpp
index 8f789de..e957da8 100644
--- a/3rdparty/stout/include/stout/os/windows/read.hpp
+++ b/3rdparty/stout/include/stout/os/windows/read.hpp
@@ -27,11 +27,7 @@ inline ssize_t read(const int_fd& fd, void* data, size_t size)
CHECK_LE(size, UINT_MAX);
switch (fd.type()) {
- // TODO(andschwa): Remove this when `FD_CRT` is removed, MESOS-8675.
- case WindowsFD::FD_CRT: {
- return ::_read(fd.crt(), data, static_cast<unsigned int>(size));
- }
- case WindowsFD::FD_HANDLE: {
+ case WindowsFD::Type::HANDLE: {
DWORD bytes;
// TODO(andschwa): Handle overlapped I/O.
const BOOL result =
@@ -47,7 +43,7 @@ inline ssize_t read(const int_fd& fd, void* data, size_t size)
return static_cast<ssize_t>(bytes);
}
- case WindowsFD::FD_SOCKET: {
+ case WindowsFD::Type::SOCKET: {
return ::recv(fd, (char*)data, static_cast<unsigned int>(size), 0);
}
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/d0b055b0/3rdparty/stout/include/stout/os/windows/socket.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/os/windows/socket.hpp b/3rdparty/stout/include/stout/os/windows/socket.hpp
index a05b0e2..4a8f52b 100644
--- a/3rdparty/stout/include/stout/os/windows/socket.hpp
+++ b/3rdparty/stout/include/stout/os/windows/socket.hpp
@@ -132,7 +132,7 @@ inline Try<int_fd> socket(
inline int_fd accept(
const int_fd& fd, sockaddr* addr, socklen_t* addrlen)
{
- return ::accept(fd, addr, reinterpret_cast<int*>(addrlen));
+ return int_fd(::accept(fd, addr, reinterpret_cast<int*>(addrlen)));
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/d0b055b0/3rdparty/stout/include/stout/os/windows/write.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/os/windows/write.hpp b/3rdparty/stout/include/stout/os/windows/write.hpp
index 982d084..295c031 100644
--- a/3rdparty/stout/include/stout/os/windows/write.hpp
+++ b/3rdparty/stout/include/stout/os/windows/write.hpp
@@ -28,11 +28,7 @@ inline ssize_t write(const int_fd& fd, const void* data, size_t size)
CHECK_LE(size, INT_MAX);
switch (fd.type()) {
- // TODO(andschwa): Remove this when `FD_CRT` is removed, MESOS-8675.
- case WindowsFD::FD_CRT: {
- return ::_write(fd.crt(), data, static_cast<unsigned int>(size));
- }
- case WindowsFD::FD_HANDLE: {
+ case WindowsFD::Type::HANDLE: {
DWORD bytes;
// TODO(andschwa): Handle overlapped I/O.
const BOOL result =
@@ -43,7 +39,7 @@ inline ssize_t write(const int_fd& fd, const void* data, size_t size)
return static_cast<ssize_t>(bytes);
}
- case WindowsFD::FD_SOCKET: {
+ case WindowsFD::Type::SOCKET: {
return ::send(fd, (const char*)data, static_cast<int>(size), 0);
}
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/d0b055b0/3rdparty/stout/tests/os/filesystem_tests.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/tests/os/filesystem_tests.cpp b/3rdparty/stout/tests/os/filesystem_tests.cpp
index c190baa..d458f1c 100644
--- a/3rdparty/stout/tests/os/filesystem_tests.cpp
+++ b/3rdparty/stout/tests/os/filesystem_tests.cpp
@@ -217,10 +217,13 @@ TEST_F(FsTest, WindowsInternalLongPath)
EXPECT_EQ(longpath(os::LONGPATH_PREFIX + path),
wide_stringify(os::LONGPATH_PREFIX + path));
}
+#endif // __WINDOWS__
// This test attempts to perform some basic file operations on a file
// with an absolute path at exactly the internal `MAX_PATH` of 248.
+//
+// NOTE: This tests an edge case on Windows, but is a cross-platform test.
TEST_F(FsTest, CreateDirectoryAtMaxPath)
{
const size_t max_path_length = 248;
@@ -242,14 +245,17 @@ TEST_F(FsTest, CreateDirectoryAtMaxPath)
// This test attempts to perform some basic file operations on a file
// with an absolute path longer than the `MAX_PATH`.
+//
+// NOTE: This tests an edge case on Windows, but is a cross-platform test.
TEST_F(FsTest, CreateDirectoryLongerThanMaxPath)
{
string testdir = sandbox.get();
- while (testdir.length() <= MAX_PATH) {
+ const size_t max_path_length = 260;
+ while (testdir.length() <= max_path_length) {
testdir = path::join(testdir, id::UUID::random().toString());
}
- EXPECT_TRUE(testdir.length() > MAX_PATH);
+ EXPECT_TRUE(testdir.length() > max_path_length);
ASSERT_SOME(os::mkdir(testdir));
const string testfile = path::join(testdir, "file.txt");
@@ -262,41 +268,23 @@ TEST_F(FsTest, CreateDirectoryLongerThanMaxPath)
// This test ensures that `os::realpath` will work on open files.
+//
+// NOTE: This tests an edge case on Windows, but is a cross-platform test.
TEST_F(FsTest, RealpathValidationOnOpenFile)
{
// Open a file to write, with "SHARE" read/write permissions,
// then call `os::realpath` on that file.
const string file = path::join(sandbox.get(), id::UUID::random().toString());
- const string data = "data";
-
- const SharedHandle handle(
- ::CreateFileW(
- wide_stringify(file).data(),
- FILE_APPEND_DATA,
- FILE_SHARE_READ | FILE_SHARE_WRITE,
- nullptr, // No inheritance.
- CREATE_ALWAYS,
- FILE_ATTRIBUTE_NORMAL,
- nullptr), // No template file.
- ::CloseHandle);
- EXPECT_NE(handle.get_handle(), INVALID_HANDLE_VALUE);
-
- DWORD bytes_written;
- BOOL written = ::WriteFile(
- handle.get_handle(),
- data.c_str(),
- static_cast<DWORD>(data.size()),
- &bytes_written,
- nullptr); // No overlapped I/O.
- EXPECT_EQ(written, TRUE);
- EXPECT_EQ(data.size(), bytes_written);
+ const Try<int_fd> fd = os::open(file, O_CREAT | O_RDWR);
+ ASSERT_SOME(fd);
+ EXPECT_SOME(os::write(fd.get(), "data"));
// Verify that `os::realpath` (which calls `CreateFileW` on Windows) is
// successful even though the file is open elsewhere.
EXPECT_SOME_EQ(file, os::realpath(file));
+ EXPECT_SOME(os::close(fd.get()));
}
-#endif // __WINDOWS__
TEST_F(FsTest, SYMLINK_Symlink)
@@ -475,15 +463,22 @@ TEST_F(FsTest, Rename)
}
-TEST_F(FsTest, Close)
-{
#ifdef __WINDOWS__
- // On Windows, CRT functions like `_close` will cause an assert dialog box
- // to pop up if you pass them a bad file descriptor. For this test, we prefer
- // to just have the functions error out.
- const int previous_report_mode = _CrtSetReportMode(_CRT_ASSERT, 0);
+TEST_F(FsTest, IntFD)
+{
+ const int_fd fd(INVALID_HANDLE_VALUE);
+ EXPECT_EQ(int_fd::Type::HANDLE, fd.type());
+ EXPECT_FALSE(fd.is_valid());
+ EXPECT_EQ(fd, int_fd(-1));
+ EXPECT_EQ(-1, fd);
+ EXPECT_LT(fd, 0);
+ EXPECT_GT(0, fd);
+}
#endif // __WINDOWS__
+
+TEST_F(FsTest, Close)
+{
const string testfile =
path::join(os::getcwd(), id::UUID::random().toString());
@@ -495,70 +490,32 @@ TEST_F(FsTest, Close)
// Open a file, and verify that writing to that file descriptor succeeds
// before we close it, and fails after.
- const Try<int_fd> open_valid_fd = os::open(testfile, O_RDWR);
- ASSERT_SOME(open_valid_fd);
- ASSERT_SOME(os::write(open_valid_fd.get(), test_message1));
+ const Try<int_fd> fd = os::open(testfile, O_CREAT | O_RDWR);
+ ASSERT_SOME(fd);
+#ifdef __WINDOWS__
+ ASSERT_EQ(fd->type(), os::WindowsFD::Type::HANDLE);
+ ASSERT_TRUE(fd->is_valid());
+#endif // __WINDOWS__
- EXPECT_SOME(os::close(open_valid_fd.get()));
+ ASSERT_SOME(os::write(fd.get(), test_message1));
- EXPECT_ERROR(os::write(open_valid_fd.get(), error_message));
+ EXPECT_SOME(os::close(fd.get()));
- const Result<string> read_valid_fd = os::read(testfile);
- EXPECT_SOME(read_valid_fd);
- ASSERT_EQ(test_message1, read_valid_fd.get());
+ EXPECT_ERROR(os::write(fd.get(), error_message));
-#ifdef __WINDOWS__
- // Open a file with the traditional Windows `HANDLE` API, then verify that
- // writing to that `HANDLE` succeeds before we close it, and fails after.
- const HANDLE open_valid_handle = CreateFileW(
- wide_stringify(testfile).data(),
- FILE_APPEND_DATA,
- 0, // No sharing mode.
- nullptr, // Default security.
- OPEN_EXISTING, // Open only if it exists.
- FILE_ATTRIBUTE_NORMAL, // Open a normal file.
- nullptr); // No attribute tempate file.
- ASSERT_NE(INVALID_HANDLE_VALUE, open_valid_handle);
-
- DWORD bytes_written;
- BOOL written = WriteFile(
- open_valid_handle,
- test_message1.c_str(), // Data to write.
- static_cast<DWORD>(test_message1.size()), // Bytes to write.
- &bytes_written, // Bytes written.
- nullptr); // No overlapped I/O.
- ASSERT_TRUE(written == TRUE);
- ASSERT_EQ(test_message1.size(), bytes_written);
-
- EXPECT_SOME(os::close(open_valid_handle));
-
- written = WriteFile(
- open_valid_handle,
- error_message.c_str(), // Data to write.
- static_cast<DWORD>(error_message.size()), // Bytes to write.
- &bytes_written, // Bytes written.
- nullptr); // No overlapped I/O.
- ASSERT_TRUE(written == FALSE);
- ASSERT_EQ(0, bytes_written);
-
- const Result<string> read_valid_handle = os::read(testfile);
- EXPECT_SOME(read_valid_handle);
- ASSERT_EQ(test_message1 + test_message1, read_valid_handle.get());
-#endif // __WINDOWS__
+ const Result<string> read = os::read(testfile);
+ EXPECT_SOME(read);
+ ASSERT_EQ(test_message1, read.get());
// Try `close` with invalid file descriptor.
+ // NOTE: This should work on both Windows and POSIX because the implicit
+ // conversion to `int_fd` maps `-1` to `INVALID_HANDLE_VALUE` on Windows.
EXPECT_ERROR(os::close(static_cast<int>(-1)));
#ifdef __WINDOWS__
- // Try `close` with invalid `SOCKET` and `HANDLE`.
- EXPECT_ERROR(os::close(static_cast<SOCKET>(INVALID_SOCKET)));
- EXPECT_ERROR(os::close(INVALID_SOCKET));
- EXPECT_ERROR(os::close(static_cast<HANDLE>(open_valid_handle)));
-#endif // __WINDOWS__
-
-#ifdef __WINDOWS__
- // Reset the CRT assert dialog settings.
- _CrtSetReportMode(_CRT_ASSERT, previous_report_mode);
+ // Try `close` with invalid `HANDLE` and `SOCKET`.
+ EXPECT_ERROR(os::close(int_fd(INVALID_HANDLE_VALUE)));
+ EXPECT_ERROR(os::close(int_fd(INVALID_SOCKET)));
#endif // __WINDOWS__
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/d0b055b0/3rdparty/stout/tests/os/socket_tests.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/tests/os/socket_tests.cpp b/3rdparty/stout/tests/os/socket_tests.cpp
index 8ea0f12..9ca236c 100644
--- a/3rdparty/stout/tests/os/socket_tests.cpp
+++ b/3rdparty/stout/tests/os/socket_tests.cpp
@@ -10,6 +10,7 @@
// See the License for the specific language governing permissions and
// limitations under the License
+#include <stout/os/int_fd.hpp>
#include <stout/os/socket.hpp>
#include <stout/tests/utils.hpp>
@@ -31,4 +32,16 @@ TEST_F(SocketTests, InitSocket)
ASSERT_TRUE(net::wsa_cleanup());
ASSERT_TRUE(net::wsa_cleanup());
}
+
+
+TEST_F(SocketTests, IntFD)
+{
+ const int_fd fd(INVALID_SOCKET);
+ EXPECT_EQ(int_fd::Type::SOCKET, fd.type());
+ EXPECT_FALSE(fd.is_valid());
+ EXPECT_EQ(fd, int_fd(-1));
+ EXPECT_EQ(-1, fd);
+ EXPECT_LT(fd, 0);
+ EXPECT_GT(0, fd);
+}
#endif // __WINDOWS__