[3191] Changes after review

 - default value is no longer 0.0.0.0, so global value is no longer
   overwritten with subnet-specific values
 - added 2 new unit-tests to check if the value is really set in
   server responses
 - guide updated to explain that
 - fix in Dhcp4ParserTest.nextServerNegative

doc/guide/bind10-guide.xml

@@ -4395,6 +4395,9 @@ Dhcp4/subnet4	[]	list	(default)
       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
       for a given subnet only. If both are defined, subnet value takes precedence.
+      The value in subnet can be set to 0.0.0.0, which means that next-server should
+      not be sent. It may also be set to empty string, which means the same as if
+      it was not defined at all - use global value.
       </para>
 
 <screen>

src/bin/dhcp4/config_parser.cc

@@ -270,7 +270,9 @@ protected:
         // Try global value first
         try {
             string next_server = globalContext()->string_values_->getParam("next-server");
-            subnet4->setSiaddr(IOAddress(next_server));
+            if (!next_server.empty()) {
+                subnet4->setSiaddr(IOAddress(next_server));
+            }
         } catch (const DhcpConfigError&) {
             // Don't care. next_server is optional. We can live without it
         }
@@ -278,7 +280,9 @@ protected:
         // Try subnet specific value if it's available
         try {
             string next_server = string_values_->getParam("next-server");
-            subnet4->setSiaddr(IOAddress(next_server));
+            if (!next_server.empty()) {
+                subnet4->setSiaddr(IOAddress(next_server));
+            }
         } catch (const DhcpConfigError&) {
             // Don't care. next_server is optional. We can live without it
         }

src/bin/dhcp4/dhcp4.spec

@@ -51,7 +51,7 @@
       { "item_name": "next-server",
         "item_type": "string",
         "item_optional": true,
-        "item_default": "0.0.0.0"
+        "item_default": ""
       },
 
       { "item_name": "option-def",

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -481,6 +481,8 @@ TEST_F(Dhcp4ParserTest, nextServerNegative) {
         "\"renew-timer\": 1000, "
         "\"subnet4\": [ { "
         "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"rebind-timer\": 2000, "
+        "    \"renew-timer\": 1000, "
         "    \"next-server\": \"a.b.c.d\", "
         "    \"subnet\": \"192.0.2.0/24\" } ],"
         "\"valid-lifetime\": 4000 }";
@@ -491,6 +493,8 @@ TEST_F(Dhcp4ParserTest, nextServerNegative) {
         "\"renew-timer\": 1000, "
         "\"subnet4\": [ { "
         "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"rebind-timer\": 2000, "
+        "    \"renew-timer\": 1000, "
         "    \"next-server\": \"2001:db8::1\", "
         "    \"subnet\": \"192.0.2.0/24\" } ],"
         "\"valid-lifetime\": 4000 }";
@@ -501,13 +505,15 @@ TEST_F(Dhcp4ParserTest, nextServerNegative) {
         "\"renew-timer\": 1000, "
         "\"subnet4\": [ { "
         "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"rebind-timer\": 2000, "
+        "    \"renew-timer\": 1000, "
         "    \"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);
+    ElementPtr json3 = Element::fromJSON(config_bogus3);
 
     // check if returned status is always a failure
     EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json1));
@@ -517,7 +523,7 @@ TEST_F(Dhcp4ParserTest, nextServerNegative) {
     checkResult(status, 1);
 
     EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json3));
-    checkResult(status, 1);
+    checkResult(status, 0);
 }
 
 // Checks if the next-server defined as global value is overridden by subnet

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -1339,6 +1339,90 @@ TEST_F(Dhcpv4SrvTest, siaddr) {
     EXPECT_EQ("192.0.2.123", offer->getSiaddr().toText());
 }
 
+// Checks if the next-server defined as global value is overridden by subnet
+// specific value and returned in server messagey. There's also similar test for
+// checking parser only configuration, see Dhcp4ParserTest.nextServerOverride in
+// config_parser_unittest.cc.
+TEST_F(Dhcpv4SrvTest, nextServerOverride) {
+
+    NakedDhcpv4Srv srv(0);
+
+    ConstElementPtr status;
+
+    string config = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"next-server\": \"192.0.0.1\", "
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"next-server\": \"1.2.3.4\", "
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    ElementPtr json = Element::fromJSON(config);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(srv, json));
+
+    // check if returned status is OK
+    ASSERT_TRUE(status);
+    comment_ = config::parseAnswer(rcode_, status);
+    ASSERT_EQ(0, rcode_);
+
+    Pkt4Ptr dis = Pkt4Ptr(new Pkt4(DHCPDISCOVER, 1234));
+    dis->setRemoteAddr(IOAddress("192.0.2.1"));
+    OptionPtr clientid = generateClientId();
+    dis->addOption(clientid);
+
+    // Pass it to the server and get an offer
+    Pkt4Ptr offer = srv.processDiscover(dis);
+    ASSERT_TRUE(offer);
+    EXPECT_EQ(DHCPOFFER, offer->getType());
+
+    EXPECT_EQ("1.2.3.4", offer->getSiaddr().toText());
+}
+
+// Checks if the next-server defined as global value is used in responses
+// when there is no specific value defined in subnet and returned to the client
+// properly. There's also similar test for checking parser only configuration,
+// see Dhcp4ParserTest.nextServerGlobal in config_parser_unittest.cc.
+TEST_F(Dhcpv4SrvTest, nextServerGlobal) {
+
+    NakedDhcpv4Srv srv(0);
+
+    ConstElementPtr status;
+
+    string config = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"next-server\": \"192.0.0.1\", "
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    ElementPtr json = Element::fromJSON(config);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(srv, json));
+
+    // check if returned status is OK
+    ASSERT_TRUE(status);
+    comment_ = config::parseAnswer(rcode_, status);
+    ASSERT_EQ(0, rcode_);
+
+    Pkt4Ptr dis = Pkt4Ptr(new Pkt4(DHCPDISCOVER, 1234));
+    dis->setRemoteAddr(IOAddress("192.0.2.1"));
+    OptionPtr clientid = generateClientId();
+    dis->addOption(clientid);
+
+    // Pass it to the server and get an offer
+    Pkt4Ptr offer = srv.processDiscover(dis);
+    ASSERT_TRUE(offer);
+    EXPECT_EQ(DHCPOFFER, offer->getType());
+
+    EXPECT_EQ("192.0.0.1", offer->getSiaddr().toText());
+}
+
+
 // a dummy MAC address
 const uint8_t dummyMacAddr[] = {0, 1, 2, 3, 4, 5};