[4096] ClientClassDef now stores CfgOption not OptionCollection

The initial choice for storing options as an OptionCollection
in the ClientClassDef was incorrect.  They are now stored within
a CfgOption which is symmetrical with how they are stored globally
and in subnets.

diff --git a/src/lib/dhcpsrv/client_class_def.cc b/src/lib/dhcpsrv/client_class_def.cc
index fc5c06409a5a9f842a5d698f6580b708f1201a0a..67a2f3462085f86fc55aff6e17887628be52e2cc 100644 (file)
--- a/src/lib/dhcpsrv/client_class_def.cc
+++ b/src/lib/dhcpsrv/client_class_def.cc
@@ -21,8 +21,8 @@ namespace dhcp {
 
 ClientClassDef::ClientClassDef(const std::string& name,
                                const ExpressionPtr& match_expr,
-                               const OptionCollectionPtr& options)
-    : name_(name), match_expr_(match_expr), options_(options) {
+                               const CfgOptionPtr& cfg_option)
+    : name_(name), match_expr_(match_expr), cfg_option_(cfg_option) {
 
     // Name can't be blank
     if (name_.empty()) {
@@ -31,8 +31,8 @@ ClientClassDef::ClientClassDef(const std::string& name,
     // @todo Does it make sense for a class to NOT have match expression?
 
     // For classes without options, make sure we have an empty collection
-    if (!options_) {
-        options_.reset(new OptionCollection());
+    if (!cfg_option_) {
+        cfg_option_.reset(new CfgOption());
     }
 }
 
@@ -59,26 +59,14 @@ ClientClassDef::setMatchExpr(const ExpressionPtr& match_expr) {
     match_expr_ = match_expr;
 }
 
-const OptionCollectionPtr&
-ClientClassDef::getOptions() const {
-    return (options_);
+const CfgOptionPtr&
+ClientClassDef::getCfgOption() const {
+    return (cfg_option_);
 }
 
 void
-ClientClassDef::setOptions(const OptionCollectionPtr& options) {
-    options_ = options;
-}
-
-OptionPtr
-ClientClassDef::findOption(uint16_t option_code) const {
-    if (options_) {
-        isc::dhcp::OptionCollection::iterator it = options_->find(option_code);
-        if (it != options_->end()) {
-            return ((*it).second);
-        }
-    }
-
-    return (OptionPtr());
+ClientClassDef::setCfgOption(const CfgOptionPtr& cfg_option) {
+    cfg_option_ = cfg_option;
 }
 
 std::ostream& operator<<(std::ostream& os, const ClientClassDef& x) {
@@ -98,8 +86,8 @@ ClientClassDictionary::~ClientClassDictionary() {
 void
 ClientClassDictionary::addClass(const std::string& name,
                                 const ExpressionPtr& match_expr,
-                                const OptionCollectionPtr& options) {
-    ClientClassDefPtr cclass(new ClientClassDef(name, match_expr, options));
+                                const CfgOptionPtr& cfg_option) {
+    ClientClassDefPtr cclass(new ClientClassDef(name, match_expr, cfg_option));
     addClass(cclass);
 }
 

diff --git a/src/lib/dhcpsrv/client_class_def.h b/src/lib/dhcpsrv/client_class_def.h
index 523c805120b24a2695396061bc13a8c41b03e57b..01d56222ea0bb7d3419d60e5b09a5a637fd2864c 100644 (file)
--- a/src/lib/dhcpsrv/client_class_def.h
+++ b/src/lib/dhcpsrv/client_class_def.h
@@ -15,7 +15,7 @@
 #ifndef CLIENT_CLASS_DEF_H
 #define CLIENT_CLASS_DEF_H
 
-#include <dhcp/option.h>
+#include <dhcpsrv/cfg_option.h>
 #include <eval/token.h>
 #include <exceptions/exceptions.h>
 
@@ -53,7 +53,7 @@ class ClientClassDef {
     /// @param match_expr Expression the class will use to determine membership
     /// @param options Collection of options members should be given
     ClientClassDef(const std::string& name, const ExpressionPtr& match_expr,
-                const OptionCollectionPtr& options = OptionCollectionPtr());
+                const CfgOptionPtr& options = CfgOptionPtr());
 
     /// @brief Destructor
     virtual ~ClientClassDef();
@@ -75,19 +75,12 @@ class ClientClassDef {
     void setMatchExpr(const ExpressionPtr& match_expr);
 
     /// @brief Fetches the class's option collection
-    const OptionCollectionPtr& getOptions() const;
+    const CfgOptionPtr& getCfgOption() const;
 
     /// @brief Sets the class's option collection
     ///
     /// @param options the option collection to assign the class
-    void setOptions(const OptionCollectionPtr& options);
-
-    /// @brief Fetches an option from the class's collection by code
-    ///
-    /// @param option_code Option code value of the desired option
-    /// @return A pointer to the option if found, otherwise an
-    /// empty pointer
-    OptionPtr findOption(uint16_t option_code) const;
+    void setCfgOption(const CfgOptionPtr& cfg_option);
 
     /// @brief Provides a convenient text representation of the class
     friend std::ostream& operator<<(std::ostream& os, const ClientClassDef& x);
@@ -100,9 +93,8 @@ class ClientClassDef {
     /// this class.
     ExpressionPtr match_expr_;
 
-    /// @brief The collection of options members should be given
-    /// Currently this is a multimap, not sure we need/want that complexity
-    OptionCollectionPtr options_;
+    /// @brief The option data configuration for this class
+    CfgOptionPtr cfg_option_;
 };
 
 /// @brief a pointer to an ClientClassDef
@@ -134,7 +126,7 @@ class ClientClassDictionary {
     /// dictionary.  See @ref dhcp::ClientClassDef::ClientClassDef() for
     /// others.
     void addClass(const std::string& name, const ExpressionPtr& match_expr,
-                  const OptionCollectionPtr& options);
+                  const CfgOptionPtr& options);
 
     /// @brief Adds a new class to the list
     ///

diff --git a/src/lib/dhcpsrv/tests/client_class_def_unittest.cc b/src/lib/dhcpsrv/tests/client_class_def_unittest.cc
index 36b98ad54572bc493b99d6e21a100d35025a1283..8aaa71311632464338db580cf9a38652e0162b26 100644 (file)
--- a/src/lib/dhcpsrv/tests/client_class_def_unittest.cc
+++ b/src/lib/dhcpsrv/tests/client_class_def_unittest.cc
@@ -26,82 +26,85 @@ using namespace isc;
 
 namespace {
 
+// Tests basic construction of ClientClassDef
 TEST(ClientClassDef, construction) {
     boost::scoped_ptr<ClientClassDef> cclass;
 
     std::string name = "class1";
     ExpressionPtr expr;
-    OptionCollectionPtr options;
+    CfgOptionPtr cfg_option;
 
     // Classes cannot have blank names
-    ASSERT_THROW(cclass.reset(new ClientClassDef("", expr, options)), BadValue);
+    ASSERT_THROW(cclass.reset(new ClientClassDef("", expr, cfg_option)), BadValue);
 
-    // Verify we can create a class with a name, expression, and no options
+    // Verify we can create a class with a name, expression, and no cfg_option
     ASSERT_NO_THROW(cclass.reset(new ClientClassDef(name, expr)));
     EXPECT_EQ(name, cclass->getName());
     ASSERT_FALSE(cclass->getMatchExpr());
 
-    // Verify we get an empty collection of options
-    options = cclass->getOptions();
-    ASSERT_TRUE(options);
-    EXPECT_EQ(0, options->size());
+    // Verify we get an empty collection of cfg_option
+    cfg_option = cclass->getCfgOption();
+    ASSERT_TRUE(cfg_option);
+    //EXPECT_EQ(0, cfg_option->size());
 }
 
-TEST(ClientClassDef, optionsBasics) {
+// Tests options operations.  Note we just do the basics
+// as CfgOption is heavily tested elsewhere.
+TEST(ClientClassDef, cfgOptionBasics) {
     boost::scoped_ptr<ClientClassDef> cclass;
 
     std::string name = "class1";
     ExpressionPtr expr;
-    OptionCollectionPtr options;
+    CfgOptionPtr test_options;
+    CfgOptionPtr class_options;
     OptionPtr opt;
 
     // First construct the class with empty option pointer
-    ASSERT_NO_THROW(cclass.reset(new ClientClassDef(name, expr, options)));
+    ASSERT_NO_THROW(cclass.reset(new ClientClassDef(name, expr, test_options)));
 
     // We should get back a collection with no entries,
     // not an empty collection pointer
-    options = cclass->getOptions();
-    ASSERT_TRUE(options);
-    EXPECT_EQ(0, options->size());
-
-    // We should not be able find an option
-    opt = cclass->findOption(17);
-    ASSERT_FALSE(opt);
-
-    // Create an option container with two options
-    options.reset(new OptionCollection());
-    EXPECT_NO_THROW(opt.reset(new Option(Option::V4, 17)));
-    options->insert(make_pair(17, opt));
-    EXPECT_NO_THROW(opt.reset(new Option(Option::V4, 18)));
-    options->insert(make_pair(18, opt));
-
-    // Now remake the client class with options
-    ASSERT_NO_THROW(cclass.reset(new ClientClassDef(name, expr, options)));
-
-    options = cclass->getOptions();
-    ASSERT_TRUE(options);
-    EXPECT_EQ(2, options->size());
-
-    // We should be able to find option 17
-    opt = cclass->findOption(17);
-    ASSERT_TRUE(opt);
-    EXPECT_EQ(17, opt->getType());
-
-    // We should be able to find option 18
-    opt = cclass->findOption(18);
-    ASSERT_TRUE(opt);
-    EXPECT_EQ(18, opt->getType());
-
-    // We should not be able to find option 90
-    opt = cclass->findOption(90);
-    ASSERT_FALSE(opt);
+    class_options = cclass->getCfgOption();
+    ASSERT_TRUE(class_options);
+
+    // Create an option container and add some options
+    OptionPtr option;
+    test_options.reset(new CfgOption());
+    option.reset(new Option(Option::V4, 17, OptionBuffer(10, 0xFF)));
+    ASSERT_NO_THROW(test_options->add(option, false, "dhcp4"));
+
+    option.reset(new Option(Option::V6, 101, OptionBuffer(10, 0xFF)));
+    ASSERT_NO_THROW(test_options->add(option, false, "isc"));
+
+    option.reset(new Option(Option::V6, 100, OptionBuffer(10, 0xFF)));
+    ASSERT_NO_THROW(test_options->add(option, false, "dhcp6"));
+
+    // Now remake the client class with cfg_option
+    ASSERT_NO_THROW(cclass.reset(new ClientClassDef(name, expr, test_options)));
+    class_options = cclass->getCfgOption();
+    ASSERT_TRUE(class_options);
+
+    // Now make sure we can find all the options
+    OptionDescriptor opt_desc = class_options->get("dhcp4",17);
+    ASSERT_TRUE(opt_desc.option_);
+    EXPECT_EQ(100, opt_desc.option_->getType());
+
+    opt_desc = class_options->get("isc",101);
+    ASSERT_TRUE(opt_desc.option_);
+    EXPECT_EQ(101, opt_desc.option_->getType());
+
+    opt_desc = class_options->get("dhcp6",100);
+    ASSERT_TRUE(opt_desc.option_);
+    EXPECT_EQ(100, opt_desc.option_->getType());
 }
 
+// Tests the basic operation of ClientClassDictionary
+// This includes adding, finding, and removing classes
 TEST(ClientClassDictionary, basics) {
     ClientClassDictionaryPtr dictionary;
     ClientClassDefPtr cclass;
     ExpressionPtr expr;
-    OptionCollectionPtr options;
+    CfgOptionPtr cfg_option;
 
     // Verify constructor doesn't throw
     ASSERT_NO_THROW(dictionary.reset(new ClientClassDictionary()));
@@ -113,19 +116,19 @@ TEST(ClientClassDictionary, basics) {
     EXPECT_EQ(0, classes->size());
 
     // Verify that we can add classes with both addClass variants
-    // First addClass(name, expression, options)
-    ASSERT_NO_THROW(dictionary->addClass("cc1", expr, options));
-    ASSERT_NO_THROW(dictionary->addClass("cc2", expr, options));
+    // First addClass(name, expression, cfg_option)
+    ASSERT_NO_THROW(dictionary->addClass("cc1", expr, cfg_option));
+    ASSERT_NO_THROW(dictionary->addClass("cc2", expr, cfg_option));
 
     // Verify duplicate add attempt throws
-    ASSERT_THROW(dictionary->addClass("cc2", expr, options),
+    ASSERT_THROW(dictionary->addClass("cc2", expr, cfg_option),
                  DuplicateClientClassDef);
 
     // Verify that you cannot add a class with no name.
-    ASSERT_THROW(dictionary->addClass("", expr, options), BadValue);
+    ASSERT_THROW(dictionary->addClass("", expr, cfg_option), BadValue);
 
     // Now with addClass(class pointer)
-    ASSERT_NO_THROW(cclass.reset(new ClientClassDef("cc3", expr, options)));
+    ASSERT_NO_THROW(cclass.reset(new ClientClassDef("cc3", expr, cfg_option)));
     ASSERT_NO_THROW(dictionary->addClass(cclass));
 
     // Verify duplicate add attempt throws