Replace non-threadsafe strerror gmtime setlocale

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

laanwj renamed this:
~~2014 05 replace strerror gmtime~~
Replace non-threadsafe strerror gmtime setlocale
on May 8, 2014

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

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.

laanwj added this to the milestone 0.9.2 on May 14, 2014

sipa commented at 11:26 am on May 22, 2014: member

untested ACK

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

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

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

laanwj merged this on May 23, 2014

laanwj closed this on May 23, 2014

laanwj referenced this in commit 97b53b581b on May 23, 2014

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.

pyritepirate referenced this in commit 4a68069f20 on Jan 25, 2019

DrahtBot locked this on Sep 8, 2021

Replace non-threadsafe strerror gmtime setlocale #4152