[4552] Addressed review comments.

doc/guide/dhcp4-srv.xml

@@ -2837,8 +2837,8 @@ It is merely echoed by the server
     "subnet4": [ {
         "reservations": [
         {
-            <userinput>"hw-address": "aa:bb:cc:dd:ee:ff",
-            "next-server": "10.1.1.2",
+            "hw-address": "aa:bb:cc:dd:ee:ff",
+            <userinput>"next-server": "10.1.1.2",
             "server-hostname": "server-hostname.example.org",
             "boot-file-name": "/tmp/bootfile.efi"</userinput>
         } ]

src/bin/dhcp4/tests/dora_unittest.cc

@@ -77,7 +77,7 @@ namespace {
 ///   - 1 subnet: 10.0.0.0/24
 ///   - 1 reservation for this subnet:
 ///     - Client's HW address: aa:bb:cc:dd:ee:ff
-///     - next-server = 192.0.0.2
+///     - next-server = 10.0.0.7
 ///     - server name = "some-name.example.org"
 ///     - boot-file-name = "bootfile.efi"
 const char* DORA_CONFIGS[] = {
@@ -262,7 +262,7 @@ const char* DORA_CONFIGS[] = {
         "       }"
         "    ]"
         "} ]"
-    "}",
+    "}"
 };
 
 /// @brief Test fixture class for testing 4-way (DORA) exchanges.
@@ -1028,8 +1028,8 @@ TEST_F(DORATest, messageFieldsReservations) {
     client.setHWAddress("aa:bb:cc:dd:ee:ff");
     // Configure DHCP server.
     configure(DORA_CONFIGS[6], *client.getServer());
-    // Client A performs 4-way exchange and should obtain a reserved
-    // address.
+    // Client performs 4-way exchange and should obtain a reserved
+    // address and fixed fields.
     ASSERT_NO_THROW(client.doDORA(boost::shared_ptr<
                                   IOAddress>(new IOAddress("0.0.0.0"))));
     // Make sure that the server responded.

src/bin/dhcp4/tests/inform_unittest.cc

@@ -39,6 +39,16 @@ namespace {
 ///   - Domain Name Server option present: 192.0.2.202, 192.0.2.203.
 ///   - Log Servers option present: 192.0.2.200 and 192.0.2.201
 ///   - Quotes Servers option present: 192.0.2.202, 192.0.2.203.
+///
+/// - Configuration 2:
+///   - This configuration provides reservations for next-server,
+///     server-hostname and boot-file-name value.
+///   - 1 subnet: 192.0.2.0/24
+///   - 1 reservation for this subnet:
+///     - Client's HW address: aa:bb:cc:dd:ee:ff
+///     - next-server = 10.0.0.7
+///     - server name = "some-name.example.org"
+///     - boot-file-name = "bootfile.efi"
 const char* INFORM_CONFIGS[] = {
 // Configuration 0
     "{ \"interfaces-config\": {"
@@ -91,7 +101,26 @@ const char* INFORM_CONFIGS[] = {
         "        \"data\": \"10.0.0.202,10.0.0.203\""
         "    } ]"
         " } ]"
-    "}"
+    "}",
+
+// Configuration 2
+    "{ \"interfaces-config\": {"
+        "      \"interfaces\": [ \"*\" ]"
+        "},"
+        "\"valid-lifetime\": 600,"
+        "\"next-server\": \"10.0.0.1\","
+        "\"subnet4\": [ { "
+        "    \"subnet\": \"192.0.2.0/24\", "
+        "    \"reservations\": [ "
+        "       {"
+        "         \"hw-address\": \"aa:bb:cc:dd:ee:ff\","
+        "         \"next-server\": \"10.0.0.7\","
+        "         \"server-hostname\": \"some-name.example.org\","
+        "         \"boot-file-name\": \"bootfile.efi\""
+        "       }"
+        "    ]"
+        "} ]"
+    "}",
 };
 
 /// @brief Test fixture class for testing DHCPINFORM.
@@ -364,6 +393,32 @@ TEST_F(InformTest, relayedClientNoCiaddr) {
     EXPECT_EQ("192.0.2.203", client.config_.dns_servers_[1].toText());
 }
 
+// This test verifies that the server assigns reserved values for the
+// siaddr, sname and file fields carried within DHCPv4 message.
+TEST_F(InformTest, messageFieldsReservations) {
+    // Client has a reservation.
+    Dhcp4Client client(Dhcp4Client::SELECTING);
+    // Message is relayed.
+    client.useRelay();
+    // Set explicit HW address so as it matches the reservation in the
+    // configuration used below.
+    client.setHWAddress("aa:bb:cc:dd:ee:ff");
+    // Configure DHCP server.
+    configure(INFORM_CONFIGS[2], *client.getServer());
+    // Client sends DHCPINFORM and should receive reserved fields.
+    ASSERT_NO_THROW(client.doInform());
+    // Make sure that the server responded.
+    ASSERT_TRUE(client.getContext().response_);
+    Pkt4Ptr resp = client.getContext().response_;
+    // Make sure that the server has responded with DHCPACK.
+    ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
+
+    // Check that the reserved values have been assigned.
+    EXPECT_EQ("10.0.0.7", client.config_.siaddr_.toText());
+    EXPECT_EQ("some-name.example.org", client.config_.sname_);
+    EXPECT_EQ("bootfile.efi", client.config_.boot_file_name_);
+}
+
 /// This test verifies that after a client completes its INFORM exchange,
 /// appropriate statistics are updated.
 TEST_F(InformTest, statisticsInform) {

src/lib/dhcpsrv/host.cc

@@ -5,6 +5,7 @@
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 #include <config.h>
+#include <dhcp/pkt4.h>
 #include <dhcpsrv/host.h>
 #include <util/encode/hex.h>
 #include <util/strutil.h>
@@ -365,7 +366,7 @@ Host::setNextServer(const asiolink::IOAddress& next_server) {
     if (!next_server.isV4()) {
         isc_throw(isc::BadValue, "next server address '" << next_server
                   << "' is not a valid IPv4 address");
-    } else if (next_server.isV4Zero() || next_server.isV4Bcast()) {
+    } else if (next_server.isV4Bcast()) {
         isc_throw(isc::BadValue, "invalid next server address '"
                   << next_server << "'");
     }
@@ -373,6 +374,24 @@ Host::setNextServer(const asiolink::IOAddress& next_server) {
     next_server_ = next_server;
 }
 
+void
+Host::setServerHostname(const std::string& server_host_name) {
+    if (server_host_name.size() > Pkt4::MAX_SNAME_LEN - 1) {
+        isc_throw(isc::BadValue, "server hostname length must not exceed "
+                  << (Pkt4::MAX_SNAME_LEN - 1));
+    }
+    server_host_name_ = server_host_name;
+}
+
+void
+Host::setBootFileName(const std::string& boot_file_name) {
+    if (boot_file_name.size() > Pkt4::MAX_FILE_LEN - 1) {
+        isc_throw(isc::BadValue, "boot file length must not exceed "
+                  << (Pkt4::MAX_FILE_LEN - 1));
+    }
+    boot_file_name_ = boot_file_name;
+}
+
 std::string
 Host::toText() const {
     std::ostringstream s;

src/lib/dhcpsrv/host.h

@@ -466,7 +466,7 @@ public:
     /// @param next_server New address of a next server.
     ///
     /// @throw isc::BadValue if the provided address is not an IPv4 address,
-    /// is a 0 address or broadcast address.
+    /// is broadcast address.
     void setNextServer(const asiolink::IOAddress& next_server);
 
     /// @brief Returns value of next server field (siaddr).
@@ -477,9 +477,9 @@ public:
     /// @brief Sets new value for server hostname (sname).
     ///
     /// @param server_host_name New value for server hostname.
-    void setServerHostname(const std::string& server_host_name) {
-        server_host_name_ = server_host_name;
-    }
+    ///
+    /// @throw BadValue if hostname is longer than 63 bytes.
+    void setServerHostname(const std::string& server_host_name);
 
     /// @brief Returns value of server hostname (sname).
     const std::string& getServerHostname() const {
@@ -489,9 +489,9 @@ public:
     /// @brief Sets new value for boot file name (file).
     ///
     /// @param boot_file_name New value of boot file name.
-    void setBootFileName(const std::string& boot_file_name) {
-        boot_file_name_ = boot_file_name;
-    }
+    ///
+    /// @throw BadValue if boot file name is longer than 128 bytes.
+    void setBootFileName(const std::string& boot_file_name);
 
     /// @brief Returns value of boot file name (file).
     const std::string& getBootFileName() const {

src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc

@@ -20,6 +20,7 @@
 #include <boost/pointer_cast.hpp>
 #include <gtest/gtest.h>
 #include <iterator>
+#include <sstream>
 #include <string>
 #include <vector>
 
@@ -327,6 +328,43 @@ TEST_F(HostReservationParserTest, dhcp4MessageFields) {
     EXPECT_EQ("/tmp/some-file.efi", hosts[0]->getBootFileName());
 }
 
+// This test verifies that the invalid value of the next server is rejected.
+TEST_F(HostReservationParserTest, invalidNextServer) {
+    // Invalid IPv4 address.
+    std::string config = "{ \"hw-address\": \"1:2:3:4:5:6\","
+        "\"next-server\": \"192.0.2.foo\" }";
+    testInvalidConfig<HostReservationParser4>(config);
+
+    // Broadcast address.
+    config = "{ \"hw-address\": \"1:2:3:4:5:6\","
+        "\"next-server\": \"255.255.255.255\" }";
+    testInvalidConfig<HostReservationParser4>(config);
+
+    // IPv6 address.
+    config = "{ \"hw-address\": \"1:2:3:4:5:6\","
+        "\"next-server\": \"2001:db8:1::1\" }";
+    testInvalidConfig<HostReservationParser4>(config);
+}
+
+// This test verifies that the invalid server hostname is rejected.
+TEST_F(HostReservationParserTest, invalidServerHostname) {
+    std::ostringstream config;
+    config << "{ \"hw-address\": \"1:2:3:4:5:6\","
+        "\"server-hostname\": \"";
+    config << std::string(64, 'a');
+    config << "\" }";
+    testInvalidConfig<HostReservationParser4>(config.str());
+}
+
+// This test verifies that the invalid boot file name is rejected.
+TEST_F(HostReservationParserTest, invalidBootFileName) {
+    std::ostringstream config;
+    config << "{ \"hw-address\": \"1:2:3:4:5:6\","
+        "\"boot-file-name\": \"";
+    config << std::string(128, 'a');
+    config << "\" }";
+    testInvalidConfig<HostReservationParser4>(config.str());
+}
 
 // This test verifies that the configuration parser for host reservations
 // throws an exception when IPv6 address is specified for IPv4 address

src/lib/dhcpsrv/tests/host_unittest.cc

@@ -686,11 +686,11 @@ TEST_F(HostTest, setValues) {
     EXPECT_THROW(host->setIPv4Reservation(IOAddress::IPV4_BCAST_ADDRESS()),
                  isc::BadValue);
 
-    // Zero or broadcast are invalid addresses for next server.
-    EXPECT_THROW(host->setNextServer(asiolink::IOAddress::IPV4_ZERO_ADDRESS()),
-                 isc::BadValue);
+    // Broadcast and IPv6 are invalid addresses for next server.
     EXPECT_THROW(host->setNextServer(asiolink::IOAddress::IPV4_BCAST_ADDRESS()),
                                      isc::BadValue);
+    EXPECT_THROW(host->setNextServer(IOAddress("2001:db8:1::1")),
+                                     isc::BadValue);
 }
 
 // Test that Host constructors initialize client classes from string.