[3274] Changes after review:

 - Copyright years updated
 - Minor corrections in BINDX Guide
 - dhcp4.spec relay structure description updated.
 - new unit-test for DHCPv4 client classification added
 - new unit-test for DHCPv6 client classification added
 - fixed Doxygen errors
 - comments for CfgMgr::getSubnet{4,6} updated

doc/guide/bind10-guide.xml

@@ -3525,7 +3525,7 @@ then change those defaults with config set Resolver/forward_addresses[0]/address
     protocols. BIND 10 offers two server implementations, one for DHCPv4
     and one for DHCPv6. The DHCP part of the BIND 10 project is codenamed
     Kea. The DHCPv4 component is colloquially referred to as Kea4 and its
-    DHCPv6 is called Kea6.</para>
+    DHCPv6 counterpart is called Kea6.</para>
     <para>This chapter covers those parts of BIND 10 that are common to
     both servers.  DHCPv4-specific details are covered in <xref linkend="dhcp4"/>,
     while those details specific to DHCPv6 are described in <xref linkend="dhcp6"/>
@@ -4416,7 +4416,7 @@ Dhcp4/subnet4	[]	list	(default)
       </para>
 
       <para>It is envisaged that the client classification will be used for changing
-      behavior of almost any part of the DHCP engine processing, including assigning
+      behavior of almost any part of the DHCP message processing, including assigning
       leases from different pools, assigning different option (or different values of
       the same options) etc. For now, there are only two mechanisms that are taking
       advantage of client classification: specific processing for cable modems and
@@ -4435,7 +4435,7 @@ Dhcp4/subnet4	[]	list	(default)
         Kea can be instructed to limit access to given subnets based on class information.
         This is particularly useful for cases where two types of devices share the
         same link and are expected to be served from two different subnets. The
-        primary use case for such a scenario are cable networks. There are two
+        primary use case for such a scenario is cable networks. There are two
         classes of devices: cable modem itself, which should be handled a lease
         from subnet A and all other devices behind modems that should get a lease
         from subnet B. That segregation is essential to prevent overly curious
@@ -4448,7 +4448,7 @@ Dhcp4/subnet4	[]	list	(default)
     <section id="dhcp4-subnet-class">
       <title>Limiting access to IPv4 subnet to certain classes</title>
       <para>
-        In certain cases it beneficial to restrict access to certains subnets
+        In certain cases it beneficial to restrict access to certain subnets
         only to clients that belong to a given subnet. For details on client
         classes, see <xref linkend="dhcp4-client-classifier"/>. This is an
         extension of a previous example from <xref linkend="dhcp4-address-config"/>.

src/bin/dhcp4/config_parser.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above

src/bin/dhcp4/dhcp4.dox

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above

src/bin/dhcp4/dhcp4.spec

@@ -261,14 +261,14 @@
                   "item_type": "map",
                   "item_optional": false,
                   "item_default": {},
-                  "item_description" : "Structure holding optional relay information.",
+                  "item_description" : "Structure holding relay information.",
                   "map_item_spec": [
                       {
                           "item_name": "ip-address",
                           "item_type": "string",
                           "item_optional": false,
                           "item_default": "0.0.0.0",
-                          "item_description" : "IPv4 address of the relay (optional)."
+                          "item_description" : "IPv4 address of the relay (defaults to 0.0.0.0 if not specified)."
                       }
                    ]
                 },

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -3201,4 +3201,68 @@ TEST_F(Dhcpv4SrvTest, clientClassification) {
     EXPECT_FALSE(dis2->inClass("docsis3.0"));
 }
 
+// Checks if the client-class field is indeed used for subnet selection.
+// Note that packet classification is already checked in Dhcpv4SrvTest
+// .clientClassification above.
+TEST_F(Dhcpv4SrvTest, clientClassify2) {
+
+    NakedDhcpv4Srv srv(0);
+
+    ConstElementPtr status;
+
+    // This test configures 2 subnets. We actually only need the
+    // first one, but since there's still this ugly hack that picks
+    // the pool if there is only one, we must use more than one
+    // subnet. That ugly hack will be removed in #3242, currently
+    // under review.
+
+    // The second subnet does not play any role here. The client's
+    // IP address belongs to the first subnet, so only that first
+    // subnet it being tested.
+    string config = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"subnet4\": [ "
+        "{   \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"client-class\": \"foo\", "
+        "    \"subnet\": \"192.0.2.0/24\" }, "
+        "{   \"pool\": [ \"192.0.3.1 - 192.0.3.100\" ],"
+        "    \"client-class\": \"xyzzy\", "
+        "    \"subnet\": \"192.0.3.0/24\" } "
+        "],"
+        "\"valid-lifetime\": 4000 }";
+
+    ElementPtr json = Element::fromJSON(config);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(srv, json));
+
+    // check if returned status is OK
+    ASSERT_TRUE(status);
+    comment_ = config::parseAnswer(rcode_, status);
+    ASSERT_EQ(0, rcode_);
+
+    Pkt4Ptr dis = Pkt4Ptr(new Pkt4(DHCPDISCOVER, 1234));
+    dis->setRemoteAddr(IOAddress("192.0.2.1"));
+    dis->setIface("eth0");
+    OptionPtr clientid = generateClientId();
+    dis->addOption(clientid);
+
+    // This discover does not belong to foo class, so it will not
+    // be serviced
+    EXPECT_FALSE(srv.selectSubnet(dis));
+
+    // Let's add the packet to bar class and try again.
+    dis->addClass("bar");
+
+    // Still not supported, because it belongs to wrong class.
+    EXPECT_FALSE(srv.selectSubnet(dis));
+
+    // Let's add it to maching class.
+    dis->addClass("foo");
+
+    // This time it should work
+    EXPECT_TRUE(srv.selectSubnet(dis));
+}
+
+
 }; // end of anonymous namespace

src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -436,6 +436,7 @@ public:
 
     std::list<Pkt4Ptr> fake_sent_;
 
+    /// Let's make those methods public, so they can be accessed easily in tests.
     using Dhcpv4Srv::adjustIfaceData;
     using Dhcpv4Srv::appendServerID;
     using Dhcpv4Srv::processDiscover;
@@ -452,6 +453,7 @@ public:
     using Dhcpv4Srv::unpackOptions;
     using Dhcpv4Srv::name_change_reqs_;
     using Dhcpv4Srv::classifyPacket;
+    using Dhcpv4Srv::selectSubnet;
 };
 
 }; // end of isc::dhcp::test namespace

src/bin/dhcp6/dhcp6.dox

@@ -32,7 +32,7 @@
 
  @section dhcpv6Session BIND10 message queue integration
 
- DHCPv4 server component is now integrated with BIND10 message queue.
+ DHCPv6 server component is now integrated with BIND10 message queue.
  It follows the same principle as DHCPv4. See \ref dhcpv4Session for
  details.
 
@@ -218,8 +218,8 @@ Currently there is no class behaviour coded in DHCPv6, hence no v6 equivalent of
 it will be conducted in an external hooks library.
 
 It is possible to define class restrictions in subnet, so a given subnet is only
-accessible to clients that belong to a given class. That is implemented as isc::dhcp::Pkt4::classes_
+accessible to clients that belong to a given class. That is implemented as isc::dhcp::Pkt6::classes_
-being passed in isc::dhcp::Dhcpv4Srv::selectSubnet() to isc::dhcp::CfgMgr::getSubnet4().
+being passed in isc::dhcp::Dhcpv6Srv::selectSubnet() to isc::dhcp::CfgMgr::getSubnet6().
 Currently this capability is usable, but the number of scenarios it supports is
 limited.
 

src/bin/dhcp6/dhcp6.spec

@@ -266,14 +266,14 @@
                   "item_type": "map",
                   "item_optional": false,
                   "item_default": {},
-                  "item_description" : "Structure holding optional relay information.",
+                  "item_description" : "Structure holding relay information.",
                   "map_item_spec": [
                       {
                           "item_name": "ip-address",
                           "item_type": "string",
                           "item_optional": false,
                           "item_default": "::",
-                          "item_description" : "IPv6 address of the relay (optional)."
+                          "item_description" : "IPv6 address of the relay (defaults to :: if not specified)."
                       }
                    ]
                 },

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

@@ -1738,6 +1738,72 @@ TEST_F(Dhcpv6SrvTest, clientClassification) {
     EXPECT_FALSE(sol2->inClass("docsis3.0"));
 }
 
+// Checks if the client-class field is indeed used for subnet selection.
+// Note that packet classification is already checked in Dhcpv6SrvTest
+// .clientClassification above.
+TEST_F(Dhcpv6SrvTest, clientClassify2) {
+
+    NakedDhcpv6Srv srv(0);
+
+    ConstElementPtr status;
+
+    // This test configures 2 subnets. We actually only need the
+    // first one, but since there's still this ugly hack that picks
+    // the pool if there is only one, we must use more than one
+    // subnet. That ugly hack will be removed in #3242, currently
+    // under review.
+
+    // The second subnet does not play any role here. The client's
+    // IP address belongs to the first subnet, so only that first
+    // subnet it being tested.
+    string config = "{ \"interfaces\": [ \"*\" ],"
+        "\"preferred-lifetime\": 3000,"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"subnet6\": [ "
+        " {  \"pool\": [ \"2001:db8:1::/64\" ],"
+        "    \"subnet\": \"2001:db8:1::/48\", "
+        "    \"client-class\": \"foo\" "
+        " }, "
+        " {  \"pool\": [ \"2001:db8:2::/64\" ],"
+        "    \"subnet\": \"2001:db8:2::/48\", "
+        "    \"client-class\": \"xyzzy\" "
+        " } "
+        "],"
+        "\"valid-lifetime\": 4000 }";
+
+    ElementPtr json = Element::fromJSON(config);
+
+    EXPECT_NO_THROW(status = configureDhcp6Server(srv, json));
+
+    // check if returned status is OK
+    ASSERT_TRUE(status);
+    comment_ = config::parseAnswer(rcode_, status);
+    ASSERT_EQ(0, rcode_);
+
+    Pkt6Ptr sol = Pkt6Ptr(new Pkt6(DHCPV6_SOLICIT, 1234));
+    sol->setRemoteAddr(IOAddress("2001:db8:1::3"));
+    sol->addOption(generateIA(D6O_IA_NA, 234, 1500, 3000));
+    OptionPtr clientid = generateClientId();
+    sol->addOption(clientid);
+
+    // This discover does not belong to foo class, so it will not
+    // be serviced
+    EXPECT_FALSE(srv.selectSubnet(sol));
+
+    // Let's add the packet to bar class and try again.
+    sol->addClass("bar");
+
+    // Still not supported, because it belongs to wrong class.
+    EXPECT_FALSE(srv.selectSubnet(sol));
+
+    // Let's add it to maching class.
+    sol->addClass("foo");
+
+    // This time it should work
+    EXPECT_TRUE(srv.selectSubnet(sol));
+}
+
 
 /// @todo: Add more negative tests for processX(), e.g. extend sanityCheck() test
 /// to call processX() methods.

src/lib/dhcp/classify.h

@@ -29,9 +29,10 @@
 /// classification is expected to grow significantly.
 ///
 /// @todo This file should be moved to dhcpsrv eventually as the classification
-/// is server side concept. That is not possible yet, as the Pkt4 and Pkt6
+/// is server side concept. Client has no notion of classifying incoming server
-/// classes have server-side implementation, even though they reside in the
+/// messages as it usually talks to only one server. That move is not possible
-/// dhcp directory.
+/// yet, as the Pkt4 and Pkt6 classes have server-side implementation, even
+/// though they reside in the dhcp directory.
 
 namespace isc {
 
@@ -40,23 +41,25 @@ namespace dhcp {
     /// Definition of a single class.
     typedef std::string ClientClass;
 
-    /// Container for storing client classes
+    /// @brief Container for storing client classes
     ///
     /// Depending on how you look at it, this is either a little more than just
     /// a set of strings or a client classifier that performs access control.
     /// For now, it is a simple access list that may contain zero or more
     /// class names. It is expected to grow in complexity once support for
     /// client classes becomes more feature rich.
+    ///
+    /// Note: This class is derived from std::set which may not have Doxygen
+    /// documentation. See  http://www.cplusplus.com/reference/set/set/.
     class ClientClasses : public std::set<ClientClass> {
     public:
-        /// @brief returns if class X belongs to the defined classes
+        /// @brief returns if class x belongs to the defined classes
         ///
         /// @param x client class to be checked
-        /// @return true if X belongs to the classes
+        /// @return true if x belongs to the classes
         bool
         contains(const ClientClass& x) const {
-            const_iterator it = find(x);
+            return (find(x) != end());
-            return (it != end());
         }
     };
 

src/lib/dhcp/pkt4.cc

@@ -482,12 +482,12 @@ Pkt4::isRelayed() const {
               "hops != 0)");
 }
 
-bool Pkt4::inClass(const std::string& client_class) {
+bool Pkt4::inClass(const isc::dhcp::ClientClass& client_class) {
     return (classes_.find(client_class) != classes_.end());
 }
 
 void
-Pkt4::addClass(const std::string& client_class) {
+Pkt4::addClass(const isc::dhcp::ClientClass& client_class) {
     if (classes_.find(client_class) == classes_.end()) {
         classes_.insert(client_class);
     }

src/lib/dhcp/pkt4.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -556,7 +556,7 @@ public:
     ///
     /// @param client_class name of the class
     /// @return true if belongs
-    bool inClass(const std::string& client_class);
+    bool inClass(const isc::dhcp::ClientClass& client_class);
 
     /// @brief Adds packet to a specified class
     ///
@@ -573,7 +573,7 @@ public:
     /// so I decided to stick with addClass().
     ///
     /// @param client_class name of the class to be added
-    void addClass(const std::string& client_class);
+    void addClass(const isc::dhcp::ClientClass& client_class);
 
     /// @brief Classes this packet belongs to.
     ///

src/lib/dhcp/pkt6.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above

src/lib/dhcpsrv/cfgmgr.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above

src/lib/dhcpsrv/cfgmgr.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -166,7 +166,11 @@ public:
     /// b) our global address on the interface the message was received on
     ///    (for directly connected clients)
     ///
+    /// If there are any classes specified in a subnet, that subnet
+    /// will be selected only if the client belongs to appropriate class.
+    ///
     /// @param hint an address that belongs to a searched subnet
+    /// @param classes classes the client belongs to
     ///
     /// @return a subnet object (or NULL if no suitable match was fount)
     Subnet6Ptr getSubnet6(const isc::asiolink::IOAddress& hint,
@@ -177,7 +181,13 @@ public:
     /// Finds a matching local subnet, based on interface name. This
     /// is used for selecting subnets that were explicitly marked by the
     /// user as reachable over specified network interface.
+    ///
+    /// If there are any classes specified in a subnet, that subnet
+    /// will be selected only if the client belongs to appropriate class.
+    ///
     /// @param iface_name interface name
+    /// @param classes classes the client belongs to
+    ///
     /// @return a subnet object (or NULL if no suitable match was fount)
     Subnet6Ptr getSubnet6(const std::string& iface_name,
                           const isc::dhcp::ClientClasses& classes);
@@ -186,7 +196,11 @@ public:
     ///
     /// Another possibility to find a subnet is based on interface-id.
     ///
+    /// If there are any classes specified in a subnet, that subnet
+    /// will be selected only if the client belongs to appropriate class.
+    ///
     /// @param interface_id content of interface-id option returned by a relay
+    /// @param classes classes the client belongs to
     ///
     /// @return a subnet object
     Subnet6Ptr getSubnet6(OptionPtr interface_id,
@@ -245,7 +259,11 @@ public:
     /// b) our global address on the interface the message was received on
     ///    (for directly connected clients)
     ///
+    /// If there are any classes specified in a subnet, that subnet
+    /// will be selected only if the client belongs to appropriate class.
+    ///
     /// @param hint an address that belongs to a searched subnet
+    /// @param classes classes the client belongs to
     ///
     /// @return a subnet object
     Subnet4Ptr getSubnet4(const isc::asiolink::IOAddress& hint,
@@ -346,7 +364,7 @@ public:
     /// pointer.
     void setD2ClientConfig(D2ClientConfigPtr& new_config);
 
-    /// @param Convenience method for checking if DHCP-DDNS updates are enabled.
+    /// @brief Convenience method for checking if DHCP-DDNS updates are enabled.
     ///
     /// @return True if the D2 configuration is enabled.
     bool ddnsEnabled();

src/lib/dhcpsrv/dhcp_parsers.cc

@@ -811,7 +811,7 @@ OptionDefListParser::commit() {
     }
 }
 
-//****************************** PoolParser ********************************
+//****************************** RelayInfoParser ********************************
 RelayInfoParser::RelayInfoParser(const std::string&,
                                  const isc::dhcp::Subnet::RelayInfoPtr& relay_info,
                                  const asiolink::IOAddress& default_addr)

src/lib/dhcpsrv/dhcp_parsers.h

@@ -768,6 +768,7 @@ public:
     /// @param dummy first argument is ignored, all Parser constructors
     /// accept string as first argument.
     /// @param relay_info is the storage in which to store the parsed
+    /// @param default_addr 0.0.0.0 for IPv4 or :: for IPv6
     /// relay information upon "commit".
     RelayInfoParser(const std::string& dummy,
                     const isc::dhcp::Subnet::RelayInfoPtr& relay_info,
@@ -803,7 +804,6 @@ public:
 
     /// @brief constructor
     ///
-    /// @param unusued
     /// @param global_context
     /// @param default_addr default IP address (0.0.0.0 for IPv4, :: for IPv6)
     SubnetConfigParser(const std::string&, ParserContextPtr global_context,

src/lib/dhcpsrv/subnet.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above

src/lib/dhcpsrv/subnet.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -412,7 +412,7 @@ public:
     ///
     /// Setting this parameter is not needed in most deployments.
     ///
-    /// @params relay IP address of the relay
+    /// @param relay IP address of the relay
     void setRelay(const isc::dhcp::Subnet::RelayInfo& relay);
 
     /// @brief Relay information
@@ -482,8 +482,9 @@ protected:
 
     /// @brief keeps the subnet-id value
     ///
-    /// It is inreased every time a new Subnet object is created.
+    /// It is inreased every time a new Subnet object is created. It is reset
-    /// It is reset (@ref resetSubnetId) every time reconfiguration occurs.
+    /// (@ref resetSubnetID) every time reconfiguration
+    /// occurs.
     ///
     /// Static value initialized in subnet.cc.
     static SubnetID static_id_;

src/lib/dhcpsrv/tests/subnet_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above