Merge bitcoin/bitcoin#26654: util: Show descriptive error messages wh…

…en FileCommit fails 5408a55 Consolidate Win32-specific error formatting (John Moffett) c95a443 Show descriptive error messages when FileCommit fails (John Moffett) Pull request description: Only raw [`errno`](https://en.cppreference.com/w/cpp/error/errno) int values are logged if `FileCommit` fails. These values are implementation-specific, so it makes it harder to debug based on user reports. For instance, bitcoin/bitcoin#26455 (comment) and [another](https://bitcointalk.org/index.php?topic=5182526.0#:~:text=FileCommit%3A%20FlushFileBuffers%20failed%3A%205). Instead, use `SysErrorString` (or the refactored Windows equivalent `Win32ErrorString`) to display both the raw int value and the descriptive message. All other instances in the code I could find where `errno` or (Windows-only) `GetLastError()`/`WSAGetLastError()` are logged use the full descriptive string. For example: https://github.com/bitcoin/bitcoin/blob/1b680948d43b1d39645b9d839a6fa7c6c1786b51/src/util/sock.cpp#L390 https://github.com/bitcoin/bitcoin/blob/1b680948d43b1d39645b9d839a6fa7c6c1786b51/src/util/sock.cpp#L272 https://github.com/bitcoin/bitcoin/blob/7e1007a3c6c9a921c2b60919b84a60eaabfe1c5d/src/netbase.cpp#L515-L516 https://github.com/bitcoin/bitcoin/blob/8ccab65f289e3cce392cbe01d5fc0e7437f51f1e/src/init.cpp#L164 I refactored the Windows formatting code to put it in `syserror.cpp`, as it's applicable to all Win32 API system errors, not just networking errors. To be clear, the Windows API functions `WSAGetLastError()` and `GetLastError()` are currently [equivalent](https://stackoverflow.com/questions/15586224/is-wsagetlasterror-just-an-alias-for-getlasterror). ACKs for top commit: MarcoFalke: lgtm ACK 5408a55 💡 Tree-SHA512: 3921cbac98bd9edaf84d3dd7a43896c7921f144c8ca2cde9bc96d5fb05281f7c55e7cc99db8debf6203b5f916f053025e4fa741f51458fe2c53bb57b0a781027
bitcoin-core · Jul 20, 2023 · ac7c177 · ac7c177
2 parents 1fde20f + 5408a55
commit ac7c177
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 33 deletions.
diff --git a/src/util/fs.cpp b/src/util/fs.cpp
@@ -81,12 +81,7 @@ bool FileLock::TryLock()
 #else
 
 static std::string GetErrorReason() {
-    wchar_t* err;
-    FormatMessageW(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
-        nullptr, GetLastError(), MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), reinterpret_cast<WCHAR*>(&err), 0, nullptr);
-    std::wstring err_str(err);
-    LocalFree(err);
-    return std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>>().to_bytes(err_str);
+    return Win32ErrorString(GetLastError());
 }
 
 FileLock::FileLock(const fs::path& file)

diff --git a/src/util/fs_helpers.cpp b/src/util/fs_helpers.cpp
@@ -14,6 +14,7 @@
 #include <tinyformat.h>
 #include <util/fs.h>
 #include <util/getuniquepath.h>
+#include <util/syserror.h>
 
 #include <cerrno>
 #include <filesystem>
@@ -120,28 +121,28 @@ std::streampos GetFileSize(const char* path, std::streamsize max)
 bool FileCommit(FILE* file)
 {
     if (fflush(file) != 0) { // harmless if redundantly called
-        LogPrintf("%s: fflush failed: %d\n", __func__, errno);
+        LogPrintf("fflush failed: %s\n", SysErrorString(errno));
         return false;
     }
 #ifdef WIN32
     HANDLE hFile = (HANDLE)_get_osfhandle(_fileno(file));
     if (FlushFileBuffers(hFile) == 0) {
-        LogPrintf("%s: FlushFileBuffers failed: %d\n", __func__, GetLastError());
+        LogPrintf("FlushFileBuffers failed: %s\n", Win32ErrorString(GetLastError()));
         return false;
     }
 #elif defined(MAC_OSX) && defined(F_FULLFSYNC)
     if (fcntl(fileno(file), F_FULLFSYNC, 0) == -1) { // Manpage says "value other than -1" is returned on success
-        LogPrintf("%s: fcntl F_FULLFSYNC failed: %d\n", __func__, errno);
+        LogPrintf("fcntl F_FULLFSYNC failed: %s\n", SysErrorString(errno));
         return false;
     }
 #elif HAVE_FDATASYNC
     if (fdatasync(fileno(file)) != 0 && errno != EINVAL) { // Ignore EINVAL for filesystems that don't support sync
-        LogPrintf("%s: fdatasync failed: %d\n", __func__, errno);
+        LogPrintf("fdatasync failed: %s\n", SysErrorString(errno));
         return false;
     }
 #else
     if (fsync(fileno(file)) != 0 && errno != EINVAL) {
-        LogPrintf("%s: fsync failed: %d\n", __func__, errno);
+        LogPrintf("fsync failed: %s\n", SysErrorString(errno));
         return false;
     }
 #endif

diff --git a/src/util/sock.cpp b/src/util/sock.cpp
@@ -15,11 +15,6 @@
 #include <stdexcept>
 #include <string>
 
-#ifdef WIN32
-#include <codecvt>
-#include <locale>
-#endif
-
 #ifdef USE_POLL
 #include <poll.h>
 #endif
@@ -416,26 +411,12 @@ void Sock::Close()
     m_socket = INVALID_SOCKET;
 }
 
-#ifdef WIN32
 std::string NetworkErrorString(int err)
 {
-    wchar_t buf[256];
-    buf[0] = 0;
-    if(FormatMessageW(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS | FORMAT_MESSAGE_MAX_WIDTH_MASK,
-            nullptr, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
-            buf, ARRAYSIZE(buf), nullptr))
-    {
-        return strprintf("%s (%d)", std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>,wchar_t>().to_bytes(buf), err);
-    }
-    else
-    {
-        return strprintf("Unknown error (%d)", err);
-    }
-}
+#if defined(WIN32)
+    return Win32ErrorString(err);
 #else
-std::string NetworkErrorString(int err)
-{
     // On BSD sockets implementations, NetworkErrorString is the same as SysErrorString.
     return SysErrorString(err);
-}
 #endif
+}
diff --git a/src/util/syserror.cpp b/src/util/syserror.cpp
@@ -12,6 +12,12 @@
 #include <cstring>
 #include <string>
 
+#if defined(WIN32)
+#include <windows.h>
+#include <locale>
+#include <codecvt>
+#endif
+
 std::string SysErrorString(int err)
 {
     char buf[1024];
@@ -33,3 +39,21 @@ std::string SysErrorString(int err)
         return strprintf("Unknown error (%d)", err);
     }
 }
+
+#if defined(WIN32)
+std::string Win32ErrorString(int err)
+{
+    wchar_t buf[256];
+    buf[0] = 0;
+    if(FormatMessageW(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS | FORMAT_MESSAGE_MAX_WIDTH_MASK,
+                       nullptr, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+                       buf, ARRAYSIZE(buf), nullptr))
+    {
+        return strprintf("%s (%d)", std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>,wchar_t>().to_bytes(buf), err);
+    }
+    else
+    {
+        return strprintf("Unknown error (%d)", err);
+    }
+}
+#endif
diff --git a/src/util/syserror.h b/src/util/syserror.h
@@ -13,4 +13,8 @@
  */
 std::string SysErrorString(int err);
 
+#if defined(WIN32)
+std::string Win32ErrorString(int err);
+#endif
+
 #endif // BITCOIN_UTIL_SYSERROR_H