Opened 7 years ago

Closed 7 years ago

#2317 closed task (complete)

Setting option definitions from Configuration Manager

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Sprint-DHCP-20130122
Component: dhcp 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

We need to create/extend the .spec file that will hold the data structure to define the option definitions. Also, we need to extend the configuration parser to read the data and create instances of option definitions.

Subtickets

Change History (12)

comment:1 Changed 7 years ago by stephen

  • Milestone changed from DHCP 2012 to Sprint-DHCP-20121018

comment:2 Changed 7 years ago by stephen

  • Milestone changed from Sprint-DHCP-20121115 to DHCP 2012

comment:3 Changed 7 years ago by stephen

  • Milestone changed from DHCP 2012 to Sprint-DHCP-20121129

comment:4 Changed 7 years ago by stephen

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

comment:5 Changed 7 years ago by stephen

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

comment:6 Changed 7 years ago by marcin

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

comment:7 Changed 7 years ago by marcin

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

The ticket is now ready for a review.

It is now possible to create option definitions for various option spaces. The dhcp4 is a default option space for DHCPv4 server and the dhcp6 is the default option space for DHCPv6. It is not allowed to override any standard option definitions within those option spaces, however it is possible to define new options within those option spaces.

With a relatively small cost I was able to implement the option value parser to be executed after option definition parser so as it is possible to define option format and its value with a single commit (in bindctl).

comment:8 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

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

  • Owner changed from stephen to marcin

Reviewed commit 6e2a0ba86f0607f3e9822a178a223e8a922de47a

src/bin/dhcp4/config_parser.cc
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
Would "option-defs" and "definition" be better names than "option-def" and "single-option-def"?

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

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

  • Owner changed from marcin to stephen

Replying to stephen:

Reviewed commit 6e2a0ba86f0607f3e9822a178a223e8a922de47a

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

comment:11 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit 7d705b35fe149cc0c6b93211f2733537b05bd905

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

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

My apologies, I gave you the wrong line number. It's at 1622, in the argument list of configureDhcp4Server.

ChangeLog
Proposed entry is OK.


All OK - after fixing the space, please merge (I don't need to see it again).

comment:12 Changed 7 years ago by marcin

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

Merged with commit 71e25eb81e58a695cf3bad465c4254b13a50696e

Note: See TracTickets for help on using tickets.