From 2910aa77b2feea39fc0618598e275b138a346e71 Mon Sep 17 00:00:00 2001
From: comex <comexk@gmail.com>
Date: Sun, 24 Jan 2021 15:17:02 -0500
Subject: [PATCH] [network] Error handling reform
`network.cpp` has several error paths which either:
- report "Unhandled host socket error=n" and return `SUCCESS`, or
- switch on a few possible errors, log them, and translate them to
Errno; the same switch statement is copied and pasted in multiple
places in the code
Convert these paths to use a helper function `GetAndLogLastError`, which
is roughly the equivalent of one of the switch statements, but:
- handling more cases (both ones that were already in `Errno`, and a few
more I added), and
- using OS functions to convert the error to a string when logging, so
it'll describe the error even if it's not one of the ones in the
switch statement.
- To handle this, refactor the logic in `GetLastErrorMsg` to expose a
new function `NativeErrorToString` which takes the error number
explicitly as an argument. And improve the Windows version a bit.
Also, add a test which exercises two random error paths.
---
src/common/common_funcs.h | 6 +-
src/common/misc.cpp | 44 +++++---
src/core/network/network.cpp | 173 +++++++++++++----------------
src/core/network/network.h | 6 +
src/tests/CMakeLists.txt | 1 +
src/tests/core/network/network.cpp | 28 +++++
6 files changed, 147 insertions(+), 111 deletions(-)
create mode 100644 src/tests/core/network/network.cpp
diff --git a/src/common/common_funcs.h b/src/common/common_funcs.h
index 71b64e32a3..4ace2cd339 100644
--- a/src/common/common_funcs.h
+++ b/src/common/common_funcs.h
@@ -52,9 +52,13 @@ __declspec(dllimport) void __stdcall DebugBreak(void);
// Generic function to get last error message.
// Call directly after the command or use the error num.
// This function might change the error code.
-// Defined in Misc.cpp.
+// Defined in misc.cpp.
[[nodiscard]] std::string GetLastErrorMsg();
+// Like GetLastErrorMsg(), but passing an explicit error code.
+// Defined in misc.cpp.
+[[nodiscard]] std::string NativeErrorToString(int e);
+
#define DECLARE_ENUM_FLAG_OPERATORS(type) \
[[nodiscard]] constexpr type operator|(type a, type b) noexcept { \
using T = std::underlying_type_t<type>; \
diff --git a/src/common/misc.cpp b/src/common/misc.cpp
index 1d5393597c..495385b9e3 100644
--- a/src/common/misc.cpp
+++ b/src/common/misc.cpp
@@ -12,27 +12,41 @@
#include "common/common_funcs.h"
-// Generic function to get last error message.
-// Call directly after the command or use the error num.
-// This function might change the error code.
-std::string GetLastErrorMsg() {
- static constexpr std::size_t buff_size = 255;
- char err_str[buff_size];
-
+std::string NativeErrorToString(int e) {
#ifdef _WIN32
- FormatMessageA(FORMAT_MESSAGE_FROM_SYSTEM, nullptr, GetLastError(),
- MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), err_str, buff_size, nullptr);
- return std::string(err_str, buff_size);
-#elif defined(__GLIBC__) && (_GNU_SOURCE || (_POSIX_C_SOURCE < 200112L && _XOPEN_SOURCE < 600))
+ LPSTR err_str;
+
+ DWORD res = FormatMessageA(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_ALLOCATE_BUFFER |
+ FORMAT_MESSAGE_IGNORE_INSERTS,
+ nullptr, e, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+ reinterpret_cast<LPSTR>(&err_str), 1, nullptr);
+ if (!res) {
+ return "(FormatMessageA failed to format error)";
+ }
+ std::string ret(err_str);
+ LocalFree(err_str);
+ return ret;
+#else
+ char err_str[255];
+#if defined(__GLIBC__) && (_GNU_SOURCE || (_POSIX_C_SOURCE < 200112L && _XOPEN_SOURCE < 600))
// Thread safe (GNU-specific)
- const char* str = strerror_r(errno, err_str, buff_size);
+ const char* str = strerror_r(e, err_str, sizeof(err_str));
return std::string(str);
#else
// Thread safe (XSI-compliant)
- const int success = strerror_r(errno, err_str, buff_size);
- if (success != 0) {
- return {};
+ int second_err = strerror_r(e, err_str, sizeof(err_str));
+ if (second_err != 0) {
+ return "(strerror_r failed to format error)";
}
return std::string(err_str);
+#endif // GLIBC etc.
+#endif // _WIN32
+}
+
+std::string GetLastErrorMsg() {
+#ifdef _WIN32
+ return NativeErrorToString(GetLastError());
+#else
+ return NativeErrorToString(errno);
#endif
}
diff --git a/src/core/network/network.cpp b/src/core/network/network.cpp
index 681e934688..526bfa1108 100644
--- a/src/core/network/network.cpp
+++ b/src/core/network/network.cpp
@@ -7,6 +7,7 @@
#include <limits>
#include <utility>
#include <vector>
+#include "common/common_funcs.h"
#ifdef _WIN32
#define _WINSOCK_DEPRECATED_NO_WARNINGS // gethostname
@@ -90,15 +91,36 @@ LINGER MakeLinger(bool enable, u32 linger_value) {
return value;
}
-int LastError() {
- return WSAGetLastError();
-}
-
bool EnableNonBlock(SOCKET fd, bool enable) {
u_long value = enable ? 1 : 0;
return ioctlsocket(fd, FIONBIO, &value) != SOCKET_ERROR;
}
+Errno TranslateNativeError(int e) {
+ switch (e) {
+ case WSAEBADF:
+ return Errno::BADF;
+ case WSAEINVAL:
+ return Errno::INVAL;
+ case WSAEMFILE:
+ return Errno::MFILE;
+ case WSAENOTCONN:
+ return Errno::NOTCONN;
+ case WSAEWOULDBLOCK:
+ return Errno::AGAIN;
+ case WSAECONNREFUSED:
+ return Errno::CONNREFUSED;
+ case WSAEHOSTUNREACH:
+ return Errno::HOSTUNREACH;
+ case WSAENETDOWN:
+ return Errno::NETDOWN;
+ case WSAENETUNREACH:
+ return Errno::NETUNREACH;
+ default:
+ return Errno::OTHER;
+ }
+}
+
#elif YUZU_UNIX // ^ _WIN32 v YUZU_UNIX
using SOCKET = int;
@@ -108,9 +130,6 @@ using ULONG = u64;
constexpr SOCKET INVALID_SOCKET = -1;
constexpr SOCKET SOCKET_ERROR = -1;
-constexpr int WSAEWOULDBLOCK = EAGAIN;
-constexpr int WSAENOTCONN = ENOTCONN;
-
constexpr int SD_RECEIVE = SHUT_RD;
constexpr int SD_SEND = SHUT_WR;
constexpr int SD_BOTH = SHUT_RDWR;
@@ -162,10 +181,6 @@ linger MakeLinger(bool enable, u32 linger_value) {
return value;
}
-int LastError() {
- return errno;
-}
-
bool EnableNonBlock(int fd, bool enable) {
int flags = fcntl(fd, F_GETFD);
if (flags == -1) {
@@ -179,8 +194,43 @@ bool EnableNonBlock(int fd, bool enable) {
return fcntl(fd, F_SETFD, flags) == 0;
}
+Errno TranslateNativeError(int e) {
+ switch (e) {
+ case EBADF:
+ return Errno::BADF;
+ case EINVAL:
+ return Errno::INVAL;
+ case EMFILE:
+ return Errno::MFILE;
+ case ENOTCONN:
+ return Errno::NOTCONN;
+ case EAGAIN:
+ return Errno::AGAIN;
+ case ECONNREFUSED:
+ return Errno::CONNREFUSED;
+ case EHOSTUNREACH:
+ return Errno::HOSTUNREACH;
+ case ENETDOWN:
+ return Errno::NETDOWN;
+ case ENETUNREACH:
+ return Errno::NETUNREACH;
+ default:
+ return Errno::OTHER;
+ }
+}
+
#endif
+Errno GetAndLogLastError() {
+#ifdef _WIN32
+ int e = WSAGetLastError();
+#else
+ int e = errno;
+#endif
+ LOG_ERROR(Network, "Socket operation error: {}", NativeErrorToString(e));
+ return TranslateNativeError(e);
+}
+
int TranslateDomain(Domain domain) {
switch (domain) {
case Domain::INET:
@@ -290,9 +340,7 @@ Errno SetSockOpt(SOCKET fd, int option, T value) {
if (result != SOCKET_ERROR) {
return Errno::SUCCESS;
}
- const int ec = LastError();
- UNREACHABLE_MSG("Unhandled host socket error={}", ec);
- return Errno::SUCCESS;
+ return GetAndLogLastError();
}
} // Anonymous namespace
@@ -308,14 +356,12 @@ NetworkInstance::~NetworkInstance() {
std::pair<IPv4Address, Errno> GetHostIPv4Address() {
std::array<char, 256> name{};
if (gethostname(name.data(), static_cast<int>(name.size()) - 1) == SOCKET_ERROR) {
- UNIMPLEMENTED_MSG("Unhandled gethostname error");
- return {IPv4Address{}, Errno::SUCCESS};
+ return {IPv4Address{}, GetAndLogLastError()};
}
hostent* const ent = gethostbyname(name.data());
if (!ent) {
- UNIMPLEMENTED_MSG("Unhandled gethostbyname error");
- return {IPv4Address{}, Errno::SUCCESS};
+ return {IPv4Address{}, GetAndLogLastError()};
}
if (ent->h_addr_list == nullptr) {
UNIMPLEMENTED_MSG("No addr provided in hostent->h_addr_list");
@@ -359,9 +405,7 @@ std::pair<s32, Errno> Poll(std::vector<PollFD>& pollfds, s32 timeout) {
ASSERT(result == SOCKET_ERROR);
- const int ec = LastError();
- UNREACHABLE_MSG("Unhandled host socket error={}", ec);
- return {-1, Errno::SUCCESS};
+ return {-1, GetAndLogLastError()};
}
Socket::~Socket() {
@@ -380,9 +424,7 @@ Errno Socket::Initialize(Domain domain, Type type, Protocol protocol) {
return Errno::SUCCESS;
}
- const int ec = LastError();
- UNREACHABLE_MSG("Unhandled host socket error={}", ec);
- return Errno::SUCCESS;
+ return GetAndLogLastError();
}
std::pair<Socket::AcceptResult, Errno> Socket::Accept() {
@@ -391,9 +433,7 @@ std::pair<Socket::AcceptResult, Errno> Socket::Accept() {
const SOCKET new_socket = accept(fd, &addr, &addrlen);
if (new_socket == INVALID_SOCKET) {
- const int ec = LastError();
- UNREACHABLE_MSG("Unhandled host socket error={}", ec);
- return {AcceptResult{}, Errno::SUCCESS};
+ return {AcceptResult{}, GetAndLogLastError()};
}
AcceptResult result;
@@ -412,23 +452,14 @@ Errno Socket::Connect(SockAddrIn addr_in) {
return Errno::SUCCESS;
}
- switch (const int ec = LastError()) {
- case WSAEWOULDBLOCK:
- LOG_DEBUG(Service, "EAGAIN generated");
- return Errno::AGAIN;
- default:
- UNREACHABLE_MSG("Unhandled host socket error={}", ec);
- return Errno::SUCCESS;
- }
+ return GetAndLogLastError();
}
std::pair<SockAddrIn, Errno> Socket::GetPeerName() {
sockaddr addr;
socklen_t addrlen = sizeof(addr);
if (getpeername(fd, &addr, &addrlen) == SOCKET_ERROR) {
- const int ec = LastError();
- UNREACHABLE_MSG("Unhandled host socket error={}", ec);
- return {SockAddrIn{}, Errno::SUCCESS};
+ return {SockAddrIn{}, GetAndLogLastError()};
}
ASSERT(addrlen == sizeof(sockaddr_in));
@@ -439,9 +470,7 @@ std::pair<SockAddrIn, Errno> Socket::GetSockName() {
sockaddr addr;
socklen_t addrlen = sizeof(addr);
if (getsockname(fd, &addr, &addrlen) == SOCKET_ERROR) {
- const int ec = LastError();
- UNREACHABLE_MSG("Unhandled host socket error={}", ec);
- return {SockAddrIn{}, Errno::SUCCESS};
+ return {SockAddrIn{}, GetAndLogLastError()};
}
ASSERT(addrlen == sizeof(sockaddr_in));
@@ -454,9 +483,7 @@ Errno Socket::Bind(SockAddrIn addr) {
return Errno::SUCCESS;
}
- const int ec = LastError();
- UNREACHABLE_MSG("Unhandled host socket error={}", ec);
- return Errno::SUCCESS;
+ return GetAndLogLastError();
}
Errno Socket::Listen(s32 backlog) {
@@ -464,9 +491,7 @@ Errno Socket::Listen(s32 backlog) {
return Errno::SUCCESS;
}
- const int ec = LastError();
- UNREACHABLE_MSG("Unhandled host socket error={}", ec);
- return Errno::SUCCESS;
+ return GetAndLogLastError();
}
Errno Socket::Shutdown(ShutdownHow how) {
@@ -489,14 +514,7 @@ Errno Socket::Shutdown(ShutdownHow how) {
return Errno::SUCCESS;
}
- switch (const int ec = LastError()) {
- case WSAENOTCONN:
- LOG_ERROR(Service, "ENOTCONN generated");
- return Errno::NOTCONN;
- default:
- UNREACHABLE_MSG("Unhandled host socket error={}", ec);
- return Errno::SUCCESS;
- }
+ return GetAndLogLastError();
}
std::pair<s32, Errno> Socket::Recv(int flags, std::vector<u8>& message) {
@@ -509,17 +527,7 @@ std::pair<s32, Errno> Socket::Recv(int flags, std::vector<u8>& message) {
return {static_cast<s32>(result), Errno::SUCCESS};
}
- switch (const int ec = LastError()) {
- case WSAEWOULDBLOCK:
- LOG_DEBUG(Service, "EAGAIN generated");
- return {-1, Errno::AGAIN};
- case WSAENOTCONN:
- LOG_ERROR(Service, "ENOTCONN generated");
- return {-1, Errno::NOTCONN};
- default:
- UNREACHABLE_MSG("Unhandled host socket error={}", ec);
- return {0, Errno::SUCCESS};
- }
+ return {-1, GetAndLogLastError()};
}
std::pair<s32, Errno> Socket::RecvFrom(int flags, std::vector<u8>& message, SockAddrIn* addr) {
@@ -541,17 +549,7 @@ std::pair<s32, Errno> Socket::RecvFrom(int flags, std::vector<u8>& message, Sock
return {static_cast<s32>(result), Errno::SUCCESS};
}
- switch (const int ec = LastError()) {
- case WSAEWOULDBLOCK:
- LOG_DEBUG(Service, "EAGAIN generated");
- return {-1, Errno::AGAIN};
- case WSAENOTCONN:
- LOG_ERROR(Service, "ENOTCONN generated");
- return {-1, Errno::NOTCONN};
- default:
- UNREACHABLE_MSG("Unhandled host socket error={}", ec);
- return {-1, Errno::SUCCESS};
- }
+ return {-1, GetAndLogLastError()};
}
std::pair<s32, Errno> Socket::Send(const std::vector<u8>& message, int flags) {
@@ -564,18 +562,7 @@ std::pair<s32, Errno> Socket::Send(const std::vector<u8>& message, int flags) {
return {static_cast<s32>(result), Errno::SUCCESS};
}
- const int ec = LastError();
- switch (ec) {
- case WSAEWOULDBLOCK:
- LOG_DEBUG(Service, "EAGAIN generated");
- return {-1, Errno::AGAIN};
- case WSAENOTCONN:
- LOG_ERROR(Service, "ENOTCONN generated");
- return {-1, Errno::NOTCONN};
- default:
- UNREACHABLE_MSG("Unhandled host socket error={}", ec);
- return {-1, Errno::SUCCESS};
- }
+ return {-1, GetAndLogLastError()};
}
std::pair<s32, Errno> Socket::SendTo(u32 flags, const std::vector<u8>& message,
@@ -597,9 +584,7 @@ std::pair<s32, Errno> Socket::SendTo(u32 flags, const std::vector<u8>& message,
return {static_cast<s32>(result), Errno::SUCCESS};
}
- const int ec = LastError();
- UNREACHABLE_MSG("Unhandled host socket error={}", ec);
- return {-1, Errno::SUCCESS};
+ return {-1, GetAndLogLastError()};
}
Errno Socket::Close() {
@@ -642,9 +627,7 @@ Errno Socket::SetNonBlock(bool enable) {
if (EnableNonBlock(fd, enable)) {
return Errno::SUCCESS;
}
- const int ec = LastError();
- UNREACHABLE_MSG("Unhandled host socket error={}", ec);
- return Errno::SUCCESS;
+ return GetAndLogLastError();
}
bool Socket::IsOpened() const {
diff --git a/src/core/network/network.h b/src/core/network/network.h
index 76b2821f22..bd30f18993 100644
--- a/src/core/network/network.h
+++ b/src/core/network/network.h
@@ -7,6 +7,7 @@
#include <array>
#include <utility>
+#include "common/common_funcs.h"
#include "common/common_types.h"
namespace Network {
@@ -21,6 +22,11 @@ enum class Errno {
MFILE,
NOTCONN,
AGAIN,
+ CONNREFUSED,
+ HOSTUNREACH,
+ NETDOWN,
+ NETUNREACH,
+ OTHER,
};
/// Address families
diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt
index 4ea0076e96..d875c4fee1 100644
--- a/src/tests/CMakeLists.txt
+++ b/src/tests/CMakeLists.txt
@@ -5,6 +5,7 @@ add_executable(tests
common/param_package.cpp
common/ring_buffer.cpp
core/core_timing.cpp
+ core/network/network.cpp
tests.cpp
video_core/buffer_base.cpp
)
diff --git a/src/tests/core/network/network.cpp b/src/tests/core/network/network.cpp
new file mode 100644
index 0000000000..b21ad8911c
--- /dev/null
+++ b/src/tests/core/network/network.cpp
@@ -0,0 +1,28 @@
+// Copyright 2021 yuzu Emulator Project
+// Licensed under GPLv2 or any later version
+// Refer to the license.txt file included.
+
+#include <catch2/catch.hpp>
+
+#include "core/network/network.h"
+#include "core/network/sockets.h"
+
+TEST_CASE("Network::Errors", "[core]") {
+ Network::NetworkInstance network_instance; // initialize network
+
+ Network::Socket socks[2];
+ for (Network::Socket& sock : socks) {
+ REQUIRE(sock.Initialize(Network::Domain::INET, Network::Type::STREAM,
+ Network::Protocol::TCP) == Network::Errno::SUCCESS);
+ }
+
+ Network::SockAddrIn addr{
+ Network::Domain::INET,
+ {127, 0, 0, 1},
+ 1, // hopefully nobody running this test has something listening on port 1
+ };
+ REQUIRE(socks[0].Connect(addr) == Network::Errno::CONNREFUSED);
+
+ std::vector<u8> message{1, 2, 3, 4};
+ REQUIRE(socks[1].Recv(0, message).second == Network::Errno::NOTCONN);
+}