[master] Merge branch 'trac5032' (mac-sources, control-socket, relay parsers)

# Conflicts:
#	src/lib/testutils/io_utils.cc
#	src/lib/testutils/io_utils.h

doc/examples/kea4/advanced.json

@@ -44,6 +44,14 @@
         "lfc-interval": 3600
     },
 
+    // This defines a control socket. If defined, Kea will open a UNIX socket
+    // and will listen for incoming commands. See section 15 of the Kea User's
+    // Guide for list of supported commands.
+    "control-socket": {
+        "socket-type": "unix",
+        "socket-name": "/tmp/kea4-ctrl-socket"
+    },
+
     // Addresses will be assigned with a lifetime of 4000 seconds.
     // The client is told to start renewing after 1000 seconds. If the server
     // does not respond within 2000 seconds of the lease being granted, client
@@ -83,7 +91,14 @@
         },
         {
             "pools": [ { "pool": "192.0.4.1 - 192.0.4.254" } ],
-            "subnet": "192.0.4.0/24"
+            "subnet": "192.0.4.0/24",
+
+            // Sometimes the relay may use an IPv4 address that does not match
+            // the subnet. This is discouraged, but there are valid cases when it
+            // makes sense. One case is when there is a shared subnet.
+            "relay": {
+                "ip-address": "192.168.1.1"
+            }
         }
     ]
 },

doc/examples/kea6/advanced.json

@@ -1,77 +1,90 @@
-# This is an example configuration file for DHCPv6 server in Kea.
-# It attempts to showcase some of the more advanced features.
-# Topology wise, it's a basic scenario with one IPv6 subnet configured.
-# It is assumed that one subnet (2001:db8:1::/64) is available directly
-# over ethX interface.
-#
-# The following features are currently showcased here:
-# 1. Configuration of MAC/hardware address sources in DHCPv6
+// This is an example configuration file for DHCPv6 server in Kea.
+// It attempts to showcase some of the more advanced features.
+// Topology wise, it's a basic scenario with one IPv6 subnet configured.
+// It is assumed that one subnet (2001:db8:1::/64) is available directly
+// over ethX interface.
+//
+// The following features are currently showcased here:
+// 1. Configuration of MAC/hardware address sources in DHCPv6
+// 2. RSOO (Relay supplied options) - Some relays may insert options with the
+//    intention for the server to insert them into client directed messages.
+// 3. Control socket. Kea can open a socket and listen for incoming
+//    commands.
 
 { "Dhcp6":
 
 {
-# Kea is told to listen on ethX network interface only.
+  // Kea is told to listen on ethX network interface only.
   "interfaces-config": {
     "interfaces": [ "ethX" ]
   },
 
-# We need to specify the the database used to store leases. As of
-# September 2016, four database backends are supported: MySQL,
-# PostgreSQL, Cassandra, and the in-memory database, Memfile.
-# We will use memfile  because it doesn't require any prior set up.
+  // We need to specify the the database used to store leases. As of
+  // September 2016, four database backends are supported: MySQL,
+  // PostgreSQL, Cassandra, and the in-memory database, Memfile.
+  // We will use memfile  because it doesn't require any prior set up.
   "lease-database": {
       "type": "memfile",
       "lfc-interval": 3600
   },
 
-# Kea 0.9.1 introduced MAC/hardware addresses support in DHCPv6. There is
-# no single reliable method of getting MAC address information in DHCPv6.
-# Kea supports several methods. Depending on your network set up, some
-# methods may be more preferable than others, hence the configuration
-# parameter. 'mac-sources' is a list of methods. Allowed parameters are:
-# any, raw, duid, ipv6-link-local, client-link-addr-option, rfc6939 (which
-# is an alias for client-link-addr-option), remote-id, rfc4649 (which is an
-# alias for remote-id, subscriber-id, rfc4580 (which is an alias for
-# subscriber-id) and docsis.
-#
-# Note that the order matters. Methods are attempted one by one in the order
-# specified until hardware address is obtained. If you don't care which method
-# is used, using 'any' is marginally faster than enumerating them all.
-#
-# If mac-sources are not specified, a default value of 'any' is used.
+// Kea 0.9.1 introduced MAC/hardware addresses support in DHCPv6. There is
+// no single reliable method of getting MAC address information in DHCPv6.
+// Kea supports several methods. Depending on your network set up, some
+// methods may be more preferable than others, hence the configuration
+// parameter. 'mac-sources' is a list of methods. Allowed parameters are:
+// any, raw, duid, ipv6-link-local, client-link-addr-option, rfc6939 (which
+// is an alias for client-link-addr-option), remote-id, rfc4649 (which is an
+// alias for remote-id, subscriber-id, rfc4580 (which is an alias for
+// subscriber-id) and docsis.
+//
+// Note that the order matters. Methods are attempted one by one in the order
+// specified until hardware address is obtained. If you don't care which method
+// is used, using 'any' is marginally faster than enumerating them all.
+//
+// If mac-sources are not specified, a default value of 'any' is used.
   "mac-sources": [ "client-link-addr-option", "duid", "ipv6-link-local" ],
 
-# RFC6422 defines a mechanism called relay-supplied options option. The relay
-# agent may insert certain options that the server will echo back to the
-# client, if certain criteria are met. One condition is that the option must
-# be RSOO-enabled (i.e. allowed to be echoed back). IANA maintains a list
-# of those options here:
-# http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xhtml#options-relay-supplied
-# However, it is possible to allow the server to echo back additional options.
-# This entry marks options 110, 120 and 130 as RSOO-enabled.
+// RFC6422 defines a mechanism called relay-supplied options option. The relay
+// agent may insert certain options that the server will echo back to the
+// client, if certain criteria are met. One condition is that the option must
+// be RSOO-enabled (i.e. allowed to be echoed back). IANA maintains a list
+// of those options here:
+// http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xhtml#options-relay-supplied
+// However, it is possible to allow the server to echo back additional options.
+// This entry marks options 110, 120 and 130 as RSOO-enabled.
    "relay-supplied-options": [ "110", "120", "130" ],
 
-# Addresses will be assigned with preferred and valid lifetimes
-# being 3000 and 4000, respectively. Client is told to start
-# renewing after 1000 seconds. If the server does not respond
-# after 2000 seconds since the lease was granted, client is supposed
-# to start REBIND procedure (emergency renewal that allows switching
-# to a different server).
+
+    // This defines a control socket. If defined, Kea will open a UNIX socket
+    // and will listen for incoming commands. See section 15 of the Kea User's
+    // Guide for list of supported commands.
+    "control-socket": {
+        "socket-type": "unix",
+        "socket-name": "/tmp/kea6-ctrl-socket"
+    },
+
+// Addresses will be assigned with preferred and valid lifetimes
+// being 3000 and 4000, respectively. Client is told to start
+// renewing after 1000 seconds. If the server does not respond
+// after 2000 seconds since the lease was granted, client is supposed
+// to start REBIND procedure (emergency renewal that allows switching
+// to a different server).
   "preferred-lifetime": 3000,
   "valid-lifetime": 4000,
   "renew-timer": 1000,
   "rebind-timer": 2000,
 
-# The following list defines subnets. Each subnet consists of at
-# least subnet and pool entries.
+// The following list defines subnets. Each subnet consists of at
+// least subnet and pool entries.
   "subnet6": [
     {
       "pools": [ { "pool": "2001:db8:1::/80" } ],
 
-# This defines PD (prefix delegation) pools. In this case
-# we have only one pool. That consists of /64 prefixes
-# being delegated out of large /48 pool. Each delegated
-# prefix will contain an excluded-prefix option.
+// This defines PD (prefix delegation) pools. In this case
+// we have only one pool. That consists of /64 prefixes
+// being delegated out of large /48 pool. Each delegated
+// prefix will contain an excluded-prefix option.
       "pd-pools": [
       {
           "prefix": "2001:db8:abcd::",
@@ -82,13 +95,21 @@
       }
       ],
       "subnet": "2001:db8:1::/64",
-      "interface": "ethX"
+      "interface": "ethX",
+
+      // Sometimes the relay may use an odd IPv6 address that's not matching
+      // the subnet. This is discouraged, but there are valid cases when it
+      // makes sense. One case is when the relay has only link-local address
+      // and another is when there is a shared subnet scenario.
+      "relay": {
+          "ip-address": "3000::1"
+      }
     }
   ]
 },
 
-# The following configures logging. It assumes that messages with at least
-# informational level (info, warn, error and fatal) should be logged to stdout.
+// The following configures logging. It assumes that messages with at least
+// informational level (info, warn, error and fatal) should be logged to stdout.
 "Logging": {
     "loggers": [
         {

doc/guide/dhcp4-srv.xml

@@ -3203,6 +3203,9 @@ src/lib/dhcpsrv/cfg_host_operations.cc -->
 </screen>
       </para>
 
+      <para>If "relay" is specified, the "ip-address" parameter within
+      it is mandatory.</para>
+
     </section>
 
       <section id="dhcp4-srv-example-client-class-relay">

doc/guide/dhcp6-srv.xml

@@ -3410,6 +3410,9 @@ If not specified, the default value is:
 </screen>
       </para>
 
+      <para>If "relay" is specified, the "ip-address" parameter within
+      it is mandatory.</para>
+
     </section>
 
       <section id="dhcp6-client-class-relay">
@@ -3500,7 +3503,8 @@ If not specified, the default value is:
 
     When not specified, a special value of "any" is used, which
     instructs the server to attempt to use all the methods in sequence and use
-    value returned by the first one that succeeds.</para>
+    value returned by the first one that succeeds. If specified, it
+    has to have at least one value.</para>
 
     <para>Supported methods are:
     <itemizedlist>

src/bin/dhcp4/json_config_parser.cc

@@ -189,8 +189,7 @@ protected:
             parser = new StringParser(config_id, string_values_);
         } else if (config_id.compare("pools") == 0) {
             parser = new Pools4ListParser(config_id, pools_);
-        } else if (config_id.compare("relay") == 0) {
-            parser = new RelayInfoParser(config_id, relay_info_, Option::V4);
+            // relay has been converted to SimpleParser already.
         // option-data has been converted to SimpleParser already.
         } else if (config_id.compare("match-client-id") == 0) {
             parser = new BooleanParser(config_id, boolean_values_);
@@ -440,8 +439,7 @@ DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id,
         parser = new D2ClientConfigParser(config_id);
     } else if (config_id.compare("match-client-id") == 0) {
         parser = new BooleanParser(config_id, globalContext()->boolean_values_);
-    } else if (config_id.compare("control-socket") == 0) {
-        parser = new ControlSocketParser(config_id);
+    // control-socket has been converted to SimpleParser already.
     } else if (config_id.compare("expired-leases-processing") == 0) {
         parser = new ExpirationConfigParser();
     } else if (config_id.compare("client-classes") == 0) {
@@ -637,6 +635,13 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
                 continue;
             }
 
+            if (config_pair.first == "control-socket") {
+                ControlSocketParser parser;
+                SrvConfigPtr srv_cfg = CfgMgr::instance().getStagingCfg();
+                parser.parse(*srv_cfg, config_pair.second);
+                continue;
+            }
+
             ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first,
                                                            config_pair.second));
             LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PARSER_CREATED)

src/bin/dhcp4/tests/parser_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2016-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -7,14 +7,14 @@
 #include <gtest/gtest.h>
 #include <cc/data.h>
 #include <dhcp4/parser_context.h>
-#include <fstream>
-#include <cstdio>
-#include <exceptions/exceptions.h>
+#include <testutils/io_utils.h>
 
 using namespace isc::data;
 using namespace std;
 
-namespace {
+namespace isc {
+namespace dhcp {
+namespace test {
 
 /// @brief compares two JSON trees
 ///
@@ -128,6 +128,8 @@ TEST(ParserTest, keywordDhcp4) {
      testParser(txt, Parser4Context::PARSER_DHCP4);
 }
 
+// Tests if bash (#) comments are supported. That's the only comment type that
+// was supported by the old parser.
 TEST(ParserTest, bashComments) {
     string txt= "{ \"Dhcp4\": { \"interfaces-config\": {"
                 "  \"interfaces\": [ \"*\" ]"
@@ -146,7 +148,8 @@ TEST(ParserTest, bashComments) {
     testParser(txt, Parser4Context::PARSER_DHCP4, false);
 }
 
-TEST(ParserTest, cComments) {
+// Tests if C++ (//) comments can start anywhere, not just in the first line.
+TEST(ParserTest, cppComments) {
     string txt= "{ \"Dhcp4\": { \"interfaces-config\": {"
                 "  \"interfaces\": [ \"*\" ]"
                 "},\n"
@@ -161,6 +164,7 @@ TEST(ParserTest, cComments) {
     testParser(txt, Parser4Context::PARSER_DHCP4, false);
 }
 
+// Tests if bash (#) comments can start anywhere, not just in the first line.
 TEST(ParserTest, bashCommentsInline) {
     string txt= "{ \"Dhcp4\": { \"interfaces-config\": {"
                 "  \"interfaces\": [ \"*\" ]"
@@ -176,6 +180,7 @@ TEST(ParserTest, bashCommentsInline) {
     testParser(txt, Parser4Context::PARSER_DHCP4, false);
 }
 
+// Tests if multi-line C style comments are handled correctly.
 TEST(ParserTest, multilineComments) {
     string txt= "{ \"Dhcp4\": { \"interfaces-config\": {"
                 "  \"interfaces\": [ \"*\" ]"
@@ -193,89 +198,13 @@ TEST(ParserTest, multilineComments) {
     testParser(txt, Parser4Context::PARSER_DHCP4, false);
 }
 
-/// @brief removes comments from a JSON file
-///
-/// This is rather naive implementation, but it's probably sufficient for
-/// testing. It won't be able to pick any trickier cases, like # or //
-/// appearing in strings, nested C++ comments etc.
-///
-/// @param input_file file to be stripped of comments
-/// @return a new file that has comments stripped from it
-std::string decommentJSONfile(const std::string& input_file) {
-    ifstream f(input_file);
-    if (!f.is_open()) {
-        isc_throw(isc::BadValue, "can't open input file for reading: " + input_file);
-    }
-
-    string outfile;
-    size_t last_slash = input_file.find_last_of("/");
-    if (last_slash != string::npos) {
-        outfile = input_file.substr(last_slash + 1);
-    } else {
-        outfile = input_file;
-    }
-    outfile += "-decommented";
-
-    ofstream out(outfile);
-    if (!out.is_open()) {
-        isc_throw(isc::BadValue, "can't open output file for writing: " + input_file);
-    }
-
-    bool in_comment = false;
-    string line;
-    while (std::getline(f, line)) {
-        // First, let's get rid of the # comments
-        size_t hash_pos = line.find("#");
-        if (hash_pos != string::npos) {
-            line = line.substr(0, hash_pos);
-        }
-
-        // Second, let's get rid of the // comments
-        size_t dblslash_pos = line.find("//");
-        if (dblslash_pos != string::npos) {
-            line = line.substr(0, dblslash_pos);
-        }
-
-        // Now the tricky part: c comments.
-        size_t begin_pos = line.find("/*");
-        size_t end_pos = line.find("*/");
-        if (in_comment && end_pos == string::npos) {
-            // we continue through multiline comment
-            line = "";
-        } else {
-
-            if (begin_pos != string::npos) {
-                in_comment = true;
-                if (end_pos != string::npos) {
-                    // sigle line comment. Let's get rid of the content in between
-                    line = line.replace(begin_pos, end_pos + 2, end_pos + 2 - begin_pos, ' ');
-                    in_comment = false;
-                } else {
-                    line = line.substr(0, begin_pos);
-                }
-            } else {
-                if (in_comment && end_pos != string::npos) {
-                    line = line.replace(0, end_pos +2 , end_pos + 2, ' ');
-                    in_comment = false;
-                }
-            }
-        }
-
-        // Finally, write the line to the output file.
-        out << line << endl;
-    }
-    f.close();
-    out.close();
-
-    return (outfile);
-}
 
 /// @brief Loads specified example config file
 ///
 /// This test loads specified example file twice: first, using the legacy
 /// JSON file and then second time using bison parser. Two created Element
 /// trees are then compared. The input is decommented before it is passed
-/// to legacy parser (as its support for comments is very limited).
+/// to legacy parser (as legacy support for comments is very limited).
 ///
 /// @param fname name of the file to be loaded
 void testFile(const std::string& fname) {
@@ -284,8 +213,7 @@ void testFile(const std::string& fname) {
 
     string decommented = decommentJSONfile(fname);
 
-    cout << "Attempting to load file " << fname << " (" << decommented
-         << ")" << endl;
+    cout << "Parsing file " << fname << " (" << decommented << ")" << endl;
 
     EXPECT_NO_THROW(reference_json = Element::fromJSONFile(decommented, true));
 
@@ -329,6 +257,11 @@ TEST(ParserTest, file) {
     }
 }
 
+/// @brief Tests error conditions in Dhcp4Parser
+///
+/// @param txt text to be parsed
+/// @param parser_type type of the parser to be used in the test
+/// @param msg expected content of the exception
 void testError(const std::string& txt,
                Parser4Context::ParserType parser_type,
                const std::string& msg)
@@ -347,7 +280,7 @@ void testError(const std::string& txt,
     }
 }
 
-// Check errors
+// Verify that error conditions are handled correctly.
 TEST(ParserTest, errors) {
     // no input
     testError("", Parser4Context::PARSER_JSON,
@@ -591,7 +524,7 @@ TEST(ParserTest, unicodeEscapes) {
     }
 }
 
-// This test checks that all representations of a slash is recognized properly.
+// This test checks that all representations of a slash are recognized properly.
 TEST(ParserTest, unicodeSlash) {
     // check the 4 possible encodings of solidus '/'
     ConstElementPtr result;
@@ -609,3 +542,5 @@ TEST(ParserTest, unicodeSlash) {
 }
 
 };
+};
+};

src/bin/dhcp6/json_config_parser.cc

@@ -424,8 +424,7 @@ protected:
             parser = new StringParser(config_id, string_values_);
         } else if (config_id.compare("pools") == 0) {
             parser = new Pools6ListParser(config_id, pools_);
-        } else if (config_id.compare("relay") == 0) {
-            parser = new RelayInfoParser(config_id, relay_info_, Option::V6);
+        // relay has been converted to SimpleParser.
         } else if (config_id.compare("pd-pools") == 0) {
             parser = new PdPoolListParser(config_id, pools_);
         // option-data was here, but it is now converted to SimpleParser
@@ -718,13 +717,10 @@ DhcpConfigParser* createGlobal6DhcpConfigParser(const std::string& config_id,
         parser = new HooksLibrariesParser(config_id);
     } else if (config_id.compare("dhcp-ddns") == 0) {
         parser = new D2ClientConfigParser(config_id);
-    } else if (config_id.compare("mac-sources") == 0) {
-        parser = new MACSourcesListConfigParser(config_id,
-                                                globalContext());
+    // mac-source has been converted to SimpleParser.
     } else if (config_id.compare("relay-supplied-options") == 0) {
         parser = new RSOOListConfigParser(config_id);
-    } else if (config_id.compare("control-socket") == 0) {
-        parser = new ControlSocketParser(config_id);
+    // control-socket has been converted to SimpleParser.
     } else if (config_id.compare("expired-leases-processing") == 0) {
         parser = new ExpirationConfigParser();
     } else if (config_id.compare("client-classes") == 0) {
@@ -911,6 +907,20 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
                 continue;
             }
 
+            if (config_pair.first == "mac-sources") {
+                MACSourcesListConfigParser parser;
+                CfgMACSource& mac_source = CfgMgr::instance().getStagingCfg()->getMACSources();
+                parser.parse(mac_source, config_pair.second);
+                continue;
+            }
+
+            if (config_pair.first == "control-socket") {
+                ControlSocketParser parser;
+                SrvConfigPtr srv_config = CfgMgr::instance().getStagingCfg();
+                parser.parse(*srv_config, config_pair.second);
+                continue;
+            }
+
             ParserPtr parser(createGlobal6DhcpConfigParser(config_pair.first,
                                                            config_pair.second));
             LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL, DHCP6_PARSER_CREATED)

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -251,6 +251,9 @@ public:
         ASSERT_TRUE(status);
         comment_ = parseAnswer(rcode_, status);
         EXPECT_EQ(expected_code, rcode_);
+        if (expected_code != rcode_) {
+            cout << "The comment returned was: [" << comment_->stringValue() << "]" << endl;
+        }
     }
 
     /// @brief Returns an interface configuration used by the most of the
@@ -762,7 +765,7 @@ TEST_F(Dhcp6ParserTest, emptySubnet) {
 
     ConstElementPtr status;
     EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json));
-                    
+
     // returned value should be 0 (success)
     checkResult(status, 0);
 }
@@ -4292,16 +4295,15 @@ TEST_F(Dhcp6ParserTest, reservationBogus) {
 }
 
 /// The goal of this test is to verify that configuration can include
-/// MAC/Hardware sources. This test also checks if the aliases are
-/// handled properly (rfc6939 = client-addr-relay, rfc4649 = remote-id,
-/// rfc4580 = subscriber-id).
-TEST_F(Dhcp6ParserTest, macSources) {
+/// MAC/Hardware sources. This case uses RFC numbers to reference methods.
+/// Also checks if the aliases are handled properly (rfc6939 = client-addr-relay,
+/// rfc4649 = remote-id, rfc4580 = subscriber-id).
+TEST_F(Dhcp6ParserTest, macSources1) {
 
     ConstElementPtr json;
     ASSERT_NO_THROW(json =
         parseDHCP6("{ " + genIfaceConfig() + ","
-                   "\"mac-sources\": [ \"rfc6939\", \"rfc4649\", \"rfc4580\","
-                   "\"client-link-addr-option\", \"remote-id\", \"subscriber-id\"],"
+                   "\"mac-sources\": [ \"rfc6939\", \"rfc4649\", \"rfc4580\" ],"
                    "\"preferred-lifetime\": 3000,"
                    "\"rebind-timer\": 2000, "
                    "\"renew-timer\": 1000, "
@@ -4310,28 +4312,49 @@ TEST_F(Dhcp6ParserTest, macSources) {
 
     ConstElementPtr status;
     EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json));
+    checkResult(status, 0);
 
-    // returned value should be 0 (success)
+    CfgMACSources sources = CfgMgr::instance().getStagingCfg()->getMACSources().get();
+
+    ASSERT_EQ(3, sources.size());
+    // Let's check the aliases. They should be recognized to their base methods.
+    EXPECT_EQ(HWAddr::HWADDR_SOURCE_CLIENT_ADDR_RELAY_OPTION, sources[0]);
+    EXPECT_EQ(HWAddr::HWADDR_SOURCE_REMOTE_ID, sources[1]);
+    EXPECT_EQ(HWAddr::HWADDR_SOURCE_SUBSCRIBER_ID, sources[2]);
+}
+
+/// The goal of this test is to verify that configuration can include
+/// MAC/Hardware sources. This uses specific method names.
+TEST_F(Dhcp6ParserTest, macSources2) {
+
+    ConstElementPtr json;
+    ASSERT_NO_THROW(json =
+        parseDHCP6("{ " + genIfaceConfig() + ","
+                   "\"mac-sources\": [ \"client-link-addr-option\", \"remote-id\", "
+                   "                   \"subscriber-id\"],"
+                   "\"preferred-lifetime\": 3000,"
+                   "\"rebind-timer\": 2000, "
+                   "\"renew-timer\": 1000, "
+                   "\"subnet6\": [  ], "
+                   "\"valid-lifetime\": 4000 }"));
+
+    ConstElementPtr status;
+    EXPECT_NO_THROW(status = configureDhcp6Server(srv_, json));
     checkResult(status, 0);
 
-    CfgMACSources mac_sources = CfgMgr::instance().getStagingCfg()->getMACSources().get();
-    ASSERT_EQ(6, mac_sources.size());
+    CfgMACSources sources = CfgMgr::instance().getStagingCfg()->getMACSources().get();
+
+    ASSERT_EQ(3, sources.size());
     // Let's check the aliases. They should be recognized to their base methods.
-    EXPECT_EQ(HWAddr::HWADDR_SOURCE_CLIENT_ADDR_RELAY_OPTION, mac_sources[0]);
-    EXPECT_EQ(HWAddr::HWADDR_SOURCE_REMOTE_ID, mac_sources[1]);
-    EXPECT_EQ(HWAddr::HWADDR_SOURCE_SUBSCRIBER_ID, mac_sources[2]);
-
-    // Let's check if the actual methods are recognized properly.
-    EXPECT_EQ(HWAddr::HWADDR_SOURCE_CLIENT_ADDR_RELAY_OPTION, mac_sources[3]);
-    EXPECT_EQ(HWAddr::HWADDR_SOURCE_REMOTE_ID, mac_sources[4]);
-    EXPECT_EQ(HWAddr::HWADDR_SOURCE_SUBSCRIBER_ID, mac_sources[5]);
+    EXPECT_EQ(HWAddr::HWADDR_SOURCE_CLIENT_ADDR_RELAY_OPTION, sources[0]);
+    EXPECT_EQ(HWAddr::HWADDR_SOURCE_REMOTE_ID, sources[1]);
+    EXPECT_EQ(HWAddr::HWADDR_SOURCE_SUBSCRIBER_ID, sources[2]);
 }
 
-/// The goal of this test is to verify that MAC sources configuration can be
-/// empty.
-/// Note the Dhcp6 parser requires the list to NOT be empty?!
-TEST_F(Dhcp6ParserTest, macSourcesEmpty) {
 
+/// The goal of this test is to verify that empty MAC sources configuration
+/// is rejected. If specified, this has to have at least one value.
+TEST_F(Dhcp6ParserTest, macSourcesEmpty) {
     ConstElementPtr status;
 
     EXPECT_NO_THROW(status = configureDhcp6Server(srv_,
@@ -4343,11 +4366,9 @@ TEST_F(Dhcp6ParserTest, macSourcesEmpty) {
                               "\"subnet6\": [  ], "
                               "\"valid-lifetime\": 4000 }")));
 
-    // returned value should be 0 (success)
-    checkResult(status, 0);
-
-    CfgMACSources mac_sources = CfgMgr::instance().getStagingCfg()->getMACSources().get();
-    EXPECT_EQ(0, mac_sources.size());
+    // returned value should be 1 (failure), because the mac-sources must not
+    // be empty when specified.
+    checkResult(status, 1);
 }
 
 /// The goal of this test is to verify that MAC sources configuration can

src/bin/dhcp6/tests/parser_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2016-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -7,28 +7,42 @@
 #include <gtest/gtest.h>
 #include <cc/data.h>
 #include <dhcp6/parser_context.h>
+#include <testutils/io_utils.h>
 
 using namespace isc::data;
 using namespace std;
 
-namespace {
-
-void compareJSON(ConstElementPtr a, ConstElementPtr b, bool print = true) {
+namespace isc {
+namespace dhcp {
+namespace test {
+
+/// @brief compares two JSON trees
+///
+/// If differences are discovered, gtest failure is reported (using EXPECT_EQ)
+///
+/// @param a first to be compared
+/// @param b second to be compared
+void compareJSON(ConstElementPtr a, ConstElementPtr b) {
     ASSERT_TRUE(a);
     ASSERT_TRUE(b);
-    if (print) {
-        // std::cout << "JSON A: -----" << endl << a->str() << std::endl;
-        // std::cout << "JSON B: -----" << endl << b->str() << std::endl;
-        // cout << "---------" << endl << endl;
-    }
     EXPECT_EQ(a->str(), b->str());
 }
 
-void testParser(const std::string& txt, Parser6Context::ParserType parser_type) {
-    ElementPtr reference_json;
+/// @brief Tests if the input string can be parsed with specific parser
+///
+/// The input text will be passed to bison parser of specified type.
+/// Then the same input text is passed to legacy JSON parser and outputs
+/// from both parsers are compared. The legacy comparison can be disabled,
+/// if the feature tested is not supported by the old parser (e.g.
+/// new comment styles)
+///
+/// @param txt text to be compared
+/// @param parser_type bison parser type to be instantiated
+/// @param compare whether to compare the output with legacy JSON parser
+void testParser(const std::string& txt, Parser6Context::ParserType parser_type,
+    bool compare = true) {
     ConstElementPtr test_json;
 
-    ASSERT_NO_THROW(reference_json = Element::fromJSON(txt, true));
     ASSERT_NO_THROW({
             try {
                 Parser6Context ctx;
@@ -40,29 +54,16 @@ void testParser(const std::string& txt, Parser6Context::ParserType parser_type)
 
     });
 
+    if (!compare) {
+        return;
+    }
+
     // Now compare if both representations are the same.
+    ElementPtr reference_json;
+    ASSERT_NO_THROW(reference_json = Element::fromJSON(txt, true));
     compareJSON(reference_json, test_json);
 }
 
-void testParser2(const std::string& txt, Parser6Context::ParserType parser_type) {
-    ConstElementPtr test_json;
-
-    ASSERT_NO_THROW({
-            try {
-                Parser6Context ctx;
-                test_json = ctx.parseString(txt, parser_type);
-            } catch (const std::exception &e) {
-                cout << "EXCEPTION: " << e.what() << endl;
-                throw;
-            }
-    });
-    /// @todo: Implement actual validation here. since the original
-    /// Element::fromJSON does not support several comment types, we don't
-    /// have anything to compare with.
-    /// std::cout << "Original text:" << txt << endl;
-    /// std::cout << "Parsed text  :" << test_json->str() << endl;
-}
-
 TEST(ParserTest, mapInMap) {
     string txt = "{ \"xyzzy\": { \"foo\": 123, \"baz\": 456 } }";
     testParser(txt, Parser6Context::PARSER_JSON);
@@ -125,9 +126,11 @@ TEST(ParserTest, keywordDhcp6) {
                   "    \"subnet\": \"2001:db8:1::/48\", "
                   "    \"interface\": \"test\" } ],\n"
                    "\"valid-lifetime\": 4000 } }";
-     testParser2(txt, Parser6Context::PARSER_DHCP6);
+     testParser(txt, Parser6Context::PARSER_DHCP6);
 }
 
+// Tests if bash (#) comments are supported. That's the only comment type that
+// was supported by the old parser.
 TEST(ParserTest, bashComments) {
     string txt= "{ \"Dhcp6\": { \"interfaces-config\": {"
                 "  \"interfaces\": [ \"*\" ]"
@@ -144,10 +147,11 @@ TEST(ParserTest, bashComments) {
                 "    \"interface\": \"eth0\""
                 " } ],"
                 "\"valid-lifetime\": 4000 } }";
-    testParser2(txt, Parser6Context::PARSER_DHCP6);
+    testParser(txt, Parser6Context::PARSER_DHCP6);
 }
 
-TEST(ParserTest, cComments) {
+// Tests if C++ (//) comments can start anywhere, not just in the first line.
+TEST(ParserTest, cppComments) {
     string txt= "{ \"Dhcp6\": { \"interfaces-config\": {"
                 "  \"interfaces\": [ \"*\" ]"
                 "},\n"
@@ -160,9 +164,10 @@ TEST(ParserTest, cComments) {
                 "    \"interface\": \"eth0\""
                 " } ],"
                 "\"valid-lifetime\": 4000 } }";
-    testParser2(txt, Parser6Context::PARSER_DHCP6);
+    testParser(txt, Parser6Context::PARSER_DHCP6, false);
 }
 
+// Tests if bash (#) comments can start anywhere, not just in the first line.
 TEST(ParserTest, bashCommentsInline) {
     string txt= "{ \"Dhcp6\": { \"interfaces-config\": {"
                 "  \"interfaces\": [ \"*\" ]"
@@ -176,9 +181,10 @@ TEST(ParserTest, bashCommentsInline) {
                 "    \"interface\": \"eth0\""
                 " } ],"
                 "\"valid-lifetime\": 4000 } }";
-    testParser2(txt, Parser6Context::PARSER_DHCP6);
+    testParser(txt, Parser6Context::PARSER_DHCP6, false);
 }
 
+// Tests if multi-line C style comments are handled correctly.
 TEST(ParserTest, multilineComments) {
     string txt= "{ \"Dhcp6\": { \"interfaces-config\": {"
                 "  \"interfaces\": [ \"*\" ]"
@@ -193,17 +199,29 @@ TEST(ParserTest, multilineComments) {
                 "    \"interface\": \"eth0\""
                 " } ],"
                 "\"valid-lifetime\": 4000 } }";
-    testParser2(txt, Parser6Context::PARSER_DHCP6);
+    testParser(txt, Parser6Context::PARSER_DHCP6, false);
 }
 
-
-void testFile(const std::string& fname, bool print) {
+/// @brief Loads specified example config file
+///
+/// This test loads specified example file twice: first, using the legacy
+/// JSON file and then second time using bison parser. Two created Element
+/// trees are then compared. The input is decommented before it is passed
+/// to legacy parser (as legacy support for comments is very limited).
+///
+/// @param fname name of the file to be loaded
+void testFile(const std::string& fname) {
     ElementPtr reference_json;
     ConstElementPtr test_json;
 
-    cout << "Attempting to load file " << fname << endl;
+    string decommented = decommentJSONfile(fname);
+
+    cout << "Parsing file " << fname << "(" << decommented << ")" << endl;
+
+    EXPECT_NO_THROW(reference_json = Element::fromJSONFile(decommented, true));
 
-    EXPECT_NO_THROW(reference_json = Element::fromJSONFile(fname, true));
+    // remove the temporary file
+    EXPECT_NO_THROW(::remove(decommented.c_str()));
 
     EXPECT_NO_THROW(
     try {
@@ -217,9 +235,7 @@ void testFile(const std::string& fname, bool print) {
     ASSERT_TRUE(reference_json);
     ASSERT_TRUE(test_json);
 
-    compareJSON(reference_json, test_json, print);
-
-
+    compareJSON(reference_json, test_json);
 }
 
 // This test loads all available existing files. Each config is loaded
@@ -243,10 +259,15 @@ TEST(ParserTest, file) {
     configs.push_back("stateless.json");
 
     for (int i = 0; i<configs.size(); i++) {
-        testFile(string(CFG_EXAMPLES) + "/" + configs[i], false);
+        testFile(string(CFG_EXAMPLES) + "/" + configs[i]);
     }
 }
 
+/// @brief Tests error conditions in Dhcp6Parser
+///
+/// @param txt text to be parsed
+/// @param parser_type type of the parser to be used in the test
+/// @param msg expected content of the exception
 void testError(const std::string& txt,
                Parser6Context::ParserType parser_type,
                const std::string& msg)
@@ -265,7 +286,7 @@ void testError(const std::string& txt,
     }
 }
 
-// Check errors
+// Verify that error conditions are handled correctly.
 TEST(ParserTest, errors) {
     // no input
     testError("", Parser6Context::PARSER_JSON,
@@ -507,9 +528,13 @@ TEST(ParserTest, unicodeEscapes) {
         ASSERT_EQ(Element::string, result->getType());
         EXPECT_EQ(ins, result->stringValue());
     }
+}
 
+// This test checks that all representations of a slash is recognized properly.
+TEST(ParserTest, unicodeSlash) {
     // check the 4 possible encodings of solidus '/'
-    json = "\"/\\/\\u002f\\u002F\"";
+    ConstElementPtr result;
+    string json = "\"/\\/\\u002f\\u002F\"";
     ASSERT_NO_THROW(
     try {
         Parser6Context ctx;
@@ -520,6 +545,8 @@ TEST(ParserTest, unicodeEscapes) {
     });
     ASSERT_EQ(Element::string, result->getType());
     EXPECT_EQ("////", result->stringValue());
-}       
+}
 
 };
+};
+};

src/lib/dhcpsrv/cfg_mac_source.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2015,2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -47,6 +47,16 @@ uint32_t CfgMACSource::MACSourceFromText(const std::string& name) {
     isc_throw(BadValue, "Can't convert '" << name << "' to any known MAC source.");
 }
 
+void CfgMACSource::add(uint32_t source) {
+    for (CfgMACSources::const_iterator it = mac_sources_.begin();
+         it != mac_sources_.end(); ++it) {
+        if (*it == source) {
+            isc_throw(InvalidParameter, "mac-source paramter " << source
+                      << "' specified twice.");
+        }
+    }
+    mac_sources_.push_back(source);
+}
 
 };
 };

src/lib/dhcpsrv/cfg_mac_source.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2015,2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -52,10 +52,8 @@ class CfgMACSource {
     /// @param source MAC source (see constants in Pkt::HWADDR_SOURCE_*)
     ///
     /// Specified source is being added to the mac_sources_ array.
-    /// @todo implement add(string) version of this method.
-    void add(uint32_t source) {
-        mac_sources_.push_back(source);
-    }
+    /// @throw InvalidParameter if such a source is already defined.
+    void add(uint32_t source);
 
     /// @brief Provides access to the configure MAC/Hardware address sources.
     ///

src/lib/dhcpsrv/parsers/dhcp_parsers.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -177,62 +177,48 @@ template <> void ValueParser<std::string>::build(ConstElementPtr value) {
 
 // ******************** MACSourcesListConfigParser *************************
 
-MACSourcesListConfigParser::
-MACSourcesListConfigParser(const std::string& param_name,
-                           ParserContextPtr global_context)
-    : param_name_(param_name), global_context_(global_context) {
-    if (param_name_ != "mac-sources") {
-        isc_throw(BadValue, "Internal error. MAC sources configuration "
-            "parser called for the wrong parameter: " << param_name);
-    }
-}
-
 void
-MACSourcesListConfigParser::build(ConstElementPtr value) {
+MACSourcesListConfigParser::parse(CfgMACSource& mac_sources, ConstElementPtr value) {
     CfgIface cfg_iface;
     uint32_t source = 0;
+    size_t cnt = 0;
 
     // By default, there's only one source defined: ANY.
     // If user specified anything, we need to get rid of that default.
-    CfgMgr::instance().getStagingCfg()->getMACSources().clear();
+    mac_sources.clear();
 
     BOOST_FOREACH(ConstElementPtr source_elem, value->listValue()) {
         std::string source_str = source_elem->stringValue();
         try {
             source = CfgMACSource::MACSourceFromText(source_str);
-            CfgMgr::instance().getStagingCfg()->getMACSources().add(source);
+            mac_sources.add(source);
+            ++cnt;
+        } catch (const InvalidParameter& ex) {
+            isc_throw(DhcpConfigError, "The mac-sources value '" << source_str
+                      << "' was specified twice (" << value->getPosition() << ")");
         } catch (const std::exception& ex) {
             isc_throw(DhcpConfigError, "Failed to convert '"
                       << source_str << "' to any recognized MAC source:"
                       << ex.what() << " (" << value->getPosition() << ")");
         }
     }
-}
 
-void
-MACSourcesListConfigParser::commit() {
-    // Nothing to do.
+    if (!cnt) {
+        isc_throw(DhcpConfigError, "If specified, MAC Sources cannot be empty");
+    }
 }
 
 // ******************** ControlSocketParser *************************
-ControlSocketParser::ControlSocketParser(const std::string& param_name) {
-    if (param_name != "control-socket") {
-        isc_throw(BadValue, "Internal error. Control socket parser called "
-                  " for wrong parameter:" << param_name);
+void ControlSocketParser::parse(SrvConfig& srv_cfg, isc::data::ConstElementPtr value) {
+    if (!value) {
+        isc_throw(DhcpConfigError, "Logic error: specified control-socket is null");
     }
-}
 
-void ControlSocketParser::build(isc::data::ConstElementPtr value) {
     if (value->getType() != Element::map) {
-        isc_throw(BadValue, "Specified control-socket is expected to be a map"
+        isc_throw(DhcpConfigError, "Specified control-socket is expected to be a map"
                   ", i.e. a structure defined within { }");
     }
-    CfgMgr::instance().getStagingCfg()->setControlSocketInfo(value);
-}
-
-/// @brief Does nothing.
-void ControlSocketParser::commit() {
-    // Nothing to do.
+    srv_cfg.setControlSocketInfo(value);
 }
 
 // ******************** HooksLibrariesParser *************************
@@ -804,66 +790,66 @@ OptionDefListParser::parse(CfgOptionDefPtr storage, ConstElementPtr option_def_l
 }
 
 //****************************** RelayInfoParser ********************************
-RelayInfoParser::RelayInfoParser(const std::string&,
-                                 const isc::dhcp::Subnet::RelayInfoPtr& relay_info,
-                                 const Option::Universe& family)
-    :storage_(relay_info), local_(isc::asiolink::IOAddress(
-                                  family == Option::V4 ? "0.0.0.0" : "::")),
-     string_values_(new StringStorage()), family_(family) {
-    if (!relay_info) {
-        isc_throw(isc::dhcp::DhcpConfigError, "parser logic error: "
-                  << "relay-info storage may not be NULL");
-    }
-
+RelayInfoParser::RelayInfoParser(const Option::Universe& family)
+    : family_(family) {
 };
 
 void
-RelayInfoParser::build(ConstElementPtr relay_info) {
-
-    BOOST_FOREACH(ConfigPair param, relay_info->mapValue()) {
-        ParserPtr parser(createConfigParser(param.first));
-        parser->build(param.second);
-        parser->commit();
+RelayInfoParser::parse(const isc::dhcp::Subnet::RelayInfoPtr& cfg,
+                       ConstElementPtr relay_info) {
+    // Let's start with some sanity checks.
+    if (!relay_info || !cfg) {
+        isc_throw(DhcpConfigError, "Logic error: RelayInfoParser::parse() called "
+                  "with at least one NULL parameter.");
     }
 
-    // Get the IP address
-    boost::scoped_ptr<asiolink::IOAddress> ip;
-    try {
-        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")
-                  << " (" << string_values_->getPosition("ip-address") << ")");
+    if (relay_info->getType() != Element::map) {
+        isc_throw(DhcpConfigError, "Configuration error: RelayInfoParser::parse() "
+                  "called with non-map parameter");
     }
 
-    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")
-                  << " (" << string_values_->getPosition("ip-address") << ")");
-    }
+    // Now create the default value.
+    isc::asiolink::IOAddress ip(family_ == Option::V4 ? IOAddress::IPV4_ZERO_ADDRESS()
+                                : IOAddress::IPV6_ZERO_ADDRESS());
 
-    local_.addr_ = *ip;
-}
+    // Now iterate over all parameters. Currently there's only one supported
+    // parameter, so it should be an easy thing to check.
+    bool ip_address_specified = false;
+    BOOST_FOREACH(ConfigPair param, relay_info->mapValue()) {
+        if (param.first == "ip-address") {
+            ip_address_specified = true;
+
+            try {
+                ip = asiolink::IOAddress(param.second->stringValue());
+            } catch (...)  {
+                isc_throw(DhcpConfigError, "Failed to parse ip-address "
+                          "value: " << param.second
+                          << " (" << param.second->getPosition() << ")");
+            }
 
-isc::dhcp::ParserPtr
-RelayInfoParser::createConfigParser(const std::string& parameter) {
-    DhcpConfigParser* parser = NULL;
-    if (parameter.compare("ip-address") == 0) {
-        parser = new StringParser(parameter, string_values_);
-    } else {
-        isc_throw(NotImplemented,
-                  "parser error: RelayInfoParser parameter not supported: "
-                  << parameter);
+            // Check if the address family matches.
+            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")
+                          << " (" << param.second->getPosition() << ")");
+            }
+        } else {
+            isc_throw(NotImplemented,
+                      "parser error: RelayInfoParser parameter not supported: "
+                      << param.second);
+        }
     }
 
-    return (isc::dhcp::ParserPtr(parser));
-}
+    if (!ip_address_specified) {
+        isc_throw(DhcpConfigError, "'relay' specified, but mandatory 'ip-address' "
+                  "paramter in it is missing");
+    }
 
-void
-RelayInfoParser::commit() {
-    *storage_ = local_;
+    // Ok, we're done with parsing. Let's store the result in the structure
+    // we were given as configuration storage.
+    *cfg = isc::dhcp::Subnet::RelayInfo(ip);
 }
 
 //****************************** PoolsListParser ********************************
@@ -1069,6 +1055,12 @@ SubnetConfigParser::build(ConstElementPtr subnet) {
             continue;
         }
 
+        if (param.first == "relay") {
+            RelayInfoParser parser(global_context_->universe_);
+            parser.parse(relay_info_, param.second);
+            continue;
+        }
+
         ParserPtr parser;
         // When unsupported parameter is specified, the function called
         // below will thrown an exception. We have to catch this exception

src/lib/dhcpsrv/parsers/dhcp_parsers.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -16,6 +16,8 @@
 #include <dhcpsrv/cfg_option.h>
 #include <dhcpsrv/subnet.h>
 #include <dhcpsrv/cfg_option_def.h>
+#include <dhcpsrv/cfg_mac_source.h>
+#include <dhcpsrv/srv_config.h>
 #include <dhcpsrv/parsers/dhcp_config_parser.h>
 #include <cc/simple_parser.h>
 #include <hooks/libinfo.h>
@@ -372,56 +374,33 @@ private:
 /// It contains a list of MAC/hardware acquisition source, i.e. methods how
 /// MAC address can possibly by obtained in DHCPv6. For a currently supported
 /// methods, see @ref isc::dhcp::Pkt::getMAC.
-class MACSourcesListConfigParser : public DhcpConfigParser {
+class MACSourcesListConfigParser : public isc::data::SimpleParser {
 public:
-
-    /// @brief constructor
-    ///
-    /// As this is a dedicated parser, it must be used to parse
-    /// "mac-sources" parameter only. All other types will throw exception.
-    ///
-    /// @param param_name name of the configuration parameter being parsed
-    /// @param global_context Global parser context.
-    /// @throw BadValue if supplied parameter name is not "mac-sources"
-    MACSourcesListConfigParser(const std::string& param_name,
-                               ParserContextPtr global_context);
-
     /// @brief parses parameters value
     ///
     /// Parses configuration entry (list of sources) and adds each element
     /// to the sources list.
     ///
     /// @param value pointer to the content of parsed values
-    virtual void build(isc::data::ConstElementPtr value);
-
-    /// @brief Does nothing.
-    virtual void commit();
-
-private:
-
-    // Parsed parameter name
-    std::string param_name_;
-
-    /// Global parser context.
-    ParserContextPtr global_context_;
+    void parse(CfgMACSource& mac_sources, isc::data::ConstElementPtr value);
 };
 
 /// @brief Parser for the control-socket structure
 ///
 /// It does not parse anything, simply stores the element in
 /// the staging config.
-class ControlSocketParser : public DhcpConfigParser {
+class ControlSocketParser : public isc::data::SimpleParser {
 public:
-
-    ControlSocketParser(const std::string& param_name);
-
-    /// @brief Stores contents of the control-socket map in the staging config.
+    /// @brief "Parses" control-socket structure
     ///
+    /// Since the SrvConfig structure takes the socket definition
+    /// as ConstElementPtr, there's really nothing to parse here.
+    /// It only does basic sanity checks and throws DhcpConfigError
+    /// if the value is null or is not a map.
+    ///
+    /// @param srv_cfg parsed values will be stored here
     /// @param value pointer to the content of parsed values
-    virtual void build(isc::data::ConstElementPtr value);
-
-    /// @brief Does nothing.
-    virtual void commit();
+    void parse(SrvConfig& srv_cfg, isc::data::ConstElementPtr value);
 };
 
 /// @brief Parser for hooks library list
@@ -849,48 +828,24 @@ protected:
 /// is expected that the number of parameters will increase over time.
 ///
 /// It is useful for parsing Dhcp<4/6>/subnet<4/6>[x]/relay parameters.
-class RelayInfoParser : public DhcpConfigParser {
+class RelayInfoParser : public isc::data::SimpleParser {
 public:
 
     /// @brief constructor
-    /// @param unused first argument is ignored, all Parser constructors
-    /// accept string as first argument.
-    /// @param relay_info is the storage in which to store the parsed
     /// @param family specifies protocol family (IPv4 or IPv6)
-    RelayInfoParser(const std::string& unused,
-                    const isc::dhcp::Subnet::RelayInfoPtr& relay_info,
-                    const isc::dhcp::Option::Universe& family);
+    RelayInfoParser(const isc::dhcp::Option::Universe& family);
 
     /// @brief parses the actual relay parameters
-    /// @param relay_info JSON structure holding relay parameters to parse
-    virtual void build(isc::data::ConstElementPtr relay_info);
-
-    /// @brief stores parsed info in relay_info
-    virtual void commit();
-
-protected:
-
-    /// @brief Creates a parser for the given "relay" member element id.
     ///
     /// The elements currently supported are:
     /// -# ip-address
     ///
-    /// @param parser is the "item_name" for a specific member element of
-    /// the "relay" specification.
-    ///
-    /// @return returns a pointer to newly created parser.
-    isc::dhcp::ParserPtr
-    createConfigParser(const std::string& parser);
-
-    /// Parsed data will be stored there on commit()
-    isc::dhcp::Subnet::RelayInfoPtr storage_;
-
-    /// Local storage information (for temporary values)
-    isc::dhcp::Subnet::RelayInfo local_;
-
-    /// Storage for subnet-specific string values.
-    StringStoragePtr string_values_;
+    /// @param cfg configuration will be stored here
+    /// @param relay_info JSON structure holding relay parameters to parse
+    void parse(const isc::dhcp::Subnet::RelayInfoPtr& cfg,
+               isc::data::ConstElementPtr relay_info);
 
+protected:
     /// Protocol family (IPv4 or IPv6)
     Option::Universe family_;
 };

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -226,48 +226,95 @@ TEST_F(DhcpParserTest, uint32ParserTest) {
     EXPECT_EQ(test_value, actual_value);
 }
 
-/// @brief Check MACSourcesListConfigParser  basic functionality
+/// Verifies the code that parses mac sources and adds them to CfgMgr
+TEST_F(DhcpParserTest, MacSources) {
+
+    // That's an equivalent of the following snippet:
+    // "mac-sources: [ \"duid\", \"ipv6\" ]";
+    ElementPtr values = Element::createList();
+    values->add(Element::create("duid"));
+    values->add(Element::create("ipv6-link-local"));
+
+    // Let's grab server configuration from CfgMgr
+    SrvConfigPtr cfg = CfgMgr::instance().getStagingCfg();
+    ASSERT_TRUE(cfg);
+    CfgMACSource& sources = cfg->getMACSources();
+
+    // This should parse the configuration and check that it doesn't throw.
+    MACSourcesListConfigParser parser;
+    EXPECT_NO_THROW(parser.parse(sources, values));
+
+    // Finally, check the sources that were configured
+    CfgMACSources configured_sources =  cfg->getMACSources().get();
+    ASSERT_EQ(2, configured_sources.size());
+    EXPECT_EQ(HWAddr::HWADDR_SOURCE_DUID, configured_sources[0]);
+    EXPECT_EQ(HWAddr::HWADDR_SOURCE_IPV6_LINK_LOCAL, configured_sources[1]);
+}
+
+/// @brief Check MACSourcesListConfigParser rejecting empty list
 ///
-/// Verifies that the parser:
-/// 1. Does not allow empty for storage.
-/// 2. Does not allow name other than "mac-sources"
-/// 3. Parses list of mac sources and adds them to CfgMgr
-TEST_F(DhcpParserTest, MacSourcesListConfigParserTest) {
+/// Verifies that the code rejects an empty mac-sources list.
+TEST_F(DhcpParserTest, MacSourcesEmpty) {
 
-    const std::string valid_name = "mac-sources";
-    const std::string bogus_name = "bogus-name";
+    // That's an equivalent of the following snippet:
+    // "mac-sources: [ \"duid\", \"ipv6\" ]";
+    ElementPtr values = Element::createList();
+
+    // Let's grab server configuration from CfgMgr
+    SrvConfigPtr cfg = CfgMgr::instance().getStagingCfg();
+    ASSERT_TRUE(cfg);
+    CfgMACSource& sources = cfg->getMACSources();
 
-    ParserContextPtr parser_context(new ParserContext(Option::V6));
+    // This should throw, because if specified, at least one MAC source
+    // has to be specified.
+    MACSourcesListConfigParser parser;
+    EXPECT_THROW(parser.parse(sources, values), DhcpConfigError);
+}
 
-    // Verify that parser constructor fails if parameter name isn't "mac-sources"
-    EXPECT_THROW(MACSourcesListConfigParser(bogus_name, parser_context),
-                 isc::BadValue);
+/// @brief Check MACSourcesListConfigParser rejecting empty list
+///
+/// Verifies that the code rejects fake mac source.
+TEST_F(DhcpParserTest, MacSourcesBogus) {
 
     // That's an equivalent of the following snippet:
     // "mac-sources: [ \"duid\", \"ipv6\" ]";
-    ElementPtr config = Element::createList();
-    config->add(Element::create("duid"));
-    config->add(Element::create("ipv6-link-local"));
+    ElementPtr values = Element::createList();
+    values->add(Element::create("from-ebay"));
+    values->add(Element::create("just-guess-it"));
+
+    // Let's grab server configuration from CfgMgr
+    SrvConfigPtr cfg = CfgMgr::instance().getStagingCfg();
+    ASSERT_TRUE(cfg);
+    CfgMACSource& sources = cfg->getMACSources();
+
+    // This should throw, because these are not valid sources.
+    MACSourcesListConfigParser parser;
+    EXPECT_THROW(parser.parse(sources, values), DhcpConfigError);
+}
 
-    boost::scoped_ptr<MACSourcesListConfigParser>
-        parser(new MACSourcesListConfigParser(valid_name, parser_context));
+/// Verifies the code that properly catches duplicate entries
+/// in mac-sources definition.
+TEST_F(DhcpParserTest, MacSourcesDuplicate) {
 
-    // This should parse the configuration and add eth0 and eth1 to the list
-    // of interfaces that server should listen on.
-    EXPECT_NO_THROW(parser->build(config));
-    EXPECT_NO_THROW(parser->commit());
+    // That's an equivalent of the following snippet:
+    // "mac-sources: [ \"duid\", \"ipv6\" ]";
+    ElementPtr values = Element::createList();
+    values->add(Element::create("ipv6-link-local"));
+    values->add(Element::create("duid"));
+    values->add(Element::create("duid"));
+    values->add(Element::create("duid"));
 
-    // Use CfgMgr instance to check if eth0 and eth1 was added, and that
-    // eth2 was not added.
+    // Let's grab server configuration from CfgMgr
     SrvConfigPtr cfg = CfgMgr::instance().getStagingCfg();
     ASSERT_TRUE(cfg);
-    CfgMACSources configured_sources =  cfg->getMACSources().get();
+    CfgMACSource& sources = cfg->getMACSources();
 
-    ASSERT_EQ(2, configured_sources.size());
-    EXPECT_EQ(HWAddr::HWADDR_SOURCE_DUID, configured_sources[0]);
-    EXPECT_EQ(HWAddr::HWADDR_SOURCE_IPV6_LINK_LOCAL, configured_sources[1]);
+    // This should parse the configuration and check that it throws.
+    MACSourcesListConfigParser parser;
+    EXPECT_THROW(parser.parse(sources, values), DhcpConfigError);
 }
 
+
 /// @brief Test Fixture class which provides basic structure for testing
 /// configuration parsing.  This is essentially the same structure provided
 /// by dhcp servers.
@@ -2416,6 +2463,19 @@ TEST_F(ParseConfigTest, validRelayInfo4) {
         "    }";
     ElementPtr json = Element::fromJSON(config_str);
 
+    // We need to set the default ip-address to something.
+    Subnet::RelayInfoPtr result(new Subnet::RelayInfo(asiolink::IOAddress("0.0.0.0")));
+
+    RelayInfoParser parser(Option::V4);
+
+    // Subnet4 parser will pass 0.0.0.0 to the RelayInfoParser
+    EXPECT_NO_THROW(parser.parse(result, json));
+    EXPECT_EQ("192.0.2.1", result->addr_.toText());
+}
+
+/// @brief Checks that a bogus relay info structure for IPv4 is rejected.
+TEST_F(ParseConfigTest, bogusRelayInfo4) {
+
     // Invalid config (wrong family type of the ip-address field)
     std::string config_str_bogus1 =
         "    {"
@@ -2430,24 +2490,25 @@ TEST_F(ParseConfigTest, validRelayInfo4) {
         "    }";
     ElementPtr json_bogus2 = Element::fromJSON(config_str_bogus2);
 
-    // We need to set the default ip-address to something.
-    Subnet::RelayInfoPtr result(new Subnet::RelayInfo(asiolink::IOAddress("0.0.0.0")));
+    // Invalid config (ip-address is mandatory)
+    std::string config_str_bogus3 =
+        "    {"
+        "    }";
+    ElementPtr json_bogus3 = Element::fromJSON(config_str_bogus3);
 
-    boost::shared_ptr<RelayInfoParser> parser;
+    // We need to set the default ip-address to something.
+    Subnet::RelayInfoPtr result(new Subnet::RelayInfo(IOAddress::IPV4_ZERO_ADDRESS()));
 
-    // Subnet4 parser will pass 0.0.0.0 to the RelayInfoParser
-    EXPECT_NO_THROW(parser.reset(new RelayInfoParser("ignored", result,
-                                                     Option::V4)));
-    EXPECT_NO_THROW(parser->build(json));
-    EXPECT_NO_THROW(parser->commit());
+    RelayInfoParser parser(Option::V4);
 
-    EXPECT_EQ("192.0.2.1", result->addr_.toText());
+    // wrong family type
+    EXPECT_THROW(parser.parse(result, json_bogus1), DhcpConfigError);
 
-    // Let's check negative scenario (wrong family type)
-    EXPECT_THROW(parser->build(json_bogus1), DhcpConfigError);
+    // Too large byte values in pseudo-IPv4 addr
+    EXPECT_THROW(parser.parse(result, json_bogus2), DhcpConfigError);
 
-    // Let's check negative scenario (too large byte values in pseudo-IPv4 addr)
-    EXPECT_THROW(parser->build(json_bogus2), DhcpConfigError);
+    // Mandatory ip-address is missing. What a pity.
+    EXPECT_THROW(parser.parse(result, json_bogus2), DhcpConfigError);
 }
 
 /// @brief Checks that a valid relay info structure for IPv6 can be handled
@@ -2460,6 +2521,18 @@ TEST_F(ParseConfigTest, validRelayInfo6) {
         "    }";
     ElementPtr json = Element::fromJSON(config_str);
 
+    // We need to set the default ip-address to something.
+    Subnet::RelayInfoPtr result(new Subnet::RelayInfo(asiolink::IOAddress("::")));
+
+    RelayInfoParser parser(Option::V6);
+    // Subnet4 parser will pass :: to the RelayInfoParser
+    EXPECT_NO_THROW(parser.parse(result, json));
+    EXPECT_EQ("2001:db8::1", result->addr_.toText());
+}
+
+/// @brief Checks that a valid relay info structure for IPv6 can be handled
+TEST_F(ParseConfigTest, bogusRelayInfo6) {
+
     // Invalid config (wrong family type of the ip-address field
     std::string config_str_bogus1 =
         "    {"
@@ -2474,23 +2547,25 @@ TEST_F(ParseConfigTest, validRelayInfo6) {
         "    }";
     ElementPtr json_bogus2 = Element::fromJSON(config_str_bogus2);
 
+    // Missing mandatory ip-address field.
+    std::string config_str_bogus3 =
+        "    {"
+        "    }";
+    ElementPtr json_bogus3 = Element::fromJSON(config_str_bogus3);
+
     // We need to set the default ip-address to something.
     Subnet::RelayInfoPtr result(new Subnet::RelayInfo(asiolink::IOAddress("::")));
 
-    boost::shared_ptr<RelayInfoParser> parser;
-    // Subnet4 parser will pass :: to the RelayInfoParser
-    EXPECT_NO_THROW(parser.reset(new RelayInfoParser("ignored", result,
-                                                     Option::V6)));
-    EXPECT_NO_THROW(parser->build(json));
-    EXPECT_NO_THROW(parser->commit());
+    RelayInfoParser parser(Option::V6);
 
-    EXPECT_EQ("2001:db8::1", result->addr_.toText());
+    // Negative scenario (wrong family type)
+    EXPECT_THROW(parser.parse(result, json_bogus1), DhcpConfigError);
 
-    // Let's check negative scenario (wrong family type)
-    EXPECT_THROW(parser->build(json_bogus1), DhcpConfigError);
+    // Looks like IPv6 address, but has too many colons
+    EXPECT_THROW(parser.parse(result, json_bogus2), DhcpConfigError);
 
-    // Unparseable text that looks like IPv6 address, but has too many colons
-    EXPECT_THROW(parser->build(json_bogus2), DhcpConfigError);
+    // Mandatory ip-address is missing. What a pity.
+    EXPECT_THROW(parser.parse(result, json_bogus3), DhcpConfigError);
 }
 
 // There's no test for ControlSocketParser, as it is tested in the DHCPv4 code

src/lib/testutils/io_utils.cc

@@ -4,6 +4,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
+#include <exceptions/exceptions.h>
 #include <testutils/io_utils.h>
 #include <gtest/gtest.h>
 #include <fstream>
@@ -36,7 +37,78 @@ std::string readFile(const std::string& file_path) {
     return (output.str());
 }
 
+std::string decommentJSONfile(const std::string& input_file) {
+
+    using namespace std;
+
+    ifstream f(input_file);
+    if (!f.is_open()) {
+        isc_throw(isc::BadValue, "can't open input file for reading: " + input_file);
+    }
+
+    string outfile;
+    size_t last_slash = input_file.find_last_of("/");
+    if (last_slash != string::npos) {
+        outfile = input_file.substr(last_slash + 1);
+    } else {
+        outfile = input_file;
+    }
+    outfile += "-decommented";
+
+    ofstream out(outfile);
+    if (!out.is_open()) {
+        isc_throw(isc::BadValue, "can't open output file for writing: " + input_file);
+    }
+
+    bool in_comment = false;
+    string line;
+    while (std::getline(f, line)) {
+        // First, let's get rid of the # comments
+        size_t hash_pos = line.find("#");
+        if (hash_pos != string::npos) {
+            line = line.substr(0, hash_pos);
+        }
+
+        // Second, let's get rid of the // comments
+        size_t dblslash_pos = line.find("//");
+        if (dblslash_pos != string::npos) {
+            line = line.substr(0, dblslash_pos);
+        }
+
+        // Now the tricky part: c comments.
+        size_t begin_pos = line.find("/*");
+        size_t end_pos = line.find("*/");
+        if (in_comment && end_pos == string::npos) {
+            // we continue through multiline comment
+            line = "";
+        } else {
+
+            if (begin_pos != string::npos) {
+                in_comment = true;
+                if (end_pos != string::npos) {
+                    // sigle line comment. Let's get rid of the content in between
+                    line = line.replace(begin_pos, end_pos + 2, end_pos + 2 - begin_pos, ' ');
+                    in_comment = false;
+                } else {
+                    line = line.substr(0, begin_pos);
+                }
+            } else {
+                if (in_comment && end_pos != string::npos) {
+                    line = line.replace(0, end_pos +2 , end_pos + 2, ' ');
+                    in_comment = false;
+                }
+            }
+        }
+
+        // Finally, write the line to the output file.
+        out << line << endl;
+    }
+    f.close();
+    out.close();
+
+    return (outfile);
+}
+
 }; // end of isc::dhcp::test namespace
 }; // end of isc::dhcp namespace
 }; // end of isc namespace
-

src/lib/testutils/io_utils.h

@@ -26,6 +26,20 @@ bool fileExists(const std::string& file_path);
 /// @return File contents.
 std::string readFile(const std::string& file_path);
 
+/// @brief Removes comments from a JSON file
+///
+/// Removes //, # and /* */ comments from the input file and writes its content
+/// to another file. The comments are replaced with spaces, so the original
+/// token locations should remain unaffected. This is rather naive
+/// implementation, but it's probably sufficient for testing. It won't be able
+/// to pick any trickier cases, like # or // appearing in strings, nested C++
+/// comments etc.
+///
+/// @param input_file file to be stripped of comments
+/// @return filename of a new file that has comments stripped from it
+/// @throw BadValue if the input file cannot be opened
+std::string decommentJSONfile(const std::string& input_file);
+
 }; // end of isc::dhcp::test namespace
 }; // end of isc::dhcp namespace
 }; // end of isc namespace