util: Show descriptive error messages when FileCommit fails #26654

john-moffett · 2022-12-07T18:21:31Z

Only raw 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, #26455 (comment) and another.

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:

bitcoin/src/util/sock.cpp

Line 390 in 1b68094

errmsg = NetworkErrorString(err);

bitcoin/src/util/sock.cpp

Line 272 in 1b68094

throw std::runtime_error(strprintf("send(): %s", NetworkErrorString(err)));

bitcoin/src/netbase.cpp

Lines 515 to 516 in 7e1007a

    
           LogPrintf("Error setting SO_NOSIGPIPE on socket: %s, continuing anyway\n", 
        
                     NetworkErrorString(WSAGetLastError()));

bitcoin/src/init.cpp

Line 164 in 8ccab65

    
           return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), SysErrorString(errno)));

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.

DrahtBot · 2022-12-07T18:21:34Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke
Concept ACK	hebasto

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:

#28075 (util: Remove DirIsWritable, GetUniquePath by MarcoFalke)

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.

fanquake

Somewhat unrelated, but after this change, how often are we using the

#ifdef WIN32
    if (strerror_s(...

case in SysErrorString? Wondering if we can (in general) further consolidate this error logging.

src/util/syserror.cpp

fanquake · 2022-12-08T15:56:17Z

src/util/sock.cpp

@@ -419,18 +419,7 @@ void Sock::Close()
 #ifdef WIN32


Could turn this into:

Suggested change

#ifdef WIN32

std::string NetworkErrorString(int err)

{

#ifdef WIN32

return Win32ErrorString(err);

#else

// On BSD sockets implementations, NetworkErrorString is the same as SysErrorString.

return SysErrorString(err);

#endif

}

john-moffett · 2022-12-08T18:24:50Z

@fanquake Great suggestions, thanks. Will incorporate them all shortly.

Somewhat unrelated, but after this change, how often are we using the
#ifdef WIN32
    if (strerror_s(...
case in SysErrorString? Wondering if we can (in general) further consolidate this error logging.

I don't think in many places. In addition to the potential use of it for fflush in this PR, there's this:

bitcoin/src/init.cpp

Line 164 in 3eaf7be

    
           return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), SysErrorString(errno)));

If I artificially trigger that using the current code (by giving it an impossible path), it shows:

If I substitute it with Win32ErrorString(GetLastError()), I get:

Since the error codes are unrelated (errno and GetLastError() return different actual values), I think it'd be difficult to consolidate effectively.

Potentially, we could have a std::string GetSystemErrorReason() function that called SysErrorString(errno) on non-Win32 and Win32ErrorString(GetLastError()) on Win32, but I'm not certain that (on Win32) every time errno is set that GetLastError() is also set.

hebasto · 2022-12-08T19:10:17Z

Concept ACK.

glozow · 2023-04-25T15:41:13Z

@john-moffett please rebase?

john-moffett · 2023-04-25T20:12:46Z

Thanks for reminding me! Will rebase tomorrow.

maflcko

nice. lgtm, but I didn't look at the windows stuff

maflcko · 2023-06-30T07:09:26Z

src/util/fs_helpers.cpp

        return false;
    }
 #else
    if (fsync(fileno(file)) != 0 && errno != EINVAL) {
-        LogPrintf("%s: fsync failed: %d\n", __func__, errno);
+        LogPrintf("%s: fsync failed: %d\n", __func__, SysErrorString(errno));


Suggested change

LogPrintf("%s: fsync failed: %d\n", __func__, SysErrorString(errno));

LogPrintf("fsync failed: %s\n", SysErrorString(errno));

nits:

Can remove the manual func logging, which isn't needed, because all strings in this function are already unique to the codebase. Also, users can enable the setting which enables function logging, if they want this.

Can change %d to %s, since it is a string. (No behavior difference though)

Only raw errno codes are logged if FileCommit fails. These are implementation-specific, so it makes it harder to debug based on user reports. Instead, use SysErrorString to display both the raw int value and the descriptive message.

GetErrorReason()'s Win32 implementation does the same thing as Win32ErrorString(int err) from syserror.cpp, so call the latter. Also remove now-unnecessary headers from sock.cpp and less verbose handling of #ifdefs.

maflcko · 2023-07-20T06:42:27Z

CI can be ignored.

lgtm ACK 5408a55 💡

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 5408a55fc87350baeae3a32f1003f956d5533a79 💡
Maic+76d/0s9p3mR11fkUMJOSi4Wt//8xLPyWD1i2HpZZ4TuNxB3ZRZR3Aq7+Kk0GonkwwTMVO/8XT4Qm8qLDQ==

…ommit f…

DrahtBot added the Utils/log/libs label Dec 7, 2022

fanquake reviewed Dec 8, 2022

View reviewed changes

DrahtBot mentioned this pull request Jan 11, 2023

refactor: Split util/system into exception, shell, and fs-specific files #25152

Closed

DrahtBot mentioned this pull request Mar 14, 2023

refactor: Extract util/fs from util/system #27254

Merged

DrahtBot added the Needs rebase label Apr 3, 2023

achow101 requested a review from jarolrod April 25, 2023 15:40

john-moffett force-pushed the 2022_12_BetterSystemErrorLogging branch from 0ded69f to 3883afe Compare April 26, 2023 16:40

DrahtBot removed the Needs rebase label Apr 26, 2023

DrahtBot mentioned this pull request May 5, 2023

Remove now-unnecessary poll, fcntl includes from net(base).cpp #27530

Merged

DrahtBot mentioned this pull request Jun 29, 2023

refactor: Open file in FileCommit #28006

Closed

maflcko approved these changes Jun 30, 2023

View reviewed changes

john-moffett added 2 commits June 30, 2023 09:48

Show descriptive error messages when FileCommit fails

c95a443

Only raw errno codes are logged if FileCommit fails. These are implementation-specific, so it makes it harder to debug based on user reports. Instead, use SysErrorString to display both the raw int value and the descriptive message.

Consolidate Win32-specific error formatting

5408a55

GetErrorReason()'s Win32 implementation does the same thing as Win32ErrorString(int err) from syserror.cpp, so call the latter. Also remove now-unnecessary headers from sock.cpp and less verbose handling of #ifdefs.

john-moffett force-pushed the 2022_12_BetterSystemErrorLogging branch from 3883afe to 5408a55 Compare June 30, 2023 13:52

DrahtBot added the CI failed label Jun 30, 2023

DrahtBot mentioned this pull request Jul 14, 2023

util: Remove DirIsWritable, GetUniquePath #28075

Merged

fanquake merged commit ac7c177 into bitcoin:master Jul 20, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 21, 2023

Merge bitcoin#26654: util: Show descriptive error messages when FileC…

97f4bfa

…ommit f…

bitcoin locked and limited conversation to collaborators Jul 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

util: Show descriptive error messages when FileCommit fails #26654

util: Show descriptive error messages when FileCommit fails #26654

Uh oh!

john-moffett commented Dec 7, 2022

Uh oh!

DrahtBot commented Dec 7, 2022 •

edited

Loading

Uh oh!

fanquake left a comment

Uh oh!

Uh oh!

Uh oh!

fanquake Dec 8, 2022

Uh oh!

john-moffett commented Dec 8, 2022

Uh oh!

hebasto commented Dec 8, 2022

Uh oh!

glozow commented Apr 25, 2023

Uh oh!

john-moffett commented Apr 25, 2023

Uh oh!

maflcko left a comment

Uh oh!

maflcko Jun 30, 2023 •

edited

Loading

Uh oh!

maflcko commented Jul 20, 2023

Uh oh!

Uh oh!

	LogPrintf("Error setting SO_NOSIGPIPE on socket: %s, continuing anyway\n",
	NetworkErrorString(WSAGetLastError()));

-#ifdef WIN32
+std::string NetworkErrorString(int err)
+{
+#ifdef WIN32
+    return Win32ErrorString(err);
+#else
+    // On BSD sockets implementations, NetworkErrorString is the same as SysErrorString.
+    return SysErrorString(err);
+#endif
+}

	LogPrintf("%s: fsync failed: %d\n", __func__, SysErrorString(errno));
	LogPrintf("fsync failed: %s\n", SysErrorString(errno));

util: Show descriptive error messages when FileCommit fails #26654

util: Show descriptive error messages when FileCommit fails #26654

Uh oh!

Conversation

john-moffett commented Dec 7, 2022

Uh oh!

DrahtBot commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fanquake Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

john-moffett commented Dec 8, 2022

Uh oh!

hebasto commented Dec 8, 2022

Uh oh!

glozow commented Apr 25, 2023

Uh oh!

john-moffett commented Apr 25, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 20, 2023

Uh oh!

Uh oh!

DrahtBot commented Dec 7, 2022 •

edited

Loading

maflcko Jun 30, 2023 •

edited

Loading