[3467] Option code, name and space are now optional.

src/lib/dhcp/option6_addrlst.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2012 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-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
@@ -17,7 +17,7 @@
 
 #include <asiolink/io_address.h>
 #include <dhcp/option.h>
-
+#include <boost/shared_ptr.hpp>
 #include <vector>
 
 namespace isc {
@@ -94,6 +94,9 @@ protected:
     AddressContainer addrs_;
 };
 
+/// @brief Pointer to the @c Option6AddrLst object.
+typedef boost::shared_ptr<Option6AddrLst> Option6AddrLstPtr;
+
 } // isc::dhcp namespace
 } // isc namespace
 

src/lib/dhcpsrv/cfg_option_def.cc

@@ -138,7 +138,27 @@ CfgOptionDef::get(const std::string& option_space,
     return (OptionDefinitionPtr());
 }
 
-
+OptionDefinitionPtr
+CfgOptionDef::get(const std::string& option_space,
+                  const std::string& option_name) const {
+    // Get the pointer to collection of the option definitions that belong
+    // to the particular option space.
+    OptionDefContainerPtr defs = getAll(option_space);
+    // If there are any option definitions for this option space, get the
+    // one that has the specified option name.
+    if (defs && !defs->empty()) {
+        const OptionDefContainerNameIndex& idx = defs->get<2>();
+        const OptionDefContainerNameRange& range = idx.equal_range(option_name);
+        // If there is more than one definition matching the option name,
+        // return the first one. In fact, it shouldn't happen that we have
+        // more than one because we check for duplicates when we add them.
+        if (std::distance(range.first, range.second) > 0) {
+            return (*range.first);
+        }
+    }
+    // Nothing found. Return NULL pointer.
+    return (OptionDefinitionPtr());
+}
 
 } // end of namespace isc::dhcp
 } // end of namespace isc

src/lib/dhcpsrv/cfg_option_def.h

@@ -110,6 +110,16 @@ public:
     OptionDefinitionPtr get(const std::string& option_space,
                             const uint16_t option_code) const;
 
+    /// @brief Return option definition for the particular option space and name.
+    ///
+    /// @param option_space pption space.
+    /// @param option_name option name.
+    ///
+    /// @return An option definition or NULL pointer if option definition
+    /// has not been found.
+    OptionDefinitionPtr get(const std::string& option_space,
+                            const std::string& option_name) const;
+
 private:
 
     /// @brief A collection of option definitions.

src/lib/dhcpsrv/dhcp_parsers.cc

@@ -388,11 +388,7 @@ OptionDataParser::extractName(ConstElementPtr parent) const {
         return (OptionalValue<std::string>(name));
     }
 
-    if (name.empty()) {
-        isc_throw(DhcpConfigError, "option name is empty ("
-                  << string_values_->getPosition("name", parent) << ")");
-
-    } else if (name.find(" ") != std::string::npos) {
+    if (name.find(" ") != std::string::npos) {
         isc_throw(DhcpConfigError, "invalid option name '" << name
                   << "', space character is not allowed ("
                   << string_values_->getPosition("name", parent) << ")");
@@ -465,18 +461,17 @@ OptionDataParser::extractSpace() const {
     return (space);
 }
 
-
+template<typename SearchKey>
 OptionDefinitionPtr
 OptionDataParser::findOptionDefinition(const std::string& option_space,
-                                       const uint32_t option_code) const {
+                                       const SearchKey& search_key) const {
     const Option::Universe u = address_family_ == AF_INET ?
         Option::V4 : Option::V6;
     OptionDefinitionPtr def;
 
-    if (((option_space == DHCP4_OPTION_SPACE) ||
-         (option_space == DHCP6_OPTION_SPACE)) &&
-        LibDHCP::isStandardOption(u, option_code)) {
-        def = LibDHCP::getOptionDef(u, option_code);
+    if ((option_space == DHCP4_OPTION_SPACE) ||
+        (option_space == DHCP6_OPTION_SPACE)) {
+        def = LibDHCP::getOptionDef(u, search_key);
 
     }
 
@@ -485,14 +480,14 @@ OptionDataParser::findOptionDefinition(const std::string& option_space,
         // definition.
         uint32_t vendor_id = CfgOption::optionSpaceToVendorId(option_space);
         if (vendor_id) {
-            def = LibDHCP::getVendorOptionDef(u, vendor_id, option_code);
+            def = LibDHCP::getVendorOptionDef(u, vendor_id, search_key);
         }
     }
 
     if (!def) {
         // Check if this is an option specified by a user.
         def = CfgMgr::instance().getStagingCfg()->getCfgOptionDef()
-            ->get(option_space, option_code);
+            ->get(option_space, search_key);
     }
 
     return (def);
@@ -509,18 +504,40 @@ OptionDataParser::createOption(ConstElementPtr option_data) {
     std::string data_param = extractData();
     std::string space_param = extractSpace();
 
-    // Try to find a corresponding option definition.
-    OptionDefinitionPtr def = findOptionDefinition(space_param, code_param);
+    // Require that option code or option name is specified.
+    if (!code_param.isSpecified() && !name_param.isSpecified()) {
+        isc_throw(DhcpConfigError, "option data configuration requires one of"
+                  " 'code' or 'name' parameters to be specified"
+                  << " (" << option_data->getPosition() << ")");
+    }
+
+    // Try to find a corresponding option definition using option code or
+    // option name.
+    OptionDefinitionPtr def = code_param.isSpecified() ?
+        findOptionDefinition(space_param, code_param) :
+        findOptionDefinition(space_param, name_param);
 
     // If there is no definition, the user must not explicitly enable the
     // use of csv-format.
-    if (!def && csv_format_param.isSpecified() && csv_format_param) {
+    if (!def) {
+        // If explicitly requested that the CSV format is to be used,
+        // the option definition is a must.
+        if (csv_format_param.isSpecified() && csv_format_param) {
             isc_throw(DhcpConfigError, "definition for the option '"
                       << space_param << "." << name_param
                       << "' having code '" << code_param
                       << "' does not exist ("
                       << string_values_->getPosition("name", option_data)
                       << ")");
+
+        // If there is no option definition and the option code is not specified
+        // we have no means to find the option code.
+        } else if (name_param.isSpecified() && !code_param.isSpecified()) {
+            isc_throw(DhcpConfigError, "definition for the option '"
+                      << space_param << "." << name_param
+                      << " does not exist ("
+                      << string_values_->getPosition("name", option_data));
+        }
     }
 
     // Transform string of hexadecimal digits into binary format.
@@ -574,12 +591,8 @@ OptionDataParser::createOption(ConstElementPtr option_data) {
 
     } else {
 
-        // Option name should match the definition. The option name
-        // may seem to be redundant but in the future we may want
-        // to reference options and definitions using their names
-        // and/or option codes so keeping the option name in the
-        // definition of option value makes sense.
-        if (def->getName() != name_param.get()) {
+        // Option name is specified it should match the name in the definition.
+        if (name_param.isSpecified() && (def->getName() != name_param.get())) {
             isc_throw(DhcpConfigError, "specified option name '"
                       << name_param << "' does not match the "
                       << "option definition: '" << space_param
@@ -593,8 +606,8 @@ OptionDataParser::createOption(ConstElementPtr option_data) {
         try {
             OptionPtr option =
                 !csv_format_param.isSpecified() || csv_format_param ?
-                def->optionFactory(universe, code_param, data_tokens) :
-                def->optionFactory(universe, code_param, binary);
+                def->optionFactory(universe, def->getCode(), data_tokens) :
+                def->optionFactory(universe, def->getCode(), binary);
             OptionDescriptor desc(option, false);
             option_descriptor_.option_ = option;
             option_descriptor_.persistent_ = false;
@@ -602,7 +615,7 @@ OptionDataParser::createOption(ConstElementPtr option_data) {
         } catch (const isc::Exception& ex) {
             isc_throw(DhcpConfigError, "option data does not match"
                       << " option definition (space: " << space_param
-                      << ", code: " << code_param << "): "
+                      << ", code: " << def->getCode() << "): "
                       << ex.what() << " ("
                       << string_values_->getPosition("data", option_data)
                       << ")");

src/lib/dhcpsrv/dhcp_parsers.h

@@ -544,8 +544,7 @@ public:
 
     /// @brief virtual destructor to ensure orderly destruction of derivations.
     virtual ~OptionDataParser(){};
-
-protected:
+private:
 
     /// @brief Finds an option definition within an option space
     ///
@@ -553,16 +552,18 @@ protected:
     /// option defintion within the option defintion storage.
     ///
     /// @param option_space name of the parameter option space
-    /// @param option_code numeric value of the parameter to find
+    /// @param search_key an option code or name to be used to lookup the
+    /// option definition.
+    /// @tparam A numeric type for searching using an option code or the
+    /// string for searching using the option name.
+    ///
     /// @return OptionDefintionPtr of the option defintion or an
     /// empty OptionDefinitionPtr if not found.
     /// @throw DhcpConfigError if the option space requested is not valid
     /// for this server.
-    virtual OptionDefinitionPtr
-    findOptionDefinition(const std::string& option_space,
-                         const uint32_t option_code) const;
-
-private:
+    template<typename SearchKey>
+    OptionDefinitionPtr findOptionDefinition(const std::string& option_space,
+                                             const SearchKey& search_key) const;
 
     /// @brief Create option instance.
     ///

src/lib/dhcpsrv/tests/cfg_option_def_unittest.cc

@@ -122,7 +122,7 @@ TEST(CfgOptionDefTest, getAll) {
 }
 
 // This test verifies that single option definition is correctly
-// returned with getOptionDef function.
+// returned with get function.
 TEST(CfgOptionDefTest, get) {
     CfgOptionDef cfg;
     // Create a set of option definitions with codes between 100 and 109.
@@ -160,6 +160,12 @@ TEST(CfgOptionDefTest, get) {
         option_name << "option-" << code;
         EXPECT_EQ(option_name.str(), def->getName());
         EXPECT_EQ(code, def->getCode());
+
+        // Try to get the same option definition using an option name as
+        // a key.
+        def = cfg.get("isc", option_name.str());
+        ASSERT_TRUE(def);
+        EXPECT_EQ(code, def->getCode());
     }
 
     // Check that the option codes are valid.
@@ -172,7 +178,12 @@ TEST(CfgOptionDefTest, get) {
         std::ostringstream option_name;
         option_name << "option-other-" << code;
         EXPECT_EQ(option_name.str(), def->getName());
+        EXPECT_EQ(code, def->getCode());
 
+        // Try to get the same option definition using an option name as
+        // a key.
+        def = cfg.get("abcde", option_name.str());
+        ASSERT_TRUE(def);
         EXPECT_EQ(code, def->getCode());
     }
 

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -18,6 +18,7 @@
 #include <dhcp/option.h>
 #include <dhcp/option_custom.h>
 #include <dhcp/option_int.h>
+#include <dhcp/option6_addrlst.h>
 #include <dhcp/tests/iface_mgr_test_config.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcpsrv/subnet.h>
@@ -404,7 +405,6 @@ public:
             ConstElementPtr status = parseElementSet(json);
             ConstElementPtr comment = parseAnswer(rcode_, status);
             error_text_ = comment->stringValue();
-            std::cout << error_text_ << std::endl;
             // If error was reported, the error string should contain
             // position of the data element which caused failure.
             if (rcode_ != 0) {
@@ -557,7 +557,7 @@ TEST_F(ParseConfigTest, basicOptionDataTest) {
 // This test checks behavior of the configuration parser for option data
 // for different values of csv-format parameter and when there is an option
 // definition present.
-TEST_F(ParseConfigTest, csvFormatWithOptionDef) {
+TEST_F(ParseConfigTest, optionDataCSVFormatWithOptionDef) {
     std::string config =
         "{ \"option-data\": [ {"
         "    \"name\": \"swap-server\","
@@ -624,7 +624,7 @@ TEST_F(ParseConfigTest, csvFormatWithOptionDef) {
 // This test checks behavior of the configuration parser for option data
 // for different values of csv-format parameter and when there is no
 // option definition.
-TEST_F(ParseConfigTest, csvFormatNoOptionDef) {
+TEST_F(ParseConfigTest, optionDataCSVFormatNoOptionDef) {
     // This option doesn't have any definition. It is ok to use such
     // an option but the data should be specified in hex, not as CSV.
     // Note that the parser will by default use the CSV format for the
@@ -698,6 +698,85 @@ TEST_F(ParseConfigTest, csvFormatNoOptionDef) {
     EXPECT_EQ(0x56, opt->getData()[2]);
 }
 
+// This test verifies that the option name is not mandatory, if the option
+// code has been specified.
+TEST_F(ParseConfigTest, optionDataNoName) {
+    std::string config =
+        "{ \"option-data\": [ {"
+        "    \"space\": \"dhcp6\","
+        "    \"code\": 23,"
+        "    \"csv-format\": True,"
+        "    \"data\": \"2001:db8:1::1\""
+        " } ]"
+        "}";
+    int rcode = 0;
+    ASSERT_NO_THROW(rcode = parseConfiguration(config));
+    EXPECT_EQ(0, rcode);
+    Option6AddrLstPtr opt = boost::dynamic_pointer_cast<
+        Option6AddrLst>(getOptionPtr("dhcp6", 23));
+    ASSERT_TRUE(opt);
+    ASSERT_EQ(1, opt->getAddresses().size());
+    EXPECT_EQ( "2001:db8:1::1", opt->getAddresses()[0].toText());
+}
+
+// This test verifies that the option code is not mandatory, if the option
+// name has been specified.
+TEST_F(ParseConfigTest, optionDataNoCode) {
+    std::string config =
+        "{ \"option-data\": [ {"
+        "    \"space\": \"dhcp6\","
+        "    \"name\": \"dns-servers\","
+        "    \"csv-format\": True,"
+        "    \"data\": \"2001:db8:1::1\""
+        " } ]"
+        "}";
+    int rcode = 0;
+    ASSERT_NO_THROW(rcode = parseConfiguration(config));
+    EXPECT_EQ(0, rcode);
+    Option6AddrLstPtr opt = boost::dynamic_pointer_cast<
+        Option6AddrLst>(getOptionPtr("dhcp6", 23));
+    ASSERT_TRUE(opt);
+    ASSERT_EQ(1, opt->getAddresses().size());
+    EXPECT_EQ( "2001:db8:1::1", opt->getAddresses()[0].toText());
+}
+
+// This test verifies that the option data configuration with a minimal
+// set of parameters works as expected.
+TEST_F(ParseConfigTest, optionDataMinimal) {
+    std::string config =
+        "{ \"option-data\": [ {"
+        "    \"name\": \"dns-servers\","
+        "    \"data\": \"2001:db8:1::10\""
+        " } ]"
+        "}";
+    int rcode = 0;
+    ASSERT_NO_THROW(rcode = parseConfiguration(config));
+    EXPECT_EQ(0, rcode);
+    Option6AddrLstPtr opt = boost::dynamic_pointer_cast<
+        Option6AddrLst>(getOptionPtr("dhcp6", 23));
+    ASSERT_TRUE(opt);
+    ASSERT_EQ(1, opt->getAddresses().size());
+    EXPECT_EQ( "2001:db8:1::10", opt->getAddresses()[0].toText());
+
+    CfgMgr::instance().clear();
+    // This time using an option code.
+    config =
+        "{ \"option-data\": [ {"
+        "    \"code\": 23,"
+        "    \"data\": \"2001:db8:1::20\""
+        " } ]"
+        "}";
+    rcode = 0;
+    ASSERT_NO_THROW(rcode = parseConfiguration(config));
+    EXPECT_EQ(0, rcode);
+    opt = boost::dynamic_pointer_cast<Option6AddrLst>(getOptionPtr("dhcp6",
+                                                                   23));
+    ASSERT_TRUE(opt);
+    ASSERT_EQ(1, opt->getAddresses().size());
+    EXPECT_EQ( "2001:db8:1::20", opt->getAddresses()[0].toText());
+}
+
+
 };  // Anonymous namespace
 
 /// These tests check basic operation of the HooksLibrariesParser.