BIND 10 #1324: Basic DHCP benchmark software framework

[BIND 10 Development]

#1324: Basic DHCP benchmark software framework
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  johnd
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:  major  |  DHCP-20111109
                  Component:  dhcp   |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DNS
Feature Depending on Ticket:         |  Estimated Difficulty:  0
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => johnd


Comment:

 Reviewed commit 06d6be693064252ed2535fc8685ca4e7b8db0989

 '''General'''[[BR]]
 From reading this, the parsing code appears to be something you wrote
 several years ago.  Assuming you have the copyright to the code, putting
 it in BIND 10 means that it is covered by ISCs standard terms and
 conditions - you relinquish copyright.

 It contains a lot of features - configuration files, multiple values, etc.
 - that are just not needed for perfdhcp.  This makes it overly complicated
 for what we want, more complex to maintain and more error prone. All
 unnecessary features should be removed.  For the moment we'll go with
 what's where but we may want to simplify it a lot later (perhaps when we
 move the code to C++).

 Looking at the tests, it appears that checking the options will be through
 the "confdata" structure.  This makes the code extremely dependent on the
 internals of that structure and consequently on the whole of the current
 parsing code.  The parser should be a stand-alone module with a defined
 set of functions to access the data e.g.
 * a parse(argc, argv) function to parse the data - and print an
 error/return an appropriate status.
 * a getDebug() function to return the current debug value
 * a getRate() function to return current rate

 etc.  "confdata" can then be hidden as a static variable in that module.
 Doing this will make it easier to replace the module should we so wish at
 a later date.  (This implies that a call to parse() should free all
 existing confdata data structures before starting the parsing afresh.)

 Code should conform to BIND 9 coding standards - which amongst other
 things means that "return" arguments should be enclosed in brackets, files
 should contain the standard header etc.

 '''tests/tools/perfdhcp/proconf.h'''[[BR]]
 Must have a BIND 10 header.

 Should have a header explaining what this file is for.

 Would be useful to explain what the enums are for.

 Don't understand about the config file - that was not in the design or
 requirements.  As we are not using a configuration file (and this module
 is not supposed to be a general library), code related to it should be
 removed.  It is just extra code to maintain.

 Given that debugging is now a bit mask (and so debugging is disabled with
 a value of 0), am not clear about the PDEBUG and SDEBUG elements.  These
 should be removed.

 It is usual in BIND 10 to include the function header (describing the
 arguments etc.) in the header file.  procOpts() should be so documented.

 '''tests/tools/perfdhcp/proconf.cc'''[[BR]]
 All external functions should have header comments in the .h file.  Static
 functions should have some form of header to aid maintenance.

 All references to a configuration file should be removed; there should be
 no redundant code in any of the files.

 addOptVal() (and others): Definition of local variables should be
 commented with a one-line comment.

 procCmdLineArgs(): still contains debugging code to print out the
 arguments.

 There is a reference to defread().  This is not part of BIND 10 and all
 code related to it should be removed.

 '''tests/tools/perfdhcp/cloptions.h'''
 This should have a standard ISC file header.

 The function declaration should have a header.

 '''tests/tools/perfdhcp/cloptions.cc'''
 procArgs(): the processing of the options is not clear.  Local variables
 are defined and their addresses passed to procOpts(), but are they then
 used to store the data?  If so, what happens when procArgs() returns?

 printHelp(): -x<diagnostic-selector>  If this is just for debug output, a
 simple numeric value will suffice - there is no need to complicate it with
 keywords.  Anyone requiring debugging output is going to be sufficiently
 knowledgable to create a value from a set of bits.

 '''tests/tools/perfdhcp/cloptions_unittest.cc'''[[BR]]
 Tests should not print the debugging information on failure (i.e.
 0:perfdhcp, 1:-n etc.)

 Even single-line "if" statements should use braces.

 No tests to check whether "-6" and "-4" can be put on the same command
 line.

 Should include a few tests with a combination of options to check that
 parsing one does not cause problems in parsing another.

 '''Other'''[[BR]]
 A number of files have been added that are not relevant to this ticket.
 Take a copy of them and remove them from the repository ("git rm") - they
 can be added in subsequent tickets. (This is because merging files into
 the "master" branch carries an implication that they have been reviewed -
 which they have not.)  It means that until we are ready for building
 perfdhcp, the "Makefile.am" in the perfdhcp directory will contain little
 more than "SUBDIRS = tests".

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