util: Show descriptive error messages when FileCommit fails #26654

1. 
   john-moffett commented at 6:21 PM on December 7, 2022: contributor

   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#issue-1436654238 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:

   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.

2. 
   DrahtBot commented at 6:21 PM on December 7, 2022: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--021abf342d371248e50ceaed478a90ca-->

   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.

   <!--174a7506f384e20aa4161008e828411d-->

   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.

3. DrahtBot added the label Utils/log/libs on Dec 7, 2022
4. in src/util/syserror.cpp:43 in 3b1d9929e7 outdated

     38 | @@ -33,3 +39,21 @@ std::string SysErrorString(int err)
     39 |          return strprintf("Unknown error (%d)", err);
     40 |      }
     41 |  }
     42 | +
     43 | +#if defined(WIN32)
   

   ---

   ---

   

   fanquake commented at 3:49 PM on December 8, 2022:

   If we are going to consolidate this code to syserror, might makes sense to consolidate the very similar GetErrorReason (fs.cpp), at the same time?

5. in src/util/syserror.cpp:15 in 3b1d9929e7 outdated

     11 | @@ -12,6 +12,12 @@
     12 |  #include <cstring>
     13 |  #include <string>
     14 |  
     15 | +#if defined(WIN32)
   

   ---

   ---

   

   fanquake commented at 3:50 PM on December 8, 2022:

   Looks like you can drop these headers out of util/sock once they are moved here?

6. in src/util/sock.cpp:419 in 3b1d9929e7 outdated

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

   ---

   ---

   

   fanquake commented at 3:56 PM on December 8, 2022:

   Could turn this into:

   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
   }
   

7. 
   fanquake commented at 4:03 PM on December 8, 2022: member

   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.

8. 
   john-moffett commented at 6:24 PM on December 8, 2022: contributor

   @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:

   https://github.com/bitcoin/bitcoin/blob/3eaf7be6ade22c99f3c0000122e49b94de868d74/src/init.cpp#L164

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

   <img width="250" alt="image" src="https://user-images.githubusercontent.com/116917595/206523440-32d8edca-54b2-4974-851f-d0077ffae27d.png">

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

   <img width="250" alt="image" src="https://user-images.githubusercontent.com/116917595/206524042-ede807f2-c588-4bc3-8415-5d5e943af95d.png">

   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.

9. 
   hebasto commented at 7:10 PM on December 8, 2022: member

   Concept ACK.

10. DrahtBot added the label Needs rebase on Apr 3, 2023
11. achow101 requested review from jarolrod on Apr 25, 2023
12. 
    glozow commented at 3:41 PM on April 25, 2023: member

    @john-moffett please rebase?

13. 
    john-moffett commented at 8:12 PM on April 25, 2023: contributor

    Thanks for reminding me! Will rebase tomorrow.

14. john-moffett force-pushed on Apr 26, 2023
15. DrahtBot removed the label Needs rebase on Apr 26, 2023
16. in src/util/fs_helpers.cpp:145 in 3883afe457 outdated

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

    ---

    ---

    

    maflcko commented at 7:09 AM on June 30, 2023:

            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)
17. maflcko approved
18. 
    maflcko commented at 7:09 AM on June 30, 2023: member

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

19. Show descriptive error messages when FileCommit fails

    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.

    c95a4432d7
20. Consolidate Win32-specific error formatting

    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.

    5408a55fc8
21. john-moffett force-pushed on Jun 30, 2023
22. DrahtBot added the label CI failed on Jun 30, 2023
23. 
    maflcko commented at 6:42 AM on July 20, 2023: member

    CI can be ignored.

    lgtm ACK 5408a55fc87350baeae3a32f1003f956d5533a79 💡

    <details><summary>Show signature</summary>

    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==
    

    </details>

24. fanquake merged this on Jul 20, 2023
25. fanquake closed this on Jul 20, 2023

    ---

26. sidhujag referenced this in commit 97f4bfad30 on Jul 21, 2023
27. bitcoin locked this on Jul 19, 2024

Review Requested
jarolrod