BIND 10 #2318: Setting option values from Configuration Manager

[BIND 10 Development]

#2318: Setting option values from Configuration Manager
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  marcin                             |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121101
  medium                             |            Resolution:
                  Component:  dhcp   |             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      |
-------------------------------------+-------------------------------------

Comment (by tomek):

 1. I assume that this is not the ultimate design of the option definition
 framework, but rather an intermediate solution that will eventually evolve
 into option definition framework. To introduce a new option, two things
 need to specified: option format (often called template) and concrete
 values for that option. Example:

 option aftr = code:64, format: fqdn (template definition)

 option aftr = 2001:db8:1::1 (actual values)

 The format is an abstract notation, not relating to any specific
 implementation. Note that the template definition will appear only once,
 but the values definition may appear many times (e.g. a different value
 for each subnet).

 With the code under review, it is possible to specify that option 64 have
 one format in subnet 1, but completely different in subnet 2.

 The code under review is doing both things (option format and option
 values) at the same time. If my assumption is correct that it will later
 evolve into separate template and values definitons, then it is ok. The
 only thing that should be kept is that once templates are defineable,
 values definition will look slightly different. It won't make sense to
 specify both name and option code when defining option values. From that
 perspective one - name or code - will likely to be removed from the
 existing code once template definitions are implemented.

 2. As for the redundancy between option name and option code, currently
 you define option name, but that names is not used. It would be simpler to
 use just codes for now, but mark somehow that once option templates will
 be supported, option codes will be replaced with option names. That is
 only a suggestion and it doesn't have to be implemented. Please consider
 Nov. 15 deadline and the goals for January. Depending on the goals and
 your plan, it may (or may not) make sense to implement this suggestion.

 3. You use Uint32_t container to store Uint16_t values. That is probably
 acceptable for now, but I think we will eventually need parsers for Uint16
 and Uint8. Otherwise, we'll have to repeat that awkward casts and checks
 everytime. In fact, they all could be instances of a single template.

 '''src/bin/dhcp6/config_parser.cc'''

 '''OptionDataParser ctor''': It must set options_ to NULL. Currently a
 random value is used.

 '''OptionDataParser::build()''': You have a shared pointer to base class,
 you dynamic cast it to normal pointer and then create shared pointer for
 the derived class. I'm not sure if the dynamic cast is safe. Luckily boost
 provides clean solution for that:
 {{{
 boost::dynamic_pointer_cast<StringParser>(parser);
 }}}

 See Subnet6ConfigParser::build() for example use.

 Also add a comment that setStorage() must be called before build().

 '''OptionDataParser::createOption()''': It must check if options_ is
 pointing to a defined location. Otherwise it may dereference NULL pointer.

 It does thorough checks if the option name is properly formed. Once the
 checks are passed, option_name is happily discarded. :-) Why do we need
 the option name for, anyway? It is part of the option template definition
 that is not implemented yet. In my opinion we should go with just option
 code for now that will be later replaced with option name.

 '''OptionDataListParser''': You may want to comment somewhere (e.g. in the
 ctor) that "Unless otherwise specified, parsed options will be stored in a
 global option containers (option_default). That storage location is
 overridden on a subnet basis.".

 '''Subnet6ConfigParser::build()''': Can you adjust indents, so all parser
 types are at the same indent level? Strictly speaking that violates
 indentation rules, but it is much more readable. And the number of parser
 types will increase in the future, so the code will become less and less
 readable.

 '''Subnet6ConfigParser::commit()''' (or
 '''OptionDataParser::createOption()'''): The code currently allows
 multiple definitions of the same option. That is allowed, but uncommon in
 DHCPv6. We may want to produce a warning about option being defined twice.
 I think we should allow it. We need to remember about it later when we
 will be coding option adding to Pkt6 routines.

 I'm not exactly sure how idx.equal_range() works. Will it work if there
 are more than one global options of a given code? If not, we can leave it
 as it is, but we should add a todo for it.

 '''configureDhcp6Server''': Good catch with the defaults being reused in
 the next reconfiguration!

 Review will continue in a second comment (I've experienced brief power
 outages twice today, so I prefer to split it into smaller entries).

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