Browse Source

[3183] Addressed review comments.

Marcin Siodelski 11 years ago
parent
commit
276e655555

+ 32 - 26
tests/tools/perfdhcp/command_options.cc

@@ -12,19 +12,21 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
+#include "command_options.h"
+#include <exceptions/exceptions.h>
+#include <dhcp/iface_mgr.h>
+#include <dhcp/duid.h>
+
+#include <boost/lexical_cast.hpp>
+#include <boost/date_time/posix_time/posix_time.hpp>
+
 #include <config.h>
+#include <sstream>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdint.h>
 #include <unistd.h>
 
-#include <boost/lexical_cast.hpp>
-#include <boost/date_time/posix_time/posix_time.hpp>
-
-#include <exceptions/exceptions.h>
-#include <dhcp/iface_mgr.h>
-#include <dhcp/duid.h>
-#include "command_options.h"
 
 using namespace std;
 using namespace isc;
@@ -700,50 +702,52 @@ CommandOptions::validate() const {
           "second -d<drop-time> is not compatible with -i");
     check((getExchangeMode() == DO_SA) &&
           ((getMaxDrop().size() > 1) || (getMaxDropPercentage().size() > 1)),
-          "second -D<max-drop> is not compatible with -i\n");
+          "second -D<max-drop> is not compatible with -i");
     check((getExchangeMode() == DO_SA) && (isUseFirst()),
-          "-1 is not compatible with -i\n");
+          "-1 is not compatible with -i");
     check((getExchangeMode() == DO_SA) && (getTemplateFiles().size() > 1),
-          "second -T<template-file> is not compatible with -i\n");
+          "second -T<template-file> is not compatible with -i");
     check((getExchangeMode() == DO_SA) && (getTransactionIdOffset().size() > 1),
-          "second -X<xid-offset> is not compatible with -i\n");
+          "second -X<xid-offset> is not compatible with -i");
     check((getExchangeMode() == DO_SA) && (getRandomOffset().size() > 1),
-          "second -O<random-offset is not compatible with -i\n");
+          "second -O<random-offset is not compatible with -i");
     check((getExchangeMode() == DO_SA) && (getElapsedTimeOffset() >= 0),
-          "-E<time-offset> is not compatible with -i\n");
+          "-E<time-offset> is not compatible with -i");
     check((getExchangeMode() == DO_SA) && (getServerIdOffset() >= 0),
-          "-S<srvid-offset> is not compatible with -i\n");
+          "-S<srvid-offset> is not compatible with -i");
     check((getExchangeMode() == DO_SA) && (getRequestedIpOffset() >= 0),
-          "-I<ip-offset> is not compatible with -i\n");
+          "-I<ip-offset> is not compatible with -i");
+    check((getExchangeMode() == DO_SA) && (getRenewRate() != 0),
+          "-f<renew-rate> is not compatible with -i");
     check((getExchangeMode() != DO_SA) && (isRapidCommit() != 0),
-          "-i must be set to use -c\n");
+          "-i must be set to use -c");
     check((getRate() == 0) && (getReportDelay() != 0),
-          "-r<rate> must be set to use -t<report>\n");
+          "-r<rate> must be set to use -t<report>");
     check((getRate() == 0) && (getNumRequests().size() > 0),
-          "-r<rate> must be set to use -n<num-request>\n");
+          "-r<rate> must be set to use -n<num-request>");
     check((getRate() == 0) && (getPeriod() != 0),
-          "-r<rate> must be set to use -p<test-period>\n");
+          "-r<rate> must be set to use -p<test-period>");
     check((getRate() == 0) &&
           ((getMaxDrop().size() > 0) || getMaxDropPercentage().size() > 0),
-          "-r<rate> must be set to use -D<max-drop>\n");
+          "-r<rate> must be set to use -D<max-drop>");
     check((getRate() != 0) && (getRenewRate() > getRate()),
-          "Renew rate specified as -f<renew-rate> must not be freater than"
+          "Renew rate specified as -f<renew-rate> must not be greater than"
           " the 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");
     check((getTemplateFiles().size() < getTransactionIdOffset().size()),
-          "-T<template-file> must be set to use -X<xid-offset>\n");
+          "-T<template-file> must be set to use -X<xid-offset>");
     check((getTemplateFiles().size() < getRandomOffset().size()),
-          "-T<template-file> must be set to use -O<random-offset>\n");
+          "-T<template-file> must be set to use -O<random-offset>");
     check((getTemplateFiles().size() < 2) && (getElapsedTimeOffset() >= 0),
-          "second/request -T<template-file> must be set to use -E<time-offset>\n");
+          "second/request -T<template-file> must be set to use -E<time-offset>");
     check((getTemplateFiles().size() < 2) && (getServerIdOffset() >= 0),
           "second/request -T<template-file> must be set to "
-          "use -S<srvid-offset>\n");
+          "use -S<srvid-offset>");
     check((getTemplateFiles().size() < 2) && (getRequestedIpOffset() >= 0),
           "second/request -T<template-file> must be set to "
-          "use -I<ip-offset>\n");
+          "use -I<ip-offset>");
 
 }
 
@@ -751,6 +755,8 @@ void
 CommandOptions::check(bool condition, const std::string& errmsg) const {
     // The same could have been done with macro or just if statement but
     // we prefer functions to macros here
+    std::ostringstream stream;
+    stream << errmsg << "\n";
     if (condition) {
         isc_throw(isc::InvalidParameter, errmsg);
     }

+ 3 - 4
tests/tools/perfdhcp/test_control.h

@@ -746,9 +746,8 @@ protected:
 
     /// \brief Send a renew message using provided socket.
     ///
-    /// This function will try to identify an existing lease for which a Renew
-    /// will be sent. If there is no lease that can be renewed this method will
-    /// return false.
+    /// This method will select an existing lease from the Reply packet cache
+    /// If there is no lease that can be renewed this method will return false.
     ///
     /// \param socket An object encapsulating socket to be used to send
     /// a packet.
@@ -1061,7 +1060,7 @@ private:
     boost::posix_time::ptime renew_due_;   ///< Due time to send next set of
                                            ///< Renew requests.
     boost::posix_time::ptime last_renew_;  ///< Indicates when the last Renew
-                                           ///< was sent.
+                                           ///< was attempted.
 
     boost::posix_time::ptime last_report_; ///< Last intermediate report time.
 

+ 4 - 0
tests/tools/perfdhcp/tests/command_options_unittest.cc

@@ -359,6 +359,10 @@ TEST_F(CommandOptionsTest, RenewRate) {
     // Renew rate should be specified.
     EXPECT_THROW(process("perfdhcp -6 -r 10 -f -l ethx all"),
                  isc::InvalidParameter);
+
+    // -f and -i are mutually exclusive
+    EXPECT_THROW(process("perfdhcp -6 -r 10 -f 10 -l ethx -i all"),
+                 isc::InvalidParameter);
 }
 
 TEST_F(CommandOptionsTest, ReportDelay) {

+ 45 - 6
tests/tools/perfdhcp/tests/packet_storage_unittest.cc

@@ -66,9 +66,17 @@ TEST_F(PacketStorageTest, getNext) {
         EXPECT_EQ(STORAGE_SIZE - i - 1, storage_.size());
     }
     EXPECT_TRUE(storage_.empty());
+    // When storage is empty, the attempt to get the next packet should
+    // result in returning NULL pointer.
     EXPECT_FALSE(storage_.getNext());
+    // Let's try it again to see if the previous call to getNext didn't
+    // put the storage into the state in which the subsequent calls to
+    // getNext would result in incorrect behaviour.
     EXPECT_FALSE(storage_.getNext());
 
+    // Let's add a new packet to the empty storage to check that storage
+    // "recovers" from being empty, i.e. that the internal indicator
+    // which points to current packet reinitializes correctly.
     storage_.append(createPacket6(DHCPV6_REPLY, 100));
     ASSERT_EQ(1, storage_.size());
     Pkt6Ptr packet = storage_.getNext();
@@ -82,16 +90,32 @@ TEST_F(PacketStorageTest, getNext) {
 // empty() and size() functions.
 TEST_F(PacketStorageTest, getRandom) {
     ASSERT_EQ(STORAGE_SIZE, storage_.size());
+    int cnt_equals = 0;
     for (int i = 0; i < STORAGE_SIZE; ++i) {
         Pkt6Ptr packet = storage_.getRandom();
         ASSERT_TRUE(packet) << "NULL packet returned by storage_.getRandom()"
             " for iteration number " << i;
         EXPECT_EQ(STORAGE_SIZE - i - 1, storage_.size());
+        cnt_equals += (i == packet->getTransid() ? 1 : 0);
     }
+    // If the number of times id is equal to i, is the same as the number
+    // of elements then they were NOT accessed randomly.
+    // The odds of 20 elements being randomly  accessed sequential order
+    // is nil isn't it?
+    EXPECT_NE(cnt_equals, STORAGE_SIZE);
+
     EXPECT_TRUE(storage_.empty());
+    // When storage is empty, the attempt to get the random packet should
+    // result in returning NULL pointer.
     EXPECT_FALSE(storage_.getRandom());
+    // Let's try it again to see if the previous call to getRandom didn't
+    // put the storage into the state in which the subsequent calls to
+    // getRandom would result in incorrect behaviour.
     EXPECT_FALSE(storage_.getRandom());
 
+    // Let's add a new packet to the empty storage to check that storage
+    // "recovers" from being empty, i.e. that the internal indicator
+    // which points to the current packet reinitializes correctly.
     storage_.append(createPacket6(DHCPV6_REPLY, 100));
     ASSERT_EQ(1, storage_.size());
     Pkt6Ptr packet = storage_.getRandom();
@@ -109,8 +133,7 @@ TEST_F(PacketStorageTest, getNextAndRandom) {
         Pkt6Ptr packet_random = storage_.getRandom();
         ASSERT_TRUE(packet_random) << "NULL packet returned by"
             " storage_.getRandom() for iteration number " << i;
-        EXPECT_EQ(STORAGE_SIZE - 2 * i - 1, storage_.size());
-        uint32_t random_packet_transid = packet_random->getTransid();
+        EXPECT_EQ(STORAGE_SIZE - 2 *i - 1, storage_.size());
         Pkt6Ptr packet_seq = storage_.getNext();
         ASSERT_TRUE(packet_seq) << "NULL packet returned by"
             " storage_.getNext()  for iteration number " << i;
@@ -120,15 +143,25 @@ TEST_F(PacketStorageTest, getNextAndRandom) {
     EXPECT_FALSE(storage_.getRandom());
     EXPECT_FALSE(storage_.getNext());
 
+    // Append two packets to the storage to check if it can "recover"
+    // from being empty and that new elements can be accessed.
     storage_.append(createPacket6(DHCPV6_REPLY, 100));
     storage_.append(createPacket6(DHCPV6_REPLY, 101));
     ASSERT_EQ(2, storage_.size());
-    Pkt6Ptr packet_random = storage_.getRandom();
-    ASSERT_TRUE(packet_random);
-    EXPECT_EQ(100, packet_random->getTransid());
+    // The newly added elements haven't been accessed yet. So, if we
+    // call getNext the first one should be returned.
     Pkt6Ptr packet_next = storage_.getNext();
     ASSERT_TRUE(packet_next);
-    EXPECT_EQ(101, packet_next->getTransid());
+    // The first packet has transaction id equal to 100.
+    EXPECT_EQ(100, packet_next->getTransid());
+    // There should be just one packet left in the storage.
+    ASSERT_EQ(1, storage_.size());
+    // The call to getRandom should return the sole packet from the
+    // storage.
+    Pkt6Ptr packet_random = storage_.getRandom();
+    ASSERT_TRUE(packet_random);
+    EXPECT_EQ(101, packet_random->getTransid());
+    // Any further calls to getRandom and getNext should return NULL.
     EXPECT_FALSE(storage_.getRandom());
     EXPECT_FALSE(storage_.getNext());
 }
@@ -153,6 +186,12 @@ TEST_F(PacketStorageTest, clear) {
     // We should have 10 remaining.
     ASSERT_EQ(10, storage_.size());
 
+    // Check that the retrieval still works after partial clear.
+    EXPECT_TRUE(storage_.getNext());
+    EXPECT_TRUE(storage_.getRandom());
+    // We should have 10 - 2 = 8 packets in the storage after retrieval.
+    ASSERT_EQ(8, storage_.size());
+
     // Try to remove more elements that actually is. It
     // should result in removal of all elements.
     ASSERT_NO_THROW(storage_.clear(15));

+ 11 - 0
tests/tools/perfdhcp/tests/test_control_unittest.cc

@@ -1340,28 +1340,39 @@ TEST_F(TestControlTest, createRenew) {
         generator(new NakedTestControl::IncrementalGenerator());
     tc.setTransidGenerator(generator);
 
+    // Create a Reply packet. The createRenew function will need Reply
+    // packet to create a corresponding Renew.
     Pkt6Ptr reply = createReplyPkt6(1);
     Pkt6Ptr renew;
+    // Check that Renew is created.
     ASSERT_NO_THROW(renew = tc.createRenew(reply));
     ASSERT_TRUE(renew);
     EXPECT_EQ(DHCPV6_RENEW, renew->getType());
     EXPECT_EQ(1, renew->getTransid());
 
+    // Now check that the Renew packet created, has expected options. The
+    // payload of these options should be the same as the payload of the
+    // options in the Reply.
+
+    // Client Identifier
     OptionPtr opt_clientid = renew->getOption(D6O_CLIENTID);
     ASSERT_TRUE(opt_clientid);
     EXPECT_TRUE(reply->getOption(D6O_CLIENTID)->getData() ==
                 opt_clientid->getData());
 
+    // Server identifier
     OptionPtr opt_serverid = renew->getOption(D6O_SERVERID);
     ASSERT_TRUE(opt_serverid);
     EXPECT_TRUE(reply->getOption(D6O_SERVERID)->getData() ==
                 opt_serverid->getData());
 
+    // IA_NA
     OptionPtr opt_ia_na = renew->getOption(D6O_IA_NA);
     ASSERT_TRUE(opt_ia_na);
     EXPECT_TRUE(reply->getOption(D6O_IA_NA)->getData() ==
                 opt_ia_na->getData());
 
+    // IA_PD
     OptionPtr opt_ia_pd = renew->getOption(D6O_IA_PD);
     ASSERT_TRUE(opt_ia_pd);
     EXPECT_TRUE(reply->getOption(D6O_IA_PD)->getData() ==