BIND 10 #2317: Setting option definitions from Configuration Manager

[BIND 10 Development]

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

 * owner:  marcin => stephen


Comment:

 Replying to [comment:9 stephen]:
 > 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.

 I created ticket the #2633 to generally improve log messages in the Config
 Managers.

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

 Added comments regarding csv-format.

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

 All of the asserts in the Confguration Parsers guard against programming
 errors. The validation of those objects is made elsewhere. I added some
 comments before asserts. I think it is a general rule followed by many
 developers to use asserts to catch programming errors and exceptions to
 catch against user errors. Also, I have seen this being followed in
 BIND10.

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

 Changed, however please note that !''a storage!'' is used in many places
 in this code. If you think that !''the data store!'' is a proper English
 it should be changed everywhere in this code and it is worth having a
 separate ticket for it.

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

 Ok. Renamed.

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

 Corrected.

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

 Corrected.


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

 Sorry. I did not catch this one... Where is this?

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

 In fact, it does not matter whether option definitions are parsed before
 or after global values. Option definitions must be parsed before global
 option values and global option values must be parsed before a subnet.

 independent_parsers may be poorly named but I don't have any better idea
 for a name.

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

 I have been following the rules that had been set by Tomek when he created
 the subnet parser. In fact the !''option-def!'' is not so bad because the
 typical use in the bindctl will be:
 {{{
 config add Dhcp6/option-def
 config set Dhcp6/option-def[0]
 }}}
 The name suggests that we are configuring a single entry. This is opposed
 to:
 {{{
 config add Dhcp6/option-defs
 config set Dhcp6/option-defs[0]
 }}}
 which suggests that you're '''adding''' multiple definitions at the same
 time.

 >
 > '''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 code is mostly copy-pasted from the DHCP4 version.

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

 Please note that I had expressed that several times that the code should
 be merged as soon as possible to avoid the need to implement new
 functionality in both places. Also, that it adds a significant amount of
 work to review the code. The decision that had been made at that time was
 to keep the code duplicated for now. I agree that this should be merged as
 the first task after the Kea release because it has already become a real
 pain to maintain it.

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

 Agreed. I have made it static.

 >
 > Can param_id be passed by reference?

 That was my idea but I accidentally missed the ampersand there.

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

 I filed the #2634.

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

 Warning added.

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

 Thanks!

 Proposed !ChangeLog entry:
 {{{
 XXX.    [func]          marcin
         DHCP option definitions can be now created using the
         Configuration Manager. The option definition specifies
         the option code, name and the types of the data being
         carried by the option.  The Configuration Manager
         reports an error on attempt to override standard DHCP
         option definition.
         (Trac #2317, git abcd)
 }}}

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