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          |  stephen
            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 marcin):

 * owner:  marcin => stephen


Comment:

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

 I fully agree but this discussion has been carried for some time already.
 I didn't want to make revolution at this point but since parsers are
 private to .cc files for now, they should be in anonymous namespace and
 this is what the change was about.

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

 Renamed.

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

 I added additional checks in constructors.

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

 Please note that this code is has not been produced as a part of this
 ticket. Nevertheless, I added some comments in places you indicated.

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

 Fixed.

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

 Yes. Fixed.

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

 > Line 1453: same comment as above.
 Fixed.

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

 Currently, the comma is used as a separator. Space separator would put you
 into trouble when setting string values. In such case you would have to
 put string values in the  double quotes or so to differentiate spaces in
 the string value and spaces separating values from each other.

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

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

 OK. I created a common header.

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

 Fixed.

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

 Yes. Removed.

 Proposed !ChangeLog entry:

 {{{
 XXX.    [func]          marcin
         DHCP Option values can be now specified using a string of
         tokens separated with comma sign. Subsequent tokens are used
         to set values for corresponding data fields in a particular
         DHCP option. The format of the token matches the data type
         of the corresponding option field: e.g. "192.168.2.1" for IPv4
         address, "5" for integer value etc.
         (Trac #2545, git abcd)
 }}}

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