BIND 10 trac3252, updated. 68b6c2b5001d19eb88ea10bb1c40744cbe1d33e5 [3252] Addressed review comments.
BIND 10 source code commits
bind10-changes at lists.isc.org
Tue Jan 14 13:07:26 UTC 2014
The branch, trac3252 has been updated
via 68b6c2b5001d19eb88ea10bb1c40744cbe1d33e5 (commit)
from d0da7ccdbf880ce20772bf69a985e43e7f083912 (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 68b6c2b5001d19eb88ea10bb1c40744cbe1d33e5
Author: Marcin Siodelski <marcin at isc.org>
Date: Tue Jan 14 14:07:05 2014 +0100
[3252] Addressed review comments.
-----------------------------------------------------------------------
Summary of changes:
src/lib/dhcp/iface_mgr.cc | 28 ++++++++++++------
src/lib/dhcp/iface_mgr.h | 46 ++++++++++++++++++++++--------
src/lib/dhcp/tests/iface_mgr_unittest.cc | 21 ++++++--------
3 files changed, 63 insertions(+), 32 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc
index 741b798..8b8e162 100644
--- a/src/lib/dhcp/iface_mgr.cc
+++ b/src/lib/dhcp/iface_mgr.cc
@@ -60,14 +60,16 @@
/// @param handler Error handler function to be called or NULL to indicate
/// that exception should be thrown instead.
/// @param stream stream object holding an error string.
-#define ifacemgr_error(ex_type, handler, stream) \
+#define IFACEMGR_ERROR(ex_type, handler, stream) \
+{ \
std::ostringstream oss__; \
oss__ << stream; \
if (handler) { \
handler(oss__.str()); \
} else { \
isc_throw(ex_type, oss__); \
- }
+ } \
+} \
using namespace std;
using namespace isc::asiolink;
@@ -405,7 +407,7 @@ IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast,
// bind to INADDR_ANY address but we can do it only once. Thus,
// if one socket has been bound we can't do it any further.
if (!bind_to_device && bcast_num > 0) {
- ifacemgr_error(SocketConfigError, error_handler,
+ IFACEMGR_ERROR(SocketConfigError, error_handler,
"SO_BINDTODEVICE socket option is"
" not supported on this OS;"
" therefore, DHCP server can only"
@@ -419,7 +421,7 @@ IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast,
// open at least one more.
openSocket(iface->getName(), *addr, port, true, true);
} catch (const Exception& ex) {
- ifacemgr_error(SocketConfigError, error_handler,
+ IFACEMGR_ERROR(SocketConfigError, error_handler,
"failed to open socket on interface "
<< iface->getName() << ", reason: "
<< ex.what());
@@ -439,7 +441,7 @@ IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast,
// Not broadcast capable, do not set broadcast flags.
openSocket(iface->getName(), *addr, port, false, false);
} catch (const Exception& ex) {
- ifacemgr_error(SocketConfigError, error_handler,
+ IFACEMGR_ERROR(SocketConfigError, error_handler,
"failed to open socket on interface "
<< iface->getName() << ", reason: "
<< ex.what());
@@ -479,7 +481,7 @@ IfaceMgr::openSockets6(const uint16_t port,
openSocket(iface->getName(), *addr, port);
} catch (const Exception& ex) {
- ifacemgr_error(SocketConfigError, error_handler,
+ IFACEMGR_ERROR(SocketConfigError, error_handler,
"Failed to open unicast socket on interface "
<< iface->getName() << ", reason: "
<< ex.what());
@@ -517,13 +519,23 @@ IfaceMgr::openSockets6(const uint16_t port,
// it for some odd use cases which may utilize non-multicast
// interfaces. Perhaps a warning should be emitted if the
// interface is not a multicast one.
+
+ // The sock variable will hold a socket descriptor. It may be
+ // used to close a socket if the function fails to bind to
+ // multicast address on Linux systems. Because we only bind
+ // a socket to multicast address on Linux, on other systems
+ // the sock variable will be initialized but unused. We have
+ // to suppress the cppcheck warning which shows up on non-Linux
+ // systems.
+ // cppcheck-suppress variableScope
int sock;
try {
+ // cppcheck-suppress unreadVariable
sock = openSocket(iface->getName(), *addr, port,
iface->flag_multicast_);
} catch (const Exception& ex) {
- ifacemgr_error(SocketConfigError, error_handler,
+ IFACEMGR_ERROR(SocketConfigError, error_handler,
"Failed to open link-local socket on "
" interface " << iface->getName() << ": "
<< ex.what());
@@ -545,7 +557,7 @@ IfaceMgr::openSockets6(const uint16_t port,
} catch (const Exception& ex) {
// Delete previously opened socket.
iface->delSocket(sock);
- ifacemgr_error(SocketConfigError, error_handler,
+ IFACEMGR_ERROR(SocketConfigError, error_handler,
"Failed to open multicast socket on"
" interface " << iface->getName()
<< ", reason: " << ex.what());
diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h
index bdae0af..09afa68 100644
--- a/src/lib/dhcp/iface_mgr.h
+++ b/src/lib/dhcp/iface_mgr.h
@@ -629,18 +629,33 @@ public:
/// @brief Opens IPv6 sockets on detected interfaces.
///
- /// @todo This function will throw an exception immediately when a socket
- /// fails to open. This is undersired behavior because it will preclude
- /// other sockets from opening. We should strive to provide similar mechanism
- /// that has been introduced for V4 sockets. If socket creation fails the
- /// appropriate error handler is called and once the handler returns the
- /// function contnues to open other sockets. The change in the IfaceMgr
- /// is quite straight forward and it is proven to work for V4. However,
- /// unit testing it is a bit involved, because for unit testing we need
- /// a replacement of the openSocket6 function which will mimic the
- /// behavior of the real socket opening. For the V4 we have the means to
- /// to achieve that with the replaceable PktFilter class. For V6, the
- /// implementation is hardcoded in the openSocket6.
+ /// On the systems with multiple interfaces, it is often desired that the
+ /// failure to open a socket on a particular interface doesn't cause a
+ /// fatal error and sockets should be opened on remaining interfaces.
+ /// However, the warning about the failure for the particular socket should
+ /// be communicated to the caller. The libdhcp++ is a common library with
+ /// no logger associated with it. Most of the functions in this library
+ /// communicate errors via exceptions. In case of openSockets6 function
+ /// exception must not be thrown if the function is supposed to continue
+ /// opening sockets, despite an error. Therefore, if such a behavior is
+ /// desired, the error handler function can be passed as a parameter.
+ /// This error handler is called (if present) with an error string.
+ /// Typically, error handler will simply log an error using an application
+ /// logger, but it can do more sophisticated error handling too.
+ ///
+ /// @todo It is possible that additional parameters will have to be added
+ /// to the error handler, e.g. Iface if it was really supposed to do
+ /// some more sophisticated error handling.
+ ///
+ /// If the error handler is not installed (is NULL), the exception is thrown
+ /// for each failure (default behavior).
+ ///
+ /// @warning This function does not check if there has been any sockets
+ /// already open by the @c IfaceMgr. Therefore a caller should call
+ /// @c IfaceMgr::closeSockets(AF_INET6) before calling this function.
+ /// If there are any sockets open, the function may either throw an
+ /// exception or invoke an error handler on attempt to bind the new socket
+ /// to the same address and port.
///
/// @param port specifies port number (usually DHCP6_SERVER_PORT)
/// @param error_handler A pointer to an error handler function which is
@@ -705,6 +720,13 @@ public:
/// If the error handler is not installed (is NULL), the exception is thrown
/// for each failure (default behavior).
///
+ /// @warning This function does not check if there has been any sockets
+ /// already open by the @c IfaceMgr. Therefore a caller should call
+ /// @c IfaceMgr::closeSockets(AF_INET) before calling this function.
+ /// If there are any sockets open, the function may either throw an
+ /// exception or invoke an error handler on attempt to bind the new socket
+ /// to the same address and port.
+ ///
/// @param port specifies port number (usually DHCP4_SERVER_PORT)
/// @param use_bcast configure sockets to support broadcast messages.
/// @param error_handler A pointer to an error handler function which is
diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc
index 00ab342..d538dc8 100644
--- a/src/lib/dhcp/tests/iface_mgr_unittest.cc
+++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc
@@ -1457,9 +1457,7 @@ TEST_F(IfaceMgrTest, openSocket4ErrorHandler) {
ASSERT_TRUE(custom_packet_filter);
ASSERT_NO_THROW(ifacemgr.setPacketFilter(custom_packet_filter));
- // Open socket on eth0. The openSockets4 should detect that this
- // socket has been already open and an attempt to open another socket
- // and bind to this address and port should fail.
+ // Open socket on eth0.
ASSERT_NO_THROW(ifacemgr.openSocket("eth0", IOAddress("10.0.0.1"),
DHCP4_SERVER_PORT));
@@ -1467,6 +1465,9 @@ TEST_F(IfaceMgrTest, openSocket4ErrorHandler) {
// should be called when the IfaceMgr fails to open socket on eth0.
isc::dhcp::IfaceMgrErrorMsgCallback error_handler =
boost::bind(&IfaceMgrTest::ifaceMgrErrorHandler, this, _1);
+ // The openSockets4 should detect that there is another socket already
+ // open and bound to the same address and port. An attempt to open
+ // another socket and bind to this address and port should fail.
ASSERT_NO_THROW(ifacemgr.openSockets4(DHCP4_SERVER_PORT, true, error_handler));
// We expect that an error occured when we tried to open a socket on
// eth0, but the socket on eth1 should open just fine.
@@ -1849,9 +1850,7 @@ TEST_F(IfaceMgrTest, openSocket6ErrorHandler) {
ASSERT_TRUE(filter);
ASSERT_NO_THROW(ifacemgr.setPacketFilter(filter));
- // Open socket on eth0. The openSockets6 should detect that this
- // socket has been already open and an attempt to open another socket
- // and bind to this address and port should fail.
+ // Open socket on eth0.
ASSERT_NO_THROW(ifacemgr.openSocket("eth0",
IOAddress("fe80::3a60:77ff:fed5:cdef"),
DHCP6_SERVER_PORT));
@@ -1860,12 +1859,10 @@ TEST_F(IfaceMgrTest, openSocket6ErrorHandler) {
// should be called when the IfaceMgr fails to open socket on eth0.
isc::dhcp::IfaceMgrErrorMsgCallback error_handler =
boost::bind(&IfaceMgrTest::ifaceMgrErrorHandler, this, _1);
- // ASSERT_NO_THROW(ifacemgr.openSockets6(DHCP6_SERVER_PORT, error_handler));
- try {
- ifacemgr.openSockets6(DHCP6_SERVER_PORT, error_handler);
- } catch (const Exception& ex) {
- std::cout << ex.what() << std::endl;
- }
+ // The openSockets6 should detect that a socket has been already
+ // opened on eth0 and an attempt to open another socket and bind to
+ // the same address and port should fail.
+ ASSERT_NO_THROW(ifacemgr.openSockets6(DHCP6_SERVER_PORT, error_handler));
// We expect that an error occured when we tried to open a socket on
// eth0, but the socket on eth1 should open just fine.
EXPECT_EQ(1, errors_count_);
More information about the bind10-changes
mailing list