BIND 10 trac2983, updated. 3bcbbb7f26e1a36ad152912d8bea1a1013c622c9 [2983] Unit-tests for buffer4_receive implemented.
BIND 10 source code commits
bind10-changes at lists.isc.org
Tue Aug 20 12:02:58 UTC 2013
The branch, trac2983 has been updated
via 3bcbbb7f26e1a36ad152912d8bea1a1013c622c9 (commit)
from 4cd6015489d77e842735a1ecd431ea68af1820bd (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 3bcbbb7f26e1a36ad152912d8bea1a1013c622c9
Author: Tomek Mrugalski <tomasz at isc.org>
Date: Tue Aug 20 14:02:27 2013 +0200
[2983] Unit-tests for buffer4_receive implemented.
-----------------------------------------------------------------------
Summary of changes:
src/bin/dhcp4/dhcp4_srv.cc | 15 ++-
src/bin/dhcp4/tests/dhcp4_srv_unittest.cc | 201 ++++++++++++++++++++++++++---
src/lib/dhcp/pkt4.cc | 12 +-
src/lib/dhcp/pkt4.h | 54 ++++----
4 files changed, 235 insertions(+), 47 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index 515f5f0..e238253 100644
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -198,7 +198,7 @@ Dhcpv4Srv::run() {
bool skip_unpack = false;
// The packet has just been received so contains the uninterpreted wire
- // data; execute callouts registered for buffer6_receive.
+ // data; execute callouts registered for buffer4_receive.
if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_buffer4_receive_)) {
CalloutHandlePtr callout_handle = getCalloutHandle(query);
@@ -236,12 +236,21 @@ Dhcpv4Srv::run() {
}
}
+ // When receiving a packet without message type option, getType() will
+ // throw. Let's set type to -1 as default error indicator.
+ int type = -1;
+ try {
+ type = query->getType();
+ } catch (const std::exception& e) {
+
+ }
+
LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PACKET_RECEIVED)
.arg(serverReceivedPacketName(query->getType()))
- .arg(query->getType())
+ .arg(type)
.arg(query->getIface());
LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_QUERY_DATA)
- .arg(static_cast<int>(query->getType()))
+ .arg(type)
.arg(query->toText());
// Let's execute all callouts registered for packet_received
diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
index 0251991..2178b51 100644
--- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
+++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
@@ -1631,21 +1631,33 @@ TEST_F(Dhcpv4SrvTest, Hooks) {
NakedDhcpv4Srv srv(0);
// check if appropriate hooks are registered
- int hook_index_pkt4_received = -1;
- int hook_index_select_subnet = -1;
- int hook_index_pkt4_send = -1;
+ int hook_index_buffer4_receive = -1;
+ int hook_index_pkt4_receive = -1;
+ int hook_index_select_subnet = -1;
+ int hook_index_lease4_release = -1;
+ int hook_index_pkt4_send = -1;
+ int hook_index_buffer4_send = -1;
// check if appropriate indexes are set
- EXPECT_NO_THROW(hook_index_pkt4_received = ServerHooks::getServerHooks()
+ EXPECT_NO_THROW(hook_index_buffer4_receive = ServerHooks::getServerHooks()
+ .getIndex("buffer4_receive"));
+ EXPECT_NO_THROW(hook_index_pkt4_receive = ServerHooks::getServerHooks()
.getIndex("pkt4_receive"));
EXPECT_NO_THROW(hook_index_select_subnet = ServerHooks::getServerHooks()
.getIndex("subnet4_select"));
- EXPECT_NO_THROW(hook_index_pkt4_send = ServerHooks::getServerHooks()
+ EXPECT_NO_THROW(hook_index_lease4_release = ServerHooks::getServerHooks()
+ .getIndex("lease4_release"));
+ EXPECT_NO_THROW(hook_index_pkt4_send = ServerHooks::getServerHooks()
.getIndex("pkt4_send"));
+ EXPECT_NO_THROW(hook_index_buffer4_send = ServerHooks::getServerHooks()
+ .getIndex("buffer4_send"));
- EXPECT_TRUE(hook_index_pkt4_received > 0);
+ EXPECT_TRUE(hook_index_buffer4_receive > 0);
+ EXPECT_TRUE(hook_index_pkt4_receive > 0);
EXPECT_TRUE(hook_index_select_subnet > 0);
+ EXPECT_TRUE(hook_index_lease4_release > 0);
EXPECT_TRUE(hook_index_pkt4_send > 0);
+ EXPECT_TRUE(hook_index_buffer4_send > 0);
}
// a dummy MAC address
@@ -1766,6 +1778,67 @@ public:
return (Pkt4Ptr(new Pkt4(&buf[0], buf.size())));
}
+ /// Test callback that stores received callout name and pkt4 value
+ /// @param callout_handle handle passed by the hooks framework
+ /// @return always 0
+ static int
+ buffer4_receive_callout(CalloutHandle& callout_handle) {
+ callback_name_ = string("buffer4_receive");
+
+ callout_handle.getArgument("query4", callback_pkt4_);
+
+ callback_argument_names_ = callout_handle.getArgumentNames();
+ return (0);
+ }
+
+ /// Test callback that changes hwaddr value
+ /// @param callout_handle handle passed by the hooks framework
+ /// @return always 0
+ static int
+ buffer4_receive_change_hwaddr(CalloutHandle& callout_handle) {
+
+ Pkt4Ptr pkt;
+ callout_handle.getArgument("query4", pkt);
+
+ // If there is at least one option with data
+ if (pkt->data_.size() >= Pkt4::DHCPV4_PKT_HDR_LEN) {
+ // Offset of the first byte of the CHWADDR field. Let's the first
+ // byte to some new value that we could later check
+ pkt->data_[28] = 0xff;
+ }
+
+ // Carry on as usual
+ return buffer4_receive_callout(callout_handle);
+ }
+
+ /// Test callback that deletes MAC address
+ /// @param callout_handle handle passed by the hooks framework
+ /// @return always 0
+ static int
+ buffer4_receive_delete_hwaddr(CalloutHandle& callout_handle) {
+
+ Pkt4Ptr pkt;
+ callout_handle.getArgument("query4", pkt);
+
+ pkt->data_[2] = 0; // offset 2 is hlen, let's set it to zero
+ memset(&pkt->data_[28], 0, Pkt4::MAX_CHADDR_LEN); // Clear CHADDR content
+
+ // carry on as usual
+ return buffer4_receive_callout(callout_handle);
+ }
+
+ /// Test callback that sets skip flag
+ /// @param callout_handle handle passed by the hooks framework
+ /// @return always 0
+ static int
+ buffer4_receive_skip(CalloutHandle& callout_handle) {
+
+ callout_handle.setSkip(true);
+
+ // Carry on as usual
+ return buffer4_receive_callout(callout_handle);
+ }
+
/// test callback that stores received callout name and pkt4 value
/// @param callout_handle handle passed by the hooks framework
/// @return always 0
@@ -1970,8 +2043,106 @@ Subnet4Ptr HooksDhcpv4SrvTest::callback_subnet4_;
const Subnet4Collection* HooksDhcpv4SrvTest::callback_subnet4collection_;
vector<string> HooksDhcpv4SrvTest::callback_argument_names_;
+// Checks if callouts installed on pkt4_receive are indeed called and the
+// all necessary parameters are passed.
+//
+// Note that the test name does not follow test naming convention,
+// but the proper hook name is "buffer4_receive".
+TEST_F(HooksDhcpv4SrvTest, simple_buffer4_receive) {
+
+ // Install pkt6_receive_callout
+ EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+ "buffer4_receive", buffer4_receive_callout));
+
+ // Let's create a simple DISCOVER
+ Pkt4Ptr dis = generateSimpleDiscover();
+
+ // Simulate that we have received that traffic
+ srv_->fakeReceive(dis);
+
+ // Server will now process to run its normal loop, but instead of calling
+ // IfaceMgr::receive4(), it will read all packets from the list set by
+ // fakeReceive()
+ // In particular, it should call registered buffer4_receive callback.
+ srv_->run();
+
+ // Check that the callback called is indeed the one we installed
+ EXPECT_EQ("buffer4_receive", callback_name_);
+
+ // Check that pkt4 argument passing was successful and returned proper value
+ EXPECT_TRUE(callback_pkt4_.get() == dis.get());
+
+ // Check that all expected parameters are there
+ vector<string> expected_argument_names;
+ expected_argument_names.push_back(string("query4"));
+
+ EXPECT_TRUE(expected_argument_names == callback_argument_names_);
+}
+
+// Checks if callouts installed on buffer4_receive is able to change
+// the values and the parameters are indeed used by the server.
+TEST_F(HooksDhcpv4SrvTest, valueChange_buffer4_receive) {
+
+ // Install callback that modifies MAC addr of incoming packet
+ EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+ "buffer4_receive", buffer4_receive_change_hwaddr));
+
+ // Let's create a simple DISCOVER
+ Pkt4Ptr discover = generateSimpleDiscover();
+
+ // Simulate that we have received that traffic
+ srv_->fakeReceive(discover);
+
+ // Server will now process to run its normal loop, but instead of calling
+ // IfaceMgr::receive6(), it will read all packets from the list set by
+ // fakeReceive()
+ // In particular, it should call registered buffer4_receive callback.
+ srv_->run();
+
+ // Check that the server did send a reposonce
+ ASSERT_EQ(1, srv_->fake_sent_.size());
+
+ // Make sure that we received a response
+ Pkt4Ptr offer = srv_->fake_sent_.front();
+ ASSERT_TRUE(offer);
+
+ // Get client-id...
+ HWAddrPtr hwaddr = offer->getHWAddr();
+
+ ASSERT_TRUE(hwaddr); // basic sanity check. HWAddr is always present
+
+ // ... and check if it is the modified value
+ ASSERT_FALSE(hwaddr->hwaddr_.empty()); // there must be a MAC address
+ EXPECT_EQ(0xff, hwaddr->hwaddr_[0]); // check that its first byte was modified
+}
+
+// Checks if callouts installed on buffer4_receive is able to set skip flag that
+// will cause the server to not parse the packet. Even though the packet is valid,
+// the server should eventually drop it, because there won't be mandatory options
+// (or rather option objects) in it.
+TEST_F(HooksDhcpv4SrvTest, skip_buffer4_receive) {
+
+ // Install pkt6_receive_callout
+ EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle().registerCallout(
+ "buffer4_receive", buffer4_receive_skip));
+
+ // Let's create a simple DISCOVER
+ Pkt4Ptr discover = generateSimpleDiscover();
+
+ // Simulate that we have received that traffic
+ srv_->fakeReceive(discover);
+
+ // Server will now process to run its normal loop, but instead of calling
+ // IfaceMgr::receive6(), it will read all packets from the list set by
+ // fakeReceive()
+ // In particular, it should call registered pkt6_receive callback.
+ srv_->run();
+
+ // Check that the server dropped the packet and did not produce any response
+ ASSERT_EQ(0, srv_->fake_sent_.size());
+}
-// Checks if callouts installed on pkt4_received are indeed called and the
+// Checks if callouts installed on pkt4_receive are indeed called and the
// all necessary parameters are passed.
//
// Note that the test name does not follow test naming convention,
@@ -1983,7 +2154,7 @@ TEST_F(HooksDhcpv4SrvTest, simple_pkt4_receive) {
"pkt4_receive", pkt4_receive_callout));
// Let's create a simple DISCOVER
- Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+ Pkt4Ptr sol = generateSimpleDiscover();
// Simulate that we have received that traffic
srv_->fakeReceive(sol);
@@ -2016,7 +2187,7 @@ TEST_F(HooksDhcpv4SrvTest, valueChange_pkt4_receive) {
"pkt4_receive", pkt4_receive_change_clientid));
// Let's create a simple DISCOVER
- Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+ Pkt4Ptr sol = generateSimpleDiscover();
// Simulate that we have received that traffic
srv_->fakeReceive(sol);
@@ -2052,7 +2223,7 @@ TEST_F(HooksDhcpv4SrvTest, deleteClientId_pkt4_receive) {
"pkt4_receive", pkt4_receive_delete_clientid));
// Let's create a simple DISCOVER
- Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+ Pkt4Ptr sol = generateSimpleDiscover();
// Simulate that we have received that traffic
srv_->fakeReceive(sol);
@@ -2076,7 +2247,7 @@ TEST_F(HooksDhcpv4SrvTest, skip_pkt4_receive) {
"pkt4_receive", pkt4_receive_skip));
// Let's create a simple DISCOVER
- Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+ Pkt4Ptr sol = generateSimpleDiscover();
// Simulate that we have received that traffic
srv_->fakeReceive(sol);
@@ -2101,7 +2272,7 @@ TEST_F(HooksDhcpv4SrvTest, simple_pkt4_send) {
"pkt4_send", pkt4_send_callout));
// Let's create a simple DISCOVER
- Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+ Pkt4Ptr sol = generateSimpleDiscover();
// Simulate that we have received that traffic
srv_->fakeReceive(sol);
@@ -2137,7 +2308,7 @@ TEST_F(HooksDhcpv4SrvTest, valueChange_pkt4_send) {
"pkt4_send", pkt4_send_change_serverid));
// Let's create a simple DISCOVER
- Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+ Pkt4Ptr sol = generateSimpleDiscover();
// Simulate that we have received that traffic
srv_->fakeReceive(sol);
@@ -2174,7 +2345,7 @@ TEST_F(HooksDhcpv4SrvTest, deleteServerId_pkt4_send) {
"pkt4_send", pkt4_send_delete_serverid));
// Let's create a simple DISCOVER
- Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+ Pkt4Ptr sol = generateSimpleDiscover();
// Simulate that we have received that traffic
srv_->fakeReceive(sol);
@@ -2205,7 +2376,7 @@ TEST_F(HooksDhcpv4SrvTest, skip_pkt4_send) {
"pkt4_send", pkt4_send_skip));
// Let's create a simple REQUEST
- Pkt4Ptr sol = Pkt4Ptr(generateSimpleDiscover());
+ Pkt4Ptr sol = generateSimpleDiscover();
// Simulate that we have received that traffic
srv_->fakeReceive(sol);
diff --git a/src/lib/dhcp/pkt4.cc b/src/lib/dhcp/pkt4.cc
index 07ff7c7..7433e8d 100644
--- a/src/lib/dhcp/pkt4.cc
+++ b/src/lib/dhcp/pkt4.cc
@@ -33,7 +33,8 @@ namespace dhcp {
const IOAddress DEFAULT_ADDRESS("0.0.0.0");
Pkt4::Pkt4(uint8_t msg_type, uint32_t transid)
- :local_addr_(DEFAULT_ADDRESS),
+ :bufferOut_(DHCPV4_PKT_HDR_LEN),
+ local_addr_(DEFAULT_ADDRESS),
remote_addr_(DEFAULT_ADDRESS),
iface_(""),
ifindex_(0),
@@ -48,8 +49,7 @@ Pkt4::Pkt4(uint8_t msg_type, uint32_t transid)
ciaddr_(DEFAULT_ADDRESS),
yiaddr_(DEFAULT_ADDRESS),
siaddr_(DEFAULT_ADDRESS),
- giaddr_(DEFAULT_ADDRESS),
- bufferOut_(DHCPV4_PKT_HDR_LEN)
+ giaddr_(DEFAULT_ADDRESS)
{
memset(sname_, 0, MAX_SNAME_LEN);
memset(file_, 0, MAX_FILE_LEN);
@@ -58,7 +58,8 @@ Pkt4::Pkt4(uint8_t msg_type, uint32_t transid)
}
Pkt4::Pkt4(const uint8_t* data, size_t len)
- :local_addr_(DEFAULT_ADDRESS),
+ :bufferOut_(0), // not used, this is RX packet
+ local_addr_(DEFAULT_ADDRESS),
remote_addr_(DEFAULT_ADDRESS),
iface_(""),
ifindex_(0),
@@ -72,8 +73,7 @@ Pkt4::Pkt4(const uint8_t* data, size_t len)
ciaddr_(DEFAULT_ADDRESS),
yiaddr_(DEFAULT_ADDRESS),
siaddr_(DEFAULT_ADDRESS),
- giaddr_(DEFAULT_ADDRESS),
- bufferOut_(0) // not used, this is RX packet
+ giaddr_(DEFAULT_ADDRESS)
{
if (len < DHCPV4_PKT_HDR_LEN) {
isc_throw(OutOfRange, "Truncated DHCPv4 packet (len=" << len
diff --git a/src/lib/dhcp/pkt4.h b/src/lib/dhcp/pkt4.h
index 3b945f1..c3ca6d5 100644
--- a/src/lib/dhcp/pkt4.h
+++ b/src/lib/dhcp/pkt4.h
@@ -489,6 +489,37 @@ public:
/// @throw isc::Unexpected if timestamp update failed
void updateTimestamp();
+ /// output buffer (used during message transmission)
+ ///
+ /// @warning This public member is accessed by derived
+ /// classes directly. One of such derived classes is
+ /// @ref perfdhcp::PerfPkt4. The impact on derived clasess'
+ /// behavior must be taken into consideration before making
+ /// changes to this member such as access scope restriction or
+ /// data format change etc. This field is also public, because
+ /// it may be modified by callouts (which are written in C++ now,
+ /// but we expect to also have them in Python, so any accesibility
+ /// methods would overly complicate things here and degrade
+ /// performance).
+ isc::util::OutputBuffer bufferOut_;
+
+ /// that's the data of input buffer used in RX packet. Note that
+ /// InputBuffer does not store the data itself, but just expects that
+ /// data will be valid for the whole life of InputBuffer. Therefore we
+ /// need to keep the data around.
+ ///
+ /// @warning This public member is accessed by derived
+ /// classes directly. One of such derived classes is
+ /// @ref perfdhcp::PerfPkt4. The impact on derived clasess'
+ /// behavior must be taken into consideration before making
+ /// changes to this member such as access scope restriction or
+ /// data format change etc. This field is also public, because
+ /// it may be modified by callouts (which are written in C++ now,
+ /// but we expect to also have them in Python, so any accesibility
+ /// methods would overly complicate things here and degrade
+ /// performance).
+ std::vector<uint8_t> data_;
+
private:
/// @brief Generic method that validates and sets HW address.
@@ -591,29 +622,6 @@ protected:
// end of real DHCPv4 fields
- /// output buffer (used during message transmission)
- ///
- /// @warning This protected member is accessed by derived
- /// classes directly. One of such derived classes is
- /// @ref perfdhcp::PerfPkt4. The impact on derived clasess'
- /// behavior must be taken into consideration before making
- /// changes to this member such as access scope restriction or
- /// data format change etc.
- isc::util::OutputBuffer bufferOut_;
-
- /// that's the data of input buffer used in RX packet. Note that
- /// InputBuffer does not store the data itself, but just expects that
- /// data will be valid for the whole life of InputBuffer. Therefore we
- /// need to keep the data around.
- ///
- /// @warning This protected member is accessed by derived
- /// classes directly. One of such derived classes is
- /// @ref perfdhcp::PerfPkt4. The impact on derived clasess'
- /// behavior must be taken into consideration before making
- /// changes to this member such as access scope restriction or
- /// data format change etc.
- std::vector<uint8_t> data_;
-
/// collection of options present in this message
///
/// @warning This protected member is accessed by derived
More information about the bind10-changes
mailing list