BIND 10 #2315: Extensions to option lookup mechanism

[BIND 10 Development]

#2315: Extensions to option lookup mechanism
-------------------------------------+-------------------------------------
            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:5 tomek]:
 > That's a very good piece of code. It seems that the use of option spaces
 will be easy to use.
 > I haven't found much to point out in the review.

 It only looks like that but returning a pointer instead of const reference
 to the container was pretty extensive effort.

 >
 > '''src/lib/dhcpsrv/cfgmgr.cc|h'''
 > addOptionDef(): Can you extract option_space validation into separate
 method?

 This is a part of #2313. With this ticket I have implemented static
 function that validates option space name. I could directly use this
 function here once the ticket is merged.

 > For now it would be a simple check is the string is empty, but later we
 may develop a dictionary that will keep list of defined option_spaces.

 Agreed. This is planned with #2313.

 >
 > Why it is not possible to use addOptionDef() to add custom option
 definitions to dhcp4 and dhcp6 option spaces?

 Good catch. I modified the check so as you can add option definition to
 dhcp4 and dhcp6 option spaces with this restriction that the new option
 definition must not override a standard option definition.

 >
 > getOptionDefs(): Can you use that new method here?

 For now the check is not so important because the method will not find any
 definition if the option space name is invalid. When #2313 is merged, I am
 going to use the validation function implemented there.

 >
 > This method returns a reference to OptionDefContainer. How long the
 reference is valid? It would safer to return shared_ptr.

 You're right. I used to return a reference to the container that I stored
 within a class. Once the container became a part of the std::map I started
 to return the reference to the value of the map and that caused the risk
 that the reference invalidates when the map is modified (e.g. elements are
 removed). As you suggested, I now return pointer to the container for a
 particular option space.

 >
 > Can you add a type for std::map<std::string, OptionContainer>?

 Added.

 >
 > '''src/lib/dhcpsrv/subnet.h'''
 > Can getOptionSingle be renamed to getOptionDescriptor()? It may be
 confusing. My first (wrong) impression was that it returns a single
 option, which is not true.

 Ok. I renamed that function. Also I renamed getOptions to
 getOptionDescriptors.

 >
 > '''src/lib/dhcpsrv/tests/cfgmgr_unittest.cc'''
 > getOptionDef() or getOptionDefs(): should check that option 105 added to
 namespace "isc" is really different than option 105 from "abcde".

 I added a check for option names which are different for different option
 spaces.

 >
 > You should extend getOptionDef() to see that requesting non-existing
 option from existing option space will return NULL. Something like this
 will do the trick:
 >
 > {{{
 > EXPECT_FALSE(cfg_mgr.getOptionDef("isc", 56));
 > }}}
 >

 Added.

 > addOptionDefNegative: Try to add the same (valid) option twice and check
 that it fails the second time.

 Added.


 ----

 Proposed !ChangeLog entry

 {{{
 XYZ.    [func]          marcin
         Added routines to search for configured options and their
         definitions using name of the option space they belong to.
         (Trac #2315, git xyz)
 }}}

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