[3768] Address review comments.

3 files changed, 79 insertions, 63 deletions

diff --git a/src/bin/dhcp4/tests/dhcp4_client.cc b/src/bin/dhcp4/tests/dhcp4_client.cc
index dc80fb718a..a3c3436d74 100644
--- a/src/bin/dhcp4/tests/dhcp4_client.cc
+++ b/src/bin/dhcp4/tests/dhcp4_client.cc
@@ -87,6 +87,54 @@ Dhcp4Client::addRequestedAddress(const asiolink::IOAddress& addr) {
 }
 
 void
+Dhcp4Client::appendClientId() {
+    if (!context_.query_) {
+        isc_throw(Dhcp4ClientError, "pointer to the query must not be NULL"
+                  " when adding Client Identifier option");
+    }
+
+    if (clientid_) {
+        OptionPtr opt(new Option(Option::V4, DHO_DHCP_CLIENT_IDENTIFIER,
+                                 clientid_->getClientId()));
+        context_.query_->addOption(opt);
+    }
+}
+
+void
+Dhcp4Client::appendName() {
+    if (!context_.query_) {
+        isc_throw(Dhcp4ClientError, "pointer to the query must not be NULL"
+                  " when adding FQDN or Hostname option");
+    }
+
+    if (fqdn_) {
+        context_.query_->addOption(fqdn_);
+
+    } else if (hostname_) {
+        context_.query_->addOption(hostname_);
+    }
+}
+
+void
+Dhcp4Client::appendPRL() {
+    if (!context_.query_) {
+        isc_throw(Dhcp4ClientError, "pointer to the query must not be NULL"
+                  " when adding option codes to the PRL option");
+
+    } else if (!requested_options_.empty()) {
+        // Include Parameter Request List if at least one option code
+        // has been specified to be requested.
+        OptionUint8ArrayPtr prl(new OptionUint8Array(Option::V4,
+                                  DHO_DHCP_PARAMETER_REQUEST_LIST));
+        for (std::set<uint8_t>::const_iterator opt = requested_options_.begin();
+             opt != requested_options_.end(); ++opt) {
+            prl->addValue(*opt);
+        }
+        context_.query_->addOption(prl);
+    }
+}
+
+void
 Dhcp4Client::applyConfiguration() {
     Pkt4Ptr resp = context_.response_;
     if (!resp) {
@@ -152,11 +200,11 @@ void
 Dhcp4Client::doDiscover(const boost::shared_ptr<IOAddress>& requested_addr) {
     context_.query_ = createMsg(DHCPDISCOVER);
     // Request options if any.
-    includePRL();
+    appendPRL();
     // Include FQDN or Hostname.
-    includeName();
+    appendName();
     // Include Client Identifier
-    includeClientId();
+    appendClientId();
     if (requested_addr) {
         addRequestedAddress(*requested_addr);
     }
@@ -182,7 +230,7 @@ void
 Dhcp4Client::doInform(const bool set_ciaddr) {
     context_.query_ = createMsg(DHCPINFORM);
     // Request options if any.
-    includePRL();
+    appendPRL();
     // The client sending a DHCPINFORM message has an IP address obtained
     // by some other means, e.g. static configuration. The lease which we
     // are using here is most likely set by the createLease method.
@@ -245,11 +293,11 @@ Dhcp4Client::doRequest() {
     }
 
     // Request options if any.
-    includePRL();
+    appendPRL();
     // Include FQDN or Hostname.
-    includeName();
+    appendName();
     // Include Client Identifier
-    includeClientId();
+    appendClientId();
     // Send the message to the server.
     sendMsg(context_.query_);
     // Expect response.
@@ -271,20 +319,6 @@ Dhcp4Client::includeClientId(const std::string& clientid) {
 }
 
 void
-Dhcp4Client::includeClientId() {
-    if (!context_.query_) {
-        isc_throw(Dhcp4ClientError, "pointer to the query must not be NULL"
-                  " when adding Client Identifier option");
-    }
-
-    if (clientid_) {
-        OptionPtr opt(new Option(Option::V4, DHO_DHCP_CLIENT_IDENTIFIER,
-                                 clientid_->getClientId()));
-        context_.query_->addOption(opt);
-    }
-}
-
-void
 Dhcp4Client::includeFQDN(const uint8_t flags, const std::string& fqdn_name,
                          Option4ClientFqdn::DomainNameType fqdn_type) {
     fqdn_.reset(new Option4ClientFqdn(flags, Option4ClientFqdn::RCODE_CLIENT(),
@@ -296,39 +330,6 @@ Dhcp4Client::includeHostname(const std::string& name) {
     hostname_.reset(new OptionString(Option::V4, DHO_HOST_NAME, name));
 }
 
-void
-Dhcp4Client::includeName() {
-    if (!context_.query_) {
-        isc_throw(Dhcp4ClientError, "pointer to the query must not be NULL"
-                  " when adding FQDN or Hostname option");
-    }
-
-    if (fqdn_) {
-        context_.query_->addOption(fqdn_);
-
-    } else if (hostname_) {
-        context_.query_->addOption(hostname_);
-    }
-}
-
-void
-Dhcp4Client::includePRL() {
-    if (!context_.query_) {
-        isc_throw(Dhcp4ClientError, "pointer to the query must not be NULL"
-                  " when adding option codes to the PRL option");
-
-    } else if (!requested_options_.empty()) {
-        // Include Parameter Request List if at least one option code
-        // has been specified to be requested.
-        OptionUint8ArrayPtr prl(new OptionUint8Array(Option::V4,
-                                  DHO_DHCP_PARAMETER_REQUEST_LIST));
-        for (std::set<uint8_t>::const_iterator opt = requested_options_.begin();
-             opt != requested_options_.end(); ++opt) {
-            prl->addValue(*opt);
-        }
-        context_.query_->addOption(prl);
-    }
-}
 
 HWAddrPtr
 Dhcp4Client::generateHWAddr(const uint8_t htype) const {
@@ -369,7 +370,6 @@ Dhcp4Client::requestOptions(const uint8_t option1, const uint8_t option2,
     requestOption(option3);
 }
 
-
 Pkt4Ptr
 Dhcp4Client::receiveOneMsg() {
     // Return empty pointer if server hasn't responded.
diff --git a/src/bin/dhcp4/tests/dhcp4_client.h b/src/bin/dhcp4/tests/dhcp4_client.h
index bc8ddffd0f..38b9025d80 100644
--- a/src/bin/dhcp4/tests/dhcp4_client.h
+++ b/src/bin/dhcp4/tests/dhcp4_client.h
@@ -375,14 +375,14 @@ private:
     /// This function creates an instance of the Client Identifier option
     /// if the client identifier has been specified and includes this
     /// option in the client's message to the server.
-    void includeClientId();
+    void appendClientId();
 
     /// @brief Includes FQDN or Hostname option in the client's message.
     ///
     /// This method checks if @c fqdn_ or @c hostname_ is specified and
     /// includes it in the client's message. If both are specified, the
     /// @c fqdn_ will be used.
-    void includeName();
+    void appendName();
 
     /// @brief Include PRL Option in the query message.
     ///
@@ -390,7 +390,7 @@ private:
     /// option and adds option codes from the @c requested_options_ to it.
     /// It later adds the PRL option to the @c context_.query_ message
     /// if it is non-NULL.
-    void includePRL();
+    void appendPRL();
 
     /// @brief Simulates reception of the message from the server.
     ///
diff --git a/src/bin/dhcp4/tests/dora_unittest.cc b/src/bin/dhcp4/tests/dora_unittest.cc
index fdf32fec43..7ab47b3337 100644
--- a/src/bin/dhcp4/tests/dora_unittest.cc
+++ b/src/bin/dhcp4/tests/dora_unittest.cc
@@ -191,6 +191,7 @@ public:
     ///   lease without a unique identifier.
     /// - The client sends the DHCPREQUEST and the server sends the DHCPNAK for the
     ///   same reason.
+    /// - The client A renews its address successfully.
     ///
     /// The specific test cases using this test must make sure that one of the
     /// provided parameters is an empty string. This simulates the situation where
@@ -218,7 +219,7 @@ public:
     ///   identifier of the client A.
     /// - Client B sends DHCPDISCOVER.
     /// - Server should determine that the client B is not client A, because
-    ///   it is using a different hadrware address or client identifier, even
+    ///   it is using a different hadrware address or client identifier.
     ///   As a consequence, the server should offer a different address to the
     ///   client B.
     /// - The client B performs the 4-way exchange again, and the server
@@ -226,6 +227,7 @@ public:
     ///   than the address used by the client A.
     /// - Client B is in the renewing state and it successfully renews its
     ///   address.
+    /// - The client A also renews its address successfully.
     ///
     /// @param hwaddr_a HW address of client A.
     /// @param clientid_a Client id of client A.
@@ -569,6 +571,14 @@ DORATest::twoAllocationsOverlapTest(const std::string& hwaddr_a,
     resp_b = client_b.getContext().response_;
     ASSERT_EQ(DHCPACK, static_cast<int>(resp_b->getType()));
     ASSERT_NE(client_b.config_.lease_.addr_, client_a.config_.lease_.addr_);
+
+    // Client A should also be able to renew its address.
+    client_a.setState(Dhcp4Client::RENEWING);
+    ASSERT_NO_THROW(client_a.doRequest());
+    ASSERT_TRUE(client_a.getContext().response_);
+    resp_b = client_a.getContext().response_;
+    ASSERT_EQ(DHCPACK, static_cast<int>(resp_b->getType()));
+    ASSERT_NE(client_a.config_.lease_.addr_, client_b.config_.lease_.addr_);
 }
 
 void
@@ -612,6 +622,14 @@ DORATest::oneAllocationOverlapTest(const std::string& hwaddr_a,
     resp_b = client_b.getContext().response_;
     ASSERT_TRUE(resp_b);
     ASSERT_EQ(DHCPNAK, static_cast<int>(resp_b->getType()));
+
+    // Client A should also be able to renew its address.
+    client_a.setState(Dhcp4Client::RENEWING);
+    ASSERT_NO_THROW(client_a.doRequest());
+    ASSERT_TRUE(client_a.getContext().response_);
+    resp_b = client_a.getContext().response_;
+    ASSERT_EQ(DHCPACK, static_cast<int>(resp_b->getType()));
+    ASSERT_NE(client_a.config_.lease_.addr_, client_b.config_.lease_.addr_);
 }
 
 // This test checks the server behavior in the following situation:
@@ -630,10 +648,7 @@ DORATest::oneAllocationOverlapTest(const std::string& hwaddr_a,
 //   than the address used by the client A.
 // - Client B is in the renewing state and it successfully renews its
 //   address.
-// - Client B goes to INIT_REBOOT state and asks for the address used
-//   by the client A.
-// - The server sends DHCPNAK to refuse the allocation of this address
-//   to the client B.
+// - Client A also renews its address successfully.
 TEST_F(DORATest, twoAllocationsOverlap1) {
     twoAllocationsOverlapTest("01:02:03:04:05:06", "12:34",
                               "02:02:03:03:04:04", "12:34");
@@ -659,6 +674,7 @@ TEST_F(DORATest, twoAllocationsOverlap2) {
 //   lease without a unique identifier.
 // - The client sends the DHCPREQUEST and the server sends the DHCPNAK for the
 //   same reason.
+// - Client A renews its address successfully.
 TEST_F(DORATest, oneAllocationOverlap1) {
     oneAllocationOverlapTest("01:02:03:04:05:06", "12:34",
                              "01:02:03:04:05:06", "");