[1954] Updated with suggestions from second round of review

tests/tools/perfdhcp/command_options.cc

@@ -39,11 +39,11 @@ CommandOptions::instance() {
 
 void
 CommandOptions::reset() {
-    // default mac address used in DHCP messages
+    // Default mac address used in DHCP messages
     // if -b mac=<mac-address> was not specified
     uint8_t mac[6] = { 0x0, 0xC, 0x1, 0x2, 0x3, 0x4 };
 
-    // default packet drop time if -D<drop-time> parameter
+    // Default packet drop time if -D<drop-time> parameter
     // was not specified
     double dt[2] = { 1., 1. };
 
@@ -53,15 +53,14 @@ CommandOptions::reset() {
     exchange_mode_ = DORA_SARR;
     rate_ = 0;
     report_delay_ = 0;
-    random_range_ = 0;
-    max_random_ = 0;
+    clients_num_ = 0;
+    clients_num_ = 0;
     mac_prefix_.assign(mac, mac + 6);
     base_.resize(0);
     num_request_.resize(0);
     period_ = 0;
     drop_time_set_ = 0;
     drop_time_.assign(dt, dt + 2);
-    max_drop_set_ = 0;
     max_drop_.clear();
     max_pdrop_.clear();
     localname_.clear();
@@ -92,33 +91,37 @@ CommandOptions::parse(int argc, char** const argv) {
     // parsing different command lines multiple times
     optind = 1;
     opterr = 0;
+
+    // Reset values of class members
+    reset();
+
     initialize(argc, argv);
     validate();
 }
 
 void
 CommandOptions::initialize(int argc, char** argv) {
-    char opt = 0;               // subsequent options returned by getopt()
-    std::string dropArg;        // value of -D<value>argument
-    size_t percentLoc = 0;      // location of % sign in -D<value>
-    double dropPercent = 0;     // % value (1..100) in -D<%value>
-    int numDrops = 0;           // max number of drops specified in -D<value>
-    int numReq = 0;             // max number of dropped requests in -n<max-drops>
-    int offsetArg = 0;          // temp variable holiding offset arguments
-    std::string sarg;           // temp variable for string args
-
-    // in this section we collect argument values from command line
+    char opt = 0;               // Subsequent options returned by getopt()
+    std::string drop_arg;       // Value of -D<value>argument
+    size_t percent_loc = 0;     // Location of % sign in -D<value>
+    double drop_percent = 0;    // % value (1..100) in -D<%value>
+    int num_drops = 0;          // Max number of drops specified in -D<value>
+    int num_req = 0;            // Max number of dropped requests in -n<max-drops>
+    int offset_arg = 0;         // Temporary variable holding offset arguments
+    std::string sarg;           // Temporary variable for string args
+
+    // In this section we collect argument values from command line
     // they will be tuned and validated elsewhere
     while((opt = getopt(argc, argv, "hv46r:t:R:b:n:p:d:D:l:P:a:L:s:iBc1T:X:O:E:S:I:x:w:")) != -1) {
         switch (opt) {
-        case 'h':
-            usage();
-            return;
-
         case 'v':
             version();
             return;
 
+        case '1':
+            use_first_ = true;
+            break;
+
         case '4':
             check(ipversion_ == 6, "IP version already set to 6");
             ipversion_ = 4;
@@ -129,38 +132,27 @@ CommandOptions::initialize(int argc, char** argv) {
             ipversion_ = 6;
             break;
 
-        case 'r':
-            rate_ = positiveInteger("rate_ must be a positive integer");
-            break;
-
-        case 't':
-            report_delay_ = positiveInteger("report_delay_ must be a positive integer");
-            break;
-
-        case 'R':
-            initRandomRange(optarg);
+        case 'a':
+            aggressivity_ = positiveInteger("value of aggressivity: -a<value> must be a positive integer");
             break;
 
         case 'b':
-            check(base_.size() > 3, "too many bases");
+            check(base_.size() > 3, "-b<value> already specified, unexpected occurence of 5th -b<value>");
             base_.push_back(optarg);
             decodeBase(base_.back());
             break;
 
-        case 'n':
-            numReq = positiveInteger("num-request must be a positive integer");
-            if (num_request_.size() >= 2) {
-                isc_throw(isc::InvalidParameter,
-                          "too many num-request values");
-            }
-            num_request_.push_back(numReq);
+        case 'B':
+            broadcast_ = true;
             break;
 
-        case 'p':
-            period_ = positiveInteger("test-period must be a positive integer");
+        case 'c':
+            rapid_commit_ = true;
             break;
 
         case 'd':
+            check(drop_time_set_ > 1, "maximum number of drops already specified, "
+                  "unexpected 3rd occurence of -d<value>");
             try {
                 drop_time_[drop_time_set_] = boost::lexical_cast<double>(optarg);
             } catch (boost::bad_lexical_cast&) {
@@ -168,133 +160,152 @@ CommandOptions::initialize(int argc, char** argv) {
                           "value of drop time: -d<value> must be positive number");
             }
             check(drop_time_[drop_time_set_] <= 0., "drop-time must be a positive number");
-            drop_time_set_ = 1;
+            drop_time_set_ = true;
             break;
 
         case 'D':
-            dropArg = std::string(optarg);
-            percentLoc = dropArg.find('%');
-            if ((percentLoc) != std::string::npos) {
-                dropPercent = boost::lexical_cast<double>(dropArg.substr(percentLoc + 1));
-                check((dropPercent <= 0) || (dropPercent >= 100),
+            drop_arg = std::string(optarg);
+            percent_loc = drop_arg.find('%');
+            check(max_pdrop_.size() > 1 || max_drop_.size() > 1, "values of maximum drops: -D<value> already "
+                  "specified, unexpected 3rd occurence of -D,value>");
+            if ((percent_loc) != std::string::npos) {
+                try {
+                    drop_percent = boost::lexical_cast<double>(drop_arg.substr(0, percent_loc));
+                } catch (boost::bad_lexical_cast&) {
+                    isc_throw(isc::InvalidParameter,
+                              "value of drop percentage: -D<%value> must be 0..100");
+                }
+                check((drop_percent <= 0) || (drop_percent >= 100),
                   "value of drop percentage: -D<%value> must be 0..100");
-                max_pdrop_.push_back(dropPercent);
-                break;
+                max_pdrop_.push_back(drop_percent);
+            } else {
+                num_drops = positiveInteger("value of max drops number: -d<value> must be a positive integer");
+                max_drop_.push_back(num_drops);
             }
-            numDrops = positiveInteger("value of max drops number: -d<value> must be a positive integer");
-            max_drop_.push_back(numDrops);
             break;
 
-        case 'l':
-            localname_ = std::string(optarg);
+        case 'E':
+            elp_offset_ = nonNegativeInteger("value of time-offset: -E<value> must not be a negative integer");
             break;
 
-        case 'P':
-            preload_ = nonNegativeInteger("number of preload packets: -P<value> must not be "
-                                          "a negative integer");
+        case 'h':
+            usage();
+            return;
+
+        case 'i':
+            exchange_mode_ = DO_SA;
             break;
 
-        case 'a':
-            aggressivity_ = positiveInteger("value of aggressivity: -a<value> must be a positive integer");
+        case 'I':
+            rip_offset_ = positiveInteger("value of ip address offset: -I<value> must be a positive integer");
+            break;
+
+        case 'l':
+            localname_ = std::string(optarg);
             break;
 
         case 'L':
              local_port_ = nonNegativeInteger("value of local port: -L<value> must not be a negative integer");
-             check(local_port_ > static_cast<int>( std::numeric_limits<uint16_t>::max()),
+             check(local_port_ > static_cast<int>(std::numeric_limits<uint16_t>::max()),
                   "local-port must be lower than " +
                   boost::lexical_cast<std::string>(std::numeric_limits<uint16_t>::max()));
             break;
 
-        case 's':
-            seed_ = static_cast<unsigned int>
-                (nonNegativeInteger("value of -s <seed> must be non-negative integer"));
-            seeded_ = seed_ > 0 ? true : false;
+        case 'n':
+            num_req = positiveInteger("value of num-request: -n<value> must be a positive integer");
+            if (num_request_.size() >= 2) {
+                isc_throw(isc::InvalidParameter,"value of maximum number of requests: -n<value> "
+                          "already specified, unexpected 3rd occurence of -n<value>");
+            }
+            num_request_.push_back(num_req);
             break;
 
-        case 'i':
-            exchange_mode_ = DO_SA;
+        case 'O':
+            if (rnd_offset_.size() < 2) {
+                offset_arg = positiveInteger("value of random offset: -O<value> must be greater than 3");
+            } else {
+                isc_throw(isc::InvalidParameter,
+                          "random offsets already specified, unexpected 3rd occurence of -O<value>");
+            }
+            check(offset_arg < 3, "value of random random-offset: -O<value> must be greater than 3 ");
+            rnd_offset_.push_back(offset_arg);
             break;
 
-        case 'B':
-            broadcast_ = true;
+        case 'p':
+            period_ = positiveInteger("value of test period: -p<value> must be a positive integer");
             break;
 
-        case 'c':
-            rapid_commit_ = true;
+        case 'P':
+            preload_ = nonNegativeInteger("number of preload packets: -P<value> must not be "
+                                          "a negative integer");
             break;
 
-        case '1':
-            use_first_ = true;
+        case 'r':
+            rate_ = positiveInteger("value of rate: -r<value> must be a positive integer");
             break;
 
-        case 'T':
-            switch (template_file_.size()) {
-            case 0:
-            case 1:
-                sarg = nonEmptyString("template file name not specified, expected -T<filename>");
-                template_file_.push_back(sarg);
-                break;
-            default:
-                isc_throw(isc::InvalidParameter,
-                          "template-files are already set");
-            }
+        case 'R':
+            initClientsNum();
             break;
 
-        case 'X':
-            offsetArg = positiveInteger("value of transaction id: -X<value> must be a positive integer");
-            if (xid_offset_.size() >= 2) {
-                xid_offset_.clear();
-            }
-            xid_offset_.push_back(offsetArg);
+        case 's':
+            seed_ = static_cast<unsigned int>
+                (nonNegativeInteger("value of seed: -s <seed> must be non-negative integer"));
+            seeded_ = seed_ > 0 ? true : false;
             break;
 
-        case 'O':
-            try {
-                offsetArg = boost::lexical_cast<int>(optarg);
-            } catch (boost::bad_lexical_cast&) {
-                isc_throw(isc::InvalidParameter, "value of random random-offset: -O<value> "
-                          "must be greater than 3");
-            }
-            check(offsetArg < 3, "value of random random-offset: -O<value> must be greater than 3 ");
-            if (rnd_offset_.size() >= 2) {
-                rnd_offset_.clear();
-            }
-            rnd_offset_.push_back(offsetArg);
+        case 'S':
+            sid_offset_ = positiveInteger("value of server id offset: -S<value> must be a positive integer");
             break;
 
-        case 'E':
-            elp_offset_ = nonNegativeInteger("value of time-offset: -E<value> must not be a negative integer");
+        case 't':
+            report_delay_ = positiveInteger("value of report delay: -t<value> must be a positive integer");
             break;
 
-        case 'S':
-            sid_offset_ = positiveInteger("value of server id offset: -S<value> must be a positive integer");
+        case 'T':
+            if (template_file_.size() < 2) {
+                sarg = nonEmptyString("template file name not specified, expected -T<filename>");
+                template_file_.push_back(sarg);
+            } else {
+                isc_throw(isc::InvalidParameter,
+                          "template files are already specified, unexpected 3rd -T<filename> occurence");
+            }
             break;
 
-        case 'I':
-            rip_offset_ = positiveInteger("value of ip addr offset: -I<value> must be a positive integer");
+        case 'w':
+            wrapped_ = nonEmptyString("command for wrapped mode: -w<command> must be specified");
             break;
 
         case 'x':
             diags_ = nonEmptyString("value of diagnostics selectors: -x<value> must be specified");
             break;
 
-        case 'w':
-            wrapped_ = nonEmptyString("command for wrapped mode: -w<command> must be specified");
+        case 'X':
+            if (xid_offset_.size() < 2) {
+                offset_arg = positiveInteger("value of transaction id: -X<value> must be a positive integer");
+            } else {
+                isc_throw(isc::InvalidParameter,
+                          "transaction ids already specified, unexpected 3rd -X<value> occurence");
+            }
+            xid_offset_.push_back(offset_arg);
             break;
 
         default:
-            isc_throw(isc::InvalidParameter,
-                      "unknown command line option");
+            isc_throw(isc::InvalidParameter, "unknown command line option");
         }
     }
 
-    // Tune ip version as it still might be zero
-    // if not specified in command line was used
+    // If the IP version was not specified in the
+    // command line, assume IPv4.
     if (ipversion_ == 0) {
         ipversion_ = 4;
     }
 
-    // Tune up template file lists
+    // If template packet files specified for both DISCOVER/SOLICIT
+    // and REQUEST/REPLY exchanges make sure we have transaction id
+    // and random duid offsets for both exchanges. We will duplicate
+    // value specified as -X<value> and -R<value> for second
+    // exchange if user did not specified otherwise.
     if (template_file_.size() > 1) {
         if (xid_offset_.size() == 1) {
             xid_offset_.push_back(xid_offset_[0]);
@@ -305,12 +316,16 @@ CommandOptions::initialize(int argc, char** argv) {
     }
 
     // Get server argument
+    // NoteFF02::1:2 and FF02::1:3 are defined in RFC3315 as
+    // All_DHCP_Relay_Agents_and_Servers and All_DHCP_Servers
+    // addresses
     check(optind < argc -1, "extra arguments?");
     if (optind == argc - 1) {
         server_name_ = argv[optind];
         // Decode special cases
         if ((ipversion_ == 4) && (server_name_.compare("all") == 0)) {
             broadcast_ = 1;
+            // 255.255.255.255 is IPv4 broadcast address
             server_name_ = "255.255.255.255";
         } else if ((ipversion_ == 6) && (server_name_.compare("all") == 0)) {
             server_name_ = "FF02::1:2";
@@ -323,38 +338,25 @@ CommandOptions::initialize(int argc, char** argv) {
 }
 
 void
-CommandOptions::initRandomRange(const std::string& opt) {
+CommandOptions::initClientsNum() {
     const std::string errmsg = "value of -R <value> must be non-negative integer";
-    long long randomRange = 0;
+
+    // Declare clients_num as as 64-bit signed value to
+    // be able to detect negative values provided
+    // by user. We would not detect negative values
+    // if we casted directly to unsigned value.
+    long long clients_num = 0;
     try {
-        randomRange = boost::lexical_cast<long long>(opt);
+        clients_num = boost::lexical_cast<long long>(optarg);
     } catch (boost::bad_lexical_cast&) {
         isc_throw(isc::InvalidParameter, errmsg.c_str());
     }
-    check(randomRange < 0, errmsg.c_str());
+    check(clients_num < 0, errmsg);
     try {
-        random_range_ = boost::lexical_cast<uint32_t>(randomRange);
+        clients_num_ = boost::lexical_cast<uint32_t>(optarg);
     } catch (boost::bad_lexical_cast&) {
         isc_throw(isc::InvalidParameter, errmsg);
     }
-
-    if ((random_range_ != 0) && (random_range_ != std::numeric_limits<uint32_t>::max())) {
-        // randomization range is restricted by number of clients
-        uint32_t maxTargetRand    = random_range_ + 1;
-        // randomization with random() function is within uint32_t range
-        uint64_t maxRandom        = std::numeric_limits<uint32_t>::max() + 1;
-        // restrict max randomized value to
-        // multiple of number of clients
-        uint64_t restrictedRandom = (maxRandom / maxTargetRand) * maxTargetRand;
-        if (restrictedRandom == maxRandom) {
-            // no need to restrict max random value
-            // uint32_t range is already multiple
-            // of number of clients range
-            max_random_ = 0;
-        } else {
-            max_random_ = static_cast<uint32_t> (restrictedRandom);
-        }
-    }
 }
 
 void
@@ -384,20 +386,24 @@ CommandOptions::decodeMac(const std::string& base) {
     std::istringstream s1(base.substr(found + 1));
     std::string token;
     mac_prefix_.clear();
+    // Get pieces of MAC address separated with : (or even ::)
     while (std::getline(s1, token, ':')) {
         unsigned int ui = 0;
-        std::istringstream s2(token);
+        // Convert token to byte value using std::istringstream
         if (token.length() > 0) {
             try {
+                // Do actual conversion
                 ui = convertHexString(token);
             } catch (isc::InvalidParameter&) {
                 isc_throw(isc::InvalidParameter,
                           "invalid characters in MAC provided");
 
             }
+            // If conversion succeeded store byte value
             mac_prefix_.push_back(ui);
         }
     }
+    // MAC address must consist of 6 octets, otherwise it is invalid
     check(mac_prefix_.size() != 6, errmsg);
 }
 
@@ -408,15 +414,16 @@ CommandOptions::decodeDuid(const std::string& base) {
     check(found == std::string::npos, "expected -b<base> format for duid is -b duid=<duid>");
     std::string b = base.substr(found + 1);
 
-    // duid must have even number of digits and must not be longer than 64 bytes
+    // DUID must have even number of digits and must not be longer than 64 bytes
     check(b.length() & 1, "odd number of hexadecimal digits in duid");
     check(b.length() > 128, "duid too large");
     check(b.length() == 0, "no duid specified");
 
-    // Turn pairs of hex digits into vector of bytes
+    // Turn pairs of hexadecimal digits into vector of octets
     for (int i = 0; i < b.length(); i += 2) {
         unsigned int ui = 0;
         try {
+            // Do actual conversion
             ui = convertHexString(b.substr(i, 2));
         } catch (isc::InvalidParameter&) {
             isc_throw(isc::InvalidParameter,
@@ -427,17 +434,20 @@ CommandOptions::decodeDuid(const std::string& base) {
 }
 
 uint8_t
-CommandOptions::convertHexString(const std::string& s) {
+CommandOptions::convertHexString(const std::string& text) const {
     unsigned int ui = 0;
-    for (int i = 0; i < s.length(); ++i) {
-        if (!std::isxdigit(s[i])) {
+    // First, check if we are dealing with hexadecimal digits only
+    for (int i = 0; i < text.length(); ++i) {
+        if (!std::isxdigit(text[i])) {
             isc_throw(isc::InvalidParameter,
-                      "The following digit: " << s[i] << " in "
-                      << s << "is not hexadecimal");
+                      "The following digit: " << text[i] << " in "
+                      << text << "is not hexadecimal");
         }
     }
-    std::istringstream ss(s);
-    ss >> std::hex >> ui >> std::dec;
+    // If we are here, we have valid string to convert to octet
+    std::istringstream text_stream(text);
+    text_stream >> std::hex >> ui >> std::dec;
+    // Check if for some reason we have overflow - this should never happen!
     if (ui > 0xFF) {
         isc_throw(isc::InvalidParameter, "Can't convert more than two hex digits to byte");
     }
@@ -476,7 +486,7 @@ CommandOptions::validate() const {
 	check((getRate() == 0) && (getReportDelay() != 0),
           "-r<rate> must be set to use -t<report>\n");
 	check((getRate() == 0) && (getNumRequests().size() > 0),
-          "-r<getRate()> must be set to use -n<num-request>\n");
+          "-r<rate> must be set to use -n<num-request>\n");
 	check((getRate() == 0) && (getPeriod() != 0),
           "-r<rate> must be set to use -p<test-period>\n");
 	check((getRate() == 0) &&
@@ -507,7 +517,7 @@ CommandOptions::check(bool condition, const std::string& errmsg) const {
 }
 
 int
-CommandOptions::positiveInteger(const std::string& errmsg) {
+CommandOptions::positiveInteger(const std::string& errmsg) const {
     try {
         int value = boost::lexical_cast<int>(optarg);
         check(value <= 0, errmsg);
@@ -518,7 +528,7 @@ CommandOptions::positiveInteger(const std::string& errmsg) {
 }
 
 int
-CommandOptions::nonNegativeInteger(const std::string& errmsg) {
+CommandOptions::nonNegativeInteger(const std::string& errmsg) const {
     try {
         int value = boost::lexical_cast<int>(optarg);
         check(value < 0, errmsg);
@@ -529,7 +539,7 @@ CommandOptions::nonNegativeInteger(const std::string& errmsg) {
 }
 
 std::string
-CommandOptions::nonEmptyString(const std::string& errmsg) {
+CommandOptions::nonEmptyString(const std::string& errmsg) const {
     std::string sarg = optarg;
     if (sarg.length() == 0) {
         isc_throw(isc::InvalidParameter, errmsg);

tests/tools/perfdhcp/command_options.h

@@ -44,17 +44,19 @@ public:
 
     /// \brief Reset to defaults
     ///
-    /// Resets the CommandOptions object to default values.
+    /// Reset data members to default values. This is specifically
+    /// useful when unit tests are performed using different
+    /// command line options.
     void reset();
 
     /// \brief Parse command line
     ///
-    /// Parses the command line and stores the selected options.
+    /// Parses the command line and stores the selected options
+    /// in class data members.
     ///
     /// \param argc Argument count passed to main().
     /// \param argv Argument value array passed to main().
-    /// \param force_reset Force  reset of state variables
-    /// \throws isc::InvalidParameter if parsing fails
+    /// \throws isc::InvalidParameter if parse fails
     void parse(int argc, char** const argv);
 
     /// \brief Returns IP version
@@ -72,64 +74,76 @@ public:
     /// \return exchange rate per second
     int getRate() const { return rate_; }
 
-    /// \brief Returns report delay
+    /// \brief Returns delay between two performance reports
     ///
-    /// \return delay between two consecutive reports
+    /// \return delay between two consecutive performance reports
     int getReportDelay() const { return report_delay_; }
 
-    /// \brief Returns randomization range
+    /// \brief Returns number of simulated clients
     ///
-    /// \return randomization range
-    uint32_t getRandomRange() const { return random_range_; }
+    /// \return number of simulated clients
+    uint32_t getClientsNum() const { return clients_num_; }
 
-    /// \brief Returns MAC addr prefix
+    /// \brief Returns MAC address prefix
     ///
-    /// \ return MAC addr prefix to generate different clients
+    /// \ return MAC address prefix to simulate different clients
     std::vector<uint8_t> getMacPrefix() const { return mac_prefix_; }
 
     /// \brief Returns DUID prefix
     ///
-    /// \return DUID prefix to generate different clients
+    /// \return DUID prefix to simulate different clients
     std::vector<uint8_t> getDuidPrefix() const { return duid_prefix_; }
 
     /// \brief Returns base values
     ///
-    /// \return base values for xid generation
+    /// \return all base values specified
     std::vector<std::string> getBase() const { return base_; }
 
-    /// \brief Returns number of exchanges
+    /// \brief Returns maximum number of exchanges
     ///
-    /// \return number of exchange requests
+    /// \return number of exchange requests before test is aborted
     std::vector<int> getNumRequests() const { return num_request_; }
 
     /// \brief Returns test period
     ///
-    /// \return test period
+    /// \return test period before it is aborted
     int getPeriod() const { return period_; }
 
-    /// \brief Returns lost time
+    /// \brief Returns drop time
     ///
-    /// \return return time before request is lost
+    /// The method returns maximum time elapsed from
+    /// sending the packet before it is assumed dropped.
+    ///
+    /// \return return time before request is assumed dropped
     std::vector<double> getDropTime() const { return drop_time_; }
 
-    /// \brief Returns max drops number
+    /// \brief Returns maximum drops number
+    ///
+    /// Returns maximum number of packet drops before
+    /// aborting a test.
     ///
-    /// \return maximum number of lost requests
+    /// \return maximum number of dropped requests
     std::vector<int> getMaxDrop() const { return max_drop_; }
 
-    /// \brief Returns max percentage of drops
+    /// \brief Returns maximal percentage of drops
+    ///
+    /// Returns maximal percentage of packet drops
+    /// before aborting a test.
     ///
     /// \return maximum percentage of lost requests
     std::vector<double> getMaxDropPercentage() const { return max_pdrop_; }
 
-    /// \brief Returns local address or interface
+    /// \brief Returns local address or interface name
     ///
-    /// \return local address or interface
+    /// \return local address or interface name
     std::string getLocalName() const { return localname_; }
 
-    /// \brief Checks if interface name is used
+    /// \brief Checks if interface name was used
     ///
-    /// \return true if interface name was specified
+    /// The method checks if interface name was used
+    /// rather than address.
+    ///
+    /// \return true if interface name was used
     bool isInterface() const { return is_interface_; }
 
     /// \brief Returns number of preload exchanges
@@ -157,17 +171,17 @@ public:
     /// \return random seed
     uint32_t getSeed() const { return seed_; }
 
-    /// \brief Checks if broadcast
+    /// \brief Checks if broadcast address is to be used
     ///
-    /// \return true if broadcast handling
+    /// \return true if broadcast address is to be used
     bool isBroadcast() const { return broadcast_; }
 
-    /// \brief Check if rapid commit used
+    /// \brief Check if rapid commit option used
     ///
-    /// \return true if rapid commit is used
+    /// \return true if rapid commit option is used
     bool isRapidCommit() const { return rapid_commit_; }
 
-    /// \brief Check if server-ID taken from first package
+    /// \brief Check if server-ID to be taken from first package
     ///
     /// \return true if server-iD to be taken from first package
     bool isUseFirst() const { return use_first_; }
@@ -204,12 +218,12 @@ public:
 
     /// \brief Returns diagnostic selectors
     ///
-    /// \return diagnostic selector
+    /// \return diagnostics selector
     std::string getDiags() const { return diags_; }
 
     /// \brief Returns wrapped command
     ///
-    /// \return wrapped command
+    /// \return wrapped command (start/stop)
     std::string getWrapped() const { return wrapped_; }
 
     /// \brief Returns server name
@@ -231,13 +245,12 @@ private:
 
     /// \brief Default Constructor
     ///
-    /// Protected constructor as this is a singleton class.
+    /// Private constructor as this is a singleton class.
     /// Use CommandOptions::instance() to get instance of it.
     CommandOptions() {
         reset();
     }
 
-private:
     /// \brief Initializes class members based command line
     ///
     /// Reads each command line parameter and sets class member values
@@ -254,7 +267,7 @@ private:
 
     /// \brief Throws !InvalidParameter exception if condition is true
     ///
-    /// Convenience function that throws an !InvalidParameter exception if
+    /// Convenience function that throws an InvalidParameter exception if
     /// the condition argument is true
     ///
     /// \param condition Condition to be checked
@@ -262,38 +275,41 @@ private:
     /// \throws isc::InvalidParameter if condition argument true
     inline void check(bool condition, const std::string& errmsg) const;
 
-    /// \brief Casts command line arg to positive int
+    /// \brief Casts command line argument to positive integer
     ///
     /// \param errmsg Error message if lexical cast fails
     /// \throw InvalidParameter if lexical cast fails
-    int positiveInteger(const std::string& errmsg);
+    int positiveInteger(const std::string& errmsg) const;
 
-    /// \brief Casts command line arg to non-negative int
+    /// \brief Casts command line argument to non-negative integer
     ///
     /// \param errmsg Error message if lexical cast fails
     /// \throw InvalidParameter if lexical cast fails
-    int nonNegativeInteger(const std::string& errmsg);
+    int nonNegativeInteger(const std::string& errmsg) const;
 
-    /// \brief Returns command line string if it is non-empty
+    /// \brief Returns command line string if it is not empty
     ///
     /// \param errmsg Error message if string is empty
     /// \throw InvalidParameter if string is empty
-    std::string nonEmptyString(const std::string& errmsg);
+    std::string nonEmptyString(const std::string& errmsg) const;
 
     /// \brief Calculates max_random_ and random_range_
     ///
     /// \param opt Value of -R option
     /// \throw InvalidParameter if -R<value> is wrong
-    void initRandomRange(const std::string& opt);
+    void initClientsNum();
 
     /// \brief Decodes base provided with -b<base>
     ///
-    /// Function decodes parameters given with -b<base>
-    /// parameter. Currently it supports the following command line options e.g.:
-    /// -b mac=00:01:02:03:04:05
-    /// -b duid=0F1234 (duid can be up to 128 hex digits)
+    /// Function decodes argument of -b switch, which
+    /// specifies a base value used to generate unique
+    /// mac or duid values in packets sent to system
+    /// under test.
+    /// The following forms of switch arguments are supported:
+    /// - -b mac=00:01:02:03:04:05
+    /// - -b duid=0F1234 (duid can be up to 128 hex digits)
     //  Function will decode 00:01:02:03:04:05 and/or
-    /// 0F1234 repsectively and initialize mac_prefix_
+    /// 0F1234 respectively and initialize mac_prefix_
     /// and/or duid_prefix_ members
     ///
     /// \param base Base in string format
@@ -303,7 +319,7 @@ private:
     /// \brief Decodes base MAC address provided with -b<base>
     ///
     /// Function decodes parameter given as -b mac=00:01:02:03:04:05
-    /// Function will decode 00:01:02:03:04:05 initialize mac_prefix_
+    /// The function will decode 00:01:02:03:04:05 initialize mac_prefix_
     /// class member.
     /// Provided MAC address is for example only
     ///
@@ -314,55 +330,78 @@ private:
     /// \brief Decodes base DUID provided with -b<base>
     ///
     /// Function decodes parameter given as -b duid=0F1234
-    /// Function will decode 0F1234 and initialize duid_prefix_
+    /// The function will decode 0F1234 and initialize duid_prefix_
     /// class member.
     /// Provided DUID is for example only.
     ///
-    /// \param base Base string given as -b mac=00:01:02:03:04:05
+    /// \param base Base string given as -b duid=0F1234
     /// \throws isc::InvalidParameter if DUID is invalid
     void decodeDuid(const std::string& base);
 
-    /// \brief converts two digit hex string to byte
+    /// \brief Converts two-digit hexadecimal string to a byte
     ///
-    /// \param s hex string e.g. AF
+    /// \param hex_text Hexadecimal string e.g. AF
     /// \throw isc::InvalidParameter if string does not represent hex byte
-    uint8_t convertHexString(const std::string& s);
-
-    uint8_t ipversion_;                      ///< IP version
-    ExchangeMode exchange_mode_  ;           ///< Packet exchange mode (e.g. DORR/SARR)
-    int rate_;                               ///< rate in exchange per second
-    int report_delay_;                       ///< delay between two reports
-    uint32_t random_range_;                  ///< number of simulated clients
-    uint32_t max_random_;                    ///< maximum random value
-    std::vector<uint8_t> mac_prefix_;        ///< MAC addr prefix
-    std::vector<uint8_t> duid_prefix_;       ///< DUID prefix
-    std::vector<std::string> base_;          ///< base values for xid
-    std::vector<int> num_request_;           ///< number of exchanges
-    int period_;                             ///< test period
-    int drop_time_set_;                      ///< losttime[0] was set
-    std::vector<double> drop_time_;          ///< time before a request is lost
-    int max_drop_set_;                       ///< max{p}drop[0] was set
-    std::vector<int> max_drop_;              ///< maximum number of lost requests
-    std::vector<double> max_pdrop_;          ///< maximum percentage
-    std::string localname_;                  ///< local address or interface
-    bool is_interface_;                      ///< interface vs local address
-    int preload_;                            ///< preload exchanges
-    int aggressivity_;                       ///< back to back exchanges
-    int local_port_;                         ///< local port number (host endian)
-    bool seeded_;                            ///< is a seed provided
-    uint32_t seed_;                          ///< randomization seed
-    bool broadcast_;                         ///< use broadcast
-    bool rapid_commit_;                      ///< add rapid commit option
-    bool use_first_;                         ///< where to take the server-ID
-    std::vector<std::string> template_file_; ///< template file name
-    std::vector<int> xid_offset_;            ///< template offsets (xid)*/
-    std::vector<int> rnd_offset_;            ///< template offsets (random)
-    int elp_offset_;                         ///< template offset (elapsed time)
-    int sid_offset_;                         ///< template offset (server ID)
-    int rip_offset_;                         ///< template offset (requested IP)
-    std::string diags_;                      ///< diagnostic selectors
-    std::string wrapped_;                    ///< wrapped command
-    std::string server_name_;                ///< server
+    uint8_t convertHexString(const std::string& hex_text) const;
+
+    uint8_t ipversion_;                      ///< IP protocol version to be used, expected values are:
+                                             ///< 4 for IPv4 and 6 for IPv6, default value 0 means "not set"
+    ExchangeMode exchange_mode_;             ///< Packet exchange mode (e.g. DORA/SARR)
+    int rate_;                               ///< Rate in exchange per second
+    int report_delay_;                       ///< Delay between generation of two consecutive
+                                             ///< performance reports
+    uint32_t clients_num_;                   ///< Number of simulated clients (aka randomization range).
+    std::vector<uint8_t> mac_prefix_;        ///< MAC address prefix used to generate unique DUIDs
+                                             ///< for simulated clients.
+    std::vector<uint8_t> duid_prefix_;       ///< DUID prefix used to generate unique DUIDs for
+                                             ///< simulated clients
+    std::vector<std::string> base_;          ///< Collection of base values specified with -b<value>
+                                             ///< options. Supported "bases" are mac=<mac> and duid=<duid>
+    std::vector<int> num_request_;           ///< Number of 2 or 4-way exchanges to perform
+    int period_;                             ///< Test period in seconds
+    uint8_t drop_time_set_;                  ///< Indicates number of -d<value> parameters specified by user.
+                                             ///< If this value goes above 2, command line parsing fails.
+    std::vector<double> drop_time_;          ///< Time to elapse before request is lost. The fisrt value of
+                                             ///< two-element vector refers to DO/SA exchanges,
+                                             ///< second value refers to RA/RR. Default values are { 1, 1 }
+    std::vector<int> max_drop_;              ///< Maximum number of drops request before aborting test.
+                                             ///< First value of two-element vector specifies maximum
+                                             ///< number of drops for DO/SA exchange, second value
+                                             ///< specifies maximum number of drops for RA/RR.
+    std::vector<double> max_pdrop_;          ///< Maximal percentage of lost requests before aborting test.
+                                             ///< First value of two-element vector specifies percentage for
+                                             ///< DO/SA exchanges, second value for RA/RR.
+    std::string localname_;                  ///< Local address or interface specified with -l<value> option.
+    bool is_interface_;                      ///< Indicates that specified value with -l<value> is
+                                             ///< rather interface (not address)
+    int preload_;                            ///< Number of preload packets. Preload packets are used to
+                                             ///< initiate communication with server before doing performance
+                                             ///< measurements.
+    int aggressivity_;                       ///< Number of exchanges sent before next pause.
+    int local_port_;                         ///< Local port number (host endian)
+    bool seeded_;                            ///< Indicates that randomization seed was provided.
+    uint32_t seed_;                          ///< Randomization seed.
+    bool broadcast_;                         ///< Indicates that we use broadcast address.
+    bool rapid_commit_;                      ///< Indicates that we do rapid commit option.
+    bool use_first_;                         ///< Indicates that we take server id from first received packet.
+    std::vector<std::string> template_file_; ///< Packet template file names. These files store template packets
+                                             ///< that are used for initiating echanges. Template packets
+                                             ///< read from files are later tuned with variable data.
+    std::vector<int> xid_offset_;            ///< Offset of transaction id in template files. First vector
+                                             ///< element points to offset for DISCOVER/SOLICIT messages,
+                                             ///< second element points to trasaction id offset for
+                                             ///< REQUEST messages
+    std::vector<int> rnd_offset_;            ///< Random value offset in templates. Random value offset
+                                             ///< points to last octet of DUID. Up to 4 last octets of
+                                             ///< DUID are randomized to simulate differnt clients.
+    int elp_offset_;                         ///< Offset of elapsed time option in template packet.
+    int sid_offset_;                         ///< Offset of server id option in template packet.
+    int rip_offset_;                         ///< Offset of requested ip data in template packet/
+    std::string diags_;                      ///< String representing diagnostic selectors specified
+                                             ///< by user with -x<value>.
+    std::string wrapped_;                    ///< Wrapped command specified as -w<value>. Expected
+                                             ///< values are start and stop.
+    std::string server_name_;                ///< Server name specified as last argument of command line.
 };
 
 } // namespace perfdhcp

tests/tools/perfdhcp/tests/command_options_unittest.cc

@@ -26,6 +26,9 @@ using namespace isc;
 using namespace isc::perfdhcp;
 
 /// \brief Test Fixture Class
+///
+/// This test fixture class is used to perform
+/// unit tests on perfdhcp CommandOptions class.
 class CommandOptionsTest : public virtual ::testing::Test
 {
 public:
@@ -33,18 +36,18 @@ public:
     CommandOptionsTest() { }
 
 protected:
-
     /// \brief Parse command line and cleanup
     ///
-    /// Tokenizes command line to array of C-strings,
-    /// parses arguments and de-allocates C-strings
+    /// The method tokenizes command line to array of C-strings,
+    /// parses arguments using CommandOptions class to set
+    /// its data members and de-allocates array of C-strings.
     ///
-    /// \param s Command line to parse
-    /// \return non-zero if parsing failed
-    void process(const std::string& s) {
+    /// \param cmdline Command line to parse
+    /// \throws std::bad allocation if tokenization failed
+    void process(const std::string& cmdline) {
         CommandOptions& opt = CommandOptions::instance();
         int argc = 0;
-        char** argv = tokenizeString(s, &argc);
+        char** argv = tokenizeString(cmdline, &argc);
         opt.reset();
         opt.parse(argc, argv);
         for(int i = 0; i < argc; ++i) {
@@ -54,7 +57,7 @@ protected:
         free(argv);
     }
 
-    /// \brief Check initialized values
+    /// \brief Check default initialized values
     ///
     /// Check if initialized values are correct
     void checkDefaults() {
@@ -64,7 +67,7 @@ protected:
         EXPECT_EQ(CommandOptions::DORA_SARR, opt.getExchangeMode());
         EXPECT_EQ(0, opt.getRate());
         EXPECT_EQ(0, opt.getReportDelay());
-        EXPECT_EQ(0, opt.getRandomRange());
+        EXPECT_EQ(0, opt.getClientsNum());
 
         // default mac
         uint8_t mac[6] = { 0x00, 0x0C, 0x01, 0x02, 0x03, 0x04 };
@@ -109,28 +112,33 @@ protected:
     /// \param s String to split (tokenize)
     /// \param num Number of tokens returned
     /// \return array of C-strings (tokens)
-    char** tokenizeString(const std::string& s, int* num) const {
+    char** tokenizeString(const std::string& text_to_split, int* num) const {
         char** results = NULL;
-        std::stringstream ss(s);
-        std::istream_iterator<std::string> sit(ss);
-        std::istream_iterator<std::string> send;
-        std::vector<std::string> tokens(sit, send);
+        // Tokenization with std streams
+        std::stringstream text_stream(text_to_split);
+        // Iterators to be used for tokenization
+        std::istream_iterator<std::string> text_iterator(text_stream);
+        std::istream_iterator<std::string> text_end;
+        // Tokenize string (space is a separator) using begin and end iteratos
+        std::vector<std::string> tokens(text_iterator, text_end);
 
         if (tokens.size() > 0) {
+            // Allocate array of C-strings where we will store tokens
             results = static_cast<char**>(malloc(tokens.size() * sizeof(char*)));
             if (results == NULL) {
                 throw std::bad_alloc();
             }
+            // Store tokens in C-strings array
             for (int i = 0; i < tokens.size(); ++i) {
                 char* cs = static_cast<char*>(malloc(tokens[i].length() + 1));
                 strcpy(cs, tokens[i].c_str());
                 results[i] = cs;
             }
+            // Return number of tokens to calling function
             if (num != NULL) {
                 *num = tokens.size();
             }
         }
-
         return results;
     }
 
@@ -159,7 +167,11 @@ TEST_F(CommandOptionsTest, IpVersion) {
     EXPECT_FALSE(opt.isRapidCommit());
 
     // Negative test cases
+    // -4 and -6 must not coexist
+    EXPECT_THROW(process("perfdhcp -4 -6 -l ethx"), isc::InvalidParameter);
+    // -6 and -B must not coexist
     EXPECT_THROW(process("perfdhcp -6 -B -l ethx"), isc::InvalidParameter);
+    // -c and -4 (default) must not coexist
     EXPECT_THROW(process("perfdhcp -c -l ethx"), isc::InvalidParameter);
 }
 
@@ -169,7 +181,9 @@ TEST_F(CommandOptionsTest, Rate) {
     EXPECT_EQ(10, opt.getRate());
 
     // Negative test cases
+    // Rate must not be 0
     EXPECT_THROW(process("perfdhcp -4 -r 0 -l ethx"), isc::InvalidParameter);
+    // -r must be specified to use -n, -p and -D
     EXPECT_THROW(process("perfdhcp -6 -t 5 -l ethx"), isc::InvalidParameter);
     EXPECT_THROW(process("perfdhcp -4 -n 150 -l ethx"), isc::InvalidParameter);
     EXPECT_THROW(process("perfdhcp -6 -p 120 -l ethx"), isc::InvalidParameter);
@@ -182,19 +196,21 @@ TEST_F(CommandOptionsTest, ReportDelay) {
     EXPECT_EQ(17, opt.getReportDelay());
 
     // Negative test cases
-    EXPECT_THROW(process("perfdhcp -r -3 -t 17 -l ethx"), isc::InvalidParameter);
-    EXPECT_THROW(process("perfdhcp -r 0 -t 17 -l ethx"), isc::InvalidParameter);
-    EXPECT_THROW(process("perfdhcp -r s -t 17 -l ethx"), isc::InvalidParameter);
+    // -t must be positive integer
+    EXPECT_THROW(process("perfdhcp -r 10 -t -8 -l ethx"), isc::InvalidParameter);
+    EXPECT_THROW(process("perfdhcp -r 10 -t 0 -l ethx"), isc::InvalidParameter);
+    EXPECT_THROW(process("perfdhcp -r 10 -t s -l ethx"), isc::InvalidParameter);
 }
 
-TEST_F(CommandOptionsTest, RandomRange) {
+TEST_F(CommandOptionsTest, ClientsNum) {
     CommandOptions& opt = CommandOptions::instance();
     process("perfdhcp -R 200 -l ethx");
-    EXPECT_EQ(200, opt.getRandomRange());
+    EXPECT_EQ(200, opt.getClientsNum());
     process("perfdhcp -R 0 -l ethx");
-    EXPECT_EQ(0, opt.getRandomRange());
+    EXPECT_EQ(0, opt.getClientsNum());
 
     // Negative test cases
+    // Number of clients must be non-negative integer
     EXPECT_THROW(process("perfdhcp -R -5 -l ethx"), isc::InvalidParameter);
     EXPECT_THROW(process("perfdhcp -R gs -l ethx"), isc::InvalidParameter);
 }
@@ -209,16 +225,20 @@ TEST_F(CommandOptionsTest, Base) {
     std::vector<uint8_t> v1 = opt.getMacPrefix();
     ASSERT_EQ(6, v1.size());
     EXPECT_TRUE(std::equal(v1.begin(), v1.end(), mac));
+    // "3x" is invalid value in MAC address
     EXPECT_THROW(process("perfdhcp -b mac=10::2::3x::4::5::6 -l ethx"), isc::InvalidParameter);
 
     // Test DUID
     std::vector<uint8_t> v2 = opt.getDuidPrefix();
     ASSERT_EQ(sizeof(duid) / sizeof(uint8_t), v2.size());
     EXPECT_TRUE(std::equal(v2.begin(), v2.end(), duid));
+    // "t" is invalid digit in DUID
     EXPECT_THROW(process("perfdhcp -6 -l ethx -b duiD=1AB7Ft670901FF"), isc::InvalidParameter);
 
     // Some more negative test cases
+    // Base is not specified
     EXPECT_THROW(process("perfdhcp -b -l ethx"), isc::InvalidParameter);
+    // Typo: should be mac= instead of mc=
     EXPECT_THROW(process("perfdhcp -l ethx -b mc=00:01:02:03::04:05"), isc::InvalidParameter);
 }
 
@@ -235,6 +255,7 @@ TEST_F(CommandOptionsTest, DropTime) {
     EXPECT_DOUBLE_EQ(4.7, opt.getDropTime()[1]);
 
     // Negative test cases
+    // Drop time must not be negative
     EXPECT_THROW(process("perfdhcp -l ethx -d -2 -d 4.7"), isc::InvalidParameter);
     EXPECT_THROW(process("perfdhcp -l ethx -d -9.1 -d 0"), isc::InvalidParameter);
 }
@@ -245,9 +266,12 @@ TEST_F(CommandOptionsTest, TimeOffset) {
     EXPECT_EQ(4, opt.getElapsedTimeOffset());
 
     // Negative test cases
+    // Argument -E must be used with -T
     EXPECT_THROW(process("perfdhcp -l ethx -E 3 -i"), isc::InvalidParameter);
-    EXPECT_THROW(process("perfdhcp -l ethx -E -i"), isc::InvalidParameter);
-    EXPECT_THROW(process("perfdhcp -l ethx -E -3"), isc::InvalidParameter);
+    // Value in -E not specified
+    EXPECT_THROW(process("perfdhcp -l ethx -T file.x -E -i"), isc::InvalidParameter);
+    // Value for -E must not be negative
+    EXPECT_THROW(process("perfdhcp -l ethx -E -3 -T file.x"), isc::InvalidParameter);
 }
 
 TEST_F(CommandOptionsTest, ExchangeMode) {
@@ -256,11 +280,13 @@ TEST_F(CommandOptionsTest, ExchangeMode) {
     EXPECT_EQ(CommandOptions::DO_SA, opt.getExchangeMode());
 
     // Negative test cases
+    // No template file specified
     EXPECT_THROW(process("perfdhcp -i -l ethx -X 3"), isc::InvalidParameter);
-    EXPECT_THROW(process("perfdhcp -i -l ethx -O 2"), isc::InvalidParameter);
-    EXPECT_THROW(process("perfdhcp -i -l ethx -E 3"), isc::InvalidParameter);
-    EXPECT_THROW(process("perfdhcp -i -l ethx -S 1"), isc::InvalidParameter);
-    EXPECT_THROW(process("perfdhcp -i -l ethx -I 2"), isc::InvalidParameter);
+    // Offsets can't be used in simple exchanges (-i)
+    EXPECT_THROW(process("perfdhcp -i -l ethx -O 2 -T file.x"), isc::InvalidParameter);
+    EXPECT_THROW(process("perfdhcp -i -l ethx -E 3 -T file.x"), isc::InvalidParameter);
+    EXPECT_THROW(process("perfdhcp -i -l ethx -S 1 -T file.x"), isc::InvalidParameter);
+    EXPECT_THROW(process("perfdhcp -i -l ethx -I 2 -T file.x"), isc::InvalidParameter);
 }
 
 TEST_F(CommandOptionsTest, Offsets) {
@@ -277,6 +303,7 @@ TEST_F(CommandOptionsTest, Offsets) {
     EXPECT_EQ(3, opt.getTransactionIdOffset()[1]);
 
     // Negative test cases
+    // IP offset/IA_NA offset must be positive
     EXPECT_THROW(process("perfdhcp -6 -I 0 -l ethx"), isc::InvalidParameter);
     EXPECT_THROW(process("perfdhcp -6 -I -4 -l ethx"), isc::InvalidParameter);
 
@@ -289,8 +316,10 @@ TEST_F(CommandOptionsTest, LocalPort) {
     EXPECT_EQ(2000, opt.getLocalPort());
 
     // Negative test cases
+    // Local port must be between 0..65535
     EXPECT_THROW(process("perfdhcp -l ethx -L -2"), isc::InvalidParameter);
     EXPECT_THROW(process("perfdhcp -l ethx -L"), isc::InvalidParameter);
+    EXPECT_THROW(process("perfdhcp -l ethx -L 65540"), isc::InvalidParameter);
 }
 
 TEST_F(CommandOptionsTest, Preload) {
@@ -299,7 +328,9 @@ TEST_F(CommandOptionsTest, Preload) {
     EXPECT_EQ(3, opt.getPreload());
 
     // Negative test cases
+    // Number of preload packages must not be negative integer
     EXPECT_THROW(process("perfdhcp -P -1 -l ethx"), isc::InvalidParameter);
+    EXPECT_THROW(process("perfdhcp -P -3 -l ethx"), isc::InvalidParameter);
 }
 
 TEST_F(CommandOptionsTest, Seed) {
@@ -313,6 +344,7 @@ TEST_F(CommandOptionsTest, Seed) {
     EXPECT_FALSE(opt.isSeeded());
 
     // Negtaive test cases
+    // Seed must be non-negative integer
     EXPECT_THROW(process("perfdhcp -6 -P 2 -s -5 -l ethx"), isc::InvalidParameter);
     EXPECT_THROW(process("perfdhcp -6 -P 2 -s -l ethx"), isc::InvalidParameter);
 }
@@ -329,7 +361,10 @@ TEST_F(CommandOptionsTest, TemplateFiles) {
     EXPECT_EQ("file2.x", opt.getTemplateFiles()[1]);
 
     // Negative test cases
+    // No template file specified
     EXPECT_THROW(process("perfdhcp -s 12 -l ethx -T"), isc::InvalidParameter);
+    // Too many template files specified
+    EXPECT_THROW(process("perfdhcp -s 12 -l ethx -T file.x -T file.x -T file.x"), isc::InvalidParameter);
 }
 
 TEST_F(CommandOptionsTest, Wrapped) {
@@ -338,6 +373,7 @@ TEST_F(CommandOptionsTest, Wrapped) {
     EXPECT_EQ("start", opt.getWrapped());
 
     // Negative test cases
+    // Missing command after -w, expected start/stop
     EXPECT_THROW(process("perfdhcp -B -i -l ethx -w"), isc::InvalidParameter);
 }
 
@@ -345,5 +381,74 @@ TEST_F(CommandOptionsTest, Diagnostics) {
     CommandOptions& opt = CommandOptions::instance();
     process("perfdhcp -l ethx -i -x asTe");
     EXPECT_EQ("asTe", opt.getDiags());
+
+    // Negative test cases
+    // No diagnostics string specified
+    EXPECT_THROW(process("perfdhcp -l ethx -i -x"), isc::InvalidParameter);
+}
+
+TEST_F(CommandOptionsTest, Aggressivity) {
+    CommandOptions& opt = CommandOptions::instance();
+    process("perfdhcp -a 10 -l 192.168.0.1");
+    EXPECT_EQ(10, opt.getAggressivity());
+
+    // Negative test cases
+    // Aggressivity must be non negative integer
+    EXPECT_THROW(process("perfdhcp -l ethx -a 0"), isc::InvalidParameter);
+    EXPECT_THROW(process("perfdhcp -l ethx -a"), isc::InvalidParameter);
+    EXPECT_THROW(process("perfdhcp -a -2 -l ethx -a 3"), isc::InvalidParameter);
 }
 
+TEST_F(CommandOptionsTest, MaxDrop) {
+    CommandOptions& opt = CommandOptions::instance();
+    process("perfdhcp -D 25 -l ethx -r 10");
+    EXPECT_EQ(25, opt.getMaxDrop()[0]);
+    process("perfdhcp -D 25 -l ethx -D 15 -r 10");
+    EXPECT_EQ(25, opt.getMaxDrop()[0]);
+    EXPECT_EQ(15, opt.getMaxDrop()[1]);
+
+    process("perfdhcp -D 15% -l ethx -r 10");
+    EXPECT_EQ(15, opt.getMaxDropPercentage()[0]);
+    process("perfdhcp -D 15% -D25% -l ethx -r 10");
+    EXPECT_EQ(15, opt.getMaxDropPercentage()[0]);
+    EXPECT_EQ(25, opt.getMaxDropPercentage()[1]);
+    process("perfdhcp -D 1% -D 99% -l ethx -r 10");
+    EXPECT_EQ(1, opt.getMaxDropPercentage()[0]);
+    EXPECT_EQ(99, opt.getMaxDropPercentage()[1]);
+
+    // Negative test cases
+    // Too many -D<value> options
+    EXPECT_THROW(process("perfdhcp -D 0% -D 1 -l ethx -r20 -D 3"), isc::InvalidParameter);
+    // Too many -D<value%> options
+    EXPECT_THROW(process("perfdhcp -D 99% -D 13% -l ethx -r20 -D 10%"), isc::InvalidParameter);
+    // Percentage is out of bounds
+    EXPECT_THROW(process("perfdhcp -D101% -D 13% -l ethx -r20"), isc::InvalidParameter);
+    EXPECT_THROW(process("perfdhcp -D0% -D 13% -l ethx -r20"), isc::InvalidParameter);
+}
+
+TEST_F(CommandOptionsTest, NumRequest) {
+    CommandOptions& opt = CommandOptions::instance();
+    process("perfdhcp -n 1000 -r 10 -l ethx");
+    EXPECT_EQ(1000, opt.getNumRequests()[0]);
+    process("perfdhcp -n 5 -r 10 -n 500 -l ethx");
+    EXPECT_EQ(5, opt.getNumRequests()[0]);
+    EXPECT_EQ(500, opt.getNumRequests()[1]);
+
+    // Negative test cases
+    // Too many -n<value> parameters, expected maximum 2
+    EXPECT_THROW(process("perfdhcp -n 1 -n 2 -l ethx -n3 -r 20"), isc::InvalidParameter);
+    // Num request must be positive integer
+    EXPECT_THROW(process("perfdhcp -n 1 -n -22 -l ethx -r 10"), isc::InvalidParameter);
+    EXPECT_THROW(process("perfdhcp -n 0 -l ethx -r 10"), isc::InvalidParameter);
+}
+
+TEST_F(CommandOptionsTest, Period) {
+    CommandOptions& opt = CommandOptions::instance();
+    process("perfdhcp -p 120 -l ethx -r 100");
+    EXPECT_EQ(120, opt.getPeriod());
+
+    // Negative test cases
+    // Test period must be positive integer
+    EXPECT_THROW(process("perfdhcp -p 0 -l ethx -r 50"), isc::InvalidParameter);
+    EXPECT_THROW(process("perfdhcp -p -3 -l ethx -r 50"), isc::InvalidParameter);
+}