BIND 10 trac1522, updated. 182024bdd5343af59f6e4550eb977168d1745154 [1522] made sure the status code includes \n. also made sure passed sockets will be successfully closed.
BIND 10 source code commits
bind10-changes at lists.isc.org
Thu Dec 29 08:24:24 UTC 2011
The branch, trac1522 has been updated
via 182024bdd5343af59f6e4550eb977168d1745154 (commit)
via d8ac471ae3dc121fd598c47d19bf047ecb1b443d (commit)
via f02087d57966c11bd12197ad778f781ca0a8ddb6 (commit)
via 80d9d4fa922b82c6f47b5482de84399b1eee3c45 (commit)
from 412ea40020ab62f1b83fa78aa41d2b461f97a996 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 182024bdd5343af59f6e4550eb977168d1745154
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Thu Dec 29 00:21:50 2011 -0800
[1522] made sure the status code includes \n. also made sure passed sockets
will be successfully closed.
commit d8ac471ae3dc121fd598c47d19bf047ecb1b443d
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Dec 28 23:05:50 2011 -0800
[1522] some more cleanups: mainly style, some for more constify; some for
simplification; some for preferring reference over real object when possible.
commit f02087d57966c11bd12197ad778f781ca0a8ddb6
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Dec 28 18:58:59 2011 -0800
[1522] more cleanup: changed some static member functions to non members
as they don't have to get access to class internal.
commit 80d9d4fa922b82c6f47b5482de84399b1eee3c45
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Wed Dec 28 18:38:14 2011 -0800
[1522] cleanups: move some private member function to non member when they
don't have to access class internals; constify; other style fixes.
-----------------------------------------------------------------------
Summary of changes:
src/lib/server_common/socket_request.cc | 372 ++++++++++----------
src/lib/server_common/socket_request.h | 90 +++---
.../server_common/tests/socket_requestor_test.cc | 119 ++++---
3 files changed, 304 insertions(+), 277 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/server_common/socket_request.cc b/src/lib/server_common/socket_request.cc
index 6946672..42198cd 100644
--- a/src/lib/server_common/socket_request.cc
+++ b/src/lib/server_common/socket_request.cc
@@ -34,14 +34,14 @@ SocketRequestor* requestor(NULL);
// Before the boss process calls send_fd, it first sends this
// string to indicate success, followed by the file descriptor
const std::string& CREATOR_SOCKET_OK() {
- static const std::string str("1");
+ static const std::string str("1\n");
return (str);
}
// Before the boss process calls send_fd, it sends this
// string to indicate failure. It will not send a file descriptor.
const std::string& CREATOR_SOCKET_UNAVAILABLE() {
- static const std::string str("0");
+ static const std::string str("0\n");
return (str);
}
@@ -61,6 +61,167 @@ const std::string& RELEASE_SOCKET_COMMAND() {
return (str);
}
+// Creates the cc session message to request a socket.
+// The actual command format is hardcoded, and should match
+// the format as read in bind10_src.py.in
+isc::data::ConstElementPtr
+createRequestSocketMessage(SocketRequestor::Protocol protocol,
+ const std::string& address, uint16_t port,
+ SocketRequestor::ShareMode share_mode,
+ const std::string& share_name)
+{
+ const isc::data::ElementPtr request = isc::data::Element::createMap();
+ request->set("address", isc::data::Element::create(address));
+ request->set("port", isc::data::Element::create(port));
+ switch (protocol) {
+ case SocketRequestor::UDP:
+ request->set("protocol", isc::data::Element::create("UDP"));
+ break;
+ case SocketRequestor::TCP:
+ request->set("protocol", isc::data::Element::create("TCP"));
+ break;
+ }
+ switch (share_mode) {
+ case SocketRequestor::DONT_SHARE:
+ request->set("share_mode", isc::data::Element::create("NO"));
+ break;
+ case SocketRequestor::SHARE_SAME:
+ request->set("share_mode", isc::data::Element::create("SAMEAPP"));
+ break;
+ case SocketRequestor::SHARE_ANY:
+ request->set("share_mode", isc::data::Element::create("ANY"));
+ break;
+ }
+ request->set("share_name", isc::data::Element::create(share_name));
+
+ return (isc::config::createCommand(REQUEST_SOCKET_COMMAND(), request));
+}
+
+isc::data::ConstElementPtr
+createReleaseSocketMessage(const std::string& token) {
+ const isc::data::ElementPtr release = isc::data::Element::createMap();
+ release->set("token", isc::data::Element::create(token));
+
+ return (isc::config::createCommand(RELEASE_SOCKET_COMMAND(), release));
+}
+
+// Checks and parses the response receive from Boss
+// If successful, token and path will be set to the values found in the
+// answer.
+// If the response was an error response, or does not contain the
+// expected elements, a CCSessionError is raised.
+void
+readRequestSocketAnswer(isc::data::ConstElementPtr recv_msg,
+ std::string& token, std::string& path)
+{
+ int rcode;
+ isc::data::ConstElementPtr answer = isc::config::parseAnswer(rcode,
+ recv_msg);
+ if (rcode != 0) {
+ isc_throw(isc::config::CCSessionError,
+ "Error response when requesting socket: " << answer->str());
+ }
+
+ if (!answer || !answer->contains("token") || !answer->contains("path")) {
+ isc_throw(isc::config::CCSessionError,
+ "Malformed answer when requesting socket");
+ }
+ token = answer->get("token")->stringValue();
+ path = answer->get("path")->stringValue();
+}
+
+// Connect to the domain socket that has been received from Boss.
+// (i.e. the one that is used to pass created sockets over).
+//
+// This should only be called if the socket had not been connected to
+// already. To get the socket and reuse existing ones, use
+// getFdShareSocket()
+//
+// \param path The domain socket to connect to
+// \exception SocketError if the socket cannot be connected to
+// \return the socket file descriptor
+int
+createFdShareSocket(const std::string& path) {
+ // TODO: Current master has socketsession code and better way
+ // of handling errors without potential leaks for this. It is
+ // not public there at this moment, but when this is merged
+ // we should make a ticket to move this functionality to the
+ // SocketSessionReceiver and use that.
+ const int sock_pass_fd = socket(AF_UNIX, SOCK_STREAM, 0);
+ if (sock_pass_fd == -1) {
+ isc_throw(SocketRequestor::SocketError,
+ "Unable to open domain socket " << path <<
+ ": " << strerror(errno));
+ }
+ struct sockaddr_un sock_pass_addr;
+ sock_pass_addr.sun_family = AF_UNIX;
+ if (path.size() >= sizeof(sock_pass_addr.sun_path)) {
+ close(sock_pass_fd);
+ isc_throw(SocketRequestor::SocketError,
+ "Unable to open domain socket " << path <<
+ ": path too long");
+ }
+#ifdef HAVE_SA_LEN
+ sock_pass_addr.sun_len = path.size();
+#endif
+ strcpy(sock_pass_addr.sun_path, path.c_str());
+ const socklen_t len = path.size() + offsetof(struct sockaddr_un, sun_path);
+ // Yes, C-style cast bad. See previous comment about SocketSessionReceiver.
+ if (connect(sock_pass_fd, (const struct sockaddr*)&sock_pass_addr,
+ len) == -1) {
+ close(sock_pass_fd);
+ isc_throw(SocketRequestor::SocketError,
+ "Unable to open domain socket " << path <<
+ ": " << strerror(errno));
+ }
+ return (sock_pass_fd);
+}
+
+// Reads a socket fd over the given socket (using recv_fd()).
+//
+// \exception SocketError if the socket cannot be read
+// \return the socket fd that has been read
+int
+getSocketFd(int sock_pass_fd) {
+ // Boss first sends some data to signal that getting the socket
+ // from its cache succeeded
+ char status[3]; // We need a space for trailing \0, hence 3
+ memset(status, 0, 3);
+ if (isc::util::io::read_data(sock_pass_fd, status, 2) < 2) {
+ isc_throw(SocketRequestor::SocketError,
+ "Error reading status code while requesting socket");
+ }
+ // Actual status value hardcoded by boss atm.
+ if (CREATOR_SOCKET_UNAVAILABLE() == status) {
+ isc_throw(SocketRequestor::SocketError,
+ "CREATOR_SOCKET_UNAVAILABLE returned");
+ } else if (CREATOR_SOCKET_OK() != status) {
+ isc_throw(SocketRequestor::SocketError,
+ "Unknown status code returned before recv_fd '" << status <<
+ "'");
+ }
+
+ const int passed_sock_fd = isc::util::io::recv_fd(sock_pass_fd);
+
+ // check for error values of passed_sock_fd (see fd_share.h)
+ if (passed_sock_fd < 0) {
+ switch (passed_sock_fd) {
+ case isc::util::io::FD_COMM_ERROR:
+ isc_throw(SocketRequestor::SocketError,
+ "FD_COMM_ERROR while requesting socket");
+ break;
+ case isc::util::io::FD_OTHER_ERROR:
+ isc_throw(SocketRequestor::SocketError,
+ "FD_OTHER_ERROR while requesting socket");
+ break;
+ default:
+ isc_throw(SocketRequestor::SocketError,
+ "Unknown error while requesting socket");
+ }
+ }
+ return (passed_sock_fd);
+}
+
// This implementation class for SocketRequestor uses
// a ModuleCCSession for communication with the boss process,
// and fd_share to read out the socket(s).
@@ -80,12 +241,12 @@ public:
const std::string& address,
uint16_t port, ShareMode share_mode,
const std::string& share_name) {
- isc::data::ConstElementPtr request_msg =
+ const isc::data::ConstElementPtr request_msg =
createRequestSocketMessage(protocol, address, port,
share_mode, share_name);
// Send it to boss
- int seq = session_.groupSendMsg(request_msg, "Boss");
+ const int seq = session_.groupSendMsg(request_msg, "Boss");
// Get the answer from the boss.
// Just do a blocking read, we can't really do much anyway
@@ -100,19 +261,19 @@ public:
readRequestSocketAnswer(recv_msg, token, path);
// get the domain socket over which we will receive the
// real socket
- int sock_pass_fd = getFdShareSocket(path);
+ const int sock_pass_fd = getFdShareSocket(path);
// and finally get the socket itself
- int passed_sock_fd = getSocketFd(sock_pass_fd);
+ const int passed_sock_fd = getSocketFd(sock_pass_fd);
return (SocketID(passed_sock_fd, token));
- };
+ }
virtual void releaseSocket(const std::string& token) {
- isc::data::ConstElementPtr release_msg =
+ const isc::data::ConstElementPtr release_msg =
createReleaseSocketMessage(token);
// Send it to boss
- int seq = session_.groupSendMsg(release_msg, "Boss");
+ const int seq = session_.groupSendMsg(release_msg, "Boss");
// Get the answer from the boss.
// Just do a blocking read, we can't really do much anyway
@@ -130,89 +291,18 @@ public:
isc_throw(SocketError,
"Error requesting release of socket: " << error->str());
}
- };
-
-private:
- // Creates the cc session message to request a socket.
- // The actual command format is hardcoded, and should match
- // the format as read in bind10_src.py.in
- isc::data::ConstElementPtr
- createRequestSocketMessage(Protocol protocol,
- const std::string& address,
- uint16_t port, ShareMode share_mode,
- const std::string& share_name)
- {
- isc::data::ElementPtr request = isc::data::Element::createMap();
- request->set("address", isc::data::Element::create(address));
- request->set("port", isc::data::Element::create(port));
- switch (protocol) {
- case SocketRequestor::UDP:
- request->set("protocol", isc::data::Element::create("UDP"));
- break;
- case SocketRequestor::TCP:
- request->set("protocol", isc::data::Element::create("TCP"));
- break;
- }
- switch (share_mode) {
- case DONT_SHARE:
- request->set("share_mode",
- isc::data::Element::create("NO"));
- break;
- case SHARE_SAME:
- request->set("share_mode",
- isc::data::Element::create("SAMEAPP"));
- break;
- case SHARE_ANY:
- request->set("share_mode",
- isc::data::Element::create("ANY"));
- break;
- }
- request->set("share_name", isc::data::Element::create(share_name));
-
- return (isc::config::createCommand(REQUEST_SOCKET_COMMAND(), request));
- }
-
- isc::data::ConstElementPtr
- createReleaseSocketMessage(const std::string& token) {
- isc::data::ElementPtr release = isc::data::Element::createMap();
- release->set("token", isc::data::Element::create(token));
-
- return (isc::config::createCommand(RELEASE_SOCKET_COMMAND(), release));
- }
-
- // Checks and parses the response receive from Boss
- // If successful, token and path will be set to the values found in the
- // answer.
- // If the response was an error response, or does not contain the
- // expected elements, a CCSessionError is raised.
- void readRequestSocketAnswer(isc::data::ConstElementPtr recv_msg,
- std::string& token,
- std::string& path) {
- int rcode;
- isc::data::ConstElementPtr answer = isc::config::parseAnswer(rcode,
- recv_msg);
- if (rcode != 0) {
- isc_throw(isc::config::CCSessionError,
- "Error response when requesting socket: " <<
- answer->str());
- }
-
- if (!answer ||
- !answer->contains("token") ||
- !answer->contains("path")) {
- isc_throw(isc::config::CCSessionError,
- "Malformed answer when requesting socket");
- }
- token = answer->get("token")->stringValue();
- path = answer->get("path")->stringValue();
}
+private:
// Returns the domain socket file descriptor
// If we had not opened it yet, opens it now
int
getFdShareSocket(const std::string& path) {
if (fd_share_sockets_.find(path) == fd_share_sockets_.end()) {
- int new_fd = createFdShareSocket(path);
+ const int new_fd = createFdShareSocket(path);
+ // Technically, the (creation and) assignment of the new map entry
+ // could thrown an exception and lead to FD leak. This should be
+ // cleaned up later (see comment about SocketSessionReceiver above)
fd_share_sockets_[path] = new_fd;
return (new_fd);
} else {
@@ -220,103 +310,13 @@ private:
}
}
- // Connect to the domain socket that has been received from Boss.
- // (i.e. the one that is used to pass created sockets over).
- //
- // This should only be called if the socket had not been connected to
- // already. To get the socket and reuse existing ones, use
- // getFdShareSocket()
- //
- // \param path The domain socket to connect to
- // \exception SocketError if the socket cannot be connected to
- // \return the socket file descriptor
- int
- createFdShareSocket(const std::string& path) {
- // TODO: Current master has socketsession code and better way
- // of handling errors without potential leaks for this. It is
- // not public there at this moment, but when this is merged
- // we should make a ticket to move this functionality to the
- // SocketSessionReceiver and use that.
- int sock_pass_fd = socket(AF_UNIX, SOCK_STREAM, 0);
- if (sock_pass_fd == -1) {
- isc_throw(SocketError, "Unable to open domain socket " << path <<
- ": " << strerror(errno));
- }
- struct sockaddr_un sock_pass_addr;
- sock_pass_addr.sun_family = AF_UNIX;
- if (path.size() >= sizeof(sock_pass_addr.sun_path)) {
- close(sock_pass_fd);
- isc_throw(SocketError, "Unable to open domain socket " << path <<
- ": path too long");
- }
-#ifdef HAVE_SA_LEN
- sock_pass_addr.sun_len = path.size();
-#endif
- strcpy(sock_pass_addr.sun_path, path.c_str());
- const socklen_t len = path.size() +
- offsetof(struct sockaddr_un, sun_path);
- // Yes, C-style cast bad. See previous comment about
- // SocketSessionReceiver.
- if (connect(sock_pass_fd,
- (struct sockaddr *)&sock_pass_addr,
- len) == -1) {
- close(sock_pass_fd);
- isc_throw(SocketError, "Unable to open domain socket " << path <<
- ": " << strerror(errno));
- }
- return (sock_pass_fd);
- }
-
- // Reads a socket fd over the given socket (using recv_fd()).
- //
- // \exception SocketError if the socket cannot be read
- // \return the socket fd that has been read
- int getSocketFd(int sock_pass_fd) {
- // Boss first sends some data to signal that getting the socket
- // from its cache succeeded
- char status[2];
- memset(status, 0, 2);
- if (isc::util::io::read_data(sock_pass_fd, &status, 2) < 2) {
- isc_throw(SocketError,
- "Error reading status code while requesting socket");
- }
- // Actual status value hardcoded by boss atm.
- if (CREATOR_SOCKET_UNAVAILABLE() == status) {
- isc_throw(SocketError,
- "CREATOR_SOCKET_UNAVAILABLE returned");
- } else if (CREATOR_SOCKET_OK() != status) {
- isc_throw(SocketError,
- "Unknown status code returned before recv_fd " << status);
- }
-
- const int passed_sock_fd = isc::util::io::recv_fd(sock_pass_fd);
-
- // check for error values of passed_sock_fd (see fd_share.h)
- if (passed_sock_fd < 0) {
- switch (passed_sock_fd) {
- case isc::util::io::FD_COMM_ERROR:
- isc_throw(SocketError,
- "FD_COMM_ERROR while requesting socket");
- break;
- case isc::util::io::FD_OTHER_ERROR:
- isc_throw(SocketError,
- "FD_OTHER_ERROR while requesting socket");
- break;
- default:
- isc_throw(SocketError,
- "Unknown error while requesting socket");
- }
- }
- return (passed_sock_fd);
- }
-
// Closes the sockets that has been used for fd_share
void
closeFdShareSockets() {
- for (std::map<std::string, int>::iterator it =
+ for (std::map<std::string, int>::const_iterator it =
fd_share_sockets_.begin();
it != fd_share_sockets_.end();
- ++it ) {
+ ++it) {
close((*it).second);
}
}
@@ -337,22 +337,22 @@ socketRequestor() {
}
void
-SocketRequestor::initTest(SocketRequestor* new_requestor) {
- requestor = new_requestor;
-}
-
-
-void
-SocketRequestor::init(config::ModuleCCSession& session) {
+initSocketReqeustor(config::ModuleCCSession& session) {
if (requestor != NULL) {
- isc_throw(InvalidOperation, "The socket requestor was already initialized");
+ isc_throw(InvalidOperation,
+ "The socket requestor was already initialized");
} else {
requestor = new SocketRequestorCCSession(session);
}
}
void
-SocketRequestor::cleanup() {
+initTestSocketRequestor(SocketRequestor* new_requestor) {
+ requestor = new_requestor;
+}
+
+void
+cleanupSocketRequestor() {
if (requestor != NULL) {
delete requestor;
requestor = NULL;
diff --git a/src/lib/server_common/socket_request.h b/src/lib/server_common/socket_request.h
index 093c05c..f4dcfea 100644
--- a/src/lib/server_common/socket_request.h
+++ b/src/lib/server_common/socket_request.h
@@ -39,7 +39,7 @@ namespace server_common {
/// sense to have two of them.
///
/// This is actually an abstract base class. There'll be one with
-/// hidden implementation and we expect the tests to create it's own
+/// hidden implementation and we expect the tests to create its own
/// subclass when needed.
///
/// \see socketRequestor function to access the object of this class.
@@ -51,20 +51,21 @@ protected:
/// (which it can't anyway, as it has pure virtual methods, but just to
/// be sure).
SocketRequestor() {}
+
public:
/// \brief virtual destructor
///
/// A virtual destructor, as we have virtual methods, to make sure it is
/// destroyed by the destructor of the subclass. This shouldn't matter, as
/// a singleton class wouldn't get destroyed, but just to be sure.
-
virtual ~ SocketRequestor() {}
+
/// \brief A representation of received socket
///
/// The pair holds two parts. The OS-level file descriptor acting as the
/// socket (you might want to use it directly with functions like recv,
- /// or fill it into an asio socket). The other part is the token representing
- /// the socket, which allows it to be given up again.
+ /// or fill it into an asio socket). The other part is the token
+ /// representing the socket, which allows it to be given up again.
typedef std::pair<int, std::string> SocketID;
/// \brief The protocol of requested socket
@@ -98,7 +99,7 @@ public:
/// else or ask for nonsense (releasing a socket we don't own).
class SocketError : public Exception {
public:
- SocketError(const char* file, size_t line, const char *what) :
+ SocketError(const char* file, size_t line, const char* what) :
Exception(file, line, what)
{ }
};
@@ -144,43 +145,6 @@ public:
/// release (like we're trying to release a socket that doesn't
/// belong to us or exist at all).
virtual void releaseSocket(const std::string& token) = 0;
-
- /// \brief Initialize the singleton object
- ///
- /// This creates the object that will be used to request sockets.
- /// It can be called only once per the life of application.
- ///
- /// \param session the CC session that'll be used to talk to the
- /// socket creator.
- /// \throw InvalidOperation when it is called more than once
- static void init(config::ModuleCCSession& session);
-
- /// \brief Initialization for tests
- ///
- /// This is to support different subclasses in tests. It replaces
- /// the object used by socketRequestor() function by this one provided
- /// as parameter. The ownership is not taken, eg. it's up to the caller
- /// to delete it when necessary.
- ///
- /// This is not to be used in production applications. It is meant as
- /// an replacement of init.
- ///
- /// This never throws.
- ///
- /// \param requestor the object to be used. It can be NULL to reset to
- /// an "virgin" state (which acts as if initTest or init was never
- /// called before).
- static void initTest(SocketRequestor* requestor);
-
- /// \brief Destroy the singleton instance
- ///
- /// Calling this function is not strictly necessary; the socket
- /// requestor is a singleton anyway. However, for some tests it
- /// is useful to destroy and recreate it, as well as for programs
- /// that want to be completely clean on exit.
- /// After this method has been called, all operations except init
- /// will fail.
- static void cleanup();
};
/// \brief Access the requestor object.
@@ -190,10 +154,46 @@ public:
/// \return the active socket requestor object.
/// \throw InvalidOperation if the object was not yet initialized.
/// \see SocketRequestor::init to initialize the object.
-SocketRequestor&
-socketRequestor();
+SocketRequestor& socketRequestor();
+
+/// \brief Initialize the singleton object
+///
+/// This creates the object that will be used to request sockets.
+/// It can be called only once per the life of application.
+///
+/// \param session the CC session that'll be used to talk to the
+/// socket creator.
+/// \throw InvalidOperation when it is called more than once
+void initSocketReqeustor(config::ModuleCCSession& session);
+
+/// \brief Initialization for tests
+///
+/// This is to support different subclasses in tests. It replaces
+/// the object used by socketRequestor() function by this one provided
+/// as parameter. The ownership is not taken, eg. it's up to the caller
+/// to delete it when necessary.
+///
+/// This is not to be used in production applications. It is meant as
+/// an replacement of init.
+///
+/// This never throws.
+///
+/// \param requestor the object to be used. It can be NULL to reset to
+/// an "virgin" state (which acts as if initTest or init was never
+/// called before).
+void initTestSocketRequestor(SocketRequestor* requestor);
+
+/// \brief Destroy the singleton instance
+///
+/// Calling this function is not strictly necessary; the socket
+/// requestor is a singleton anyway. However, for some tests it
+/// is useful to destroy and recreate it, as well as for programs
+/// that want to be completely clean on exit.
+/// After this function has been called, all operations except init
+/// will fail.
+void cleanupSocketRequestor();
}
}
-#endif
+#endif // __SOCKET_REQUEST_H
diff --git a/src/lib/server_common/tests/socket_requestor_test.cc b/src/lib/server_common/tests/socket_requestor_test.cc
index b04e7f9..33c92bf 100644
--- a/src/lib/server_common/tests/socket_requestor_test.cc
+++ b/src/lib/server_common/tests/socket_requestor_test.cc
@@ -12,6 +12,8 @@
// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
// PERFORMANCE OF THIS SOFTWARE.
+#include <config.h>
+
#include <server_common/socket_request.h>
#include <gtest/gtest.h>
@@ -29,6 +31,7 @@
#include <sys/un.h>
#include <boost/foreach.hpp>
+#include <boost/scoped_ptr.hpp>
#include <util/io/fd.h>
#include <util/io/fd_share.h>
@@ -43,7 +46,7 @@ namespace {
// Check it throws an exception when it is not initialized
TEST(SocketRequestorAccess, unitialized) {
// Make sure it is not initialized
- SocketRequestor::initTest(NULL);
+ initTestSocketRequestor(NULL);
EXPECT_THROW(socketRequestor(), InvalidOperation);
}
@@ -62,7 +65,7 @@ TEST(SocketRequestorAccess, initialized) {
};
DummyRequestor requestor;
// Make sure it is initialized (the test way, of course)
- SocketRequestor::initTest(&requestor);
+ initTestSocketRequestor(&requestor);
// It returs the same "pointer" as inserted
// The casts are there as the template system seemed to get confused
// without them, the types should be correct even without them, but
@@ -70,7 +73,7 @@ TEST(SocketRequestorAccess, initialized) {
EXPECT_EQ(static_cast<const SocketRequestor*>(&requestor),
static_cast<const SocketRequestor*>(&socketRequestor()));
// Just that we don't have an invalid pointer anyway
- SocketRequestor::initTest(NULL);
+ initTestSocketRequestor(NULL);
}
// This class contains a fake (module)ccsession to emulate answers from Boss
@@ -79,16 +82,17 @@ public:
SocketRequestorTest() : session(ElementPtr(new ListElement),
ElementPtr(new ListElement),
ElementPtr(new ListElement)),
- specfile(std::string(TEST_DATA_PATH) + "/spec.spec")
+ specfile(std::string(TEST_DATA_PATH) +
+ "/spec.spec")
{
session.getMessages()->add(createAnswer());
cc_session.reset(new ModuleCCSession(specfile, session, NULL, NULL,
false, false));
- SocketRequestor::init(*cc_session);
- };
+ initSocketReqeustor(*cc_session);
+ }
~SocketRequestorTest() {
- SocketRequestor::cleanup();
+ cleanupSocketRequestor();
}
// Do a standard request with some default values
@@ -114,14 +118,14 @@ public:
// (for easier access to new messages later)
void
clearMsgQueue() {
- while(session.getMsgQueue()->size() > 0) {
+ while (session.getMsgQueue()->size() > 0) {
session.getMsgQueue()->remove(0);
}
}
isc::cc::FakeSession session;
- std::auto_ptr<ModuleCCSession> cc_session;
- std::string specfile;
+ boost::scoped_ptr<ModuleCCSession> cc_session;
+ const std::string specfile;
};
// helper function to create the request packet as we expect the
@@ -131,9 +135,10 @@ createExpectedRequest(const std::string& address,
int port,
const std::string& protocol,
const std::string& share_mode,
- const std::string& share_name) {
+ const std::string& share_name)
+{
// create command arguments
- ElementPtr command_args = Element::createMap();
+ const ElementPtr command_args = Element::createMap();
command_args->set("address", Element::create(address));
command_args->set("port", Element::create(port));
command_args->set("protocol", Element::create(protocol));
@@ -141,7 +146,7 @@ createExpectedRequest(const std::string& address,
command_args->set("share_name", Element::create(share_name));
// create the envelope
- ElementPtr packet = Element::createList();
+ const ElementPtr packet = Element::createList();
packet->add(Element::create("Boss"));
packet->add(Element::create("*"));
packet->add(createCommand("get_socket", command_args));
@@ -205,17 +210,16 @@ TEST_F(SocketRequestorTest, testBadRequestAnswers) {
ASSERT_THROW(doRequest(), SocketRequestor::SocketError);
// Another failure (domain socket path too long)
- std::string long_str(1000, 'x');
- addAnswer("foo", long_str);
+ addAnswer("foo", std::string(1000, 'x'));
ASSERT_THROW(doRequest(), SocketRequestor::SocketError);
// Test values around path boundary
struct sockaddr_un sock_un;
- std::string max_len(sizeof(sock_un.sun_path) - 1, 'x');
+ const std::string max_len(sizeof(sock_un.sun_path) - 1, 'x');
addAnswer("foo", max_len);
// The failure should NOT contain 'too long'
- // (explicitely checking for existance of nonexistence of 'too long',
- // as opposed to the actual error, since 'too long' is a value we set.
+ // (explicitly checking for existance of nonexistence of 'too long',
+ // as opposed to the actual error, since 'too long' is a value we set).
try {
doRequest();
FAIL() << "doRequest did not throw an exception";
@@ -223,7 +227,7 @@ TEST_F(SocketRequestorTest, testBadRequestAnswers) {
ASSERT_EQ(std::string::npos, std::string(se.what()).find("too long"));
}
- std::string too_long(sizeof(sock_un.sun_path), 'x');
+ const std::string too_long(sizeof(sock_un.sun_path), 'x');
addAnswer("foo", too_long);
// The failure SHOULD contain 'too long'
try {
@@ -239,15 +243,15 @@ TEST_F(SocketRequestorTest, testBadRequestAnswers) {
}
// Helper function to create the release commands as we expect
-// them to be sent by the socketrequestor class
+// them to be sent by the SocketRequestor class
ConstElementPtr
createExpectedRelease(const std::string& token) {
// create command arguments
- ElementPtr command_args = Element::createMap();
+ const ElementPtr command_args = Element::createMap();
command_args->set("token", Element::create(token));
// create the envelope
- ElementPtr packet = Element::createList();
+ const ElementPtr packet = Element::createList();
packet->add(Element::create("Boss"));
packet->add(Element::create("*"));
packet->add(createCommand("drop_socket", command_args));
@@ -296,7 +300,10 @@ public:
TestSocket() : fd_(-1) {
path_ = strdup("test_socket.XXXXXX");
// Misuse mkstemp to generate a file name.
- int f = mkstemp(path_);
+ const int f = mkstemp(path_);
+ if (f == -1) {
+ isc_throw(Unexpected, "mkstemp failed: " << strerror(errno));
+ }
// Just need the name, so immediately close
close(f);
}
@@ -311,6 +318,7 @@ public:
free(path_);
if (fd_ != -1) {
close(fd_);
+ fd_ = -1;
}
}
@@ -320,10 +328,10 @@ public:
}
// create socket, fork, and serve if child (child will exit when done)
- void run(std::vector<int> data) {
+ void run(const std::vector<int>& data) {
try {
create();
- int child_pid = fork();
+ const int child_pid = fork();
if (child_pid == 0) {
serve(data);
exit(0);
@@ -343,29 +351,36 @@ private:
create() {
fd_ = socket(AF_UNIX, SOCK_STREAM, 0);
if (fd_ == -1) {
- isc_throw(Exception, "Unable to create socket");
+ isc_throw(Unexpected, "Unable to create socket");
}
struct sockaddr_un socket_address;
socket_address.sun_family = AF_UNIX;
socklen_t len = strlen(path_);
if (len > sizeof(socket_address.sun_path)) {
- isc_throw(Exception,
+ isc_throw(Unexpected,
"mkstemp() created a filename too long for sun_path");
}
strncpy(socket_address.sun_path, path_, len);
+#ifdef HAVE_SA_LEN
+ socket_address.sun_len = len;
+#endif
len += offsetof(struct sockaddr_un, sun_path);
// Remove the random file we created so we can reuse it for
// a domain socket connection. This contains a minor race condition
// but for the purposes of this test it should be small enough
unlink(path_);
- if (bind(fd_, (struct sockaddr *)&socket_address, len) == -1) {
- isc_throw(Exception,
+ if (bind(fd_, (const struct sockaddr*)&socket_address, len) == -1) {
+ isc_throw(Unexpected,
"unable to bind to test domain socket " << path_ <<
": " << strerror(errno));
}
- listen(fd_, 1);
+ if (listen(fd_, 1) == -1) {
+ isc_throw(Unexpected,
+ "unable to listen on test domain socket " << path_ <<
+ ": " << strerror(errno));
+ }
}
// Accept one connection, then send all values from the vector using
@@ -376,39 +391,47 @@ private:
// when the value is -2, it will send a byte signaling CREATOR_SOCKET_OK
// first, and then one byte from some string (i.e. bad data, not using
// send_fd())
+ //
+ // NOTE: client_fd could leak on exception. This should be cleaned up.
+ // See the note about SocketSessionReceiver in socket_request.cc.
void
- serve(std::vector<int> data) {
- int client_fd = accept(fd_, NULL, NULL);
+ serve(const std::vector<int>& data) {
+ const int client_fd = accept(fd_, NULL, NULL);
if (client_fd == -1) {
isc_throw(Exception, "Error in accept(): " << strerror(errno));
}
BOOST_FOREACH(int cur_data, data) {
- int result;
+ bool result;
if (cur_data == -1) {
// send 'CREATOR_SOCKET_UNAVAILABLE'
- result = isc::util::io::write_data(client_fd, "0", 2);
+ result = isc::util::io::write_data(client_fd, "0\n", 2);
} else if (cur_data == -2) {
// send 'CREATOR_SOCKET_OK' first
- result = isc::util::io::write_data(client_fd, "1", 2);
- if (result == 1) {
- result = send(client_fd, "a", 1, 0);
+ result = isc::util::io::write_data(client_fd, "1\n", 2);
+ if (result) {
+ if (send(client_fd, "a", 1, 0) != 1) {
+ result = false;
+ }
}
} else {
// send 'CREATOR_SOCKET_OK' first
- result = isc::util::io::write_data(client_fd, "1", 2);
- if (result == 1) {
- result = isc::util::io::send_fd(client_fd, cur_data);
+ result = isc::util::io::write_data(client_fd, "1\n", 2);
+ if (result) {
+ if (isc::util::io::send_fd(client_fd, cur_data) != 0) {
+ result = false;
+ }
}
}
- if (result < 0) {
- isc_throw(Exception, "Error in send_fd(): " << strerror(errno));
+ if (!result) {
+ isc_throw(Exception, "Error in send_fd(): " <<
+ strerror(errno));
}
}
close(client_fd);
}
int fd_;
- char *path_;
+ char* path_;
};
TEST_F(SocketRequestorTest, testSocketPassing) {
@@ -426,16 +449,19 @@ TEST_F(SocketRequestorTest, testSocketPassing) {
addAnswer("foo", ts.getPath());
SocketRequestor::SocketID socket_id = doRequest();
ASSERT_EQ("foo", socket_id.second);
+ ASSERT_EQ(0, close(socket_id.first));
// 2 should be ok too
addAnswer("bar", ts.getPath());
socket_id = doRequest();
ASSERT_EQ("bar", socket_id.second);
+ ASSERT_EQ(0, close(socket_id.first));
// 3 should be ok too (reuse earlier token)
addAnswer("foo", ts.getPath());
socket_id = doRequest();
ASSERT_EQ("foo", socket_id.second);
+ ASSERT_EQ(0, close(socket_id.first));
// -1 should not
addAnswer("foo", ts.getPath());
@@ -456,21 +482,22 @@ TEST_F(SocketRequestorTest, testSocketPassing) {
addAnswer("foo", ts2.getPath());
socket_id = doRequest();
ASSERT_EQ("foo", socket_id.second);
+ ASSERT_EQ(0, close(socket_id.first));
// Now use first socket again
addAnswer("foo", ts.getPath());
socket_id = doRequest();
ASSERT_EQ("foo", socket_id.second);
+ ASSERT_EQ(0, close(socket_id.first));
// Vector is of first socket is now empty, so the socket should be gone
addAnswer("foo", ts.getPath());
ASSERT_THROW(doRequest(), SocketRequestor::SocketError);
- // Vector is of second socket is now empty too, so the socket should be gone
+ // Vector is of second socket is now empty too, so the socket should be
+ // gone
addAnswer("foo", ts2.getPath());
ASSERT_THROW(doRequest(), SocketRequestor::SocketError);
-
-
}
More information about the bind10-changes
mailing list