BIND 10 trac3183, updated. 276e655555df9302ba56ea5520a5713d1915bd7f [3183] Addressed review comments.
BIND 10 source code commits
bind10-changes at lists.isc.org
Fri Oct 25 09:26:56 UTC 2013
The branch, trac3183 has been updated
via 276e655555df9302ba56ea5520a5713d1915bd7f (commit)
from 2838c0769e219dbd2c8a479efe87f9bfdb4da8e9 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 276e655555df9302ba56ea5520a5713d1915bd7f
Author: Marcin Siodelski <marcin at isc.org>
Date: Fri Oct 25 11:26:46 2013 +0200
[3183] Addressed review comments.
-----------------------------------------------------------------------
Summary of changes:
tests/tools/perfdhcp/command_options.cc | 58 +++++++++++---------
tests/tools/perfdhcp/test_control.h | 7 +--
.../perfdhcp/tests/command_options_unittest.cc | 4 ++
.../perfdhcp/tests/packet_storage_unittest.cc | 51 +++++++++++++++--
.../tools/perfdhcp/tests/test_control_unittest.cc | 11 ++++
5 files changed, 95 insertions(+), 36 deletions(-)
-----------------------------------------------------------------------
diff --git a/tests/tools/perfdhcp/command_options.cc b/tests/tools/perfdhcp/command_options.cc
index 45c73e7..7edb84b 100644
--- a/tests/tools/perfdhcp/command_options.cc
+++ b/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);
}
diff --git a/tests/tools/perfdhcp/test_control.h b/tests/tools/perfdhcp/test_control.h
index b15cc32..95df395 100644
--- a/tests/tools/perfdhcp/test_control.h
+++ b/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.
diff --git a/tests/tools/perfdhcp/tests/command_options_unittest.cc b/tests/tools/perfdhcp/tests/command_options_unittest.cc
index d37c27d..3431b87 100644
--- a/tests/tools/perfdhcp/tests/command_options_unittest.cc
+++ b/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) {
diff --git a/tests/tools/perfdhcp/tests/packet_storage_unittest.cc b/tests/tools/perfdhcp/tests/packet_storage_unittest.cc
index 6a8a316..15ba012 100644
--- a/tests/tools/perfdhcp/tests/packet_storage_unittest.cc
+++ b/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));
diff --git a/tests/tools/perfdhcp/tests/test_control_unittest.cc b/tests/tools/perfdhcp/tests/test_control_unittest.cc
index 114ec68..0d45c88 100644
--- a/tests/tools/perfdhcp/tests/test_control_unittest.cc
+++ b/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() ==
More information about the bind10-changes
mailing list