Opened 7 years ago

Closed 7 years ago

#2634 closed enhancement (fixed)

The DhcpConfigParser::getParam should be abstracted to a separate class.

Reported by: marcin Owned by: tmark
Priority: low Milestone: Sprint-DHCP-20130411
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

As pointed out in the review of #2317 the DhcpConfigParser::getParam should be abstracted to a separate class, similar to this:

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

Subtickets

Attachments (4)

dhcpsrv.test-suite.log (13.8 KB) - added by tmark 7 years ago.
dhcp6.test-suite.log (275 bytes) - added by tmark 7 years ago.
dhcp4.test-suite.log (275 bytes) - added by tmark 7 years ago.
bindctl.txt (1.6 KB) - added by tmark 7 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 7 years ago by stephen

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

Changed 7 years ago by tmark

Changed 7 years ago by tmark

Changed 7 years ago by tmark

Changed 7 years ago by tmark

comment:2 Changed 7 years ago by tmark

  • Owner set to UnAssigned
  • Status changed from new to reviewing

Replaced the static template method, getParam, with a new template class:

ValueStorarge?<typename ValueType?>

which provides the following methods:

void setParam(const std::string name, const ValueType? value);
ValueType? getParam(const std::string& name);
void delParam(const std::string name);
void clear();

in lib/dhcpsrc/dhcp_config_parser.h. This class replaces the use of std:map
in the typedefs for BooleanStorage?, Uint32Storage, and StringStorage?. These
typedefs are now defined in dhcp_config_parser.h.

Unit tests were added to dhcpsrv/tests/cfgmgr_tests.cc to verify proper functioning
of the new stores.

Modified bin/dhcp4, bin/dhcp6, and unit tests to use the new forms of storage.

All unit tests pass (see attached test logs except two tests in dhcpsrv due to
unrelated issue trac# 2837).

Also attached binctl output demonstrating the ability to load/view configuration for both
dhcp4 and dhcp6.

While the code for BooleanParser?,Uint32Parser,and StringParser? which all uses the
new class, ValueStore?, is virtually identical in dhcp4 and dhcp6, refactoring these

classes will be done under 2355.

comment:3 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:4 Changed 7 years ago by stephen

  • Owner changed from stephen to tmark

Reviewed commit ff8f84b695de0e5350af325c675ab2bdbab79cc0

General
There is no need to attach the output of unit tests to the ticket.

src/bin/dhcp4/config_parser.cc
Why did one of the changes move the include of dhcp4/config_parser.h lower down the list of includes (originally the include files were in alphabetical order)?

The include of dhcpsrv/dhcp_config_parser.h is commented out.

createSubnet - please start comments with a capital letter ("Rethrow with precise error").

src/bin/dhcp4/tests/config_parser_unittest.cc
Dhcp4ParserTest::checkGlobalUint32: commented-out declarations have been left in. They should be removed.

src/lib/dhcpsrv/dhcp_config_parser.h
The file "stdint.h" is included. The C++ version of this is "cstdint"

Typo in header of ValueStorage class - "@tparam".

setParam - typos in header: "parmater", "paramater". (Second typo also occurs in the description of the return value in getParam.)

setParam takes its parameters by value. If these are complex objects (like strings), that will cause a local variable to be created via a copy constructor then deleted. I suggest that they are passed by reference.

Suggest adding a blank line after the end of setParam to separate it from the next method. (This is done elsewhere.)

getParam - text in exception should start with a capital letter ("Missing parameter").

getParam - is there a need for the intermediate statement setting "value"? return (param->second) seems reasonably clear.

delParam - pass parameter by reference?

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
(All comments apply to the new tests)

Please start comments with capital letters.

What is the "them" in the comments?

The tests should also check:

  • Trying to get something that never existed in the storage throws an exception. (The current test only checks that getting something that has been deleted throws an exception.)
  • The expected behaviour of deleting something that does not exist.

The failure of deleting one of the parameters can be done via an EXPECT_THROW. If that test fails, the subsequent tests can still be carried out. (The same goes for the test after emptying the list.)

I'm surprised it's not caught by the compiler, but in the testing of Uint32Storage, one of the test value is negative.

comment:5 Changed 7 years ago by tmark

  • Owner changed from tmark to stephen

Replying to stephen:

Reviewed commit ff8f84b695de0e5350af325c675ab2bdbab79cc0

General
There is no need to attach the output of unit tests to the ticket.

Duly noted. Will exclude from future tickets.

src/bin/dhcp4/config_parser.cc
Why did one of the changes move the include of dhcp4/config_parser.h lower down the list of includes (originally the include files were in alphabetical order)?

The include of dhcpsrv/dhcp_config_parser.h is commented out.

Didn't mean to commit this change.

createSubnet - please start comments with a capital letter ("Rethrow with precise error").

Done.

src/bin/dhcp4/tests/config_parser_unittest.cc
Dhcp4ParserTest::checkGlobalUint32: commented-out declarations have been left in. They should be removed.

Done. I need to be more diligent with my pre-commit "review".

src/lib/dhcpsrv/dhcp_config_parser.h
The file "stdint.h" is included. The C++ version of this is "cstdint"

Here, I was following suit. "stdint.h" is included all over our source tree. I attempted to use <cstdint> but it does not compile cleanly under OS-X (not found) or Debian:

"/usr/include/c++/4.4/c++0x_warning.h:31:2: error: #error This file requires compiler and library support for the upcoming ISO C++ standard, C++0x. This support is currently experimental, and must be enabled with the -std=c++0x or -std=gnu++0x compiler options."

To use it would require build option changes. I recommend leaving it at stdint.h.

Typo in header of ValueStorage class - "@tparam".

setParam - typos in header: "parmater", "paramater". (Second typo also occurs in the description of the retur

n value in getParam.)

Done.

setParam takes its parameters by value. If these are complex objects (like strings), that will cause a local

variable to be created via a copy constructor then deleted. I suggest that they are passed by reference.

Done.

Suggest adding a blank line after the end of setParam to separate it from the next method. (This is done else

where.)

getParam - text in exception should start with a capital letter ("Missing parameter").

getParam - is there a need for the intermediate statement setting "value"? return (param->second) seem

s reasonably clear.
Done.

delParam - pass parameter by reference?

Done.

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
(All comments apply to the new tests)

Please start comments with capital letters.

What is the "them" in the comments?

Done.

The tests should also check:

  • Trying to get something that never existed in the storage throws an exception. (The current test only checks that getting something that has been deleted throws an exception.)
  • The expected behaviour of deleting something that does not exist.

The failure of deleting one of the parameters can be done via an EXPECT_THROW. If that test fails, the subsequent tests can still be carried out. (The same goes for the test after emptying the list.)

Adding cases to:

  1. Verify that asking for something that never existed throws and exception.
  2. Verify that deleteing something that never existed does NOT throw.

For all three storage tests.

I'm surprised it's not caught by the compiler, but in the testing of Uint32Storage, one of the test value is negative.

Changed negative test value to positive.

comment:6 Changed 7 years ago by tmark

Replaced ASSERTs with EXPECTs in storage unit tests.

comment:7 Changed 7 years ago by stephen

  • Owner changed from stephen to tmark

Reviewed commit 3f3bc96b8f17677c90dc7cb049a36666164ef908.

All OK, please merge. This needs a ChangeLog entry though.

comment:8 Changed 7 years ago by tmark

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

After brief discussion it was agreed that this doesn't affect outward behavior and scope is small enough to not warrant a Changelog entry.

All changes merged.

Note: See TracTickets for help on using tickets.