[3200] Changes after code review.

src/bin/dhcp4/dhcp4_messages.mes

@@ -118,7 +118,7 @@ a lease. It is up to the client to choose one server out of othe advertised
 and continue allocation with that server. This is a normal behavior and
 indicates successful operation.
 
-% DHCP4_LEASE_ADVERT_FAIL failed to advertise a lease for client client-id %1, hwaddr %2, yiaddr %3
+% DHCP4_LEASE_ADVERT_FAIL failed to advertise a lease for client client-id %1, hwaddr %2, client sent yiaddr %3
 This message indicates that the server has failed to offer a lease to
 the specified client after receiving a DISCOVER message from it. There are
 many possible reasons for such a failure.
@@ -128,7 +128,7 @@ This debug message indicates that the server successfully granted a lease
 in response to client's REQUEST message. This is a normal behavior and
 indicates successful operation.
 
-% DHCP4_LEASE_ALLOC_FAIL failed to grant a lease for client-id %1, hwaddr %2, yiaddr %3
+% DHCP4_LEASE_ALLOC_FAIL failed to grant a lease for client-id %1, hwaddr %2, client sent yiaddr %3
 This message indicates that the server failed to grant a lease to the
 specified client after receiving a REQUEST message from it.  There are many
 possible reasons for such a failure. Additional messages will indicate the

src/bin/dhcp4/tests/dhcp4_test_utils.cc

@@ -124,51 +124,87 @@ void Dhcpv4SrvTest::messageCheck(const Pkt4Ptr& q, const Pkt4Ptr& a) {
     EXPECT_TRUE(q->getLocalHWAddr() == a->getLocalHWAddr());
     EXPECT_TRUE(q->getRemoteHWAddr() == a->getRemoteHWAddr());
 
-    // Check that bare minimum of required options are there.
-    // We don't check options requested by a client. Those
-    // are checked elsewhere.
-    EXPECT_TRUE(a->getOption(DHO_SUBNET_MASK));
-    EXPECT_TRUE(a->getOption(DHO_ROUTERS));
+    // Check that the server identifier is present in the response.
+    // Presence (or absence) of other options is checked elsewhere.
     EXPECT_TRUE(a->getOption(DHO_DHCP_SERVER_IDENTIFIER));
-    EXPECT_TRUE(a->getOption(DHO_DHCP_LEASE_TIME));
-    EXPECT_TRUE(a->getOption(DHO_SUBNET_MASK));
-    EXPECT_TRUE(a->getOption(DHO_DOMAIN_NAME));
-    EXPECT_TRUE(a->getOption(DHO_DOMAIN_NAME_SERVERS));
 
     // Check that something is offered
     EXPECT_TRUE(a->getYiaddr().toText() != "0.0.0.0");
 }
 
-void Dhcpv4SrvTest::optionsCheck(const Pkt4Ptr& pkt) {
-    // Check that the requested and configured options are returned
-    // in the ACK message.
-    EXPECT_TRUE(pkt->getOption(DHO_DOMAIN_NAME))
-        << "domain-name not present in the response";
-    EXPECT_TRUE(pkt->getOption(DHO_DOMAIN_NAME_SERVERS))
-        << "dns-servers not present in the response";
-    EXPECT_TRUE(pkt->getOption(DHO_LOG_SERVERS))
-        << "log-servers not present in the response";
-    EXPECT_TRUE(pkt->getOption(DHO_COOKIE_SERVERS))
-        << "cookie-servers not present in the response";
-    // Check that the requested but not configured options are not
-    // returned in the ACK message.
-    EXPECT_FALSE(pkt->getOption(DHO_LPR_SERVERS))
-        << "domain-name present in the response but it is"
-        << " expected not to be present";
+::testing::AssertionResult
+Dhcpv4SrvTest::basicOptionsPresent(const Pkt4Ptr& pkt) {
+    std::ostringstream errmsg;
+    errmsg << "option missing in the response";
+    if (!pkt->getOption(DHO_DOMAIN_NAME)) {
+        return (::testing::AssertionFailure() << "domain-name " << errmsg);
+
+    } else if (!pkt->getOption(DHO_DOMAIN_NAME_SERVERS)) {
+        return (::testing::AssertionFailure() << "dns-servers " << errmsg);
+
+    } else if (!pkt->getOption(DHO_SUBNET_MASK)) {
+        return (::testing::AssertionFailure() << "subnet-mask " << errmsg);
+
+    } else if (!pkt->getOption(DHO_ROUTERS)) {
+        return (::testing::AssertionFailure() << "routers " << errmsg);
+
+    } else if (!pkt->getOption(DHO_DHCP_LEASE_TIME)) {
+        return (::testing::AssertionFailure() << "dhcp-lease-time " << errmsg);
+
+    }
+    return (::testing::AssertionSuccess());
+
 }
 
-void Dhcpv4SrvTest::noOptionsCheck(const Pkt4Ptr& pkt) {
-    // Check that certain options are not returned in the packet.
-    // This is the case, when client didn't ask for them or when
-    // NAK was returned by the server.
-    EXPECT_FALSE(pkt->getOption(DHO_DOMAIN_NAME))
-        << "domain-name present in the response";
-    EXPECT_FALSE(pkt->getOption(DHO_DOMAIN_NAME_SERVERS))
-        << "dns-servers present in the response";
-    EXPECT_FALSE(pkt->getOption(DHO_LOG_SERVERS))
-        << "log-servers present in the response";
-    EXPECT_FALSE(pkt->getOption(DHO_COOKIE_SERVERS))
-        << "cookie-servers present in the response";
+::testing::AssertionResult
+Dhcpv4SrvTest::noBasicOptions(const Pkt4Ptr& pkt) {
+    std::ostringstream errmsg;
+    errmsg << "option present in the response";
+    if (pkt->getOption(DHO_DOMAIN_NAME)) {
+        return (::testing::AssertionFailure() << "domain-name " << errmsg);
+
+    } else if (pkt->getOption(DHO_DOMAIN_NAME_SERVERS)) {
+        return (::testing::AssertionFailure() << "dns-servers " << errmsg);
+
+    } else if (pkt->getOption(DHO_SUBNET_MASK)) {
+        return (::testing::AssertionFailure() << "subnet-mask " << errmsg);
+
+    } else if (pkt->getOption(DHO_ROUTERS)) {
+        return (::testing::AssertionFailure() << "routers " << errmsg);
+
+    } else if (pkt->getOption(DHO_DHCP_LEASE_TIME)) {
+        return (::testing::AssertionFailure() << "dhcp-lease-time " << errmsg);
+
+    }
+    return (::testing::AssertionSuccess());
+}
+
+::testing::AssertionResult
+Dhcpv4SrvTest::requestedOptionsPresent(const Pkt4Ptr& pkt) {
+    std::ostringstream errmsg;
+    errmsg << "option missing in the response";
+    if (!pkt->getOption(DHO_LOG_SERVERS)) {
+        return (::testing::AssertionFailure() << "log-servers " << errmsg);
+
+    } else if (!pkt->getOption(DHO_COOKIE_SERVERS)) {
+        return (::testing::AssertionFailure() << "cookie-servers " << errmsg);
+
+    }
+    return (::testing::AssertionSuccess());
+}
+
+::testing::AssertionResult
+Dhcpv4SrvTest::noRequestedOptions(const Pkt4Ptr& pkt) {
+    std::ostringstream errmsg;
+    errmsg << "option present in the response";
+    if (pkt->getOption(DHO_LOG_SERVERS)) {
+        return (::testing::AssertionFailure() << "log-servers " << errmsg);
+
+    } else if (pkt->getOption(DHO_COOKIE_SERVERS)) {
+        return (::testing::AssertionFailure() << "cookie-servers " << errmsg);
+
+    }
+    return (::testing::AssertionSuccess());
 }
 
 OptionPtr Dhcpv4SrvTest::generateClientId(size_t size /*= 4*/) {
@@ -397,11 +433,11 @@ void Dhcpv4SrvTest::testDiscoverRequest(const uint8_t msg_type) {
 
     messageCheck(received, rsp);
 
+    // Basic options should be present when we got the lease.
+    EXPECT_TRUE(basicOptionsPresent(rsp));
     // We did not request any options so these should not be present
     // in the RSP.
-    EXPECT_FALSE(rsp->getOption(DHO_LOG_SERVERS));
-    EXPECT_FALSE(rsp->getOption(DHO_COOKIE_SERVERS));
-    EXPECT_FALSE(rsp->getOption(DHO_LPR_SERVERS));
+    EXPECT_TRUE(noRequestedOptions(rsp));
 
     // Repeat the test but request some options.
     // Add 'Parameter Request List' option.
@@ -426,7 +462,8 @@ void Dhcpv4SrvTest::testDiscoverRequest(const uint8_t msg_type) {
     }
 
     // Check that the requested options are returned.
-    optionsCheck(rsp);
+    EXPECT_TRUE(basicOptionsPresent(rsp));
+    EXPECT_TRUE(requestedOptionsPresent(rsp));
 
     // The following part of the test will test that the NAK is sent when
     // there is no address pool configured. In the same time, we expect
@@ -456,7 +493,10 @@ void Dhcpv4SrvTest::testDiscoverRequest(const uint8_t msg_type) {
     ASSERT_EQ("0.0.0.0", rsp->getYiaddr().toText());
 
     // Make sure that none of the requested options is returned in NAK.
-    noOptionsCheck(rsp);
+    // Also options such as Routers or Subnet Mask should not be there,
+    // because lease hasn't been acquired.
+    EXPECT_TRUE(noRequestedOptions(rsp));
+    EXPECT_TRUE(noBasicOptions(rsp));
 }
 
 /// @brief This function cleans up after the test.

src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -109,15 +109,32 @@ public:
     /// @param a answer (server's message)
     void messageCheck(const Pkt4Ptr& q, const Pkt4Ptr& a);
 
-    /// @brief Check that requested options are present.
+    /// @brief Check that certain basic (always added when lease is acquired)
+    /// are present in a message.
     ///
-    /// @param pkt packet to be checked.
-    void optionsCheck(const Pkt4Ptr& pkt);
+    /// @param pkt A message to be checked.
+    /// @return Assertion result which indicates whether test passed or failed.
+    ::testing::AssertionResult basicOptionsPresent(const Pkt4Ptr& pkt);
 
-    /// @brief Check that certain options are not present.
+    /// @brief Check that certain basic (always added when lease is acquired)
+    /// are not present.
     ///
     /// @param pkt A packet to be checked.
-    void noOptionsCheck(const Pkt4Ptr& pkt);
+    /// @return Assertion result which indicates whether test passed or failed.
+    ::testing::AssertionResult noBasicOptions(const Pkt4Ptr& pkt);
+
+    /// @brief Check that certain requested options are present in the message.
+    ///
+    /// @param pkt A message to be checked.
+    /// @return Assertion result which indicates whether test passed or failed.
+    ::testing::AssertionResult requestedOptionsPresent(const Pkt4Ptr& pkt);
+
+    /// @brief Check that certain options (requested with PRL option)
+    /// are not present.
+    ///
+    /// @param pkt A packet to be checked.
+    /// @return Assertion result which indicates whether test passed or failed.
+    ::testing::AssertionResult noRequestedOptions(const Pkt4Ptr& pkt);
 
     /// @brief generates client-id option
     ///
@@ -221,6 +238,14 @@ public:
 
     /// @brief Tests if Discover or Request message is processed correctly
     ///
+    /// This test verifies that the Parameter Request List option is handled
+    /// correctly, i.e. it checks that certain options are present in the
+    /// server's response when they are requested and that they are not present
+    /// when they are not requested or NAK occurs.
+    ///
+    /// @todo We need an additional test for PRL option using real traffic
+    /// capture.
+    ///
     /// @param msg_type DHCPDISCOVER or DHCPREQUEST
     void testDiscoverRequest(const uint8_t msg_type);