[3191] Changes after review:

 - new negative unit-test for next-server written
 - comments update (capitalized!)
 - indents corrected
 - no more raw pointers
 - ChangeLog added

ChangeLog

+6XX.	[func]		tomek
+	b10-dhcp4: It is now possible to specify value of siaddr field
+	in DHCPv4 responses. It is used to point out to the next
+	server in the boot process (that typically is TFTP server).
+	(Trac #3191, git ABCD)
+
 690.	[bug]		tomek
 	b10-dhcp4: Relay Agent Info option is now echoed back in
 	DHCPv4 responses.

doc/guide/bind10-guide.xml

@@ -4390,7 +4390,7 @@ Dhcp4/subnet4	[]	list	(default)
 
     <section id="dhcp4-next-server">
       <title>Next server (siaddr)</title>
-      <para>In some cases, clients want to obtain configuration form the TFTP server.
+      <para>In some cases, clients want to obtain configuration from the TFTP server.
       Although there is a dedicated option for it, some devices may use siaddr field
       in the DHCPv4 packet for that purpose. That specific field can be configured
       using next-server directive. It is possible to define it in global scope or

src/bin/dhcp4/config_parser.cc

@@ -264,15 +264,15 @@ protected:
 
         LOG_INFO(dhcp4_logger, DHCP4_CONFIG_NEW_SUBNET).arg(tmp.str());
 
-        Subnet4* subnet4 = new Subnet4(addr, len, t1, t2, valid);
-        subnet_.reset(subnet4);
+        Subnet4Ptr subnet4(new Subnet4(addr, len, t1, t2, valid));
+        subnet_ = subnet4;
 
         // Try global value first
         try {
             string next_server = globalContext()->string_values_->getParam("next-server");
             subnet4->setSiaddr(IOAddress(next_server));
         } catch (const DhcpConfigError&) {
-            // don't care. next_server is optional. We can live without it
+            // Don't care. next_server is optional. We can live without it
         }
 
         // Try subnet specific value if it's available
@@ -280,7 +280,7 @@ protected:
             string next_server = string_values_->getParam("next-server");
             subnet4->setSiaddr(IOAddress(next_server));
         } catch (const DhcpConfigError&) {
-            // don't care. next_server is optional. We can live without it
+            // Don't care. next_server is optional. We can live without it
         }
     }
 };

src/bin/dhcp4/dhcp4.spec

@@ -226,9 +226,9 @@
                 },
 
                 { "item_name": "next-server",
-                    "item_type": "string",
-                    "item_optional": true,
-                    "item_default": "0.0.0.0"
+                  "item_type": "string",
+                  "item_optional": true,
+                  "item_default": "0.0.0.0"
                 },
 
                 { "item_name": "pool",

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -469,6 +469,57 @@ TEST_F(Dhcp4ParserTest, nextServerSubnet) {
     EXPECT_EQ("1.2.3.4", subnet->getSiaddr().toText());
 }
 
+// Test checks several negative scenarios for next-server configuration: bogus
+// address, IPv6 adddress and empty string.
+TEST_F(Dhcp4ParserTest, nextServerNegative) {
+
+    ConstElementPtr status;
+
+    // Config with junk instead of next-server address
+    string config_bogus1 = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"next-server\": \"a.b.c.d\", "
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    // Config with IPv6 next server address
+    string config_bogus2 = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"next-server\": \"2001:db8::1\", "
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    // Config with empty next server address
+    string config_bogus3 = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"next-server\": \"\", "
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    ElementPtr json1 = Element::fromJSON(config_bogus1);
+    ElementPtr json2 = Element::fromJSON(config_bogus2);
+    ElementPtr json3 = Element::fromJSON(config_bogus2);
+
+    // check if returned status is always a failure
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json1));
+    checkResult(status, 1);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json2));
+    checkResult(status, 1);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json3));
+    checkResult(status, 1);
+}
+
 // Checks if the next-server defined as global value is overridden by subnet
 // specific value.
 TEST_F(Dhcp4ParserTest, nextServerOverride) {

src/lib/dhcpsrv/subnet.cc

@@ -154,7 +154,7 @@ Subnet4::Subnet4(const isc::asiolink::IOAddress& prefix, uint8_t length,
 
 void Subnet4::setSiaddr(const isc::asiolink::IOAddress& siaddr) {
     if (!siaddr.isV4()) {
-        isc_throw(BadValue, "Can't set siaddr to non-IPv4 addr "
+        isc_throw(BadValue, "Can't set siaddr to non-IPv4 address "
                   << siaddr.toText());
     }
     siaddr_ = siaddr;

src/lib/dhcpsrv/subnet.h

@@ -466,14 +466,15 @@ public:
             const Triplet<uint32_t>& t2,
             const Triplet<uint32_t>& valid_lifetime);
 
-    /// @brief sets siaddr for the Subnet4
+    /// @brief Sets siaddr for the Subnet4
     ///
     /// Will be used for siaddr field (the next server) that typically is used
     /// as TFTP server. If not specified, the default value of 0.0.0.0 is
     /// used.
     void setSiaddr(const isc::asiolink::IOAddress& siaddr);
 
-    /// @brief returns siaddr for this subnet
+    /// @brief Returns siaddr for this subnet
+    ///
     /// @return siaddr value
     isc::asiolink::IOAddress getSiaddr() const;
 

src/lib/dhcpsrv/tests/subnet_unittest.cc

@@ -58,7 +58,7 @@ TEST(Subnet4Test, in_range) {
     EXPECT_FALSE(subnet.inRange(IOAddress("255.255.255.255")));
 }
 
-// Checks whether siaddr field is handle correctly
+// Checks whether siaddr field can be set and retrieved correctly.
 TEST(Subnet4Test, siaddr) {
     Subnet4 subnet(IOAddress("192.0.2.1"), 24, 1000, 2000, 3000);