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:         |            Resolution:
  perfdhcp                           |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => johnd


Comment:

 Reviewed commit 0ad9b8c8482a134af7c47b64b412f642d08ce642

 '''tests/tools/perfdhcp/cloptions.cc'''[[BR]]
 > 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.
 The variable holding the version number should be in capitals (the
 convention is that capitalised variables are constants).

 Similarly, progName is also used a a constant, so should be PROGNAME and
 declared as "const char*". (Also, the program name is "perfdhcp", not
 "dhcpperf".)

 (Note that as these are being compiled with the C++ compiler, there is no
 need to declare them static - by default, "const" variables declared
 outside a program unit have internal linkage.)

 '''tests/tools/perfdhcp/dkdebug.cc'''[[BR]]
 dk_diag_mask should be defined as "static" - it should not be directly
 accessiible outside the module.

 '''tests/tools/perfdhcp/perfdhcp.h'''[[BR]]
 progName[] is defined as extern - why?  As it is a constant, why not
 #define it in a header file?

 '''tests/tools/perfdhcp/procconf.cc'''[[BR]]
 > 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).
 What is the reason for this?

 > I left the extern C declarations in because these files will be included
 by the packet-display code that will be added back in. This is currently
 built as C code; the plan calls for it to built a C++ when it is actually
 rewritten as C++
 This seems to imply that there is somthing in that code that is
 incompatible with g++; that just means work downstream when perfdhcp is
 converted to C++.  Why can't that code be made compatible with C++?

 > and left the pc_malloc handling of allocation failure as is, to continue
 to give callers of the option processor the opportunity to report/exit as
 they see fit.
 OK - but (a) who else is going to call the option processor, and (b) what
 can they do (other than exit) if it reports a memory allocation failure?

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