From: Marcin Siodelski Date: Tue, 22 Oct 2013 12:47:24 +0000 (+0200) Subject: [3200] Changes after code review. X-Git-Tag: bind10-1.2.0beta1-release~102^2~62^2~3 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=1bbab8c3c1d12e1a7aaa2e2fc36ab4fc5653c92e;p=thirdparty%2Fkea.git [3200] Changes after code review. --- diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index 5423a21383..1bfce66e71 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/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 diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.cc b/src/bin/dhcp4/tests/dhcp4_test_utils.cc index 81416b6dcf..471e0beb67 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc +++ b/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. diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.h b/src/bin/dhcp4/tests/dhcp4_test_utils.h index 97eaaff106..ee19728ce5 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.h +++ b/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);