[3274] Changes after review:

 - RelayInfoParser now takes Option::Universe as last parameter;
 - Extra tests for checking invalid parameters in RelayInfoParser
 - Comments in subnet.h improved
 - Sanity checks in RelayInfoParser implemented, doc improved

src/bin/dhcp4/config_parser.cc

@@ -211,7 +211,7 @@ protected:
         } else if (config_id.compare("pool") == 0) {
             parser = new Pool4Parser(config_id, pools_);
         } else if (config_id.compare("relay") == 0) {
-            parser = new RelayInfoParser(config_id, relay_info_, IOAddress("0.0.0.0"));
+            parser = new RelayInfoParser(config_id, relay_info_, Option::V4);
         } else if (config_id.compare("option-data") == 0) {
            parser = new OptionDataListParser(config_id, options_,
                                              global_context_,

src/bin/dhcp6/config_parser.cc

@@ -416,7 +416,7 @@ protected:
         } else if (config_id.compare("pool") == 0) {
             parser = new Pool6Parser(config_id, pools_);
         } else if (config_id.compare("relay") == 0) {
-            parser = new RelayInfoParser(config_id, relay_info_, IOAddress("::"));
+            parser = new RelayInfoParser(config_id, relay_info_, Option::V6);
         } else if (config_id.compare("pd-pools") == 0) {
             parser = new PdPoolListParser(config_id, pools_);
         } else if (config_id.compare("option-data") == 0) {

src/lib/dhcpsrv/dhcp_parsers.cc

@@ -814,9 +814,10 @@ OptionDefListParser::commit() {
 //****************************** RelayInfoParser ********************************
 RelayInfoParser::RelayInfoParser(const std::string&,
                                  const isc::dhcp::Subnet::RelayInfoPtr& relay_info,
-                                 const asiolink::IOAddress& default_addr)
-    :storage_(relay_info), local_(default_addr),
-     string_values_(new StringStorage()) {
+                                 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");
@@ -834,9 +835,22 @@ RelayInfoParser::build(ConstElementPtr relay_info) {
     }
 
     // Get the IP address
-    asiolink::IOAddress ip(string_values_->getParam("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"));
+    }
+
+    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"));
+    }
 
-    local_.addr_ = ip;
+    local_.addr_ = *ip;
 }
 
 isc::dhcp::ParserPtr

src/lib/dhcpsrv/dhcp_parsers.h

@@ -765,14 +765,13 @@ class RelayInfoParser : public DhcpConfigParser {
 public:
 
     /// @brief constructor
-    /// @param dummy first argument is ignored, all Parser constructors
+    /// @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 default_addr 0.0.0.0 for IPv4 or :: for IPv6
-    /// relay information upon "commit".
-    RelayInfoParser(const std::string& dummy,
+    /// @param family specifies protocol family (IPv4 or IPv6)
+    RelayInfoParser(const std::string& unused,
                     const isc::dhcp::Subnet::RelayInfoPtr& relay_info,
-                    const asiolink::IOAddress& default_addr);
+                    const isc::dhcp::Option::Universe family);
 
     /// @brief parses the actual relay parameters
     /// @param relay_info JSON strcture holding relay parameters to parse
@@ -782,6 +781,16 @@ public:
     virtual void commit();
 
 protected:
+
+    /// @brief Creates a parser for the given "relay" member element id.
+    ///
+    /// The elements currently supported are:
+    /// -# ip-address
+    ///
+    /// @param config_id 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);
 
@@ -793,6 +802,9 @@ protected:
 
     /// Storage for subnet-specific string values.
     StringStoragePtr string_values_;
+
+    /// Protocol family (IPv4 or IPv6)
+    Option::Universe family_;
 };
 
 /// @brief this class parses a single subnet

src/lib/dhcpsrv/subnet.h

@@ -396,7 +396,7 @@ public:
         static_id_ = 1;
     }
 
-    /// @brief Sets address of the relay
+    /// @brief Sets information about relay
     ///
     /// In some situations where there are shared subnets (i.e. two different
     /// subnets are available on the same physical link), there is only one
@@ -411,8 +411,10 @@ public:
     /// from different subnet, so users won't tinker with their modems.
     ///
     /// Setting this parameter is not needed in most deployments.
+    /// This structure holds IP address only for now, but it is expected to
+    /// be extended in the future.
     ///
-    /// @param relay IP address of the relay
+    /// @param relay structure that contains relay information
     void setRelay(const isc::dhcp::Subnet::RelayInfo& relay);
 
     /// @brief Relay information
@@ -439,6 +441,8 @@ public:
 
     /// @brief adds class class_name to the list of supported classes
     ///
+    /// Also see explanation note in @ref white_list_.
+    ///
     /// @param class_name client class to be supported by this subnet
     void
     allowClientClass(const isc::dhcp::ClientClass& class_name);
@@ -575,6 +579,12 @@ protected:
     /// If defined, only clients belonging to that class will be allowed to use
     /// this particular subnet. The default value for this is "", which means
     /// that any client is allowed, regardless of its class.
+    ///
+    /// @todo This is just a single class for now, but it will eventually be
+    /// list of classes (only classes on the list are allowed, the rest is
+    /// rejected) and we'll also have black-list (only classes on the list are
+    /// rejected, the rest are allowed). Implementing this will require
+    /// fancy parser logic, so it may be a while until we support this.
     ClientClass white_list_;
 
 private:

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -1278,6 +1278,20 @@ TEST_F(ParseConfigTest, validRelayInfo4) {
         "    }";
     ElementPtr json = Element::fromJSON(config_str);
 
+    // Invalid config (wrong family type of the ip-address field)
+    std::string config_str_bogus1 =
+        "    {"
+        "     \"ip-address\" : \"2001:db8::1\""
+        "    }";
+    ElementPtr json_bogus1 = Element::fromJSON(config_str_bogus1);
+
+    // Invalid config (that thing is not an IPv4 address)
+    std::string config_str_bogus2 =
+        "    {"
+        "     \"ip-address\" : \"256.345.123.456\""
+        "    }";
+    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")));
 
@@ -1285,11 +1299,17 @@ TEST_F(ParseConfigTest, validRelayInfo4) {
 
     // Subnet4 parser will pass 0.0.0.0 to the RelayInfoParser
     EXPECT_NO_THROW(parser.reset(new RelayInfoParser("ignored", result,
-                                      asiolink::IOAddress("0.0.0.0"))));
+                                                     Option::V4)));
     EXPECT_NO_THROW(parser->build(json));
     EXPECT_NO_THROW(parser->commit());
 
     EXPECT_EQ("192.0.2.1", result->addr_.toText());
+
+    // Let's check negative scenario (wrong family type)
+    EXPECT_THROW(parser->build(json_bogus1), DhcpConfigError);
+
+    // Let's check negative scenario (too large byte values in pseudo-IPv4 addr)
+    EXPECT_THROW(parser->build(json_bogus2), DhcpConfigError);
 }
 
 /// @brief Checks that a valid relay info structure for IPv6 can be handled
@@ -1302,15 +1322,35 @@ TEST_F(ParseConfigTest, validRelayInfo6) {
         "    }";
     ElementPtr json = Element::fromJSON(config_str);
 
+    // Invalid config (wrong family type of the ip-address field
+    std::string config_str_bogus1 =
+        "    {"
+        "     \"ip-address\" : \"192.0.2.1\""
+        "    }";
+    ElementPtr json_bogus1 = Element::fromJSON(config_str_bogus1);
+
+    // That IPv6 address doesn't look right
+    std::string config_str_bogus2 =
+        "    {"
+        "     \"ip-address\" : \"2001:db8:::4\""
+        "    }";
+    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("::")));
 
     boost::shared_ptr<RelayInfoParser> parser;
     // Subnet4 parser will pass :: to the RelayInfoParser
     EXPECT_NO_THROW(parser.reset(new RelayInfoParser("ignored", result,
-                                      asiolink::IOAddress("::"))));
+                                                     Option::V6)));
     EXPECT_NO_THROW(parser->build(json));
     EXPECT_NO_THROW(parser->commit());
 
     EXPECT_EQ("2001:db8::1", result->addr_.toText());
+
+    // Let's check negative scenario (wrong family type)
+    EXPECT_THROW(parser->build(json_bogus1), DhcpConfigError);
+
+    // Unparseable text that looks like IPv6 address, but has too many colons
+    EXPECT_THROW(parser->build(json_bogus2), DhcpConfigError);
 }