BIND 10 #2269: Configuration parser for DHCPv6 server

[BIND 10 Development]

#2269: Configuration parser for DHCPv6 server
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  tomek
                       Type:  task   |                Status:  reviewing
                   Priority:         |             Milestone:  Sprint-
  medium                             |  DHCP-20121004
                  Component:         |            Resolution:
  dhcpconf                           |             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 => tomek


Comment:

 Reviewed commit 5b9ab4bc5c038d3fe6e12a0cf90c33e5f54d0f45

 Owing to the history (#2238 was merged into this ticket a couple of times
 during development), the review was conducted on the following deltas:

  * b8f538b9fcf70394e5737269d4bed660721c245d to
 fb86b2f215ca1a08440c5c200f22af7747c9de2a
  * e3cc1803c6fb2aa3373528b6095eb9cf6e8b24d1 to
 ff837bb1fc512920556a796c40cacc20407b165c
  * 680279e6e9ac7a15d23b8a4d6d03a391166a7c8b to
 5b9ab4bc5c038d3fe6e12a0cf90c33e5f54d0f45

 Some minor changes have been made and pushed to the repository.

 '''doc/guide/bind10-guide.xml'''[[BR]]
 I've made some minor corrections (typos etc.) and pushed, but this is
 really very good documentation - thank-you.

 We need to explicitly state what addresses <address>/<prefix> corresponds
 to.  (In the v4 case, we have talked about the lowest address and the
 highest address not being included in the address range.  And in this
 example, although the document talks about 2001:db8:1::/64, it mentions
 that the network administrator uses addresses 2001:db8:1::1 through
 2001:db8:1::ffff - why not use 2001:db8:1::0?)


 '''src/bin/dhcp6/dhcp6.spec'''[[BR]]
 The indentation of lines in the definition of "single-subnet6" was
 inconsistent (some lines indented a couple of spaces more than the
 preceding one).  I've modified this and pushed.

 '''src/bin/dhcp6/config_parser.cc'''[[BR]]
 If !DummyParser is for debugging, I would perhaps call it
 !Dhcp6DebugParser.

 StringParser::build(): not clear why back-slashes are removed from the
 string value passed in.

 !StringParser (general): Use "!StringStorage* storage" rather than
 "!StringStorage * storage"

 PoolParser::build(): require spaces around operators such as "+" and "-"
 in an expression.

 PoolParser::build(): When parsing the string after the "/", the variable
 "num" is probably better called "prefixlen".  Also, the comment talks
 about downcasting to a

 PoolParser::build(): Downcasting to a uint8_t won't catch any errors (such
 as prefixlen > 127 or negative).  The comment should indicate that such
 errors will be caught by the Pool6 constructor.

 Subnet6ConfigParser::build(): In the BOOST_FOREACH block, nested if's
 would prevent the repeated pointer casting, i.e.
 {{{
 if (uintParser) {
     uintParser->setStorage(...);
 } else {
     boost::shared_ptr<StringParser> stringParser = ...
     if (stringParser) {
         :
     } else {
         boost::shared_ptr<PoolParser> poolParser = ...
             :
     }
 }
 }}}


 '''src/bin/dhcp6/config_parser.h'''[[BR]]
 Need "\brief" Doxygen tag at the start of the first line of each method
 header.

 build(): s/an data/a data/

 build(): Header refers to "above example" which is not present.

 build(): method not 4expected to be called more than once: is this more
 than once in the life of the object, or more than once in the life of the
 program?

 createDhcp6ConfigParser(): is this more logically a static method of the
 Dhcp6ConfigParser class?

 createDhcp6ConfigParser(): Not clear what the config_id actually is.

 '''src/bin/dhcp6/ctrl_dhcp6_srv.cc'''[[BR]]
 dhcp6ConfigHandler: comment seems truncated ("that should never
 happen...")

 '''src/bin/dhcp6/dhcp6_messages.mes'''[[BR]]
 Minor corrections to wording have been pushed.  Incidentally, the python
 script "tools/reorder_message_file.py" will reorder the message file so
 that the message definitions are
 in alphabetical order.

 '''src/bin/dhcp6/tests/config_parser_unittest.cc'''[[BR]]
 Dhcp6ParserTest: "Dhcp6Srv * srv_" should be "Dhcp6Srv* srv_"

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