[#3592] Addressed review comments

Addressed first round of comments.

Changes to be committed:
modified:   ChangeLog
modified:   doc/sphinx/arm/classify.rst
modified:   doc/sphinx/arm/dhcp4-srv.rst
modified:   doc/sphinx/arm/dhcp6-srv.rst
modified:   src/bin/dhcp4/tests/classify_unittest.cc
modified:   src/lib/dhcp/classify.cc
modified:   src/lib/dhcp/classify.h
modified:   src/lib/dhcpsrv/parsers/base_network_parser.cc
modified:   src/lib/dhcpsrv/parsers/base_network_parser.h
modified:   src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc

10 files changed, 232 insertions, 222 deletions

diff --git a/ChangeLog b/ChangeLog
index ec7e411cc0..2679445c85 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -12,15 +12,15 @@
 	added to HTTP responses.
 	(Gitlab #3609)
 
-2304.	[func]		tmark
+2305.	[func]		tmark
 	Both kea-dhcp4 and kea-dhcp6 servers will now
 	log a warning message when they detect classes that
 	configure lease life time parameters (e.g. 'valid-lifetime',
 	'preferred-lifetime') while also setting
-	'only-in-addditiional-list' to true.
+	'only-in-additiional-list' to true.
 	(Gitlab #2736)
 
-2303.	[bug]		tmark
+2304.	[bug]		tmark
 	Modified both kea-dhcp4 and kea-dhcp6 to avoid
 	generating DDNS update requests when leases are
 	being reused due to lease caching.
diff --git a/doc/sphinx/arm/classify.rst b/doc/sphinx/arm/classify.rst
index d47c1abf69..e7a4218695 100644
--- a/doc/sphinx/arm/classify.rst
+++ b/doc/sphinx/arm/classify.rst
@@ -1005,11 +1005,11 @@ Configuring Subnets With Class Information
     As of Kea 2.7.5, ``client-class`` (a single class name) has been replaced
     with ``client-classes`` (a list of one or more class names) and is now
     deprecated. It will still be accepted as input for a time to allow users
-    to migrate but will eventually be unsupported.
+    to migrate but will eventually be rejected.
 
 In certain cases it is beneficial to restrict access to certain subnets
 only to clients that belong to a given class, using the ``client-classes``
-parameter when defining the subnet.  This parameter may be used to sepcify
+parameter when defining the subnet.  This parameter may be used to specify
 a list of one or more classes to which clients must belong in order to
 use the subnet.
 
diff --git a/doc/sphinx/arm/dhcp4-srv.rst b/doc/sphinx/arm/dhcp4-srv.rst
index 7af7ab2aee..7ef183ff39 100644
--- a/doc/sphinx/arm/dhcp4-srv.rst
+++ b/doc/sphinx/arm/dhcp4-srv.rst
@@ -3499,7 +3499,7 @@ to always be evaluated to ``true``.
    As of Kea version 2.7.4, ``only-if-required`` and ``require-client-classes``
    have been renamed to ``only-in-additional-list`` and ``evaluate-additional-classes``
    respectivley.  The original names will still be accepted as input to allow
-   users to migrate but will eventually be unsupported.
+   users to migrate but will eventually be rejected.
 
 .. note::
 
@@ -6640,7 +6640,7 @@ for clients when client classification is in use, to ensure that the
 appropriate subnet is selected for a given client type.
 
 If a subnet is associated with one or more classes, only the clients belonging
-to at least one of these classes may this subnet. If there are no classes
+to at least one of these classes may use this subnet. If there are no classes
 specified for a subnet, any client connected to a given shared network can use
 this subnet. A common mistake is to assume that a subnet that includes a client
 class is preferred over subnets without client classes.
diff --git a/doc/sphinx/arm/dhcp6-srv.rst b/doc/sphinx/arm/dhcp6-srv.rst
index 8088cd3ccf..43db93df02 100644
--- a/doc/sphinx/arm/dhcp6-srv.rst
+++ b/doc/sphinx/arm/dhcp6-srv.rst
@@ -3230,7 +3230,7 @@ to always be evaluated to ``true``.
    As of Kea version 2.7.4, ``only-if-required`` and ``require-client-classes``
    have been renamed to ``only-in-additional-list`` and ``evaluate-additional-classes``
    respectivley.  The original names will still be accepted as input to allow
-   users to migrate but will eventually be unsupported.
+   users to migrate but will eventually be rejected.
 
 .. _dhcp6-ddns-config:
 
diff --git a/src/bin/dhcp4/tests/classify_unittest.cc b/src/bin/dhcp4/tests/classify_unittest.cc
index cd31b1d20e..51fc400f1b 100644
--- a/src/bin/dhcp4/tests/classify_unittest.cc
+++ b/src/bin/dhcp4/tests/classify_unittest.cc
@@ -2355,13 +2355,13 @@ TEST_F(ClassifyTest, networkScopeClientClasses) {
     };
 
     // Run scenarios.
-    for (const auto& scenario : scenarios) {
+    for (auto const& scenario : scenarios) {
         ClientId id(scenario.client_id_);
         SCOPED_TRACE(id.toText());
         Pkt4Ptr query(new Pkt4(DHCPDISCOVER, 1234));
         query->addOption(OptionPtr(new Option(Option::V4,
-                                               DHO_DHCP_CLIENT_IDENTIFIER,
-                                               id.getClientId())));
+                                              DHO_DHCP_CLIENT_IDENTIFIER,
+                                              id.getClientId())));
         query->setIface("eth0");
         query->setIndex(ETH0_INDEX);
 
diff --git a/src/lib/dhcp/classify.cc b/src/lib/dhcp/classify.cc
index 7a001ba621..38943342be 100644
--- a/src/lib/dhcp/classify.cc
+++ b/src/lib/dhcp/classify.cc
@@ -65,8 +65,7 @@ ClientClasses::intersects(const ClientClasses& cclasses) const {
                 return (true);
             }
         }
-    }
-    else {
+    } else {
         for (const auto& cclass : cclasses) {
             if (contains(cclass)) {
                 return (true);
diff --git a/src/lib/dhcp/classify.h b/src/lib/dhcp/classify.h
index 69cdffd417..8571cf9909 100644
--- a/src/lib/dhcp/classify.h
+++ b/src/lib/dhcp/classify.h
@@ -8,6 +8,7 @@
 #define CLASSIFY_H
 
 #include <cc/data.h>
+#include <cc/cfg_to_element.h>
 
 #include <boost/multi_index_container.hpp>
 #include <boost/multi_index/member.hpp>
@@ -21,7 +22,10 @@
 /// @file   classify.h
 ///
 /// @brief Defines elements for storing the names of client classes
-/// /// This file defines common elements used to track the client classes /// that may be associated with a given packet.  In order to minimize the /// exposure of the DHCP library to server side concepts such as client
+///
+/// This file defines common elements used to track the client classes
+/// that may be associated with a given packet.  In order to minimize the
+/// exposure of the DHCP library to server side concepts such as client
 /// classification the classes herein provide a mechanism to maintain lists
 /// of class names, rather than the classes they represent.  It is the
 /// upper layers' prerogative to use these names as they see fit.
@@ -35,214 +39,222 @@
 namespace isc {
 namespace dhcp {
 
-    /// @brief Defines a single class name.
-    typedef std::string ClientClass;
-
-    /// @brief Tag for the sequence index.
-    struct ClassSequenceTag { };
-
-    /// @brief Tag for the name index.
-    struct ClassNameTag { };
-
-    /// @brief the client class multi-index.
-    typedef boost::multi_index_container<
-        ClientClass,
-        boost::multi_index::indexed_by<
-            // First index is the sequence one.
-            boost::multi_index::sequenced<
-                boost::multi_index::tag<ClassSequenceTag>
-            >,
-            // Second index is the name hash one.
-            boost::multi_index::hashed_unique<
-                boost::multi_index::tag<ClassNameTag>,
-                boost::multi_index::identity<ClientClass>
-            >
+/// @brief Defines a single class name.
+typedef std::string ClientClass;
+
+/// @brief Tag for the sequence index.
+struct ClassSequenceTag { };
+
+/// @brief Tag for the name index.
+struct ClassNameTag { };
+
+/// @brief the client class multi-index.
+typedef boost::multi_index_container<
+    ClientClass,
+    boost::multi_index::indexed_by<
+        // First index is the sequence one.
+        boost::multi_index::sequenced<
+            boost::multi_index::tag<ClassSequenceTag>
+        >,
+        // Second index is the name hash one.
+        boost::multi_index::hashed_unique<
+            boost::multi_index::tag<ClassNameTag>,
+            boost::multi_index::identity<ClientClass>
         >
-    > ClientClassContainer;
-
-    /// @brief Defines a subclass to template class relation.
-    struct SubClassRelation {
-        /// @brief Constructor.
-        SubClassRelation(const ClientClass& class_def, const ClientClass& subclass) :
-            class_def_(class_def), class_(subclass) {
-        }
-
-        /// @brief The class definition name.
-        ClientClass class_def_;
-
-        /// @brief The class or subclass name.
-        ClientClass class_;
-    };
-
-    /// @brief Tag for the sequence index.
-    struct TemplateClassSequenceTag { };
-
-    /// @brief Tag for the name index.
-    struct TemplateClassNameTag { };
-
-    /// @brief the subclass multi-index.
-    typedef boost::multi_index_container<
-        SubClassRelation,
-        boost::multi_index::indexed_by<
-            // First index is the sequence one.
-            boost::multi_index::sequenced<
-                boost::multi_index::tag<TemplateClassSequenceTag>
-            >,
-            // Second index is the name hash one.
-            boost::multi_index::hashed_unique<
-                boost::multi_index::tag<TemplateClassNameTag>,
-                boost::multi_index::member<SubClassRelation,
-                                           ClientClass,
-                                           &SubClassRelation::class_def_>
-            >
+    >
+> ClientClassContainer;
+
+/// @brief Defines a subclass to template class relation.
+struct SubClassRelation {
+    /// @brief Constructor.
+    SubClassRelation(const ClientClass& class_def, const ClientClass& subclass) :
+        class_def_(class_def), class_(subclass) {
+    }
+
+    /// @brief The class definition name.
+    ClientClass class_def_;
+
+    /// @brief The class or subclass name.
+    ClientClass class_;
+};
+
+/// @brief Tag for the sequence index.
+struct TemplateClassSequenceTag { };
+
+/// @brief Tag for the name index.
+struct TemplateClassNameTag { };
+
+/// @brief the subclass multi-index.
+typedef boost::multi_index_container<
+    SubClassRelation,
+    boost::multi_index::indexed_by<
+        // First index is the sequence one.
+        boost::multi_index::sequenced<
+            boost::multi_index::tag<TemplateClassSequenceTag>
+        >,
+        // Second index is the name hash one.
+        boost::multi_index::hashed_unique<
+            boost::multi_index::tag<TemplateClassNameTag>,
+            boost::multi_index::member<SubClassRelation,
+                                       ClientClass,
+                                       &SubClassRelation::class_def_>
         >
-    > SubClassRelationContainer;
+    >
+> SubClassRelationContainer;
+
+/// @brief Container for storing client class names
+///
+/// Both a list to iterate on it in insert order and unordered
+/// set of names for existence.
+class ClientClasses : public isc::data::CfgToElement {
+public:
+
+    /// @brief Type of iterators
+    typedef ClientClassContainer::const_iterator const_iterator;
+    typedef ClientClassContainer::iterator iterator;
+
+    /// @brief Default constructor.
+    ClientClasses() : container_() {
+    }
+
+    /// @brief Constructor from comma separated values.
+    ///
+    /// @param class_names A string containing a client classes separated
+    /// with commas. The class names are trimmed before insertion to the set.
+    ClientClasses(const std::string& class_names);
+
+    /// @brief Destructor.
+    virtual ~ClientClasses() {}
+
+    /// @brief Copy constructor.
+    ///
+    /// @param other ClientClasses object to be copied.
+    ClientClasses(const ClientClasses& other);
+
+    /// @brief Assigns the contents of on container to another.
+    ClientClasses& operator=(const ClientClasses& other);
+
+    /// @brief Compares two ClientClasses container for equality
+    ///
+    /// @return True if the two containers are equal, false otherwise.
+    bool equals(const ClientClasses& other) const;
+
+    /// @brief Compares two ClientClasses containers for equality.
+    ///
+    /// @return True if the two containers are equal, false otherwise.
+    bool operator==(const ClientClasses& other) const {
+        return(equals(other));
+    }
+
+    /// @brief Compares two ClientClasses container for inequality
+    ///
+    /// @return True if the two containers are not equal, false otherwise.
+    bool operator!=(const ClientClasses& other) const {
+        return(!equals(other));
+    }
+
+    /// @brief Insert an element.
+    ///
+    /// @param class_name The name of the class to insert
+    void insert(const ClientClass& class_name) {
+        static_cast<void>(container_.push_back(class_name));
+    }
+
+    /// @brief Erase element by name.
+    ///
+    /// @param class_name The name of the class to erase.
+    void erase(const ClientClass& class_name);
+
+    /// @brief Check if classes is empty.
+    bool empty() const {
+        return (container_.empty());
+    }
+
+    /// @brief Returns the number of classes.
+    ///
+    /// @note; in C++ 11 list size complexity is constant so
+    /// there is no advantage to use the set part.
+    size_t size() const {
+        return (container_.size());
+    }
+
+    /// @brief Iterators to the first element.
+    /// @{
+    const_iterator cbegin() const {
+        return (container_.cbegin());
+    }
+
+    const_iterator begin() const {
+        return (container_.begin());
+    }
+
+    iterator begin() {
+        return (container_.begin());
+    }
+    /// @}
+
+    /// @brief Iterators to the past the end element.
+    /// @{
+    const_iterator cend() const {
+        return (container_.cend());
+    }
+
+    const_iterator end() const {
+        return (container_.end());
+    }
+
+    iterator end() {
+        return (container_.end());
+    }
+    /// @}
+
+    /// @brief returns whether this container has at least one class
+    /// in common with a given container.
+    ///
+    /// @param cclasses list of classes to check for intersection with
+    /// @return true if this container has at least one class that is
+    /// also in cclasses, false otherwise.
+    bool intersects(const ClientClasses& cclasses) const;
+
+    /// @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
+    bool contains(const ClientClass& x) const;
 
-    /// @brief Container for storing client class names
+    /// @brief Clears containers.
+    void clear() {
+        container_.clear();
+    }
+
+    /// @brief Returns all class names as text
+    ///
+    /// @param separator Separator to be used between class names. The
+    /// default separator comprises comma sign followed by space
+    /// character.
     ///
-    /// Both a list to iterate on it in insert order and unordered
-    /// set of names for existence.
-    class ClientClasses {
-    public:
-
-        /// @brief Type of iterators
-        typedef ClientClassContainer::const_iterator const_iterator;
-        typedef ClientClassContainer::iterator iterator;
-
-        /// @brief Default constructor.
-        ClientClasses() : container_() {
-        }
-
-        /// @brief Constructor from comma separated values.
-        ///
-        /// @param class_names A string containing a client classes separated
-        /// with commas. The class names are trimmed before insertion to the set.
-        ClientClasses(const std::string& class_names);
-
-        /// @brief Copy constructor.
-        ///
-        /// @param other ClientClasses object to be copied.
-        ClientClasses(const ClientClasses& other);
-
-        /// @brief Assigns the contents of on container to another.
-        ClientClasses& operator=(const ClientClasses& other);
-
-        /// @brief Compares two ClientClasses container for equality
-        ///
-        /// @return True if the two containers are equal, false otherwise.
-        bool equals(const ClientClasses& other) const;
-
-        /// @brief Compares two ClientClasses containers for equality.
-        ///
-        /// @return True if the two containers are equal, false otherwise.
-        bool operator==(const ClientClasses& other) const {
-            return(equals(other));
-        }
-
-        /// @brief Compares two ClientClasses container for inequality
-        ///
-        /// @return True if the two containers are not equal, false otherwise.
-        bool operator!=(const ClientClasses& other) const {
-            return(!equals(other));
-        }
-
-        /// @brief Insert an element.
-        ///
-        /// @param class_name The name of the class to insert
-        void insert(const ClientClass& class_name) {
-            static_cast<void>(container_.push_back(class_name));
-        }
-
-        /// @brief Erase element by name.
-        ///
-        /// @param class_name The name of the class to erase.
-        void erase(const ClientClass& class_name);
-
-        /// @brief Check if classes is empty.
-        bool empty() const {
-            return (container_.empty());
-        }
-
-        /// @brief Returns the number of classes.
-        ///
-        /// @note; in C++ 11 list size complexity is constant so
-        /// there is no advantage to use the set part.
-        size_t size() const {
-            return (container_.size());
-        }
-
-        /// @brief Iterators to the first element.
-        /// @{
-        const_iterator cbegin() const {
-            return (container_.cbegin());
-        }
-        const_iterator begin() const {
-            return (container_.begin());
-        }
-        iterator begin() {
-            return (container_.begin());
-        }
-        /// @}
-
-        /// @brief Iterators to the past the end element.
-        /// @{
-        const_iterator cend() const {
-            return (container_.cend());
-        }
-        const_iterator end() const {
-            return (container_.end());
-        }
-        iterator end() {
-            return (container_.end());
-        }
-        /// @}
-
-        /// @brief returns whether this container has at least one class
-        /// in given container.
-        ///
-        /// @param cclasses list of classes to check for intersection with
-        /// @return true if this container has at least one class that is
-        /// also in cclasses, false otherwise.
-        bool intersects(const ClientClasses& cclasses) const;
-
-        /// @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
-        bool contains(const ClientClass& x) const;
-
-        /// @brief Clears containers.
-        void clear() {
-            container_.clear();
-        }
-
-        /// @brief Returns all class names as text
-        ///
-        /// @param separator Separator to be used between class names. The
-        /// default separator comprises comma sign followed by space
-        /// character.
-        ///
-        /// @return the string representation of all classes
-        std::string toText(const std::string& separator = ", ") const;
-
-        /// @brief Returns all class names as an ElementPtr of type ListElement
-        ///
-        /// @return the list
-        isc::data::ElementPtr toElement() const;
-
-        /// @brief Sets contents from a ListElement
-        ///
-        /// @param list JSON list of classes from which to populate
-        /// the container.
-        ///
-        /// @throw BadValue if the element is not a list or contents
-        /// are invalid
-        void fromElement(isc::data::ConstElementPtr list);
-
-    private:
-        /// @brief container part
-        ClientClassContainer container_;
-    };
+    /// @return the string representation of all classes
+    std::string toText(const std::string& separator = ", ") const;
+
+    /// @brief Returns all class names as an ElementPtr of type ListElement
+    ///
+    /// @return the list
+    virtual isc::data::ElementPtr toElement() const;
+
+    /// @brief Sets contents from a ListElement
+    ///
+    /// @param list JSON list of classes from which to populate
+    /// the container.
+    ///
+    /// @throw BadValue if the element is not a list or contents
+    /// are invalid
+    void fromElement(isc::data::ConstElementPtr list);
+
+private:
+    /// @brief container part
+    ClientClassContainer container_;
+};
+
 }
 }
 
diff --git a/src/lib/dhcpsrv/parsers/base_network_parser.cc b/src/lib/dhcpsrv/parsers/base_network_parser.cc
index cc3bb5b45d..7cfad4b83e 100644
--- a/src/lib/dhcpsrv/parsers/base_network_parser.cc
+++ b/src/lib/dhcpsrv/parsers/base_network_parser.cc
@@ -305,8 +305,7 @@ BaseNetworkParser::getClientClassesElem(ConstElementPtr params,
     }
 
     if (class_list) {
-        const std::vector<data::ElementPtr>& classes = class_list->listValue();
-        for (auto const& cclass : classes) {
+        for (auto const& cclass : class_list->listValue()) {
             if ((cclass->getType() != Element::string) ||
                 cclass->stringValue().empty()) {
                 isc_throw(DhcpConfigError, "invalid class name (" << cclass->getPosition() << ")");
diff --git a/src/lib/dhcpsrv/parsers/base_network_parser.h b/src/lib/dhcpsrv/parsers/base_network_parser.h
index f61cca2002..6a5760c240 100644
--- a/src/lib/dhcpsrv/parsers/base_network_parser.h
+++ b/src/lib/dhcpsrv/parsers/base_network_parser.h
@@ -144,7 +144,6 @@ public:
     ///
     /// @param params configuration element tree to search.
     /// @param adder_func function to add class names to an object's client class list.
-    /// @return Element referred to or an empty pointer.
     /// @throw DhcpConfigError if both entries are present.
     static void getClientClassesElem(data::ConstElementPtr params,
                                      ClassAdderFunc adder_func);
diff --git a/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc b/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc
index 8474fe3fc1..3eba3f573a 100644
--- a/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc
+++ b/src/lib/dhcpsrv/tests/shared_network_parser_unittest.cc
@@ -1094,7 +1094,8 @@ TEST_F(SharedNetwork6ParserTest, deprecatedRequireClientClasses) {
     std::string config =
        R"^({
             "name": "foo",
-            "require-client-classes": [ "one", "two" ] })^";
+            "require-client-classes": [ "one", "two" ]
+       })^";
 
     ElementPtr config_element = Element::fromJSON(config);