Replace non-threadsafe strerror gmtime setlocale #4152

1. 
   laanwj commented at 4:12 pm on May 8, 2014: member

   • Log the name of the error as well as the error code if a network problem happens. This makes network troubleshooting more convenient
     • Use thread-safe strerror_r and the WIN32 equivalent FormatMessage.
   • Replace non-threadsafe gmtime and setlocale
     • Make DateTimeStrFormat use boost::posix_time
     • Also re-enable the util_DateTimeStrFormat tests, as they are no longer platform specific
2. laanwj renamed this:
   2014 05 replace strerror gmtime
   Replace non-threadsafe strerror gmtime setlocale
   on May 8, 2014
3. in src/netbase.cpp: in a18b9af3ed outdated

   1143+    /* Too bad there are two incompatible implementations of the
   1144+     * thread-safe strerror. */
   1145+#ifdef STRERROR_R_CHAR_P /* GNU variant can return a pointer */
   1146+    s = strerror_r(err, buf, sizeof(buf));
   1147+#else /* POSIX variant always returns message in buffer */
   1148+    (void) strerror_r(err, buf, sizeof(buf));
   

   ---

   ---

   

   sipa commented at 7:41 pm on May 8, 2014:

   Why can’t we just always use this form?

   ---

   

   luke-jr commented at 7:56 pm on May 8, 2014:

   Should be safe as long as we put something like

   0#if defined(_GNU_SOURCE) || _POSIX_C_SOURCE < 200112L
   1#  error "We might be getting GNU strerror_r here"
   2#endif
   

   ---

   

   laanwj commented at 7:59 pm on May 8, 2014:

   Well I get the GNU form here by default… It would be possible to define _POSIX_C_SOURCE in the configure script, but there is no telling what impact that will have on other functions. Detecting what form to use is the safer way.

   ---

   

   sipa commented at 8:01 pm on May 8, 2014:

   Ah, the documentation clarifies. The GNU version may return something else than buf… puke

   ---

   

   laanwj commented at 8:28 pm on May 8, 2014:

   Exactly, in that case buf will remain empty :(
4. in src/util.cpp: in a18b9af3ed outdated

   1428@@ -1427,3 +1429,12 @@ void RenameThread(const char* name)
   1429 #endif
   1430 }
   1431 
   1432+std::string DateTimeStrFormat(const char* pszFormat, int64_t nTime)
   1433+{
   1434+    // std::locale takes ownership of the pointer
   1435+    std::locale loc(std::locale::classic(), new boost::posix_time::time_facet(pszFormat));
   

   ---

   ---

   

   sipa commented at 7:43 pm on May 8, 2014:

   No need to destruct that new time_facet?

   ---

   

   laanwj commented at 8:00 pm on May 8, 2014:

   The std::locale takes ownership of it and frees it when it goes out of scope

   ---

   

   laanwj commented at 9:37 am on May 9, 2014:

   Granted, it is kind of counter-intuitive, we had quite an ugly issue due to this once where we passed a statically-allocated facet object to std::locale and it got freed at shutdown: a143d4ce58d1103b37fa297fe6f4f4878eea59ca.

   ---

   

   sipa commented at 10:27 am on May 9, 2014:

   I vaguely remember, yes.
5. laanwj added this to the milestone 0.9.2 on May 14, 2014
6. 
   sipa commented at 11:26 am on May 22, 2014: member

   untested ACK
7. in src/netbase.cpp: in b8a03021f1 outdated

   1236@@ -1237,3 +1237,31 @@ bool operator!=(const CSubNet& a, const CSubNet& b)
   1237 {
   1238     return !(a==b);
   1239 }
   1240+
   1241+#ifdef WIN32
   1242+std::string NetworkErrorString(int err)
   1243+{
   1244+    char buf[256];
   

   ---

   ---

   

   Diapolo commented at 2:06 pm on May 22, 2014:

   Couldn’t you write just char buf[256] = 0; and it would zero the first array element?

   ---

   

   leofidus commented at 1:23 am on May 23, 2014:

   But that version is more likely to confuse people. I prefer laanwj’s more obvious version, even though it’s one line longer.

   ---

   

   laanwj commented at 7:34 am on May 23, 2014:

   @diapolo That would zero out the entire buffer, which is not necessary here (according to the documentation FormatMessage will always return a null-terminated string or nothing at all).

   http://msdn.microsoft.com/en-us/library/windows/desktop/ms679351%28v=vs.85%29.aspx

   I see though that we don’t check the output of FormatMessageA - which can fail, itself. We should probably set a default message (like ‘Unknown’) in case it fails. Edit: fixed

8. Replace non-threadsafe strerror

   Log the name of the error as well as the error code if a network problem
   happens. This makes network troubleshooting more convenient.
   
   Use thread-safe strerror_r and the WIN32 equivalent FormatMessage.

   a60838d09a
9. Replace non-threadsafe gmtime and setlocale

   Make DateTimeStrFormat use boost::posix_time.
   
   Also re-enable the util_DateTimeStrFormat tests, as they are no
   longer platform specific.

   3e8ac6af9a
10. laanwj merged this on May 23, 2014
11. laanwj closed this on May 23, 2014

    ---

12. laanwj referenced this in commit 97b53b581b on May 23, 2014
13. 
    BitcoinPullTester commented at 2:06 pm on May 23, 2014: none

    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/3e8ac6af9a993e262d1160fb2e6e1e1f1d5d19f2 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
14. pyritepirate referenced this in commit 4a68069f20 on Jan 25, 2019
15. DrahtBot locked this on Sep 8, 2021

Contributors
laanwj sipa luke-jr Diapolo leofidus BitcoinPullTester