[3181] Addressed review comments.

tests/tools/perfdhcp/command_options.cc

@@ -743,8 +743,8 @@ CommandOptions::validate() const {
           "-r<rate> must be set to use -D<max-drop>");
     check((getRate() != 0) && (getRenewRate() + getReleaseRate() > getRate()),
           "The sum of Renew rate (-f<renew-rate>) and Release rate"
-          " (-F<release-rate>) must not be greater than the rate specified"
-          " as -r<rate>");
+          " (-F<release-rate>) must not be greater than the exchange"
+          " rate specified as -r<rate>");
     check((getRate() == 0) && (getRenewRate() != 0),
           "Renew rate specified as -f<renew-rate> must not be specified"
           " when -r<rate> 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<renew-rate>: 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<rate>. If -r<rate> is\n"
-        "    not specified, this parameter must not be specified too.\n"
-        "-F<release-rate>: 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<rate>. If -r<rate> is not\n"
-        "    specified, this parameter must not be specified too.\n"
+        "-f<renew-rate>: 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<rate>).  Furthermore the sum of\n"
+        "    this value and the release-rate (given by -F<rate) must be equal\n"
+        "    to or less than the exchange rate.\n"
+        "-F<release-rate>: 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<rate>).  Furthermore the sum of\n"
+        "    this value and the renew-rate (given by -f<rate) must be equal\n"
+        "    to or less than the exchange rate.\n"
         "\n"
         "The remaining options are used only in conjunction with -r:\n"
         "\n"

tests/tools/perfdhcp/rate_control.cc

@@ -28,10 +28,14 @@ RateControl::RateControl()
 RateControl::RateControl(const int rate, const int aggressivity)
     : send_due_(currentTime()), last_sent_(currentTime()),
       aggressivity_(aggressivity), rate_(rate), late_sent_(false) {
-    if (aggressivity < 1) {
+    if (aggressivity_ < 1) {
         isc_throw(isc::BadValue, "invalid value of aggressivity "
                   << aggressivity << ", expected value is greater than 0");
     }
+    if (rate_ < 0) {
+        isc_throw(isc::BadValue, "invalid value of rate " << rate
+                  << ", expected non-negative value");
+    }
 }
 
 uint64_t
@@ -120,6 +124,24 @@ RateControl::updateSendDue() {
     }
 }
 
+void
+RateControl::setAggressivity(const int aggressivity) {
+    if (aggressivity < 1) {
+        isc_throw(isc::BadValue, "invalid value of aggressivity "
+                  << aggressivity << ", expected value is greater than 0");
+    }
+    aggressivity_ = aggressivity;
+}
+
+void
+RateControl::setRate(const int rate) {
+    if (rate < 0) {
+        isc_throw(isc::BadValue, "invalid value of rate " << rate
+                  << ", expected non-negative value");
+    }
+    rate_ = rate;
+}
+
 void
 RateControl::setRelativeDue(const int offset) {
     send_due_ = offset > 0 ?

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 <b>duration</b> 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.
     ///

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");

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);
         }
     }

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<rate> is not specified the -f<renew-rate> 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<rate> is not specified the -F<release-rate> should not
     // be accepted.
     EXPECT_THROW(process("perfdhcp -6 -F 10 -l ethx all"),

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);
 
 }
 

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));
 
 }