BIND 10 trac3221, updated. 18c4f954d1a7466f2b5bde33f855aa69707e2406 [3221] Addressed review comments.
BIND 10 source code commits
bind10-changes at lists.isc.org
Sat Feb 8 18:30:29 UTC 2014
The branch, trac3221 has been updated
via 18c4f954d1a7466f2b5bde33f855aa69707e2406 (commit)
from 400bf9a1f5d4e667c72a51255a2477721876d103 (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 18c4f954d1a7466f2b5bde33f855aa69707e2406
Author: Thomas Markwalder <tmark at isc.org>
Date: Sat Feb 8 13:26:01 2014 -0500
[3221] Addressed review comments.
Made behavior of dhcp_ddns::WatchSocket a bit more robust in handling
programmatic abuse.
Cleaned up addresses used in ncr_upd_unittest.cc, added specific test
for client side address if 0.0.0.0/port 0.
Updated copyrights as appropriate and other cosmetics.
-----------------------------------------------------------------------
Summary of changes:
src/lib/dhcp_ddns/dhcp_ddns_messages.mes | 2 +-
src/lib/dhcp_ddns/ncr_io.cc | 12 +-
src/lib/dhcp_ddns/ncr_io.h | 13 +-
src/lib/dhcp_ddns/ncr_udp.cc | 2 +-
src/lib/dhcp_ddns/ncr_udp.h | 8 +-
src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc | 57 ++++++--
src/lib/dhcp_ddns/tests/watch_socket_unittests.cc | 145 ++++++++++++++++++++-
src/lib/dhcp_ddns/watch_socket.cc | 112 +++++++++++-----
src/lib/dhcp_ddns/watch_socket.h | 34 ++++-
src/lib/dhcpsrv/d2_client.cc | 4 +-
src/lib/dhcpsrv/d2_client.h | 3 +-
src/lib/dhcpsrv/dhcpsrv_messages.mes | 2 +-
src/lib/dhcpsrv/tests/d2_udp_unittest.cc | 2 +-
13 files changed, 323 insertions(+), 73 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/dhcp_ddns/dhcp_ddns_messages.mes b/src/lib/dhcp_ddns/dhcp_ddns_messages.mes
index c1361a9..c5f6f7d 100644
--- a/src/lib/dhcp_ddns/dhcp_ddns_messages.mes
+++ b/src/lib/dhcp_ddns/dhcp_ddns_messages.mes
@@ -1,4 +1,4 @@
-# Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+# Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
#
# Permission to use, copy, modify, and/or distribute this software for any
# purpose with or without fee is hereby granted, provided that the above
diff --git a/src/lib/dhcp_ddns/ncr_io.cc b/src/lib/dhcp_ddns/ncr_io.cc
index 22fa7ac..52a72c9 100644
--- a/src/lib/dhcp_ddns/ncr_io.cc
+++ b/src/lib/dhcp_ddns/ncr_io.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
//
// Permission to use, copy, modify, and/or distribute this software for any
// purpose with or without fee is hereby granted, provided that the above
@@ -338,8 +338,8 @@ NameChangeSender::peekAt(const size_t index) const {
void
-NameChangeSender::assumeQueue(NameChangeSender& sourceSender) {
- if (sourceSender.amSending()) {
+NameChangeSender::assumeQueue(NameChangeSender& source_sender) {
+ if (source_sender.amSending()) {
isc_throw(NcrSenderError, "Cannot assume queue:"
" source sender is actively sending");
}
@@ -349,17 +349,17 @@ NameChangeSender::assumeQueue(NameChangeSender& sourceSender) {
" target sender is actively sending");
}
- if (getQueueMaxSize() < sourceSender.getQueueSize()) {
+ if (getQueueMaxSize() < source_sender.getQueueSize()) {
isc_throw(NcrSenderError, "Cannot assume queue:"
" source queue count exceeds target queue max");
}
- if (send_queue_.size() != 0) {
+ if (!send_queue_.empty()) {
isc_throw(NcrSenderError, "Cannot assume queue:"
" target queue is not empty");
}
- send_queue_.swap(sourceSender.getSendQueue());
+ send_queue_.swap(source_sender.getSendQueue());
}
int
diff --git a/src/lib/dhcp_ddns/ncr_io.h b/src/lib/dhcp_ddns/ncr_io.h
index 10d39d0..65b8929 100644
--- a/src/lib/dhcp_ddns/ncr_io.h
+++ b/src/lib/dhcp_ddns/ncr_io.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
//
// Permission to use, copy, modify, and/or distribute this software for any
// purpose with or without fee is hereby granted, provided that the above
@@ -554,28 +554,27 @@ public:
/// messages from one sender to another. This is useful for dealing with
/// dynamic configuration changes.
///
- /// @param Sender from whom the queued messages will be taken
+ /// @param source_sender from whom the queued messages will be taken
///
/// @throw NcrSenderError if either sender is in send mode, if the number of
/// messages in the source sender's queue is larger than this sender's
/// maxium queue size, or if this sender's queue is not empty.
- void assumeQueue(NameChangeSender& fromSender);
+ void assumeQueue(NameChangeSender& source_sender);
- /// @brief Returns a file description suitable for use with select
+ /// @brief Returns a file descriptor suitable for use with select
///
/// The value returned is an open file descriptor which can be used with
/// select() system call to monitor the sender for IO events. This allows
/// NameChangeSenders to be used in applications which use select, rather
/// than IOService to wait for IO events to occur.
///
- /// @note Attempting other use of this value may lead to unpredictable
+ /// @warning Attempting other use of this value may lead to unpredictable
/// behavior in the sender.
///
/// @return Returns an "open" file descriptor
///
/// @throw NcrSenderError if the sender is not in send mode,
- /// NotImplemented if the implementation does not support such an fd.
- virtual int getSelectFd();
+ virtual int getSelectFd() = 0;
protected:
/// @brief Dequeues and sends the next request on the send queue.
diff --git a/src/lib/dhcp_ddns/ncr_udp.cc b/src/lib/dhcp_ddns/ncr_udp.cc
index 29d6e94..3906c9b 100644
--- a/src/lib/dhcp_ddns/ncr_udp.cc
+++ b/src/lib/dhcp_ddns/ncr_udp.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
//
// Permission to use, copy, modify, and/or distribute this software for any
// purpose with or without fee is hereby granted, provided that the above
diff --git a/src/lib/dhcp_ddns/ncr_udp.h b/src/lib/dhcp_ddns/ncr_udp.h
index 03ac5bf..97f8316 100644
--- a/src/lib/dhcp_ddns/ncr_udp.h
+++ b/src/lib/dhcp_ddns/ncr_udp.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
//
// Permission to use, copy, modify, and/or distribute this software for any
// purpose with or without fee is hereby granted, provided that the above
@@ -504,6 +504,7 @@ public:
/// asyncSend() method is called, passing in send_callback_ member's
/// transfer buffer as the send buffer and the send_callback_ itself
/// as the callback object.
+ /// @param ncr NameChangeRequest to send.
virtual void doSend(NameChangeRequestPtr& ncr);
/// @brief Implements the NameChangeRequest level send completion handler.
@@ -526,14 +527,14 @@ public:
void sendCompletionHandler(const bool successful,
const UDPCallback* send_callback);
- /// @brief Returns a file description suitable for use with select
+ /// @brief Returns a file descriptor suitable for use with select
///
/// The value returned is an open file descriptor which can be used with
/// select() system call to monitor the sender for IO events. This allows
/// NameChangeUDPSenders to be used in applications which use select,
/// rather than IOService to wait for IO events to occur.
///
- /// @note Attempting other use of this value may lead to unpredictable
+ /// @warning Attempting other use of this value may lead to unpredictable
/// behavior in the sender.
///
/// @return Returns an "open" file descriptor
@@ -572,6 +573,7 @@ private:
/// @brief Flag which enables the reuse address socket option if true.
bool reuse_address_;
+ /// @brief Pointer to WatchSocket instance supplying the "select-fd".
WatchSocketPtr watch_socket_;
};
diff --git a/src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc b/src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc
index 5f438c3..7ea4b49 100644
--- a/src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc
+++ b/src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
//
// Permission to use, copy, modify, and/or distribute this software for any
// purpose with or without fee is hereby granted, provided that the above
@@ -71,7 +71,6 @@ const char *valid_msgs[] =
};
const char* TEST_ADDRESS = "127.0.0.1";
-//const char* TEST_ADDRESS = "192.0.2.10";
const uint32_t LISTENER_PORT = 5301;
const uint32_t SENDER_PORT = LISTENER_PORT+1;
const long TEST_TIMEOUT = 5 * 1000;
@@ -315,8 +314,7 @@ TEST(NameChangeUDPSenderBasicTest, constructionTests) {
/// @brief Tests NameChangeUDPSender basic send functionality
/// This test verifies that:
TEST(NameChangeUDPSenderBasicTest, basicSendTests) {
- isc::asiolink::IOAddress ip_address("127.0.0.1");
- uint32_t port = 5301;
+ isc::asiolink::IOAddress ip_address(TEST_ADDRESS);
isc::asiolink::IOService io_service;
SimpleSendHandler ncr_handler;
@@ -325,9 +323,9 @@ TEST(NameChangeUDPSenderBasicTest, basicSendTests) {
// Create the sender, setting the queue max equal to the number of
// messages we will have in the list.
- isc::asiolink::IOAddress any("0.0.0.0");
- NameChangeUDPSender sender(any, 0, ip_address, port,
- FMT_JSON, ncr_handler, num_msgs);
+ NameChangeUDPSender sender(ip_address, SENDER_PORT, ip_address,
+ LISTENER_PORT, FMT_JSON, ncr_handler,
+ num_msgs, true);
// Verify that we can start sending.
EXPECT_NO_THROW(sender.startSending(io_service));
@@ -426,6 +424,45 @@ TEST(NameChangeUDPSenderBasicTest, basicSendTests) {
EXPECT_EQ(0, sender.getQueueSize());
}
+/// @brief Tests NameChangeUDPSender basic send with INADDR_ANY and port 0.
+TEST(NameChangeUDPSenderBasicTest, anyAddressSend) {
+ isc::asiolink::IOAddress ip_address(TEST_ADDRESS);
+ isc::asiolink::IOAddress any_address("0.0.0.0");
+ isc::asiolink::IOService io_service;
+ SimpleSendHandler ncr_handler;
+
+ // Tests are based on a list of messages, get the count now.
+ int num_msgs = sizeof(valid_msgs)/sizeof(char*);
+
+ // Create the sender, setting the queue max equal to the number of
+ // messages we will have in the list.
+ NameChangeUDPSender sender(any_address, 0, ip_address, LISTENER_PORT,
+ FMT_JSON, ncr_handler, num_msgs);
+
+ // Enter send mode.
+ ASSERT_NO_THROW(sender.startSending(io_service));
+ EXPECT_TRUE(sender.amSending());
+
+ // Fetch the sender's select-fd.
+ int select_fd = sender.getSelectFd();
+
+ // Create and queue up a message.
+ NameChangeRequestPtr ncr;
+ ASSERT_NO_THROW(ncr = NameChangeRequest::fromJSON(valid_msgs[0]));
+ EXPECT_NO_THROW(sender.sendRequest(ncr));
+ EXPECT_EQ(1, sender.getQueueSize());
+
+ // message and the queue count should decrement accordingly.
+ // Execute at one ready handler.
+ ASSERT_TRUE(selectCheck(select_fd) > 0);
+ ASSERT_NO_THROW(io_service.run_one());
+
+ // Verify that sender shows no IO ready.
+ // and that the queue is empty.
+ EXPECT_EQ(0, selectCheck(select_fd));
+ EXPECT_EQ(0, sender.getQueueSize());
+}
+
/// @brief Test the NameChangeSender::assumeQueue method.
TEST(NameChangeSender, assumeQueue) {
isc::asiolink::IOAddress ip_address(TEST_ADDRESS);
@@ -519,9 +556,9 @@ public:
*this, true));
// Create our sender instance. Note that reuse_address is true.
- sender_.reset(
- new NameChangeUDPSender(addr, SENDER_PORT, addr, LISTENER_PORT,
- FMT_JSON, *this, 100, true));
+ sender_.reset(
+ new NameChangeUDPSender(addr, SENDER_PORT, addr, LISTENER_PORT,
+ FMT_JSON, *this, 100, true));
// Set the test timeout to break any running tasks if they hang.
test_timer_.setup(boost::bind(&NameChangeUDPTest::testTimeoutHandler,
diff --git a/src/lib/dhcp_ddns/tests/watch_socket_unittests.cc b/src/lib/dhcp_ddns/tests/watch_socket_unittests.cc
index c54ec99..82999f6 100644
--- a/src/lib/dhcp_ddns/tests/watch_socket_unittests.cc
+++ b/src/lib/dhcp_ddns/tests/watch_socket_unittests.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014 Internet Systems Consortium, Inc. ("ISC")
//
// Permission to use, copy, modify, and/or distribute this software for any
// purpose with or without fee is hereby granted, provided that the above
@@ -37,8 +37,8 @@ TEST(WatchSocketTest, basics) {
/// Verify that post-construction the state the select-fd is valid.
int select_fd = watch->getSelectFd();
EXPECT_NE(select_fd, WatchSocket::INVALID_SOCKET);
-
- /// Verify that isReady() is false and that a call to select agrees.
+
+ /// Verify that isReady() is false and that a call to select agrees.
EXPECT_FALSE(watch->isReady());
EXPECT_EQ(0, selectCheck(select_fd));
@@ -57,16 +57,151 @@ TEST(WatchSocketTest, basics) {
EXPECT_FALSE(ioctl(select_fd, FIONREAD, &count));
EXPECT_EQ(sizeof(WatchSocket::MARKER), count);
- /// Verify that isReady() is true and that a call to select agrees.
+ /// Verify that isReady() is true and that a call to select agrees.
EXPECT_TRUE(watch->isReady());
EXPECT_EQ(1, selectCheck(select_fd));
/// Verify that the socket can be cleared.
ASSERT_NO_THROW(watch->clearReady());
- /// Verify that isReady() is false and that a call to select agrees.
+ /// Verify that isReady() is false and that a call to select agrees.
EXPECT_FALSE(watch->isReady());
EXPECT_EQ(0, selectCheck(select_fd));
}
+/// @brief Checks behavior when select_fd is closed externally while in the
+/// "cleared" state.
+TEST(WatchSocketTest, closedWhileClear) {
+ WatchSocketPtr watch;
+
+ /// Verify that we can construct a WatchSocket.
+ ASSERT_NO_THROW(watch.reset(new WatchSocket()));
+ ASSERT_TRUE(watch);
+
+ /// Verify that post-construction the state the select-fd is valid.
+ int select_fd = watch->getSelectFd();
+ ASSERT_NE(select_fd, WatchSocket::INVALID_SOCKET);
+
+ // Verify that socket does not appear ready.
+ ASSERT_EQ(0, watch->isReady());
+
+ // Interfere by closing the fd.
+ ASSERT_EQ(0, close(select_fd));
+
+ // Verify that socket does not appear ready.
+ ASSERT_EQ(0, watch->isReady());
+
+ // Verify that clear does NOT throw.
+ ASSERT_NO_THROW(watch->clearReady());
+
+ // Verify that trying to mark it fails.
+ ASSERT_THROW(watch->markReady(), WatchSocketError);
+
+ // Verify that clear does NOT throw.
+ ASSERT_NO_THROW(watch->clearReady());
+
+ // Verify that getSelectFd() returns invalid socket.
+ ASSERT_EQ(WatchSocket::INVALID_SOCKET, watch->getSelectFd());
+}
+
+/// @brief Checks behavior when select_fd has closed while in the "ready"
+/// state.
+TEST(WatchSocketTest, closedWhileReady) {
+ WatchSocketPtr watch;
+
+ /// Verify that we can construct a WatchSocket.
+ ASSERT_NO_THROW(watch.reset(new WatchSocket()));
+ ASSERT_TRUE(watch);
+
+ /// Verify that post-construction the state the select-fd is valid.
+ int select_fd = watch->getSelectFd();
+ ASSERT_NE(select_fd, WatchSocket::INVALID_SOCKET);
+
+ /// Verify that the socket can be marked ready.
+ ASSERT_NO_THROW(watch->markReady());
+ EXPECT_EQ(1, selectCheck(select_fd));
+
+ // Interfere by closing the fd.
+ ASSERT_EQ(0, close(select_fd));
+
+ // Verify that trying to clear it does not throw.
+ ASSERT_NO_THROW(watch->clearReady());
+
+ // Verify the select_fd fails as socket is invalid/closed.
+ EXPECT_EQ(-1, selectCheck(select_fd));
+
+ // Verify that subsequent attempts to mark it will fail.
+ ASSERT_THROW(watch->markReady(), WatchSocketError);
+}
+
+/// @brief Checks behavior when select_fd has been marked ready but then
+/// emptied by an external read.
+TEST(WatchSocketTest, emptyReadySelectFd) {
+ WatchSocketPtr watch;
+
+ /// Verify that we can construct a WatchSocket.
+ ASSERT_NO_THROW(watch.reset(new WatchSocket()));
+ ASSERT_TRUE(watch);
+
+ /// Verify that post-construction the state the select-fd is valid.
+ int select_fd = watch->getSelectFd();
+ ASSERT_NE(select_fd, WatchSocket::INVALID_SOCKET);
+
+ /// Verify that the socket can be marked ready.
+ ASSERT_NO_THROW(watch->markReady());
+ EXPECT_EQ(1, selectCheck(select_fd));
+
+ // Interfere by reading the fd. This should empty the read pipe.
+ uint32_t buf = 0;
+ ASSERT_EQ((read (select_fd, &buf, sizeof(buf))), sizeof(buf));
+ ASSERT_EQ(WatchSocket::MARKER, buf);
+
+ // Really nothing that can be done to protect against this, but let's
+ // make sure we aren't in a weird state.
+ ASSERT_NO_THROW(watch->clearReady());
+
+ // Verify the select_fd fails as socket is invalid/closed.
+ EXPECT_EQ(0, selectCheck(select_fd));
+
+ // Verify that getSelectFd() returns is still good.
+ ASSERT_EQ(select_fd, watch->getSelectFd());
+}
+
+/// @brief Checks behavior when select_fd has been marked ready but then
+/// contents have been "corrupted" by a partial read.
+TEST(WatchSocketTest, badReadOnClear) {
+ WatchSocketPtr watch;
+
+ /// Verify that we can construct a WatchSocket.
+ ASSERT_NO_THROW(watch.reset(new WatchSocket()));
+ ASSERT_TRUE(watch);
+
+ /// Verify that post-construction the state the select-fd is valid.
+ int select_fd = watch->getSelectFd();
+ ASSERT_NE(select_fd, WatchSocket::INVALID_SOCKET);
+
+ /// Verify that the socket can be marked ready.
+ ASSERT_NO_THROW(watch->markReady());
+ EXPECT_EQ(1, selectCheck(select_fd));
+
+ // Interfere by reading the fd. This should empty the read pipe.
+ uint32_t buf = 0;
+ ASSERT_EQ((read (select_fd, &buf, 1)), 1);
+ ASSERT_NE(WatchSocket::MARKER, buf);
+
+ // Really nothing that can be done to protect against this, but let's
+ // make sure we aren't in a weird state.
+ /// @todo maybe clear should never throw, log only
+ ASSERT_THROW(watch->clearReady(), WatchSocketError);
+
+ // Verify the select_fd fails as socket is invalid/closed.
+ EXPECT_EQ(-1, selectCheck(select_fd));
+
+ // Verify that getSelectFd() returns INVALID.
+ ASSERT_EQ(WatchSocket::INVALID_SOCKET, watch->getSelectFd());
+
+ // Verify that subsequent attempt to mark it fails.
+ ASSERT_THROW(watch->markReady(), WatchSocketError);
+}
+
} // end of anonymous namespace
diff --git a/src/lib/dhcp_ddns/watch_socket.cc b/src/lib/dhcp_ddns/watch_socket.cc
index a123fa2..0a437a6 100644
--- a/src/lib/dhcp_ddns/watch_socket.cc
+++ b/src/lib/dhcp_ddns/watch_socket.cc
@@ -17,16 +17,19 @@
#include <dhcp_ddns/dhcp_ddns_log.h>
#include <dhcp_ddns/watch_socket.h>
+#include <fcntl.h>
#include <errno.h>
+#include <sys/select.h>
namespace isc {
namespace dhcp_ddns {
+
const int WatchSocket::INVALID_SOCKET;
const uint32_t WatchSocket::MARKER;
-WatchSocket::WatchSocket()
- : source_(INVALID_SOCKET), sink_(INVALID_SOCKET), ready_flag_(false) {
+WatchSocket::WatchSocket()
+ : source_(INVALID_SOCKET), sink_(INVALID_SOCKET) {
// Open the pipe.
int fds[2];
if (pipe(fds)) {
@@ -36,64 +39,109 @@ WatchSocket::WatchSocket()
source_ = fds[1];
sink_ = fds[0];
-}
-WatchSocket::~WatchSocket() {
- // Close the pipe fds. Technically a close can fail (hugely unlikely)
- // but there's no recovery for it either. If one does fail we log it
- // and go on. Plus no likes destructors that throw.
- if (source_ != INVALID_SOCKET) {
- if (close(source_)) {
- const char* errstr = strerror(errno);
- LOG_ERROR(dhcp_ddns_logger, DHCP_DDNS_WATCH_SOURCE_CLOSE_ERROR)
- .arg(errstr);
- }
+ if (fcntl(sink_, F_SETFL, O_NONBLOCK)) {
+ const char* errstr = strerror(errno);
+ isc_throw(WatchSocketError, "Cannot set sink to non-blocking: "
+ << errstr);
}
+}
- if (sink_ != INVALID_SOCKET) {
- if (close(sink_)) {
- const char* errstr = strerror(errno);
- LOG_ERROR(dhcp_ddns_logger, DHCP_DDNS_WATCH_SINK_CLOSE_ERROR)
- .arg(errstr);
- }
- }
+WatchSocket::~WatchSocket() {
+ closeSocket();
}
-void
+void
WatchSocket::markReady() {
+ // Make sure it hasn't been orphaned! Otherwise we may get SIGPIPE.
+ // We use fcntl to check as select() on some systems may show it as ready to read.
+ if (fcntl(sink_, F_GETFL) < 0) {
+ closeSocket();
+ isc_throw(WatchSocketError, "WatchSocket markReady - select_fd was closed!");
+ }
+
if (!isReady()) {
int nbytes = write (source_, &MARKER, sizeof(MARKER));
if (nbytes != sizeof(MARKER)) {
+ // If there's an error get the error message than close
+ // the pipe. This should ensure any further use of the socket
+ // or testing the fd with select_fd will fail.
const char* errstr = strerror(errno);
+ closeSocket();
isc_throw(WatchSocketError, "WatchSocket markReady failed:"
<< " bytes written: " << nbytes << " : " << errstr);
}
-
- ready_flag_ = true;
- }
+ }
}
-bool
+bool
WatchSocket::isReady() {
- return (ready_flag_);
+ // Report it as not ready rather than error here.
+ if (sink_ == INVALID_SOCKET) {
+ return (false);
+ }
+
+ fd_set read_fds;
+ FD_ZERO(&read_fds);
+
+ // Add select_fd socket to listening set
+ FD_SET(sink_, &read_fds);
+
+ // Set zero timeout (non-blocking).
+ struct timeval select_timeout;
+ select_timeout.tv_sec = 0;
+ select_timeout.tv_usec = 0;
+
+ // Return true only if read ready, treat error same as not ready.
+ return (select(sink_ + 1, &read_fds, NULL, NULL, &select_timeout) > 0);
}
-void
+void
WatchSocket::clearReady() {
if (isReady()) {
- uint32_t buf;
+ uint32_t buf = 0;
int nbytes = read (sink_, &buf, sizeof(buf));
- if (nbytes != sizeof(MARKER)) {
+ if ((nbytes != sizeof(MARKER) || (buf != MARKER))) {
+ // If there's an error get the error message than close
+ // the pipe. This should ensure any further use of the socket
+ // or testing the fd with select_fd will fail.
const char* errstr = strerror(errno);
+ closeSocket();
isc_throw(WatchSocketError, "WatchSocket clearReady failed:"
- << " bytes read: " << nbytes << " : " << errstr);
+ << " bytes read: " << nbytes << " : "
+ << " value read: " << buf << " error :" <<errstr);
+ }
+ }
+}
+
+void
+WatchSocket::closeSocket() {
+ // Close the pipe fds. Technically a close can fail (hugely unlikely)
+ // but there's no recovery for it either. If one does fail we log it
+ // and go on. Plus this is called by the destructor and no one likes
+ // destructors that throw.
+ if (source_ != INVALID_SOCKET) {
+ if (close(source_)) {
+ const char* errstr = strerror(errno);
+ LOG_ERROR(dhcp_ddns_logger, DHCP_DDNS_WATCH_SOURCE_CLOSE_ERROR)
+ .arg(errstr);
+ }
+
+ source_ = INVALID_SOCKET;
+ }
+
+ if (sink_ != INVALID_SOCKET) {
+ if (close(sink_)) {
+ const char* errstr = strerror(errno);
+ LOG_ERROR(dhcp_ddns_logger, DHCP_DDNS_WATCH_SINK_CLOSE_ERROR)
+ .arg(errstr);
}
- ready_flag_ = false;
+ sink_ = INVALID_SOCKET;
}
}
-int
+int
WatchSocket::getSelectFd() {
return (sink_);
}
diff --git a/src/lib/dhcp_ddns/watch_socket.h b/src/lib/dhcp_ddns/watch_socket.h
index d0113f7..f067ea6 100644
--- a/src/lib/dhcp_ddns/watch_socket.h
+++ b/src/lib/dhcp_ddns/watch_socket.h
@@ -44,11 +44,20 @@ public:
/// to the pipe. To clear the socket, the marker is read from the pipe. Note
/// that WatchSocket will only write the marker if it is not already marked.
/// This prevents the socket's pipe from filling endlessly.
+///
+/// @warning Because the read or "sink" side of the pipe is used as the select_fd,
+/// it is possible for that fd to be interfered with, albeit only from within the
+/// process space which owns it. Performing operations that may alter the fd's state
+/// such as close, read, or altering behavior flags with fcntl or ioctl can have
+/// unpredictable results. It is intended strictly use with functions such as select()
+/// poll() or their variants.
class WatchSocket {
public:
/// @brief Value used to signify an invalid descriptor.
static const int INVALID_SOCKET = -1;
/// @brief Value written to the source when marking the socket as ready.
+ /// The value itself is arbitrarily chosen as one that is unlikely to occur
+ /// otherwise and easy to debug.
static const uint32_t MARKER = 0xDEADBEEF;
/// @brief Constructor
@@ -65,16 +74,32 @@ public:
/// @brief Marks the select-fd as ready to read.
///
/// Marks the socket as ready to read, if is not already so marked.
+ /// If an error occurs, closeSocket is called. This will force any further
+ /// use of the select_fd to fail rather than show the fd as READY. Such
+ /// an error is almost surely a programmatic error which has corrupted the
+ /// select_fd.
///
/// @throw WatchSocketError if an error occurs marking the socket.
void markReady();
/// @brief Returns true the if socket is marked as ready.
+ ///
+ /// This method uses a non-blocking call to select() to test read state of the
+ /// select_fd. Rather than track what the status "should be" it tests the status.
+ /// This should eliminate conditions where the select-fd appear to be perpetually
+ /// ready.
+ /// @return Returns true if select_fd is not INVALID_SOCKET and select() reports it
+ /// as !EWOULDBLOCK, otherwise it returns false.
+ /// This method is guaranteed NOT to throw.
bool isReady();
/// @brief Clears the socket's ready to read marker.
///
/// Clears the socket if it is currently marked as ready to read.
+ /// If an error occurs, closeSocket is called. This will force any further
+ /// use of the select_fd to fail rather than show the fd as READY. Such
+ /// an error is almost surely a programmatic error which has corrupted the
+ /// select_fd.
///
/// @throw WatchSocketError if an error occurs clearing the socket
/// marker.
@@ -90,15 +115,18 @@ public:
int getSelectFd();
private:
+ /// @brief Closes the descriptors associated with the socket.
+ ///
+ /// Used internally in the destructor and if an error occurs marking or
+ /// clearing the socket.
+ void closeSocket();
+
/// @brief The end of the pipe to which the marker is written
int source_;
/// @brief The end of the pipe from which the marker is read.
/// This is the value returned as the select-fd.
int sink_;
-
- /// @brief True the socket is currently marked ready to read.
- bool ready_flag_;
};
/// @brief Defines a smart pointer to an instance of a WatchSocket.
diff --git a/src/lib/dhcpsrv/d2_client.cc b/src/lib/dhcpsrv/d2_client.cc
index 70aebe4..c918735 100644
--- a/src/lib/dhcpsrv/d2_client.cc
+++ b/src/lib/dhcpsrv/d2_client.cc
@@ -173,7 +173,7 @@ D2ClientMgr::setD2ClientConfig(D2ClientConfigPtr& new_config) {
/// but are covered in Trac# 3328.
isc::asiolink::IOAddress any_addr("0.0.0.0");
uint32_t any_port = 0;
- uint32_t queueMax = 1024;
+ uint32_t queue_max = 1024;
// Instantiate a new sender.
new_sender.reset(new dhcp_ddns::NameChangeUDPSender(
@@ -181,7 +181,7 @@ D2ClientMgr::setD2ClientConfig(D2ClientConfigPtr& new_config) {
new_config->getServerIp(),
new_config->getServerPort(),
new_config->getNcrFormat(),
- *this, queueMax));
+ *this, queue_max));
break;
}
default:
diff --git a/src/lib/dhcpsrv/d2_client.h b/src/lib/dhcpsrv/d2_client.h
index 9112abf..7574fe1 100644
--- a/src/lib/dhcpsrv/d2_client.h
+++ b/src/lib/dhcpsrv/d2_client.h
@@ -232,7 +232,8 @@ boost::function<void(const dhcp_ddns::NameChangeSender::Result result,
/// provides services to store, update, and access the current dhcp-ddns
/// configuration. As for b10-dhcp-ddns communications, D2ClientMgr creates
/// maintains a NameChangeSender appropriate to the current configuration and
-/// provides services to start, stop, and post NCRs to the sender. Additionally/// there are methods to examine the queue of requests currently waiting for
+/// provides services to start, stop, and post NCRs to the sender. Additionally
+/// there are methods to examine the queue of requests currently waiting for
/// transmission.
///
/// The manager also provides the mechanics to integrate the ASIO-based IO
diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes
index b964103..e8dd2e7 100644
--- a/src/lib/dhcpsrv/dhcpsrv_messages.mes
+++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes
@@ -1,4 +1,4 @@
-# Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
+# Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC")
#
# Permission to use, copy, modify, and/or distribute this software for any
# purpose with or without fee is hereby granted, provided that the above
diff --git a/src/lib/dhcpsrv/tests/d2_udp_unittest.cc b/src/lib/dhcpsrv/tests/d2_udp_unittest.cc
index 67475a6..22893cc 100644
--- a/src/lib/dhcpsrv/tests/d2_udp_unittest.cc
+++ b/src/lib/dhcpsrv/tests/d2_udp_unittest.cc
@@ -28,7 +28,7 @@
#include <gtest/gtest.h>
#include <sys/select.h>
-#include <sys/ioctl.h>
+//#include <sys/ioctl.h>
using namespace std;
using namespace isc::dhcp;
More information about the bind10-changes
mailing list