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

[BIND 10 Development]

#2417: Standard option definitions initialization for DHCPv6.
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  marcin                             |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121115
  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 marcin):

 * owner:  marcin => stephen


Comment:

 Replying to [comment:4 stephen]:
 > 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.)

 Comment added.

 >
 > createOption(): Also not clear why, if the number of definitions is
 zero, an option is created.  Shouldn't this be an error?
 I added the comment. In the future an error will be issued if option
 definition is not found. Due to my minimalistic approach to implement
 definitions for only those options that are critical for now, we can't
 emit error for now.
 >
 > createOption(): if getFactory() "should never return NULL", check that
 with an assert() call (include "cassert", not "assert.h").
 Added.

 >
 >
 > '''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]

 Changed.

 > 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.
 >
 Uh! I was not aware of this philosophy. But it is good that I am aware
 from now on. :-)

 > 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?

 Options are stored for subnets (in Subnet objets). There is a way to
 specify option values globally and they are inherited by subnets in case
 the particular option value is not overriden in a subnet.

 >
 > '''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).

 I removed all extra spaces in this file.

 >
 > '''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).

 Fixed.

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

 Fixed.

 >
 > 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.)
 >
 Extended comments either in the typdef header or at the index.

 > '''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.

 Comments added.

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

 They are laid in the order or actual option codes. This is intentional and
 it helps me to identify which options are already processed in the code
 and which not. I match it with options listed here
 http://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml
 for this.

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

 Added.


 ----
 Since we initialize just a limited set of option definitions I suggest
 that we postpone changelog entry until it is fully implemented.

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