BIND 10 #1265: DHCP benchmarking utility: send/receive DHCP packets

[BIND 10 Development]

#1265: DHCP benchmarking utility: send/receive DHCP packets
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  johnd
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:  major  |  DHCP-20111123
                  Component:         |            Resolution:
  perfdhcp                           |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DHCP
            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 809dc75718908b39668046c2d3db8bfb51947ecc

 '''General'''[[BR]]
 Once #1324 is committed, you should merge the master branch into this
 branch to pick up the final version of the command line processor.

 Question about the name of the files perfdhcp.* - "perfdhcp" is the name
 of the utility.  These functions are solely related to the I/O.  Unless
 everything else (e.g. as statistics accumulation and output) is going to
 go into this file - which would be a bad idea - I suggest the files be
 renamed to something like dhcp_io.*

 '''tests/tools/perfdhcp/perfdhcp.h'''[[BR]]
 Some argument names have the same format (lowercase first word,
 Capitalised second word) as the function names.  Others are lower-case
 words separated by underscores.  They should be consistent and be
 lowercase.  There is no firm guide but typically full words are separated
 by underscores (e.g. send_port) whereas others - usually if they include
 abbreviations - may be concatendated together (e.g. msgsize).

 The same rules for argument names apply to local variables (i.e. lower-
 case).

 In the function headers, remove the "globals" section - that is a detail
 of the implementation and not of interest to any caller of the function.
 (Also, they aren't global variables - they have file-limited scope).

 Remember that you need to remove the #define when this is merged with the
 code in #1324.

 It would be worth learning Doxygen to comment your headers. Only a few
 tags are needed e.g.
 {{{
 /**
  * Send a DHCP packet.
  *
  * \param dhcp_packet DHCP message to send.
  * \param num_octets Size of message.
  *
  * \return 1 on success.  If the send fails (in full or part), an error
  *         message is printed to stderr and 0 is returned.
  */
 }}}

 Typo in header of dhcpReceive - "reeive".

 '''tests/tools/perfdhcp/perfdhcp.cc'''[[BR]]
 This should be a .c file.

 Would prefer that the declarations of the static variables are one per
 line.

 If you put the main dhcp* functions after the static helper functions in
 the file, you don't need to declare the latter.

 All error messages include the program name and output text of the form
 {{{
 <program-name>: <error-message>
 }}}
 Why not create a "reporterr()" function (in a separate module, with its
 own header file) that eliminates the need to have PROGNAME scattered all
 over the place, e.g.
 {{{
 void reporterr(const char* format, ...) {
     va_list ap;

     va_start(ap, format);
     fprintf(stderr, "%s: ", PROGNAME);
     vfprintf(stderr, format, ap);
     fprintf(stderr, "\n");
     va_end(ap);
 }
 }}}

 If there is a function header in the .h file, there is no need to repeat
 it in the .c file.  A short one or two-line summary of what it does will
 be enough.


 dhcpSetup(): The variable v6 is declared as equal to the result of isV6().
 It is used in the next line then ignored as the following test uses isV6()
 directly.  It might as well be removed.

 dhcpSetup(): What happens if "isV6()" is true and localAddr is null? NULL
 is then passed to socketSetup - is this OK? (If so, a comment is needed to
 explain why.)

 dhcpReceive(): The "if" test for msgsize equal to zero should use braces,
 even though there is a single statement.

 socketSetup(): If the address family is V6 and the local address is null,
 it is not clear what the resulting memcpy and assignments are going - a
 comment would be helpful.

 socketSetup(): at the point where a diagnostic message is printed
 ("creating socket from addrinfo"), the address information is printed
 twice.

 getAddrs header: strictly speaking, PROGNAME is not a global, it's a
 defined symbol here.

 getAdxdrs header: can "hostname" also be an IPV6 address?

 getAddrs(): why not call it getAddress() - it's only two characters more.


 '''tests/tools/perfdhcp/tests/sendreceive_unittest.cc'''[[BR]]
 I would not use "do" as the name of a test (it is a reserved word).

 Add a comment to the effect that "procArgs" is called to set up the
 desired options.

 There should be two tests - one for IPv4 and one for IPv6.

 The receive buffer should be much larger than the message sent.
 recvfrom() can truncate a returned message to fit the buffer; if the
 buffer size is equal to the size of the message sent, you don't know
 whether some additional data has been sent but was then truncated.

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