Parcourir la source

[master] Merge branch 'trac3409'

Conflicts:
	src/lib/dhcpsrv/d2_client_cfg.cc
	src/lib/dhcpsrv/dhcp_parsers.cc
Marcin Siodelski il y a 11 ans
Parent
commit
777dbdb29a

+ 1 - 0
configure.ac

@@ -1466,6 +1466,7 @@ AC_CONFIG_FILES([compatcheck/Makefile
                  src/lib/dhcp_ddns/tests/Makefile
                  src/lib/dhcp/Makefile
                  src/lib/dhcpsrv/Makefile
+                 src/lib/dhcpsrv/testutils/Makefile
                  src/lib/dhcpsrv/tests/Makefile
                  src/lib/dhcpsrv/tests/test_libraries.h
                  src/lib/dhcp/tests/Makefile

+ 48 - 19
src/bin/dhcp4/config_parser.cc

@@ -167,9 +167,13 @@ public:
         :SubnetConfigParser("", globalContext(), IOAddress("0.0.0.0")) {
     }
 
-    /// @brief Adds the created subnet to a server's configuration.
-    /// @throw throws Unexpected if dynamic cast fails.
-    void commit() {
+    /// @brief Parses a single IPv4 subnet configuration and adds to the
+    /// Configuration Manager.
+    ///
+    /// @param subnet A new subnet being configured.
+    void build(ConstElementPtr subnet) {
+        SubnetConfigParser::build(subnet);
+
         if (subnet_) {
             Subnet4Ptr sub4ptr = boost::dynamic_pointer_cast<Subnet4>(subnet_);
             if (!sub4ptr) {
@@ -183,10 +187,24 @@ public:
                 sub4ptr->setRelayInfo(*relay_info_);
             }
 
-            isc::dhcp::CfgMgr::instance().addSubnet4(sub4ptr);
+            // Adding a subnet to the Configuration Manager may fail if the
+            // subnet id is invalid (duplicate). Thus, we catch exceptions
+            // here to append a position in the configuration string.
+            try {
+                isc::dhcp::CfgMgr::instance().addSubnet4(sub4ptr);
+            } catch (const std::exception& ex) {
+                isc_throw(DhcpConfigError, ex.what() << " ("
+                          << subnet->getPosition() << ")");
+            }
         }
     }
 
+    /// @brief Commits subnet configuration.
+    ///
+    /// This function is currently no-op because subnet should already
+    /// be added into the Config Manager in the build().
+    void commit() { }
+
 protected:
 
     /// @brief Creates parsers for entries in subnet definition.
@@ -218,8 +236,7 @@ protected:
                                              global_context_,
                                              Dhcp4OptionDataParser::factory);
         } else {
-            isc_throw(NotImplemented,
-                "parser error: Subnet4 parameter not supported: " << config_id);
+            isc_throw(NotImplemented, "unsupported parameter: " << config_id);
         }
 
         return (parser);
@@ -294,6 +311,10 @@ protected:
             }
         } catch (const DhcpConfigError&) {
             // Don't care. next_server is optional. We can live without it
+        } catch (...) {
+            isc_throw(DhcpConfigError, "invalid parameter next-server ("
+                      << globalContext()->string_values_->getPosition("next-server")
+                      << ")");
         }
 
         // Try subnet specific value if it's available
@@ -304,8 +325,13 @@ protected:
             }
         } catch (const DhcpConfigError&) {
             // Don't care. next_server is optional. We can live without it
+        } catch (...) {
+            isc_throw(DhcpConfigError, "invalid parameter next-server ("
+                      << string_values_->getPosition("next-server")
+                      << ")");
         }
 
+
         // Try setting up client class (if specified)
         try {
             string client_class = string_values_->getParam("client-class");
@@ -338,6 +364,12 @@ public:
     ///
     /// @param subnets_list pointer to a list of IPv4 subnets
     void build(ConstElementPtr subnets_list) {
+        // @todo: Implement more subtle reconfiguration than toss
+        // the old one and replace with the new one.
+
+        // remove old subnets
+        CfgMgr::instance().deleteSubnets4();
+
         BOOST_FOREACH(ConstElementPtr subnet, subnets_list->listValue()) {
             ParserPtr parser(new Subnet4ConfigParser("subnet"));
             parser->build(subnet);
@@ -350,12 +382,6 @@ public:
     /// Iterates over all Subnet4 parsers. Each parser contains definitions of
     /// a single subnet and its parameters and commits each subnet separately.
     void commit() {
-        // @todo: Implement more subtle reconfiguration than toss
-        // the old one and replace with the new one.
-
-        // remove old subnets
-        CfgMgr::instance().deleteSubnets4();
-
         BOOST_FOREACH(ParserPtr subnet, subnets_) {
             subnet->commit();
         }
@@ -388,7 +414,8 @@ namespace dhcp {
 /// @return parser for specified global DHCPv4 parameter
 /// @throw NotImplemented if trying to create a parser for unknown
 /// config element
-DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) {
+    DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id,
+                                                    ConstElementPtr element) {
     DhcpConfigParser* parser = NULL;
     if ((config_id.compare("valid-lifetime") == 0)  ||
         (config_id.compare("renew-timer") == 0)  ||
@@ -405,8 +432,7 @@ DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) {
                                           globalContext(),
                                           Dhcp4OptionDataParser::factory);
     } else if (config_id.compare("option-def") == 0) {
-        parser  = new OptionDefListParser(config_id,
-                                          globalContext()->option_defs_);
+        parser  = new OptionDefListParser(config_id, globalContext());
     } else if ((config_id.compare("version") == 0) ||
                (config_id.compare("next-server") == 0)) {
         parser  = new StringParser(config_id,
@@ -420,9 +446,9 @@ DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) {
     } else if (config_id.compare("dhcp-ddns") == 0) {
         parser = new D2ClientConfigParser(config_id);
     } else {
-        isc_throw(NotImplemented,
-                "Parser error: Global configuration parameter not supported: "
-                << config_id);
+        isc_throw(DhcpConfigError,
+                "unsupported global configuration parameter: "
+                  << config_id << " (" << element->getPosition() << ")");
     }
 
     return (parser);
@@ -500,7 +526,8 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
         const std::map<std::string, ConstElementPtr>& values_map =
                                                         config_set->mapValue();
         BOOST_FOREACH(config_pair, values_map) {
-            ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first));
+            ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first,
+                                                           config_pair.second));
             LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PARSER_CREATED)
                       .arg(config_pair.first);
             if (config_pair.first == "subnet4") {
@@ -534,6 +561,7 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
         std::map<std::string, ConstElementPtr>::const_iterator option_config =
             values_map.find("option-data");
         if (option_config != values_map.end()) {
+            config_pair.first = "option-data";
             option_parser->build(option_config->second);
             option_parser->commit();
         }
@@ -542,6 +570,7 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
         std::map<std::string, ConstElementPtr>::const_iterator subnet_config =
             values_map.find("subnet4");
         if (subnet_config != values_map.end()) {
+            config_pair.first = "subnet4";
             subnet_parser->build(subnet_config->second);
         }
 

+ 1 - 0
src/bin/dhcp4/tests/Makefile.am

@@ -106,6 +106,7 @@ dhcp4_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libkea-exceptions.la
 dhcp4_unittests_LDADD += $(top_builddir)/src/lib/log/libkea-log.la
 dhcp4_unittests_LDADD += $(top_builddir)/src/lib/util/libkea-util.la
 dhcp4_unittests_LDADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
+dhcp4_unittests_LDADD += $(top_builddir)/src/lib/dhcpsrv/testutils/libdhcpsrvtest.la
 endif
 
 noinst_PROGRAMS = $(TESTS)

+ 34 - 45
src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -27,6 +27,7 @@
 #include <dhcp/classify.h>
 #include <dhcpsrv/subnet.h>
 #include <dhcpsrv/cfgmgr.h>
+#include <dhcpsrv/testutils/config_result_check.h>
 #include <hooks/hooks_manager.h>
 
 #include "marker_file.h"
@@ -286,9 +287,8 @@ public:
         std::string config = createConfigWithOption(param_value, parameter);
         ElementPtr json = Element::fromJSON(config);
         EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
-        ASSERT_TRUE(x);
-        comment_ = parseAnswer(rcode_, x);
-        ASSERT_EQ(1, rcode_);
+        checkResult(x, 1);
+        EXPECT_TRUE(errorContainsPosition(x, "<string>"));
     }
 
     /// @brief Test invalid option paramater value.
@@ -304,9 +304,8 @@ public:
         std::string config = createConfigWithOption(params);
         ElementPtr json = Element::fromJSON(config);
         EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
-        ASSERT_TRUE(x);
-        comment_ = parseAnswer(rcode_, x);
-        ASSERT_EQ(1, rcode_);
+        checkResult(x, 1);
+        EXPECT_TRUE(errorContainsPosition(x, "<string>"));
     }
 
     /// @brief Test option against given code and data.
@@ -580,9 +579,7 @@ TEST_F(Dhcp4ParserTest, multipleSubnets) {
 
     do {
         EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
-        ASSERT_TRUE(x);
-        comment_ = parseAnswer(rcode_, x);
-        ASSERT_EQ(0, rcode_);
+        checkResult(x, 0);
 
         const Subnet4Collection* subnets = CfgMgr::instance().getSubnets4();
         ASSERT_TRUE(subnets);
@@ -635,9 +632,7 @@ TEST_F(Dhcp4ParserTest, multipleSubnetsExplicitIDs) {
     int cnt = 0; // Number of reconfigurations
     do {
         EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
-        ASSERT_TRUE(x);
-        comment_ = parseAnswer(rcode_, x);
-        ASSERT_EQ(0, rcode_);
+        checkResult(x, 0);
 
         const Subnet4Collection* subnets = CfgMgr::instance().getSubnets4();
         ASSERT_TRUE(subnets);
@@ -686,9 +681,8 @@ TEST_F(Dhcp4ParserTest, multipleSubnetsOverlapingIDs) {
     ElementPtr json = Element::fromJSON(config);
 
     EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    EXPECT_NE(rcode_, 0);
+    checkResult(x, 1);
+    EXPECT_TRUE(errorContainsPosition(x, "<string>"));
 }
 
 // Goal of this test is to verify that a previously configured subnet can be
@@ -769,9 +763,7 @@ TEST_F(Dhcp4ParserTest, reconfigureRemoveSubnet) {
 
     ElementPtr json = Element::fromJSON(config4);
     EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     const Subnet4Collection* subnets = CfgMgr::instance().getSubnets4();
     ASSERT_TRUE(subnets);
@@ -780,9 +772,7 @@ TEST_F(Dhcp4ParserTest, reconfigureRemoveSubnet) {
     // Do the reconfiguration (the last subnet is removed)
     json = Element::fromJSON(config_first3);
     EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     subnets = CfgMgr::instance().getSubnets4();
     ASSERT_TRUE(subnets);
@@ -799,16 +789,12 @@ TEST_F(Dhcp4ParserTest, reconfigureRemoveSubnet) {
     /// @todo: Uncomment subnet removal test as part of #3281.
     json = Element::fromJSON(config4);
     EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     // Do reconfiguration
     json = Element::fromJSON(config_second_removed);
     EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     subnets = CfgMgr::instance().getSubnets4();
     ASSERT_TRUE(subnets);
@@ -932,12 +918,15 @@ TEST_F(Dhcp4ParserTest, nextServerNegative) {
     // check if returned status is always a failure
     EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json1));
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 
     EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json2));
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 
     EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json3));
     checkResult(status, 0);
+    EXPECT_FALSE(errorContainsPosition(status, "<string>"));
 }
 
 // Checks if the next-server defined as global value is overridden by subnet
@@ -1065,6 +1054,7 @@ TEST_F(Dhcp4ParserTest, poolOutOfSubnet) {
     // returned value must be 1 (values error)
     // as the pool does not belong to that subnet
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 // Goal of this test is to verify if pools can be defined
@@ -1284,6 +1274,7 @@ TEST_F(Dhcp4ParserTest, optionDefDuplicate) {
     EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json));
     ASSERT_TRUE(status);
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 // The goal of this test is to verify that the option definition
@@ -1392,6 +1383,7 @@ TEST_F(Dhcp4ParserTest, optionDefInvalidName) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 /// The purpose of this test is to verify that the option definition
@@ -1418,6 +1410,7 @@ TEST_F(Dhcp4ParserTest, optionDefInvalidType) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 /// The purpose of this test is to verify that the option definition
@@ -1444,6 +1437,7 @@ TEST_F(Dhcp4ParserTest, optionDefInvalidRecordType) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 /// The goal of this test is to verify that the invalid encapsulated
@@ -1470,6 +1464,7 @@ TEST_F(Dhcp4ParserTest, optionDefInvalidEncapsulatedSpace) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 /// The goal of this test is to verify that the encapsulated
@@ -1498,6 +1493,7 @@ TEST_F(Dhcp4ParserTest, optionDefEncapsulatedSpaceAndArray) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 /// The goal of this test is to verify that the option may not
@@ -1524,6 +1520,7 @@ TEST_F(Dhcp4ParserTest, optionDefEncapsulateOwnSpace) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 /// The purpose of this test is to verify that it is not allowed
@@ -1588,6 +1585,7 @@ TEST_F(Dhcp4ParserTest, optionStandardDefOverride) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 
     /// @todo The option 65 is a standard DHCPv4 option. However, at this point
     /// there is no definition for this option in libdhcp++, so it should be
@@ -1655,9 +1653,7 @@ TEST_F(Dhcp4ParserTest, optionDataDefaults) {
     ElementPtr json = Element::fromJSON(config);
 
     EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(IOAddress("192.0.2.200"),
                                                       classify_);
@@ -1960,9 +1956,7 @@ TEST_F(Dhcp4ParserTest, optionDataInSingleSubnet) {
     ElementPtr json = Element::fromJSON(config);
 
     EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(IOAddress("192.0.2.24"),
                                                       classify_);
@@ -2113,9 +2107,7 @@ TEST_F(Dhcp4ParserTest, optionDataInMultipleSubnets) {
     ElementPtr json = Element::fromJSON(config);
 
     EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     Subnet4Ptr subnet1 = CfgMgr::instance().getSubnet4(IOAddress("192.0.2.100"),
                                                        classify_);
@@ -2218,9 +2210,7 @@ TEST_F(Dhcp4ParserTest, optionDataLowerCase) {
     ElementPtr json = Element::fromJSON(config);
 
     EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(IOAddress("192.0.2.5"),
                                                       classify_);
@@ -2263,9 +2253,7 @@ TEST_F(Dhcp4ParserTest, stdOptionData) {
     ElementPtr json = Element::fromJSON(config);
 
     EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(IOAddress("192.0.2.5"),
                                                       classify_);
@@ -2343,6 +2331,7 @@ TEST_F(Dhcp4ParserTest, DISABLED_Uint32Parser) {
 
     // returned value must be rejected (1 configuration error)
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 
     // CASE 4: -1 (UINT_MIN -1 ) should not work
     EXPECT_NO_THROW(status = configureDhcp4Server(*srv_,
@@ -2351,6 +2340,7 @@ TEST_F(Dhcp4ParserTest, DISABLED_Uint32Parser) {
 
     // returned value must be rejected (1 configuration error)
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 // The goal of this test is to verify that the standard option can
@@ -2941,6 +2931,7 @@ TEST_F(Dhcp4ParserTest, invalidD2ClientConfig) {
 
     // check if returned status is failed.
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 
     // Verify that the D2 configuraiton can be fetched and is set to disabled.
     D2ClientConfigPtr d2_client_config = CfgMgr::instance().getD2ClientConfig();
@@ -3014,9 +3005,7 @@ TEST_F(Dhcp4ParserTest, classifySubnets) {
     ElementPtr json = Element::fromJSON(config);
 
     EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     const Subnet4Collection* subnets = CfgMgr::instance().getSubnets4();
     ASSERT_TRUE(subnets);

+ 59 - 35
src/bin/dhcp6/config_parser.cc

@@ -231,30 +231,33 @@ public:
                                                              uint32_values_));
                 parser = code_parser;
             } else {
-                isc_throw(DhcpConfigError, "invalid parameter: " << entry);
+                isc_throw(DhcpConfigError, "unsupported parameter: " << entry
+                          << " (" << param.second->getPosition() << ")");
             }
 
             parser->build(param.second);
             parser->commit();
         }
 
+        // Try to obtain the pool parameters. It will throw an exception if any
+        // of the required parameters are not present or invalid.
+        std::string addr_str;
+        uint32_t prefix_len;
+        uint32_t delegated_len;
         try {
-            // We should now have all of the pool elements we need to create
-            // the pool.  Fetch them and pass them into the Pool6 constructor.
-            // The constructor is expected to enforce any value validation.
-            const std::string addr_str = string_values_->getParam("prefix");
-            IOAddress addr(addr_str);
-
-            uint32_t prefix_len = uint32_values_->getParam("prefix-len");
-
-            uint32_t delegated_len = uint32_values_->getParam("delegated-len");
+            addr_str = string_values_->getParam("prefix");
+            prefix_len = uint32_values_->getParam("prefix-len");
+            delegated_len = uint32_values_->getParam("delegated-len");
 
             // Attempt to construct the local pool.
-            pool_.reset(new Pool6(Lease::TYPE_PD, addr, prefix_len,
-                                 delegated_len));
+            pool_.reset(new Pool6(Lease::TYPE_PD, IOAddress(addr_str),
+                                  prefix_len, delegated_len));
         } catch (const std::exception& ex) {
-            isc_throw(isc::dhcp::DhcpConfigError,
-                      "PdPoolParser failed to build pool: " << ex.what());
+            // Some parameters don't exist or are invalid. Since we are not
+            // aware whether they don't exist or are invalid, let's append
+            // the position of the pool map element.
+            isc_throw(isc::dhcp::DhcpConfigError, ex.what()
+                      << " (" << pd_pool_->getPosition() << ")");
         }
     }
 
@@ -319,7 +322,7 @@ public:
         // Make sure we have a configuration elements to parse.
         if (!pd_pool_list) {
             isc_throw(DhcpConfigError,
-                      "PdPoolListParser: list of pool definitions is empty");
+                      "PdPoolListParser: list of pool definitions is NULL");
         }
 
         // Loop through the list of pd pools.
@@ -371,15 +374,19 @@ public:
         :SubnetConfigParser("", globalContext(), IOAddress("::")) {
     }
 
-    /// @brief Adds the created subnet to a server's configuration.
-    /// @throw throws Unexpected if dynamic cast fails.
-    void commit() {
+    /// @brief Parses a single IPv4 subnet configuration and adds to the
+    /// Configuration Manager.
+    ///
+    /// @param subnet A new subnet being configured.
+    void build(ConstElementPtr subnet) {
+        SubnetConfigParser::build(subnet);
+
         if (subnet_) {
             Subnet6Ptr sub6ptr = boost::dynamic_pointer_cast<Subnet6>(subnet_);
             if (!sub6ptr) {
                 // If we hit this, it is a programming error.
                 isc_throw(Unexpected,
-                          "Invalid cast in Subnet4ConfigParser::commit");
+                          "Invalid cast in Subnet6ConfigParser::commit");
             }
 
             // Set relay infomation if it was provided
@@ -387,10 +394,25 @@ public:
                 sub6ptr->setRelayInfo(*relay_info_);
             }
 
-            isc::dhcp::CfgMgr::instance().addSubnet6(sub6ptr);
+            // Adding a subnet to the Configuration Manager may fail if the
+            // subnet id is invalid (duplicate). Thus, we catch exceptions
+            // here to append a position in the configuration string.
+            try {
+                isc::dhcp::CfgMgr::instance().addSubnet6(sub6ptr);
+            } catch (const std::exception& ex) {
+                isc_throw(DhcpConfigError, ex.what() << " ("
+                          << subnet->getPosition() << ")");
+            }
+
         }
     }
 
+    /// @brief Commits subnet configuration.
+    ///
+    /// This function is currently no-op because subnet should already
+    /// be added into the Config Manager in the build().
+    void commit() { }
+
 protected:
 
     /// @brief creates parsers for entries in subnet definition
@@ -425,8 +447,7 @@ protected:
                                              global_context_,
                                              Dhcp6OptionDataParser::factory);
         } else {
-            isc_throw(NotImplemented,
-                "parser error: Subnet6 parameter not supported: " << config_id);
+            isc_throw(NotImplemented, "unsupported parameter: " << config_id);
         }
 
         return (parser);
@@ -569,6 +590,12 @@ public:
     ///
     /// @param subnets_list pointer to a list of IPv6 subnets
     void build(ConstElementPtr subnets_list) {
+        // @todo: Implement more subtle reconfiguration than toss
+        // the old one and replace with the new one.
+
+        // remove old subnets
+        isc::dhcp::CfgMgr::instance().deleteSubnets6();
+
         BOOST_FOREACH(ConstElementPtr subnet, subnets_list->listValue()) {
             ParserPtr parser(new Subnet6ConfigParser("subnet"));
             parser->build(subnet);
@@ -582,12 +609,6 @@ public:
     /// Iterates over all Subnet6 parsers. Each parser contains definitions of
     /// a single subnet and its parameters and commits each subnet separately.
     void commit() {
-        // @todo: Implement more subtle reconfiguration than toss
-        // the old one and replace with the new one.
-
-        // remove old subnets
-        isc::dhcp::CfgMgr::instance().deleteSubnets6();
-
         BOOST_FOREACH(ParserPtr subnet, subnets_) {
             subnet->commit();
         }
@@ -619,7 +640,8 @@ namespace dhcp {
 /// @return parser for specified global DHCPv6 parameter
 /// @throw NotImplemented if trying to create a parser for unknown config
 /// element
-DhcpConfigParser* createGlobal6DhcpConfigParser(const std::string& config_id) {
+    DhcpConfigParser* createGlobal6DhcpConfigParser(const std::string& config_id,
+                                                    ConstElementPtr element) {
     DhcpConfigParser* parser = NULL;
     if ((config_id.compare("preferred-lifetime") == 0)  ||
         (config_id.compare("valid-lifetime") == 0)  ||
@@ -637,8 +659,7 @@ DhcpConfigParser* createGlobal6DhcpConfigParser(const std::string& config_id) {
                                           globalContext(),
                                           Dhcp6OptionDataParser::factory);
     } else if (config_id.compare("option-def") == 0) {
-        parser  = new OptionDefListParser(config_id,
-                                          globalContext()->option_defs_);
+        parser  = new OptionDefListParser(config_id, globalContext());
     } else if (config_id.compare("version") == 0) {
         parser  = new StringParser(config_id,
                                    globalContext()->string_values_);
@@ -649,9 +670,9 @@ DhcpConfigParser* createGlobal6DhcpConfigParser(const std::string& config_id) {
     } else if (config_id.compare("dhcp-ddns") == 0) {
         parser = new D2ClientConfigParser(config_id);
     } else {
-        isc_throw(NotImplemented,
-                "Parser error: Global configuration parameter not supported: "
-                << config_id);
+        isc_throw(DhcpConfigError,
+                "unsupported global configuration parameter: "
+                  << config_id << " (" << element->getPosition() << ")");
     }
 
     return (parser);
@@ -716,7 +737,8 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
         const std::map<std::string, ConstElementPtr>& values_map =
             config_set->mapValue();
         BOOST_FOREACH(config_pair, values_map) {
-            ParserPtr parser(createGlobal6DhcpConfigParser(config_pair.first));
+            ParserPtr parser(createGlobal6DhcpConfigParser(config_pair.first,
+                                                           config_pair.second));
             LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL, DHCP6_PARSER_CREATED)
                       .arg(config_pair.first);
             if (config_pair.first == "subnet6") {
@@ -751,6 +773,7 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
         std::map<std::string, ConstElementPtr>::const_iterator option_config =
             values_map.find("option-data");
         if (option_config != values_map.end()) {
+            config_pair.first = "option-data";
             option_parser->build(option_config->second);
             option_parser->commit();
         }
@@ -759,6 +782,7 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
         std::map<std::string, ConstElementPtr>::const_iterator subnet_config =
             values_map.find("subnet6");
         if (subnet_config != values_map.end()) {
+            config_pair.first = "subnet6";
             subnet_parser->build(subnet_config->second);
         }
 

+ 1 - 0
src/bin/dhcp6/tests/Makefile.am

@@ -101,6 +101,7 @@ dhcp6_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libkea-dhcp++.la
 dhcp6_unittests_LDADD += $(top_builddir)/src/lib/dhcp/tests/libdhcptest.la
 dhcp6_unittests_LDADD += $(top_builddir)/src/lib/dhcp_ddns/libkea-dhcp_ddns.la
 dhcp6_unittests_LDADD += $(top_builddir)/src/lib/dhcpsrv/libkea-dhcpsrv.la
+dhcp6_unittests_LDADD += $(top_builddir)/src/lib/dhcpsrv/testutils/libdhcpsrvtest.la
 dhcp6_unittests_LDADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
 dhcp6_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libkea-exceptions.la
 dhcp6_unittests_LDADD += $(top_builddir)/src/lib/log/libkea-log.la

+ 52 - 100
src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -25,6 +25,7 @@
 #include <dhcpsrv/addr_utilities.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcpsrv/subnet.h>
+#include <dhcpsrv/testutils/config_result_check.h>
 #include <hooks/hooks_manager.h>
 
 #include "test_data_files_config.h"
@@ -375,9 +376,8 @@ public:
         std::string config = createConfigWithOption(param_value, parameter);
         ElementPtr json = Element::fromJSON(config);
         EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-        ASSERT_TRUE(x);
-        comment_ = parseAnswer(rcode_, x);
-        ASSERT_EQ(1, rcode_);
+        checkResult(x, 1);
+        EXPECT_TRUE(errorContainsPosition(x, "<string>"));
     }
 
     /// @brief Test invalid option paramater value.
@@ -393,9 +393,8 @@ public:
         std::string config = createConfigWithOption(params);
         ElementPtr json = Element::fromJSON(config);
         EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-        ASSERT_TRUE(x);
-        comment_ = parseAnswer(rcode_, x);
-        ASSERT_EQ(1, rcode_);
+        checkResult(x, 1);
+        EXPECT_TRUE(errorContainsPosition(x, "<string>"));
     }
 
     /// @brief Test option against given code and data.
@@ -491,9 +490,7 @@ TEST_F(Dhcp6ParserTest, version) {
                     Element::fromJSON("{\"version\": 0}")));
 
     // returned value must be 0 (configuration accepted)
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    EXPECT_EQ(0, rcode_);
+    checkResult(x, 0);
 }
 
 /// The goal of this test is to verify that the code accepts only
@@ -506,9 +503,7 @@ TEST_F(Dhcp6ParserTest, bogusCommand) {
                     Element::fromJSON("{\"bogus\": 5}")));
 
     // returned value must be 1 (configuration parse error)
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    EXPECT_EQ(1, rcode_);
+    checkResult(x, 1);
 }
 
 /// The goal of this test is to verify if configuration without any
@@ -526,9 +521,7 @@ TEST_F(Dhcp6ParserTest, emptySubnet) {
                                       "\"valid-lifetime\": 4000 }")));
 
     // returned value should be 0 (success)
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(0, rcode_);
+    checkResult(status, 0);
 }
 
 /// The goal of this test is to verify if defined subnet uses global
@@ -551,9 +544,7 @@ TEST_F(Dhcp6ParserTest, subnetGlobalDefaults) {
     EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json));
 
     // check if returned status is OK
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(0, rcode_);
+    checkResult(status, 0);
 
     // Now check if the configuration was indeed handled and we have
     // expected pool configured.
@@ -602,9 +593,7 @@ TEST_F(Dhcp6ParserTest, multipleSubnets) {
 
     do {
         EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-        ASSERT_TRUE(x);
-        comment_ = parseAnswer(rcode_, x);
-        ASSERT_EQ(0, rcode_);
+        checkResult(x, 0);
 
         const Subnet6Collection* subnets = CfgMgr::instance().getSubnets6();
         ASSERT_TRUE(subnets);
@@ -659,9 +648,7 @@ TEST_F(Dhcp6ParserTest, multipleSubnetsExplicitIDs) {
 
     do {
         EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-        ASSERT_TRUE(x);
-        comment_ = parseAnswer(rcode_, x);
-        ASSERT_EQ(0, rcode_);
+        checkResult(x, 0);
 
         const Subnet6Collection* subnets = CfgMgr::instance().getSubnets6();
         ASSERT_TRUE(subnets);
@@ -711,9 +698,8 @@ TEST_F(Dhcp6ParserTest, multipleSubnetsOverlapingIDs) {
     ElementPtr json = Element::fromJSON(config);
 
     EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_NE(rcode_, 0);
+    checkResult(x, 1);
+    EXPECT_TRUE(errorContainsPosition(x, "<string>"));
 }
 
 
@@ -798,9 +784,7 @@ TEST_F(Dhcp6ParserTest, reconfigureRemoveSubnet) {
 
     ElementPtr json = Element::fromJSON(config4);
     EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     const Subnet6Collection* subnets = CfgMgr::instance().getSubnets6();
     ASSERT_TRUE(subnets);
@@ -809,9 +793,7 @@ TEST_F(Dhcp6ParserTest, reconfigureRemoveSubnet) {
     // Do the reconfiguration (the last subnet is removed)
     json = Element::fromJSON(config_first3);
     EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     subnets = CfgMgr::instance().getSubnets6();
     ASSERT_TRUE(subnets);
@@ -826,16 +808,12 @@ TEST_F(Dhcp6ParserTest, reconfigureRemoveSubnet) {
 
     json = Element::fromJSON(config4);
     EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     // Do reconfiguration
     json = Element::fromJSON(config_second_removed);
     EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     subnets = CfgMgr::instance().getSubnets6();
     ASSERT_TRUE(subnets);
@@ -873,9 +851,7 @@ TEST_F(Dhcp6ParserTest, subnetLocal) {
     EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json));
 
     // returned value should be 0 (configuration success)
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(0, rcode_);
+    checkResult(status, 0);
 
     Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(IOAddress("2001:db8:1::5"),
                                                       classify_);
@@ -910,9 +886,7 @@ TEST_F(Dhcp6ParserTest, subnetInterface) {
     EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json));
 
     // returned value should be 0 (configuration success)
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(0, rcode_);
+    checkResult(status, 0);
 
     Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(IOAddress("2001:db8:1::5"),
                                                       classify_);
@@ -944,9 +918,8 @@ TEST_F(Dhcp6ParserTest, subnetInterfaceBogus) {
     EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json));
 
     // returned value should be 1 (configuration error)
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(1, rcode_);
+    checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 
     Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(IOAddress("2001:db8:1::5"),
                                                       classify_);
@@ -976,9 +949,8 @@ TEST_F(Dhcp6ParserTest, interfaceGlobal) {
     EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json));
 
     // returned value should be 1 (parse error)
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(1, rcode_);
+    checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 
@@ -1007,9 +979,7 @@ TEST_F(Dhcp6ParserTest, subnetInterfaceId) {
     EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json));
 
     // Returned value should be 0 (configuration success)
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(0, rcode_);
+    checkResult(status, 0);
 
     // Try to get a subnet based on bogus interface-id option
     OptionBuffer tmp(bogus_interface_id.begin(), bogus_interface_id.end());
@@ -1046,9 +1016,8 @@ TEST_F(Dhcp6ParserTest, interfaceIdGlobal) {
     EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json));
 
     // Returned value should be 1 (parse error)
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(1, rcode_);
+    checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 // This test checks if it is not possible to define a subnet with an
@@ -1071,10 +1040,8 @@ TEST_F(Dhcp6ParserTest, subnetInterfaceAndInterfaceId) {
     EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json));
 
     // Returned value should be 1 (configuration error)
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(1, rcode_);
-
+    checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 
@@ -1101,9 +1068,8 @@ TEST_F(Dhcp6ParserTest, poolOutOfSubnet) {
 
     // returned value must be 1 (values error)
     // as the pool does not belong to that subnet
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(1, rcode_);
+    checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 // Goal of this test is to verify if pools can be defined
@@ -1129,9 +1095,7 @@ TEST_F(Dhcp6ParserTest, poolPrefixLen) {
     EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
 
     // returned value must be 1 (configuration parse error)
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    EXPECT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(IOAddress("2001:db8:1::5"),
                                                       classify_);
@@ -1172,9 +1136,7 @@ TEST_F(Dhcp6ParserTest, pdPoolBasics) {
     // Returned value must be non-empty ConstElementPtr to config result.
     // rcode should be 0 which indicates successful configuration processing.
     EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    EXPECT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     // Test that we can retrieve the subnet.
     Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(IOAddress("2001:db8:1::5"),
@@ -1246,9 +1208,7 @@ TEST_F(Dhcp6ParserTest, pdPoolList) {
     // Returned value must be non-empty ConstElementPtr to config result.
     // rcode should be 0 which indicates successful configuration processing.
     EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    EXPECT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     // Test that we can retrieve the subnet.
     Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(IOAddress("2001:db8:1::5"),
@@ -1304,9 +1264,7 @@ TEST_F(Dhcp6ParserTest, subnetAndPrefixDelegated) {
     // Returned value must be non-empty ConstElementPtr to config result.
     // rcode should be 0 which indicates successful configuration processing.
     EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    EXPECT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     // Test that we can retrieve the subnet.
     Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(IOAddress("2001:db8:1::5"),
@@ -1423,9 +1381,8 @@ TEST_F(Dhcp6ParserTest, invalidPdPools) {
 
         // Returned value must be non-empty ConstElementPtr to config result.
         // rcode should be 1 which indicates configuration error.
-        ASSERT_TRUE(x);
-        comment_ = parseAnswer(rcode_, x);
-        EXPECT_EQ(1, rcode_);
+        checkResult(x, 1);
+        EXPECT_TRUE(errorContainsPosition(x, "<string>"));
     }
 }
 
@@ -1611,6 +1568,7 @@ TEST_F(Dhcp6ParserTest, optionDefDuplicate) {
     EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json));
     ASSERT_TRUE(status);
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 // The goal of this test is to verify that the option definition
@@ -1718,6 +1676,7 @@ TEST_F(Dhcp6ParserTest, optionDefInvalidName) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 /// The purpose of this test is to verify that the option definition
@@ -1744,6 +1703,7 @@ TEST_F(Dhcp6ParserTest, optionDefInvalidType) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 /// The purpose of this test is to verify that the option definition
@@ -1770,6 +1730,7 @@ TEST_F(Dhcp6ParserTest, optionDefInvalidRecordType) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 /// The goal of this test is to verify that the invalid encapsulated
@@ -1796,6 +1757,7 @@ TEST_F(Dhcp6ParserTest, optionDefInvalidEncapsulatedSpace) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 /// The goal of this test is to verify that the encapsulated
@@ -1824,6 +1786,7 @@ TEST_F(Dhcp6ParserTest, optionDefEncapsulatedSpaceAndArray) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 /// The goal of this test is to verify that the option may not
@@ -1850,6 +1813,7 @@ TEST_F(Dhcp6ParserTest, optionDefEncapsulateOwnSpace) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 }
 
 /// The purpose of this test is to verify that it is not allowed
@@ -1915,6 +1879,7 @@ TEST_F(Dhcp6ParserTest, optionStandardDefOverride) {
     ASSERT_TRUE(status);
     // Expecting parsing error (error code 1).
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 
     /// @todo The option 59 is a standard DHCPv6 option. However, at this point
     /// there is no definition for this option in libdhcp++, so it should be
@@ -1982,9 +1947,7 @@ TEST_F(Dhcp6ParserTest, optionDataDefaults) {
     ElementPtr json = Element::fromJSON(config);
 
     EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(IOAddress("2001:db8:1::5"),
                                                       classify_);
@@ -2292,9 +2255,7 @@ TEST_F(Dhcp6ParserTest, optionDataInMultipleSubnets) {
     ElementPtr json = Element::fromJSON(config);
 
     EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     Subnet6Ptr subnet1 = CfgMgr::instance().getSubnet6(IOAddress("2001:db8:1::5"),
                                                        classify_);
@@ -2489,9 +2450,7 @@ TEST_F(Dhcp6ParserTest, optionDataLowerCase) {
     ElementPtr json = Element::fromJSON(config);
 
     EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(IOAddress("2001:db8:1::5"),
                                                       classify_);
@@ -2534,9 +2493,7 @@ TEST_F(Dhcp6ParserTest, stdOptionData) {
     ElementPtr json = Element::fromJSON(config);
 
     EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(IOAddress("2001:db8:1::5"),
                                                       classify_);
@@ -3025,9 +2982,7 @@ TEST_F(Dhcp6ParserTest, selectedInterfaces) {
 
     // returned value must be 1 (values error)
     // as the pool does not belong to that subnet
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(0, rcode_);
+    checkResult(status, 0);
 
     // eth0 and eth1 were explicitly selected. eth2 was not.
     EXPECT_TRUE(CfgMgr::instance().isActiveIface("eth0"));
@@ -3063,9 +3018,7 @@ TEST_F(Dhcp6ParserTest, allInterfaces) {
 
     // returned value must be 1 (values error)
     // as the pool does not belong to that subnet
-    ASSERT_TRUE(status);
-    comment_ = parseAnswer(rcode_, status);
-    EXPECT_EQ(0, rcode_);
+    checkResult(status, 0);
 
     // All interfaces should be now active.
     EXPECT_TRUE(CfgMgr::instance().isActiveIface("eth0"));
@@ -3137,9 +3090,7 @@ TEST_F(Dhcp6ParserTest, classifySubnets) {
     ElementPtr json = Element::fromJSON(config);
 
     EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
-    ASSERT_TRUE(x);
-    comment_ = parseAnswer(rcode_, x);
-    ASSERT_EQ(0, rcode_);
+    checkResult(x, 0);
 
     const Subnet6Collection* subnets = CfgMgr::instance().getSubnets6();
     ASSERT_TRUE(subnets);
@@ -3300,6 +3251,7 @@ TEST_F(Dhcp6ParserTest, invalidD2ClientConfig) {
 
     // check if returned status is failed.
     checkResult(status, 1);
+    EXPECT_TRUE(errorContainsPosition(status, "<string>"));
 
     // Verify that the D2 configuraiton can be fetched and is set to disabled.
     D2ClientConfigPtr d2_client_config = CfgMgr::instance().getD2ClientConfig();

+ 24 - 8
src/lib/cc/data.cc

@@ -42,6 +42,19 @@ namespace isc {
 namespace data {
 
 std::string
+Element::Position::str() const {
+    std::ostringstream ss;
+    ss << file_ << ":" << line_ << ":" << pos_;
+    return (ss.str());
+}
+
+std::ostream&
+operator<<(std::ostream& out, const Element::Position& pos) {
+    out << pos.str();
+    return (out);
+}
+
+std::string
 Element::str() const {
     std::stringstream ss;
     toJSON(ss);
@@ -425,7 +438,7 @@ fromStringstreamNumber(std::istream& in, const std::string& file,
     if (number.find_first_of(".eE") < number.size()) {
         try {
             return (Element::create(boost::lexical_cast<double>(number),
-                                    Element::Position(line, start_pos)));
+                                    Element::Position(file, line, start_pos)));
         } catch (const boost::bad_lexical_cast&) {
             throwJSONError(std::string("Number overflow: ") + number,
                            file, line, start_pos);
@@ -433,7 +446,7 @@ fromStringstreamNumber(std::istream& in, const std::string& file,
     } else {
         try {
             return (Element::create(boost::lexical_cast<int64_t>(number),
-                                    Element::Position(line, start_pos)));
+                                    Element::Position(file, line, start_pos)));
         } catch (const boost::bad_lexical_cast&) {
             throwJSONError(std::string("Number overflow: ") + number, file,
                            line, start_pos);
@@ -453,9 +466,11 @@ fromStringstreamBool(std::istream& in, const std::string& file,
     const std::string word = wordFromStringstream(in, pos);
 
     if (boost::iequals(word, "True")) {
-        return (Element::create(true, Element::Position(line, start_pos)));
+        return (Element::create(true, Element::Position(file, line,
+                                                        start_pos)));
     } else if (boost::iequals(word, "False")) {
-        return (Element::create(false, Element::Position(line, start_pos)));
+        return (Element::create(false, Element::Position(file, line,
+                                                         start_pos)));
     } else {
         throwJSONError(std::string("Bad boolean value: ") + word, file,
                        line, start_pos);
@@ -473,7 +488,7 @@ fromStringstreamNull(std::istream& in, const std::string& file,
     // This will move the pos to the end of the value.
     const std::string word = wordFromStringstream(in, pos);
     if (boost::iequals(word, "null")) {
-        return (Element::create(Element::Position(line, start_pos)));
+        return (Element::create(Element::Position(file, line, start_pos)));
     } else {
         throwJSONError(std::string("Bad null value: ") + word, file,
                        line, start_pos);
@@ -490,7 +505,8 @@ fromStringstreamString(std::istream& in, const std::string& file, int& line,
     const uint32_t start_pos = pos;
     // This will move the pos to the end of the value.
     const std::string string_value = strFromStringstream(in, file, line, pos);
-    return (Element::create(string_value, Element::Position(line, start_pos)));
+    return (Element::create(string_value, Element::Position(file, line,
+                                                            start_pos)));
 }
 
 ElementPtr
@@ -498,7 +514,7 @@ fromStringstreamList(std::istream& in, const std::string& file, int& line,
                      int& pos)
 {
     int c = 0;
-    ElementPtr list = Element::createList(Element::Position(line, pos));
+    ElementPtr list = Element::createList(Element::Position(file, line, pos));
     ConstElementPtr cur_list_element;
 
     skipChars(in, WHITESPACE, line, pos);
@@ -519,7 +535,7 @@ ElementPtr
 fromStringstreamMap(std::istream& in, const std::string& file, int& line,
                     int& pos)
 {
-    ElementPtr map = Element::createMap(Element::Position(line, pos));
+    ElementPtr map = Element::createMap(Element::Position(file, line, pos));
     skipChars(in, WHITESPACE, line, pos);
     int c = in.peek();
     if (c == EOF) {

+ 34 - 8
src/lib/cc/data.h

@@ -77,8 +77,8 @@ public:
     /// \brief Represents the position of the data element within a
     /// configuration string.
     ///
-    /// Position comprises a line number and an offset within this line
-    /// where the element value starts. For example, if the JSON string is
+    /// Position comprises a file name, line number and an offset within this
+    /// line where the element value starts. For example, if the JSON string is
     ///
     /// \code
     /// { "foo": "some string",
@@ -94,26 +94,39 @@ public:
     /// uint32_t arguments holding line number and position within the line are
     /// not confused with the @c Element values passed to these functions.
     struct Position {
-        uint32_t line_; ///< Line number.
-        uint32_t pos_;  ///< Position within the line.
+        std::string file_; ///< File name.
+        uint32_t line_;    ///< Line number.
+        uint32_t pos_;     ///< Position within the line.
+
+        /// \brief Default constructor.
+        Position() : file_(""), line_(0), pos_(0) {
+        }
 
         /// \brief Constructor.
         ///
+        /// \param file File name.
         /// \param line Line number.
         /// \param pos Position within the line.
-        Position(const uint32_t line, const uint32_t pos)
-            : line_(line), pos_(pos) {
+        Position(const std::string& file, const uint32_t line,
+                 const uint32_t pos)
+            : file_(file), line_(line), pos_(pos) {
         }
+
+        /// \brief Returns the position in the textual format.
+        ///
+        /// The returned position has the following format: file:line:pos.
+        std::string str() const;
     };
 
-    /// \brief Returns @c Position object with line_ and pos_ set to 0.
+    /// \brief Returns @c Position object with line_ and pos_ set to 0, and
+    /// with an empty file name.
     ///
     /// The object containing two zeros is a default for most of the
     /// methods creating @c Element objects. The returned value is static
     /// so as it is not created everytime the function with the default
     /// position argument is called.
     static const Position& ZERO_POSITION() {
-        static Position position(0, 0);
+        static Position position("", 0, 0);
         return (position);
     }
 
@@ -658,6 +671,19 @@ ConstElementPtr removeIdentical(ConstElementPtr a, ConstElementPtr b);
 void merge(ElementPtr element, ConstElementPtr other);
 
 ///
+/// \brief Insert Element::Position as a string into stream.
+///
+/// This operator converts the \c Element::Position into a string and
+/// inserts it into the output stream \c out.
+///
+/// \param out A \c std::ostream object on which the insertion operation is
+/// performed.
+/// \param pos The \c Element::Position structure to insert.
+/// \return A reference to the same \c std::ostream object referenced by
+/// parameter \c out after the insertion operation.
+std::ostream& operator<<(std::ostream& out, const Element::Position& pos);
+
+///
 /// \brief Insert the Element as a string into stream.
 ///
 /// This method converts the \c ElementPtr into a string with

+ 34 - 13
src/lib/cc/tests/data_unittests.cc

@@ -30,6 +30,15 @@ using std::setw;
 using std::string;
 
 namespace {
+
+TEST(Position, str) {
+    Element::Position position("kea.conf", 30, 20);
+    EXPECT_EQ("kea.conf:30:20", position.str());
+
+    Element::Position position2("another.conf", 123, 24);
+    EXPECT_EQ("another.conf:123:24", position2.str());
+}
+
 TEST(Element, type) {
     // this tests checks whether the getType() function returns the
     // correct type
@@ -931,22 +940,24 @@ TEST(Element, merge) {
 }
 
 TEST(Element, getPosition) {
+    std::istringstream ss("{\n"
+                          "    \"a\":  2,\n"
+                          "    \"b\":true,\n"
+                          "    \"cy\": \"a string\",\n"
+                          "    \"dyz\": {\n"
+                          "\n"
+                          "      \"e\": 3,\n"
+                          "        \"f\": null\n"
+                          "\n"
+                          "    },\n"
+                          "    \"g\": [ 5, 6,\n"
+                          "             7 ]\n"
+                          "}\n");
+
     // Create a JSON string holding different type of values. Some of the
     // values in the config string are not aligned, so as we can check that
     // the position is set correctly for the elements.
-    ElementPtr top = Element::fromJSON("{\n"
-                                       "    \"a\":  2,\n"
-                                       "    \"b\":true,\n"
-                                       "    \"cy\": \"a string\",\n"
-                                       "    \"dyz\": {\n"
-                                       "\n"
-                                       "      \"e\": 3,\n"
-                                       "        \"f\": null\n"
-                                       "\n"
-                                       "    },\n"
-                                       "    \"g\": [ 5, 6,\n"
-                                       "             7 ]\n"
-                                       "}\n");
+    ElementPtr top = Element::fromJSON(ss, "kea.conf");
     ASSERT_TRUE(top);
 
     // Element "a"
@@ -954,36 +965,42 @@ TEST(Element, getPosition) {
     ASSERT_TRUE(level1_el);
     EXPECT_EQ(2, level1_el->getPosition().line_);
     EXPECT_EQ(11, level1_el->getPosition().pos_);
+    EXPECT_EQ("kea.conf", level1_el->getPosition().file_);
 
     // Element "b"
     level1_el = top->get("b");
     ASSERT_TRUE(level1_el);
     EXPECT_EQ(3, level1_el->getPosition().line_);
     EXPECT_EQ(9, level1_el->getPosition().pos_);
+    EXPECT_EQ("kea.conf", level1_el->getPosition().file_);
 
     // Element "cy"
     level1_el = top->get("cy");
     ASSERT_TRUE(level1_el);
     EXPECT_EQ(4, level1_el->getPosition().line_);
     EXPECT_EQ(11, level1_el->getPosition().pos_);
+    EXPECT_EQ("kea.conf", level1_el->getPosition().file_);
 
     // Element "dyz"
     level1_el = top->get("dyz");
     ASSERT_TRUE(level1_el);
     EXPECT_EQ(5, level1_el->getPosition().line_);
     EXPECT_EQ(13, level1_el->getPosition().pos_);
+    EXPECT_EQ("kea.conf", level1_el->getPosition().file_);
 
     // Element "e" is a sub element of "dyz".
     ConstElementPtr level2_el = level1_el->get("e");
     ASSERT_TRUE(level2_el);
     EXPECT_EQ(7, level2_el->getPosition().line_);
     EXPECT_EQ(12, level2_el->getPosition().pos_);
+    EXPECT_EQ("kea.conf", level2_el->getPosition().file_);
 
     // Element "f" is also a sub element of "dyz"
     level2_el = level1_el->get("f");
     ASSERT_TRUE(level2_el);
     EXPECT_EQ(8, level2_el->getPosition().line_);
     EXPECT_EQ(14, level2_el->getPosition().pos_);
+    EXPECT_EQ("kea.conf", level2_el->getPosition().file_);
 
     // Element "g" is a list.
     level1_el = top->get("g");
@@ -991,24 +1008,28 @@ TEST(Element, getPosition) {
     EXPECT_EQ(11, level1_el->getPosition().line_);
     // Position indicates where the values start (excluding the "[" character)"
     EXPECT_EQ(11, level1_el->getPosition().pos_);
+    EXPECT_EQ("kea.conf", level1_el->getPosition().file_);
 
     // First element from the list.
     level2_el = level1_el->get(0);
     ASSERT_TRUE(level2_el);
     EXPECT_EQ(11, level2_el->getPosition().line_);
     EXPECT_EQ(12, level2_el->getPosition().pos_);
+    EXPECT_EQ("kea.conf", level2_el->getPosition().file_);
 
     // Second element from the list.
     level2_el = level1_el->get(1);
     ASSERT_TRUE(level2_el);
     EXPECT_EQ(11, level2_el->getPosition().line_);
     EXPECT_EQ(15, level2_el->getPosition().pos_);
+    EXPECT_EQ("kea.conf", level2_el->getPosition().file_);
 
     // Third element from the list.
     level2_el = level1_el->get(2);
     ASSERT_TRUE(level2_el);
     EXPECT_EQ(12, level2_el->getPosition().line_);
     EXPECT_EQ(14, level2_el->getPosition().pos_);
+    EXPECT_EQ("kea.conf", level2_el->getPosition().file_);
 
 }
 

+ 1 - 1
src/lib/dhcp/option_definition.cc

@@ -273,7 +273,7 @@ OptionDefinition::validate() const {
 
     } else if (type_ >= OPT_UNKNOWN_TYPE) {
         // Option definition must be of a known type.
-        err_str << "option type value " << type_ << " is out of range.";
+        err_str << "option type " << type_ << " not supported.";
 
     } else if (array_type_) {
         if (type_ == OPT_STRING_TYPE) {

+ 1 - 1
src/lib/dhcpsrv/Makefile.am

@@ -1,4 +1,4 @@
-SUBDIRS = . tests
+SUBDIRS = . testutils tests
 
 dhcp_data_dir = @localstatedir@/@PACKAGE@
 

+ 2 - 2
src/lib/dhcpsrv/d2_client_cfg.cc

@@ -99,13 +99,13 @@ D2ClientConfig::enableUpdates(bool enable) {
 void
 D2ClientConfig::validateContents() {
     if (ncr_format_ != dhcp_ddns::FMT_JSON) {
-        isc_throw(D2ClientError, "D2ClientConfig: NCR Format:"
+        isc_throw(D2ClientError, "D2ClientConfig: NCR Format: "
                     << dhcp_ddns::ncrFormatToString(ncr_format_)
                     << " is not yet supported");
     }
 
     if (ncr_protocol_ != dhcp_ddns::NCR_UDP) {
-        isc_throw(D2ClientError, "D2ClientConfig: NCR Protocol:"
+        isc_throw(D2ClientError, "D2ClientConfig: NCR Protocol: "
                   << dhcp_ddns::ncrProtocolToString(ncr_protocol_)
                   << " is not yet supported");
     }

+ 17 - 10
src/lib/dhcpsrv/dbaccess_parser.cc

@@ -57,14 +57,20 @@ DbAccessParser::build(isc::data::ConstElementPtr config_value) {
 
     // 3. Update the copy with the passed keywords.
     BOOST_FOREACH(ConfigPair param, config_value->mapValue()) {
-        // The persist parameter is the only boolean parameter at the
-        // moment. It needs special handling.
-        if (param.first != "persist") {
-            values_copy[param.first] = param.second->stringValue();
-
-        } else {
-            values_copy[param.first] = (param.second->boolValue() ?
-                                        "true" : "false");
+        try {
+            // The persist parameter is the only boolean parameter at the
+            // moment. It needs special handling.
+            if (param.first != "persist") {
+                values_copy[param.first] = param.second->stringValue();
+
+            } else {
+                values_copy[param.first] = (param.second->boolValue() ?
+                                            "true" : "false");
+            }
+        } catch (const isc::data::TypeError& ex) {
+            // Append position of the element.
+            isc_throw(isc::data::TypeError, ex.what() << " ("
+                      << param.second->getPosition() << ")");
         }
     }
 
@@ -75,13 +81,14 @@ DbAccessParser::build(isc::data::ConstElementPtr config_value) {
     if (type_ptr == values_copy.end()) {
         isc_throw(TypeKeywordMissing, "lease database access parameters must "
                   "include the keyword 'type' to determine type of database "
-                  "to be accessed");
+                  "to be accessed (" << config_value->getPosition() << ")");
     }
 
     // b. Check if the 'type; keyword known and throw an exception if not.
     string dbtype = type_ptr->second;
     if ((dbtype != "memfile") && (dbtype != "mysql") && (dbtype != "postgresql")) {
-        isc_throw(BadValue, "unknown backend database type: " << dbtype);
+        isc_throw(BadValue, "unknown backend database type: " << dbtype
+                  << " (" << config_value->getPosition() << ")");
     }
 
     // 5. If all is OK, update the stored keyword/value pairs.  We do this by

+ 319 - 202
src/lib/dhcpsrv/dhcp_parsers.cc

@@ -29,6 +29,7 @@
 #include <vector>
 
 using namespace std;
+using namespace isc::asiolink;
 using namespace isc::data;
 using namespace isc::hooks;
 
@@ -125,6 +126,8 @@ DebugParser::commit() {
 // **************************** BooleanParser  *************************
 
 template<> void ValueParser<bool>::build(isc::data::ConstElementPtr value) {
+    // Invoke common code for all specializations of build().
+    buildCommon(value);
     // The Config Manager checks if user specified a
     // valid value for a boolean parameter: True or False.
     // We should have a boolean Element, use value directly
@@ -132,28 +135,35 @@ template<> void ValueParser<bool>::build(isc::data::ConstElementPtr value) {
         value_ = value->boolValue();
     } catch (const isc::data::TypeError &) {
         isc_throw(BadValue, " Wrong value type for " << param_name_
-                  << " : build called with a non-boolean element.");
+                  << " : build called with a non-boolean element "
+                  << "(" << value->getPosition() << ").");
     }
 }
 
 // **************************** Uin32Parser  *************************
 
 template<> void ValueParser<uint32_t>::build(ConstElementPtr value) {
+    // Invoke common code for all specializations of build().
+    buildCommon(value);
+
     int64_t check;
     string x = value->str();
     try {
         check = boost::lexical_cast<int64_t>(x);
     } catch (const boost::bad_lexical_cast &) {
         isc_throw(BadValue, "Failed to parse value " << value->str()
-                  << " as unsigned 32-bit integer.");
+                  << " as unsigned 32-bit integer "
+                  "(" << value->getPosition() << ").");
     }
     if (check > std::numeric_limits<uint32_t>::max()) {
-        isc_throw(BadValue, "Value " << value->str() << "is too large"
-                  << " for unsigned 32-bit integer.");
+        isc_throw(BadValue, "Value " << value->str() << " is too large"
+                  " for unsigned 32-bit integer "
+                  "(" << value->getPosition() << ").");
     }
     if (check < 0) {
-        isc_throw(BadValue, "Value " << value->str() << "is negative."
-               << " Only 0 or larger are allowed for unsigned 32-bit integer.");
+        isc_throw(BadValue, "Value " << value->str() << " is negative."
+               << " Only 0 or larger are allowed for unsigned 32-bit integer "
+                  "(" << value->getPosition() << ").");
     }
 
     // value is small enough to fit
@@ -163,6 +173,9 @@ template<> void ValueParser<uint32_t>::build(ConstElementPtr value) {
 // **************************** StringParser  *************************
 
 template <> void ValueParser<std::string>::build(ConstElementPtr value) {
+    // Invoke common code for all specializations of build().
+    buildCommon(value);
+
     value_ = value->str();
     boost::erase_all(value_, "\"");
 }
@@ -194,7 +207,8 @@ InterfaceListConfigParser::build(ConstElementPtr value) {
             if (isIfaceAdded(iface_name)) {
                 isc_throw(isc::dhcp::DhcpConfigError, "duplicate interface"
                           << " name '" << iface_name << "' specified in '"
-                          << param_name_ << "' configuration parameter");
+                          << param_name_ << "' configuration parameter "
+                          "(" << value->getPosition() << ")");
             }
             // @todo check that this interface exists in the system!
             // The IfaceMgr exposes mechanisms to check this.
@@ -285,7 +299,8 @@ HooksLibrariesParser::build(ConstElementPtr value) {
             error_list += (string(", ") + error_libs[i]);
         }
         isc_throw(DhcpConfigError, "hooks libraries failed to validate - "
-                  "library or libraries in error are: " + error_list);
+                  "library or libraries in error are: " << error_list
+                  << " (" << value->getPosition() << ")");
     }
 
     // The library list has changed and the libraries are valid, so flag for
@@ -349,8 +364,8 @@ OptionDataParser::build(ConstElementPtr option_data_entries) {
             parser = value_parser;
         } else {
             isc_throw(DhcpConfigError,
-                      "Parser error: option-data parameter not supported: "
-                      << param.first);
+                      "option-data parameter not supported: " << param.first
+                      << " (" << param.second->getPosition() << ")");
         }
 
         parser->build(param.second);
@@ -364,7 +379,7 @@ OptionDataParser::build(ConstElementPtr option_data_entries) {
     }
 
     // Try to create the option instance.
-    createOption();
+    createOption(option_data_entries);
 }
 
 void
@@ -397,70 +412,93 @@ OptionDataParser::commit() {
 }
 
 void
-OptionDataParser::createOption() {
+OptionDataParser::createOption(ConstElementPtr option_data) {
+    // Check if mandatory parameters are specified.
+    uint32_t code;
+    std::string name;
+    std::string data;
+    try {
+        code = uint32_values_->getParam("code");
+        name = string_values_->getParam("name");
+        data = string_values_->getParam("data");
+    } catch (const std::exception& ex) {
+        isc_throw(DhcpConfigError,
+                  ex.what() << "(" << option_data->getPosition() << ")");
+    }
+    // Check parameters having default values.
+    std::string space = string_values_->getOptionalParam("space",
+              global_context_->universe_ == Option::V4 ? "dhcp4" : "dhcp6");
+    bool csv_format = boolean_values_->getOptionalParam("csv-format", false);
+
     // Option code is held in the uint32_t storage but is supposed to
     // be uint16_t value. We need to check that value in the configuration
     // does not exceed range of uint8_t for DHCPv4, uint16_t for DHCPv6 and
     // is not zero.
-    uint32_t option_code = uint32_values_->getParam("code");
-    if (option_code == 0) {
-        isc_throw(DhcpConfigError, "option code must not be zero."
-                << " Option code '0' is reserved.");
+    if (code == 0) {
+        isc_throw(DhcpConfigError, "option code must not be zero "
+                  "(" << uint32_values_->getPosition("code") << ")");
 
     } else if (global_context_->universe_ == Option::V4 &&
-               option_code > std::numeric_limits<uint8_t>::max()) {
-        isc_throw(DhcpConfigError, "invalid option code '" << option_code
+               code > std::numeric_limits<uint8_t>::max()) {
+        isc_throw(DhcpConfigError, "invalid option code '" << code
                 << "', it must not exceed '"
                   << static_cast<int>(std::numeric_limits<uint8_t>::max())
-                  << "'");
+                  << "' (" << uint32_values_->getPosition("code") << ")");
 
     } else if (global_context_->universe_ == Option::V6 &&
-               option_code > std::numeric_limits<uint16_t>::max()) {
-        isc_throw(DhcpConfigError, "invalid option code '" << option_code
+               code > std::numeric_limits<uint16_t>::max()) {
+        isc_throw(DhcpConfigError, "invalid option code '" << code
                 << "', it must not exceed '"
                   << std::numeric_limits<uint16_t>::max()
-                  << "'");
+                  << "' (" << uint32_values_->getPosition("code") << ")");
 
     }
 
-    // Check that the option name has been specified, is non-empty and does not
-    // contain spaces
-    std::string option_name = string_values_->getParam("name");
-    if (option_name.empty()) {
+    // Check that the option name is non-empty and does not contain spaces
+    if (name.empty()) {
         isc_throw(DhcpConfigError, "name of the option with code '"
-                << option_code << "' is empty");
-    } else if (option_name.find(" ") != std::string::npos) {
-        isc_throw(DhcpConfigError, "invalid option name '" << option_name
-                << "', space character is not allowed");
+                  << code << "' is empty ("
+                  << string_values_->getPosition("name") << ")");
+    } else if (name.find(" ") != std::string::npos) {
+        isc_throw(DhcpConfigError, "invalid option name '" << name
+                  << "', space character is not allowed ("
+                  << string_values_->getPosition("name") << ")");
     }
 
-    std::string option_space = string_values_->getParam("space");
-    if (!OptionSpace::validateName(option_space)) {
+    if (!OptionSpace::validateName(space)) {
         isc_throw(DhcpConfigError, "invalid option space name '"
-                << option_space << "' specified for option '"
-                << option_name << "' (code '" << option_code
-                << "')");
+                << space << "' specified for option '"
+                << name << "', code '" << code
+                  << "' (" << string_values_->getPosition("space") << ")");
     }
 
-    const bool csv_format = boolean_values_->getParam("csv-format");
-
     // Find the Option Definition for the option by its option code.
     // findOptionDefinition will throw if not found, no need to test.
+    // Find the definition for the option by its code. This function
+    // may throw so we catch exceptions to log the culprit line of the
+    // configuration.
     OptionDefinitionPtr def;
-    if (!(def = findServerSpaceOptionDefinition(option_space, option_code))) {
+    try {
+        def = findServerSpaceOptionDefinition(space, code);
+
+    } catch (const std::exception& ex) {
+        isc_throw(DhcpConfigError, ex.what()
+                  << " (" << string_values_->getPosition("space") << ")");
+    }
+    if (!def) {
         // If we are not dealing with a standard option then we
         // need to search for its definition among user-configured
         // options. They are expected to be in the global storage
         // already.
         OptionDefContainerPtr defs =
-            global_context_->option_defs_->getItems(option_space);
+            global_context_->option_defs_->getItems(space);
 
         // The getItems() should never return the NULL pointer. If there are
         // no option definitions for the particular option space a pointer
         // to an empty container should be returned.
         assert(defs);
         const OptionDefContainerTypeIndex& idx = defs->get<1>();
-        OptionDefContainerTypeRange range = idx.equal_range(option_code);
+        OptionDefContainerTypeRange range = idx.equal_range(code);
         if (std::distance(range.first, range.second) > 0) {
             def = *range.first;
         }
@@ -469,15 +507,13 @@ OptionDataParser::createOption() {
         // specified as hex
         if (!def && csv_format) {
             isc_throw(DhcpConfigError, "definition for the option '"
-                      << option_space << "." << option_name
-                      << "' having code '" <<  option_code
-                      << "' does not exist");
+                      << space << "." << name
+                      << "' having code '" << code
+                      << "' does not exist ("
+                      << string_values_->getPosition("name") << ")");
         }
     }
 
-    // Get option data from the configuration database ('data' field).
-    std::string option_data = string_values_->getParam("data");
-
     // Transform string of hexadecimal digits into binary format.
     std::vector<uint8_t> binary;
     std::vector<std::string> data_tokens;
@@ -487,7 +523,7 @@ OptionDataParser::createOption() {
         // separated values then we need to split this string into
         // individual values - each value will be used to initialize
         // one data field of an option.
-        data_tokens = isc::util::str::tokens(option_data, ",");
+        data_tokens = isc::util::str::tokens(data, ",");
     } else {
         // Otherwise, the option data is specified as a string of
         // hexadecimal digits that we have to turn into binary format.
@@ -495,13 +531,14 @@ OptionDataParser::createOption() {
             // The decodeHex function expects that the string contains an
             // even number of digits. If we don't meet this requirement,
             // we have to insert a leading 0.
-            if (!option_data.empty() && option_data.length() % 2) {
-                option_data = option_data.insert(0, "0");
+            if (!data.empty() && data.length() % 2) {
+                data = data.insert(0, "0");
             }
-            util::encode::decodeHex(option_data, binary);
+            util::encode::decodeHex(data, binary);
         } catch (...) {
             isc_throw(DhcpConfigError, "option data is not a valid"
-                      << " string of hexadecimal digits: " << option_data);
+                      << " string of hexadecimal digits: " << data
+                      << " (" << string_values_->getPosition("data") << ")");
         }
     }
 
@@ -510,17 +547,18 @@ OptionDataParser::createOption() {
         if (csv_format) {
             isc_throw(DhcpConfigError, "the CSV option data format can be"
                       " used to specify values for an option that has a"
-                      " definition. The option with code " << option_code
-                      << " does not have a definition.");
+                      " definition. The option with code " << code
+                      << " does not have a definition ("
+                      << boolean_values_->getPosition("csv-format") << ")");
         }
 
-        // @todo We have a limited set of option definitions intiialized at
+        // @todo We have a limited set of option definitions initalized at
         // the moment.  In the future we want to initialize option definitions
         // for all options.  Consequently an error will be issued if an option
         // definition does not exist for a particular option code. For now it is
         // ok to create generic option if definition does not exist.
         OptionPtr option(new Option(global_context_->universe_,
-                        static_cast<uint16_t>(option_code), binary));
+                        static_cast<uint16_t>(code), binary));
         // The created option is stored in option_descriptor_ class member
         // until the commit stage when it is inserted into the main storage.
         // If an option with the same code exists in main storage already the
@@ -534,11 +572,12 @@ OptionDataParser::createOption() {
         // 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() != option_name) {
+        if (def->getName() != name) {
             isc_throw(DhcpConfigError, "specified option name '"
-                      << option_name << "' does not match the "
-                      << "option definition: '" << option_space
-                      << "." << def->getName() << "'");
+                      << name << "' does not match the "
+                      << "option definition: '" << space
+                      << "." << def->getName() << "' ("
+                      << string_values_->getPosition("name") << ")");
         }
 
         // Option definition has been found so let's use it to create
@@ -546,22 +585,23 @@ OptionDataParser::createOption() {
         try {
             OptionPtr option = csv_format ?
                 def->optionFactory(global_context_->universe_,
-                                  option_code, data_tokens) :
+                                  code, data_tokens) :
                 def->optionFactory(global_context_->universe_,
-                                  option_code, binary);
+                                   code, binary);
             Subnet::OptionDescriptor desc(option, false);
             option_descriptor_.option = option;
             option_descriptor_.persistent = false;
         } catch (const isc::Exception& ex) {
             isc_throw(DhcpConfigError, "option data does not match"
-                      << " option definition (space: " << option_space
-                      << ", code: " << option_code << "): "
-                      << ex.what());
+                      << " option definition (space: " << space
+                      << ", code: " << code << "): "
+                      << ex.what() << " ("
+                      << string_values_->getPosition("data") << ")");
         }
     }
 
     // All went good, so we can set the option space name.
-    option_space_ = option_space;
+    option_space_ = space;
 }
 
 // **************************** OptionDataListParser *************************
@@ -617,9 +657,13 @@ OptionDataListParser::commit() {
 
 // ******************************** OptionDefParser ****************************
 OptionDefParser::OptionDefParser(const std::string&,
-                                OptionDefStoragePtr storage)
-    : storage_(storage), boolean_values_(new BooleanStorage()),
-    string_values_(new StringStorage()), uint32_values_(new Uint32Storage()) {
+                                 OptionDefStoragePtr storage,
+                                 ParserContextPtr global_context)
+    : storage_(storage),
+      boolean_values_(new BooleanStorage()),
+      string_values_(new StringStorage()),
+      uint32_values_(new Uint32Storage()),
+      global_context_(global_context) {
     if (!storage_) {
         isc_throw(isc::dhcp::DhcpConfigError, "parser logic error:"
              << "options storage may not be NULL");
@@ -629,7 +673,7 @@ OptionDefParser::OptionDefParser(const std::string&,
 void
 OptionDefParser::build(ConstElementPtr option_def) {
     // Parse the elements that make up the option definition.
-     BOOST_FOREACH(ConfigPair param, option_def->mapValue()) {
+    BOOST_FOREACH(ConfigPair param, option_def->mapValue()) {
         std::string entry(param.first);
         ParserPtr parser;
         if (entry == "name" || entry == "type" || entry == "record-types"
@@ -646,15 +690,15 @@ OptionDefParser::build(ConstElementPtr option_def) {
                                          boolean_values_));
             parser = array_parser;
         } else {
-            isc_throw(DhcpConfigError, "invalid parameter: " << entry);
+            isc_throw(DhcpConfigError, "invalid parameter '" << entry
+                      << "' (" << param.second->getPosition() << ")");
         }
 
         parser->build(param.second);
         parser->commit();
     }
-
     // Create an instance of option definition.
-    createOptionDef();
+    createOptionDef(option_def);
 
     // Get all items we collected so far for the particular option space.
     OptionDefContainerPtr defs = storage_->getItems(option_space_name_);
@@ -670,7 +714,8 @@ OptionDefParser::build(ConstElementPtr option_def) {
     // option definitions within an option space.
     if (std::distance(range.first, range.second) > 0) {
         isc_throw(DhcpConfigError, "duplicated option definition for"
-                << " code '" << option_definition_->getCode() << "'");
+                  << " code '" << option_definition_->getCode() << "' ("
+                  << option_def->getPosition() << ")");
     }
 }
 
@@ -683,22 +728,34 @@ OptionDefParser::commit() {
 }
 
 void
-OptionDefParser::createOptionDef() {
-    // Get the option space name and validate it.
-    std::string space = string_values_->getParam("space");
+OptionDefParser::createOptionDef(ConstElementPtr option_def_element) {
+    // Check if mandatory parameters have been specified.
+    std::string name;
+    uint32_t code;
+    std::string type;
+    try {
+        name = string_values_->getParam("name");
+        code = uint32_values_->getParam("code");
+        type = string_values_->getParam("type");
+    } catch (const std::exception& ex) {
+        isc_throw(DhcpConfigError, ex.what() << " ("
+                  << option_def_element->getPosition() << ")");
+    }
+
+    bool array_type = boolean_values_->getOptionalParam("array", false);
+    std::string record_types =
+        string_values_->getOptionalParam("record-types", "");
+    std::string space = string_values_->getOptionalParam("space",
+              global_context_->universe_ == Option::V4 ? "dhcp4" : "dhcp6");
+    std::string encapsulates =
+        string_values_->getOptionalParam("encapsulate", "");
+
     if (!OptionSpace::validateName(space)) {
         isc_throw(DhcpConfigError, "invalid option space name '"
-                  << space << "'");
+                  << space << "' ("
+                  << string_values_->getPosition("space") << ")");
     }
 
-    // Get other parameters that are needed to create the
-    // option definition.
-    std::string name = string_values_->getParam("name");
-    uint32_t code = uint32_values_->getParam("code");
-    std::string type = string_values_->getParam("type");
-    bool array_type = boolean_values_->getParam("array");
-    std::string encapsulates = string_values_->getParam("encapsulate");
-
     // Create option definition.
     OptionDefinitionPtr def;
     // We need to check if user has set encapsulated option space
@@ -708,13 +765,15 @@ OptionDefParser::createOptionDef() {
         if (array_type) {
             isc_throw(DhcpConfigError, "option '" << space << "."
                       << "name" << "', comprising an array of data"
-                      << " fields may not encapsulate any option space");
+                      << " fields may not encapsulate any option space ("
+                      << option_def_element->getPosition() << ")");
 
         } else if (encapsulates == space) {
             isc_throw(DhcpConfigError, "option must not encapsulate"
                       << " an option space it belongs to: '"
                       << space << "." << name << "' is set to"
-                      << " encapsulate '" << space << "'");
+                      << " encapsulate '" << space << "' ("
+                      << option_def_element->getPosition() << ")");
 
         } else {
             def.reset(new OptionDefinition(name, code, type,
@@ -726,10 +785,6 @@ OptionDefParser::createOptionDef() {
 
     }
 
-    // The record-types field may carry a list of comma separated names
-    // of data types that form a record.
-    std::string record_types = string_values_->getParam("record-types");
-
     // Split the list of record types into tokens.
     std::vector<std::string> record_tokens =
     isc::util::str::tokens(record_types, ",");
@@ -744,16 +799,17 @@ OptionDefParser::createOptionDef() {
         } catch (const Exception& ex) {
             isc_throw(DhcpConfigError, "invalid record type values"
                       << " specified for the option definition: "
-                      << ex.what());
+                      << ex.what() << " ("
+                      << string_values_->getPosition("record-types") << ")");
         }
     }
 
-    // Check the option definition parameters are valid.
+    // Validate the definition.
     try {
         def->validate();
-    } catch (const isc::Exception& ex) {
-        isc_throw(DhcpConfigError, "invalid option definition"
-                  << " parameters: " << ex.what());
+    } catch (const std::exception& ex) {
+        isc_throw(DhcpConfigError, ex.what()
+                  << " (" << option_def_element->getPosition() << ")");
     }
 
     // Option definition has been created successfully.
@@ -763,7 +819,9 @@ OptionDefParser::createOptionDef() {
 
 // ******************************** OptionDefListParser ************************
 OptionDefListParser::OptionDefListParser(const std::string&,
-    OptionDefStoragePtr storage) :storage_(storage) {
+                                         ParserContextPtr global_context)
+    : storage_(global_context->option_defs_),
+      global_context_(global_context) {
     if (!storage_) {
         isc_throw(isc::dhcp::DhcpConfigError, "parser logic error:"
              << "storage may not be NULL");
@@ -778,26 +836,24 @@ OptionDefListParser::build(ConstElementPtr option_def_list) {
 
     if (!option_def_list) {
         isc_throw(DhcpConfigError, "parser error: a pointer to a list of"
-                  << " option definitions is NULL");
+                  << " option definitions is NULL ("
+                  << option_def_list->getPosition() << ")");
     }
 
     BOOST_FOREACH(ConstElementPtr option_def, option_def_list->listValue()) {
         boost::shared_ptr<OptionDefParser>
-                parser(new OptionDefParser("single-option-def", storage_));
+            parser(new OptionDefParser("single-option-def", storage_,
+                                       global_context_));
         parser->build(option_def);
         parser->commit();
     }
-}
 
-void
-OptionDefListParser::commit() {
     CfgMgr& cfg_mgr = CfgMgr::instance();
     cfg_mgr.deleteOptionDefs();
 
     // We need to move option definitions from the temporary
     // storage to the storage.
-    std::list<std::string> space_names =
-    storage_->getOptionSpaceNames();
+    std::list<std::string> space_names = storage_->getOptionSpaceNames();
 
     BOOST_FOREACH(std::string space_name, space_names) {
         BOOST_FOREACH(OptionDefinitionPtr def,
@@ -806,11 +862,24 @@ OptionDefListParser::commit() {
             // values. The validation is expected to be made by the
             // OptionDefParser when creating an option definition.
             assert(def);
-            cfg_mgr.addOptionDef(def, space_name);
+            // The Config Manager may thrown an exception if the duplicated
+            // definition is being added. Catch the exceptions here to and
+            // append the position in the config.
+            try {
+                cfg_mgr.addOptionDef(def, space_name);
+            } catch (const std::exception& ex) {
+                isc_throw(DhcpConfigError, ex.what() << " ("
+                          << option_def_list->getPosition() << ")");
+            }
         }
     }
 }
 
+void
+OptionDefListParser::commit() {
+    // Do nothing.
+}
+
 //****************************** RelayInfoParser ********************************
 RelayInfoParser::RelayInfoParser(const std::string&,
                                  const isc::dhcp::Subnet::RelayInfoPtr& relay_info,
@@ -840,14 +909,16 @@ RelayInfoParser::build(ConstElementPtr relay_info) {
         ip.reset(new asiolink::IOAddress(string_values_->getParam("ip-address")));
     } catch (...)  {
         isc_throw(DhcpConfigError, "Failed to parse ip-address "
-                  "value: " << string_values_->getParam("ip-address"));
+                  "value: " << string_values_->getParam("ip-address")
+                  << " (" << string_values_->getPosition("ip-address") << ")");
     }
 
     if ( (ip->isV4() && family_ != Option::V4) ||
          (ip->isV6() && family_ != Option::V6) ) {
         isc_throw(DhcpConfigError, "ip-address field " << ip->toText()
                   << "does not have IP address of expected family type: "
-                  << (family_ == Option::V4?"IPv4":"IPv6"));
+                  << (family_ == Option::V4 ? "IPv4" : "IPv6")
+                  << " (" << string_values_->getPosition("ip-address") << ")");
     }
 
     local_.addr_ = *ip;
@@ -916,7 +987,8 @@ PoolParser::build(ConstElementPtr pools_list) {
                 len = boost::lexical_cast<int>(prefix_len);
             } catch (...)  {
                 isc_throw(DhcpConfigError, "Failed to parse pool "
-                          "definition: " << text_pool->stringValue());
+                          "definition: " << text_pool->stringValue()
+                          << " (" << text_pool->getPosition() << ")");
             }
 
             PoolPtr pool(poolMaker(addr, len));
@@ -936,9 +1008,11 @@ PoolParser::build(ConstElementPtr pools_list) {
             continue;
         }
 
-        isc_throw(DhcpConfigError, "Failed to parse pool definition:"
+        isc_throw(DhcpConfigError, "invalid pool definition: "
                   << text_pool->stringValue() <<
-                  ". Does not contain - (for min-max) nor / (prefix/len)");
+                  ". There are two acceptable formats <min address-max address>"
+                  " or <prefix/len> ("
+                  << text_pool->getPosition() << ")");
         }
 }
 
@@ -972,7 +1046,16 @@ SubnetConfigParser::SubnetConfigParser(const std::string&,
 void
 SubnetConfigParser::build(ConstElementPtr subnet) {
     BOOST_FOREACH(ConfigPair param, subnet->mapValue()) {
-        ParserPtr parser(createSubnetConfigParser(param.first));
+        ParserPtr parser;
+        // When unsupported parameter is specified, the function called
+        // below will thrown an exception. We have to catch this exception
+        // to append the line number where the parameter is.
+        try {
+            parser.reset(createSubnetConfigParser(param.first));
+        } catch (const std::exception& ex) {
+            isc_throw(DhcpConfigError, ex.what() << " ("
+                      << param.second->getPosition() << ")");
+        }
         parser->build(param.second);
         parsers_.push_back(parser);
     }
@@ -990,7 +1073,13 @@ SubnetConfigParser::build(ConstElementPtr subnet) {
     }
 
     // Create a subnet.
-    createSubnet();
+    try {
+        createSubnet();
+    } catch (const std::exception& ex) {
+        isc_throw(DhcpConfigError,
+                  "subnet configuration failed (" << subnet->getPosition()
+                  << "): " << ex.what());
+    }
 }
 
 void
@@ -1056,7 +1145,8 @@ SubnetConfigParser::createSubnet() {
     } catch (const DhcpConfigError &) {
         // rethrow with precise error
         isc_throw(DhcpConfigError,
-                 "Mandatory subnet definition in subnet missing");
+                 "mandatory 'subnet' parameter is missing for a subnet being"
+                  " configured");
     }
 
     // Remove any spaces or tabs.
@@ -1071,7 +1161,8 @@ SubnetConfigParser::createSubnet() {
     size_t pos = subnet_txt.find("/");
     if (pos == string::npos) {
         isc_throw(DhcpConfigError,
-                  "Invalid subnet syntax (prefix/len expected):" << subnet_txt);
+                  "Invalid subnet syntax (prefix/len expected):" << subnet_txt
+                  << " (" << string_values_->getPosition("subnet") << ")");
     }
 
     // Try to create the address object. It also validates that
@@ -1102,8 +1193,9 @@ SubnetConfigParser::createSubnet() {
     if (!iface.empty()) {
         if (!IfaceMgr::instance().getIface(iface)) {
             isc_throw(DhcpConfigError, "Specified interface name " << iface
-                     << " for subnet " << subnet_->toText()
-                     << " is not present" << " in the system.");
+                      << " for subnet " << subnet_->toText()
+                      << " is not present" << " in the system ("
+                      << string_values_->getPosition("interface") << ")");
         }
 
         subnet_->setIface(iface);
@@ -1257,100 +1349,125 @@ D2ClientConfigParser::~D2ClientConfigParser() {
 void
 D2ClientConfigParser::build(isc::data::ConstElementPtr client_config) {
     BOOST_FOREACH(ConfigPair param, client_config->mapValue()) {
-        ParserPtr parser(createConfigParser(param.first));
+        ParserPtr parser;
+        try {
+            parser = createConfigParser(param.first);
+        } catch (std::exception& ex) {
+            // Catch exception in case the configuration contains the
+            // unsupported parameter. In this case, we will need to
+            // append the position of this element.
+            isc_throw(DhcpConfigError, ex.what() << " ("
+                      << param.second->getPosition() << ")");
+        }
+
         parser->build(param.second);
         parser->commit();
     }
 
-    bool enable_updates = boolean_values_->getParam("enable-updates");
-    if (!enable_updates && (client_config->mapValue().size() == 1)) {
-        // If enable-updates is the only parameter and it is false then
-        // we're done.  This allows for an abbreviated configuration entry
-        // that only contains that flag.  Use the default D2ClientConfig
-        // constructor to a create a disabled instance.
-        local_client_config_.reset(new D2ClientConfig());
-        return;
-    }
+    /// @todo Create configuration from the configuration parameters. Because
+    /// the validation of the D2 configuration is atomic, there is no way to
+    /// tell which parameter is invalid. Therefore, we catch all exceptions
+    /// and append the line number of the parent element. In the future we
+    /// may should extend D2ClientConfig code so as it returns the name of
+    /// the invalid parameter.
+    try {
+        bool enable_updates = boolean_values_->getParam("enable-updates");
+        if (!enable_updates && (client_config->mapValue().size() == 1)) {
+            // If enable-updates is the only parameter and it is false then
+            // we're done.  This allows for an abbreviated configuration entry
+            // that only contains that flag.  Use the default D2ClientConfig
+            // constructor to a create a disabled instance.
+            local_client_config_.reset(new D2ClientConfig());
 
-    // Get all parameters that are needed to create the D2ClientConfig.
-    asiolink::IOAddress server_ip(string_values_->
-                                  getOptionalParam("server-ip",
-                                                   D2ClientConfig::
-                                                   DFT_SERVER_IP));
-
-    uint32_t server_port = uint32_values_->getOptionalParam("server-port",
-                                                             D2ClientConfig::
-                                                             DFT_SERVER_PORT);
-
-    // The default sender IP depends on the server IP family
-    asiolink::IOAddress
-        sender_ip(string_values_->
-                  getOptionalParam("sender-ip",
-                                   (server_ip.isV4() ?
-                                    D2ClientConfig::DFT_V4_SENDER_IP :
-                                    D2ClientConfig::DFT_V6_SENDER_IP)));
-
-    uint32_t sender_port = uint32_values_->getOptionalParam("sender-port",
-                                                             D2ClientConfig::
-                                                             DFT_SENDER_PORT);
-    uint32_t max_queue_size
-        = uint32_values_->getOptionalParam("max-queue-size",
-                                            D2ClientConfig::
-                                            DFT_MAX_QUEUE_SIZE);
-
-    dhcp_ddns::NameChangeProtocol ncr_protocol
-        = dhcp_ddns::stringToNcrProtocol(string_values_->
-                                         getOptionalParam("ncr-protocol",
-                                                          D2ClientConfig::
-                                                          DFT_NCR_PROTOCOL));
-    dhcp_ddns::NameChangeFormat ncr_format
-        = dhcp_ddns::stringToNcrFormat(string_values_->
-                                       getOptionalParam("ncr-format",
-                                                          D2ClientConfig::
-                                                          DFT_NCR_FORMAT));
-    std::string generated_prefix = string_values_->
-                                   getOptionalParam("generated-prefix",
-                                                    D2ClientConfig::
-                                                    DFT_GENERATED_PREFIX);
-    std::string qualifying_suffix = string_values_->
-                                    getOptionalParam("qualifying-suffix",
-                                                     D2ClientConfig::
-                                                     DFT_QUALIFYING_SUFFIX);
-
-    bool always_include_fqdn = boolean_values_->
-                               getOptionalParam("always-include-fqdn",
-                                                D2ClientConfig::
-                                                DFT_ALWAYS_INCLUDE_FQDN);
+            return;
+        }
 
-    bool override_no_update = boolean_values_->
-                              getOptionalParam("override-no-update",
+        // Get all parameters that are needed to create the D2ClientConfig.
+        IOAddress server_ip =
+            IOAddress(string_values_->getOptionalParam("server-ip",
+                                                       D2ClientConfig::
+                                                       DFT_SERVER_IP));
+
+        uint32_t server_port =
+            uint32_values_->getOptionalParam("server-port",
+                                             D2ClientConfig::DFT_SERVER_PORT);
+
+        // The default sender IP depends on the server IP family
+        asiolink::IOAddress
+            sender_ip(string_values_->
+                      getOptionalParam("sender-ip",
+                                       (server_ip.isV4() ?
+                                        D2ClientConfig::DFT_V4_SENDER_IP :
+                                        D2ClientConfig::DFT_V6_SENDER_IP)));
+
+        uint32_t sender_port =
+            uint32_values_->getOptionalParam("sender-port",
+                                             D2ClientConfig::
+                                             DFT_SENDER_PORT);
+        uint32_t max_queue_size
+            = uint32_values_->getOptionalParam("max-queue-size",
                                                D2ClientConfig::
-                                               DFT_OVERRIDE_NO_UPDATE);
-
-    bool override_client_update = boolean_values_->
-                                  getOptionalParam("override-client-update",
-                                                   D2ClientConfig::
-                                                   DFT_OVERRIDE_CLIENT_UPDATE);
-    bool replace_client_name = boolean_values_->
-                               getOptionalParam("replace-client-name",
+                                               DFT_MAX_QUEUE_SIZE);
+
+        dhcp_ddns::NameChangeProtocol ncr_protocol =
+            dhcp_ddns::stringToNcrProtocol(string_values_->
+                                           getOptionalParam("ncr-protocol",
+                                                            D2ClientConfig::
+                                                            DFT_NCR_PROTOCOL));
+        dhcp_ddns::NameChangeFormat ncr_format
+            = dhcp_ddns::stringToNcrFormat(string_values_->
+                                           getOptionalParam("ncr-format",
+                                                            D2ClientConfig::
+                                                            DFT_NCR_FORMAT));
+        std::string generated_prefix =
+            string_values_->getOptionalParam("generated-prefix",
+                                             D2ClientConfig::
+                                             DFT_GENERATED_PREFIX);
+        std::string qualifying_suffix =
+            string_values_->getOptionalParam("qualifying-suffix",
+                                             D2ClientConfig::
+                                             DFT_QUALIFYING_SUFFIX);
+
+        bool always_include_fqdn =
+            boolean_values_->getOptionalParam("always-include-fqdn",
                                                 D2ClientConfig::
-                                                DFT_REPLACE_CLIENT_NAME);
-
-    // Attempt to create the new client config.
-    local_client_config_.reset(new D2ClientConfig(enable_updates,
-                                                  server_ip,
-                                                  server_port,
-                                                  sender_ip,
-                                                  sender_port,
-                                                  max_queue_size,
-                                                  ncr_protocol,
-                                                  ncr_format,
-                                                  always_include_fqdn,
-                                                  override_no_update,
-                                                  override_client_update,
-                                                  replace_client_name,
-                                                  generated_prefix,
-                                                  qualifying_suffix));
+                                                DFT_ALWAYS_INCLUDE_FQDN);
+
+        bool override_no_update =
+            boolean_values_->getOptionalParam("override-no-update",
+                                              D2ClientConfig::
+                                              DFT_OVERRIDE_NO_UPDATE);
+
+        bool override_client_update =
+            boolean_values_->getOptionalParam("override-client-update",
+                                              D2ClientConfig::
+                                              DFT_OVERRIDE_CLIENT_UPDATE);
+
+        bool replace_client_name =
+            boolean_values_->getOptionalParam("replace-client-name",
+                                              D2ClientConfig::
+                                              DFT_REPLACE_CLIENT_NAME);
+
+        // Attempt to create the new client config.
+        local_client_config_.reset(new D2ClientConfig(enable_updates,
+                                                      server_ip,
+                                                      server_port,
+                                                      sender_ip,
+                                                      sender_port,
+                                                      max_queue_size,
+                                                      ncr_protocol,
+                                                      ncr_format,
+                                                      always_include_fqdn,
+                                                      override_no_update,
+                                                      override_client_update,
+                                                      replace_client_name,
+                                                      generated_prefix,
+                                                      qualifying_suffix));
+
+    }  catch (const std::exception& ex) {
+        isc_throw(DhcpConfigError, ex.what() << " ("
+                  << client_config->getPosition() << ")");
+    }
 }
 
 isc::dhcp::ParserPtr

+ 158 - 82
src/lib/dhcpsrv/dhcp_parsers.h

@@ -51,86 +51,127 @@ typedef boost::shared_ptr<std::vector<std::string> > HooksLibsStoragePtr;
 
 /// @brief A template class that stores named elements of a given data type.
 ///
-/// This template class is provides data value storage for configuration parameters
-/// of a given data type.  The values are stored by parameter name and as instances
-/// of type "ValueType".
+/// This template class is provides data value storage for configuration
+/// parameters of a given data type.  The values are stored by parameter name
+/// and as instances of type "ValueType". Each value held in the storage has
+/// a corresponding position within a configuration string (file) specified
+/// as a: file name, line number and position within the line. The position
+/// information is used for logging when the particular configuration value
+/// causes a configuration error.
 ///
-/// @param ValueType is the data type of the elements to store.
+/// @tparam ValueType is the data type of the elements to store.
 template<typename ValueType>
 class ValueStorage {
-    public:
-        /// @brief  Stores the the parameter and its value in the store.
-        ///
-        /// If the parameter does not exist in the store, then it will be added,
-        /// otherwise its data value will be updated with the given value.
-        ///
-        /// @param name is the name of the paramater to store.
-        /// @param value is the data value to store.
-        void setParam(const std::string& name, const ValueType& value) {
-            values_[name] = value;
-        }
+public:
+    /// @brief  Stores the the parameter, its value and the position in the
+    /// store.
+    ///
+    /// If the parameter does not exist in the store, then it will be added,
+    /// otherwise its data value and the position will be updated with the
+    /// given values.
+    ///
+    /// @param name is the name of the paramater to store.
+    /// @param value is the data value to store.
+    /// @param position is the position of the data element within a
+    /// configuration string (file).
+    void setParam(const std::string& name, const ValueType& value,
+                  const data::Element::Position& position) {
+        values_[name] = value;
+        positions_[name] = position;
+    }
 
-        /// @brief Returns the data value for the given parameter.
-        ///
-        /// Finds and returns the data value for the given parameter.
-        /// @param name is the name of the parameter for which the data
-        /// value is desired.
-        ///
-        /// @return The paramater's data value of type @c ValueType.
-        /// @throw DhcpConfigError if the parameter is not found.
-        ValueType getParam(const std::string& name) const {
-            typename std::map<std::string, ValueType>::const_iterator param
-                = values_.find(name);
-
-            if (param == values_.end()) {
-                isc_throw(DhcpConfigError, "Missing parameter '"
-                       << name << "'");
-            }
-
-            return (param->second);
-        }
+    /// @brief Returns the data value for the given parameter.
+    ///
+    /// Finds and returns the data value for the given parameter.
+    /// @param name is the name of the parameter for which the data
+    /// value is desired.
+    ///
+    /// @return The paramater's data value of type @c ValueType.
+    /// @throw DhcpConfigError if the parameter is not found.
+    ValueType getParam(const std::string& name) const {
+        typename std::map<std::string, ValueType>::const_iterator param
+            = values_.find(name);
 
-        /// @brief Returns the data value for an optional parameter.
-        ///
-        /// Finds and returns the data value for the given parameter or
-        /// a supplied default value if it is not found.
-        ///
-        /// @param name is the name of the parameter for which the data
-        /// value is desired.
-        /// @param default_value value to use the default
-        ///
-        /// @return The paramater's data value of type @c ValueType.
-        ValueType getOptionalParam(const std::string& name,
-                                   const ValueType& default_value) const {
-            typename std::map<std::string, ValueType>::const_iterator param
-                = values_.find(name);
-
-            if (param == values_.end()) {
-                return (default_value);
-            }
-
-            return (param->second);
+        if (param == values_.end()) {
+            isc_throw(DhcpConfigError, "Missing parameter '"
+                      << name << "'");
         }
 
-        /// @brief  Remove the parameter from the store.
-        ///
-        /// Deletes the entry for the given parameter from the store if it
-        /// exists.
-        ///
-        /// @param name is the name of the paramater to delete.
-        void delParam(const std::string& name) {
-            values_.erase(name);
+        return (param->second);
+    }
+
+    /// @brief Returns position of the data element in the configuration string.
+    ///
+    /// The returned object comprises file name, line number and the position
+    /// within the particular line of the configuration string where the data
+    /// element holding a particular value is located.
+    ///
+    /// @param name is the name of the parameter which position is desired.
+    ///
+    /// @return Position of the data element or the position holding empty
+    /// file name and two zeros if the position hasn't been specified for the
+    /// particular value.
+    const data::Element::Position& getPosition(const std::string& name) const {
+        typename std::map<std::string, data::Element::Position>::const_iterator
+            pos = positions_.find(name);
+        if (pos == positions_.end()) {
+            return (data::Element::ZERO_POSITION());
         }
 
-        /// @brief Deletes all of the entries from the store.
-        ///
-        void clear() {
-            values_.clear();
+        return (pos->second);
+    }
+
+    /// @brief Returns the data value for an optional parameter.
+    ///
+    /// Finds and returns the data value for the given parameter or
+    /// a supplied default value if it is not found.
+    ///
+    /// @param name is the name of the parameter for which the data
+    /// value is desired.
+    /// @param default_value value to use the default
+    ///
+    /// @return The paramater's data value of type @c ValueType.
+    ValueType getOptionalParam(const std::string& name,
+                               const ValueType& default_value) const {
+        typename std::map<std::string, ValueType>::const_iterator param
+            = values_.find(name);
+
+        if (param == values_.end()) {
+            return (default_value);
         }
 
-    private:
-        /// @brief An std::map of the data values, keyed by parameter names.
-        std::map<std::string, ValueType> values_;
+        return (param->second);
+    }
+
+    /// @brief  Remove the parameter from the store.
+    ///
+    /// Deletes the entry for the given parameter from the store if it
+    /// exists.
+    ///
+    /// @param name is the name of the paramater to delete.
+    void delParam(const std::string& name) {
+        values_.erase(name);
+        positions_.erase(name);
+    }
+
+    /// @brief Deletes all of the entries from the store.
+    ///
+    void clear() {
+        values_.clear();
+        positions_.clear();
+    }
+
+private:
+    /// @brief An std::map of the data values, keyed by parameter names.
+    std::map<std::string, ValueType> values_;
+
+    /// @brief An std::map holding positions of the data elements in the
+    /// configuration, which values are held in @c values_.
+    ///
+    /// The position is used for logging, when the particular value
+    /// causes a configuration error.
+    std::map<std::string, data::Element::Position> positions_;
+
 };
 
 
@@ -237,7 +278,7 @@ public:
     /// @throw isc::dhcp::DhcpConfigError if storage is null.
     ValueParser(const std::string& param_name,
         boost::shared_ptr<ValueStorage<ValueType> > storage)
-        : storage_(storage), param_name_(param_name), value_() {
+        : storage_(storage), param_name_(param_name), value_(), pos_() {
         // Empty parameter name is invalid.
         if (param_name_.empty()) {
             isc_throw(isc::dhcp::DhcpConfigError, "parser logic error:"
@@ -251,7 +292,6 @@ public:
         }
     }
 
-
     /// @brief Parse a given element into a value of type @c ValueType
     ///
     /// @param value a value to be parsed.
@@ -264,10 +304,23 @@ public:
     void commit() {
         // If a given parameter already exists in the storage we override
         // its value. If it doesn't we insert a new element.
-        storage_->setParam(param_name_, value_);
+        storage_->setParam(param_name_, value_, pos_);
     }
 
 private:
+
+    /// @brief Performs operations common for all specializations of the
+    /// @c build function.
+    ///
+    /// This method should be called by all specializations of the @c build
+    /// method.
+    ///
+    /// @param value a value being parsed.
+    void buildCommon(isc::data::ConstElementPtr value) {
+        // Remember position of the data element.
+        pos_ = value->getPosition();
+    }
+
     /// Pointer to the storage where committed value is stored.
     boost::shared_ptr<ValueStorage<ValueType> > storage_;
 
@@ -276,6 +329,8 @@ private:
 
     /// Parsed value.
     ValueType value_;
+
+    data::Element::Position pos_;
 };
 
 /// @brief typedefs for simple data type parsers
@@ -535,9 +590,12 @@ private:
     /// is intitialized but this check is not needed here because it is done
     /// in the \ref build function.
     ///
+    /// @param option_data An element holding data for a single option being
+    /// created.
+    ///
     /// @throw DhcpConfigError if parameters provided in the configuration
     /// are invalid.
-    void createOption();
+    void createOption(isc::data::ConstElementPtr option_data);
 
     /// Storage for boolean values.
     BooleanStoragePtr boolean_values_;
@@ -635,8 +693,11 @@ public:
     /// accept string as first argument.
     /// @param storage is the definition storage in which to store the parsed
     /// definition upon "commit".
+    /// @param global_context is a pointer to the global context which
+    /// stores global scope parameters, options, option defintions.
     /// @throw isc::dhcp::DhcpConfigError if storage is null.
-    OptionDefParser(const std::string& dummy, OptionDefStoragePtr storage);
+    OptionDefParser(const std::string& dummy, OptionDefStoragePtr storage,
+                    ParserContextPtr global_context);
 
     /// @brief Parses an entry that describes single option definition.
     ///
@@ -651,7 +712,10 @@ public:
 private:
 
     /// @brief Create option definition from the parsed parameters.
-    void createOptionDef();
+    ///
+    /// @param option_def_element A data element holding the configuration
+    /// for an option definition.
+    void createOptionDef(isc::data::ConstElementPtr option_def_element);
 
     /// Instance of option definition being created by this parser.
     OptionDefinitionPtr option_definition_;
@@ -670,6 +734,10 @@ private:
 
     /// Storage for uint32 values.
     Uint32StoragePtr uint32_values_;
+
+    /// Parsing context which contains global values, options and option
+    /// definitions.
+    ParserContextPtr global_context_;
 };
 
 /// @brief Parser for a list of option definitions.
@@ -684,27 +752,35 @@ public:
     ///
     /// @param dummy first argument is ignored, all Parser constructors
     /// accept string as first argument.
-    /// @param storage is the definition storage in which to store the parsed
-    /// definitions in this list
+    /// @param global_context is a pointer to the global context which
+    /// stores global scope parameters, options, option defintions.
     /// @throw isc::dhcp::DhcpConfigError if storage is null.
-    OptionDefListParser(const std::string& dummy, OptionDefStoragePtr storage);
+    OptionDefListParser(const std::string& dummy,
+                        ParserContextPtr global_context);
 
     /// @brief Parse configuration entries.
     ///
-    /// This function parses configuration entries and creates instances
-    /// of option definitions.
+    /// This function parses configuration entries, creates instances
+    /// of option definitions and tries to add them to the Configuration
+    /// Manager.
     ///
     /// @param option_def_list pointer to an element that holds entries
     /// that define option definitions.
     /// @throw DhcpConfigError if configuration parsing fails.
     void build(isc::data::ConstElementPtr option_def_list);
 
-    /// @brief Stores option definitions in the CfgMgr.
+    /// @brief Commits option definitions.
+    ///
+    /// Currently this function is no-op, because option definitions are
+    /// added to the Configuration Manager in the @c build method.
     void commit();
 
-private:
     /// @brief storage for option definitions.
     OptionDefStoragePtr storage_;
+
+    /// Parsing context which contains global values, options and option
+    /// definitions.
+    ParserContextPtr global_context_;
 };
 
 /// @brief a collection of pools

+ 1 - 0
src/lib/dhcpsrv/tests/Makefile.am

@@ -107,6 +107,7 @@ libdhcpsrv_unittests_CXXFLAGS += -Wno-unused-variable -Wno-unused-parameter
 endif
 
 libdhcpsrv_unittests_LDADD  = $(top_builddir)/src/lib/dhcpsrv/libkea-dhcpsrv.la
+libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/dhcpsrv/testutils/libdhcpsrvtest.la
 libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/dhcp/tests/libdhcptest.la
 libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libkea-dhcp++.la
 libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/dhcp_ddns/libkea-dhcp_ddns.la

+ 126 - 28
src/lib/dhcpsrv/tests/cfgmgr_unittest.cc

@@ -29,6 +29,7 @@
 
 using namespace std;
 using namespace isc::asiolink;
+using namespace isc::data;
 using namespace isc::dhcp;
 using namespace isc::dhcp::test;
 using namespace isc::util;
@@ -40,34 +41,67 @@ using boost::scoped_ptr;
 
 namespace {
 
+template <typename Storage>
+bool isZeroPosition(const Storage& storage, const std::string& param_name) {
+    Element::Position position = storage.getPosition(param_name);
+    return ((position.line_ == 0) && (position.pos_ == 0) &&
+            (position.file_.empty()));
+}
+
 // This test verifies that BooleanStorage functions properly.
 TEST(ValueStorageTest, BooleanTesting) {
     BooleanStorage testStore;
 
     // Verify that we can add and retrieve parameters.
-    testStore.setParam("firstBool", false);
-    testStore.setParam("secondBool", true);
+    testStore.setParam("firstBool", false, Element::Position("kea.conf", 123, 234));
+    testStore.setParam("secondBool", true, Element::Position("keax.conf", 10, 20));
 
     EXPECT_FALSE(testStore.getParam("firstBool"));
     EXPECT_TRUE(testStore.getParam("secondBool"));
 
+    EXPECT_EQ(123, testStore.getPosition("firstBool").line_);
+    EXPECT_EQ(234, testStore.getPosition("firstBool").pos_);
+    EXPECT_EQ("kea.conf", testStore.getPosition("firstBool").file_);
+
+    EXPECT_EQ(10, testStore.getPosition("secondBool").line_);
+    EXPECT_EQ(20, testStore.getPosition("secondBool").pos_);
+    EXPECT_EQ("keax.conf", testStore.getPosition("secondBool").file_);
+
     // Verify that we can update parameters.
-    testStore.setParam("firstBool", true);
-    testStore.setParam("secondBool", false);
+    testStore.setParam("firstBool", true, Element::Position("keax.conf", 555, 111));
+    testStore.setParam("secondBool", false, Element::Position("kea.conf", 1, 3));
 
     EXPECT_TRUE(testStore.getParam("firstBool"));
     EXPECT_FALSE(testStore.getParam("secondBool"));
 
+    EXPECT_EQ(555, testStore.getPosition("firstBool").line_);
+    EXPECT_EQ(111, testStore.getPosition("firstBool").pos_);
+    EXPECT_EQ("keax.conf", testStore.getPosition("firstBool").file_);
+
+    EXPECT_EQ(1, testStore.getPosition("secondBool").line_);
+    EXPECT_EQ(3, testStore.getPosition("secondBool").pos_);
+    EXPECT_EQ("kea.conf", testStore.getPosition("secondBool").file_);
+
     // Verify that we can delete a parameter and it will no longer be found.
     testStore.delParam("firstBool");
     EXPECT_THROW(testStore.getParam("firstBool"), isc::dhcp::DhcpConfigError);
 
+    // Verify that the "zero" position is returned when parameter doesn't exist.
+    EXPECT_TRUE(isZeroPosition(testStore, "firstBool"));
+
     // Verify that the delete was safe and the store still operates.
     EXPECT_FALSE(testStore.getParam("secondBool"));
 
+    EXPECT_EQ(1, testStore.getPosition("secondBool").line_);
+    EXPECT_EQ(3, testStore.getPosition("secondBool").pos_);
+    EXPECT_EQ("kea.conf", testStore.getPosition("secondBool").file_);
+
     // Verify that looking for a parameter that never existed throws.
     ASSERT_THROW(testStore.getParam("bogusBool"), isc::dhcp::DhcpConfigError);
 
+    // Verify that the "zero" position is returned when parameter doesn't exist.
+    EXPECT_TRUE(isZeroPosition(testStore, "bogusBool"));
+
     // Verify that attempting to delete a parameter that never existed does not throw.
     EXPECT_NO_THROW(testStore.delParam("bogusBool"));
 
@@ -75,35 +109,60 @@ TEST(ValueStorageTest, BooleanTesting) {
     testStore.clear();
     EXPECT_THROW(testStore.getParam("secondBool"), isc::dhcp::DhcpConfigError);
 
+    // Verify that the "zero" position is returned when parameter doesn't exist.
+    EXPECT_TRUE(isZeroPosition(testStore, "secondBool"));
 }
 
 // This test verifies that Uint32Storage functions properly.
 TEST(ValueStorageTest, Uint32Testing) {
     Uint32Storage testStore;
 
-    uint32_t intOne = 77;
-    uint32_t intTwo = 33;
+    uint32_t int_one = 77;
+    uint32_t int_two = 33;
 
     // Verify that we can add and retrieve parameters.
-    testStore.setParam("firstInt", intOne);
-    testStore.setParam("secondInt", intTwo);
+    testStore.setParam("firstInt", int_one, Element::Position("kea.conf", 123, 234));
+    testStore.setParam("secondInt", int_two, Element::Position("keax.conf", 10, 20));
+
+    EXPECT_EQ(testStore.getParam("firstInt"), int_one);
+    EXPECT_EQ(testStore.getParam("secondInt"), int_two);
 
-    EXPECT_EQ(testStore.getParam("firstInt"), intOne);
-    EXPECT_EQ(testStore.getParam("secondInt"), intTwo);
+    EXPECT_EQ(123, testStore.getPosition("firstInt").line_);
+    EXPECT_EQ(234, testStore.getPosition("firstInt").pos_);
+    EXPECT_EQ("kea.conf", testStore.getPosition("firstInt").file_);
+
+    EXPECT_EQ(10, testStore.getPosition("secondInt").line_);
+    EXPECT_EQ(20, testStore.getPosition("secondInt").pos_);
+    EXPECT_EQ("keax.conf", testStore.getPosition("secondInt").file_);
 
     // Verify that we can update parameters.
-    testStore.setParam("firstInt", --intOne);
-    testStore.setParam("secondInt", ++intTwo);
+    testStore.setParam("firstInt", --int_one, Element::Position("keax.conf", 555, 111));
+    testStore.setParam("secondInt", ++int_two, Element::Position("kea.conf", 1, 3));
+
+    EXPECT_EQ(testStore.getParam("firstInt"), int_one);
+    EXPECT_EQ(testStore.getParam("secondInt"), int_two);
+
+    EXPECT_EQ(555, testStore.getPosition("firstInt").line_);
+    EXPECT_EQ(111, testStore.getPosition("firstInt").pos_);
+    EXPECT_EQ("keax.conf", testStore.getPosition("firstInt").file_);
 
-    EXPECT_EQ(testStore.getParam("firstInt"), intOne);
-    EXPECT_EQ(testStore.getParam("secondInt"), intTwo);
+    EXPECT_EQ(1, testStore.getPosition("secondInt").line_);
+    EXPECT_EQ(3, testStore.getPosition("secondInt").pos_);
+    EXPECT_EQ("kea.conf", testStore.getPosition("secondInt").file_);
 
     // Verify that we can delete a parameter and it will no longer be found.
     testStore.delParam("firstInt");
     EXPECT_THROW(testStore.getParam("firstInt"), isc::dhcp::DhcpConfigError);
 
+    // Verify that the "zero" position is returned when parameter doesn't exist.
+    EXPECT_TRUE(isZeroPosition(testStore, "firstInt"));
+
     // Verify that the delete was safe and the store still operates.
-    EXPECT_EQ(testStore.getParam("secondInt"), intTwo);
+    EXPECT_EQ(testStore.getParam("secondInt"), int_two);
+
+    EXPECT_EQ(1, testStore.getPosition("secondInt").line_);
+    EXPECT_EQ(3, testStore.getPosition("secondInt").pos_);
+    EXPECT_EQ("kea.conf", testStore.getPosition("secondInt").file_);
 
     // Verify that looking for a parameter that never existed throws.
     ASSERT_THROW(testStore.getParam("bogusInt"), isc::dhcp::DhcpConfigError);
@@ -111,41 +170,74 @@ TEST(ValueStorageTest, Uint32Testing) {
     // Verify that attempting to delete a parameter that never existed does not throw.
     EXPECT_NO_THROW(testStore.delParam("bogusInt"));
 
+    // Verify that the "zero" position is returned when parameter doesn't exist.
+    EXPECT_TRUE(isZeroPosition(testStore, "bogusInt"));
+
     // Verify that we can empty the list.
     testStore.clear();
     EXPECT_THROW(testStore.getParam("secondInt"), isc::dhcp::DhcpConfigError);
+
+    // Verify that the "zero" position is returned when parameter doesn't exist.
+    EXPECT_TRUE(isZeroPosition(testStore, "secondInt"));
 }
 
 // This test verifies that StringStorage functions properly.
 TEST(ValueStorageTest, StringTesting) {
     StringStorage testStore;
 
-    std::string stringOne = "seventy-seven";
-    std::string stringTwo = "thirty-three";
+    std::string string_one = "seventy-seven";
+    std::string string_two = "thirty-three";
 
     // Verify that we can add and retrieve parameters.
-    testStore.setParam("firstString", stringOne);
-    testStore.setParam("secondString", stringTwo);
+    testStore.setParam("firstString", string_one,
+                       Element::Position("kea.conf", 123, 234));
+    testStore.setParam("secondString", string_two,
+                       Element::Position("keax.conf", 10, 20));
+
+    EXPECT_EQ(testStore.getParam("firstString"), string_one);
+    EXPECT_EQ(testStore.getParam("secondString"), string_two);
+
+    EXPECT_EQ(123, testStore.getPosition("firstString").line_);
+    EXPECT_EQ(234, testStore.getPosition("firstString").pos_);
+    EXPECT_EQ("kea.conf", testStore.getPosition("firstString").file_);
 
-    EXPECT_EQ(testStore.getParam("firstString"), stringOne);
-    EXPECT_EQ(testStore.getParam("secondString"), stringTwo);
+    EXPECT_EQ(10, testStore.getPosition("secondString").line_);
+    EXPECT_EQ(20, testStore.getPosition("secondString").pos_);
+    EXPECT_EQ("keax.conf", testStore.getPosition("secondString").file_);
 
     // Verify that we can update parameters.
-    stringOne.append("-boo");
-    stringTwo.append("-boo");
+    string_one.append("-boo");
+    string_two.append("-boo");
 
-    testStore.setParam("firstString", stringOne);
-    testStore.setParam("secondString", stringTwo);
+    testStore.setParam("firstString", string_one,
+                       Element::Position("kea.conf", 555, 111));
+    testStore.setParam("secondString", string_two,
+                       Element::Position("keax.conf", 1, 3));
 
-    EXPECT_EQ(testStore.getParam("firstString"), stringOne);
-    EXPECT_EQ(testStore.getParam("secondString"), stringTwo);
+    EXPECT_EQ(testStore.getParam("firstString"), string_one);
+    EXPECT_EQ(testStore.getParam("secondString"), string_two);
+
+    EXPECT_EQ(555, testStore.getPosition("firstString").line_);
+    EXPECT_EQ(111, testStore.getPosition("firstString").pos_);
+    EXPECT_EQ("kea.conf", testStore.getPosition("firstString").file_);
+
+    EXPECT_EQ(1, testStore.getPosition("secondString").line_);
+    EXPECT_EQ(3, testStore.getPosition("secondString").pos_);
+    EXPECT_EQ("keax.conf", testStore.getPosition("secondString").file_);
 
     // Verify that we can delete a parameter and it will no longer be found.
     testStore.delParam("firstString");
     EXPECT_THROW(testStore.getParam("firstString"), isc::dhcp::DhcpConfigError);
 
+    // Verify that the "zero" position is returned when parameter doesn't exist.
+    EXPECT_TRUE(isZeroPosition(testStore, "firstString"));
+
     // Verify that the delete was safe and the store still operates.
-    EXPECT_EQ(testStore.getParam("secondString"), stringTwo);
+    EXPECT_EQ(testStore.getParam("secondString"), string_two);
+
+    EXPECT_EQ(1, testStore.getPosition("secondString").line_);
+    EXPECT_EQ(3, testStore.getPosition("secondString").pos_);
+    EXPECT_EQ("keax.conf", testStore.getPosition("secondString").file_);
 
     // Verify that looking for a parameter that never existed throws.
     ASSERT_THROW(testStore.getParam("bogusString"), isc::dhcp::DhcpConfigError);
@@ -153,9 +245,15 @@ TEST(ValueStorageTest, StringTesting) {
     // Verify that attempting to delete a parameter that never existed does not throw.
     EXPECT_NO_THROW(testStore.delParam("bogusString"));
 
+    // Verify that the "zero" position is returned when parameter doesn't exist.
+    EXPECT_TRUE(isZeroPosition(testStore, "bogusString"));
+
     // Verify that we can empty the list.
     testStore.clear();
     EXPECT_THROW(testStore.getParam("secondString"), isc::dhcp::DhcpConfigError);
+
+    // Verify that the "zero" position is returned when parameter doesn't exist.
+    EXPECT_TRUE(isZeroPosition(testStore, "secondString"));
 }
 
 

+ 116 - 14
src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -14,6 +14,7 @@
 
 #include <config.h>
 #include <config/ccsession.h>
+#include <cc/data.h>
 #include <dhcp/option.h>
 #include <dhcp/option_custom.h>
 #include <dhcp/option_int.h>
@@ -21,6 +22,7 @@
 #include <dhcpsrv/subnet.h>
 #include <dhcpsrv/dhcp_parsers.h>
 #include <dhcpsrv/tests/test_libraries.h>
+#include <dhcpsrv/testutils/config_result_check.h>
 #include <exceptions/exceptions.h>
 #include <hooks/hooks_manager.h>
 
@@ -36,6 +38,7 @@ using namespace isc;
 using namespace isc::config;
 using namespace isc::data;
 using namespace isc::dhcp;
+using namespace isc::dhcp::test;
 using namespace isc::hooks;
 
 namespace {
@@ -383,7 +386,7 @@ public:
 
         } else if (config_id.compare("option-def") == 0) {
             parser.reset(new OptionDefListParser(config_id,
-                                              parser_context_->option_defs_));
+                                                 parser_context_));
 
         } else if (config_id.compare("hooks-libraries") == 0) {
             parser.reset(new HooksLibrariesParser(config_id));
@@ -418,6 +421,11 @@ public:
             ConstElementPtr status = parseElementSet(json);
             ConstElementPtr comment = parseAnswer(rcode_, status);
             error_text_ = comment->stringValue();
+            // If error was reported, the error string should contain
+            // position of the data element which caused failure.
+            if (rcode_ != 0) {
+                EXPECT_TRUE(errorContainsPosition(status, "<string>"));
+            }
         }
 
         return (rcode_);
@@ -1053,6 +1061,32 @@ public:
         EXPECT_EQ(ref_values->getParam("foo"), values->getParam("foo"));
     }
 
+    /// @brief Check that the storages of the specific type hold the same
+    /// position.
+    ///
+    /// This function assumes that the @c ref_values storage holds exactly
+    /// one parameter called 'foo'.
+    ///
+    /// @param ref_values A storage holding reference position. In the typical
+    /// case it is a storage held in the original context, which is assigned
+    /// to another context.
+    /// @param values A storage holding position to be checked.
+    /// @tparam ContainerType A type of the storage.
+    /// @tparam ValueType A type of the value in the container.
+    template<typename ContainerType, typename ValueType>
+    void checkPositionEq(const boost::shared_ptr<ContainerType>& ref_values,
+                         const boost::shared_ptr<ContainerType>& values) {
+        // Verify that the position is correct.
+        EXPECT_EQ(ref_values->getPosition("foo").line_,
+                  values->getPosition("foo").line_);
+
+        EXPECT_EQ(ref_values->getPosition("foo").pos_,
+                  values->getPosition("foo").pos_);
+
+        EXPECT_EQ(ref_values->getPosition("foo").file_,
+                  values->getPosition("foo").file_);
+    }
+
     /// @brief Check that the storages of the specific type hold different
     /// value.
     ///
@@ -1072,6 +1106,30 @@ public:
         EXPECT_NE(ref_values->getParam("foo"), values->getParam("foo"));
     }
 
+    /// @brief Check that the storages of the specific type hold fifferent
+    /// position.
+    ///
+    /// This function assumes that the ref_values storage holds exactly
+    /// one parameter called 'foo'.
+    ///
+    /// @param ref_values A storage holding reference position. In the typical
+    /// case it is a storage held in the original context, which is assigned
+    /// to another context.
+    /// @param values A storage holding position to be checked.
+    /// @tparam ContainerType A type of the storage.
+    /// @tparam ValueType A type of the value in the container.
+    template<typename ContainerType, typename ValueType>
+    void checkPositionNeq(const boost::shared_ptr<ContainerType>& ref_values,
+                          const boost::shared_ptr<ContainerType>& values) {
+        // At least one of the position fields must be different.
+        EXPECT_TRUE((ref_values->getPosition("foo").line_ !=
+                     values->getPosition("foo").line_) ||
+                    (ref_values->getPosition("foo").pos_ !=
+                     values->getPosition("foo").pos_) ||
+                    (ref_values->getPosition("foo").pos_ !=
+                     values->getPosition("foo").pos_));
+    }
+
     /// @brief Check that option definition storage in the context holds
     /// one option definition of the specified type.
     ///
@@ -1146,15 +1204,18 @@ public:
 
         // Set boolean parameter 'foo'.
         ASSERT_TRUE(ctx.boolean_values_);
-        ctx.boolean_values_->setParam("foo", true);
+        ctx.boolean_values_->setParam("foo", true,
+                                      Element::Position("kea.conf", 123, 234));
 
         // Set uint32 parameter 'foo'.
         ASSERT_TRUE(ctx.uint32_values_);
-        ctx.uint32_values_->setParam("foo", 123);
+        ctx.uint32_values_->setParam("foo", 123,
+                                     Element::Position("kea.conf", 123, 234));
 
         // Ser string parameter 'foo'.
         ASSERT_TRUE(ctx.string_values_);
-        ctx.string_values_->setParam("foo", "some string");
+        ctx.string_values_->setParam("foo", "some string",
+                                     Element::Position("kea.conf", 123, 234));
 
         // Add new option, with option code 10, to the context.
         ASSERT_TRUE(ctx.options_);
@@ -1191,6 +1252,14 @@ public:
                                                ctx_new->boolean_values_);
         }
 
+        // New context has the same boolean value position.
+        {
+            SCOPED_TRACE("Check that positions of boolean values are equal"
+                         " in both contexts");
+            checkPositionEq<BooleanStorage, bool>(ctx.boolean_values_,
+                                                  ctx_new->boolean_values_);
+        }
+
         // New context has the same uint32 value.
         ASSERT_TRUE(ctx_new->uint32_values_);
         {
@@ -1200,6 +1269,14 @@ public:
                                                   ctx_new->uint32_values_);
         }
 
+        // New context has the same uint32 value position.
+        {
+            SCOPED_TRACE("Check that positions of uint32_t values are equal"
+                         " in both contexts");
+            checkPositionEq<Uint32Storage, uint32_t>(ctx.uint32_values_,
+                                                     ctx_new->uint32_values_);
+        }
+
         // New context has the same string value.
         ASSERT_TRUE(ctx_new->string_values_);
         {
@@ -1208,6 +1285,14 @@ public:
                                                      ctx_new->string_values_);
         }
 
+        // New context has the same string value position.
+        {
+            SCOPED_TRACE("Check that position of string values are equal"
+                         " in both contexts");
+            checkPositionEq<StringStorage, std::string>(ctx.string_values_,
+                                                        ctx_new->string_values_);
+        }
+
         // New context has the same option.
         ASSERT_TRUE(ctx_new->options_);
         {
@@ -1237,31 +1322,49 @@ public:
         // Change the value of the boolean parameter. This should not affect the
         // corresponding value in the new context.
         {
-            SCOPED_TRACE("Check that boolean value isn't changed when original"
-                         " value is changed");
-            ctx.boolean_values_->setParam("foo", false);
+            SCOPED_TRACE("Check that boolean value and position isn't changed"
+                         " when original value and position is changed");
+            ctx.boolean_values_->setParam("foo", false,
+                                          Element::Position("kea.conf",
+                                                            12, 10));
             checkValueNeq<BooleanStorage, bool>(ctx.boolean_values_,
                                                 ctx_new->boolean_values_);
+
+            checkPositionNeq<BooleanStorage, bool>(ctx.boolean_values_,
+                                                   ctx_new->boolean_values_);
         }
 
         // Change the value of the uint32_t parameter. This should not affect
         // the corresponding value in the new context.
         {
-            SCOPED_TRACE("Check that uint32_t value isn't changed when original"
-                         " value is changed");
-            ctx.uint32_values_->setParam("foo", 987);
+            SCOPED_TRACE("Check that uint32_t value and position isn't changed"
+                         " when original value and position is changed");
+            ctx.uint32_values_->setParam("foo", 987,
+                                         Element::Position("kea.conf",
+                                                           10, 11));
             checkValueNeq<Uint32Storage, uint32_t>(ctx.uint32_values_,
                                                    ctx_new->uint32_values_);
+
+            checkPositionNeq<Uint32Storage, uint32_t>(ctx.uint32_values_,
+                                                      ctx_new->uint32_values_);
+
         }
 
         // Change the value of the string parameter. This should not affect the
         // corresponding value in the new context.
         {
-            SCOPED_TRACE("Check that string value isn't changed when original"
-                         " value is changed");
-            ctx.string_values_->setParam("foo", "different string");
+            SCOPED_TRACE("Check that string value and position isn't changed"
+                         " when original value and position is changed");
+            ctx.string_values_->setParam("foo", "different string",
+                                         Element::Position("kea.conf",
+                                                           10, 11));
             checkValueNeq<StringStorage, std::string>(ctx.string_values_,
                                                       ctx_new->string_values_);
+
+            checkPositionNeq<
+                StringStorage, std::string>(ctx.string_values_,
+                                            ctx_new->string_values_);
+
         }
 
         // Change the option. This should not affect the option instance in the
@@ -1306,7 +1409,6 @@ public:
         EXPECT_EQ(Option::V6, ctx_new->universe_);
 
     }
-
 };
 
 // Check that the assignment operator of the ParserContext class copies all

+ 25 - 0
src/lib/dhcpsrv/testutils/Makefile.am

@@ -0,0 +1,25 @@
+SUBDIRS = .
+
+AM_CPPFLAGS = -I$(top_builddir)/src/lib -I$(top_srcdir)/src/lib
+AM_CPPFLAGS += $(BOOST_INCLUDES)
+
+AM_CXXFLAGS = $(B10_CXXFLAGS)
+
+if USE_STATIC_LINK
+AM_LDFLAGS = -static
+endif
+
+CLEANFILES = *.gcno *.gcda
+
+if HAVE_GTEST
+
+noinst_LTLIBRARIES = libdhcpsrvtest.la
+
+libdhcpsrvtest_la_SOURCES  = config_result_check.cc config_result_check.h
+libdhcpsrvtest_la_CXXFLAGS = $(AM_CXXFLAGS)
+libdhcpsrvtest_la_CPPFLAGS = $(AM_CPPFLAGS)
+libdhcpsrvtest_la_LDFLAGS  = $(AM_LDFLAGS)
+libdhcpsrvtest_la_LIBADD   = $(top_builddir)/src/lib/cc/libkea-cc.la
+libdhcpsrvtest_la_LIBADD  += $(top_builddir)/src/lib/log/libkea-log.la
+
+endif

+ 94 - 0
src/lib/dhcpsrv/testutils/config_result_check.cc

@@ -0,0 +1,94 @@
+// Copyright (C) 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
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include <cc/proto_defs.h>
+#include <dhcpsrv/testutils/config_result_check.h>
+#include <boost/algorithm/string/classification.hpp>
+#include <boost/algorithm/string/constants.hpp>
+#include <boost/algorithm/string/split.hpp>
+#include <string>
+
+namespace isc {
+namespace dhcp {
+namespace test {
+
+using namespace isc;
+using namespace isc::data;
+
+bool errorContainsPosition(ConstElementPtr error_element,
+                           const std::string& file_name) {
+    if (error_element->contains(isc::cc::CC_PAYLOAD_RESULT)) {
+        ConstElementPtr result = error_element->get(isc::cc::CC_PAYLOAD_RESULT);
+        if ((result->getType() != Element::list) || (result->size() < 2) ||
+            (result->get(1)->getType() != Element::string)) {
+            return (false);
+        }
+
+        // Get the error message in the textual format.
+        std::string error_string = result->get(1)->stringValue();
+
+        // The position of the data element causing an error has the following
+        // format: <filename>:<linenum>:<pos>. The <filename> has been specified
+        // by a caller, so let's first check if this file name is present in the
+        // error message.
+        size_t pos = error_string.find(file_name);
+
+        // If the file name is present, check that it is followed by the line
+        // number and position within the line.
+        if (pos != std::string::npos) {
+            // Split the string starting at the begining of the <filename>. It
+            // should return a vector of strings.
+            std::string sub = error_string.substr(pos);
+            std::vector<std::string> split_pos;
+            boost::split(split_pos, sub, boost::is_any_of(":"),
+                         boost::algorithm::token_compress_off);
+
+            // There should be at least three elements: <filename>, <linenum>
+            // and <pos>. There can be even more, because one error string may
+            // contain more positions of data elements for multiple
+            // configuration nesting levels. We want at least one position.
+            if ((split_pos.size() >= 3) && (split_pos[0] == file_name) &&
+                (!split_pos[1].empty()) && !(split_pos[2].empty())) {
+
+                // Make sure that the line number comprises only digits.
+                for (int i = 0; i < split_pos[1].size(); ++i) {
+                    if (!isdigit(split_pos[1][i])) {
+                        return (false);
+                    }
+                }
+
+                // Go over digits of the position within the line.
+                int i = 0;
+                while (isdigit(split_pos[2][i])) {
+                    ++i;
+                }
+
+                // Make sure that there has been at least one digit and that the
+                // position is followed by the paren.
+                if ((i == 0) || (split_pos[2][i] != ')')) {
+                    return (false);
+                }
+
+                // All checks passed.
+                return (true);
+            }
+        }
+    }
+
+    return (false);
+}
+
+}
+}
+}

+ 56 - 0
src/lib/dhcpsrv/testutils/config_result_check.h

@@ -0,0 +1,56 @@
+// Copyright (C) 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
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#ifndef CONFIG_RESULT_CHECK_H
+#define CONFIG_RESULT_CHECK_H
+
+#include <cc/data.h>
+
+namespace isc {
+namespace dhcp {
+namespace test {
+
+/// @brief Checks that the error string created by the configuration parsers
+/// contains the location of the data element...
+///
+/// This function checks that the error string returned by the configuration
+/// parsers contains the position of the element which caused an error. The
+/// error string is expected to contain at least one occurence of the following:
+///
+/// @code
+///     [filename]:[linenum]:[pos]
+/// @endcode
+///
+/// where:
+/// - [filename] is a configuration file name (provided by a caller),
+/// - [linenum] is a line number of the element,
+/// - [pos] is a position of the element within the line.
+///
+/// Both [linenum] and [pos] must contain decimal digits. The [filename]
+/// must match the name provided by the caller.
+///
+/// @param error_element A result returned by the configuration.
+/// @param file_name A configuration file name.
+///
+/// @return true if the provided configuration result comprises a string
+/// which holds a position of the data element which caused the error;
+/// false otherwise.
+bool errorContainsPosition(isc::data::ConstElementPtr error_element,
+                           const std::string& file_name);
+
+}
+}
+}
+
+#endif