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      |
-------------------------------------+-------------------------------------

Comment (by marcin):

 Replying to [comment:6 stephen]:
 > 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.

 Moved.

 >
 > 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).

 Added define as suggested.

 >
 > 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'.

 Fixed.

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

 Fixed.

 >
 > 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.)

 I got rid of second clause as suggested.

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

 It can be even more colons. I extended error message to point out that
 single colon is fine too.

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

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

 Fixed.

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

 Fixed.

 >
 > 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.)?

 I made it shorter. For example I created new function sendPackets() that
 is used both to preload the server and send packets when not preloading.

 >
 > 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.

 I added some more comments about wrapped commands.

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

 Since testDiags('T') repeats in every sendX() function I created the
 function which does testDiags('T') and saves the first packet. Its
 description tells what is going on there.

 >
 > 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?

 Comment added.

 >
 > '''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 extended the class description.

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

 Added. I hope that everywhere it was missing.

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

 I renamed the class.

 >
 > Typo in !SequentialGenerator header - "maximym".

 Fixed.

 >
 > !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.

 I changed the description.



 Apart from changes suggested in the review I also added EXPECT_NO_THROW to
 each call to process() function in command_options_unittest where I don't
 expect it to throw.

 I fixed some doxygen issues.

 I also corrected the configure.ac as there was some redundant code there
 (for old perfdhcp).

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