BIND 10 #2544: DHCPv4 Options Config Parser

[BIND 10 Development]

#2544: DHCPv4 Options Config Parser
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  task          |  stephen
            Priority:  high          |                       Status:
           Component:  dhcpconf      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130103
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => stephen


Comment:

 Replying to [comment:5 stephen]:
 > Reviewed commit 393c70b7a308f5d816a87cf21e017351dbf09260.
 >
 > '''src/bin/dhcp4/config_parser.cc'''[[BR]]
 > Line 469: s/than/then/

 Corrected.

 >
 > Line 474: s/util/until/

 Corrected.

 >
 > Line 621: I would suggest abstracting the name validation code into a
 separate method, perhaps in a base class common to both the V4 and V6
 parsers.

 That's good idea but I would like to postpone it until we actually have
 the common base for V4 and V6 parsers. Currently the code is simply
 duplicated and needs a lot of reorganization anyway.

 >
 > Line 641: could we consider adding a method to !LibDHCP (e.g.
 getMatchingOptionDefs())to take a code value and return the range of
 relevant options?  It would eliminate the need to introduce the concept of
 an index at this level of the code. (It could also throw the configuration
 error exception if there were multiple definitions.)

 Yes, that's the idea but I would like to defer that until we implement
 option spaces, as it will introduce a bunch of functions to search options
 and definitions.

 >
 > Line 830 onwards: using "else if" has the same effect and looks better.
 Alternatively, if the various buildParser calls could be logically ORed
 together: they will be called left to right until the first evaluates as
 true.  Doing that means only one "continue" is needed.
 >
 I combined them in a single if statement.

 > Line 850: Should the code throw an exception if it gets here?  All
 parsers have been tried but nothing is applicable.
 >
 I now throw exception if I was unable to find a matching parser but it is
 worth to note that this is a case only if programming error occurs.
 Otherwise parser should be matched.

 >
 > '''src/bin/dhcp4/dhcp4_messages.mes'''[[BR]]
 > Line 48: s/on attempt/on an attempt/
 >
 Corrected.

 > Line 50: s/yet/but/
 Corrected.
 >
 >
 > '''src/bin/dhcp4/tests/config_parser_unittest.cc'''[[BR]]
 > Starts with a blank line.
 Removed blank line.
 >
 > Line 210: Would prefer: EXPECT_NE(0, memcmp(...)).

 In fact the test was wrong. I should have had EXPECT_EQ(0, memcmp(...)). I
 used to compare non-matching buffers because the reference buffer
 comprises an option header that should have been omitted in this
 comparison. The comparison result gave non-zero value for non-matching
 buffers so the EXPECT_TRUE passed. I modified the test to omit the option
 header in this comparison and used EXPECT_EQ(0, memcmp(...)) instead of
 EXPECT_TRUE. Thankfully, the tested code appeared to be valid, because the
 fixed test passed.

 >
 > Line 244: if status is expected to be zero, check "if (status != 0)"
 >

 The comment was in a wrong place. Fixed.

 ----

 The proposed !ChangeLog entry is almost the same as I had for DHCPv6
 config parser:
 {{{
 XXX.    [func]          marcin
         Implemented DHCPv4 option values configuration using configuration
         manager. In order to set values for data fields carried by the
         particular option, user specifies the string of hexadecimal digits
         that is in turn converted to binary data and stored into option
         buffer. More user friendly way of option content specification is
         planned.
         (Trac #2544, git xyz)
 }}}

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