BIND 10 trac3195, updated. 72e601f2a57ab70b25d50877c8e49242739d1c9f [3195] Changes after review:
BIND 10 source code commits
bind10-changes at lists.isc.org
Tue Oct 22 18:18:49 UTC 2013
The branch, trac3195 has been updated
via 72e601f2a57ab70b25d50877c8e49242739d1c9f (commit)
from 358735b6bc9fe4caf2b12466e54a90eb69a4e673 (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 72e601f2a57ab70b25d50877c8e49242739d1c9f
Author: Tomek Mrugalski <tomasz at isc.org>
Date: Tue Oct 22 20:18:30 2013 +0200
[3195] Changes after review:
- Guide updated
- Iface::addUnicast() now refeses duplicates
- addUnicast and clearUnicasts are now commented
- new unit-tests
- argument for addActiveIface is const reference again
-
-----------------------------------------------------------------------
Summary of changes:
doc/guide/bind10-guide.xml | 6 +++++
src/lib/dhcp/iface_mgr.cc | 33 +++++++++++++++++++++++-----
src/lib/dhcp/iface_mgr.h | 15 ++++++++++---
src/lib/dhcp/tests/iface_mgr_unittest.cc | 18 +++++++++++++++
src/lib/dhcpsrv/cfgmgr.cc | 15 +++++++------
src/lib/dhcpsrv/cfgmgr.h | 6 +++--
src/lib/dhcpsrv/tests/cfgmgr_unittest.cc | 35 ++++++++++++++++++++++++------
7 files changed, 103 insertions(+), 25 deletions(-)
-----------------------------------------------------------------------
diff --git a/doc/guide/bind10-guide.xml b/doc/guide/bind10-guide.xml
index 4635fe9..edbdff3 100644
--- a/doc/guide/bind10-guide.xml
+++ b/doc/guide/bind10-guide.xml
@@ -4696,6 +4696,12 @@ Dhcp6/subnet6/ list
on the Dhcp6/interface list. It is not possible to specify more than one
unicast address on a given interface.
</para>
+ <para>
+ Care should be taken to specify proper unicast addresses. The server will
+ attempt to bind to those addresses specified, without any additional checks.
+ That approach is selected on purpose, so in the software can be used to
+ communicate over uncommon addresses if the administrator desires so.
+ </para>
</section>
<section>
diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc
index 19860b6..0ad35a5 100644
--- a/src/lib/dhcp/iface_mgr.cc
+++ b/src/lib/dhcp/iface_mgr.cc
@@ -177,6 +177,17 @@ IfaceMgr::IfaceMgr()
}
}
+void Iface::addUnicast(const isc::asiolink::IOAddress& addr) {
+ for (Iface::AddressCollection::const_iterator i = unicasts_.begin();
+ i != unicasts_.end(); ++i) {
+ if (*i == addr) {
+ isc_throw(BadValue, "Address " << addr.toText()
+ << " already defined on the " << name_ << " interface.");
+ }
+ }
+ unicasts_.push_back(addr);
+}
+
void IfaceMgr::closeSockets() {
for (IfaceCollection::iterator iface = ifaces_.begin();
iface != ifaces_.end(); ++iface) {
@@ -343,8 +354,10 @@ bool IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast) {
}
if (sock < 0) {
+ const char* errstr = strerror(errno);
isc_throw(SocketConfigError, "failed to open IPv4 socket"
- << " supporting broadcast traffic");
+ << " supporting broadcast traffic, reason:"
+ << errstr);
}
count++;
@@ -375,8 +388,10 @@ bool IfaceMgr::openSockets6(const uint16_t port) {
sock = openSocket(iface->getName(), *addr, port);
if (sock < 0) {
+ const char* errstr = strerror(errno);
isc_throw(SocketConfigError, "failed to open unicast socket on "
- << addr->toText() << " on interface " << iface->getName());
+ << addr->toText() << " on interface " << iface->getName()
+ << ", reason: " << errstr);
}
count++;
@@ -404,9 +419,10 @@ bool IfaceMgr::openSockets6(const uint16_t port) {
sock = openSocket(iface->getName(), *addr, port);
if (sock < 0) {
+ const char* errstr = strerror(errno);
isc_throw(SocketConfigError, "failed to open link-local socket on "
<< addr->toText() << " on interface "
- << iface->getName());
+ << iface->getName() << ", reason: " << errstr);
}
// Binding socket to unicast address and then joining multicast group
@@ -431,8 +447,10 @@ bool IfaceMgr::openSockets6(const uint16_t port) {
IOAddress(ALL_DHCP_RELAY_AGENTS_AND_SERVERS),
port);
if (sock2 < 0) {
+ const char* errstr = strerror(errno);
isc_throw(SocketConfigError, "Failed to open multicast socket on "
- << " interface " << iface->getFullName());
+ << " interface " << iface->getFullName() << ", reason:"
+ << errstr);
iface->delSocket(sock); // delete previously opened socket
}
#endif
@@ -625,7 +643,9 @@ IfaceMgr::getLocalAddress(const IOAddress& remote_addr, const uint16_t port) {
// interface.
sock.open(asio::ip::udp::v4(), err_code);
if (err_code) {
- isc_throw(Unexpected, "failed to open UDPv4 socket");
+ const char* errstr = strerror(errno);
+ isc_throw(Unexpected, "failed to open UDPv4 socket, reason:"
+ << errstr);
}
sock.set_option(asio::socket_base::broadcast(true), err_code);
if (err_code) {
@@ -1190,7 +1210,7 @@ uint16_t IfaceMgr::getSocket(const isc::dhcp::Pkt6& pkt) {
if ( (pkt.getRemoteAddr().getAddress().to_v6().is_link_local() &&
s->addr_.getAddress().to_v6().is_link_local()) ||
(!pkt.getRemoteAddr().getAddress().to_v6().is_link_local() &&
- s->addr_.getAddress().to_v6().is_link_local()) ) {
+ !s->addr_.getAddress().to_v6().is_link_local()) ) {
candidate = s;
}
}
@@ -1226,5 +1246,6 @@ uint16_t IfaceMgr::getSocket(isc::dhcp::Pkt4 const& pkt) {
<< " does not have any suitable IPv4 sockets open.");
}
+
} // end of namespace isc::dhcp
} // end of namespace isc
diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h
index 87488e8..2f48813 100644
--- a/src/lib/dhcp/iface_mgr.h
+++ b/src/lib/dhcp/iface_mgr.h
@@ -264,14 +264,23 @@ public:
/// @return collection of sockets added to interface
const SocketCollection& getSockets() const { return sockets_; }
+ /// @brief Removes any unicast addresses
+ ///
+ /// Removes any unicast addresses that the server was configured to
+ /// listen on
void clearUnicasts() {
unicasts_.clear();
}
- void addUnicast(const isc::asiolink::IOAddress& addr) {
- unicasts_.push_back(addr);
- }
+ /// @brief Adds unicast the server should listen on
+ ///
+ /// @throw BadValue if specified address is already defined on interface
+ /// @param addr unicast address to listen on
+ void addUnicast(const isc::asiolink::IOAddress& addr);
+ /// @brief Returns a container of addresses the server should listen on
+ ///
+ /// @return address collection (may be empty)
const AddressCollection& getUnicasts() const {
return unicasts_;
}
diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc
index 2518f46..b936b5c 100644
--- a/src/lib/dhcp/tests/iface_mgr_unittest.cc
+++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc
@@ -802,6 +802,7 @@ TEST_F(IfaceMgrTest, sendReceive6) {
// try to send/receive data over the closed socket. Closed socket's descriptor is
// still being hold by IfaceMgr which will try to use it to receive data.
close(socket1);
+ close(socket2);
EXPECT_THROW(ifacemgr->receive6(10), SocketReadError);
EXPECT_THROW(ifacemgr->send(sendPkt), SocketWriteError);
}
@@ -1579,6 +1580,23 @@ TEST_F(IfaceMgrTest, DISABLED_openUnicastSockets) {
EXPECT_TRUE(getSocketByAddr(sockets, IOAddress("figure-out-link-local-addr")));
}
+// Checks if there is a protection against unicast duplicates.
+TEST_F(IfaceMgrTest, unicastDuplicates) {
+ NakedIfaceMgr ifacemgr;
+
+ Iface* iface = ifacemgr.getIface(LOOPBACK);
+ if (iface == NULL) {
+ cout << "Local loopback interface not found. Skipping test. " << endl;
+ return;
+ }
+
+ // Tell the interface that it should bind to this global interface
+ EXPECT_NO_THROW(iface->addUnicast(IOAddress("2001:db8::1")));
+
+ // Tell the interface that it should bind to this global interface
+ EXPECT_THROW(iface->addUnicast(IOAddress("2001:db8::1")), BadValue);
+}
+
// This test requires addresses 2001:db8:15c::1/128 and fe80::1/64 to be
// configured on loopback interface
//
diff --git a/src/lib/dhcpsrv/cfgmgr.cc b/src/lib/dhcpsrv/cfgmgr.cc
index f0f6886..b4fb11d 100644
--- a/src/lib/dhcpsrv/cfgmgr.cc
+++ b/src/lib/dhcpsrv/cfgmgr.cc
@@ -269,16 +269,17 @@ std::string CfgMgr::getDataDir() {
}
void
-CfgMgr::addActiveIface(std::string iface) {
+CfgMgr::addActiveIface(const std::string& iface) {
size_t pos = iface.find("/");
+ std::string iface_copy = iface;
if (pos != std::string::npos) {
std::string addr_string = iface.substr(pos + 1);
try {
IOAddress addr(addr_string);
- iface = iface.substr(0,pos);
- unicast_addrs_.insert(make_pair(iface, addr));
+ iface_copy = iface.substr(0,pos);
+ unicast_addrs_.insert(make_pair(iface_copy, addr));
} catch (...) {
isc_throw(BadValue, "Can't convert '" << addr_string
<< "' into address in interface defition ('"
@@ -286,14 +287,14 @@ CfgMgr::addActiveIface(std::string iface) {
}
}
- if (isIfaceListedActive(iface)) {
+ if (isIfaceListedActive(iface_copy)) {
isc_throw(DuplicateListeningIface,
- "attempt to add duplicate interface '" << iface << "'"
+ "attempt to add duplicate interface '" << iface_copy << "'"
" to the set of interfaces on which server listens");
}
LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE, DHCPSRV_CFGMGR_ADD_IFACE)
- .arg(iface);
- active_ifaces_.push_back(iface);
+ .arg(iface_copy);
+ active_ifaces_.push_back(iface_copy);
}
void
diff --git a/src/lib/dhcpsrv/cfgmgr.h b/src/lib/dhcpsrv/cfgmgr.h
index fc8bf9d..b063f92 100644
--- a/src/lib/dhcpsrv/cfgmgr.h
+++ b/src/lib/dhcpsrv/cfgmgr.h
@@ -272,7 +272,7 @@ public:
/// server should listen.
///
/// @param iface A name of the interface being added to the listening set.
- void addActiveIface(std::string iface);
+ void addActiveIface(const std::string& iface);
/// @brief Sets the flag which indicates that server is supposed to listen
/// on all available interfaces.
@@ -308,7 +308,9 @@ public:
/// @brief returns unicast a given interface should listen on (or NULL)
///
/// This method will return an address for a specified interface, if the
- /// server is supposed to listen on.
+ /// server is supposed to listen on unicast address. This address is
+ /// intended to be used immediately. This pointer is valid only until
+ /// the next configuration change.
///
/// @return IOAddress pointer (or NULL if none)
const isc::asiolink::IOAddress*
diff --git a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
index 31389d0..94f78c3 100644
--- a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
@@ -579,14 +579,14 @@ TEST_F(CfgMgrTest, optionSpace6) {
TEST_F(CfgMgrTest, addActiveIface) {
CfgMgr& cfg_mgr = CfgMgr::instance();
- cfg_mgr.addActiveIface("eth0");
- cfg_mgr.addActiveIface("eth1");
+ EXPECT_NO_THROW(cfg_mgr.addActiveIface("eth0"));
+ EXPECT_NO_THROW(cfg_mgr.addActiveIface("eth1"));
EXPECT_TRUE(cfg_mgr.isActiveIface("eth0"));
EXPECT_TRUE(cfg_mgr.isActiveIface("eth1"));
EXPECT_FALSE(cfg_mgr.isActiveIface("eth2"));
- cfg_mgr.deleteActiveIfaces();
+ EXPECT_NO_THROW(cfg_mgr.deleteActiveIfaces());
EXPECT_FALSE(cfg_mgr.isActiveIface("eth0"));
EXPECT_FALSE(cfg_mgr.isActiveIface("eth1"));
@@ -599,9 +599,9 @@ TEST_F(CfgMgrTest, addActiveIface) {
TEST_F(CfgMgrTest, addUnicastAddresses) {
CfgMgr& cfg_mgr = CfgMgr::instance();
- cfg_mgr.addActiveIface("eth1/2001:db8::1");
- cfg_mgr.addActiveIface("eth2/2001:db8::2");
- cfg_mgr.addActiveIface("eth3");
+ EXPECT_NO_THROW(cfg_mgr.addActiveIface("eth1/2001:db8::1"));
+ EXPECT_NO_THROW(cfg_mgr.addActiveIface("eth2/2001:db8::2"));
+ EXPECT_NO_THROW(cfg_mgr.addActiveIface("eth3"));
EXPECT_TRUE(cfg_mgr.isActiveIface("eth1"));
EXPECT_TRUE(cfg_mgr.isActiveIface("eth2"));
@@ -614,7 +614,7 @@ TEST_F(CfgMgrTest, addUnicastAddresses) {
EXPECT_FALSE(cfg_mgr.getUnicast("eth3"));
EXPECT_FALSE(cfg_mgr.getUnicast("eth4"));
- cfg_mgr.deleteActiveIfaces();
+ EXPECT_NO_THROW(cfg_mgr.deleteActiveIfaces());
EXPECT_FALSE(cfg_mgr.isActiveIface("eth1"));
EXPECT_FALSE(cfg_mgr.isActiveIface("eth2"));
@@ -653,6 +653,27 @@ TEST_F(CfgMgrTest, activateAllIfaces) {
EXPECT_FALSE(cfg_mgr.isActiveIface("eth2"));
}
+/// @todo Add unit-tests for testing:
+/// - addActiveIface() with invalid interface name
+/// - addActiveIface() with the same interface twice
+/// - addActiveIface() with a bogus address
+///
+/// This is somewhat tricky. Care should be taken here, because it is rather
+/// difficult to decide if interface name is valid or not. Some servers, e.g.
+/// dibbler, allow to specify interface names that are not currently present in
+/// the system. The server accepts them, but upon discovering that they are
+/// yet available (for different definitions of not being available), adds
+/// the to to-be-activated list.
+///
+/// Cases covered by dibbler are:
+/// - missing interface (e.g. PPP connection that is not established yet)
+/// - downed interface (no link local address, no way to open sockets)
+/// - up, but not running interface (wifi up, but not associated)
+/// - tentative addresses (interface up and running, but DAD procedure is
+/// still in progress)
+/// - weird interfaces without link-local addresses (don't ask, 6rd tunnels
+/// look weird to me as well)
+
// No specific tests for getSubnet6. That method (2 overloaded versions) is tested
// in Dhcpv6SrvTest.selectSubnetAddr and Dhcpv6SrvTest.selectSubnetIface
// (see src/bin/dhcp6/tests/dhcp6_srv_unittest.cc)
More information about the bind10-changes
mailing list