BIND 10 #1959: Implement perfdhcp control logic

[BIND 10 Development]

#1959: Implement perfdhcp control logic
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:  Sprint-
  enhancement                        |  DHCP-20120903
                   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 tomek):

 '''tests/tools/perfdhcp/command_options.h'''
 Comment for initialize() method: Initializes class members based [on the]
 command line. There are missing dots at the end of many doxygen comments.

 Why is the generateDuidPrefix() function contains prefix in its name? It
 generates the whole DUID, not just prefix. Prefix has no meaning in the
 DUID context. As prefix has specific meaning in DHCP context, it is better
 to avoid ambiguity here. Perhaps it would be better to use "template"
 instead?

 generateDuidPrefix() uses 6 byte long MACs. That is ok for now, but it
 should be extended eventually. The new DUID type DUID-UUID is more than 14
 octets long. RFC3315 states that DUIDs can be up to 128 bytes long. We
 should test it one day. I don't suggest that we should implement it now,
 but adding some TODO comment is reasonable.

 CommandOptions::printCommandLine() should also print specified command-
 line parameters as they are. That may seem redundant, but there is very
 good reason to print them out. It is easy to reproduce the case, based
 only on log file. It is very useful to keep a format that can be copy-
 pasted easily into the next execution.

 There are no tests for initIsInterface() and generateDuidPrefix().

 '''tests/tools/perfdhcp/localized_option.h'''
 Comments for constructors should advise that options 0 and 255 (v4) and 0
 (v6) are not valid. It is still very much ok to allow them for server
 testing purposes.

 Constructors:
 {{{
     LocalizedOption(dhcp::Option::Universe u,
                     uint16_t type,
                     dhcp::OptionBufferConstIter first,
                     dhcp::OptionBufferConstIter last)
 }}}
 and
 {{{

     LocalizedOption(dhcp::Option::Universe u,
                     uint16_t type,
                     dhcp::OptionBufferConstIter first,
                     dhcp::OptionBufferConstIter last, const size_t offset)
 }}}
 can be combined, with the default value of the fifth parameter being 0.

 Invalid namespace specified in line 172. It is isc::perfdhcp, not
 perfdhcp.

 '''perf_pkt4.cc''' and '''perf_pkt6.cc'''
 Methods writeAt() and writeValueAt are not tested.

 '''pkt_transform.cc'''
 PktTransform::writeAt() uses inefficient byte-by-byte copying. It would be
 faster to use something like
 {{{
 memcpy(&in_buffer[dst_pos], &(*first), distance(first, last));
 }}}
 This is a performance testing tool - using tradeoffs between clean C++
 approach vs raw speed should usually favor speed.

 I couldn't find any tests for PktTransform class. Are there any? I see
 that it is used by PerfPkt4, PerfPkt6 that are tested, but it would be
 better to test PktTransform directly as well.

 '''pkt_transform.h'''
 writeValueAt() is intended to be used for integer types only, right?
 Aren't there a simpler way to do it?

 '''stats_mgr.h'''
 Why are two opertor++ definitions are needed?

 getValue(), getName() and similar - for simple methods no repetition of
 \brief descriptions are needed, the brief description is sufficient.

 PktList type - I bow to your superior template handling techniques, sir.
 How many nested templates are there? 6? :) Seriously speaking, I do not
 understand that PktList definition.

 sum_delay_squared_ field - Is this squared sum of delays or sum of squared
 delays? Note that it is not the same. If it is the former, then it is not
 needed and sum_delay_ may be used instead.

 getDroppedPacketsNum(): Unless this method is expected to become more
 complex in the future, the drops variable is not needed. You ca use:
 {{{
 if (getSentPacketsNum() > getRcvdPacketsNum()) {
     return (getSentPacketsNum() - getRcvdPacketsNum());
 } else {
     return (0);
 }
 }}}

 printIntermediateStats(): ostringstream objects don't have to be
 explicitly initialized to empty strings. The default constructor does that
 as well, but is faster. ++it in "for" loop doesn't have to be written in a
 separate line.

 Review finished on line 1502 of the patch. It is after 8pm Friday night.
 The review will be continued on Monday.

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