From: David Reiss Date: Wed, 22 Aug 2007 23:20:24 +0000 (+0000) Subject: Thrift: Better handling of strerror_r. X-Git-Tag: 0.2.0~1257 X-Git-Url: https://source.supwisdom.com/gerrit/gitweb?a=commitdiff_plain;h=bc3dddb91c1e26ecfa290c10f9a5950adc8dedd1;p=common%2Fthrift.git Thrift: Better handling of strerror_r. Summary: Someone thought it would be a good idea to have two different signatures for strerror_r, with subtly different semantics (strlcpy = smart). We now work properly with either of them. Also fixed a test to work on 32-bit, you sloppy s. Reviewed By: mcslee Test Plan: Rebuild thrift. Force one of these errors to be thrown. Revert Plan: ok git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@665215 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/lib/cpp/Makefile.am b/lib/cpp/Makefile.am index 88da56ae..b8d3cea4 100644 --- a/lib/cpp/Makefile.am +++ b/lib/cpp/Makefile.am @@ -14,6 +14,7 @@ libthrift_sources = src/Thrift.cpp \ src/processor/PeekProcessor.cpp \ src/protocol/TBinaryProtocol.cpp \ src/protocol/TDebugProtocol.cpp \ + src/transport/TTransportException.cpp \ src/transport/TFileTransport.cpp \ src/transport/THttpClient.cpp \ src/transport/TSocket.cpp \ diff --git a/lib/cpp/configure.ac b/lib/cpp/configure.ac index a6d61e8c..0208f1b2 100644 --- a/lib/cpp/configure.ac +++ b/lib/cpp/configure.ac @@ -4,6 +4,12 @@ AC_INIT(thriftcpp, 1.0) AC_CONFIG_SRCDIR(src/Thrift.h) +AC_PROG_CC + +AC_PROG_CXX + +AC_LANG([C++]) + AM_INIT_AUTOMAKE AC_FUNC_MALLOC @@ -12,6 +18,8 @@ AC_FUNC_REALLOC AC_FUNC_SELECT_ARGTYPES +AC_FUNC_STRERROR_R + AC_CHECK_FUNCS([bzero]) AC_CHECK_FUNCS([gethostbyname]) @@ -100,10 +108,6 @@ AC_TYPE_UINT8_T AC_CONFIG_HEADERS(config.h:config.hin) -AC_PROG_CC - -AC_PROG_CXX - AC_PROG_INSTALL AC_PROG_LIBTOOL diff --git a/lib/cpp/src/transport/TServerSocket.cpp b/lib/cpp/src/transport/TServerSocket.cpp index ade0fd40..9182da7e 100644 --- a/lib/cpp/src/transport/TServerSocket.cpp +++ b/lib/cpp/src/transport/TServerSocket.cpp @@ -239,25 +239,22 @@ shared_ptr TServerSocket::acceptImpl() { (socklen_t *) &size); if (clientSocket < 0) { + int errno_copy = errno; GlobalOutput("TServerSocket::accept()"); - char b_error[1024]; - strerror_r(errno, b_error, sizeof(b_error)); - throw TTransportException(TTransportException::UNKNOWN, string("ERROR:") + b_error); + throw TTransportException(TTransportException::UNKNOWN, "accept()", errno_copy); } // Make sure client socket is blocking int flags = fcntl(clientSocket, F_GETFL, 0); if (flags == -1) { + int errno_copy = errno; GlobalOutput("TServerSocket::select() fcntl GETFL"); - char b_error[1024]; - strerror_r(errno, b_error, sizeof(b_error)); - throw TTransportException(TTransportException::UNKNOWN, string("ERROR:") + b_error); + throw TTransportException(TTransportException::UNKNOWN, "fcntl(F_GETFL)", errno_copy); } if (-1 == fcntl(clientSocket, F_SETFL, flags & ~O_NONBLOCK)) { + int errno_copy = errno; GlobalOutput("TServerSocket::select() fcntl SETFL"); - char b_error[1024]; - strerror_r(errno, b_error, sizeof(b_error)); - throw TTransportException(TTransportException::UNKNOWN, string("ERROR:") + b_error); + throw TTransportException(TTransportException::UNKNOWN, "fcntl(F_SETFL)", errno_copy); } shared_ptr client(new TSocket(clientSocket)); diff --git a/lib/cpp/src/transport/TSocket.cpp b/lib/cpp/src/transport/TSocket.cpp index 9615b2ac..90f07f84 100644 --- a/lib/cpp/src/transport/TSocket.cpp +++ b/lib/cpp/src/transport/TSocket.cpp @@ -97,11 +97,9 @@ bool TSocket::peek() { uint8_t buf; int r = recv(socket_, &buf, 1, MSG_PEEK); if (r == -1) { + int errno_copy = errno; GlobalOutput("TSocket::peek()"); - close(); - char b_error[1024]; - strerror_r(errno, b_error, sizeof(b_error)); - throw TTransportException(TTransportException::UNKNOWN, string("recv() ERROR:") + b_error); + throw TTransportException(TTransportException::UNKNOWN, "recv()", errno_copy); } return (r > 0); } @@ -113,10 +111,9 @@ void TSocket::openConnection(struct addrinfo *res) { socket_ = socket(res->ai_family, res->ai_socktype, res->ai_protocol); if (socket_ == -1) { + int errno_copy = errno; GlobalOutput("TSocket::open() socket"); - char b_error[1024]; - strerror_r(errno, b_error, sizeof(b_error)); - throw TTransportException(TTransportException::NOT_OPEN, string("socket() ERROR:") + b_error); + throw TTransportException(TTransportException::NOT_OPEN, "socket()", errno_copy); } // Send timeout @@ -159,13 +156,12 @@ void TSocket::openConnection(struct addrinfo *res) { } if (errno != EINPROGRESS) { + int errno_copy = errno; char buff[1024]; - GlobalOutput(buff); sprintf(buff, "TSocket::open() connect %s %d", host_.c_str(), port_); + GlobalOutput(buff); - char b_error[1024]; - strerror_r(errno, b_error, sizeof(b_error)); - throw TTransportException(TTransportException::NOT_OPEN, string("open() ERROR: ") + b_error); + throw TTransportException(TTransportException::NOT_OPEN, "connect()", errno_copy); } fd_set fds; @@ -180,28 +176,24 @@ void TSocket::openConnection(struct addrinfo *res) { lon = sizeof(int); int ret2 = getsockopt(socket_, SOL_SOCKET, SO_ERROR, (void *)&val, &lon); if (ret2 == -1) { + int errno_copy = errno; GlobalOutput("TSocket::open() getsockopt SO_ERROR"); - char b_error[1024]; - strerror_r(errno, b_error, sizeof(b_error)); - throw TTransportException(TTransportException::NOT_OPEN, string("open() ERROR: ") + b_error); + throw TTransportException(TTransportException::NOT_OPEN, "open()", errno_copy); } if (val == 0) { goto done; } + int errno_copy = errno; GlobalOutput("TSocket::open() SO_ERROR was set"); - char b_error[1024]; - strerror_r(errno, b_error, sizeof(b_error)); - throw TTransportException(TTransportException::NOT_OPEN, string("open() ERROR: ") + b_error); + throw TTransportException(TTransportException::NOT_OPEN, "open()", errno_copy); } else if (ret == 0) { - GlobalOutput("TSocket::open() timeed out"); - char b_error[1024]; - strerror_r(errno, b_error, sizeof(b_error)); - throw TTransportException(TTransportException::NOT_OPEN, string("open() ERROR: ") + b_error); + int errno_copy = errno; + GlobalOutput("TSocket::open() timed out"); + throw TTransportException(TTransportException::NOT_OPEN, "open()", errno_copy); } else { + int errno_copy = errno; GlobalOutput("TSocket::open() select error"); - char b_error[1024]; - strerror_r(errno, b_error, sizeof(b_error)); - throw TTransportException(TTransportException::NOT_OPEN, string("open() ERROR: ") + b_error); + throw TTransportException(TTransportException::NOT_OPEN, "open()", errno_copy); } done: @@ -377,25 +369,15 @@ void TSocket::write(const uint8_t* buf, uint32_t len) { // Fail on a send error if (b < 0) { - if (errno == EPIPE) { - close(); - throw TTransportException(TTransportException::NOT_OPEN, "EPIPE"); - } - - if (errno == ECONNRESET) { - close(); - throw TTransportException(TTransportException::NOT_OPEN, "ECONNRESET"); - } - - if (errno == ENOTCONN) { + if (errno == EPIPE || errno == ECONNRESET || errno == ENOTCONN) { + int errno_copy = errno; close(); - throw TTransportException(TTransportException::NOT_OPEN, "ENOTCONN"); + throw TTransportException(TTransportException::NOT_OPEN, "send()", errno_copy); } + int errno_copy = errno; GlobalOutput("TSocket::write() send < 0"); - char b_error[1024]; - strerror_r(errno, b_error, sizeof(b_error)); - throw TTransportException(TTransportException::UNKNOWN, string("ERROR:") + b_error); + throw TTransportException(TTransportException::UNKNOWN, "send", errno_copy); } // Fail on blocked send diff --git a/lib/cpp/src/transport/TTransportException.cpp b/lib/cpp/src/transport/TTransportException.cpp new file mode 100644 index 00000000..85b77b84 --- /dev/null +++ b/lib/cpp/src/transport/TTransportException.cpp @@ -0,0 +1,42 @@ +// Copyright (c) 2006- Facebook +// Distributed under the Thrift Software License +// +// See accompanying file LICENSE or visit the Thrift site at: +// http://developers.facebook.com/thrift/ + +#include +#include +#include +#include + +using std::string; +using boost::lexical_cast; + +namespace facebook { namespace thrift { namespace transport { + +string TTransportException::strerror_s(int errno_copy) { +#ifndef HAVE_STRERROR_R + return "errno = " + lexical_cast(errno_copy); +#else // HAVE_STRERROR_R + + char b_errbuf[1024] = { '\0' }; +#ifdef STRERROR_R_CHAR_P + char *b_error = strerror_r(errno_copy, b_errbuf, sizeof(b_errbuf)); +#else + char *b_error = b_errbuf; + int rv = strerror_r(errno_copy, b_errbuf, sizeof(b_errbuf)); + if (rv == -1) { + // strerror_r failed. omgwtfbbq. + return "XSI-compliant strerror_r() failed with errno = " + + lexical_cast(errno_copy); + } +#endif + // Can anyone prove that explicit cast is probably not necessary + // to ensure that the string object is constructed before + // b_error becomes invalid? + return string(b_error); + +#endif // HAVE_STRERROR_R +} + +}}} // facebook::thrift::transport diff --git a/lib/cpp/src/transport/TTransportException.h b/lib/cpp/src/transport/TTransportException.h index df279208..bb8b8a9f 100644 --- a/lib/cpp/src/transport/TTransportException.h +++ b/lib/cpp/src/transport/TTransportException.h @@ -9,6 +9,7 @@ #include #include +#include namespace facebook { namespace thrift { namespace transport { @@ -52,6 +53,12 @@ class TTransportException : public facebook::thrift::TException { facebook::thrift::TException(message), type_(type) {} + TTransportException(TTransportExceptionType type, + const std::string& message, + int errno_copy) : + facebook::thrift::TException(message + ": " + strerror_s(errno_copy)), + type_(type) {} + virtual ~TTransportException() throw() {} /** @@ -74,6 +81,9 @@ class TTransportException : public facebook::thrift::TException { } protected: + /** Just like strerror_r but returns a C++ string object. */ + std::string strerror_s(int errno_copy); + /** Error code */ TTransportExceptionType type_; diff --git a/test/cpp/src/TestClient.cpp b/test/cpp/src/TestClient.cpp index e1110b4d..c2efb1ac 100644 --- a/test/cpp/src/TestClient.cpp +++ b/test/cpp/src/TestClient.cpp @@ -8,6 +8,9 @@ #include #include "ThriftTest.h" +#define __STDC_FORMAT_MACROS +#include + using namespace boost; using namespace std; using namespace facebook::thrift; @@ -137,7 +140,7 @@ int main(int argc, char** argv) { */ printf("testI64(-34359738368)"); int64_t i64 = testClient.testI64(-34359738368LL); - printf(" = %ld\n", i64); + printf(" = %"PRId64"\n", i64); /** * DOUBLE TEST @@ -157,7 +160,7 @@ int main(int argc, char** argv) { out.i64_thing = -5; Xtruct in; testClient.testStruct(in, out); - printf(" = {\"%s\", %d, %d, %ld}\n", + printf(" = {\"%s\", %d, %d, %"PRId64"}\n", in.string_thing.c_str(), (int)in.byte_thing, in.i32_thing, @@ -174,7 +177,7 @@ int main(int argc, char** argv) { Xtruct2 in2; testClient.testNest(in2, out2); in = in2.struct_thing; - printf(" = {%d, {\"%s\", %d, %d, %ld}, %d}\n", + printf(" = {%d, {\"%s\", %d, %d, %"PRId64"}, %d}\n", in2.byte_thing, in.string_thing.c_str(), (int)in.byte_thing, @@ -309,7 +312,7 @@ int main(int argc, char** argv) { */ printf("testTypedef(309858235082523)"); UserId uid = testClient.testTypedef(309858235082523LL); - printf(" = %ld\n", uid); + printf(" = %"PRId64"\n", uid); /** * NESTED MAP TEST @@ -346,7 +349,7 @@ int main(int argc, char** argv) { printf(" = {"); map >::const_iterator i_iter; for (i_iter = whoa.begin(); i_iter != whoa.end(); ++i_iter) { - printf("%ld => {", i_iter->first); + printf("%"PRId64" => {", i_iter->first); map::const_iterator i2_iter; for (i2_iter = i_iter->second.begin(); i2_iter != i_iter->second.end(); @@ -356,7 +359,7 @@ int main(int argc, char** argv) { map::const_iterator um; printf("{"); for (um = userMap.begin(); um != userMap.end(); ++um) { - printf("%d => %ld, ", um->first, um->second); + printf("%d => %"PRId64", ", um->first, um->second); } printf("}, "); @@ -364,7 +367,7 @@ int main(int argc, char** argv) { vector::const_iterator x; printf("{"); for (x = xtructs.begin(); x != xtructs.end(); ++x) { - printf("{\"%s\", %d, %d, %ld}, ", + printf("{\"%s\", %d, %d, %"PRId64"}, ", x->string_thing.c_str(), (int)x->byte_thing, x->i32_thing, @@ -430,7 +433,7 @@ int main(int argc, char** argv) { uint64_t stop = now(); uint64_t tot = stop-start; - printf("Total time: %lu us\n", stop-start); + printf("Total time: %"PRIu64" us\n", stop-start); time_tot += tot; if (time_min == 0 || tot < time_min) { @@ -448,9 +451,9 @@ int main(int argc, char** argv) { uint64_t time_avg = time_tot / numTests; - printf("Min time: %lu us\n", time_min); - printf("Max time: %lu us\n", time_max); - printf("Avg time: %lu us\n", time_avg); + printf("Min time: %"PRIu64" us\n", time_min); + printf("Max time: %"PRIu64" us\n", time_max); + printf("Avg time: %"PRIu64" us\n", time_avg); return 0; } diff --git a/test/cpp/src/TestServer.cpp b/test/cpp/src/TestServer.cpp index c8ecf15c..3afc955e 100644 --- a/test/cpp/src/TestServer.cpp +++ b/test/cpp/src/TestServer.cpp @@ -13,6 +13,9 @@ #include #include +#define __STDC_FORMAT_MACROS +#include + using namespace std; using namespace boost; @@ -48,7 +51,7 @@ class TestHandler : public ThriftTestIf { } int64_t testI64(const int64_t thing) { - printf("testI64(%ld)\n", thing); + printf("testI64(%"PRId64")\n", thing); return thing; } @@ -58,13 +61,13 @@ class TestHandler : public ThriftTestIf { } void testStruct(Xtruct& out, const Xtruct &thing) { - printf("testStruct({\"%s\", %d, %d, %ld})\n", thing.string_thing.c_str(), (int)thing.byte_thing, thing.i32_thing, thing.i64_thing); + printf("testStruct({\"%s\", %d, %d, %"PRId64"})\n", thing.string_thing.c_str(), (int)thing.byte_thing, thing.i32_thing, thing.i64_thing); out = thing; } void testNest(Xtruct2& out, const Xtruct2& nest) { const Xtruct &thing = nest.struct_thing; - printf("testNest({%d, {\"%s\", %d, %d, %ld}, %d})\n", (int)nest.byte_thing, thing.string_thing.c_str(), (int)thing.byte_thing, thing.i32_thing, thing.i64_thing, nest.i32_thing); + printf("testNest({%d, {\"%s\", %d, %d, %"PRId64"}, %d})\n", (int)nest.byte_thing, thing.string_thing.c_str(), (int)thing.byte_thing, thing.i32_thing, thing.i64_thing, nest.i32_thing); out = nest; } @@ -122,7 +125,7 @@ class TestHandler : public ThriftTestIf { } UserId testTypedef(const UserId thing) { - printf("testTypedef(%ld)\n", thing); + printf("testTypedef(%"PRId64")\n", thing); return thing; } @@ -179,7 +182,7 @@ class TestHandler : public ThriftTestIf { printf(" = {"); map >::const_iterator i_iter; for (i_iter = insane.begin(); i_iter != insane.end(); ++i_iter) { - printf("%ld => {", i_iter->first); + printf("%"PRId64" => {", i_iter->first); map::const_iterator i2_iter; for (i2_iter = i_iter->second.begin(); i2_iter != i_iter->second.end(); @@ -189,7 +192,7 @@ class TestHandler : public ThriftTestIf { map::const_iterator um; printf("{"); for (um = userMap.begin(); um != userMap.end(); ++um) { - printf("%d => %ld, ", um->first, um->second); + printf("%d => %"PRId64", ", um->first, um->second); } printf("}, "); @@ -197,7 +200,7 @@ class TestHandler : public ThriftTestIf { vector::const_iterator x; printf("{"); for (x = xtructs.begin(); x != xtructs.end(); ++x) { - printf("{\"%s\", %d, %d, %ld}, ", x->string_thing.c_str(), (int)x->byte_thing, x->i32_thing, x->i64_thing); + printf("{\"%s\", %d, %d, %"PRId64"}, ", x->string_thing.c_str(), (int)x->byte_thing, x->i32_thing, x->i64_thing); } printf("}");