BIND 10 #2317: Setting option definitions from Configuration Manager

[BIND 10 Development]

#2317: Setting option definitions from Configuration Manager
-------------------------------------+-------------------------------------
            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 stephen):

 * owner:  stephen => marcin


Comment:

 Reviewed commit 6e2a0ba86f0607f3e9822a178a223e8a922de47a

 '''src/bin/dhcp4/config_parser.cc'''[[BR]]
 General: a number of exceptions are thrown.  I think that a message should
 be logged as well in many cases (E.g. line 808 throws a configuration
 error because option data is now valid: but from a diagnostic point of
 view, what option is at fault?)  You may want to create a separate ticket
 for that.

 OptionDataParser::build(): need to extend the header comment to include
 the fact that this also parses spaces.  (Should also explain how "csv-
 format" fits into that - is this an alternative data representation?)

 OptionDataParser::build(), line 690: would prefer a "throw" instead of an
 assert.  assert() will cause the process to die and it may be difficult to
 find out why.  Presumably somewhere there is a "catch" that will catch a
 thrown exception and print out the reason.  This applies to other uses of
 "assert()" in the code.

 Lines 1039 through 1055: s/a storage/the data store/

 !OptionDefListParser; not sure if "option_def_defaults" is the best name
 for the local storage.  AIUI, the updated configuration is stored there in
 a "build" stage, then copied to the active location in the "commit" call.
 So the intermediate store is probably best named
 "option_def_intermediate".

 OptionDefParser::createOptionDef(), Line 1086: s/typy/type/

 OptionDefParser::createOptionDef(), line 1106: s/"parameters"/"parameters:
 "/ (else the words will run into one another).

 configureDhcp4Server(), Line 1337: spurious space before a comma.

 configureDhcp4Server(), Line 1609: the comment could make the order
 clearer.  Currently it seems that:

 a) option definitions must be set before option values
 b) global option values must be set before subnet4 option values

 ... so the order seems to be option definitions, global values, subnet4
 values? Perhaps renaming the parsers will also make things clearer
 ("independent_parsers" seems poorly named).

 '''src/bin/dhcp4/dhcp4.spec'''[[BR]]
 Would "option-defs" and "definition" be better names than "option-def" and
 "single-option-def"?

 '''src/bin/dhcp6/config_parser.cc'''[[BR]]
 I've not looked at this in detail, just skimmed it: most of the comments
 for the dhcp4 parser apply here as well.

 The first ticket we do after delivery will be to merge the dhcp4 and dhcp6
 configuration parsing into a single set of code. (At the very least, the
 parsers for booleans, ints and strings could be put into libdhcpsrv.)

 '''src/lib/dhcpsrv/dhcp_config_parser.h'''[[BR]]
 getParam() does not depend on class variables (there are none), so could
 be declared static.

 Can param_id be passed by reference?

 In fact, although we can leave getParam() there for the moment, it doesn't
 really belong in the class - it's related to the storage of values and
 that functionality could be abstracted into a separate class, e.g.
 {{{
 template <typename T>
 class GeneralStorage {
 public:
         void addParam(const string name, const T value);
         T getParam(const string name);
 private:
         map<string, T> values_;
 };

 typedef GeneralStorage<bool> BooleanStorage;
 typedef GeneralStorage<string> StringStorage;
 :
 etc.
 }}}
 If you think this is a valid abstraction, open a new ticket for it - we'll
 tackle this on the clean up after delivery.

 '''src/lib/dhcpsrv/option_space_container.h'''[[BR]]
 getItems(): should note that in the header, if an option space by that
 name does not exist, a new container is created and returned, but is not
 added to the list of option spaces.  (This is a trap for the unwary
 developer - they ask for the container for an option space, get a pointer
 to the container, add items to it - which are lost as soon as then drop
 their pointer to it.)  If this is not the intention, the method should
 throw an exception if passed an option space that does not exist.
 (Contrast to addItem, which creates an option space if it did not exist
 before.)

 '''Build Failure'''[[BR]]
 I got a build failure on Ubuntu:
 {{{
   CXX    libb10_dhcpsrv_la-alloc_engine.lo
 In file included from ../../../src/lib/dhcpsrv/subnet.h:27:0,
                  from ../../../src/lib/dhcpsrv/alloc_engine.h:20,
                  from alloc_engine.cc:15:
 ../../../src/lib/dhcpsrv/option_space_container.h:69:26: error: wrong
 number of template arguments (1, should be 2)
 /usr/include/boost/detail/container_fwd.hpp:85:47: error: provided for
 'template<class T, class Allocator> struct std::list'
 ../../../src/lib/dhcpsrv/option_space_container.h: In member function 'int
 isc::dhcp::OptionSpaceContainer<ContainerType,
 ItemType>::getOptionSpaceNames()':
 ../../../src/lib/dhcpsrv/option_space_container.h:70:30: error: wrong
 number of template arguments (1, should be 2)
 /usr/include/boost/detail/container_fwd.hpp:85:47: error: provided for
 'template<class T, class Allocator> struct std::list'
 ../../../src/lib/dhcpsrv/option_space_container.h:70:37: error: invalid
 type in declaration before ';' token
 ../../../src/lib/dhcpsrv/option_space_container.h:74:19: error: request
 for member 'push_back' in 'names', which is of non-class type 'int'
 make[2]: *** [libb10_dhcpsrv_la-alloc_engine.lo] Error 1
 }}}
 The problem was fixed by adding includes of "<list>" and "<string>" to
 option_container.h: I've pushed this change.

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