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 <[email protected]> Authored: Tue Mar 20 20:22:49 2018 -0700 Committer: Andrew Schwartzmeyer <[email protected]> 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__
