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      |
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => marcin


Comment:

 Replying to [comment:8 marcin]:
 > 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.
 Ok.

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

 > 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.
 True. While we need to support option spaces eventually, most use cases
 will cover adding another "regular" DHCPv6 option. Obviously we don't need
 to make any specific choices right now, so this was a just a related
 discussion point and it shouldn't hold this ticket.

 > > 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.
 Can you add a @todo in the code and a new ticket for it?

 > 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.
 Agree. Sorry for the confusion. I got the (false) impression that the
 factory method returns shared pointer, and was concerned the the code
 casts shared pointer to a regular pointer and then assigns it to the
 second shared pointer. That is not the case and the code is safe as it is.

 > > 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.
 A simple comment that explains that the name is currently not used yet,
 but it is going to be soon will do the trick.

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

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

 Finally, make sure that you test that the code is accepting configuration
 specified using bindctl. You will want to do it anyway to document how the
 options can be specified by the user. Make sure to update BIND10 Guide. I
 think there is a separate ticket planned for this, right? If not, you can
 do it in this ticket or as a separate ticket. In that separate ticket,
 please add a note that dhcp6.dox should be updated to explain option
 parser general ideas.

 Once you add aforementioned comments and create the ticket, the code will
 be ready to merge. I do not need to see it again.

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