BIND 10 #2545: Option value to be specified in in CSV format in Config Parser

[BIND 10 Development]

#2545: Option value to be specified in in CSV format in Config Parser
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  task          |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcpconf      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130103
         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 28a43fa59ffd4f772104a76e3636b8c54b329aa3

 '''src/bin/dhcp4/config_parser.{cc,h}'''[[BR]]
 OK for now.  However, at some time the various parsers need to be moved to
 dhcpsrv.  A lot of the parser code is common and it seems sensible to have
 the common code in one place.

 General: the factory function (for all parsers) should be "factory()" and
 not "Factory()".

 If the parameter name should not be empty, the best place to check that is
 in the constructor, where it is set. (The check in "build()" could be
 left.  (As a note for the future, "param_name_" could be an attribute of
 the base class, accessed by a getParamName() method - the check for
 param_name being non-null could be in the base class.)

 Subnet4ConfigParser::createSubnet() - would appreciate some more comments
 in the early part of this method (e.g. why the erase_all, why the
 searching for "/").  Also, comments around the calls to getParam()
 explaining, for example, what if no parameters are set?

 Function configureDhcp4Server:[[BR]]
 * line 1418/9 - s/accesing global storages/accessing global storage/
 * line 1421 - s/immediatelly/immediately/
 * line 1422/3 - s/global storage have been already modified/global storage
 has been modified/

 Line 1437: should the comment refer to "all but subnet4"?

 Line 1440: In the BOOST_FOREACH, the creation of "parser" should be moved
 to be within the "if" test.  Otherwise, for some config_pairs, a parser is
 created but never used.

 Line 1453: same comment as above.

 Question on csv-format: do the values need to be separated by commands, or
 will simple space separation do? (The example in the stdOptionData test
 gives a value field of "192.0.2.10, 192.0.2.1, 192.0.2.3").  I can't help
 thinking that it is likely that a user will omit commas.


 '''src/bin/dhcp4/tests/config_parser_unittest.cc'''[[BR]]
 createConfigWithOption: header documentation needs to include the
 parameter "csv-format".

 '''src/bin/dhcp6/config_parser.cc'''[[BR]]
 Although the move to make the parser code common is a separate ticket, as
 you are moving the definition of !DhcpConfigParser, why not pull out the
 abstract class !DhcpConfigParser into a header file in dhcpsrv? The base
 class definition is the same for both DHCP6 and DHCP4 parsers.

 configureDhcp6Server: same commands as for dhcp4/config_parser concerning
 the creation of the parser object before the "if" test.

 '''src/bin/dhcp6/tests/config_parser_unittest.cc'''[[BR]]
 Line 723: is this a debug statement?

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