[4303] Addressed remaining review comments.

src/bin/dhcp4/dhcp4_srv.cc

@@ -22,6 +22,7 @@
 #include <dhcpsrv/addr_utilities.h>
 #include <dhcpsrv/callout_handle_store.h>
 #include <dhcpsrv/cfgmgr.h>
+#include <dhcpsrv/cfg_host_operations.h>
 #include <dhcpsrv/cfg_subnets4.h>
 #include <dhcpsrv/lease_mgr.h>
 #include <dhcpsrv/lease_mgr_factory.h>
@@ -137,47 +138,17 @@ Dhcpv4Exchange::Dhcpv4Exchange(const AllocEnginePtr& alloc_engine,
                 .arg(query->getLabel())
                 .arg(subnet->getID());
         }
-    }
-
-    // Before we can check for static reservations, we need to prepare a set
-    // of identifiers to be used for this.
 
-    // HW address.
-    if (context_->hwaddr_ && !context_->hwaddr_->hwaddr_.empty()) {
-        context_->host_identifiers_[Host::IDENT_HWADDR] = context_->hwaddr_->hwaddr_;
-    }
+        // Find static reservations if not disabled for our subnet.
+        if (subnet->getHostReservationMode() != Subnet::HR_DISABLED) {
+            // Before we can check for static reservations, we need to prepare a set
+            // of identifiers to be used for this.
+            setHostIdentifiers();
 
-    // Client identifier
-    if (context_->clientid_) {
-        const std::vector<uint8_t>& vec = context_->clientid_->getDuid();
-        if (!vec.empty()) {
-            // Client identifier type = DUID? Client identifier holding a DUID
-            // comprises Type (1 byte), IAID (4 bytes), followed by the actual
-            // DUID. Thus, the minimal length is 6.
-            if ((vec[0] == CLIENT_ID_OPTION_TYPE_DUID) && (vec.size() > 5)) {
-                // Extract DUID, skip IAID.
-                context_->host_identifiers_.insert(
-                        AllocEngine::IdentifierPair(Host::IDENT_DUID,
-                                                    std::vector<uint8_t>(vec.begin() + 5,
-                                                                         vec.end())));
-            }
-            /// @todo Add support for other client identifiers (see #4317).
+            // Check for static reservations.
+            alloc_engine->findReservation(*context_);
         }
     }
-    // Circuit id
-    OptionPtr rai = query_->getOption(DHO_DHCP_AGENT_OPTIONS);
-    if (rai) {
-        OptionPtr circuit_id_opt = rai->getOption(RAI_OPTION_AGENT_CIRCUIT_ID);
-        if (circuit_id_opt) {
-            const OptionBuffer& circuit_id_vec = circuit_id_opt->getData();
-            if (!circuit_id_vec.empty()) {
-                context_->host_identifiers_[Host::IDENT_CIRCUIT_ID] = circuit_id_vec;
-            }
-        }
-    }
-
-    // Check for static reservations.
-    alloc_engine->findReservation(*context_);
 };
 
 void
@@ -270,6 +241,58 @@ Dhcpv4Exchange::copyDefaultOptions() {
     }
 }
 
+void
+Dhcpv4Exchange::setHostIdentifiers() {
+    const ConstCfgHostOperationsPtr cfg =
+        CfgMgr::instance().getCurrentCfg()->getCfgHostOperations4();
+    BOOST_FOREACH(const Host::IdentifierType& id_type,
+                  cfg->getIdentifierTypes()) {
+        switch (id_type) {
+        case Host::IDENT_HWADDR:
+            if (context_->hwaddr_ && !context_->hwaddr_->hwaddr_.empty()) {
+                context_->addHostIdentifier(id_type, context_->hwaddr_->hwaddr_);
+            }
+            break;
+
+        case Host::IDENT_DUID:
+            if (context_->clientid_) {
+                const std::vector<uint8_t>& vec = context_->clientid_->getDuid();
+                if (!vec.empty()) {
+                    // Client identifier type = DUID? Client identifier holding a DUID
+                    // comprises Type (1 byte), IAID (4 bytes), followed by the actual
+                    // DUID. Thus, the minimal length is 6.
+                    if ((vec[0] == CLIENT_ID_OPTION_TYPE_DUID) && (vec.size() > 5)) {
+                        // Extract DUID, skip IAID.
+                        context_->addHostIdentifier(id_type,
+                                                    std::vector<uint8_t>(vec.begin() + 5,
+                                                                         vec.end()));
+                    }
+                    /// @todo Add support for other client identifiers (see #4317).
+                }
+            }
+            break;
+
+        case Host::IDENT_CIRCUIT_ID:
+            {
+                OptionPtr rai = query_->getOption(DHO_DHCP_AGENT_OPTIONS);
+                if (rai) {
+                    OptionPtr circuit_id_opt = rai->getOption(RAI_OPTION_AGENT_CIRCUIT_ID);
+                    if (circuit_id_opt) {
+                        const OptionBuffer& circuit_id_vec = circuit_id_opt->getData();
+                        if (!circuit_id_vec.empty()) {
+                            context_->addHostIdentifier(id_type, circuit_id_vec);
+                        }
+                    }
+                }
+            }
+            break;
+
+        default:
+            ;
+        }
+    }
+}
+
 const std::string Dhcpv4Srv::VENDOR_CLASS_PREFIX("VENDOR_CLASS_");
 
 Dhcpv4Srv::Dhcpv4Srv(uint16_t port, const bool use_bcast,

src/bin/dhcp4/dhcp4_srv.h

@@ -137,6 +137,12 @@ private:
     /// not NULL.
     void copyDefaultOptions();
 
+    /// @brief Set host identifiers within a context.
+    ///
+    /// This method sets an order list of host identifier types and
+    /// values which the server should use to find host reservations.
+    void setHostIdentifiers();
+
     /// @brief Pointer to the allocation engine used by the server.
     AllocEnginePtr alloc_engine_;
     /// @brief Pointer to the DHCPv4 message sent by the client.

src/bin/dhcp6/dhcp6_srv.cc

@@ -28,6 +28,7 @@
 #include <dhcp6/dhcp6_log.h>
 #include <dhcp6/dhcp6_srv.h>
 #include <dhcpsrv/callout_handle_store.h>
+#include <dhcpsrv/cfg_host_operations.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcpsrv/lease_mgr.h>
 #include <dhcpsrv/lease_mgr_factory.h>
@@ -277,20 +278,37 @@ Dhcpv6Srv::createContext(const Pkt6Ptr& pkt) {
     AllocEngine::ClientContext6 ctx;
     ctx.subnet_ = selectSubnet(pkt);
     ctx.query_ = pkt;
-
-    // Collect host identifiers.
-    // DUID
     ctx.duid_ = pkt->getClientId();
-    if (ctx.duid_) {
-        ctx.host_identifiers_[Host::IDENT_DUID] = ctx.duid_->getDuid();
-    }
-    // HW Address.
     ctx.hwaddr_ = getMAC(pkt);
-    if (ctx.hwaddr_) {
-        ctx.host_identifiers_[Host::IDENT_HWADDR] = ctx.hwaddr_->hwaddr_;
+
+    // Collect host identifiers if host reservations enabled.
+    if (ctx.subnet_ &&
+        (ctx.subnet_->getHostReservationMode() != Subnet::HR_DISABLED)) {
+        const ConstCfgHostOperationsPtr cfg =
+            CfgMgr::instance().getCurrentCfg()->getCfgHostOperations6();
+        BOOST_FOREACH(const Host::IdentifierType& id_type,
+                      cfg->getIdentifierTypes()) {
+            switch (id_type) {
+            case Host::IDENT_DUID:
+                if (ctx.duid_) {
+                    ctx.addHostIdentifier(id_type, ctx.duid_->getDuid());
+                }
+                break;
+
+            case Host::IDENT_HWADDR:
+                if (ctx.hwaddr_) {
+                    ctx.addHostIdentifier(id_type, ctx.hwaddr_->hwaddr_);
+                }
+                break;
+
+            default:
+                ;
+            }
+        }
+
+        // Find host reservations using specified identifiers.
+        alloc_engine_->findReservation(ctx);
     }
-    // And find a host reservation using those identifiers.
-    alloc_engine_->findReservation(ctx);
 
     return (ctx);
 }

src/lib/dhcpsrv/alloc_engine.cc

@@ -306,7 +306,6 @@ AllocEngine::AllocatorPtr AllocEngine::getAllocator(Lease::Type type) {
 template<typename ContextType>
 void
 AllocEngine::findReservationInternal(ContextType& ctx,
-                                     const ConstCfgHostOperationsPtr& cfg,
                                      const AllocEngine::HostGetFunc& host_get) {
     ctx.host_.reset();
 
@@ -320,20 +319,17 @@ AllocEngine::findReservationInternal(ContextType& ctx,
         if (hr_mode != Subnet::HR_DISABLED) {
             // Iterate over configured identifiers in the order of preference
             // and try to use each of them to search for the reservations.
-            BOOST_FOREACH(const Host::IdentifierType& id, cfg->getIdentifierTypes()) {
-                IdentifierMap::const_iterator id_ctx = ctx.host_identifiers_.find(id);
-                if (id_ctx != ctx.host_identifiers_.end()) {
-                    // Attempt to find a host using a specified identifier.
-                    ctx.host_ = host_get(ctx.subnet_->getID(), id,
-                                         &id_ctx->second[0], id_ctx->second.size());
-                    // If we found matching host, return.
-                    if (ctx.host_) {
-                        return;
-                    }
+            BOOST_FOREACH(const IdentifierPair& id_pair, ctx.host_identifiers_) {
+                // Attempt to find a host using a specified identifier.
+                ctx.host_ = host_get(ctx.subnet_->getID(), id_pair.first,
+                                     &id_pair.second[0], id_pair.second.size());
+                // If we found matching host, return.
+                if (ctx.host_) {
+                    return;
                 }
             }
         }
-    }    
+    }
 }
 
 
@@ -370,17 +366,15 @@ AllocEngine::ClientContext6::ClientContext6(const Subnet6Ptr& subnet, const Duid
 
     // Initialize host identifiers.
     if (duid) {
-        host_identifiers_[Host::IDENT_DUID] = duid->getDuid();
+        addHostIdentifier(Host::IDENT_DUID, duid->getDuid());
     }
 }
 
 
 void AllocEngine::findReservation(ClientContext6& ctx) {
-    ConstCfgHostOperationsPtr cfg =
-        CfgMgr::instance().getCurrentCfg()->getCfgHostOperations6();
-    findReservationInternal(ctx, cfg, boost::bind(&HostMgr::get6,
-                                                  &HostMgr::instance(),
-                                                  _1, _2, _3, _4));
+    findReservationInternal(ctx, boost::bind(&HostMgr::get6,
+                                             &HostMgr::instance(),
+                                             _1, _2, _3, _4));
 }
 
 Lease6Collection
@@ -2099,7 +2093,7 @@ AllocEngine::ClientContext4::ClientContext4(const Subnet4Ptr& subnet,
 
     // Initialize host identifiers.
     if (hwaddr) {
-        host_identifiers_[Host::IDENT_HWADDR] = hwaddr->hwaddr_;
+        addHostIdentifier(Host::IDENT_HWADDR, hwaddr->hwaddr_);
     }
 }
 
@@ -2135,11 +2129,9 @@ AllocEngine::allocateLease4(ClientContext4& ctx) {
 
 void
 AllocEngine::findReservation(ClientContext4& ctx) {
-    ConstCfgHostOperationsPtr cfg =
-        CfgMgr::instance().getCurrentCfg()->getCfgHostOperations4();
-    findReservationInternal(ctx, cfg, boost::bind(&HostMgr::get4,
-                                                  &HostMgr::instance(),
-                                                  _1, _2, _3, _4));
+    findReservationInternal(ctx, boost::bind(&HostMgr::get4,
+                                             &HostMgr::instance(),
+                                             _1, _2, _3, _4));
 }
 
 Lease4Ptr

src/lib/dhcpsrv/alloc_engine.h

@@ -13,7 +13,6 @@
 #include <dhcp/pkt4.h>
 #include <dhcp/pkt6.h>
 #include <dhcp/option6_ia.h>
-#include <dhcpsrv/cfg_host_operations.h>
 #include <dhcpsrv/host.h>
 #include <dhcpsrv/subnet.h>
 #include <dhcpsrv/lease_mgr.h>
@@ -23,6 +22,7 @@
 #include <boost/shared_ptr.hpp>
 #include <boost/noncopyable.hpp>
 
+#include <list>
 #include <map>
 #include <utility>
 
@@ -257,7 +257,7 @@ public:
     typedef std::pair<Host::IdentifierType, std::vector<uint8_t> > IdentifierPair;
 
     /// @brief Map holding values to be used as host identifiers.
-    typedef std::map<Host::IdentifierType, std::vector<uint8_t> > IdentifierMap;
+    typedef std::list<IdentifierPair> IdentifierList;
 
     /// @brief Context information for the DHCPv6 leases allocation.
     ///
@@ -358,9 +358,19 @@ public:
         /// @brief A pointer to the IA_NA/IA_PD option to be sent in response
         Option6IAPtr ia_rsp_;
 
-        /// @brief A map holding host identifiers extracted from a message
+        /// @brief A list holding host identifiers extracted from a message
         /// received by the server.
-        IdentifierMap host_identifiers_;
+        IdentifierList host_identifiers_;
+
+        /// @brief Conveniece function adding host identifier into
+        /// @ref host_identifiers_ list.
+        ///
+        /// @param id_type Identifier type.
+        /// @param identifier Identifier value.
+        void addHostIdentifier(const Host::IdentifierType& id_type,
+                               const std::vector<uint8_t>& identifier) {
+            host_identifiers_.push_back(IdentifierPair(id_type, identifier));
+        }
 
         /// @brief Default constructor.
         ClientContext6();
@@ -634,15 +644,11 @@ private:
     /// functions.
     ///
     /// @param ctx Reference to a @ref ClientContext6 or @ref ClientContext4.
-    /// @param cfg Pointer to object holding general configuration for host
-    /// reservations. It uses this object to read the desired order of
-    /// host identifiers to be used to search for reservations.
     /// @param host_get Pointer to the @ref HostMgr functions to be used
     /// to retrieve reservation by subnet identifier and host identifier.
     /// @tparam ContextType Either @ref ClientContext6 or @ref ClientContext4.
     template<typename ContextType>
     static void findReservationInternal(ContextType& ctx,
-                                        const ConstCfgHostOperationsPtr& cfg,
                                         const HostGetFunc& host_get);
 
     /// @brief creates a lease and inserts it in LeaseMgr if necessary
@@ -1006,9 +1012,19 @@ public:
         /// transaction identification information.
         Pkt4Ptr query_;
 
-        /// @brief A map holding host identifiers extracted from a message
+        /// @brief A list holding host identifiers extracted from a message
         /// received by the server.
-        IdentifierMap host_identifiers_;
+        IdentifierList host_identifiers_;
+
+        /// @brief Conveniece function adding host identifier into
+        /// @ref host_identifiers_ list.
+        ///
+        /// @param id_type Identifier type.
+        /// @param identifier Identifier value.
+        void addHostIdentifier(const Host::IdentifierType& id_type,
+                               const std::vector<uint8_t>& identifier) {
+            host_identifiers_.push_back(IdentifierPair(id_type, identifier));
+        }
 
         /// @brief Default constructor.
         ClientContext4();

src/lib/dhcpsrv/cfg_host_operations.cc

@@ -44,7 +44,7 @@ CfgHostOperations::addIdentifierType(const std::string& identifier_name) {
 }
 
 void
-CfgHostOperations::clear() {
+CfgHostOperations::clearIdentifierTypes() {
     identifier_types_.clear();
 }
 

src/lib/dhcpsrv/cfg_host_operations.h

@@ -74,8 +74,8 @@ public:
         return (identifier_types_);
     }
 
-    /// @brief Restores default configuration.
-    void clear();
+    /// @brief Removes existing identifier types.
+    void clearIdentifierTypes();
 
 private:
 

src/lib/dhcpsrv/parsers/host_reservation_parser.cc

@@ -324,8 +324,8 @@ HostReservationIdsParser::HostReservationIdsParser()
 
 void
 HostReservationIdsParser::build(isc::data::ConstElementPtr ids_list) {
-    // Remove any default configuration.
-    staging_cfg_->clear();
+    // Remove existing identifier types.
+    staging_cfg_->clearIdentifierTypes();
 
     BOOST_FOREACH(ConstElementPtr element, ids_list->listValue()) {
         std::string id_name = element->stringValue();

src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc

@@ -1649,8 +1649,8 @@ TEST_F(AllocEngine4Test, findReservation) {
     AllocEngine::ClientContext4 ctx(subnet_, clientid_, hwaddr_,
                                     IOAddress("0.0.0.0"), false, false,
                                     "", false);
-    ctx.host_identifiers_[Host::IDENT_HWADDR] = hwaddr_->hwaddr_;
-    ctx.host_identifiers_[Host::IDENT_DUID] = clientid_->getDuid();
+    ctx.addHostIdentifier(Host::IDENT_HWADDR, hwaddr_->hwaddr_);
+    ctx.addHostIdentifier(Host::IDENT_DUID, clientid_->getDuid());
 
     // There is no reservation in the database so no host should be
     // retruned.

src/lib/dhcpsrv/tests/alloc_engine_utils.cc

@@ -270,7 +270,7 @@ AllocEngine6Test::simpleAlloc6Test(const Pool6Ptr& pool, const IOAddress& hint,
     AllocEngine::ClientContext6 ctx(subnet_, duid_, iaid_, hint, type,
                                     false, false, "", fake);
     ctx.hwaddr_ = hwaddr_;
-    ctx.host_identifiers_[Host::IDENT_HWADDR] = hwaddr_->hwaddr_;
+    ctx.addHostIdentifier(Host::IDENT_HWADDR, hwaddr_->hwaddr_);
     ctx.query_.reset(new Pkt6(fake ? DHCPV6_SOLICIT : DHCPV6_REQUEST, 1234));
 
     findReservation(*engine, ctx);

src/lib/dhcpsrv/tests/cfg_host_operations_unittest.cc

@@ -10,23 +10,44 @@
 #include <dhcpsrv/host.h>
 #include <gtest/gtest.h>
 #include <algorithm>
+#include <iterator>
 
 using namespace isc;
 using namespace isc::dhcp;
 
 namespace {
 
-/// @brief Checks if specified identifier has been added.
+/// @brief Checks if specified identifier is present.
 ///
 /// @param cfg Object holding current configuration.
 /// @param id Identifier type which presence should be checked.
+/// @return true if specified identifier is present.
 bool
-identifierAdded(const CfgHostOperations& cfg, const Host::IdentifierType& id) {
+identifierPresent(const CfgHostOperations& cfg, const Host::IdentifierType& id) {
     CfgHostOperations::IdentifierTypes types = cfg.getIdentifierTypes();
     return (std::find(types.begin(), types.end(), id) != types.end());
 }
 
-// This test checks default configuration.
+/// @brief Checks if specified identifier is at specified position.
+///
+/// @param cfg Object holding current configuration.
+/// @param id Identifier type which presence should be checked.
+/// @param pos Position at which the identifier is expected on the list.
+/// @return true if specified identifier exists at specified position.
+bool
+identifierAtPosition(const CfgHostOperations& cfg, const Host::IdentifierType& id,
+                     const size_t pos) {
+    CfgHostOperations::IdentifierTypes types = cfg.getIdentifierTypes();
+    if (types.size() > pos) {
+        CfgHostOperations::IdentifierTypes::const_iterator type = types.begin();
+        std::advance(type, pos);
+        return (*type == id);
+    }
+    return (false);
+}
+
+// This test checks that the list of identifiers is initially
+// empty.
 TEST(CfgHostOperationsTest, defaults) {
     CfgHostOperations cfg;
     EXPECT_TRUE(cfg.getIdentifierTypes().empty());
@@ -39,24 +60,24 @@ TEST(CfgHostOperationsTest, addIdentifier) {
 
     // Only HW address added.
     ASSERT_NO_THROW(cfg.addIdentifierType("hw-address"));
-    EXPECT_TRUE(identifierAdded(cfg, Host::IDENT_HWADDR));
-    EXPECT_FALSE(identifierAdded(cfg, Host::IDENT_DUID));
-    EXPECT_FALSE(identifierAdded(cfg, Host::IDENT_CIRCUIT_ID));
+    EXPECT_TRUE(identifierAtPosition(cfg, Host::IDENT_HWADDR, 0));
+    EXPECT_FALSE(identifierPresent(cfg, Host::IDENT_DUID));
+    EXPECT_FALSE(identifierPresent(cfg, Host::IDENT_CIRCUIT_ID));
 
     // HW address and DUID should be present.
     ASSERT_NO_THROW(cfg.addIdentifierType("duid"));
-    EXPECT_TRUE(identifierAdded(cfg, Host::IDENT_HWADDR));
-    EXPECT_TRUE(identifierAdded(cfg, Host::IDENT_DUID));
-    EXPECT_FALSE(identifierAdded(cfg, Host::IDENT_CIRCUIT_ID));
+    EXPECT_TRUE(identifierAtPosition(cfg, Host::IDENT_HWADDR, 0));
+    EXPECT_TRUE(identifierAtPosition(cfg, Host::IDENT_DUID, 1));
+    EXPECT_FALSE(identifierPresent(cfg, Host::IDENT_CIRCUIT_ID));
 
     // All three identifiers should be present now.
     ASSERT_NO_THROW(cfg.addIdentifierType("circuit-id"));
-    EXPECT_TRUE(identifierAdded(cfg, Host::IDENT_HWADDR));
-    EXPECT_TRUE(identifierAdded(cfg, Host::IDENT_DUID));
-    EXPECT_TRUE(identifierAdded(cfg, Host::IDENT_CIRCUIT_ID));
+    EXPECT_TRUE(identifierAtPosition(cfg, Host::IDENT_HWADDR, 0));
+    EXPECT_TRUE(identifierAtPosition(cfg, Host::IDENT_DUID, 1));
+    EXPECT_TRUE(identifierAtPosition(cfg, Host::IDENT_CIRCUIT_ID, 2));
 
     // Let's clear and make sure no identifiers are present.
-    ASSERT_NO_THROW(cfg.clear());
+    ASSERT_NO_THROW(cfg.clearIdentifierTypes());
     EXPECT_TRUE(cfg.getIdentifierTypes().empty());
 }
 
@@ -65,9 +86,9 @@ TEST(CfgHostOperationsTest, addIdentifier) {
 TEST(CfgHostOperationsTest, createConfig4) {
     CfgHostOperationsPtr cfg = CfgHostOperations::createConfig4();
 
-    EXPECT_TRUE(identifierAdded(*cfg, Host::IDENT_HWADDR));
-    EXPECT_TRUE(identifierAdded(*cfg, Host::IDENT_DUID));
-    EXPECT_TRUE(identifierAdded(*cfg, Host::IDENT_CIRCUIT_ID));
+    EXPECT_TRUE(identifierAtPosition(*cfg, Host::IDENT_HWADDR, 0));
+    EXPECT_TRUE(identifierAtPosition(*cfg, Host::IDENT_DUID, 1));
+    EXPECT_TRUE(identifierAtPosition(*cfg, Host::IDENT_CIRCUIT_ID, 2));
 }
 
 // This test verfies that the default DHCPv6 configuration is created
@@ -75,9 +96,9 @@ TEST(CfgHostOperationsTest, createConfig4) {
 TEST(CfgHostOperationsTest, createConfig6) {
     CfgHostOperationsPtr cfg = CfgHostOperations::createConfig6();
 
-    EXPECT_TRUE(identifierAdded(*cfg, Host::IDENT_HWADDR));
-    EXPECT_TRUE(identifierAdded(*cfg, Host::IDENT_DUID));
-    EXPECT_FALSE(identifierAdded(*cfg, Host::IDENT_CIRCUIT_ID));
+    EXPECT_TRUE(identifierAtPosition(*cfg, Host::IDENT_HWADDR, 0));
+    EXPECT_TRUE(identifierAtPosition(*cfg, Host::IDENT_DUID, 1));
+    EXPECT_FALSE(identifierPresent(*cfg, Host::IDENT_CIRCUIT_ID));
 }
 
 } // end of anonymous namespace