[#1373] addressed review

diff --git a/src/hooks/dhcp/flex_option/flex_option.cc b/src/hooks/dhcp/flex_option/flex_option.cc
index 83dcf34f56675b4036ed6795f4e371c057fb4d86..0ba0d4be54cc8624deb5ae341e80bed132a2398a 100644 (file)
--- a/src/hooks/dhcp/flex_option/flex_option.cc
+++ b/src/hooks/dhcp/flex_option/flex_option.cc
@@ -74,8 +74,9 @@ parseAction(ConstElementPtr option,
 namespace isc {
 namespace flex_option {
 
-FlexOptionImpl::OptionConfig::OptionConfig(uint16_t code)
-    : code_(code), action_(NONE), csv_format_(false) {
+FlexOptionImpl::OptionConfig::OptionConfig(uint16_t code,
+                                           OptionDefinitionPtr def)
+    : code_(code), def_(def), action_(NONE), csv_format_(false) {
 }
 
 FlexOptionImpl::OptionConfig::~OptionConfig() {
@@ -116,10 +117,20 @@ FlexOptionImpl::parseOptionConfig(ConstElementPtr option) {
     ConstElementPtr code_elem = option->get("code");
     ConstElementPtr name_elem = option->get("name");
     ConstElementPtr csv_format_elem = option->get("csv-format");
+    OptionDefinitionPtr def;
     if (!code_elem && !name_elem) {
         isc_throw(BadValue, "'code' or 'name' must be specified: "
                   << option->str());
     }
+    string space;
+    Option::Universe universe;
+    if (family == AF_INET) {
+        space = DHCP4_OPTION_SPACE;
+        universe = Option::V4;
+    } else {
+        space = DHCP6_OPTION_SPACE;
+        universe = Option::V6;
+    }
     uint16_t code;
     if (code_elem) {
         if (code_elem->getType() != Element::integer) {
@@ -151,6 +162,17 @@ FlexOptionImpl::parseOptionConfig(ConstElementPtr option) {
             }
         }
         code = static_cast<uint16_t>(value);
+        def = isc::dhcp::LibDHCP::getOptionDef(space, code);
+        if (!def) {
+            def = isc::dhcp::LibDHCP::getRuntimeOptionDef(space, code);
+        }
+        if (!def) {
+            def = isc::dhcp::LibDHCP::getLastResortOptionDef(space, code);
+        }
+        if (!def) {
+            isc_throw(BadValue, "no known option with code '" << code
+                      << "' in '" << space << "' space");
+        }
     }
     if (name_elem) {
         if (name_elem->getType() != Element::string) {
@@ -161,13 +183,7 @@ FlexOptionImpl::parseOptionConfig(ConstElementPtr option) {
         if (name.empty()) {
             isc_throw(BadValue, "'name' must not be empty");
         }
-        string space;
-        if (family == AF_INET) {
-            space = DHCP4_OPTION_SPACE;
-        } else {
-            space = DHCP6_OPTION_SPACE;
-        }
-        OptionDefinitionPtr def = LibDHCP::getOptionDef(space, name);
+        def = LibDHCP::getOptionDef(space, name);
         if (!def) {
             def = LibDHCP::getRuntimeOptionDef(space, name);
         }
@@ -199,14 +215,7 @@ FlexOptionImpl::parseOptionConfig(ConstElementPtr option) {
         csv_format = csv_format_elem->boolValue();
     }
 
-    Option::Universe universe;
-    if (family == AF_INET) {
-        universe = Option::V4;
-    } else {
-        universe = Option::V6;
-    }
-
-    OptionConfigPtr opt_cfg(new OptionConfig(code));
+    OptionConfigPtr opt_cfg(new OptionConfig(code, def));
 
     if (csv_format_elem) {
         opt_cfg->setCSVFormat(csv_format);

diff --git a/src/hooks/dhcp/flex_option/flex_option.dox b/src/hooks/dhcp/flex_option/flex_option.dox
index 90c2db886b53b5e2163710af74eb77a87cb4cf0e..249316f5512fe2454c68a5c48cc2179f4aa947bd 100644 (file)
--- a/src/hooks/dhcp/flex_option/flex_option.dox
+++ b/src/hooks/dhcp/flex_option/flex_option.dox
@@ -94,7 +94,9 @@ The sole parameter is a options list of options with:
    already exists in the response packet, the option is removed from the
    response. Only one action can be specified.
  - @b csv-format - Specifies the option format used for input. If not
-   specified, it will default to false (raw data).
+   specified, it will default to false (raw data). When enabled, the option
+   data will be parsed using csv format and packed according to the option
+   definition.
 
 Note for the rare options which can be empty this mechanism does not work.
 The proposed solution in this case is to use a client class to set the

diff --git a/src/hooks/dhcp/flex_option/flex_option.h b/src/hooks/dhcp/flex_option/flex_option.h
index dcfe6a36f61a959a4350689d6e4d3a03771f6207..fedfbbc1e8d83aae70c4f1d791044501a5b0a0ee 100644 (file)
--- a/src/hooks/dhcp/flex_option/flex_option.h
+++ b/src/hooks/dhcp/flex_option/flex_option.h
@@ -55,7 +55,7 @@ public:
         /// @brief Constructor.
         ///
         /// @param code the option code.
-        OptionConfig(uint16_t code);
+        OptionConfig(uint16_t code, isc::dhcp::OptionDefinitionPtr def);
 
         /// @brief Destructor.
         virtual ~OptionConfig();
@@ -67,6 +67,13 @@ public:
             return (code_);
         }
 
+        /// @brief Return option definition.
+        ///
+        /// @return option definition.
+        isc::dhcp::OptionDefinitionPtr getOptionDef() const {
+            return (def_);
+        }
+
         /// @brief Set action.
         ///
         /// @param action the action.
@@ -127,6 +134,9 @@ public:
         /// @brief The code.
         uint16_t code_;
 
+        /// @brief The option definition.
+        isc::dhcp::OptionDefinitionPtr def_;
+
         /// @brief The action.
         Action action_;
 
@@ -174,15 +184,12 @@ public:
     template <typename PktType>
     void process(isc::dhcp::Option::Universe universe,
                  PktType query, PktType response) {
-        std::string space = (universe == isc::dhcp::Option::V4 ?
-                             DHCP4_OPTION_SPACE : DHCP6_OPTION_SPACE);
-
         for (auto pair : getOptionConfigMap()) {
             const OptionConfigPtr& opt_cfg = pair.second;
             std::string value;
             isc::dhcp::OptionBuffer buffer;
             isc::dhcp::OptionPtr opt = response->getOption(opt_cfg->getCode());
-            isc::dhcp::OptionDefinitionPtr def;
+            isc::dhcp::OptionDefinitionPtr def = opt_cfg->getOptionDef();
             switch (opt_cfg->getAction()) {
             case NONE:
                 break;
@@ -196,18 +203,8 @@ public:
                 if (value.empty()) {
                     break;
                 }
-
+                // Check for cvs format.
                 if (opt_cfg->getCSVFormat()) {
-                    def = isc::dhcp::LibDHCP::getOptionDef(space, opt_cfg->getCode());
-
-                    if (!def) {
-                        def = isc::dhcp::LibDHCP::getRuntimeOptionDef(space, opt_cfg->getCode());
-                    }
-
-                    if (!def) {
-                        def = isc::dhcp::LibDHCP::getLastResortOptionDef(space, opt_cfg->getCode());
-                    }
-
                     if (!def) {
                         buffer.assign(value.begin(), value.end());
                         opt.reset(new isc::dhcp::Option(universe, opt_cfg->getCode(),
@@ -223,7 +220,6 @@ public:
                     opt.reset(new isc::dhcp::Option(universe, opt_cfg->getCode(),
                                                     buffer));
                 }
-
                 // Add the option.
                 response->addOption(opt);
                 logAction(ADD, opt_cfg->getCode(), value);
@@ -239,18 +235,8 @@ public:
                     response->delOption(opt_cfg->getCode());
                     opt = response->getOption(opt_cfg->getCode());
                 }
-
+                // Check for cvs format.
                 if (opt_cfg->getCSVFormat()) {
-                    def = isc::dhcp::LibDHCP::getOptionDef(space, opt_cfg->getCode());
-
-                    if (!def) {
-                        def = isc::dhcp::LibDHCP::getRuntimeOptionDef(space, opt_cfg->getCode());
-                    }
-
-                    if (!def) {
-                        def = isc::dhcp::LibDHCP::getLastResortOptionDef(space, opt_cfg->getCode());
-                    }
-
                     if (!def) {
                         buffer.assign(value.begin(), value.end());
                         opt.reset(new isc::dhcp::Option(universe, opt_cfg->getCode(),
@@ -266,7 +252,6 @@ public:
                     opt.reset(new isc::dhcp::Option(universe, opt_cfg->getCode(),
                                                     buffer));
                 }
-
                 // Add the option.
                 response->addOption(opt);
                 logAction(SUPERSEDE, opt_cfg->getCode(), value);

diff --git a/src/hooks/dhcp/flex_option/tests/flex_option_unittests.cc b/src/hooks/dhcp/flex_option/tests/flex_option_unittests.cc
index 368682c8b22079d573b4f46278d211f20c24c169..715fe711da920399724ffe767930ee8081710202 100644 (file)
--- a/src/hooks/dhcp/flex_option/tests/flex_option_unittests.cc
+++ b/src/hooks/dhcp/flex_option/tests/flex_option_unittests.cc
@@ -170,7 +170,7 @@ TEST_F(FlexOptionTest, optionConfigBadCode4) {
     EXPECT_NO_THROW(impl_->testConfigure(options));
     EXPECT_TRUE(impl_->getErrMsg().empty());
 
-    code = Element::create(254);
+    code = Element::create(2);
     option->set("code", code);
     EXPECT_NO_THROW(impl_->testConfigure(options));
     EXPECT_TRUE(impl_->getErrMsg().empty());
@@ -210,7 +210,7 @@ TEST_F(FlexOptionTest, optionConfigBadCode6) {
     EXPECT_NO_THROW(impl_->testConfigure(options));
     EXPECT_TRUE(impl_->getErrMsg().empty());
 
-    code = Element::create(65535);
+    code = Element::create(2);
     option->set("code", code);
     EXPECT_NO_THROW(impl_->testConfigure(options));
     EXPECT_TRUE(impl_->getErrMsg().empty());
@@ -250,11 +250,24 @@ TEST_F(FlexOptionTest, optionConfigUnknownName) {
     ElementPtr add = Element::create(string("'ab'"));
     option->set("add", add);
     ElementPtr name = Element::create(string("foobar"));
-    option->set("name",name);
+    option->set("name", name);
     EXPECT_THROW(impl_->testConfigure(options), BadValue);
     EXPECT_EQ("no known 'foobar' option in 'dhcp4' space", impl_->getErrMsg());
 }
 
+// Verify that the code must be a known option.
+TEST_F(FlexOptionTest, optionConfigUnknownCode) {
+    ElementPtr options = Element::createList();
+    ElementPtr option = Element::createMap();
+    options->add(option);
+    ElementPtr add = Element::create(string("'ab'"));
+    option->set("add", add);
+    ElementPtr code = Element::create(109);
+    option->set("code", code);
+    EXPECT_THROW(impl_->testConfigure(options), BadValue);
+    EXPECT_EQ("no known option with code '109' in 'dhcp4' space", impl_->getErrMsg());
+}
+
 // Verify that the name can be a standard option.
 TEST_F(FlexOptionTest, optionConfigStandardName) {
     ElementPtr options = Element::createList();
@@ -751,8 +764,9 @@ TEST_F(FlexOptionTest, processEmpty) {
 TEST_F(FlexOptionTest, processNone) {
     CfgMgr::instance().setFamily(AF_INET6);
 
+    OptionDefinitionPtr def = LibDHCP::getOptionDef(DHCP6_OPTION_SPACE, D6O_BOOTFILE_URL);
     FlexOptionImpl::OptionConfigPtr
-        opt_cfg(new FlexOptionImpl::OptionConfig(D6O_BOOTFILE_URL));
+        opt_cfg(new FlexOptionImpl::OptionConfig(D6O_BOOTFILE_URL, def));
     EXPECT_EQ(FlexOptionImpl::NONE, opt_cfg->getAction());
     auto map = impl_->getMutableOptionConfigMap();
     map[DHO_HOST_NAME] = opt_cfg;
@@ -769,7 +783,6 @@ TEST_F(FlexOptionTest, processNone) {
 // Verify that ADD action adds the specified option in csv format.
 TEST_F(FlexOptionTest, processAddEnableCSVFormat) {
     ElementPtr options = Element::createList();
-    ElementPtr csv_format = Element::create(true);
     ElementPtr option = Element::createMap();
     options->add(option);
     ElementPtr code = Element::create(DHO_HOST_NAME);
@@ -783,7 +796,8 @@ TEST_F(FlexOptionTest, processAddEnableCSVFormat) {
     option->set("code", code);
     add = Element::create(string("'example.com'"));
     option->set("add", add);
-    option->set("csv-format", csv_format);
+    // fqdn option data is parsed using option definition in csv format.
+    option->set("csv-format", Element::create(true));
 
     EXPECT_NO_THROW(impl_->testConfigure(options));
     EXPECT_TRUE(impl_->getErrMsg().empty());
@@ -817,7 +831,6 @@ TEST_F(FlexOptionTest, processAddEnableCSVFormat) {
 // Verify that ADD action adds the specified option in raw format.
 TEST_F(FlexOptionTest, processAddDisableCSVFormat) {
     ElementPtr options = Element::createList();
-    ElementPtr csv_format = Element::create(false);
     ElementPtr option = Element::createMap();
     options->add(option);
     ElementPtr code = Element::create(DHO_HOST_NAME);
@@ -831,7 +844,8 @@ TEST_F(FlexOptionTest, processAddDisableCSVFormat) {
     option->set("code", code);
     add = Element::create(string("0x076578616d706c6503636f6d00"));
     option->set("add", add);
-    option->set("csv-format", csv_format);
+    // fqdn option data is specified in raw format.
+    option->set("csv-format", Element::create(false));
 
     EXPECT_NO_THROW(impl_->testConfigure(options));
     EXPECT_TRUE(impl_->getErrMsg().empty());
@@ -915,7 +929,6 @@ TEST_F(FlexOptionTest, processAddEmpty) {
 // Verify that SUPERSEDE action supersedes the specified option in csv format.
 TEST_F(FlexOptionTest, processSupersedeEnableCSVFormat) {
     ElementPtr options = Element::createList();
-    ElementPtr csv_format = Element::create(true);
     ElementPtr option = Element::createMap();
     options->add(option);
     ElementPtr code = Element::create(DHO_HOST_NAME);
@@ -929,7 +942,8 @@ TEST_F(FlexOptionTest, processSupersedeEnableCSVFormat) {
     option->set("code", code);
     supersede = Element::create(string("'example.com'"));
     option->set("supersede", supersede);
-    option->set("csv-format", csv_format);
+    // fqdn option data is parsed using option definition in csv format.
+    option->set("csv-format", Element::create(true));
 
     EXPECT_NO_THROW(impl_->testConfigure(options));
     EXPECT_TRUE(impl_->getErrMsg().empty());
@@ -963,7 +977,6 @@ TEST_F(FlexOptionTest, processSupersedeEnableCSVFormat) {
 // Verify that SUPERSEDE action supersedes the specified option in raw format.
 TEST_F(FlexOptionTest, processSupersedeDisableCSVFormat) {
     ElementPtr options = Element::createList();
-    ElementPtr csv_format = Element::create(false);
     ElementPtr option = Element::createMap();
     options->add(option);
     ElementPtr code = Element::create(DHO_HOST_NAME);
@@ -977,7 +990,8 @@ TEST_F(FlexOptionTest, processSupersedeDisableCSVFormat) {
     option->set("code", code);
     supersede = Element::create(string("0x076578616d706c6503636f6d00"));
     option->set("supersede", supersede);
-    option->set("csv-format", csv_format);
+    // fqdn option data is specified in raw format.
+    option->set("csv-format", Element::create(false));
 
     EXPECT_NO_THROW(impl_->testConfigure(options));
     EXPECT_TRUE(impl_->getErrMsg().empty());
@@ -1013,7 +1027,6 @@ TEST_F(FlexOptionTest, processSupersedeExisting) {
     CfgMgr::instance().setFamily(AF_INET6);
 
     ElementPtr options = Element::createList();
-    ElementPtr csv_format = Element::create(true);
     ElementPtr option = Element::createMap();
     options->add(option);
     ElementPtr code = Element::create(D6O_BOOTFILE_URL);
@@ -1027,7 +1040,8 @@ TEST_F(FlexOptionTest, processSupersedeExisting) {
     option->set("code", code);
     supersede = Element::create(string("'example.com'"));
     option->set("supersede", supersede);
-    option->set("csv-format", csv_format);
+    // fqdn option data is parsed using option definition in csv format.
+    option->set("csv-format", Element::create(true));
 
     EXPECT_NO_THROW(impl_->testConfigure(options));
     EXPECT_TRUE(impl_->getErrMsg().empty());
@@ -1205,7 +1219,6 @@ TEST_F(FlexOptionTest, processFullTest) {
 // Verify that complex strings with escaped characters are properly parsed on add.
 TEST_F(FlexOptionTest, processFullAddWithComplexString) {
     ElementPtr options = Element::createList();
-    ElementPtr csv_format = Element::create(true);
     ElementPtr option = Element::createMap();
     options->add(option);
     ElementPtr code = Element::create(D6O_NEW_POSIX_TIMEZONE);
@@ -1213,7 +1226,8 @@ TEST_F(FlexOptionTest, processFullAddWithComplexString) {
     string expr = "ifelse(option[39].exists,'EST5EDT4\\,M3.2.0/02:00\\,M11.1.0/02:00','')";
     ElementPtr add = Element::create(expr);
     option->set("add", add);
-    option->set("csv-format", csv_format);
+    // strings with escape characters are parsed in csv format.
+    option->set("csv-format", Element::create(true));
     EXPECT_NO_THROW(impl_->testConfigure(options));
     EXPECT_TRUE(impl_->getErrMsg().empty());
 
@@ -1238,7 +1252,6 @@ TEST_F(FlexOptionTest, processFullAddWithComplexString) {
 // Verify that complex strings with escaped characters are properly parsed on supersede.
 TEST_F(FlexOptionTest, processFullSupersedeWithComplexString) {
     ElementPtr options = Element::createList();
-    ElementPtr csv_format = Element::create(true);
     ElementPtr option = Element::createMap();
     options->add(option);
     ElementPtr code = Element::create(D6O_NEW_POSIX_TIMEZONE);
@@ -1246,7 +1259,8 @@ TEST_F(FlexOptionTest, processFullSupersedeWithComplexString) {
     string expr = "ifelse(option[39].exists,'EST5EDT4\\,M3.2.0/02:00\\,M11.1.0/02:00','')";
     ElementPtr supersede = Element::create(expr);
     option->set("supersede", supersede);
-    option->set("csv-format", csv_format);
+    // strings with escape characters are parsed in csv format.
+    option->set("csv-format", Element::create(true));
     EXPECT_NO_THROW(impl_->testConfigure(options));
     EXPECT_TRUE(impl_->getErrMsg().empty());