BIND 10 #2637: DHCP code tidy-up for the release in January 2013

[BIND 10 Development]

#2637: DHCP code tidy-up for the release in January 2013
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:  tomek
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp          |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130122
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tomek


Comment:

 Replying to [comment:7 tomek]:
 > '''src/bin/dhcp4/config_parser.cc'''
 > createOption() verifies if the option_name contains spaces. Why this
 specific case is checked here, but all other checks are done in
 OptionSpace::validateName?

 The OptionSpace::validateName is validating option space name while the
 createOption checked the option name for emptiness. These are two
 different things being validated. In any case I added a static function
 OptionDefinition::validateName that is now used to validate option name.

 >
 > '''src/bin/dhcp4/ctrl_dhcp4_srv.cc''
 > establishSession(): Dumy => Dummy.

 Fixed.

 >
 > Please update (or remove) the comment starting with:
 > // We initially create ModuleCCSession() without configHandler

 Updated.

 >
 > The DHCP4_CONFIG_UPDATE log entry will print out full_config which
 ironically does not include full_config. merged_config should be used
 instead.

 Good catch. Fixed.

 >
 > On a related note, I think we should have a discussion with DNS team
 about how this configuration works. I'm uncomfortable with the
 getFullConfig not returning a full config. Perhaps it should be renamed to
 getComittedConfig? Or better yet it should be possible to easy obtain both
 the diff (essentially how it currently works) and the full merged config.
 Anyway, it's not something that should be done in this ticket.

 Maybe, as you say, it should be discussed elsewhere.

 >
 > '''src/bin/dhcp6/config_parser.cc'''
 > createOption() the same comment about checking against space as in its
 v4 counterpart.

 Fixed.

 >
 > '''src/lib/dhcp/option.cc'''
 > Please extend the comments a bit. Something like this will do the trick:
 >
 > {{{
 > -    // Write a header.
 > +    // Write a header (protocol dependent - 2 bytes for DHCPv4 and 4
 for DHCPv6)
 >      packHeader(buf);
 > -    // Write data.
 > +    // Write data (the same for both protocols)
 > }}}

 Extended comments as you suggested.

 >
 > '''src/lib/dhcp/option.h'''
 > Comment for pack() is no longer true. This method does not return a
 pointer anymore.

 Removed the section about returning a pointer.

 >
 > '''src/lib/dhcp/option_definition.cc'''
 > OptionDefinition::writeToBuffer()
 > There are no tests for adding FQDN record. If it is time consuming to
 add one, please add @todo in the test code instead.
 >

 I added two tests for FQDNs.

 Proposed !ChangeLog entry:
 {{{
 XXX.    [bug]           vorner
         Fixed DHCP servers configuration whereby the server did not
         receive a configuration stored in the database on its startup.
         Also, the configuration handler function now uses full
 configuration
         instead of partial to configure the server. This guarantees that
         dependencies between various configuration parameters are
         fulfilled.
 }}}

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