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
--- a/lib/cpp/Makefile.am
+++ b/lib/cpp/Makefile.am
@@ -14,6 +14,7 @@
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
--- a/lib/cpp/configure.ac
+++ b/lib/cpp/configure.ac
@@ -4,6 +4,12 @@
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_SELECT_ARGTYPES
+AC_FUNC_STRERROR_R
+
AC_CHECK_FUNCS([bzero])
AC_CHECK_FUNCS([gethostbyname])
@@ -100,10 +108,6 @@
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
--- a/lib/cpp/src/transport/TServerSocket.cpp
+++ b/lib/cpp/src/transport/TServerSocket.cpp
@@ -239,25 +239,22 @@
(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
--- a/lib/cpp/src/transport/TSocket.cpp
+++ b/lib/cpp/src/transport/TSocket.cpp
@@ -97,11 +97,9 @@
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 @@
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 @@
}
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 @@
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 @@
// Fail on a send error
if (b < 0) {
- if (errno == EPIPE) {
+ if (errno == EPIPE || errno == ECONNRESET || errno == ENOTCONN) {
+ int errno_copy = errno;
close();
- throw TTransportException(TTransportException::NOT_OPEN, "EPIPE");
+ throw TTransportException(TTransportException::NOT_OPEN, "send()", errno_copy);
}
- if (errno == ECONNRESET) {
- close();
- throw TTransportException(TTransportException::NOT_OPEN, "ECONNRESET");
- }
-
- if (errno == ENOTCONN) {
- close();
- throw TTransportException(TTransportException::NOT_OPEN, "ENOTCONN");
- }
-
+ 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 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
--- 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 @@
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 @@
}
protected:
+ /** Just like strerror_r but returns a C++ string object. */
+ std::string strerror_s(int errno_copy);
+
/** Error code */
TTransportExceptionType type_;