[#2246] Additional review comments

src/lib/dhcpsrv/client_class_def.cc
    ClientClassDictionary::createOptions() - modified to merge
    the client and external option definitions together prior
    to creating class options.

src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc
    Tests for a custom client option defined and specified
    in a class.

diff --git a/src/lib/dhcpsrv/client_class_def.cc b/src/lib/dhcpsrv/client_class_def.cc
index 0d090f7d240d1db0db5497059c5dbd93d39ca60d..49ff2d2da3588a30e43a25d17e79f305244c56c2 100644 (file)
--- a/src/lib/dhcpsrv/client_class_def.cc
+++ b/src/lib/dhcpsrv/client_class_def.cc
@@ -407,9 +407,42 @@ ClientClassDictionary::initMatchExpr(uint16_t family) {
 }
 
 void
-ClientClassDictionary::createOptions(const CfgOptionDefPtr& cfg_option_def) {
+ClientClassDictionary::createOptions(const CfgOptionDefPtr& external_defs) {
     for (auto c : *list_) {
-        c->getCfgOption()->createOptions(cfg_option_def);
+        // If the class has no options, skip it.
+        CfgOptionPtr class_options = c->getCfgOption();
+        if (!class_options || class_options->empty()) {
+            continue;
+        }
+
+        // If the class has no option definitions, use the set
+        // of definitions we were given as is to create its
+        // options.
+        if (!c->getCfgOptionDef()) {
+            class_options->createOptions(external_defs);
+        } else {
+            // Class has its own option definitions, we need a
+            // composite set of definitions to recreate its options.
+            // We make copies of both sets of definitions, then merge
+            // the external defs copy into the class defs copy.
+            // We do this because  merging actually effects both sets
+            // of definitions and we cannot alter either set.
+            // Seed the composite set with the class's definitions.
+            CfgOptionDefPtr composite_defs(new CfgOptionDef());
+            c->getCfgOptionDef()->copyTo(*composite_defs);
+
+            // Make a copy of the external definitions and
+            // merge those into the composite set. This should give
+            // us a set of options with class definitions taking
+            // precedence.
+            CfgOptionDefPtr external_defs_copy(new CfgOptionDef());
+            external_defs->copyTo(*external_defs_copy);
+            composite_defs->merge(*external_defs_copy);
+
+            // Now create the class options using the composite
+            // set of definitions.
+            class_options->createOptions(composite_defs);
+        }
     }
 }
 

diff --git a/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc b/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc
index 531c9ae8452c72da2fb2a7c6ca676802d4f7b423..43a2f79c62a3b8f80398b4b7f499e6b9f4eac4b6 100644 (file)
--- a/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc
+++ b/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc
@@ -21,6 +21,7 @@
 #include <hooks/server_hooks.h>
 #include <hooks/callout_manager.h>
 #include <hooks/hooks_manager.h>
+#include <testutils/gtest_utils.h>
 #include <boost/date_time/posix_time/posix_time.hpp>
 #include <boost/make_shared.hpp>
 #include <gtest/gtest.h>
@@ -386,13 +387,28 @@ public:
         client_class->setId(1);
         client_class->setModificationTime(getTimestamp("dhcp4_client_class"));
 
-        // Add an option to the class.
+        // Add a standard option to the class.
         OptionPtr option = Option::create(Option::V4, DHO_BOOT_FILE_NAME);
         OptionDescriptorPtr desc = OptionDescriptor::create(option, true, "bogus-file.txt");
         desc->space_name_ = DHCP4_OPTION_SPACE;
         desc->setModificationTime(getTimestamp("dhcp4_client_class"));
         client_class->getCfgOption()->add(*desc, desc->space_name_);
 
+        // Add a custom option definition to the class.
+        CfgOptionDefPtr cc_cfg_option_def(new CfgOptionDef());
+        def.reset(new OptionDefinition("v4str", 201, "isc", "string"));
+        def->setId(201);
+        def->setModificationTime(getTimestamp("dhcp4_client_class"));
+        cc_cfg_option_def->add(def);
+        client_class->setCfgOptionDef(cc_cfg_option_def);
+
+        // Add a custom option to the class.
+        option = Option::create(Option::V4, 201);
+        desc = OptionDescriptor::create(option, true, "custom-stuff");
+        desc->space_name_ = "isc";
+        desc->setModificationTime(getTimestamp("dhcp4_client_class"));
+        client_class->getCfgOption()->add(*desc, desc->space_name_);
+
         mgr.getPool()->createUpdateClientClass4(BackendSelector::UNSPEC(), ServerSelector::ALL(),
                                                 client_class, "");
 
@@ -523,8 +539,8 @@ public:
         ASSERT_FALSE(audit_entries_.empty())
             << "Require at least one audit entry. The test is broken!";
 
-        ctl_.databaseConfigApply(BackendSelector::UNSPEC(), ServerSelector::ALL(),
-                                 lb_modification_time, audit_entries_);
+        ASSERT_NO_THROW_LOG(ctl_.databaseConfigApply(BackendSelector::UNSPEC(), ServerSelector::ALL(),
+                            lb_modification_time, audit_entries_));
 
         // The updates should have been merged into current configuration.
         auto srv_cfg = CfgMgr::instance().getCurrentCfg();
@@ -605,7 +621,7 @@ public:
             EXPECT_GT(found_class->getMatchExpr()->size(), 0);
             EXPECT_EQ("first-class", found_class->getName());
 
-            // Check for class option, make sure it has been "created".
+            // Check for the standard class option, make sure it has been "created".
             auto cfg_option_desc = found_class->getCfgOption();
             ASSERT_TRUE(cfg_option_desc);
             auto option_desc = cfg_option_desc->get(DHCP4_OPTION_SPACE, DHO_BOOT_FILE_NAME);
@@ -614,6 +630,14 @@ public:
             EXPECT_EQ(OptionBuffer(option_desc.formatted_value_.begin(),
                                    option_desc.formatted_value_.end()),
                       option->getData());
+
+            // Check for the custom class option, make sure it has been "created".
+            option_desc = cfg_option_desc->get("isc", 201);
+            option = option_desc.option_;
+            ASSERT_TRUE(option);
+            EXPECT_EQ(OptionBuffer(option_desc.formatted_value_.begin(),
+                                   option_desc.formatted_value_.end()),
+                      option->getData());
         } else {
             EXPECT_FALSE(found_class);
         }