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:  16
                   Keywords:         |           Total Hours:  42
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  6      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => marcin


Comment:

 Review commit c65a54347a7bceee54531a6a81209d1ec6ceb82b

 '''test/tools/perfdhcp/command_options.h'''[[BR]]
 There are two "private" declarations (the second coming from changing
 "protected" to "private").  The second can be removed.

 The description of the default constructor should start "Private
 constructor..." (not "Protected constructor...").

 In the description of the check() method, there is no need to prefix
 !InvalidParameter with an exclamation mark. (That is only needed when
 documenting in the wiki.)

 positiveInteger() and related methods can be declared "const", as they
 don't change anything in the class.

 Abbreviations are best avoided (where reasonable) in the documentation as
 it makes it more like English and easier to read, e.g. "Casts command line
 arg to positive int" is better as "Casts command line argument to positive
 int". (Note: int - and not "integer" - being used as this is the name of a
 C++ type.)

 initRandomRange(): The exception is thrown if the -R value is wrong - but
 what constitutes wrong?  It would be useful for the description to explain
 this.

 decodeBase(): Several points:
 * Explanation of what the "base" is would be useful (e.g. "Method decodes
 the argument of the '-b' switch, which specifies a base value used to
 generate unique mac or duid values in packets sent to the system under
 test.")
 * (trivial) the description is 55 characters wide on the first line and 81
 on the second - it is easier to read if the lines are approximately the
 same length. (No maximum line length is given in the guidelines, but
 usually blocks of comments reach to column 80 at most.)  The three lines
 after the two examples can also extend to column 80.
 * "Currently is supports the following command line options, e.g." is
 perhaps better written as "The following forms of switch arguments are
 supported:"
 * The two lines should be prefixed by "- " to generate a list (see
 http://www.stack.nl/~dimitri/doxygen/markdown.html).
 * Typo in "respectively"

 decodeDuid(): Suggest use of the definite article where needed (i.e. "The
 function will..." rather than "Function will").  Also, the example in the
 description of the parameter "base" is a MAC example, not a DUID example.

 convertHexString(): Suggest title be "Convert two-digit hexadecimal string
 to a byte".  Suggest also using a more descriptive name for the parameter
 (e.g. "hex_text" rather than "s").  Finally, this can be a "const" method
 as it does not modify anything in the class.

 Should drop_time_set_ and max_drop_set_ be bool instead of int?

 drop_time_set_ comments refers to losttime, which was renamed in the last
 set of changes.

 '''test/tools/perfdhcp/command_options.cc'''[[BR]]
 reset(): Comments should follow normal rules of English and start with a
 capital letter.

 parse(): Resets the optind/opterr variables, but should it not also call
 reset()?

 initialize(): The [wiki:CodingGuidelines coding guidelines] state that
 lowercaseUppercase is the format reserved for method names.  Local
 variables should be all lower-case, possibly separated by underscores
 (e.g. lower_case).

 initialize(): Would prefer the in-line comments to start with a capital
 letter (and the same really goes for the inline variables in the header
 file).  Also typo in "holiding".

 initialize() - (parsing "-D"): the possibility of lexical_cast() throwing
 an exception is ignored.

 initialize() - (parsing "-D"): on inspection, it appears that the code is
 assuming that the format of the switch is "-D %<number>" (as substr() with
 a single argument is taking the contents of the string from the position
 given by the argument to the end of the string).  (It also appears that
 there is no unit test for the "-D n%" form, else this would have been
 caught.)

 initialize() - (parsing "-D"): the structure is currently:
 {{{
 if (...) {
     :
     break;
 }
 :
 break;
 }}}
 I think clearer is:
 {{{
 if (...) {
    :
 } else {
    :
 }
 break;
 }}}
 (i.e. one exit point).

 initialize() - (parsing "-L"): not need for a space following the left
 parenthesis in "static_cast<int>( ".

 initialize() - (parsing "-T"): An if/else (with a test against the value
 2) seems more logical than a switch statement here.

 initialize() - (parsing "-O"): Can't positiveInteger() be called instead
 of using a try/catch block?

 initialize() - (parsing "-O" and "-X"): is the logic correct here?  If I
 specify multiple "-O" or "-X" values on the command line, it appears that
 all but the last one or two are ignored.

 initialize() - ("default" in the switch statement): the error message
 appears to be short enough for the call to isc_throw() to be on one line.

 initialize() - (general): I think it would be easier to follow if the
 switches appeared in alphabetical order.

 initialize(): Comment preceding the checking of "ipversion_ == 0" does not
 make sense.  Something like "If the IP version was not specified in the
 command line, assume IPv4." is better.

 initialize(): Not clear why we are "tuning up template file lists".  Why
 is the first value in the xid/rnd offset array being duplicated?

 initialize(): In the "Get server argument" block, a comment that FF02::1:2
 and FF02::1:3 are  the [http://tools.ietf.org/html/rfc3315
 RFC3315]-defined All_DHCP_Relay_Agents_and_Servers and All_DHCP_Servers
 addresses would be helpful.  (And for completeness, add a comment stating
 that 255.255.255.255 is the IPv4 broadcast address.)

 initRandomRange(): "errmsg" does not need to be "std::string" - it could
 be declared as "const char*" (which avoids the need to call c_str() before
 using it).

 initRandomRange(): why is "randomRange" (which should be random_range -
 see note on variable names) declared as "long long"?  The "opt" argument
 is cast to "long long", then that value is cast to uint32_t.  Why not cast
 to uint32_t directly?

 initRandomRange(): appears to be some odd spacing before the "=" sign when
 setting maxTargetRand and maxRandom (which should be renamed
 max_target_rand and max_random).

 initRandomRange(): not quite clear what the comment including "range is
 already multiple of number of clients range" means.

 initRandomRange(): no space between the template parameter and the left
 parenthesis in the cast of restrictedRandom.

 usage(): The documentation for "-w" seems cryptic.  presumably the
 argument is a program called through use of "system()" and passed "start"
 or "stop"?.

 usage(): The documentation for diagnostics should say that unrecognised
 key letters are ignored.

 '''test/tools/perfdhcp/tests/command_options_unittest.cc'''[[BR]]
 CommandOptionsTest::process(): would prefer a more descriptive name to the
 argument than "s".

 !IpVersion: need a test that an exception is thrown if both -4 and -6 are
 given together.

 !IpVersion: the comment "negative test cases" is not strictly correct -
 these  checks are checking that invalid combinations of flags are caught.

 !RandomRange: according to usage(), the "-R" switch specifies the number
 of different clients.  It does not indicate that anything is random.  Is
 the use of the word "random" justified?

 Base: need to check invalid values passed as arguments to "mac=" and
 "duid=" are caught.

 !DropTime: question: reading usage() suggests that !DropTime should be a
 double value.  Yet !DropTime is stored as as array of two doubles: why?

 !TimeOffset: -E only appears to be valid with -T, so checking for an
 invalid value of -3 should be done with -T present in the command line.

 !ExchangeMode: not clear why the negative test cases should fail (e.g. the
 one with "-E 3" should preumably fail because there is no -T present.)

 !LocalPort: should check that a port value above 65535 should cause a
 failure.

 There appear to be no tests for: -a, -D, -n, -p.

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