BIND 10 #2269: Configuration parser for DHCPv6 server

[BIND 10 Development]

#2269: Configuration parser for DHCPv6 server
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  stephen
                       Type:  task   |                Status:  reviewing
                   Priority:         |             Milestone:  Sprint-
  medium                             |  DHCP-20121018
                  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 tomek):

 * owner:  tomek => stephen


Comment:

 Replying to [comment:5 stephen]:
 > Some minor changes have been made and pushed to the repository.
 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?)
 Added a separate paragraph that explains it. It is worth noting that with
 v6 the problem is lesser than in v4 as the last address is not reserved
 for broadcast (no broadcasts in v6).

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

 > '''src/bin/dhcp6/config_parser.cc'''[[BR]]
 > If !DummyParser is for debugging, I would perhaps call it
 !Dhcp6DebugParser.
 Renamed to DebugParser. That will make the work of merging v4 and v6
 parsers easier.

 > StringParser::build(): not clear why back-slashes are removed from the
 string value passed in.
 These are not black-slashes. \t stands for tabulation. We want to allow
 using spaces and tabs in the subnet and pool definitions. Some users may
 want to add spaces or tabs for aesthetic purposes.

 > !StringParser (general): Use "!StringStorage* storage" rather than
 "!StringStorage * storage"
 Fixed 2 such problems.

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

 > PoolParser::build(): When parsing the string after the "/", the variable
 "num" is probably better called "prefixlen".  Also, the comment talks
 about downcasting to a
 Renamed to prefix_len. I think your comment was truncated. We can't use
 direct cast to uint8_t as
 that will interpret first character as value. For example for 64 it will
 return ASCII code of '6'
 and will actually throw exception because of following characters ('4' in
 this example).

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

 > Subnet6ConfigParser::build(): In the BOOST_FOREACH block, nested if's
 would prevent the repeated pointer casting, i.e.
 As configuration parsing is not performance critial, personally I prefer
 code simplicity over efficienc y in such cases. Nevertheless, changed the
 code as you suggested.

 > '''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/
 A mistake copied over from auth :) Fixed it in dhcp6 and auth.
 >
 > build(): Header refers to "above example" which is not present.
 Removed references to example.

 > 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?
 It is called once for for each object. Each object is created to parse one
 specific instance of configuration parameter. Clarified

 > createDhcp6ConfigParser(): is this more logically a static method of the
 Dhcp6ConfigParser class?
 > createDhcp6ConfigParser(): Not clear what the config_id actually is.
 It's a bit awkward, but I just noticed that this function has no body and
 is not used anywhere. Removed :)

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

 > '''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_"
 Fixed.

 Also renamed Dhcp6ConfigParser to DhcpConfigParser. That will make
 unification of Dhcp4 and Dhcp6 parsers slightly easier.

 Another thing changed is something Shawn pointed out in his #2140 review:
 tests should have comments about their purpose. For now, a simple comments
 will do, but we will want to make them more structured in the future.
 Created #2350 for that.

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