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-20111123
                  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 3a330862f1357a4e0edd570de5896785029f4530

 '''tests/tools/perfdhcp/cloptions.h'''[[BR]]
 The convention used in BIND10 for function names comprising multiple words
 is to capitalise all words but the first, e.g. getRate(), getNumRequest(),
 getServer() etc.

 Would suggest "isV6()" rather than "getV6()".  "getV6()" - taken in
 conjunction with the "getXxx()" names - seems to suggest that this gets
 the value of the "-6" switch, whereas "isV6()" more accurately reflects
 whether the tool should operate in V6 mode.

 '''tests/tools/perfdhcp/cloptions.cc'''[[BR]]
 "static const char* server" should be initialized to NULL (as are the
 other static const char* variables).

 procArgs(): Regarding the version string, it would be better defined as a
 {{{const char* VERSION}}} at the top of the file instead of being buried
 half-way down it.

 initialize(): does not reset "server".

 '''tests/tools/perfdhcp/dkdebug.cc'''[[BR]]
 Should add a comment to say what dk_diag_mask is. (Also should note that
 it is initialized in dk_setup - although initializing it at the time of
 declaration does not harm.)

 '''tests/tools/perfdhcp/dkdebug.h'''[[BR]]
 As everything is being compiled using a C++ compiler, I'm not sure that
 you need the 'extern "C"' declarations - that's only for combining C and
 C++.

 '''tests/tools/perfdhcp/perfdhcp.h'''[[BR]]
 As everything is being compiled using a C++ compiler, I'm not sure that
 you need the 'extern "C"' declarations - that's only for combining C and
 C++.

 DK_ALL is defined in dkdebug.h but the remaining DK_ symbols are defined
 in perdhcp.h.  It would be logical to define them together in one place
 (probably in dkdebug.h).  Also, it is probably better to define these in
 an enum (explicitly setting values) rather than use a #define.  (This is a
 trick in  C++ to get round the problems of defining constants in places
 where they are not allowed.)

 progName[] is defined in the perfdhcp ".h" file.  This works (in C++), but
 it is probably better defined in perfdhcp.cc.

 '''tests/tools/perfdhcp/procconf.cc'''[[BR]]
 pc_malloc(): If out of memory, it would be better to print the message and
 exit rather than return a value.  An out of memory condition is fairly
 fatal and it is unlikely that the caller will do anything else than return
 to its caller.  By exiting on a memory allocation failure, we guarantee
 that pc_malloc() will return a value or not return at all - so we don't
 need to check its return value.

 pc_malloc(): Suggest use "calloc(1, amount-of-memory)" instead of
 "malloc()", as this guaratees that the memory is zeroed before return.
 (This also avoids use of memset() calls after memory is allocated.)
 Alternatively, zero the memory allocated by malloc() before returning it
 to avoid memset() calls all over the place.

 addOpt(): a case in point of the above comment is towards the end of this
 function where memory is assigned to ret_data and the return value is
 checked (and causes a return of -1 on a memory allocation fail).

 procOpts(): same comment applies to assignment of "first" and "last".

 procOpts(): In addOptVal(), the description of "last" is that it is used
 to track the last record in each option list so that option values can be
 appended easily.  This implies that "last" is a pointer into the chain of
 structs whose first element is pointed to by "first".  Yet in
 "procOpts()", "last" is initialized to point to a separate structure (and
 is then immediately deallocated after the call to procCmdLineArgs).

 '''tests/tools/perfdhcp/tests/Makefile.am'''[[BR]]
 We do not want a "-g" in the Makefile.  If you need to build the code with
 debugging enabled, you can do so with the command
 {{{
 CXXFLAGS=-g make
 }}}

 '''tests/tools/perfdhcp/tests/cloptions_unittest.cc'''[[BR]]
 When testing that a string is equal to a value, use EXPECT_STREQ instead
 of EXPECT_EQ - the former compares the value of strings, the latter
 whether the values of the pointers to the strings are equal. (See
 [http://code.google.com/p/googletest/wiki/Primer#String_Comparison here]
 for details.)


 '''tests/tools/perfdhcp/dhcp.h'''[[BR]]
 '''tests/tools/perfdhcp/dhcp6.h'''[[BR]]
 These two files should be removed for this ticket.

 '''General'''
 At some point we need to update the header documentation to use Doxygen
 tags, but that can be deferred for now.

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