Opened 7 years ago

Closed 7 years ago

#2270 closed task (fixed)

Configuration parser for DHCPv4 server

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20121213
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

Ticket #2237 covers implementation of a CfgMgr?? that stores the configuration. This ticket covers actual parsing of the user configuration and translation to structures used in CfgMgr??.

Task #2269 covers similar functionality for DHCPv6.

Subtickets

Change History (9)

comment:1 Changed 7 years ago by tomek

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

comment:2 Changed 7 years ago by tomek

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

Ready for review. Please note huge overlap with v6 parser. There's a separate ticket #2355 about dealing with that.

Documentation (both user's and developer's guide) has been updated.

comment:3 Changed 7 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:4 follow-up: Changed 7 years ago by marcin

  • Owner changed from marcin to tomek

Reviewed commit 142825f0caeba214a4d1884be4067c62d1cc3551

I also fixed some minor typos in a few files and corrected the copyright header in config_parser.cc. These changes have been pushed to the branch.

General (applies to all files)
According to BIND10 coding guidelines, whenever possible we should be including project headers first, then library headers and finally system headers.
See: http://bind10.isc.org/wiki/CodingGuidelines#OrderingIncludeFiles

This is not a critical, so you may drop this for now. However it may be worth to consider applying this order in the future work.

cfgmgr.h and cfgmgr.cc
Minor issue: but we used to make files comprising ''manager'' classes with underscore. So we have lease_mgr.h, iface_mgr.h etc. This is not followed by Configuration Manager, so we have cfgmgr.h and cfgmgr.cc instead of cfg_mgr.h and cfg_mgr.cc. Is there any particular reason for this?

config_parser.h
Lack of indentation in the definition of Dhcp4ConfigError constructor.

No doxygen documentation for DhcpConfigParser? class.

I can see that there is ''todo'' comment saying that bin/dhcp4/config_parser.h and bin/dhcp6/config_parser.h will be merged. However I am suggesting that if it is going to take ''long'' it would be better to rename DhcpConfigParser? classes to something like Dhcp4ConfigParser and Dhcp6ConfigParser for DHCPv4 and DHCPv6 server respectively. If it is going to be merged soon it should be ok to leave it as is to avoid overhead of refactoring. The primary reason for renaming is that the doxygen tries to combine the documentation for DhcpConfigParser? classes and you end up having descripton of each member function duplicated.
For example: open doxygen documentation -> Files -> dhcp4/config_parser.h -> class isc::dhcp::DhcpConfigParser?. You're taken to the documentation of the class that comprises documentation of both headers while you would expect documentation of the class that is declared in DHCPv4 header.
Again, it is not ''must have'' for this review.

Why do you:

#include <string>

?

There is no reference to std::string in the header file.

config_parser.cc
Copyright header updated with the current year 2012.

I am wondering if there is a need to include all headers you're including. I did not go through all of them but as an example... You include subnet.h that itself includes:

#include <boost/shared_ptr.hpp>
#include <asiolink/io_address.h>
#include <dhcp/pool.h>
#include <dhcp/triplet.h>

Inclusion of those could have been avoided in config_parser.cc

I understand the motivation to make some class members and methods protected instead of private for unit testing. However, there are some places in the code where you don't need to do it. The following members in DebugParser? could be private with no effect on unit-testing:

    /// name of the parsed parameter
    std::string param_name_;
    /// pointer to the actual value of the parameter
    ConstElementPtr value_;

The same applies to other parser classes.

There are lots of warnings produced by doxygen about lack of reference to dhcpv4-config-inherit and dhcpv6-config-inherit:

dhcp4/config_parser.cc:145: warning: unable to resolve reference to `dhcpv4-config-inherit' for \ref command

There are also some other doxygen warnings that origin in config_parser.cc files.

I am not sure if this is enforced in any coding/documentation guidelines but it helps a lot when a list of thrown exceptions is provided along with the function documentation. For example Uint32Parser::build could have something like this included in its documentation:

\throw isc::BadValue if supplied value could not be cast to uint32_t

Treat this as a suggestion, not a ''must have''.

Is this possible that the value supplied as value into StringParser::build is empty? If so, do we want to have sanity check on this?

Constructor InterfaceListConfigParser?: I think that isc::BadValue? is more suitable when supplied value is different than expected.

PoolParser::build: I think that isc::InvalidOperation? is more suitable when pools_ member is not initialized.

Subnet4ConfigParser::build: the variables: uintParser, stringParser and poolParser should be renamed to uint_parser, string_parser and pool_parser to conform with the BIND10 coding style.

Uint32Parser::build: The lexical_cast<uint32_t> and bad_lexical_cast exception do not guard you against someone passing negative value in the configuration. In other words, if I specify:

"valid-lifetime": -4000

this will be accepted by the Uint32Parser and the valid lifetime value will be configured to 4294963296.
I suggest that given value is first cast to signed integer (maybe even int64) and appropriate sanity check is done before you cast to uint32_t. Since this is a configuration that is expected to change rarely the performance impact will be negligible.
BTW, test cases should be extended to cover this as well. I modified one of the existing tests to pass negative value and this modified test passed.

The documentation for configureDhcp4Server in config_parser.cc is redundant as it is already documented in config_parser.h. BTW, the doxygen produces warnings about the fact that you're documenting ''server'' argument that is actually omitted in implementation. This would not happen if you just remove documentation from config_parser.cc

config_parser_unittest.cc
AFAIK the parseAnswer(...) may throw exception so you might want to add something like this in all tests where you don't want it to throw:

ASSERT_NO_THROW(comment_ = parseAnswer(rcode, x))

Naming of test cases: the typical naming convention we used to follow is:

TEST_F(Dhcp4ParserTest, subnetLocal)

while you have:

TEST_F(Dhcp4ParserTest, subnet_local)

The same comment applies to all other test cases.

Dhcp4ParserTest::pool_prefix_len: You seem to have invalid comment here:

// returned value must be 1 (configuration parse error)
ASSERT_TRUE(x);
comment_ = parseAnswer(rcode_, x);
EXPECT_EQ(0, rcode_);

The comment says that you expect parse error but you actually expect rcode_ == 0.

dhcp4.dox
Why do you say: DHCPv4 server component does not use BIND10 logging yet. ?
AFAIK it does use.

Will be any changelog entry for this?

comment:5 in reply to: ↑ 4 ; follow-up: Changed 7 years ago by tomek

  • Owner changed from tomek to marcin

Replying to marcin:

Reviewed commit 142825f0caeba214a4d1884be4067c62d1cc3551

I also fixed some minor typos in a few files and corrected the copyright header in config_parser.cc. These changes have been pushed to the branch.

Thanks.

General (applies to all files)
According to BIND10 coding guidelines, whenever possible we should be including project headers first, then library headers and finally system headers.
See: http://bind10.isc.org/wiki/CodingGuidelines#OrderingIncludeFiles

This is not a critical, so you may drop this for now. However it may be worth to consider applying this order in the future work.

I hope I have fixed all of them in src/bin/dhcp4. I do not want to touch src/bin/dhcp in this ticket, as this dir was split into dhcp and dhcpsrv already, so any extra changes here will cause the merge to be more difficult.

cfgmgr.h and cfgmgr.cc
Minor issue: but we used to make files comprising ''manager'' classes with underscore. So we have lease_mgr.h, iface_mgr.h etc. This is not followed by Configuration Manager, so we have cfgmgr.h and cfgmgr.cc instead of cfg_mgr.h and cfg_mgr.cc. Is there any particular reason for this?

No. This will be fixed in a separate ticket. See #2540.

config_parser.h
Lack of indentation in the definition of Dhcp4ConfigError constructor.

Fixed.

No doxygen documentation for DhcpConfigParser? class.

Added.

I can see that there is ''todo'' comment saying that bin/dhcp4/config_parser.h and bin/dhcp6/config_parser.h will be merged. However I am suggesting that if it is going to take ''long'' it would be better to rename DhcpConfigParser? classes to something like Dhcp4ConfigParser and Dhcp6ConfigParser for DHCPv4 and DHCPv6 server respectively. If it is going to be merged soon it should be ok to leave it as is to avoid overhead of refactoring. The primary reason for renaming is that the doxygen tries to combine the documentation for DhcpConfigParser? classes and you end up having descripton of each member function duplicated.
For example: open doxygen documentation -> Files -> dhcp4/config_parser.h -> class isc::dhcp::DhcpConfigParser?. You're taken to the documentation of the class that comprises documentation of both headers while you would expect documentation of the class that is declared in DHCPv4 header.
Again, it is not ''must have'' for this review.

Renamed to Dhcp4ConfigParser. The merge is planned after January release.

Why do you:

#include <string>

?

There is no reference to std::string in the header file.

There is now. Uint32Storage and StringStorage? types are moved to header file, so the containers can be used in tests.

config_parser.cc
Copyright header updated with the current year 2012.

Thanks.

I am wondering if there is a need to include all headers you're including.

Some of the headers were removed. If there are others that can be removed, please let me know.

I understand the motivation to make some class members and methods protected instead of private for unit testing. However, there are some places in the code where you don't need to do it. The following members in DebugParser? could be private with no effect on unit-testing:

    /// name of the parsed parameter
    std::string param_name_;
    /// pointer to the actual value of the parameter
    ConstElementPtr value_;

The same applies to other parser classes.

Done.

There are lots of warnings produced by doxygen about lack of reference to dhcpv4-config-inherit and dhcpv6-config-inherit:

They are gone now. I fixed some of the errors in dhcpv6 dir, but I did not want to fix them all to avoid merging issues. Created #2540 for this.

There are also some other doxygen warnings that origin in config_parser.cc files.

All doxygen warnings from src/bin/dhcp4 directory were fixed.

I am not sure if this is enforced in any coding/documentation guidelines but it helps a lot when a list of thrown exceptions is provided along with the function documentation. For example Uint32Parser::build could have something like this included in its documentation:

\throw isc::BadValue if supplied value could not be cast to uint32_t

Treat this as a suggestion, not a ''must have''.

This is tedious work, that ideally could be automated by doxygen. In theory it could parse the code and enumerate all possible exception types. Anyway, I've added them. I hope I didn't missed anything.

Is this possible that the value supplied as value into StringParser::build is empty? If so, do we want to have sanity check on this?

Empty is a valid string. Higher level parsers should throw something if empty string is not acceptable in a given context.

Constructor InterfaceListConfigParser?: I think that isc::BadValue? is more suitable when supplied value is different than expected.

Updated.

PoolParser::build: I think that isc::InvalidOperation? is more suitable when pools_ member is not initialized.

Updated.

Subnet4ConfigParser::build: the variables: uintParser, stringParser and poolParser should be renamed to uint_parser, string_parser and pool_parser to conform with the BIND10 coding style.

Done.

Uint32Parser::build: The lexical_cast<uint32_t> and bad_lexical_cast exception do not guard you against someone passing negative value in the configuration. In other words, if I specify:

"valid-lifetime": -4000

this will be accepted by the Uint32Parser and the valid lifetime value will be configured to 4294963296.
I suggest that given value is first cast to signed integer (maybe even int64) and appropriate sanity check is done before you cast to uint32_t. Since this is a configuration that is expected to change rarely the performance impact will be negligible.

It wasn't as simple as it first looks.

BTW, test cases should be extended to cover this as well. I modified one of the existing tests to pass negative value and this modified test passed.

I've implemented new tests and actually found a bug with them. The code call insert() on uint32_defaults, but silently discards it if such a value exist already. So if a test (or a real server for that matter) call configureDhcpv4Server() only the first call will store defaults, the next one will just try and since the defaults are already there, new default values will be ignored.

This applies to both v4 and v6. Fixed that in both.

The documentation for configureDhcp4Server in config_parser.cc is redundant as it is already documented in config_parser.h. BTW, the doxygen produces warnings about the fact that you're documenting ''server'' argument that is actually omitted in implementation. This would not happen if you just remove documentation from config_parser.cc

I think I've fixed all Doxygen warnings in that directory.

config_parser_unittest.cc
AFAIK the parseAnswer(...) may throw exception so you might want to add something like this in all tests where you don't want it to throw:

ASSERT_NO_THROW(comment_ = parseAnswer(rcode, x))

Not really. If there is an unexpected exception thrown, the test fails anyway. ASSERT_NO_THROW is only useful if we want to continue the test (which is not the case here).

Naming of test cases: the typical naming convention we used to follow is:

TEST_F(Dhcp4ParserTest, subnetLocal)

while you have:

TEST_F(Dhcp4ParserTest, subnet_local)

The same comment applies to all other test cases.

Updated test names.

Dhcp4ParserTest::pool_prefix_len: You seem to have invalid comment here:
The comment says that you expect parse error but you actually expect rcode_ == 0.

Fixed.

dhcp4.dox
Why do you say: DHCPv4 server component does not use BIND10 logging yet. ?
AFAIK it does use.

It was left there for historical reasons. Removed.

Will be any changelog entry for this?

There is one now. Thanks for the thorough review.

All fixes pushed.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 7 years ago by marcin

  • Owner changed from marcin to tomek

I understand the motivation to make some class members and methods protected instead of private for unit testing. However, there are some places in the code where you don't need to do it. The following members in DebugParser? could be private with no effect on unit-testing:

    /// name of the parsed parameter
    std::string param_name_;
    /// pointer to the actual value of the parameter
    ConstElementPtr value_;

The same applies to other parser classes.

Done.

You have done it for DHCPv6 only.


Uint32Parser::build: The lexical_cast<uint32_t> and bad_lexical_cast exception do not guard you against someone passing negative value in the configuration. In other words, if I specify:

"valid-lifetime": -4000

this will be accepted by the Uint32Parser and the valid lifetime value will be configured to 4294963296.
I suggest that given value is first cast to signed integer (maybe even int64) and appropriate sanity check is done before you cast to uint32_t. Since this is a configuration that is expected to change rarely the performance impact will be negligible.

It wasn't as simple as it first looks.

Instead of using C-style UINT32_MAX macro you could have used the numeric_limits trait class for integers. So, in Uint32Parser::build you would have:

if (check > std::numeric_limits<uint32_t>::max()) {
    isc_throw(BadValue, "Value " << value->str() << "is too large"
              << " for unsigned 32-bit integer.");
}

This is a C++ style approach and does not require things like (line 15, config_parser.cc):

// We want UINT32_MAX macro to be defined in stdint.h
#define __STDC_LIMIT_MACROS

BTW, test cases should be extended to cover this as well. I modified one of the existing tests to pass negative value and this modified test passed.

I've implemented new tests and actually found a bug with them. The code call insert() on uint32_defaults, but silently discards it if such a value exist already. So if a test (or a real server for that matter) call configureDhcpv4Server() only the first call will store defaults, the next one will just try and since the defaults are already there, new default values will be ignored.

This applies to both v4 and v6. Fixed that in both.

FYI... I had discovered this issue in V6 config parser some time ago and fixed it. This means that since you have done similar change you are going to have merge conflicts in V6.

The documentation for configureDhcp4Server in config_parser.cc is redundant as it is already documented in config_parser.h. BTW, the doxygen produces warnings about the fact that you're documenting ''server'' argument that is actually omitted in implementation. This would not happen if you just remove documentation from config_parser.cc

I think I've fixed all Doxygen warnings in that directory.

This will not show up as a warning but rather a duplicated documentation in the html. You have two @brief tags (one in config_parser.h, another in config_parser.cc) that describe the same function: configureDhcp4Server. The html output for this function looks like this:

configures DHCPv4 server

Configure DHCPv4 server (Dhcpv4Srv) with a set of configuration values.

...

I suggest that @brief tag is removed from .cc file. If you want to keep some description of this function in a .cc file anyway you could make it non-doxygen documentation by removing one slash.

Naming of test cases: the typical naming convention we used to follow is:

TEST_F(Dhcp4ParserTest, subnetLocal)

while you have:

TEST_F(Dhcp4ParserTest, subnet_local)

The same comment applies to all other test cases.

Updated test names.

You still have bugus_command and subnet_global_defaults.

Will be any changelog entry for this?

There is one now.

Changelog looks good.


Additional comments after the second review:
config_parser.cc
You now clear the global storage for uint32 and string values every time the configureDhcp4Server is called. I already had similar idea for DHCPv6 and had to back off changes because this causes problems when multiple config requests are issued. Note that the configuration string is partial so if you clear the storage you remove the configuration that you had entered earlier and only the recently configured parameter is up-to-date. I suggest that containers are not cleared but their values are rather overriden in a map with [] operator.

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

  • Owner changed from tomek to marcin

Replying to marcin:

I understand the motivation to make some class members and methods protected instead of private for unit testing. However, there are some places in the code where you don't need to do it. The following members in DebugParser? could be private with no effect on unit-testing:

You have done it for DHCPv6 only.

Uh oh. Ok, it is fixed now. Protected access is no longer used in in config_parser anymore in either dhcp4 nor dhcp6.

Instead of using C-style UINT32_MAX macro you could have used the numeric_limits trait class for integers. So, in Uint32Parser::build you would have:

if (check > std::numeric_limits<uint32_t>::max()) {
    isc_throw(BadValue, "Value " << value->str() << "is too large"
              << " for unsigned 32-bit integer.");
}

I didn't know that. Macro removed and the code now uses numeric_limits class.

I've implemented new tests and actually found a bug with them. The code call insert() on uint32_defaults, but silently discards it if such a value exist already. So if a test (or a real server for that matter) call configureDhcpv4Server() only the first call will store defaults, the next one will just try and since the defaults are already there, new default values will be ignored.

This applies to both v4 and v6. Fixed that in both.

FYI... I had discovered this issue in V6 config parser some time ago and fixed it. This means that since you have done similar change you are going to have merge conflicts in V6.

Ok. Removed that clear. The code now uses [] operator, so it is no longer necessary.

I fixed this in dhcp4 only to avoid merging problems in v6 code.

I suggest that @brief tag is removed from .cc file. If you want to keep some description of this function in a .cc file anyway you could make it non-doxygen documentation by removing one slash.

Ok, removed.

You still have bugus_command and subnet_global_defaults.

Not anymore.

Additional comments after the second review:
config_parser.cc
You now clear the global storage for uint32 and string values every time the configureDhcp4Server is called. I already had similar idea for DHCPv6 and had to back off changes because this causes problems when multiple config requests are issued. Note that the configuration string is partial so if you clear the storage you remove the configuration that you had entered earlier and only the recently configured parameter is up-to-date. I suggest that containers are not cleared but their values are rather overriden in a map with [] operator.

Good point. I've updated both dhcp4 and dhcp6, even it that means extra work with merging.

Ok, I hope I haven't missed anything this time.

comment:8 Changed 7 years ago by marcin

  • Owner changed from marcin to tomek

config_parser.cc
Order of header files...: Stephen recently reordered headers in all (or most of the) files. In the config_parser.cc boost includes are still before project includes. I think it may be good idea to reorder them to be consistent with the rest of the DHCP code.

namespace: Since the parser classes are not meant to be called from an outside world they could be stored in unnamed namespace rather than isc::dhcp. I admit that this would require a lot of refactoring in the present code so it may be deferred for now. Maybe until we merge the V4 and V6 config parsers.

Apart from this it is ok and can be merged (I don't have to see the code if you just reorder headers).

comment:9 Changed 7 years ago by tomek

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

Headers reordered. Code merged.

Note: See TracTickets for help on using tickets.