Opened 7 years ago

Closed 7 years ago

#2545 closed task (complete)

Option value to be specified in in CSV format in Config Parser

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Sprint-DHCP-20130122
Component: ~dhcpconf(obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Currently the option values can be specified as string of hexadecimal digits. This functionality should remain but we need an optional way to specify option values as a string of comma separated values. Each value refers to a particular data field within an option. This is more convenient and more readable format.

This refers to either DHCPv4 or DHCPv6.

Subtickets

Change History (9)

comment:1 Changed 7 years ago by stephen

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20130103

comment:2 Changed 7 years ago by marcin

  • Owner set to marcin
  • Status changed from new to accepted

comment:3 Changed 7 years ago by marcin

  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to assigned

The ticket is now ready for review. The new configuration parameter has been added to the spec file. This parameter accept boolean value that controls whether option data is entered as a string of comma separated values (where each value initializes one option field) or as a string of hexadecimal digits that represent the binary content of a whole option.

I also made a couple of changes to the existing configuration parsers:

  • more clear separation of the build() phase from the commit() phase - the latter should simply apply parsed values to the server's configuration and should be exception safe as much as possible
  • moved the parser classes to an anonymous namespace as they are not intended to be accessed externally

Changes applied to v4 and V6 parsers.

Please review.

comment:4 Changed 7 years ago by marcin

  • Status changed from assigned to reviewing

comment:5 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:6 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit 28a43fa59ffd4f772104a76e3636b8c54b329aa3

src/bin/dhcp4/config_parser.{cc,h}
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:

  • 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
createConfigWithOption: header documentation needs to include the parameter "csv-format".

src/bin/dhcp6/config_parser.cc
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
Line 723: is this a debug statement?

comment:7 in reply to: ↑ 6 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

Replying to stephen:

Reviewed commit 28a43fa59ffd4f772104a76e3636b8c54b329aa3

src/bin/dhcp4/config_parser.{cc,h}
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:

  • 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
createConfigWithOption: header documentation needs to include the parameter "csv-format".

Added.

src/bin/dhcp6/config_parser.cc
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
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)

comment:8 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

All OK, please merge.

comment:9 Changed 7 years ago by marcin

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged to master with commit 792c129a0785c73dd28fd96a8f1439fe6534a3f1.

Note: See TracTickets for help on using tickets.