BIND 10 #2315: Extensions to option lookup mechanism

[BIND 10 Development]

#2315: Extensions to option lookup mechanism
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  task          |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp          |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130122
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => marcin


Comment:

 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.

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

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

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

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

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

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

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

 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));
 }}}

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

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