- 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
Replace non-threadsafe strerror gmtime setlocale #4152
pull laanwj wants to merge 2 commits into bitcoin:master from laanwj:2014_05_replace_strerror_gmtime changing 8 files +68 −36-
laanwj commented at 4:12 pm on May 8, 2014: member
-
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 casebuf
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: memberuntested 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.
-
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 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: noneAutomatic 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