From: Marcin Siodelski Date: Tue, 10 Dec 2013 13:10:40 +0000 (+0100) Subject: [3181] Addressed review comments. X-Git-Tag: bind10-1.2.0beta1-release~178^2~4^2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=840afe672b9002ddecba485aec09ad085e09baeb;p=thirdparty%2Fkea.git [3181] Addressed review comments. --- diff --git a/tests/tools/perfdhcp/command_options.cc b/tests/tools/perfdhcp/command_options.cc index db3305aef0..26255e8e02 100644 --- a/tests/tools/perfdhcp/command_options.cc +++ b/tests/tools/perfdhcp/command_options.cc @@ -743,8 +743,8 @@ CommandOptions::validate() const { "-r must be set to use -D"); check((getRate() != 0) && (getRenewRate() + getReleaseRate() > getRate()), "The sum of Renew rate (-f) and Release rate" - " (-F) must not be greater than the rate specified" - " as -r"); + " (-F) must not be greater than the exchange" + " rate specified as -r"); check((getRate() == 0) && (getRenewRate() != 0), "Renew rate specified as -f must not be specified" " when -r parameter is not specified"); @@ -1014,14 +1014,16 @@ CommandOptions::usage() const { "\n" "DHCPv6 only options:\n" "-c: Add a rapid commit option (exchanges will be SA).\n" - "-f: A rate at which IPv6 Renew requests are sent to\n" - " a server. The sum of this value and release-rate must be equal\n" - " or lower than the rate specified as -r. If -r is\n" - " not specified, this parameter must not be specified too.\n" - "-F: A rate at which IPv6 Release requests are sent to\n" - " a server. The sum of this value and renew-rate must be equal or\n" - " lower than the rate specified as -r. If -r is not\n" - " specified, this parameter must not be specified too.\n" + "-f: Rate at which IPv6 Renew requests are sent to\n" + " a server. This value is only valid when used in conjunction with\n" + " the exchange rate (given by -r). Furthermore the sum of\n" + " this value and the release-rate (given by -F: Rate at which IPv6 Release requests are sent to\n" + " a server. This value is only valid when used in conjunction with\n" + " the exchange rate (given by -r). Furthermore the sum of\n" + " this value and the renew-rate (given by -f 0 ? diff --git a/tests/tools/perfdhcp/rate_control.h b/tests/tools/perfdhcp/rate_control.h index c52f264226..cd49c2c07e 100644 --- a/tests/tools/perfdhcp/rate_control.h +++ b/tests/tools/perfdhcp/rate_control.h @@ -26,7 +26,7 @@ namespace perfdhcp { /// of the specific type are sent by perfdhcp. Each message type, /// for which the desired rate can be specified, has a corresponding /// \c RateControl object. So, the perfdhcp is using up to three objects -/// of this type in the same time, to control the rate of the following +/// of this type at the same time, to control the rate of the following /// messages being sent: /// - Discover(DHCPv4) or Solicit (DHCPv6) /// - Renew (DHCPv6) or Request (DHCPv4) to renew leases. @@ -34,16 +34,15 @@ namespace perfdhcp { /// /// The purpose of the RateControl class is to track the due time for /// sending next message (or bunch of messages) to keep outbound rate -/// of particular messages on the desired level. The due time is calculated +/// of particular messages at the desired level. The due time is calculated /// using the desired rate value and the timestamp when the last message of /// the particular type has been sent. That puts the responsibility on the /// \c TestControl class to invoke the \c RateControl::updateSendDue, every /// time the message is sent. /// /// The \c RateControl object returns the number of messages to be sent at -/// the time. Typically the number returned is 0, if perfdhcp shouldn't send -/// any messages yet, or 1 (sometimes more) if the send due time has been -/// reached. +/// the time. The number returned is 0, if perfdhcp shouldn't send any messages +/// yet, or 1 (sometimes more) if the send due time has been reached. class RateControl { public: @@ -68,7 +67,7 @@ public: /// \brief Returns number of messages to be sent "now". /// - /// This function calculates how many masseges of the given type should + /// This function calculates how many messages of the given type should /// be sent immediately when the call to the function returns, to catch /// up with the desired message rate. /// @@ -77,6 +76,25 @@ public: /// the due time has been hit, the non-zero number of messages is returned. /// If the due time hasn't been hit, the number returned is 0. /// + /// If the rate is non-zero, the number of messages to be sent is calculated + /// as follows: + /// \code + /// num = duration * rate + /// \endcode + /// where duration is a time period between the due time to send + /// next set of messages and current time. The duration is expressed in + /// seconds with the fractional part having 6 or 9 digits (depending on + /// the timer resolution). If the calculated value is equal to 0, it is + /// rounded to 1, so as at least one message is sent. + /// + /// The value of aggressivity limits the maximal number of messages to + /// be sent one after another. If the number of messages calculated with + /// the equation above exceeds the aggressivity, this function will return + /// the value equal to aggressivity. + /// + /// If the rate is not specified (equal to 0), the value calculated by + /// this function is equal to aggressivity. + /// /// \return A number of messages to be sent immediately. uint64_t getOutboundMessageCount(); @@ -88,7 +106,7 @@ public: /// \brief Returns the value of the late send flag. /// /// The flag returned by this function indicates whether the new due time - /// calculated by the \c RateControl::updateSendDue has been in the past. + /// calculated by the \c RateControl::updateSendDue is in the past. /// This value is used by the \c TestControl object to increment the counter /// of the late sent messages in the \c StatsMgr. bool isLateSent() const { @@ -97,17 +115,16 @@ public: /// \brief Sets the value of aggressivity. /// - /// \param aggressivity A new value of aggressivity. - void setAggressivity(const int aggressivity) { - aggressivity_ = aggressivity; - } + /// \param aggressivity A new value of aggressivity. This value must be + /// a positive integer. + /// \throw isc::BadValue if new value is not a positive integer. + void setAggressivity(const int aggressivity); /// \brief Sets the new rate. /// - /// \param rate A new value of rate. - void setRate(const int rate) { - rate_ = rate; - } + /// \param rate A new value of rate. This value must not be negative. + /// \throw isc::BadValue if new rate is negative. + void setRate(const int rate); /// \brief Sets the value of the due time. /// diff --git a/tests/tools/perfdhcp/stats_mgr.h b/tests/tools/perfdhcp/stats_mgr.h index 0ab4ce1487..7486b9e5e1 100644 --- a/tests/tools/perfdhcp/stats_mgr.h +++ b/tests/tools/perfdhcp/stats_mgr.h @@ -1181,7 +1181,7 @@ public: /// /// \param xchg_type exchange type. /// \return string representing name of the exchange. - std::string exchangeToString(ExchangeType xchg_type) const { + static std::string exchangeToString(ExchangeType xchg_type) { switch(xchg_type) { case XCHG_DO: return("DISCOVER-OFFER"); diff --git a/tests/tools/perfdhcp/test_control.cc b/tests/tools/perfdhcp/test_control.cc index 6e9e1b9260..01ceb899ea 100644 --- a/tests/tools/perfdhcp/test_control.cc +++ b/tests/tools/perfdhcp/test_control.cc @@ -1117,14 +1117,35 @@ TestControl::processReceivedPacket6(const TestControlSocket& socket, } } } else if (packet_type == DHCPV6_REPLY) { + // If the received message is Reply, we have to find out which exchange + // type the Reply message belongs to. It is doable by matching the Reply + // transaction id with the transaction id of the sent Request, Renew + // or Release. First we start with the Request. if (stats_mgr6_->passRcvdPacket(StatsMgr6::XCHG_RR, pkt6)) { + // The Reply belongs to Request-Reply exchange type. So, we may need + // to keep this Reply in the storage if Renews or/and Releases are + // being sent. Note that, Reply messages hold the information about + // leases assigned. We use this information to construct Renew and + // Release messages. if (stats_mgr6_->hasExchangeStats(StatsMgr6::XCHG_RN) || stats_mgr6_->hasExchangeStats(StatsMgr6::XCHG_RL)) { + // Renew or Release messages are sent, because StatsMgr has the + // specific exchange type specified. Let's append the Reply + // message to a storage. reply_storage_.append(pkt6); } + // The Reply message is not a server's response to the Request message + // sent within the 4-way exchange. It may be a response to the Renew + // or Release message. In the if clause we first check if StatsMgr + // has exchange type for Renew specified, and if it has, if there is + // a corresponding Renew message for the received Reply. If not, + // we check that StatsMgr has exchange type for Release specified, + // as possibly the Reply has been sent in response to Release. } else if (!(stats_mgr6_->hasExchangeStats(StatsMgr6::XCHG_RN) && stats_mgr6_->passRcvdPacket(StatsMgr6::XCHG_RN, pkt6)) && stats_mgr6_->hasExchangeStats(StatsMgr6::XCHG_RL)) { + // At this point, it is only possible that the Reply has been sent + // in response to a Release. Try to match the Reply with Release. stats_mgr6_->passRcvdPacket(StatsMgr6::XCHG_RL, pkt6); } } diff --git a/tests/tools/perfdhcp/tests/command_options_unittest.cc b/tests/tools/perfdhcp/tests/command_options_unittest.cc index 0696783756..3d2ce2e9a6 100644 --- a/tests/tools/perfdhcp/tests/command_options_unittest.cc +++ b/tests/tools/perfdhcp/tests/command_options_unittest.cc @@ -351,7 +351,10 @@ TEST_F(CommandOptionsTest, RenewRate) { EXPECT_THROW(process("perfdhcp -6 -r 10 -f 11 -l ethx all"), isc::InvalidParameter); // The renew-rate of 0 is invalid. - EXPECT_THROW(process("perfdhcp -6 -r 10 -f 0 - l ethx all"), + EXPECT_THROW(process("perfdhcp -6 -r 10 -f 0 -l ethx all"), + isc::InvalidParameter); + // The negative renew-rate is invalid. + EXPECT_THROW(process("perfdhcp -6 -r 10 -f -5 -l ethx all"), isc::InvalidParameter); // If -r is not specified the -f should not // be accepted. @@ -387,6 +390,9 @@ TEST_F(CommandOptionsTest, ReleaseRate) { // The release-rate of 0 is invalid. EXPECT_THROW(process("perfdhcp -6 -r 10 -F 0 -l ethx all"), isc::InvalidParameter); + // The negative rlease-rate is invalid. + EXPECT_THROW(process("perfdhcp -6 -r 10 -F -5 -l ethx all"), + isc::InvalidParameter); // If -r is not specified the -F should not // be accepted. EXPECT_THROW(process("perfdhcp -6 -F 10 -l ethx all"), diff --git a/tests/tools/perfdhcp/tests/rate_control_unittest.cc b/tests/tools/perfdhcp/tests/rate_control_unittest.cc index ae1897ac92..99fc35e8f6 100644 --- a/tests/tools/perfdhcp/tests/rate_control_unittest.cc +++ b/tests/tools/perfdhcp/tests/rate_control_unittest.cc @@ -80,6 +80,8 @@ TEST(RateControl, constructor) { // The 0 value of aggressivity < 1 is not acceptable. EXPECT_THROW(RateControl(3, 0), isc::BadValue); + // The negative value of rate is not acceptable. + EXPECT_THROW(RateControl(-1, 3), isc::BadValue); } // Check the aggressivity accessor. @@ -98,7 +100,8 @@ TEST(RateControl, getDue) { ASSERT_FALSE(rc.getDue().is_not_a_date_time()); rc.send_due_ = NakedRateControl::currentTime(); EXPECT_TRUE(NakedRateControl::currentTime() >= rc.getDue()); - rc.send_due_ = NakedRateControl::currentTime() + boost::posix_time::seconds(10); + rc.send_due_ = NakedRateControl::currentTime() + + boost::posix_time::seconds(10); EXPECT_TRUE(NakedRateControl::currentTime() < rc.getDue()); } @@ -126,7 +129,7 @@ TEST(RateControl, isLateSent) { // it is quite hard to fully test this function as its behaviour strongly // depends on time. TEST(RateControl, getOutboundMessageCount) { - NakedRateControl rc1; + NakedRateControl rc1(1000, 1); // Set the timestamp of the last sent message well to the past. // The resulting due time will be in the past too. rc1.last_sent_ = @@ -146,12 +149,29 @@ TEST(RateControl, getOutboundMessageCount) { // to the future. If the resulting due time is well in the future too, // the number of messages to be sent must be 0. NakedRateControl rc3(10, 3); - rc3.last_sent_ = NakedRateControl::currentTime() + boost::posix_time::seconds(5); + rc3.last_sent_ = NakedRateControl::currentTime() + + boost::posix_time::seconds(5); ASSERT_NO_THROW(count = rc3.getOutboundMessageCount()); EXPECT_EQ(0, count); } +// Test the aggressivity modifier for valid and invalid values. +TEST(RateControl, setAggressivity) { + NakedRateControl rc; + ASSERT_NO_THROW(rc.setAggressivity(1)); + EXPECT_THROW(rc.setAggressivity(0), isc::BadValue); + EXPECT_THROW(rc.setAggressivity(-1), isc::BadValue); +} + +// Test the rate modifier for valid and invalid rate values. +TEST(RateControl, setRate) { + NakedRateControl rc; + EXPECT_NO_THROW(rc.setRate(1)); + EXPECT_NO_THROW(rc.setRate(0)); + EXPECT_THROW(rc.setRate(-1), isc::BadValue); +} + // Test the function which calculates the due time to send next set of // messages. TEST(RateControl, updateSendDue) { @@ -170,7 +190,7 @@ TEST(RateControl, updateSendDue) { last_send_due = rc.send_due_; ASSERT_NO_THROW(rc.updateSendDue()); // The value should be modified to the new value. - EXPECT_TRUE(rc.send_due_ != last_send_due); + EXPECT_TRUE(rc.send_due_ > last_send_due); } diff --git a/tests/tools/perfdhcp/tests/stats_mgr_unittest.cc b/tests/tools/perfdhcp/tests/stats_mgr_unittest.cc index 348ab42a31..6147bf727c 100644 --- a/tests/tools/perfdhcp/tests/stats_mgr_unittest.cc +++ b/tests/tools/perfdhcp/tests/stats_mgr_unittest.cc @@ -266,24 +266,16 @@ TEST_F(StatsMgrTest, MultipleExchanges) { TEST_F(StatsMgrTest, ExchangeToString) { // Test DHCPv4 specific exchange names. - StatsMgr4 stats_mgr4; - stats_mgr4.addExchangeStats(StatsMgr4::XCHG_DO); - stats_mgr4.addExchangeStats(StatsMgr4::XCHG_RA); EXPECT_EQ("DISCOVER-OFFER", - stats_mgr4.exchangeToString(StatsMgr4::XCHG_DO)); - EXPECT_EQ("REQUEST-ACK", stats_mgr4.exchangeToString(StatsMgr4::XCHG_RA)); + StatsMgr4::exchangeToString(StatsMgr4::XCHG_DO)); + EXPECT_EQ("REQUEST-ACK", StatsMgr4::exchangeToString(StatsMgr4::XCHG_RA)); // Test DHCPv6 specific exchange names. - StatsMgr6 stats_mgr6; - stats_mgr6.addExchangeStats(StatsMgr6::XCHG_SA); - stats_mgr6.addExchangeStats(StatsMgr6::XCHG_RR); - stats_mgr6.addExchangeStats(StatsMgr6::XCHG_RN); - stats_mgr6.addExchangeStats(StatsMgr6::XCHG_RL); EXPECT_EQ("SOLICIT-ADVERTISE", - stats_mgr6.exchangeToString(StatsMgr6::XCHG_SA)); - EXPECT_EQ("REQUEST-REPLY", stats_mgr6.exchangeToString(StatsMgr6::XCHG_RR)); - EXPECT_EQ("RENEW-REPLY", stats_mgr6.exchangeToString(StatsMgr6::XCHG_RN)); - EXPECT_EQ("RELEASE-REPLY", stats_mgr6.exchangeToString(StatsMgr6::XCHG_RL)); + StatsMgr6::exchangeToString(StatsMgr6::XCHG_SA)); + EXPECT_EQ("REQUEST-REPLY", StatsMgr6::exchangeToString(StatsMgr6::XCHG_RR)); + EXPECT_EQ("RENEW-REPLY", StatsMgr6::exchangeToString(StatsMgr6::XCHG_RN)); + EXPECT_EQ("RELEASE-REPLY", StatsMgr6::exchangeToString(StatsMgr6::XCHG_RL)); }