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 <expletive>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 88da56a..b8d3cea 100644 (file)
--- 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 a6d61e8..0208f1b 100644 (file)
--- 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 ade0fd4..9182da7 100644 (file)
--- a/lib/cpp/src/transport/TServerSocket.cpp
+++ b/lib/cpp/src/transport/TServerSocket.cpp
@@ -239,25 +239,22 @@ shared_ptr<TTransport> 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<TSocket> client(new TSocket(clientSocket));

diff --git a/lib/cpp/src/transport/TSocket.cpp b/lib/cpp/src/transport/TSocket.cpp
index 9615b2a..90f07f8 100644 (file)
--- 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 (file)
index 0000000..85b77b8
--- /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 <transport/TTransportException.h>
+#include <boost/lexical_cast.hpp>
+#include <cstring>
+#include <config.h>
+
+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<string>(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<string>(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 df27920..bb8b8a9 100644 (file)
--- a/lib/cpp/src/transport/TTransportException.h
+++ b/lib/cpp/src/transport/TTransportException.h
@@ -9,6 +9,7 @@
 
 #include <boost/lexical_cast.hpp>
 #include <string>
+#include <Thrift.h>
 
 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 e1110b4..c2efb1a 100644 (file)
--- a/test/cpp/src/TestClient.cpp
+++ b/test/cpp/src/TestClient.cpp
@@ -8,6 +8,9 @@
 #include <boost/shared_ptr.hpp>
 #include "ThriftTest.h"
 
+#define __STDC_FORMAT_MACROS
+#include <inttypes.h>
+
 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<UserId, map<Numberz,Insanity> >::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<Numberz,Insanity>::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<Numberz, UserId>::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<Xtruct>::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 c8ecf15..3afc955 100644 (file)
--- a/test/cpp/src/TestServer.cpp
+++ b/test/cpp/src/TestServer.cpp
@@ -13,6 +13,9 @@
 #include <stdexcept>
 #include <sstream>
 
+#define __STDC_FORMAT_MACROS
+#include <inttypes.h>
+
 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<UserId, map<Numberz,Insanity> >::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<Numberz,Insanity>::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<Numberz, UserId>::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<Xtruct>::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("}");