From a19cdcff7accbce09eab6bf7ea359a830564c1f8 Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Tue, 7 May 2013 19:46:57 +0200 Subject: [PATCH] [2898] First set of changes after review --- src/bin/dhcp6/config_parser.cc | 4 +- src/bin/dhcp6/dhcp6_srv.cc | 34 +++++++-------- src/bin/dhcp6/tests/config_parser_unittest.cc | 38 +++++++---------- src/bin/dhcp6/tests/dhcp6_srv_unittest.cc | 42 +++++++++++-------- src/lib/dhcpsrv/cfgmgr.cc | 2 +- 5 files changed, 59 insertions(+), 61 deletions(-) diff --git a/src/bin/dhcp6/config_parser.cc b/src/bin/dhcp6/config_parser.cc index 243d217605..f386243940 100644 --- a/src/bin/dhcp6/config_parser.cc +++ b/src/bin/dhcp6/config_parser.cc @@ -1490,7 +1490,7 @@ private: std::string ifaceid; try { ifaceid = string_values_.getParam("interface-id"); - } catch (DhcpConfigError) { + } catch (const DhcpConfigError&) { // interface-id is not mandatory } @@ -1503,7 +1503,7 @@ private: } stringstream tmp; - tmp << addr.toText() << "/" << (int)len + tmp << addr.toText() << "/" << static_cast(len) << " with params t1=" << t1 << ", t2=" << t2 << ", pref=" << pref << ", valid=" << valid; diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index ec74bb1810..0252c5199f 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -405,7 +405,7 @@ Dhcpv6Srv::copyDefaultOptions(const Pkt6Ptr& question, Pkt6Ptr& answer) { } /// @todo: Should throw if there is no client-id (except anonymous INF-REQUEST) - // if this is a relayed message, we need to copy relay information + // If this is a relayed message, we need to copy relay information if (!question->relay_info_.empty()) { answer->copyRelayInfo(question); } @@ -534,14 +534,11 @@ Dhcpv6Srv::selectSubnet(const Pkt6Ptr& question) { // This is a direct (non-relayed) message // Try to find a subnet if received packet from a directly connected client - Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(question->getIface()); - if (subnet) { - return (subnet); + subnet = CfgMgr::instance().getSubnet6(question->getIface()); + if (!subnet) { + // If no subnet was found, try to find it based on remote address + subnet = CfgMgr::instance().getSubnet6(question->getRemoteAddr()); } - - // If no subnet was found, try to find it based on remote address - subnet = CfgMgr::instance().getSubnet6(question->getRemoteAddr()); - return (subnet); } else { // This is a relayed message @@ -551,20 +548,19 @@ Dhcpv6Srv::selectSubnet(const Pkt6Ptr& question) { subnet = CfgMgr::instance().getSubnet6(interface_id); } - if (subnet) { - return (subnet); - } + if (!subnet) { + // If no interface-id was specified (or not configured on server), let's + // try address matching + IOAddress link_addr = question->relay_info_.back().linkaddr_; - // If no interface-id was specified (or not configured on server), let's - // try address matching - IOAddress link_addr = question->relay_info_.back().linkaddr_; - - // if relay filled in link_addr field, then let's use it - if (link_addr != IOAddress("::")) { - subnet = CfgMgr::instance().getSubnet6(link_addr); + // if relay filled in link_addr field, then let's use it + if (link_addr != IOAddress("::")) { + subnet = CfgMgr::instance().getSubnet6(link_addr); + } } - return (subnet); } + + return (subnet); } void diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index 37dd783cc9..be3e3be3ca 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -277,13 +277,13 @@ public: expected_data_len)); } - int rcode_; - Dhcpv6Srv srv_; + int rcode_; ///< return core (see @ref isc::config::parseAnswer) + Dhcpv6Srv srv_; ///< instance of the Dhcp6Srv used during tests - ConstElementPtr comment_; + ConstElementPtr comment_; ///< comment (see @ref isc::config::parseAnswer) - string valid_iface_; - string bogus_iface_; + string valid_iface_; ///< name of a valid network interface (present in system) + string bogus_iface_; ///< name of a invalid network interface (not present in system) }; // Goal of this test is a verification if a very simple config update @@ -508,11 +508,9 @@ TEST_F(Dhcp6ParserTest, subnetInterfaceId) { const string valid_interface_id = "foobar"; const string bogus_interface_id = "blah"; - ConstElementPtr status; - // There should be at least one interface - string config = "{ " + const string config = "{ " "\"preferred-lifetime\": 3000," "\"rebind-timer\": 2000, " "\"renew-timer\": 1000, " @@ -521,24 +519,24 @@ TEST_F(Dhcp6ParserTest, subnetInterfaceId) { " \"interface-id\": \"" + valid_interface_id + "\"," " \"subnet\": \"2001:db8:1::/64\" } ]," "\"valid-lifetime\": 4000 }"; - cout << config << endl; ElementPtr json = Element::fromJSON(config); + ConstElementPtr status; EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json)); - // returned value should be 0 (configuration success) + // Returned value should be 0 (configuration success) ASSERT_TRUE(status); comment_ = parseAnswer(rcode_, status); EXPECT_EQ(0, rcode_); - // try to get a subnet based on bogus interface-id option + // Try to get a subnet based on bogus interface-id option OptionBuffer tmp(bogus_interface_id.begin(), bogus_interface_id.end()); OptionPtr ifaceid(new Option(Option::V6, D6O_INTERFACE_ID, tmp)); Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(ifaceid); EXPECT_FALSE(subnet); - // now try to get subnet for valid interface-id value + // Now try to get subnet for valid interface-id value tmp = OptionBuffer(valid_interface_id.begin(), valid_interface_id.end()); ifaceid.reset(new Option(Option::V6, D6O_INTERFACE_ID, tmp)); subnet = CfgMgr::instance().getSubnet6(ifaceid); @@ -551,9 +549,7 @@ TEST_F(Dhcp6ParserTest, subnetInterfaceId) { // parameter. TEST_F(Dhcp6ParserTest, interfaceIdGlobal) { - ConstElementPtr status; - - string config = "{ \"interface\": [ \"all\" ]," + const string config = "{ \"interface\": [ \"all\" ]," "\"preferred-lifetime\": 3000," "\"rebind-timer\": 2000, " "\"renew-timer\": 1000, " @@ -562,13 +558,13 @@ TEST_F(Dhcp6ParserTest, interfaceIdGlobal) { " \"pool\": [ \"2001:db8:1::1 - 2001:db8:1::ffff\" ]," " \"subnet\": \"2001:db8:1::/64\" } ]," "\"valid-lifetime\": 4000 }"; - cout << config << endl; ElementPtr json = Element::fromJSON(config); + ConstElementPtr status; EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json)); - // returned value should be 1 (parse error) + // Returned value should be 1 (parse error) ASSERT_TRUE(status); comment_ = parseAnswer(rcode_, status); EXPECT_EQ(1, rcode_); @@ -578,9 +574,7 @@ TEST_F(Dhcp6ParserTest, interfaceIdGlobal) { // interface (i.e. local subnet) and interface-id (remote subnet) defined. TEST_F(Dhcp6ParserTest, subnetInterfaceAndInterfaceId) { - ConstElementPtr status; - - string config = "{ \"preferred-lifetime\": 3000," + const string config = "{ \"preferred-lifetime\": 3000," "\"rebind-timer\": 2000, " "\"renew-timer\": 1000, " "\"subnet6\": [ { " @@ -589,13 +583,13 @@ TEST_F(Dhcp6ParserTest, subnetInterfaceAndInterfaceId) { " \"interface-id\": \"foobar\"," " \"subnet\": \"2001:db8:1::/64\" } ]," "\"valid-lifetime\": 4000 }"; - cout << config << endl; ElementPtr json = Element::fromJSON(config); + ConstElementPtr status; EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json)); - // returned value should be 0 (configuration success) + // Returned value should be 0 (configuration success) ASSERT_TRUE(status); comment_ = parseAnswer(rcode_, status); EXPECT_EQ(1, rcode_); diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index d63d518b39..53b1daf47d 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc @@ -80,7 +80,7 @@ public: static const char* DUID_FILE = "server-id-test.txt"; // test fixture for any tests requiring blank/empty configuration -// serves as base class for additional tests +// serves as base class for additional tests class NakedDhcpv6SrvTest : public ::testing::Test { public: @@ -146,12 +146,12 @@ public: // Checks if server response is a NAK void checkNakResponse(const Pkt6Ptr& rsp, uint8_t expected_message_type, - uint32_t expected_transid, + uint32_t expected_transid, uint16_t expected_status_code) { // Check if we get response at all checkResponse(rsp, expected_message_type, expected_transid); - // Check that IA_NA was returned + // Check that IA_NA was returned OptionPtr option_ia_na = rsp->getOption(D6O_IA_NA); ASSERT_TRUE(option_ia_na); @@ -237,7 +237,7 @@ public: ConstElementPtr comment_; }; -// Provides suport for tests against a preconfigured subnet6 +// Provides suport for tests against a preconfigured subnet6 // extends upon NakedDhcp6SrvTest class Dhcpv6SrvTest : public NakedDhcpv6SrvTest { public: @@ -264,7 +264,7 @@ public: ADD_FAILURE() << "IA_NA option not present in response"; return (boost::shared_ptr()); } - + boost::shared_ptr ia = boost::dynamic_pointer_cast(tmp); if (!ia) { ADD_FAILURE() << "IA_NA cannot convert option ptr to Option6"; @@ -274,7 +274,7 @@ public: EXPECT_EQ(expected_iaid, ia->getIAID()); EXPECT_EQ(expected_t1, ia->getT1()); EXPECT_EQ(expected_t2, ia->getT2()); - + tmp = ia->getOption(D6O_IAADDR); boost::shared_ptr addr = boost::dynamic_pointer_cast(tmp); return (addr); @@ -330,10 +330,10 @@ public: }; // This test verifies that incoming SOLICIT can be handled properly when -// there are no subnets configured. +// there are no subnets configured. // -// This test sends a SOLICIT and the expected response -// is an ADVERTISE with STATUS_NoAddrsAvail and no address provided in the +// This test sends a SOLICIT and the expected response +// is an ADVERTISE with STATUS_NoAddrsAvail and no address provided in the // response TEST_F(NakedDhcpv6SrvTest, SolicitNoSubnet) { NakedDhcpv6Srv srv(0); @@ -352,10 +352,10 @@ TEST_F(NakedDhcpv6SrvTest, SolicitNoSubnet) { } // This test verifies that incoming REQUEST can be handled properly when -// there are no subnets configured. +// there are no subnets configured. // -// This test sends a REQUEST and the expected response -// is an REPLY with STATUS_NoAddrsAvail and no address provided in the +// This test sends a REQUEST and the expected response +// is an REPLY with STATUS_NoAddrsAvail and no address provided in the // response TEST_F(NakedDhcpv6SrvTest, RequestNoSubnet) { NakedDhcpv6Srv srv(0); @@ -386,8 +386,8 @@ TEST_F(NakedDhcpv6SrvTest, RequestNoSubnet) { // This test verifies that incoming RENEW can be handled properly, even when // no subnets are configured. // -// This test sends a RENEW and the expected response -// is an REPLY with STATUS_NoBinding and no address provided in the +// This test sends a RENEW and the expected response +// is an REPLY with STATUS_NoBinding and no address provided in the // response TEST_F(NakedDhcpv6SrvTest, RenewNoSubnet) { NakedDhcpv6Srv srv(0); @@ -421,8 +421,8 @@ TEST_F(NakedDhcpv6SrvTest, RenewNoSubnet) { // This test verifies that incoming RELEASE can be handled properly, even when // no subnets are configured. // -// This test sends a RELEASE and the expected response -// is an REPLY with STATUS_NoBinding and no address provided in the +// This test sends a RELEASE and the expected response +// is an REPLY with STATUS_NoBinding and no address provided in the // response TEST_F(NakedDhcpv6SrvTest, ReleaseNoSubnet) { NakedDhcpv6Srv srv(0); @@ -951,6 +951,8 @@ TEST_F(Dhcpv6SrvTest, RequestBasic) { TEST_F(Dhcpv6SrvTest, ManyRequests) { NakedDhcpv6Srv srv(0); + ASSERT_TRUE(subnet_); + Pkt6Ptr req1 = Pkt6Ptr(new Pkt6(DHCPV6_REQUEST, 1234)); Pkt6Ptr req2 = Pkt6Ptr(new Pkt6(DHCPV6_REQUEST, 2345)); Pkt6Ptr req3 = Pkt6Ptr(new Pkt6(DHCPV6_REQUEST, 3456)); @@ -995,6 +997,10 @@ TEST_F(Dhcpv6SrvTest, ManyRequests) { boost::shared_ptr addr3 = checkIA_NA(reply3, 3, subnet_->getT1(), subnet_->getT2()); + ASSERT_TRUE(addr1); + ASSERT_TRUE(addr2); + ASSERT_TRUE(addr3); + // Check that the assigned address is indeed from the configured pool checkIAAddr(addr1, addr1->getAddress(), subnet_->getPreferred(), subnet_->getValid()); checkIAAddr(addr2, addr2->getAddress(), subnet_->getPreferred(), subnet_->getValid()); @@ -1083,6 +1089,8 @@ TEST_F(Dhcpv6SrvTest, RenewBasic) { boost::shared_ptr addr_opt = checkIA_NA(reply, 234, subnet_->getT1(), subnet_->getT2()); + ASSERT_TRUE(addr_opt); + // Check that we've got the address we requested checkIAAddr(addr_opt, addr, subnet_->getPreferred(), subnet_->getValid()); @@ -1649,7 +1657,7 @@ TEST_F(Dhcpv6SrvTest, selectSubnetRelayLinkaddr) { CfgMgr::instance().addSubnet6(subnet2); CfgMgr::instance().addSubnet6(subnet3); - // source of the packet should have no meaning. Selection is based + // Source of the packet should have no meaning. Selection is based // on linkaddr field in the relay pkt->setRemoteAddr(IOAddress("2001:db8:1::baca")); selected = srv.selectSubnet(pkt); diff --git a/src/lib/dhcpsrv/cfgmgr.cc b/src/lib/dhcpsrv/cfgmgr.cc index 8c0742c136..745322ecd2 100644 --- a/src/lib/dhcpsrv/cfgmgr.cc +++ b/src/lib/dhcpsrv/cfgmgr.cc @@ -147,7 +147,7 @@ CfgMgr::getSubnet6(const isc::asiolink::IOAddress& hint) { // If there's only one subnet configured, let's just use it // The idea is to keep small deployments easy. In a small network - one - // router that also runs DHCPv6 server. Users specifies a single pool and + // router that also runs DHCPv6 server. User specifies a single pool and // expects it to just work. Without this, the server would complain that it // doesn't have IP address on its interfaces that matches that // configuration. Such requirement makes sense in IPv4, but not in IPv6.