BIND 10 #2318: Setting option values from Configuration Manager

[BIND 10 Development]

#2318: Setting option values from Configuration Manager
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  marcin
  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 marcin):

 Replying to [comment:6 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)

 This ticket is just about setting option values. Option format
 specification via configuration manager will be implemented as a part of
 different ticket. For now, we will rely on the standard options and their
 definitions will be "hard coded". The OptionDefinition object basic
 implementation is already there but in order to use it to set data for
 particular standard options we first need to create instances of
 OptionDefinition objects. Again, this will be implemented as a part of
 another ticket.

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

 Yes, this is how it will work.

 >
 > 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 format is not checked here because we don't yet use OptionDefinition
 objects here. Once we do, the format consistency will be sanity-checked.

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

 User will have two options: use a string with hexadecimal digits (as it is
 now) to specify the option data or CSV format that will be more strictly
 validated against option definitions. The hexadecimal format is simpler
 and can be almost directly used without having option definitions up and
 running thus was implemented in this first step.

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

 There is a concept of "option spaces" that is present in DHCPv4 and we
 agreed to adopt something similar in Kea. Following this concept, you may
 have multiple options having the same code and belonging to different
 option spaces (I know this more refers to DHCPv4 but isn't it DHCPv6
 problem too?). In any case, the option spaces are still TBD. When they
 are, it will be possible to refer to the particular option by its name and
 then the code to access option by type is redundant. However, refering by
 type is now "more common" in Kea and this is why I think we could keep it
 and make it up to the user if he wants to use code or option name to
 specify an option he is configuring. If the code is ambiguous than error
 could be issued.

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

 Yes. I was considering it but I did not want to add more containers for
 now. I support the idea to make a template from them but I would prefer to
 make it a part of some hardening sprint.

 >
 > '''src/bin/dhcp6/config_parser.cc'''
 >
 > '''OptionDataParser ctor''': It must set options_ to NULL. Currently a
 random value is used.

 Fixed.

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

 We already discussed this on Jabber. Short summary: dynamic_cast will
 throw exception if the one type can't be cast to the other. It will also
 throw if the pointer is NULL. In this context I think it is safe and
 clean. The raw pointer that I am getting from the dynamic_cast is passed
 directly to smart pointer's constructor and starting from there the smart
 pointer takes here of pointer's disposal etc. Code has been tested and
 worked fine. I also checked with valgrind and it did not complain.

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

 Added

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

 It checks now.

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

 Yes. It is by intention. I wanted to have some basic code for option type
 and option name. It did not bring much overhead but shortly I am planning
 to implement support for option spaces and the code which refers to option
 name will be useful.

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

 Comment added.

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

 I did not like the idea of going against indentation rules. I even doubt
 if the code would have been more readable then. To keep all checks aligned
 I added new function that tries to cast to the specific parse type and if
 it is successful it will do all the required things like: setStorage(),
 build(), parsers_.push_back(). This function is in turn invoked for
 various parser types and invocations are aligned. This should be more
 readable:
 {{{
 // Try uint32 type parser.
 if (buildParser<Uint32Parser, Uint32Storage >(parser, uint32_values_,
                                               param.second)) {
     // Storage set, build invoked on the parser, proceed with
     // next configuration element.
     continue;
 }
 // Try string type parser.
 if (buildParser<StringParser, StringStorage >(parser, string_values_,
                                               param.second)) {
     continue;
 }
 // Try pools parser.
 if (buildParser<PoolParser, PoolStorage >(parser, pools_,
                                           param.second)) {
     continue;
 }
 // Try option data parser.
 if (buildParser<OptionDataListParser, OptionStorage >(parser, options_,
                                                       param.second)) {
     continue;
 }
 }}}

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

 Warning message added.

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

 I added some comment on this.

 >
 > '''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:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development