util: Remove `fsbridge::get_filesystem_error

hebasto commented at 0:18 am on April 30, 2025: member

The fsbridge::get_filesystem_error_message() function exhibits several drawbacks:

It was introduced in #14192 to account for platform-specific variations in boost::filesystem::filesystem_error::what(). Since migrating to std::filesystem, those discrepancies no longer exist.
It fails to display UTF-8 paths correctly on Windows:

0> build\bin\Release\bitcoind.exe -datadir="C:\Users\hebasto\dd_₿_🏃" -regtest
1...
22025-04-30T00:17:48Z DeleteAuthCookie: Unable to remove random auth cookie file: remove: Access is denied.: "C:\Users\hebasto\dd_?_??\regtest\.cookie"
3...

It relies on std::wstring_convert, which was deprecated in C++17 and removed in C++26 (also see #32361).

This PR removes the obsolete fsbridge::get_filesystem_error_message() function, thereby resolving all of the above issues.

hebasto added the label Utils/log/libs on Apr 30, 2025

DrahtBot commented at 0:18 am on April 30, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32383.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, laanwj, davidgumberg, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#28710 (Remove the legacy wallet and BDB dependency by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto commented at 9:24 am on April 30, 2025: member

Friendly ping @ryanofsky @davidgumberg @hodlinator @sipsorcery :)

in src/rpc/request.cpp:173 in 671a9a2587 outdated

169@@ -170,7 +170,7 @@ void DeleteAuthCookie()
170             fs::remove(GetAuthCookieFile());
171         }
172     } catch (const fs::filesystem_error& e) {
173-        LogPrintf("%s: Unable to remove random auth cookie file: %s\n", __func__, fsbridge::get_filesystem_error_message(e));
174+        LogPrintf("%s: Unable to remove random auth cookie file %s: %s\n", __func__, fs::PathToString(e.path1()), e.code().message());

maflcko commented at 9:28 am on April 30, 2025:

0        LogInfo/Warning("Unable to remove random auth cookie file %s: %s\n", fs::PathToString(e.path1()), e.code().message());

Since you are changing the error message anyway, seems fine to remove the redundant __func__. Also, could clarify to info or warning level?

maflcko commented at 9:29 am on April 30, 2025:

(all other messages touched here look like warnings to me, too)

hebasto commented at 9:42 am on April 30, 2025:

Thanks! Updated.

maflcko approved

maflcko commented at 9:31 am on April 30, 2025: member

lgtm ACK 671a9a258705b22b26cb8066fc0e53988c960fc6

The new error messages also read nicer. Left some further nits, while touching those lines.

util: Remove `fsbridge::get_filesystem_error_message()`

The `fsbridge::get_filesystem_error_message()` function exhibits several
drawbacks:

1. It was introduced in https://github.com/bitcoin/bitcoin/pull/14192 to
account for platform-specific variations in
`boost::filesystem::filesystem_error::what()`. Since migrating to
`std::filesystem`, those discrepancies no longer exist.

2. It fails to display UTF-8 paths correctly on Windows.

3. It relies on `std::wstring_convert`, which was deprecated in C++17
and removed in C++26.

This change removes the `fsbridge::get_filesystem_error_message()`
function, thereby resolving all of the above issues.

Additionally, filesystem error messages now use the "Warning" log level.

97eaadc3bf

hebasto force-pushed on Apr 30, 2025

hebasto commented at 9:43 am on April 30, 2025: member

The recent feedback from @maflcko has been addressed.

maflcko commented at 11:04 am on April 30, 2025: member

lgtm re-ACK 97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3

Only change are some minor format cleanups for the error messages.

laanwj approved

laanwj commented at 1:02 pm on April 30, 2025: member

Code review ACK 97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3

This was sure some horrible code, i’m confused what it even does ~~(calling MultiByteToWideChar twice then do another pass of std::codecvt_utf8_utf16 seems way overkill, it)~~ (edit: ok, it does makes sense, it’s still awful though). Glad to replace it with a standardized method.

in src/rpc/request.cpp:173 in 97eaadc3bf

169@@ -170,7 +170,7 @@ void DeleteAuthCookie()
170             fs::remove(GetAuthCookieFile());
171         }
172     } catch (const fs::filesystem_error& e) {
173-        LogPrintf("%s: Unable to remove random auth cookie file: %s\n", __func__, fsbridge::get_filesystem_error_message(e));
174+        LogWarning("Unable to remove random auth cookie file %s: %s\n", fs::PathToString(e.path1()), e.code().message());

davidgumberg commented at 1:13 pm on April 30, 2025:

non blocking nit, I think we can use e.what() in place of e.code().message()

(https://en.cppreference.com/w/cpp/filesystem/filesystem_error/what)

maflcko commented at 1:26 pm on April 30, 2025:

non blocking nit, I think we can use e.what() in place of e.code().message()

This will redundantly include the filenames again, or it may not, so it seems worse?

hebasto commented at 1:31 pm on April 30, 2025:

UTF-8 paths included in e.what() are not displayed correctly on Windows.

laanwj commented at 1:57 pm on April 30, 2025:

UTF-8 paths included in e.what() are not displayed correctly on Windows.

Maybe they will after #32380? In any case, including the filename doesn’t seem necessary.

davidgumberg commented at 1:21 pm on April 30, 2025: contributor

untested crACK https://github.com/bitcoin/bitcoin/pull/32383/commits/97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3

This code made sense when using boost, but I think std::filesystem implementations are responsible for and better equipped to handle platform specific character width issues.

achow101 commented at 5:43 pm on April 30, 2025: member

ACK 97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3

achow101 merged this on Apr 30, 2025

achow101 closed this on Apr 30, 2025

hebasto deleted the branch on Apr 30, 2025

util: Remove fsbridge::get_filesystem_error_message() #32383

Code Coverage & Benchmarks

Reviews

Conflicts

util: Remove `fsbridge::get_filesystem_error_message()` #32383