BIND 10 trac2994, updated. 89d6be56eefa1ed79d9c0cfb8112243e1435beb6 [2994] Changes after review:
BIND 10 source code commits
bind10-changes at lists.isc.org
Fri Jul 19 13:54:20 UTC 2013
The branch, trac2994 has been updated
via 89d6be56eefa1ed79d9c0cfb8112243e1435beb6 (commit)
from 0657510f32552e1d2510ec34e807d41c1a0f964e (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 89d6be56eefa1ed79d9c0cfb8112243e1435beb6
Author: Tomek Mrugalski <tomasz at isc.org>
Date: Fri Jul 19 15:54:02 2013 +0200
[2994] Changes after review:
- pointers to DHCPv4 and DHCPv6 hooks removed
- Many methods in HooksManager are now called directly in
dhcp4_srv.cc, dhcp4_srv_unittest.cc and alloc_engine_unittest.cc
- Comments wrapped up differently in dhcp4_srv_unittest.cc
- Comments clarified in alloc_engine.{cc|h}
-----------------------------------------------------------------------
Summary of changes:
doc/devel/mainpage.dox | 2 --
src/bin/dhcp4/dhcp4_srv.cc | 34 ++++++++++++------------
src/bin/dhcp4/tests/dhcp4_srv_unittest.cc | 29 ++++++++++----------
src/lib/dhcpsrv/alloc_engine.cc | 14 +++-------
src/lib/dhcpsrv/alloc_engine.h | 6 +++--
src/lib/dhcpsrv/tests/alloc_engine_unittest.cc | 22 ++++++++-------
6 files changed, 53 insertions(+), 54 deletions(-)
-----------------------------------------------------------------------
diff --git a/doc/devel/mainpage.dox b/doc/devel/mainpage.dox
index 799ee37..9b8d2a1 100644
--- a/doc/devel/mainpage.dox
+++ b/doc/devel/mainpage.dox
@@ -51,13 +51,11 @@
* - @subpage dhcpv4ConfigParser
* - @subpage dhcpv4ConfigInherit
* - @subpage dhcpv4Other
- * - @subpage dhcpv4Hooks
* - @subpage dhcp6
* - @subpage dhcpv6Session
* - @subpage dhcpv6ConfigParser
* - @subpage dhcpv6ConfigInherit
* - @subpage dhcpv6Other
- * - @subpage dhcpv6Hooks
* - @subpage libdhcp
* - @subpage libdhcpIntro
* - @subpage libdhcpRelay
diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index c98651f..33458f8 100644
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -134,9 +134,6 @@ Dhcpv4Srv::Dhcpv4Srv(uint16_t port, const char* dbconfig, const bool use_bcast,
hook_index_pkt4_send_ = Hooks.hook_index_pkt4_send_;
/// @todo call loadLibraries() when handling configuration changes
- vector<string> libraries; // no libraries at this time
- HooksManager::loadLibraries(libraries);
-
} catch (const std::exception &e) {
LOG_ERROR(dhcp4_logger, DHCP4_SRV_CONSTRUCT_ERROR).arg(e.what());
@@ -157,11 +154,13 @@ Dhcpv4Srv::shutdown() {
shutdown_ = true;
}
-Pkt4Ptr Dhcpv4Srv::receivePacket(int timeout) {
+Pkt4Ptr
+Dhcpv4Srv::receivePacket(int timeout) {
return (IfaceMgr::instance().receive4(timeout));
}
-void Dhcpv4Srv::sendPacket(const Pkt4Ptr& packet) {
+void
+Dhcpv4Srv::sendPacket(const Pkt4Ptr& packet) {
IfaceMgr::instance().send(packet);
}
@@ -200,7 +199,7 @@ Dhcpv4Srv::run() {
.arg(query->toText());
// Let's execute all callouts registered for packet_received
- if (HooksManager::getHooksManager().calloutsPresent(hook_index_pkt4_receive_)) {
+ if (HooksManager::calloutsPresent(hook_index_pkt4_receive_)) {
CalloutHandlePtr callout_handle = getCalloutHandle(query);
// Delete previously set arguments
@@ -210,8 +209,8 @@ Dhcpv4Srv::run() {
callout_handle->setArgument("query4", query);
// Call callouts
- HooksManager::getHooksManager().callCallouts(hook_index_pkt4_receive_,
- *callout_handle);
+ HooksManager::callCallouts(hook_index_pkt4_receive_,
+ *callout_handle);
// Callouts decided to skip the next processing step. The next
// processing step would to process the packet, so skip at this
@@ -289,7 +288,7 @@ Dhcpv4Srv::run() {
rsp->setIndex(query->getIndex());
// Execute all callouts registered for packet6_send
- if (HooksManager::getHooksManager().calloutsPresent(hook_index_pkt4_send_)) {
+ if (HooksManager::calloutsPresent(hook_index_pkt4_send_)) {
CalloutHandlePtr callout_handle = getCalloutHandle(query);
// Delete all previous arguments
@@ -302,8 +301,8 @@ Dhcpv4Srv::run() {
callout_handle->setArgument("response4", rsp);
// Call all installed callouts
- HooksManager::getHooksManager().callCallouts(hook_index_pkt4_send_,
- *callout_handle);
+ HooksManager::callCallouts(hook_index_pkt4_send_,
+ *callout_handle);
// Callouts decided to skip the next processing step. The next
// processing step would to send the packet, so skip at this
@@ -885,8 +884,9 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) {
Subnet4Ptr subnet;
// Is this relayed message?
IOAddress relay = question->getGiaddr();
- if (relay.toText() != "0.0.0.0") {
+ static const IOAddress notset("0.0.0.0");
+ if (relay != notset) {
// Yes: Use relay address to select subnet
subnet = CfgMgr::instance().getSubnet4(relay);
} else {
@@ -898,7 +898,7 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) {
/// @todo Implement getSubnet4(interface-name)
// Let's execute all callouts registered for packet_received
- if (HooksManager::getHooksManager().calloutsPresent(hook_index_subnet4_select_)) {
+ if (HooksManager::calloutsPresent(hook_index_subnet4_select_)) {
CalloutHandlePtr callout_handle = getCalloutHandle(question);
// We're reusing callout_handle from previous calls
@@ -910,8 +910,7 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) {
callout_handle->setArgument("subnet4collection", CfgMgr::instance().getSubnets4());
// Call user (and server-side) callouts
- HooksManager::getHooksManager().callCallouts(hook_index_subnet4_select_,
- *callout_handle);
+ HooksManager::callCallouts(hook_index_subnet4_select_, *callout_handle);
// Callouts decided to skip this step. This means that no subnet will be
// selected. Packet processing will continue, but it will be severly limited
@@ -953,8 +952,9 @@ Dhcpv4Srv::sanityCheck(const Pkt4Ptr& pkt, RequirementLevel serverid) {
}
// If there is HWAddress set and it is non-empty, then we're good
- if (pkt->getHWAddr() && !pkt->getHWAddr()->hwaddr_.empty())
+ if (pkt->getHWAddr() && !pkt->getHWAddr()->hwaddr_.empty()) {
return;
+ }
// There has to be something to uniquely identify the client:
// either non-zero MAC address or client-id option present (or both)
@@ -989,7 +989,7 @@ isc::hooks::CalloutHandlePtr Dhcpv4Srv::getCalloutHandle(const Pkt4Ptr& pkt) {
// Remember the pointer to this packet
old_pointer = pkt;
- callout_handle = HooksManager::getHooksManager().createCalloutHandle();
+ callout_handle = HooksManager::createCalloutHandle();
}
return (callout_handle);
diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
index 7dc460c..458c5e7 100644
--- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
+++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
@@ -89,32 +89,28 @@ public:
/// @brief fakes packet reception
/// @param timeout ignored
///
- /// The method receives all packets queued in receive
- /// queue, one after another. Once the queue is empty,
- /// it initiates the shutdown procedure.
+ /// The method receives all packets queued in receive queue, one after
+ /// another. Once the queue is empty, it initiates the shutdown procedure.
///
/// See fake_received_ field for description
virtual Pkt4Ptr receivePacket(int /*timeout*/) {
- // If there is anything prepared as fake incoming
- // traffic, use it
+ // If there is anything prepared as fake incoming traffic, use it
if (!fake_received_.empty()) {
Pkt4Ptr pkt = fake_received_.front();
fake_received_.pop_front();
return (pkt);
}
- // If not, just trigger shutdown and
- // return immediately
+ // If not, just trigger shutdown and return immediately
shutdown();
return (Pkt4Ptr());
}
/// @brief fake packet sending
///
- /// Pretend to send a packet, but instead just store
- /// it in fake_send_ list where test can later inspect
- /// server's response.
+ /// Pretend to send a packet, but instead just store it in fake_send_ list
+ /// where test can later inspect server's response.
virtual void sendPacket(const Pkt4Ptr& pkt) {
fake_sent_.push_back(pkt);
}
@@ -157,11 +153,11 @@ static const char* SRVID_FILE = "server-id-test.txt";
/// @brief Dummy Packet Filtering class.
///
-/// This class reports capability to respond directly to the
-/// client which doesn't have address configured yet.
+/// This class reports capability to respond directly to the client which
+/// doesn't have address configured yet.
///
-/// All packet and socket handling functions do nothing because
-/// they are not used in unit tests.
+/// All packet and socket handling functions do nothing because they are not
+/// used in unit tests.
class PktFilterTest : public PktFilter {
public:
@@ -1690,6 +1686,11 @@ public:
/// @brief destructor (deletes Dhcpv4Srv)
virtual ~HooksDhcpv4SrvTest() {
+
+ HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("pkt4_receive");
+ HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("pkt4_send");
+ HooksManager::preCalloutsLibraryHandle().deregisterAllCallouts("subnet4_select");
+
delete srv_;
}
diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc
index f4b4c98..d7417d2 100644
--- a/src/lib/dhcpsrv/alloc_engine.cc
+++ b/src/lib/dhcpsrv/alloc_engine.cc
@@ -578,10 +578,10 @@ Lease4Ptr AllocEngine::reuseExpiredLease(Lease4Ptr& expired,
// Pass necessary arguments
- // Subnet from which we do the allocation (That's as far as we can go
- // with using SubnetPtr to point to Subnet4 object. Users should not
- // be confused with dynamic_pointer_casts. They should get a concrete
- // pointer (Subnet4Ptr) pointing to a Subnet4 object.
+ // Subnet from which we do the allocation. Convert the general subnet
+ // pointer to a pointer to a Subnet4. Note that because we are using
+ // boost smart pointers here, we need to do the cast using the boost
+ // version of dynamic_pointer_cast.
Subnet4Ptr subnet4 = boost::dynamic_pointer_cast<Subnet4>(subnet);
callout_handle->setArgument("subnet4", subnet4);
@@ -638,9 +638,6 @@ Lease6Ptr AllocEngine::createLease6(const Subnet6Ptr& subnet,
// Delete all previous arguments
callout_handle->deleteAllArguments();
- // Clear skip flag if it was set in previous callouts
- callout_handle->setSkip(false);
-
// Pass necessary arguments
// Subnet from which we do the allocation
@@ -723,9 +720,6 @@ Lease4Ptr AllocEngine::createLease4(const SubnetPtr& subnet,
// Delete all previous arguments
callout_handle->deleteAllArguments();
- // Clear skip flag if it was set in previous callouts
- callout_handle->setSkip(false);
-
// Pass necessary arguments
// Subnet from which we do the allocation (That's as far as we can go
diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h
index 639e333..08381ab 100644
--- a/src/lib/dhcpsrv/alloc_engine.h
+++ b/src/lib/dhcpsrv/alloc_engine.h
@@ -266,7 +266,8 @@ private:
/// @param hwaddr client's hardware address
/// @param addr an address that was selected and is confirmed to be available
/// @param callout_handle a callout handle (used in hooks). A lease callouts
- /// will be executed if this parameter is passed.
+ /// will be executed if this parameter is passed (and there are callouts
+ /// registered)
/// @param fake_allocation is this real i.e. REQUEST (false) or just picking
/// an address for DISCOVER that is not really allocated (true)
/// @return allocated lease (or NULL in the unlikely case of the lease just
@@ -288,7 +289,8 @@ private:
/// @param iaid IAID from the IA_NA container the client sent to us
/// @param addr an address that was selected and is confirmed to be available
/// @param callout_handle a callout handle (used in hooks). A lease callouts
- /// will be executed if this parameter is passed.
+ /// will be executed if this parameter is passed (and there are callouts
+ /// registered)
/// @param fake_allocation is this real i.e. REQUEST (false) or just picking
/// an address for SOLICIT that is not really allocated (true)
/// @return allocated lease (or NULL in the unlikely case of the lease just
diff --git a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
index 5a9ce29..c03cab6 100644
--- a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
+++ b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
@@ -1151,13 +1151,13 @@ TEST_F(HookAllocEngine6Test, lease6_select) {
// Initialize Hooks Manager
vector<string> libraries; // no libraries at this time
- HooksManager::getHooksManager().loadLibraries(libraries);
+ HooksManager::loadLibraries(libraries);
// Install pkt6_receive_callout
EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
"lease6_select", lease6_select_callout));
- CalloutHandlePtr callout_handle = HooksManager::getHooksManager().createCalloutHandle();
+ CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle();
Lease6Ptr lease = engine->allocateAddress6(subnet_, duid_, iaid_, IOAddress("::"),
false, callout_handle);
@@ -1181,7 +1181,7 @@ TEST_F(HookAllocEngine6Test, lease6_select) {
ASSERT_TRUE(callback_subnet6_);
EXPECT_EQ(subnet_->toText(), callback_subnet6_->toText());
- EXPECT_EQ(callback_fake_allocation_, false);
+ EXPECT_FALSE(callback_fake_allocation_);
// Check if all expected parameters are reported. It's a bit tricky, because
// order may be different. If the test starts failing, because someone tweaked
@@ -1190,6 +1190,10 @@ TEST_F(HookAllocEngine6Test, lease6_select) {
expected_argument_names.push_back("fake_allocation");
expected_argument_names.push_back("lease6");
expected_argument_names.push_back("subnet6");
+
+ sort(callback_argument_names_.begin(), callback_argument_names_.end());
+ sort(expected_argument_names.begin(), expected_argument_names.end());
+
EXPECT_TRUE(callback_argument_names_ == expected_argument_names);
}
@@ -1212,7 +1216,7 @@ TEST_F(HookAllocEngine6Test, change_lease6_select) {
// Initialize Hooks Manager
vector<string> libraries; // no libraries at this time
- HooksManager::getHooksManager().loadLibraries(libraries);
+ HooksManager::loadLibraries(libraries);
// Install a callout
EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
@@ -1220,7 +1224,7 @@ TEST_F(HookAllocEngine6Test, change_lease6_select) {
// Normally, dhcpv6_srv would passed the handle when calling allocateAddress6,
// but in tests we need to create it on our own.
- CalloutHandlePtr callout_handle = HooksManager::getHooksManager().createCalloutHandle();
+ CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle();
// Call allocateAddress6. Callouts should be triggered here.
Lease6Ptr lease = engine->allocateAddress6(subnet_, duid_, iaid_, IOAddress("::"),
@@ -1363,13 +1367,13 @@ TEST_F(HookAllocEngine4Test, lease4_select) {
// Initialize Hooks Manager
vector<string> libraries; // no libraries at this time
- HooksManager::getHooksManager().loadLibraries(libraries);
+ HooksManager::loadLibraries(libraries);
// Install pkt4_receive_callout
EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
"lease4_select", lease4_select_callout));
- CalloutHandlePtr callout_handle = HooksManager::getHooksManager().createCalloutHandle();
+ CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle();
Lease4Ptr lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_,
IOAddress("0.0.0.0"),
@@ -1424,7 +1428,7 @@ TEST_F(HookAllocEngine4Test, change_lease4_select) {
// Initialize Hooks Manager
vector<string> libraries; // no libraries at this time
- HooksManager::getHooksManager().loadLibraries(libraries);
+ HooksManager::loadLibraries(libraries);
// Install a callout
EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
@@ -1432,7 +1436,7 @@ TEST_F(HookAllocEngine4Test, change_lease4_select) {
// Normally, dhcpv4_srv would passed the handle when calling allocateAddress4,
// but in tests we need to create it on our own.
- CalloutHandlePtr callout_handle = HooksManager::getHooksManager().createCalloutHandle();
+ CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle();
// Call allocateAddress4. Callouts should be triggered here.
Lease4Ptr lease = engine->allocateAddress4(subnet_, clientid_, hwaddr_, IOAddress("0.0.0.0"),
More information about the bind10-changes
mailing list