BIND 10 #1960: perfdhcp integration

[BIND 10 Development]

#1960: perfdhcp integration
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  marcin
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:  Sprint-
  enhancement                        |  DHCP-20121004
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DHCP
  perfdhcp                           |  Estimated Difficulty:  0
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => marcin


Comment:

 Reviewed commit 5da8f0ce275404a225759f9673daffa4d9da386a

 '''tests/tools/perfdhcp/command_options.cc'''[[BR]]
 initialize(): Why not have the list of switches in the "getopt" argument
 in alphabetical order?

 initialize(): the list of options in the various "case" labels is in
 alphabetical order ''apart'' from "v".  Suggest moving it to the
 appropriate place.

 initialize(): As "255.255.255.255" and "FF02::1:2" are used more than
 once, why not define symbols for them?  (In fact, as the program links
 against libdhcp++, it is permissible for it to use symbols from there.
 dhcp6.h defines the symbols for the DHCP6 broadcast addresses, and dhcp4.h
 could be extended to define something like DHCP_IPV4_BROADCAST_ADDRESS).

 initialize(): At the block of code with the comment "Handle the local '-l'
 address/interface", the variable broadcast_ is set to 1; as broadcast_ is
 a bool, it should be set 'true'.

 initClientsNum(): isc_throw is passed 'errmsg.c_str()' in the first
 invocation and 'errmsg' in the second.

 initClientsNum(): the use of two try/catch blocks is not needed: a single
 one is sufficient:
 {{{
 const std::string errmsg = "...";
 try {
     long long client_num = boost::lexical_cast<long long>(optarg);
     check(client_num < 0, errmsg);
     clients_num_ = boost::lexical_cast<uint32_t>(optarg);
 } catch (boost::lexical_cast&) {
     isc_throw(isc::InvalidParameter, errmsg);
 }
 }}}
 (If check() fails, it throws an isc::!InvalidParameter exception, which
 will not be caught by the "catch" clause.)

 decodeMac(): the error message states that MAC components should be
 separated by double-colons: is this correct?

 '''tests/tools/perfdhcp/main.cc'''[[BR]]
 If an error message is output, shouldn't it be written to cerr, not cout?

 '''tests/tools/perfdhcp/stats_mgr.h'''[[BR]]
 CustomCounter::operator+=(): Could use {{{counter_ += val}}} construct.

 '''tests/tools/perfdhcp/test_control.cc'''[[BR]]
 receivePackets(): return value should be enclosed in parentheses.

 run() is a bit long: can it be split up at all (e.g. move the "preload
 server" loop into a separate method, put the printing of diagnostic
 messages into a separate method etc.)?

 run(): when calling runWrapped(do_stop), why the intermediate variable
 do_stop?  Why not just call runWrapped() with a literal value of "true"?
 Also, the reason for this call needs a comkment.

 sendDiscover4(): add a comment before the testDiags('T')?  Not clear what
 is happening here.

 sendRequest6(): A comment would help in at the head of the first "if" in
 this method.  Why are you testing the result of the
 CommandOptions::instance()::isUseFirst() method?

 '''tests/tools/perfdhcp/test_control.h'''[[BR]]
 The header comments for the !TestControl class needs to be extended.  For
 example, !TestControl appears to start subprocesses - there is no
 indication in the header that this is done.  Is a subprocess started in
 all cases, or just some?  What does it do? etc.

 I presume that the offset variables are offsets in the DHCP packet.  If
 so, this should be stated (the comments only mention "offsets").

 Typo in the name of a class: "Sequencial" should be "Sequential".

 Typo in !SequentialGenerator header - "maximym".

 !SequentialGenerator: strictly speaking, range_ is not the maximum number
 generated, it is one more than the maximum number: (num_ + 1) % range_
 returns values between 0 and range_ - 1 inclusive.

-- 
Ticket URL: <http://bind10.isc.org/ticket/1960#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development