BIND 10 #2417: Standard option definitions initialization for DHCPv6.

[BIND 10 Development]

#2417: Standard option definitions initialization for DHCPv6.
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  marcin
  marcin                             |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121101
  medium                             |            Resolution:
                  Component:  dhcp6  |             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:  UnAssigned => marcin


Comment:

 Reviewed commit 2c70372efd0b0ddb8059d5bbe9eb195f024d4df8

 '''src/bin/dhcp6/config_parser.cc'''[[BR]]
 createOption(): Need a comment before the block of close starting with the
 call to getOptionDefs. (I'd also suggest adding a comment to the preceding
 code block, the one that gets the string parameter "data": it's not clear
 why the data should be a string of hexadecimal digits.)

 createOption(): Also not clear why, if the number of definitions is zero,
 an option is created.  Shouldn't this be an error?

 createOption(): if getFactory() "should never return NULL", check that
 with an assert() call (include "cassert", not "assert.h").


 '''src/bin/dhcp6/dhcp6_messages.mes'''[[BR]]
 Suggest replacing this message with two messages:
 {{{
 DHCP6_NO_SUBNET_DEF_OPT failed to find subnet for address %1 when adding
 default options
 This warning message indicates that when attempting to add default options
 to a response,
 the server found that it was not configured to support the subnet from
 which the DHCPv6
 request was received.  The packet has been ignored.

 DHCP6_NO_SUBNET_REQ_OPT failed to find subnet for address %1 when adding
 requested options
 This warning message indicates that when attempting to add requested
 options to a response,
 the server found that it was not configured to support the subnet from
 which the DHCPv6
 request was received.  The packet has been ignored.
 }}}

 (See below for reason.)


 '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 DHCP6_NO_SUBNET_FOR_ADDRESS: two places where this message is issued,
 which goes against the philospohy of the BIND 10 messaging system.
 Suggest we have two separarate message (listed above) for the two places
 where the warning is issued.

 append{Default,Requested}Options(): presumably it is only because this is
 an early implementation that options are associated with the subnet.  Are
 there global options?

 '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''[[BR]]
 solicitBasic test: avoid having spaces after an opening parenthesis and
 before a closing one (this appears in a number of gtest ASSERT_xxx and
 EXPECT_xxx).

 '''src/lib/dhcp/libdhcp++.cc'''[[BR]]
 initStdOptionDefs6(): In the "switch" statement, the D60_STATUS_CODE case
 should end with a "break".  Also, the "default:" does not need a "break" -
 it can be empty (although add a comment to note that the default case has
 been explicitly considered and that no default processing is needed).

 '''src/lib/dhcp/option_definition.h'''[[BR]]
 typo: "oreder"

 Suggest that the header for the !OptionDefContainer typedef is extended to
 clarify that there can be multiple elements for a given option code index.
 (It's clear once you note that a hashed_non-unique index is used, but
 making it explicitly clear wouldn't hurt.)

 '''src/lib/dhcp/tests/libdhcp++_unittest.cc'''[[BR]]
 testInitOptionDefs6: That's quite a block of solid code: would prefer a
 few blank lines and a brief comment indicating what is being checked and
 why.

 initStdOptionDefs: trivial point - why not layout the tests in
 alphabetical order of the option code?

 '''src/lib/dhcp/tests/option_definition_unittest.cc'''[[BR]]
 factoryBinary test: comments explaining what is being tested (and why)
 would be useful.

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