[2898] First set of changes after review

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<int>(len)
             << " with params t1=" << t1 << ", t2=" << t2 << ", pref="
             << pref << ", valid=" << valid;
 

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());
+        subnet = CfgMgr::instance().getSubnet6(question->getIface());
-        if (subnet) {
+        if (!subnet) {
-            return (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) {
+        if (!subnet) {
-            return (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
+            // if relay filled in link_addr field, then let's use it
-        // try address matching
+            if (link_addr != IOAddress("::")) {
-        IOAddress link_addr = question->relay_info_.back().linkaddr_;
+                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

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -277,13 +277,13 @@ public:
                             expected_data_len));
     }
 
-    int rcode_;
+    int rcode_; ///< return core (see @ref isc::config::parseAnswer)
-    Dhcpv6Srv srv_;
+    Dhcpv6Srv srv_; ///< instance of the Dhcp6Srv used during tests
 
-    ConstElementPtr comment_;
+    ConstElementPtr comment_; ///< comment (see @ref isc::config::parseAnswer)
 
-    string valid_iface_;
+    string valid_iface_; ///< name of a valid network interface (present in system)
-    string bogus_iface_;
+    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;
+    const string config = "{ \"interface\": [ \"all\" ],"
-
-    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;
+    const string config = "{ \"preferred-lifetime\": 3000,"
-
-    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_);

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<Option6IAAddr>());
         }
- 
+
         boost::shared_ptr<Option6IA> ia = boost::dynamic_pointer_cast<Option6IA>(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<Option6IAAddr> addr = boost::dynamic_pointer_cast<Option6IAAddr>(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 
+// This test sends a SOLICIT and the expected response
-// is an ADVERTISE with STATUS_NoAddrsAvail and no address provided in the 
+// 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 
+// This test sends a REQUEST and the expected response
-// is an REPLY with STATUS_NoAddrsAvail and no address provided in the 
+// 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 
+// This test sends a RENEW and the expected response
-// is an REPLY with STATUS_NoBinding and no address provided in the 
+// 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 
+// This test sends a RELEASE and the expected response
-// is an REPLY with STATUS_NoBinding and no address provided in the 
+// 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<Option6IAAddr> 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<Option6IAAddr> 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);

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.