BIND 10 #1954: Implement perfdhcp command parser

[BIND 10 Development]

#1954: Implement perfdhcp command parser
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  marcin
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:  Sprint-
  enhancement                        |  DHCP-20120528
                   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:  36     |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => marcin


Comment:

 Reviewed commit d81f573764fe2df0006527145e439893872e6c69

 '''tests/tools/perfdhcp/command_options.h'''[[BR]]
 Should have a comment explaining what !ExchangeMode is.

 DORR_SARR should be DORA_SARR (final packet in an IPv4 exchange is an Ack)

 Do we need force_reset?  The command should only be parsed once in the the
 main perfdhcp code so it is not needed.  And during testing, resetting the
 state of the variables on each parse seems to be the logical way to use
 the class.

 getSidOffset() (etc).  How often are these functions this used in the
 code?  If just once, it may be clearer to expand the names (e.g.
 getServerIdOffset()).

 Default constructor: can this be private (a more restrictive scope)?

 check(): A bit more documentation is needed - although the functionality
 can be deduced by reading the description of the exception, just something
 like "Convenience function that throws an !InvalidParameter exception if
 the conditional argument is false" is clearer.

 validate(): typo in !InvalidParameter

 decodeBase(): An explanation of what the "base" is would be useful.
 (Likewise for decodeMac and decodeDuid.  A sentence here saves a couple of
 minutes finding the help documentation and locating the information for
 the "-b" flag.)



 '''tests/tools/perfdhcp/command_options.cc'''[[BR]]
 The single instance of !CommandOptions can be more simply be created by:
 {{{
 CommandOptions&
 CommandOptions::instance() {
     static CommandOptions options();
     return (options);
 }
 }}}
 (A static variable within a function is initialized the first time the
 function is called.  Thereafter the initialization is skipped.)

 reset(): Add a comment explaining what "mac" and "lt" are (and why they
 don't default to zeroes).

 reset(): calling vector::resize(0) to empty is vector is correct, although
 I get the impression that calling clear() is more usual.

 initialize(): the use of lexical_cast here does not take account of the
 fact that the argument could be of the incorrect type and that
 lexical_cast would throw an error.  Given the number of command switches
 that require a positive integer, a lot of the processing could be
 extracted into a separate function, e.g.
 {{{
 int
 positiveInteger(const char* errmsg) {
    try {
       int value = boost::lexical_cast<int>(optarg);
       check(value <= 0, errmsg);
       return (value);
    } catch (boost::bad_lexical_cast&) {
       isc_throw(InvalidParameter, errmsg);
    }
 }
 }}}
 (A similar function could be created to check for non-negative integers.)

 initialize(): Useful to document all the local variables with an inline
 comment.  Also, unless there is a good reason, I suggest avoiding single-
 character or two-character names - it is less clear for a reader.

 initialize(): Clearer to explicitly declare "of" on a line by itself.
 (Declaring variables one per line is preferred as it encourages the
 addition of an in-line comment describing the variable.)

 initialize(): The "switch" statement should be indented with respect to
 the "while".

 initialize(): Should "-h" cause parsing to terminate?

 initialize(): "r" is declared as "long long" but is then cast to unit32_t.
 This is not safe if "long long" is 64 bits.  Why not directly lexical_cast
 optarg to uint32_t?

 initialize(): More descriptive names in the calculations instead of "m, b
 and s" would be helpful.  Also "m" can be declared on the line in which it
 is set instead of as the second variable declaration in the preceding
 line.

 initialize(): Even single-line "if" clauses must be enclosed in braces (in
 the checking of ipversion_ equal to zero, and in the following "if"
 checks.)

 initialize(): when there are only two boolean checks in an "if" check, it
 is probably clearer to put them both on the same line if they fit (block
 of code prefixed "Decode special cases").

 initialize(): check the boolean variables.  broadcast_ and rapid_commit_
 (amongst others) are boolean, so should be set to "true", not to "1".

 decodeBase(): What if the "-b" argument is unknown? (In other words,
 should there be an "else" clause?)

 decodeBase(): if the base is a MAC address, the code expects "mac=" to be
 lower-case.  Yet the error message in decodeMac indicates that "MAC="
 should be uppercase.

 decodeMac(): "found+1" should be "found + 1".

 decodeMac(): Although the compiler should take care of it, suggest that
 the error message be declared as a "static const char*" variable and that
 the variable be passed to the check() call.

 usage(): suggest that we omit the "\f" (form-feed) character in the
 output.

 version(): just a remark that this will need to be changed once we've
 sorted out how we are going to maintain a version number.


 '''tests/tools/perfdhcp/tests/command_options_unittest.cc'''[[BR]]
 Test Base: if the vector::operator==() fails on FreeBSD, does std::equal()
 work?  In other words, does
 {{{
 EXPECT_TRUE(std::equal(v1.begin(), v1.end(), mac));
 }}}
 ... work?

 Test Base: not too important for an int, but in general "++obj" is
 preferred to "obj++" as it eliminates the need to hold a temporary object.

 Test Base: should check for invalid MAC and DUID values.

 Test !DropTime: is this test correctly named?  It checks the !LostTime
 variables. (Or is the !LostTime variable incorrectly named?)

 Tests !ReportDelay/!RandomRange/!LocalPort/Preload/Seed: should also check
 for invalid values of the argument.

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